Re: [PATCH edk2-platforms v2 13/18] ARM/VExpressPkg: PL111 and HDLCD: Add PCD to select pixel format

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 22 December 2017 at 19:08,  <evan.lloyd@arm.com> wrote:
> From: Girish Pathak <girish.pathak at arm.com>
>
> Current HDLCD and PL111 platform libraries do not support display modes
> with PixelBlueGreenRedReserved8BitPerColor format,  i.e. because of
> historical confusion, they do not support the UEFI default
> PixelBlueGreenRedReserved8BitPerColor
>
> LcdPlatformLib for PL111, LcdPlatformQueryMode function returns the
> pixel format as PixelRedGreenBlueReserved8BitPerColor which is wrong,
> because that does not match the display controller's pixel format which
> is set to BGR in PL111Lcd GOP driver.
>
> Also it is not possible to configure pixel format as RGB/BGR for the
> display modes for a platform at build time.
>
> This change adds PcdGopPixelFormat to configure pixel format as
>     PixelRedGreenBlueReserved8BitPerColor    or
>     PixelBlueGreenRedReserved8BitPerColor    or
>     PixelBitMask.
> With this change, pixel format can be selected in the platform specific
> .dsc file for all supported display modes.
>
> Support for PixelBitMask is not implemented in PL111 or HDLCD
> GOP driver, hence  HDLCD and PL111 platform libraries will return error
> EFI_UNSUPPORTED if PcdGopPixelFormat is set to PixelBitMask.
> Indeed, it is not clear what selecting PixelBitMask might mean, but
> the option is allowed as it might suit a custom platform.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> ---
>  Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf       |  1 +
>  Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf |  1 +
>  Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c            | 23 ++++++++----
>  Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c      | 38 +++++++++-----------
>  4 files changed, 35 insertions(+), 28 deletions(-)
>
> diff --git a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf
> index 2e83736609cf8c63cb498368cd377f6a23964ef4..4cbd324338be76a0b0bfb811159d893628e74155 100644
> --- a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf
> +++ b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf
> @@ -41,3 +41,4 @@ [Protocols]
>
>  [FixedPcd]
>    gArmVExpressTokenSpaceGuid.PcdHdLcdVideoModeOscId
> +  gArmPlatformTokenSpaceGuid.PcdGopPixelFormat
> diff --git a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf
> index 1ee878041559fa84a810f65175f2507bda663d76..20045380149241ce14f41bcb70afcb8145fe821f 100644
> --- a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf
> +++ b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf
> @@ -42,3 +42,4 @@ [Protocols]
>  [FixedPcd]
>    gArmVExpressTokenSpaceGuid.PcdPL111LcdMaxMode
>    gArmVExpressTokenSpaceGuid.PcdPL111LcdVideoModeOscId
> +  gArmPlatformTokenSpaceGuid.PcdGopPixelFormat
> diff --git a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> index b448d70f86d6acbc6bdae9749c7b6d0205935ba7..f1c18ac955f621e9eab3dede86758f5f1b1a791d 100644
> --- a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> +++ b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> @@ -15,7 +15,6 @@
>  #include <PiDxe.h>
>
>  #include <Library/ArmPlatformSysConfigLib.h>
> -#include <Library/IoLib.h>
>  #include <Library/PcdLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/DxeServicesTableLib.h>
> @@ -93,6 +92,10 @@ EFI_EDID_ACTIVE_PROTOCOL      mEdidActive = {
>    @param[in] Handle              Handle to the LCD device instance.
>
>    @retval EFI_SUCCESS            Plaform library initialized successfully.
> +  @retval EFI_UNSUPPORTED        PcdGopPixelFormat must be
> +                                 PixelRedGreenBlueReserved8BitPerColor OR
> +                                 PixelBlueGreenRedReserved8BitPerColor
> +                                 any other format is not supported.
>    @retval !(EFI_SUCCESS)         Other errors.
>  **/
>  EFI_STATUS
> @@ -101,6 +104,17 @@ LcdPlatformInitializeDisplay (
>    )
>  {
>    EFI_STATUS  Status;
> +  EFI_GRAPHICS_PIXEL_FORMAT PixelFormat;
> +
> +  // PixelBitMask and PixelBltOnly pixel formats are not supported
> +  PixelFormat = FixedPcdGet32 (PcdGopPixelFormat);
> +  if (PixelFormat != PixelRedGreenBlueReserved8BitPerColor
> +    && PixelFormat != PixelBlueGreenRedReserved8BitPerColor) {
> +

Please fix the weird indentation here

> +    ASSERT (PixelFormat == PixelRedGreenBlueReserved8BitPerColor
> +      ||  PixelFormat == PixelBlueGreenRedReserved8BitPerColor);

and here (just put the boolean operator on the previous line)

> +   return EFI_UNSUPPORTED;
> +  }
>
>    // Set the FPGA multiplexer to select the video output from the
>    // motherboard or the daughterboard
> @@ -285,12 +299,7 @@ LcdPlatformQueryMode (
>    Info->VerticalResolution = mDisplayModes[ModeNumber].Vertical.Resolution;
>    Info->PixelsPerScanLine = mDisplayModes[ModeNumber].Horizontal.Resolution;
>
> -  /* Bits per Pixel is always LCD_BITS_PER_PIXEL_24 */
> -  Info->PixelFormat                   = PixelRedGreenBlueReserved8BitPerColor;
> -  Info->PixelInformation.RedMask      = LCD_24BPP_RED_MASK;
> -  Info->PixelInformation.GreenMask    = LCD_24BPP_GREEN_MASK;
> -  Info->PixelInformation.BlueMask     = LCD_24BPP_BLUE_MASK;
> -  Info->PixelInformation.ReservedMask = LCD_24BPP_RESERVED_MASK;
> +  Info->PixelFormat = FixedPcdGet32 (PcdGopPixelFormat);
>
>    return EFI_SUCCESS;
>  }
> diff --git a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> index 439cbdb1a73145fc4dc9c3c9587ce3fd9b9fff56..16a74c76cf27526493165dc6d81384f752fd3f20 100644
> --- a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> +++ b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> @@ -149,7 +149,12 @@ EFI_EDID_ACTIVE_PROTOCOL      mEdidActive = {
>  /** PL111 Platform specific initialization function.
>
>    @param[in] Handle              Handle to the LCD device instance.
> +
>    @retval EFI_SUCCESS            Plaform library initialized successfully.
> +  @retval EFI_UNSUPPORTED        PcdGopPixelFormat must be
> +                                 PixelRedGreenBlueReserved8BitPerColor OR
> +                                 PixelBlueGreenRedReserved8BitPerColor
> +                                 any other format is not supported
>    @retval !(EFI_SUCCESS)         Other errors.
>  **/
>  EFI_STATUS
> @@ -158,6 +163,17 @@ LcdPlatformInitializeDisplay (
>    )
>  {
>    EFI_STATUS  Status;
> +  EFI_GRAPHICS_PIXEL_FORMAT PixelFormat;
> +
> +  // PixelBitMask and PixelBltOnly pixel formats are not supported
> +  PixelFormat = FixedPcdGet32 (PcdGopPixelFormat);
> +  if (PixelFormat != PixelRedGreenBlueReserved8BitPerColor
> +    && PixelFormat != PixelBlueGreenRedReserved8BitPerColor) {
> +
> +    ASSERT (PixelFormat == PixelRedGreenBlueReserved8BitPerColor
> +      ||  PixelFormat == PixelBlueGreenRedReserved8BitPerColor);

Same here

> +   return EFI_UNSUPPORTED;
> +  }
>
>    // Set the FPGA multiplexer to select the video output from the motherboard
>    // or the daughterboard
> @@ -360,27 +376,7 @@ LcdPlatformQueryMode (
>    Info->VerticalResolution = mDisplayModes[ModeNumber].Vertical.Resolution;
>    Info->PixelsPerScanLine = mDisplayModes[ModeNumber].Horizontal.Resolution;
>
> -  switch (mDisplayModes[ModeNumber].Bpp) {
> -  case LCD_BITS_PER_PIXEL_24:
> -    Info->PixelFormat                   = PixelRedGreenBlueReserved8BitPerColor;
> -    Info->PixelInformation.RedMask      = LCD_24BPP_RED_MASK;
> -    Info->PixelInformation.GreenMask    = LCD_24BPP_GREEN_MASK;
> -    Info->PixelInformation.BlueMask     = LCD_24BPP_BLUE_MASK;
> -    Info->PixelInformation.ReservedMask = LCD_24BPP_RESERVED_MASK;
> -    break;
> -
> -  case LCD_BITS_PER_PIXEL_16_555:
> -  case LCD_BITS_PER_PIXEL_16_565:
> -  case LCD_BITS_PER_PIXEL_12_444:
> -  case LCD_BITS_PER_PIXEL_8:
> -  case LCD_BITS_PER_PIXEL_4:
> -  case LCD_BITS_PER_PIXEL_2:
> -  case LCD_BITS_PER_PIXEL_1:
> -  default:
> -    // These are not supported
> -    ASSERT (FALSE);
> -    break;
> -  }
> +  Info->PixelFormat = FixedPcdGet32 (PcdGopPixelFormat);
>
>    return EFI_SUCCESS;
>  }
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>

With the above fixed

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>