From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::242; helo=mail-it0-x242.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x242.google.com (mail-it0-x242.google.com [IPv6:2607:f8b0:4001:c0b::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id CDBA922198F6C for ; Sat, 23 Dec 2017 05:24:49 -0800 (PST) Received: by mail-it0-x242.google.com with SMTP id f190so17325541ita.5 for ; Sat, 23 Dec 2017 05:29:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=AqxGdWXKQGv1D1sLhnCO+/2jrGoKfJJY9hYKHXccUr4=; b=R2Dp3q/wabFUolSXe8JsDEvbYdzHtC27/4fqgoMjZB2TNG/DgXoZZQ5o+e3d2ja+SH XSZ1f5JXggrkgfYff7Jr0OhwJVwuYHyBawvYhYmn/fX7rupYfNE5+C7/FfUSn/HuTHxa 86H3WfGw68LvWtVPK/LTP/74201B943ShEdNs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=AqxGdWXKQGv1D1sLhnCO+/2jrGoKfJJY9hYKHXccUr4=; b=oOA1HfYc4mpisPXbWKVqWxYq94VzkSBve0dC3bN1/7fAJszz8o7N38sy//B5Lq8AeC 1fhAzIaFt6RbZzQ8dchvhcncTCQfui6G5taanS8th3bntrVhDN2r6Ok13qgXAzmTd8kY HyGdygK32cCnnTsx1JUKHpfyg1fsxljflJHaw5k1zdceJjqROoHa9snXMSPagHVgh7mm URDgiKuHDGXCVNrpwa6NCUVHQIyPJwcr7fk05zGP3GARPblvmW2vylLW3dpxNCGB7MH1 F4k2Sq40+0e4C05K2Z40B/GTg7XgLpG7L+ngqCYYIQhPo0Vg0Ee4uDjgF0g7D5C6Fael DBVw== X-Gm-Message-State: AKGB3mIUIyEQldkvgFItxGnNDu+4+eU3fvfztZnhnqCHjY6vnutqNSq6 WeMlI1bnrk9uWkLkDKFTRLacILDBZguDvpvAjcfK0fTz1Cs= X-Google-Smtp-Source: ACJfBovxDtMOWfekKnl7VRMd5zIgqtuYYvZj0tGgPs/iPHi1GJaFrmGRPgkhCNq4LtxOYcL6vI7tZAwKnDIw2gu7gBc= X-Received: by 10.36.78.212 with SMTP id r203mr21636044ita.58.1514035779943; Sat, 23 Dec 2017 05:29:39 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.52.14 with HTTP; Sat, 23 Dec 2017 05:29:39 -0800 (PST) In-Reply-To: <20171222183418.8616-9-evan.lloyd@arm.com> References: <20171222183418.8616-1-evan.lloyd@arm.com> <20171222183418.8616-9-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Sat, 23 Dec 2017 13:29:39 +0000 Message-ID: To: Evan Lloyd Cc: "edk2-devel@lists.01.org" , <"ard.biesheuvel@linaro.org"@arm.com>, <"leif.lindholm@linaro.org"@arm.com>, <"Matteo.Carlini@arm.com"@arm.com>, <"nd@arm.com"@arm.com> Subject: Re: [PATCH v2 08/13] ArmPlatformPkg: Add PCD to select pixel format X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 23 Dec 2017 13:24:50 -0000 Content-Type: text/plain; charset="UTF-8" On 22 December 2017 at 18:34, wrote: > 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 > 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 > Signed-off-by: Evan Lloyd Reviewed-by: Ard Biesheuvel > --- > 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") >