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

[Leif Lindholm <leif.lindholm@linaro.org>]

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
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  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?

> +
> +// 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
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  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.inf
> @@ -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
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  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
>