Re: [edk2-devel] [PATCH v3] MdeModulePkg: AllocatePages for TranslateBmpToGopBlt

["Mike Maslenkin" <mike.maslenkin@gmail.com>]

Very nice, that functional changes were separated.
But, as far as I remember, Pedro noticed that AllocatePages() takes
number of pages, not a bytes.
So, the change could be as:
  *GopBlt     = AllocatePages (EFI_SIZE_TO_PAGES(*GopBltSize));

But I I would like you to pay special attention that this change
breaks a previous contract.
AllocatePages() requires FreePages() to free memory, and currently all
consumers of TranslateBmpToGopBlt()
expect memory was allocated in pool and call FreePool() to release this buffer:

https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c#L439
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Application/CapsuleApp/CapsuleApp.c#L96
https://github.com/tianocore/edk2-platforms/blob/master/Platform/Intel/TigerlakeOpenBoardPkg/FspWrapper/Library/PeiFspPolicyInitLib/PeiFspSaPolicyInitLib.c#L176

So why don't you want to allocate enough memory and pass this pointer
to TranslateBmpToGopBlt().
As I can see there are some places where it has been done in that way
(thus I was not able to find where buffer is released) :

https://github.com/tianocore/edk2-platforms/blob/master/Platform/Intel/WhiskeylakeOpenBoardPkg/Policy/Library/PeiPolicyUpdateLib/PeiSaPolicyUpdate.c#L133
https://github.com/tianocore/edk2-platforms/blob/master/Platform/Intel/CometlakeOpenBoardPkg/Policy/Library/PeiPolicyUpdateLib/PeiSaPolicyUpdate.c#L133

On Fri, Aug 4, 2023 at 9:08 PM chitralekha ck <chitralekha.ck@intel.com> wrote:
>
> https://bugzilla.tianocore.org/show_bug.cgi?id=4507
> AllocatePool limits to allocate memory of 64 KB at most in PEI Phase.
> AllocatePool() is being avoided due to its 64k allocation size limit
> when the library is incorporated into a PEI component.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Ashraf Ali S <ashraf.ali.s@intel.com>
> Cc: Chinni B Duggapu <chinni.b.duggapu@intel.com>
> Signed-off-by: chitralekha ck <chitralekha.ck@intel.com>
> ---
>  MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
> index c5e885d7a6..a7ebcd1d65 100644
> --- a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
> +++ b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
> @@ -52,7 +52,7 @@ const BMP_IMAGE_HEADER  mBmpImageHeaderTemplate = {
>  /**
>    Translate a *.BMP graphics image to a GOP blt buffer. If a NULL Blt buffer
>    is passed in a GopBlt buffer will be allocated by this routine using
> -  EFI_BOOT_SERVICES.AllocatePool(). If a GopBlt buffer is passed in it will be
> +  EFI_BOOT_SERVICES.AllocatePages(). If a GopBlt buffer is passed in it will be
>    used if it is big enough.
>
>    @param[in]       BmpImage      Pointer to BMP file.
> @@ -312,7 +312,7 @@ TranslateBmpToGopBlt (
>      //
>      DEBUG ((DEBUG_INFO, "Bmp Support: Allocating 0x%X bytes of memory\n", BltBufferSize));
>      *GopBltSize = (UINTN)BltBufferSize;
> -    *GopBlt     = AllocatePool (*GopBltSize);
> +    *GopBlt     = AllocatePages (*GopBltSize);
>      IsAllocated = TRUE;
>      if (*GopBlt == NULL) {
>        return RETURN_OUT_OF_RESOURCES;
> --
> 2.38.1.windows.1
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107585): https://edk2.groups.io/g/devel/message/107585
Mute This Topic: https://groups.io/mt/100551576/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-