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

["Ard Biesheuvel" <ardb@kernel.org>]

On Mon, 24 Jul 2023 at 06:56, S, Ashraf Ali <ashraf.ali.s@intel.com> wrote:
>
> Hi.,
>
> The observation is based on if the Library consumed in the PEI Phase (PostMem).
>

In that case, please fix the comment that refers to
EFI_BOOT_SERVICES.AllocatePool/Pages() directly, and instead, update
the comment to reflect that AllocatePool() is being avoided due to its
64k allocation size limit when the library is incorporated into a PEI
component.

Thanks,


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> Sent: Friday, July 21, 2023 10:49 PM
> To: devel@edk2.groups.io; Ck, Chitralekha <chitralekha.ck@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; S, Ashraf Ali <ashraf.ali.s@intel.com>; Duggapu, Chinni B <chinni.b.duggapu@intel.com>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg: AllocatePages for TranslateBmpToGopBlt
>
> On Fri, 21 Jul 2023 at 17:38, 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.
>
> What is the basis for this observation? In DXE, pool allocations are unbounded afaik.
>
>
>
> > if the the image size is higher than that it will fail to allocate.
> > change the function debug string to __func__
> >
>
> Please don't mix unrelated changes in the same patch.
>
> > 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>
> > ---
> >  .../Library/BaseBmpSupportLib/BmpSupportLib.c | 58
> > +++++++++++--------
> >  1 file changed, 33 insertions(+), 25 deletions(-)
> >
> > diff --git a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
> > b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
> > index c5e885d7a6..d0833a721f 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.
> > @@ -113,14 +113,14 @@ TranslateBmpToGopBlt (
> >    }
> >
> >    if (BmpImageSize < sizeof (BMP_IMAGE_HEADER)) {
> > -    DEBUG ((DEBUG_ERROR, "TranslateBmpToGopBlt: BmpImageSize too small\n"));
> > +    DEBUG ((DEBUG_ERROR, "%a: BmpImageSize too small\n", __func__));
> >      return RETURN_UNSUPPORTED;
> >    }
> >
> >    BmpHeader = (BMP_IMAGE_HEADER *)BmpImage;
> >
> >    if ((BmpHeader->CharB != 'B') || (BmpHeader->CharM != 'M')) {
> > -    DEBUG ((DEBUG_ERROR, "TranslateBmpToGopBlt: BmpHeader->Char fields incorrect\n"));
> > +    DEBUG ((DEBUG_ERROR, "%a: BmpHeader->Char fields incorrect\n",
> > + __func__));
> >      return RETURN_UNSUPPORTED;
> >    }
> >
> > @@ -128,12 +128,12 @@ TranslateBmpToGopBlt (
> >    // Doesn't support compress.
> >    //
> >    if (BmpHeader->CompressionType != 0) {
> > -    DEBUG ((DEBUG_ERROR, "TranslateBmpToGopBlt: Compression Type unsupported.\n"));
> > +    DEBUG ((DEBUG_ERROR, "%a: Compression Type unsupported\n",
> > + __func__));
> >      return RETURN_UNSUPPORTED;
> >    }
> >
> >    if ((BmpHeader->PixelHeight == 0) || (BmpHeader->PixelWidth == 0)) {
> > -    DEBUG ((DEBUG_ERROR, "TranslateBmpToGopBlt: BmpHeader->PixelHeight or BmpHeader->PixelWidth is 0.\n"));
> > +    DEBUG ((DEBUG_ERROR, "%a: BmpHeader PixelHeight or PixelWidth is
> > + 0\n", __func__));
> >      return RETURN_UNSUPPORTED;
> >    }
> >
> > @@ -144,7 +144,8 @@ TranslateBmpToGopBlt (
> >    if (BmpHeader->HeaderSize != sizeof (BMP_IMAGE_HEADER) - OFFSET_OF (BMP_IMAGE_HEADER, HeaderSize)) {
> >      DEBUG ((
> >        DEBUG_ERROR,
> > -      "TranslateBmpToGopBlt: BmpHeader->Headership is not as expected.  Headersize is 0x%x\n",
> > +      "%a: BmpHeader->Headership is not as expected. Headersize is 0x%x\n",
> > +      __func__,
> >        BmpHeader->HeaderSize
> >        ));
> >      return RETURN_UNSUPPORTED;
> > @@ -161,7 +162,8 @@ TranslateBmpToGopBlt (
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((
> >        DEBUG_ERROR,
> > -      "TranslateBmpToGopBlt: invalid BmpImage... PixelWidth:0x%x BitPerPixel:0x%x\n",
> > +      "%a: invalid BmpImage. PixelWidth:0x%x BitPerPixel:0x%x\n",
> > +      __func__,
> >        BmpHeader->PixelWidth,
> >        BmpHeader->BitPerPixel
> >        ));
> > @@ -172,7 +174,8 @@ TranslateBmpToGopBlt (
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((
> >        DEBUG_ERROR,
> > -      "TranslateBmpToGopBlt: invalid BmpImage... DataSizePerLine:0x%x\n",
> > +      "%a: invalid BmpImage. DataSizePerLine:0x%x\n",
> > +      __func__,
> >        DataSizePerLine
> >        ));
> >
> > @@ -189,7 +192,8 @@ TranslateBmpToGopBlt (
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((
> >        DEBUG_ERROR,
> > -      "TranslateBmpToGopBlt: invalid BmpImage... DataSizePerLine:0x%x PixelHeight:0x%x\n",
> > +      "%a: invalid BmpImage. DataSizePerLine:0x%x PixelHeight:0x%x\n",
> > +      __func__,
> >        DataSizePerLine,
> >        BmpHeader->PixelHeight
> >        ));
> > @@ -206,7 +210,8 @@ TranslateBmpToGopBlt (
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((
> >        DEBUG_ERROR,
> > -      "TranslateBmpToGopBlt: invalid BmpImage... PixelHeight:0x%x DataSizePerLine:0x%x\n",
> > +      "%a: invalid BmpImage. PixelHeight:0x%x DataSizePerLine:0x%x\n",
> > +      __func__,
> >        BmpHeader->PixelHeight,
> >        DataSizePerLine
> >        ));
> > @@ -218,11 +223,11 @@ TranslateBmpToGopBlt (
> >        (BmpHeader->Size < BmpHeader->ImageOffset) ||
> >        (BmpHeader->Size - BmpHeader->ImageOffset != DataSize))
> >    {
> > -    DEBUG ((DEBUG_ERROR, "TranslateBmpToGopBlt: invalid BmpImage... \n"));
> > -    DEBUG ((DEBUG_ERROR, "   BmpHeader->Size: 0x%x\n", BmpHeader->Size));
> > -    DEBUG ((DEBUG_ERROR, "   BmpHeader->ImageOffset: 0x%x\n", BmpHeader->ImageOffset));
> > -    DEBUG ((DEBUG_ERROR, "   BmpImageSize: 0x%lx\n", (UINTN)BmpImageSize));
> > -    DEBUG ((DEBUG_ERROR, "   DataSize: 0x%lx\n", (UINTN)DataSize));
> > +    DEBUG ((DEBUG_ERROR, "%a: invalid BmpImage... \n", __func__));
> > +    DEBUG ((DEBUG_ERROR, " Size: 0x%x\n", BmpHeader->Size));
> > +    DEBUG ((DEBUG_ERROR, " ImageOffset: 0x%x\n", BmpHeader->ImageOffset));
> > +    DEBUG ((DEBUG_ERROR, " BmpImageSize: 0x%lx\n", (UINTN)BmpImageSize));
> > +    DEBUG ((DEBUG_ERROR, " DataSize: 0x%lx\n", (UINTN)DataSize));
> >
> >      return RETURN_UNSUPPORTED;
> >    }
> > @@ -279,7 +284,8 @@ TranslateBmpToGopBlt (
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((
> >        DEBUG_ERROR,
> > -      "TranslateBmpToGopBlt: invalid BltBuffer needed size... PixelWidth:0x%x PixelHeight:0x%x\n",
> > +      "%a: invalid BltBuffer needed size. PixelWidth:0x%x PixelHeight:0x%x\n",
> > +      __func__,
> >        BmpHeader->PixelWidth,
> >        BmpHeader->PixelHeight
> >        ));
> > @@ -297,7 +303,8 @@ TranslateBmpToGopBlt (
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((
> >        DEBUG_ERROR,
> > -      "TranslateBmpToGopBlt: invalid BltBuffer needed size... PixelWidth x PixelHeight:0x%x struct size:0x%x\n",
> > +      "%a: invalid BltBuffer needed size. PixelWidth x PixelHeight:0x%x struct size:0x%x\n",
> > +      __func__,
> >        Temp,
> >        sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)
> >        ));
> > @@ -312,7 +319,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; @@ -329,13 +336,14 @@
> > TranslateBmpToGopBlt (
> >
> >    *PixelWidth  = BmpHeader->PixelWidth;
> >    *PixelHeight = BmpHeader->PixelHeight;
> > -  DEBUG ((DEBUG_INFO, "BmpHeader->ImageOffset 0x%X\n",
> > BmpHeader->ImageOffset));
> > -  DEBUG ((DEBUG_INFO, "BmpHeader->PixelWidth 0x%X\n",
> > BmpHeader->PixelWidth));
> > -  DEBUG ((DEBUG_INFO, "BmpHeader->PixelHeight 0x%X\n",
> > BmpHeader->PixelHeight));
> > -  DEBUG ((DEBUG_INFO, "BmpHeader->BitPerPixel 0x%X\n",
> > BmpHeader->BitPerPixel));
> > -  DEBUG ((DEBUG_INFO, "BmpHeader->ImageSize 0x%X\n",
> > BmpHeader->ImageSize));
> > -  DEBUG ((DEBUG_INFO, "BmpHeader->HeaderSize 0x%X\n",
> > BmpHeader->HeaderSize));
> > -  DEBUG ((DEBUG_INFO, "BmpHeader->Size 0x%X\n", BmpHeader->Size));
> > +  DEBUG ((DEBUG_INFO, "BmpHeader :\n"));  DEBUG ((DEBUG_INFO, "
> > + ImageOffset 0x%X\n", BmpHeader->ImageOffset));  DEBUG ((DEBUG_INFO,
> > + " PixelWidth 0x%X\n", BmpHeader->PixelWidth));  DEBUG ((DEBUG_INFO,
> > + " PixelHeight 0x%X\n", BmpHeader->PixelHeight));  DEBUG
> > + ((DEBUG_INFO, " BitPerPixel 0x%X\n", BmpHeader->BitPerPixel));
> > + DEBUG ((DEBUG_INFO, " ImageSize 0x%X\n", BmpHeader->ImageSize));
> > + DEBUG ((DEBUG_INFO, " HeaderSize 0x%X\n", BmpHeader->HeaderSize));
> > + DEBUG ((DEBUG_INFO, " Size 0x%X\n", BmpHeader->Size));
> >
> >    //
> >    // Translate image from BMP to Blt buffer format
> > --
> > 2.38.1.windows.1
> >
> >
> >
> >
> >
> >
>
>
> 
>
>


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