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 274DB222F4E29 for ; Sat, 23 Dec 2017 07:55:33 -0800 (PST) Received: by mail-it0-x242.google.com with SMTP id r6so17586612itr.3 for ; Sat, 23 Dec 2017 08:00:24 -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=8Zqsx5oRL2lezEs+iUSd2NvfzK5YHQZ+UBWpnt86L8A=; b=dpsA+NCJCg1u3F9EF7xpWu+omvDL8r7QqaDP8/SXEm1nBva29teiQ0I7RH+EpZPfFz 9j/H6IDJx973Q1itfOfxFiS7s++CaAQQPYE3kfvSot9JlYK1dDhbhwKgQg8cz3zkq6n9 Bp6OcZ5mqGhI90BIF4dhKtDQveJ6hERAFnpIs= 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=8Zqsx5oRL2lezEs+iUSd2NvfzK5YHQZ+UBWpnt86L8A=; b=SjSphynS08P96TPPPFipIU4sFn1Rq1/6u5sdal091lUV/d+sAErcRnrcCYnjXJuUPI Zt6YJegau1PFTWvTtBZlJADZXsE08FmM74urwKzwpLFApo7f9seooYWIenJbabJv2eVF jHynjrglFBu9h88Fjh1v9HM+d0sGo9bf1V0xrOJzipDWnetRgbhnkScT5vHMpUKr2RaX tPgJpCsxJNKC+tkZkinKZaSrUBEm6ZAVclQst9GXPd0dGlxH7SvAZj47kXhdTiYtWtq5 mSnsqufwAcDgryxYnrZVBwlEuCPDgpebWPW76EC+ZVcMpe7yq8H6fbTUFUrxxMxej7aS a5eQ== X-Gm-Message-State: AKGB3mLYQnuPm7IScgUTaT0ubCMH/+/WOPdANHfuY54wxdObFhHQHk+5 j0aDY7P7GywDVuyxJQv/QJZb2Hx45ERpp6XkR48Oy3XFW94= X-Google-Smtp-Source: ACJfBosgFRxDTBAy2RpAqGeat4/gGyFD0gGUYWeFyR9fk7KMQz/+UejBaeYNixo0IJUljm9jftyeUHyOFpBu5chDDaA= X-Received: by 10.36.71.83 with SMTP id t80mr21520695itb.48.1514044823614; Sat, 23 Dec 2017 08:00:23 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.52.14 with HTTP; Sat, 23 Dec 2017 08:00:23 -0800 (PST) In-Reply-To: <20171222190821.12440-14-evan.lloyd@arm.com> References: <20171222190821.12440-1-evan.lloyd@arm.com> <20171222190821.12440-14-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Sat, 23 Dec 2017 16:00:23 +0000 Message-ID: To: Evan Lloyd Cc: "edk2-devel@lists.01.org" , Arvind Chauhan , Daniil Egranov , Thomas Panakamattam Abraham , <"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 edk2-platforms v2 13/18] ARM/VExpressPkg: PL111 and HDLCD: 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 15:55:33 -0000 Content-Type: text/plain; charset="UTF-8" On 22 December 2017 at 19:08, 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 > > LcdPlatformLib for PL111, LcdPlatformQueryMode function 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 > Signed-off-by: Evan Lloyd > --- > Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf | 1 + > Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf | 1 + > Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c | 23 ++++++++---- > Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c | 38 +++++++++----------- > 4 files changed, 35 insertions(+), 28 deletions(-) > > diff --git a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf > index 2e83736609cf8c63cb498368cd377f6a23964ef4..4cbd324338be76a0b0bfb811159d893628e74155 100644 > --- a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf > +++ b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf > @@ -41,3 +41,4 @@ [Protocols] > > [FixedPcd] > gArmVExpressTokenSpaceGuid.PcdHdLcdVideoModeOscId > + gArmPlatformTokenSpaceGuid.PcdGopPixelFormat > diff --git a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf > index 1ee878041559fa84a810f65175f2507bda663d76..20045380149241ce14f41bcb70afcb8145fe821f 100644 > --- a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf > +++ b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf > @@ -42,3 +42,4 @@ [Protocols] > [FixedPcd] > gArmVExpressTokenSpaceGuid.PcdPL111LcdMaxMode > gArmVExpressTokenSpaceGuid.PcdPL111LcdVideoModeOscId > + gArmPlatformTokenSpaceGuid.PcdGopPixelFormat > diff --git a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c > index b448d70f86d6acbc6bdae9749c7b6d0205935ba7..f1c18ac955f621e9eab3dede86758f5f1b1a791d 100644 > --- a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c > +++ b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c > @@ -15,7 +15,6 @@ > #include > > #include > -#include > #include > #include > #include > @@ -93,6 +92,10 @@ EFI_EDID_ACTIVE_PROTOCOL mEdidActive = { > @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 > @@ -101,6 +104,17 @@ LcdPlatformInitializeDisplay ( > ) > { > EFI_STATUS Status; > + EFI_GRAPHICS_PIXEL_FORMAT PixelFormat; > + > + // PixelBitMask and PixelBltOnly pixel formats are not supported > + PixelFormat = FixedPcdGet32 (PcdGopPixelFormat); > + if (PixelFormat != PixelRedGreenBlueReserved8BitPerColor > + && PixelFormat != PixelBlueGreenRedReserved8BitPerColor) { > + Please fix the weird indentation here > + ASSERT (PixelFormat == PixelRedGreenBlueReserved8BitPerColor > + || PixelFormat == PixelBlueGreenRedReserved8BitPerColor); and here (just put the boolean operator on the previous line) > + return EFI_UNSUPPORTED; > + } > > // Set the FPGA multiplexer to select the video output from the > // motherboard or the daughterboard > @@ -285,12 +299,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/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c > index 439cbdb1a73145fc4dc9c3c9587ce3fd9b9fff56..16a74c76cf27526493165dc6d81384f752fd3f20 100644 > --- a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c > +++ b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c > @@ -149,7 +149,12 @@ EFI_EDID_ACTIVE_PROTOCOL mEdidActive = { > /** PL111 Platform specific initialization function. > > @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 > @@ -158,6 +163,17 @@ LcdPlatformInitializeDisplay ( > ) > { > EFI_STATUS Status; > + EFI_GRAPHICS_PIXEL_FORMAT PixelFormat; > + > + // PixelBitMask and PixelBltOnly pixel formats are not supported > + PixelFormat = FixedPcdGet32 (PcdGopPixelFormat); > + if (PixelFormat != PixelRedGreenBlueReserved8BitPerColor > + && PixelFormat != PixelBlueGreenRedReserved8BitPerColor) { > + > + ASSERT (PixelFormat == PixelRedGreenBlueReserved8BitPerColor > + || PixelFormat == PixelBlueGreenRedReserved8BitPerColor); Same here > + return EFI_UNSUPPORTED; > + } > > // Set the FPGA multiplexer to select the video output from the motherboard > // or the daughterboard > @@ -360,27 +376,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; > } > -- > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759") > With the above fixed Reviewed-by: Ard Biesheuvel