Re: [PATCH v2 08/13] ArmPlatformPkg: Add PCD to select pixel format

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

On 22 December 2017 at 18:34,  <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 format
>
> In LcdPlatformLib for PL111, LcdPlatformQueryMode 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 LcdHwLib.
>
> 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 LcdHwLib
> libraries, 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>

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

> ---
>  ArmPlatformPkg/ArmPlatformPkg.dec               |  7 +++
>  ArmPlatformPkg/Include/Library/LcdPlatformLib.h |  4 ++
>  ArmPlatformPkg/Library/HdLcd/HdLcd.c            | 55 +++++++-------------
>  ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c      | 15 +++++-
>  4 files changed, 45 insertions(+), 36 deletions(-)
>
> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
> index 7cec775abeee219e6821488a2c5abe88d23bbed1..e412414e0ba6506c7158e69bac04469e45601736 100644
> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -97,6 +97,13 @@ [PcdsFixedAtBuild.common]
>    gArmPlatformTokenSpaceGuid.PcdPL180SysMciRegAddress|0x00000000|UINT32|0x00000028
>    gArmPlatformTokenSpaceGuid.PcdPL180MciBaseAddress|0x00000000|UINT32|0x00000029
>
> +  # Graphics Output Pixel format
> +  # 0 : PixelRedGreenBlueReserved8BitPerColor
> +  # 1 : PixelBlueGreenRedReserved8BitPerColor
> +  # 2 : PixelBitMask
> +  # Default is set to UEFI console font format PixelBlueGreenRedReserved8BitPerColor
> +  gArmPlatformTokenSpaceGuid.PcdGopPixelFormat|0x00000001|UINT32|0x00000040
> +
>  [PcdsFixedAtBuild.common,PcdsDynamic.common]
>    ## PL031 RealTimeClock
>    gArmPlatformTokenSpaceGuid.PcdPL031RtcBase|0x0|UINT32|0x00000024
> diff --git a/ArmPlatformPkg/Include/Library/LcdPlatformLib.h b/ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> index 0943f28a19133e7bc558f9d529bb8ac8f66ba3fd..02be124f00ff5c34c3f8c07ff16ebb4ffc1ba20f 100644
> --- a/ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> +++ b/ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> @@ -166,6 +166,10 @@ typedef struct {
>    @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
> diff --git a/ArmPlatformPkg/Library/HdLcd/HdLcd.c b/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> index 1fc04c2d14d8185370454be459a23bdec41f6602..72cd5fa33b2553195638c595e72843a56b2e267c 100644
> --- a/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> +++ b/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> @@ -22,31 +22,7 @@
>
>  #include "HdLcd.h"
>
> -STATIC
> -UINTN
> -GetBytesPerPixel (
> -  IN  LCD_BPP       Bpp
> -  )
> -{
> -  switch (Bpp) {
> -  case LCD_BITS_PER_PIXEL_24:
> -    return 4;
> -
> -  case LCD_BITS_PER_PIXEL_16_565:
> -  case LCD_BITS_PER_PIXEL_16_555:
> -  case LCD_BITS_PER_PIXEL_12_444:
> -    return 2;
> -
> -  case LCD_BITS_PER_PIXEL_8:
> -  case LCD_BITS_PER_PIXEL_4:
> -  case LCD_BITS_PER_PIXEL_2:
> -  case LCD_BITS_PER_PIXEL_1:
> -    return 1;
> -
> -  default:
> -    return 0;
> -  }
> -}
> +#define BYTES_PER_PIXEL 4
>
>  /** Initialize display.
>
> @@ -78,10 +54,6 @@ LcdInitialize (
>      HDLCD_LITTLE_ENDIAN | HDLCD_4BYTES_PER_PIXEL
>      );
>
> -  MmioWrite32 (HDLCD_REG_RED_SELECT,   (0 << 16 | 8 << 8 | 0));
> -  MmioWrite32 (HDLCD_REG_GREEN_SELECT, (0 << 16 | 8 << 8 | 8));
> -  MmioWrite32 (HDLCD_REG_BLUE_SELECT,  (0 << 16 | 8 << 8 | 16));
> -
>    return EFI_SUCCESS;
>  }
>
> @@ -92,6 +64,7 @@ LcdInitialize (
>    @retval EFI_SUCCESS            Display mode set successfully.
>    @retval EFI_DEVICE_ERROR       Reurns an error if display timing
>                                   information is not available.
> +  @retval !EFI_SUCCESS           Other errors.
>  **/
>  EFI_STATUS
>  LcdSetMode (
> @@ -101,8 +74,8 @@ LcdSetMode (
>    EFI_STATUS          Status;
>    CONST SCAN_TIMINGS *Horizontal;
>    CONST SCAN_TIMINGS *Vertical;
> -  UINT32              BytesPerPixel;
> -  LCD_BPP             LcdBpp;
> +
> +  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  ModeInfo;
>
>    // Set the video mode timings and other relevant information
>    Status = LcdPlatformGetTimings (
> @@ -118,13 +91,22 @@ LcdSetMode (
>    ASSERT (Horizontal != NULL);
>    ASSERT (Vertical != NULL);
>
> -  Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> +  // Get the pixel format information.
> +  Status = LcdPlatformQueryMode (ModeNumber, &ModeInfo);
>    if (EFI_ERROR (Status)) {
>      ASSERT (FALSE);
>      return Status;
>    }
>
> -  BytesPerPixel = GetBytesPerPixel (LcdBpp);
> +  if (ModeInfo.PixelFormat == PixelBlueGreenRedReserved8BitPerColor) {
> +    MmioWrite32 (HDLCD_REG_RED_SELECT,  (8 << 8) | 16);
> +    MmioWrite32 (HDLCD_REG_BLUE_SELECT, (8 << 8) | 0);
> +  } else {
> +    MmioWrite32 (HDLCD_REG_BLUE_SELECT, (8 << 8) | 16);
> +    MmioWrite32 (HDLCD_REG_RED_SELECT,  (8 << 8) | 0);
> +  }
> +
> +  MmioWrite32 (HDLCD_REG_GREEN_SELECT, (8 << 8) | 8);
>
>    // Disable the controller
>    MmioWrite32 (HDLCD_REG_COMMAND, HDLCD_DISABLE);
> @@ -132,10 +114,13 @@ LcdSetMode (
>    // Update the frame buffer information with the new settings
>    MmioWrite32 (
>      HDLCD_REG_FB_LINE_LENGTH,
> -    Horizontal->Resolution * BytesPerPixel
> +    Horizontal->Resolution * BYTES_PER_PIXEL
>      );
>
> -  MmioWrite32 (HDLCD_REG_FB_LINE_PITCH, Horizontal->Resolution * BytesPerPixel);
> +  MmioWrite32 (
> +    HDLCD_REG_FB_LINE_PITCH,
> +    Horizontal->Resolution * BYTES_PER_PIXEL
> +    );
>
>    MmioWrite32 (HDLCD_REG_FB_LINE_COUNT, Vertical->Resolution - 1);
>
> diff --git a/ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c b/ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c
> index d1aba083fdc2ee4a7c25294955c4413465dca1e8..6f4fe9c051ff3524b3d26daddb0ac5e0e3ebe19d 100644
> --- a/ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c
> +++ b/ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c
> @@ -75,6 +75,7 @@ LcdInitialize (
>    @retval EFI_SUCCESS            Display set mode successfuly.
>    @retval EFI_DEVICE_ERROR       It returns an error if display timing
>                                   information is not available.
> +  @retval !EFI_SUCCESS           Other errors.
>  **/
>  EFI_STATUS
>  LcdSetMode (
> @@ -87,6 +88,8 @@ LcdSetMode (
>    UINT32              LcdControl;
>    LCD_BPP             LcdBpp;
>
> +  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  ModeInfo;
> +
>    // Set the video mode timings and other relevant information
>    Status = LcdPlatformGetTimings (
>               ModeNumber,
> @@ -107,6 +110,13 @@ LcdSetMode (
>      return Status;
>    }
>
> +  // Get the pixel format information
> +  Status = LcdPlatformQueryMode (ModeNumber, &ModeInfo);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (FALSE);
> +    return Status;
> +  }
> +
>    // Disable the CLCD_LcdEn bit
>    MmioAnd32 (PL111_REG_LCD_CONTROL, ~PL111_CTRL_LCD_EN);
>
> @@ -140,7 +150,10 @@ LcdSetMode (
>
>    // PL111_REG_LCD_CONTROL
>    LcdControl = PL111_CTRL_LCD_EN | PL111_CTRL_LCD_BPP (LcdBpp)
> -               | PL111_CTRL_LCD_TFT | PL111_CTRL_BGR;
> +               | PL111_CTRL_LCD_TFT;
> +  if (ModeInfo.PixelFormat == PixelBlueGreenRedReserved8BitPerColor) {
> +    LcdControl |= PL111_CTRL_BGR;
> +  }
>    MmioWrite32 (PL111_REG_LCD_CONTROL, LcdControl);
>
>    // Turn on power to the LCD Panel
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>