Re: [PATCH v3 10/16] ArmPlatformPkg: Add PCD to select pixel format

[Evan Lloyd <Evan.Lloyd@arm.com>]

Reviewed-by: Evan Lloyd <evan.lloyd@arm.com>

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Girish
> Pathak
> Sent: 20 March 2018 16:12
> To: edk2-devel@lists.01.org
> Cc: nd <nd@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-
> Fitt@arm.com>; leif.lindholm@linaro.org; ard.biesheuvel@linaro.org
> Subject: [edk2] [PATCH v3 10/16] ArmPlatformPkg: Add PCD to select pixel
> format
> 
> 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          |  9 +++-
>  ArmPlatformPkg/Library/HdLcd/HdLcd.c       | 54 +++++++-------------
>  ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c | 15 +++++-
>  3 files changed, 40 insertions(+), 38 deletions(-)
> 
> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec
> b/ArmPlatformPkg/ArmPlatformPkg.dec
> index
> 7cec775abeee219e6821488a2c5abe88d23bbed1..378bee9cbc9e4bd50c37
> b38156016424e24cba73 100644
> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -1,6 +1,6 @@
>  #/** @file
>  #
> -#  Copyright (c) 2011-2017, ARM Limited. All rights reserved.
> +#  Copyright (c) 2011-2018, ARM Limited. All rights reserved.
>  #  Copyright (c) 2015, Intel Corporation. All rights reserved.
>  #
>  #  This program and the accompanying materials @@ -97,6 +97,13 @@
> [PcdsFixedAtBuild.common]
> 
> gArmPlatformTokenSpaceGuid.PcdPL180SysMciRegAddress|0x00000000|
> UINT32|0x00000028
> 
> gArmPlatformTokenSpaceGuid.PcdPL180MciBaseAddress|0x00000000|UI
> NT32|0x00000029
> 
> +  # Graphics Output Pixel format
> +  # 0 : PixelRedGreenBlueReserved8BitPerColor
> +  # 1 : PixelBlueGreenRedReserved8BitPerColor
> +  # 2 : PixelBitMask
> +  # Default is set to UEFI console font format
> + PixelBlueGreenRedReserved8BitPerColor
> +
> +
> gArmPlatformTokenSpaceGuid.PcdGopPixelFormat|0x00000001|UINT32|0
> x00000
> + 040
> +
>  [PcdsFixedAtBuild.common,PcdsDynamic.common]
>    ## PL031 RealTimeClock
> 
> gArmPlatformTokenSpaceGuid.PcdPL031RtcBase|0x0|UINT32|0x00000024
> diff --git a/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> b/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> index
> f5886848ce582b475b597ccca015c816707ade0e..96f2bf437fbabd2509f86
> 0c67c5442def5b5f03d 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;
>  }
> 
> @@ -100,8 +72,8 @@ LcdSetMode (
>    EFI_STATUS        Status;
>    SCAN_TIMINGS      *Horizontal;
>    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 (
> @@ -117,13 +89,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_EFI_ERROR (Status);
>      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); @@ -131,10
> +112,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
> c9e2736911881fc36b51562b9259b7bccf30747d..0496376fffe514651a70f
> 58a3316894a4b2b319c 100644
> --- a/ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c
> +++ b/ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c
> @@ -76,7 +76,6 @@ LcdInitialize (
> 
>    @retval EFI_SUCCESS            Display mode set successfuly.
>    @retval !(EFI_SUCCESS)         Other errors.
> -
>  **/
>  EFI_STATUS
>  LcdSetMode (
> @@ -89,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,
> @@ -109,6 +110,13 @@ LcdSetMode (
>      return Status;
>    }
> 
> +  // Get the pixel format information
> +  Status = LcdPlatformQueryMode (ModeNumber, &ModeInfo);  if
> (EFI_ERROR
> + (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return Status;
> +  }
> +
>    // Disable the CLCD_LcdEn bit
>    MmioAnd32 (PL111_REG_LCD_CONTROL, ~PL111_CTRL_LCD_EN);
> 
> @@ -142,7 +150,10 @@ LcdSetMode (
> 
>    // PL111_REG_LCD_CONTROL
>    LcdControl = PL111_CTRL_LCD_EN | PL111_CTRL_LCD_BPP (LcdBpp) |
> -               PL111_CTRL_LCD_TFT | PL111_CTRL_LCD_PWR |
> PL111_CTRL_BGR;
> +               PL111_CTRL_LCD_TFT | PL111_CTRL_LCD_PWR;  if
> + (ModeInfo.PixelFormat == PixelBlueGreenRedReserved8BitPerColor) {
> +    LcdControl |= PL111_CTRL_BGR;
> +  }
>    MmioWrite32 (PL111_REG_LCD_CONTROL, LcdControl);
> 
>    return EFI_SUCCESS;
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel