Re: [PATCH edk2-platforms 07/27] Silicon/NXP: Add i.MX display library support

[Chris Co <Christopher.Co@microsoft.com>]

Hi Leif,

> -----Original Message-----
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Sent: Thursday, November 1, 2018 11:05 AM
> To: Chris Co <Christopher.Co@microsoft.com>
> Cc: edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Michael D Kinney <michael.d.kinney@intel.com>
> 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 <christopher.co@microsoft.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > ---
> >  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&amp;data=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C06e5c1f1adef43f79f9008d6402494f7%7C72f988bf86
> f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636766923203571004&amp;sdata=6GJ
> qphnDZ
> > +gT%2FGOiEeSSnPeTRGiRn%2B9CB%2Fi4A0ilq3MA%3D&amp;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&amp;data=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C06e5c1f1adef43f79f9008d6402494f7%7C72f988bf86
> f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636766923203571004&amp;sdata=6GJ
> qphnDZ
> > +gT%2FGOiEeSSnPeTRGiRn%2B9CB%2Fi4A0ilq3MA%3D&amp;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&amp;data=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C06e5c1f1adef43f79f9008d6402494f7%7C72f988bf86
> f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636766923203581009&amp;sdata=s3Y
> tp2LL8
> > +T9AkKX6X%2FK2QCn3%2FXgj5wqjS%2BIE9EBYwzk%3D&amp;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
> >