On Tue, Sep 26, 2017 at 09:15:24PM +0100, [email protected] wrote:
> From: Girish Pathak <[email protected]>
>
> 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!
>
> 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 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 <[email protected]>
> Signed-off-by: Evan Lloyd <[email protected]>
> ---
> ArmPlatformPkg/ArmPlatformPkg.dec
> | 9 ++++-
>
> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf
> | 1 +
>
> ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf
> | 1 +
> ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> | 6 +++-
> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> | 23 ++++++++----
>
> ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> | 37 +++++++++-----------
> ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> | 32 +++++++++++------
> ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> | 16 ++++++++-
> 8 files changed, 83 insertions(+), 42 deletions(-)
>
> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec
> b/ArmPlatformPkg/ArmPlatformPkg.dec
> index
> b8a6b131632dc6788b73a034a41b9e3176dffafa..548d2b211753275337c6973e05227cee8451c185
> 100644
> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -1,6 +1,6 @@
> #/** @file
> #
> -# Copyright (c) 2011-2016, ARM Limited. All rights reserved.
> +# Copyright (c) 2011-2017, ARM Limited. All rights reserved.
> # Copyright (c) 2015, Intel Corporation. All rights reserved.
> #
> # This program and the accompanying materials
> @@ -126,6 +126,13 @@ [PcdsFixedAtBuild.common]
> gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L""|VOID*|0x0000001B
> gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L""|VOID*|0x0000001C
>
> + # 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/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf
>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf
> index
> bba851c9bd6089cee748b45f40599b24c1293785..37756481596c7e978ed9ed0a932eeb2aa0a3b657
> 100644
> ---
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf
> +++
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf
> @@ -42,3 +42,4 @@ [Protocols]
> [FixedPcd]
> gArmVExpressTokenSpaceGuid.PcdPL111LcdMaxMode
> gArmVExpressTokenSpaceGuid.PcdHdLcdVideoModeOscId
> + gArmPlatformTokenSpaceGuid.PcdGopPixelFormat
> diff --git
> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf
>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf
> index
> 1a044baf4698aa6bfa5cd6038f01e84f7a633ea9..6f1cb3b55ff814d007718b5597f821dd20100477
> 100644
> ---
> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf
> +++
> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf
> @@ -42,3 +42,4 @@ [Protocols]
> [FixedPcd]
> gArmVExpressTokenSpaceGuid.PcdPL111LcdMaxMode
> gArmVExpressTokenSpaceGuid.PcdPL111LcdVideoModeOscId
> + gArmPlatformTokenSpaceGuid.PcdGopPixelFormat
> diff --git a/ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> b/ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> index
> f2f345b18fd15f2cde159fd42d3208a28f598a1f..d357c22c46b62966859793372c447883e12e1e80
> 100644
> --- a/ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> +++ b/ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> @@ -139,7 +139,6 @@
> #define LCD_12BPP_444_BLUE_MASK 0x0000000F
> #define LCD_12BPP_444_RESERVED_MASK 0x0000F000
>
> -
Spurious whitespace change adding an extra hunk to the patch, please drop.
> // The enumeration indexes maps the PL111 LcdBpp values used in the LCD
> Control Register
> typedef enum {
> LCD_BITS_PER_PIXEL_1 = 0,
> @@ -165,6 +164,10 @@ typedef struct {
> * @param IN Handle Handle to the LCD device instance.
> *
> * @retval EFI_SUCCESS Platform initialization success.
> + * @retval EFI_UNSUPPORTED PcdGopPixelFormat must be
> + * PixelRedGreenBlueReserved8BitPerColor OR
> + * PixelBlueGreenRedReserved8BitPerColor
> + * any other format is not supported.
> * @retval !(EFI_SUCCESS) Other errors.
> **/
> EFI_STATUS
> @@ -216,6 +219,7 @@ LcdPlatformSetMode (
> * (on success).
> *
> * @retval EFI_SUCCESS Success if the requested mode is found.
> + * @retval EFI_INVALID_PARAMETER Info is NULL.
> * @retval EFI_INVALID_PARAMETER Requested mode not found.
> **/
> EFI_STATUS
> diff --git
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> index
> 2401cdb30cb7252964ce1f363aa26d99933c09be..07730ed0d7a84b3fb64047bd1013fbc97301db53
> 100644
> ---
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> +++
> b/ArmPlatformPkg/ArmVExpressPkg/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>
> @@ -91,6 +90,10 @@ EFI_EDID_ACTIVE_PROTOCOL mEdidActive = {
> /** HDLCD Platform specific initialization function.
> *
> * @retval EFI_SUCCESS Plaform library initialization success.
> + * @retval EFI_UNSUPPORTED PcdGopPixelFormat must be
> + * PixelRedGreenBlueReserved8BitPerColor OR
> + * PixelBlueGreenRedReserved8BitPerColor
> + * any other format is not supported.
> * @retval !(EFI_SUCCESS) Other errors.
> **/
> EFI_STATUS
> @@ -99,6 +102,7 @@ LcdPlatformInitializeDisplay (
> )
> {
> EFI_STATUS Status;
> + EFI_GRAPHICS_PIXEL_FORMAT PixelFormat;
>
> /* Set the FPGA multiplexer to select the video output from the
> * motherboard or the daughterboard */
> @@ -120,6 +124,16 @@ LcdPlatformInitializeDisplay (
> NULL
> );
>
> + // PixelBitMask and PixelBltOnly pixel formats are not supported
> + PixelFormat = FixedPcdGet32 (PcdGopPixelFormat);
> + if (PixelFormat != PixelRedGreenBlueReserved8BitPerColor
> + && PixelFormat != PixelBlueGreenRedReserved8BitPerColor) {
> +
> + ASSERT (PixelFormat == PixelRedGreenBlueReserved8BitPerColor
> + || PixelFormat == PixelBlueGreenRedReserved8BitPerColor);
> + return EFI_UNSUPPORTED;
> + }
> +
This block looks identical to a similar block in the next file.
Could this be a shared helper function?
> return Status;
> }
>
> @@ -282,12 +296,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/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> index
> 753c5b615361f83625cdd4f0506909721da014b6..31d5e037e42a51f1c08e5bf2fa22eeb7be23e45f
> 100644
> ---
> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> +++
> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> @@ -149,6 +149,10 @@ EFI_EDID_ACTIVE_PROTOCOL mEdidActive = {
> /** PL111 Platform specific initialization function.
> *
> * @retval EFI_SUCCESS Plaform library initialization success.
> + * @retval EFI_UNSUPPORTED PcdGopPixelFormat must be
> + * PixelRedGreenBlueReserved8BitPerColor OR
> + * PixelBlueGreenRedReserved8BitPerColor
> + * any other format is not supported
> * @retval !(EFI_SUCCESS) Other errors.
> **/
> EFI_STATUS
> @@ -157,6 +161,7 @@ LcdPlatformInitializeDisplay (
> )
> {
> EFI_STATUS Status;
> + EFI_GRAPHICS_PIXEL_FORMAT PixelFormat;
>
> // Set the FPGA multiplexer to select the video output from the
> motherboard or the daughterboard
> Status = ArmPlatformSysConfigSet (SYS_CFG_MUXFPGA, PL111_CLCD_SITE);
> @@ -172,6 +177,16 @@ LcdPlatformInitializeDisplay (
> );
> }
>
> + // PixelBitMask and PixelBltOnly pixel formats are not supported
> + PixelFormat = FixedPcdGet32 (PcdGopPixelFormat);
> + if (PixelFormat != PixelRedGreenBlueReserved8BitPerColor
> + && PixelFormat != PixelBlueGreenRedReserved8BitPerColor) {
> +
> + ASSERT (PixelFormat == PixelRedGreenBlueReserved8BitPerColor
> + || PixelFormat == PixelBlueGreenRedReserved8BitPerColor);
> + return EFI_UNSUPPORTED;
> + }
> +
(This one.)
> return Status;
> }
>
> @@ -366,27 +381,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;
> }
> diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> index
> 03153c06d314cb497c91889386ca6075c0c9f718..3ea7feca439e7ae9a610b6b3139ddbfad3ac6f9c
> 100644
> --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> @@ -22,6 +22,8 @@
>
> #include "LcdGraphicsOutputDxe.h"
>
> +#define BYTES_PER_PIXEL 4
> +
> /**********************************************************************
> *
> * This file contains all the bits of the Lcd that are
> @@ -66,10 +68,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;
> }
>
> @@ -77,7 +75,8 @@ LcdInitialize (
> *
> * @param ModeNumber Display mode number.
> * @retval EFI_SUCCESS Display set mode success.
> - * @retval EFI_DEVICE_ERROR If mode not found/supported.
> + * @retval EFI_DEVICE_ERROR Mode not found/supported.
> + * @retval !EFI_SUCCESS Other errors.
> **/
> EFI_STATUS
> LcdSetMode (
> @@ -87,9 +86,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 (
> @@ -104,13 +102,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));
Could we have some parentheses above, to clarify (intended) precedence?
>
> // Disable the controller
> MmioWrite32 (HDLCD_REG_COMMAND, HDLCD_DISABLE);
> @@ -118,10 +125,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/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> index
> 6de60491e9fd0c5bca71e743aac2862ff85f6e7e..4bad2367982e16d5d23c4eab2e6d91bf7db1c031
> 100644
> --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> @@ -80,6 +80,7 @@ LcdInitialize (
> * @param ModeNumber Display mode number.
> * @retval EFI_SUCCESS Display set mode success.
> * @retval EFI_DEVICE_ERROR If mode not found/supported.
> + @retval !EFI_SUCCESS Other errors.
> **/
> EFI_STATUS
> LcdSetMode (
> @@ -92,6 +93,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,
> @@ -111,6 +114,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);
>
> @@ -144,7 +154,11 @@ 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;
This is not a function call, so I do not believe the continuation line
should be indented.
/
Leif
> +
> + if (ModeInfo.PixelFormat == PixelBlueGreenRedReserved8BitPerColor) {
> + LcdControl |= PL111_CTRL_BGR;
> + }
>
> MmioWrite32 (PL111_REG_LCD_CONTROL, LcdControl);
>
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
