Hi Leif,
> -----Original Message-----
> From: Leif Lindholm <[email protected]>
> Sent: Thursday, November 1, 2018 11:05 AM
> To: Chris Co <[email protected]>
> Cc: [email protected]; Ard Biesheuvel <[email protected]>;
> Michael D Kinney <[email protected]>
> Subject: Re: [PATCH edk2-platforms 07/27] Silicon/NXP: Add i.MX display
> library support
>
> On Fri, Sep 21, 2018 at 08:25:58AM +0000, Chris Co wrote:
> > This adds support for processing EDID data on NXP i.MX platforms.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Christopher Co <[email protected]>
> > Cc: Ard Biesheuvel <[email protected]>
> > Cc: Leif Lindholm <[email protected]>
> > Cc: Michael D Kinney <[email protected]>
> > ---
> > Silicon/NXP/iMXPlatformPkg/Include/iMXDisplay.h | 114
> +++++++++++++++
> > Silicon/NXP/iMXPlatformPkg/Library/iMXDisplayLib/iMXDisplayLib.c | 152
> ++++++++++++++++++++
> > Silicon/NXP/iMXPlatformPkg/Library/iMXDisplayLib/iMXDisplayLib.inf |
> > 31 ++++
> > 3 files changed, 297 insertions(+)
> >
> > diff --git a/Silicon/NXP/iMXPlatformPkg/Include/iMXDisplay.h
> > b/Silicon/NXP/iMXPlatformPkg/Include/iMXDisplay.h
> > new file mode 100644
> > index 000000000000..70ef8d0af97f
> > --- /dev/null
> > +++ b/Silicon/NXP/iMXPlatformPkg/Include/iMXDisplay.h
> > @@ -0,0 +1,114 @@
> > +/** @file
> > +*
> > +* Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> > +*
> > +* This program and the accompanying materials
> > +* are licensed and made available under the terms and conditions of
> > +the BSD License
> > +* which accompanies this distribution. The full text of the license
> > +may be found at
> > +*
> > +https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopens
> > +ource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C06e5c1f1adef43f79f9008d6402494f7%7C72f988bf86
> f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636766923203571004&sdata=6GJ
> qphnDZ
> > +gT%2FGOiEeSSnPeTRGiRn%2B9CB%2Fi4A0ilq3MA%3D&reserved=0
> > +*
> > +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > +BASIS,
> > +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> > +*
> > +**/
> > +
> > +#ifndef __IMX_DISPLAY_H__
> > +#define __IMX_DISPLAY_H__
> > +
> > +#define EDID_MIN_SIZE 128
> > +#define EDID_I2C_ADDRESS 0x50
>
> Are all of these #defines and functions called only from within iMX platform
> code?
>
These defines and functions are called by iMX platform and silicon (iMX6Pkg)
code. Should I prefix these defines with "IMX_" and function names with "Imx"?
I made changes to address the rest of the feedback.
Chris
> > +
> > +// The first DTD is the preferred timing, refer to 3.1 VESA EDID spec.
> > +#define EDID_DTD_1_OFFSET 0x36
> > +#define EDID_DTD_2_OFFSET 0x48
> > +#define EDID_DTD_3_OFFSET 0x5A
> > +#define EDID_DTD_4_OFFSET 0x6C
> > +
> > +typedef enum {
> > + PIXEL_FORMAT_ARGB32,
> > + PIXEL_FORMAT_BGRA32,
> > +} PIXEL_FORMAT;
> > +
> > +typedef struct _DISPLAY_TIMING {
> > + UINT32 PixelClock;
> > + UINT32 HActive;
> > + UINT32 HBlank;
> > + UINT32 VActive;
> > + UINT32 VBlank;
> > + UINT32 HSync;
> > + UINT32 VSync;
> > + UINT32 HSyncOffset;
> > + UINT32 VSyncOffset;
> > + UINT32 HImageSize;
> > + UINT32 VImageSize;
> > + UINT32 HBorder;
> > + UINT32 VBorder;
> > + UINT32 EdidFlags;
> > + UINT32 Flags;
> > + UINT32 PixelRepetition;
> > + UINT32 Bpp;
> > + PIXEL_FORMAT PixelFormat;
> > +} DISPLAY_TIMING, *PDISPLAY_TIMING, DTD;
> > +
> > +typedef struct _DETAILED_TIMING_DESCRIPTOR {
> > + UINT8 PixelClock[2];
> > + UINT8 HActive;
> > + UINT8 HBlank;
> > + UINT8 HActiveBlank;
> > + UINT8 VActive;
> > + UINT8 VBlank;
> > + UINT8 VActiveBlank;
> > + UINT8 HSyncOffset;
> > + UINT8 HSyncWidth;
> > + UINT8 VSyncOffsetWidth;
> > + UINT8 HVOffsetWidth;
> > + UINT8 HImageSize;
> > + UINT8 VImageSize;
> > + UINT8 HVImageSize;
> > + UINT8 HBorder;
> > + UINT8 VBorder;
> > + UINT8 EdidFlags;
> > +} DETAILED_TIMING_DESCRIPTOR, *PDETAILED_TIMING_DESCRIPTOR;
> > +
> > +/**
> > + Convert detailed timing descriptor to display timing format
> > +
> > + @param[in] DTDPtr Pointer to detailed timing descriptor.
> > + @param[out] DisplayTimingPtr Pointer to display timing structure.
> > +
> > + @retval EFI_SUCCESS Detailed timing descriptor data was converted.
> > +
> > +**/
> > +EFI_STATUS
> > +ConvertDTDToDisplayTiming (
> > + IN DETAILED_TIMING_DESCRIPTOR *DTDPtr,
> > + OUT DISPLAY_TIMING *DisplayTimingPtr
> > + );
> > +
> > +/**
> > + Debug dump of Display Timing structure
> > +
> > + @param[in] DisplayTimingNamePtr Name of display timing structure.
> > + @param[in] DisplayTimingPtr Pointer to display timing structure.
> > +**/
> > +VOID
> > +PrintDisplayTiming (
> > + IN CHAR8 *DisplayTimingNamePtr,
> > + IN DISPLAY_TIMING *DisplayTimingPtr
> > + );
> > +
> > +/**
> > + Check if EDID is valid
> > +
> > + @param[in] EdidDataPtr Pointer to EDID data.
> > +
> > + @retval EFI_SUCCESS EDID data is a valid EDID.
> > + @retval EFI_INVALID_PARAMETER EDID data is invalid.
> > +
> > +**/
> > +EFI_STATUS
> > +ValidateEdidData (
> > + IN UINT8 *EdidDataPtr
> > + );
> > +
> > +#endif // __IMX_DISPLAY_H__
> > diff --git
> > a/Silicon/NXP/iMXPlatformPkg/Library/iMXDisplayLib/iMXDisplayLib.c
> > b/Silicon/NXP/iMXPlatformPkg/Library/iMXDisplayLib/iMXDisplayLib.c
> > new file mode 100644
> > index 000000000000..9e90ece96260
> > --- /dev/null
> > +++ b/Silicon/NXP/iMXPlatformPkg/Library/iMXDisplayLib/iMXDisplayLib.c
> > @@ -0,0 +1,152 @@
> > +/** @file
> > +*
> > +* Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> > +*
> > +* This program and the accompanying materials
> > +* are licensed and made available under the terms and conditions of
> > +the BSD License
> > +* which accompanies this distribution. The full text of the license
> > +may be found at
> > +*
> > +https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopens
> > +ource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C06e5c1f1adef43f79f9008d6402494f7%7C72f988bf86
> f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636766923203571004&sdata=6GJ
> qphnDZ
> > +gT%2FGOiEeSSnPeTRGiRn%2B9CB%2Fi4A0ilq3MA%3D&reserved=0
> > +*
> > +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > +BASIS,
> > +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> > +*
> > +**/
> > +
> > +#include <Uefi.h>
> > +
> > +#include <Library/DebugLib.h>
> > +
> > +#include <iMXDisplay.h>
> > +
> > +/**
> > + Convert detailed timing descriptor to display timing format
> > +
> > + @param[in] DTDPtr Pointer to detailed timing descriptor.
> > + @param[out] DisplayTimingPtr Pointer to display timing structure.
> > +
> > + @retval EFI_SUCCESS Detailed timing descriptor data was converted.
> > +
> > +**/
> > +EFI_STATUS
> > +ConvertDTDToDisplayTiming (
> > + IN DETAILED_TIMING_DESCRIPTOR *DTDPtr,
> > + OUT DISPLAY_TIMING *DisplayTimingPtr
> > + )
> > +{
> > + UINT32 edidPixelClock;
>
> e -> E.
>
> > +
> > + DEBUG ((DEBUG_INFO, "++ConvertDTDToDisplayTiming()\r\n"));
>
> Please use %a and __FUNCTION__ instead of hard-coding function name.
>
> > + // Refer to 3.10.2 VESA EDID spec
> > + edidPixelClock = (DTDPtr->PixelClock[0] | (DTDPtr->PixelClock[1] <<
> > + 8)); DisplayTimingPtr->PixelClock = edidPixelClock * 10000;
> > + DisplayTimingPtr->HActive = (DTDPtr->HActiveBlank & 0xF0);
> > + DisplayTimingPtr->HActive = (DisplayTimingPtr->HActive << 4) |
> > + DTDPtr->HActive; DisplayTimingPtr->HBlank = (DTDPtr->HActiveBlank &
> > + 0x0F); DisplayTimingPtr->HBlank = (DisplayTimingPtr->HBlank << 8) |
> > + DTDPtr->HBlank; DisplayTimingPtr->VActive = (DTDPtr->VActiveBlank &
> > + 0xF0); DisplayTimingPtr->VActive = (DisplayTimingPtr->VActive << 4)
> > + | DTDPtr->VActive; DisplayTimingPtr->VBlank = (DTDPtr->VActiveBlank
> > + & 0x0F); DisplayTimingPtr->VBlank = (DisplayTimingPtr->VBlank << 8)
> > + | DTDPtr->VBlank; DisplayTimingPtr->HSyncOffset =
> > + (DTDPtr->HVOffsetWidth & 0xC0); DisplayTimingPtr->HSyncOffset =
> (DisplayTimingPtr->HSyncOffset << 2) |
> > + DTDPtr->HSyncOffset;
> > + DisplayTimingPtr->VSyncOffset = (DTDPtr->HVOffsetWidth & 0x0C);
> > + DisplayTimingPtr->VSyncOffset = (DisplayTimingPtr->VSyncOffset << 2) |
> > + ((DTDPtr->VSyncOffsetWidth & 0xF0)
> > + >> 4); DisplayTimingPtr->HSync = (DTDPtr->HVOffsetWidth & 0x30);
> > + DisplayTimingPtr->HSync = (DisplayTimingPtr->HSync << 4) |
> > + DTDPtr->HSyncWidth; DisplayTimingPtr->VSync =
> > + (DTDPtr->HVOffsetWidth & 0x03); DisplayTimingPtr->VSync =
> (DisplayTimingPtr->VSync << 4) |
> > + (DTDPtr->VSyncOffsetWidth & 0x0F);
> > + DisplayTimingPtr->HImageSize = ((DTDPtr->HVImageSize & 0xF0) << 4) |
> > + DTDPtr->HImageSize;
> > + DisplayTimingPtr->VImageSize = ((DTDPtr->HVImageSize & 0x0F) << 8) |
> > + DTDPtr->VImageSize;
>
> A lot of these above transforms are a fairly unreadable, but maybe they need
> to be?
>
> > + DisplayTimingPtr->HBorder = DTDPtr->HBorder;
> > + DisplayTimingPtr->VBorder = DTDPtr->VBorder;
> > + DisplayTimingPtr->EdidFlags = DTDPtr->EdidFlags;
> > + DisplayTimingPtr->Flags = 0;
>
> A #define for that 0?
>
> > +
> > + DEBUG ((DEBUG_INFO, "--ConvertDTDToDisplayTiming()=ok\r\n"));
>
> __FUNCTION__
>
> > + return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > + Debug dump of Display Timing structure
> > +
> > + @param[in] DisplayTimingNamePtr Name of display timing structure.
> > + @param[in] DisplayTimingPtr Pointer to display timing structure.
> > +**/
> > +VOID
> > +PrintDisplayTiming (
> > + IN CHAR8 *DisplayTimingNamePtr,
> > + IN DISPLAY_TIMING *DisplayTimingPtr
> > + )
> > +{
> > + DEBUG ((DEBUG_INFO, "**********************\n"));
> > + DEBUG ((DEBUG_INFO, "%a\n", DisplayTimingNamePtr));
> > + DEBUG ((DEBUG_INFO, "**********************\n"));
> > + DEBUG ((DEBUG_INFO, "PixelClock %d\n",
> > +DisplayTimingPtr->PixelClock));
> > + DEBUG ((DEBUG_INFO, "HActive %d\n", DisplayTimingPtr->HActive));
> > + DEBUG ((DEBUG_INFO, "HBlank %d\n", DisplayTimingPtr->HBlank));
> > + DEBUG ((DEBUG_INFO, "VActive %d\n", DisplayTimingPtr->VActive));
> > + DEBUG ((DEBUG_INFO, "VBlank %d\n", DisplayTimingPtr->VBlank));
> > + DEBUG ((DEBUG_INFO, "HSync %d\n", DisplayTimingPtr->HSync));
> > + DEBUG ((DEBUG_INFO, "VSync %d\n", DisplayTimingPtr->VSync));
> > + DEBUG ((DEBUG_INFO, "HSyncOffset %d\n",
> > +DisplayTimingPtr->HSyncOffset));
> > + DEBUG ((DEBUG_INFO, "VSyncOffset %d\n",
> > +DisplayTimingPtr->VSyncOffset));
> > + DEBUG ((DEBUG_INFO, "HBorder %d\n", DisplayTimingPtr->HBorder));
> > + DEBUG ((DEBUG_INFO, "VBorder %d\n", DisplayTimingPtr->VBorder));
> > + DEBUG ((DEBUG_INFO, "EdidFlags %d\n",
> > +DisplayTimingPtr->EdidFlags));
> > + DEBUG ((DEBUG_INFO, "Flags %d\n", DisplayTimingPtr->Flags));
> > + DEBUG ((DEBUG_INFO, "PixelRepetition %d\n",
> > +DisplayTimingPtr->PixelRepetition));
> > + DEBUG ((DEBUG_INFO, "BPP %d\n", DisplayTimingPtr->Bpp));
> > + DEBUG ((DEBUG_INFO, "PixelFormat %d\n",
> > +DisplayTimingPtr->PixelFormat));
> > + DEBUG ((DEBUG_INFO, "**********************\n")); }
> > +
> > +/**
> > + Check if EDID is valid
> > +
> > + @param[in] EdidDataPtr Pointer to EDID data.
> > +
> > + @retval EFI_SUCCESS EDID data is a valid EDID.
> > + @retval EFI_INVALID_PARAMETER EDID data is invalid.
> > +
> > +**/
> > +EFI_STATUS
> > +ValidateEdidData (
> > + IN UINT8 *EdidDataPtr
> > + )
> > +{
> > + UINT8 Checksum;
> > + UINT8 Index;
> > +
> > + DEBUG ((DEBUG_INFO, "++ValidateEdidData()\r\n"));
>
> __FUNCTION__
>
> > +
> > + if (EdidDataPtr[0] != 0x00 ||
> > + EdidDataPtr[1] != 0xFF ||
> > + EdidDataPtr[2] != 0xFF ||
> > + EdidDataPtr[3] != 0xFF ||
> > + EdidDataPtr[4] != 0xFF ||
> > + EdidDataPtr[5] != 0xFF ||
> > + EdidDataPtr[6] != 0xFF ||
> > + EdidDataPtr[7] != 0x00) {
> > + DEBUG ((DEBUG_ERROR, "Invalid EDID header\n"));
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + // Validate EDID checksum
> > + Checksum = 0;
> > + for (Index = 0; Index < EDID_MIN_SIZE; Index++) {
> > + Checksum += EdidDataPtr[Index];
> > + }
> > +
> > + if (Checksum != 0) {
> > + DEBUG ((DEBUG_ERROR, "Invalid EDID checksum\n"));
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + DEBUG ((DEBUG_INFO, "--ValidateEdidData()=ok\r\n"));
>
> __FUNCTION__
>
> /
> Leif
>
> > + return EFI_SUCCESS;
> > +}
> > diff --git
> > a/Silicon/NXP/iMXPlatformPkg/Library/iMXDisplayLib/iMXDisplayLib.inf
> > b/Silicon/NXP/iMXPlatformPkg/Library/iMXDisplayLib/iMXDisplayLib.inf
> > new file mode 100644
> > index 000000000000..bd77d4159639
> > --- /dev/null
> > +++ b/Silicon/NXP/iMXPlatformPkg/Library/iMXDisplayLib/iMXDisplayLib.i
> > +++ nf
> > @@ -0,0 +1,31 @@
> > +## @file
> > +#
> > +# Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> > +#
> > +# This program and the accompanying materials # are licensed and
> > +made available under the terms and conditions of the BSD License #
> > +which accompanies this distribution. The full text of the license
> > +may be found at #
> > +https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopens
> > +ource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C06e5c1f1adef43f79f9008d6402494f7%7C72f988bf86
> f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636766923203581009&sdata=s3Y
> tp2LL8
> > +T9AkKX6X%2FK2QCn3%2FXgj5wqjS%2BIE9EBYwzk%3D&reserved=0
> > +#
> > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > +BASIS, # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> > +#
> > +##
> > +
> > +[Defines]
> > + INF_VERSION = 0x0001001A
> > + BASE_NAME = iMXDisplayLib
> > + FILE_GUID = C0408490-F09B-4CFA-9A2F-5159F2705323
> > + MODULE_TYPE = BASE
> > + VERSION_STRING = 1.0
> > + LIBRARY_CLASS = iMXDisplayLib
> > +
> > +[Packages]
> > + EmbeddedPkg/EmbeddedPkg.dec
> > + MdePkg/MdePkg.dec
> > + Silicon/NXP/iMXPlatformPkg/iMXPlatformPkg.dec
> > +
> > +[LibraryClasses]
> > +
> > +[Sources.common]
> > + iMXDisplayLib.c
> > --
> > 2.16.2.gvfs.1.33.gf5370f1
> >
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
