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

2018-03-21 Thread Evan Lloyd 
Reviewed-by: Evan Lloyd <evan.ll...@arm.com>

> -Original Message-
> From: edk2-devel <edk2-devel-boun...@lists.01.org> On Behalf Of Girish
> Pathak
> Sent: 20 March 2018 16:12
> To: edk2-devel@lists.01.org
> Cc: nd <n...@arm.com>; Stephanie Hughes-Fitt  f...@arm.com>; leif.lindh...@linaro.org; ard.biesheu...@linaro.org
> Subject: [edk2] [PATCH v3 10/16] ArmPlatformPkg: Add PCD to select pixel
> format
> 
> From: Girish Pathak 
> 
> 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
> PixelRedGreenBlueReserved8BitPerColoror
> PixelBlueGreenRedReserved8BitPerColoror
> 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.pat...@arm.com>
> Signed-off-by: Evan Lloyd <evan.ll...@arm.com>
> Reviewed-by: Ard Biesheuvel <ard.biesheu...@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|0x|
> UINT32|0x0028
> 
> gArmPlatformTokenSpaceGuid.PcdPL180MciBaseAddress|0x|UI
> NT32|0x0029
> 
> +  # Graphics Output Pixel format
> +  # 0 : PixelRedGreenBlueReserved8BitPerColor
> +  # 1 : PixelBlueGreenRedReserved8BitPerColor
> +  # 2 : PixelBitMask
> +  # Default is set to UEFI console font format
> + PixelBlueGreenRedReserved8BitPerColor
> +
> +
> gArmPlatformTokenSpaceGuid.PcdGopPixelFormat|0x0001|UINT32|0
> x0
> + 040
> +
>  [PcdsFixedAtBuild.common,PcdsDynamic.common]
>## PL031 RealTimeClock
> 
> gArmPlatformTokenSpaceGuid.PcdPL031RtcBase|0x0|UINT32|0x0024
> 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

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

2018-03-20 Thread Girish Pathak 
From: Girish Pathak 

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
PixelRedGreenBlueReserved8BitPerColoror
PixelBlueGreenRedReserved8BitPerColoror
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 
Signed-off-by: Evan Lloyd 
Reviewed-by: Ard Biesheuvel 
---
 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..378bee9cbc9e4bd50c37b38156016424e24cba73
 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|0x|UINT32|0x0028
   
gArmPlatformTokenSpaceGuid.PcdPL180MciBaseAddress|0x|UINT32|0x0029
 
+  # Graphics Output Pixel format
+  # 0 : PixelRedGreenBlueReserved8BitPerColor
+  # 1 : PixelBlueGreenRedReserved8BitPerColor
+  # 2 : PixelBitMask
+  # Default is set to UEFI console font format 
PixelBlueGreenRedReserved8BitPerColor
+  gArmPlatformTokenSpaceGuid.PcdGopPixelFormat|0x0001|UINT32|0x0040
+
 [PcdsFixedAtBuild.common,PcdsDynamic.common]
   ## PL031 RealTimeClock
   gArmPlatformTokenSpaceGuid.PcdPL031RtcBase|0x0|UINT32|0x0024
diff --git a/ArmPlatformPkg/Library/HdLcd/HdLcd.c 
b/ArmPlatformPkg/Library/HdLcd/HdLcd.c
index 
f5886848ce582b475b597ccca015c816707ade0e..96f2bf437fbabd2509f860c67c5442def5b5f03d
 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_STATUSStatus;
   SCAN_TIMINGS  *Horizontal;
   SCAN_TIMINGS  *Vertical;
-  UINT32BytesPerPixel;
-  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, );
+  // Get the pixel format information.
+  Status = LcdPlatformQueryMode (ModeNumber, );
   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);
+