Re: [PATCH edk2-platforms v2 15/18] ARM/VExpressPkg: New DP500/DP550/DP650 platform library.

[Evan Lloyd <Evan.Lloyd@arm.com>]

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: 23 December 2017 16:07
> To: Evan Lloyd <Evan.Lloyd@arm.com>
> Cc: edk2-devel@lists.01.org; Arvind Chauhan <Arvind.Chauhan@arm.com>;
> Daniil Egranov <Daniil.Egranov@arm.com>; Thomas Abraham
> <thomas.abraham@arm.com>; "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 15/18] ARM/VExpressPkg: New
> DP500/DP550/DP650 platform library.
>
> On 22 December 2017 at 19:08,  <evan.lloyd@arm.com> wrote:
> > From: Girish Pathak <girish.pathak at arm.com>
> >
...
> > +
> > +/* Internal helper function to allocate memory if memory is not
> > +already
> > +  reserved for framebuffer
> > +
> > +  @param[in]  VramSize          Requested framebuffer size in bytes.
> > +  @param[out] VramBaseAddress   Pointer to memory allocated for
> framebuffer.
> > +
> > +  @retval EFI_SUCCESS           Framebuffer memory allocated successfully.
> > +  @retval EFI_UNSUPPORTED       Allocated address wider than 40 bits.
> > +  @retval !EFI_SUCCESS          Other errors.
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +GetVram (
> > +  IN  UINTN                 CONST  VramSize,
> > +  OUT EFI_PHYSICAL_ADDRESS *CONST  VramBaseAddress
> > +  )
> > +{
> > +  EFI_STATUS Status;
> > +
> > +  // Check if memory is already reserved for the framebuffer.
> > +#if (FixedPcdGet64 (PcdArmLcdDdrFrameBufferBase) != 0)
> > +
>
> Please don't use CPP conditionals for control flow
>
> > +#if (!DP_VALID_BASE_ADDR (FixedPcdGet64
> > +(PcdArmLcdDdrFrameBufferBase))) #error ARM Mali DP framebuffer
> base address cannot be wider than 40 bits.
> > +#else
> > +  *VramBaseAddress =
> > +    (EFI_PHYSICAL_ADDRESS)FixedPcdGet64
> > +(PcdArmLcdDdrFrameBufferBase);
> > +
> > +  Status = EFI_SUCCESS;
> > +#endif
> > +
> > +#else
> > +  // If not already reserved, attempt to allocate the VRAM from the
> DRAM.
> > +  Status = gBS->AllocatePages (
> > +                  AllocateAnyPages,
> > +                  EfiBootServicesData,
> > +                  EFI_SIZE_TO_PAGES (*VramSize),
> > +                  VramBaseAddress
> > +                  );
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "ArmMaliDpLib: Failed to allocate
> framebuffer.\n"));
> > +    ASSERT (FALSE);
> > +    return Status;
> > +  }
> > +
> > +  // ARM Mali DP framebuffer base address can not be wider than 40 bits.
> > +  if (!DP_VALID_BASE_ADDR (*VramBaseAddress)) {
> > +    gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES
> (*VramSize));
> > +    ASSERT (DP_VALID_BASE_ADDR (*VramBaseAddress));
> > +    return EFI_UNSUPPORTED;
> > +  }
> > +
> > +  // Mark the VRAM as write-combining. The VRAM is inside the DRAM,
> > + which is  // cacheable, for ARM/AArch64 EFI_MEMORY_WC memory is
> actually uncached.
> > +  Status = gDS->SetMemorySpaceAttributes (
> > +                  *VramBaseAddress,
> > +                  *VramSize,
> > +                  EFI_MEMORY_WC
>
> Please add EFI_MEMORY_XP here
>

 [[Evan Lloyd]] We can do that, happily.  However, in looking at this we found that the UEFI spec has in "2.3.6 AArch64 Platforms", section "2.3.6.1 Memory types":
EFI_MEMORY_XP, ...                                                                             Not used or defined

Does that suggest we need a minor spec update?

> > +                  );
> > +  if (EFI_ERROR (Status)) {
> > +    ASSERT (FALSE);
> > +    gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES
> (*VramSize));
> > +  }
> > +#endif
> > +
> > +  return Status;
> > +}
> > +
> > +/** Allocate VRAM memory in DRAM for the framebuffer
> > +  (unless it is reserved already).
> > +
> > +  The allocated address can be used to set the framebuffer as a base
> > + buffer  address for any layer of the ARM Mali DP.
> > +
> > +  @param[out] VramBaseAddress     A pointer to the framebuffer
> address.
> > +  @param[out] VramSize            A pointer to the size of the frame
> > +                                  buffer in bytes
> > +
> > +  @retval EFI_SUCCESS             Frame buffer memory allocation success.
> > +  @retval EFI_UNSUPPORTED         Allocated address wider than 40 bits
> > +  @retval !EFI_SUCCESS            Other errors.
> > +**/
> > +EFI_STATUS
> > +LcdPlatformGetVram (
> > +  OUT EFI_PHYSICAL_ADDRESS * CONST VramBaseAddress,
> > +  OUT UINTN                * CONST VramSize
> > +  )
> > +{
> > +  ASSERT (VramBaseAddress != NULL);
> > +  ASSERT (VramSize != NULL);
> > +
> > +  // Set the VRAM size.
> > +  *VramSize = (UINTN)FixedPcdGet32 (PcdArmLcdDdrFrameBufferSize);
> > +
> > +  return GetVram (*VramSize, VramBaseAddress); }
> > +
> > +/** Return total number of modes supported.
> > +
> > +  Note: Valid mode numbers are 0 to MaxMode - 1  See Section 12.9 of
> > + the UEFI Specification 2.7
> > +
> > +  @retval UINT32             Mode Number.
> > +**/
> > +UINT32
> > +LcdPlatformGetMaxMode (VOID)
> > +{
> > +  return  mMaxMode;
> > +}
> > +
> > +/** Set the requested display mode.
> > +
> > +  @param[in] ModeNumber            Mode Number.
> > +
> > +  @retval EFI_SUCCESS              Mode set successful.
> > +  @retval EFI_INVALID_PARAMETER    Requested mode not found.
> > +**/
> > +EFI_STATUS
> > +LcdPlatformSetMode (
> > +  IN CONST UINT32 ModeNumber
> > +  )
> > +{
> > +
> > +  if (ModeNumber >= mMaxMode) {
> > +    ASSERT (ModeNumber < mMaxMode);
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  // On models, platform specific clock/mux settings are not required.
> > +  // Display controller specific settings for Mali DP are done in
> LcdSetMode.
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/** Return information for the requested mode number.
> > +
> > +  @param[in]  ModeNumber          Mode Number.
> > +  @param[out] Info                Pointer for returned mode information
> > +                                  (on success).
> > +
> > +  @retval EFI_SUCCESS             Display mode information of the
> requested
> > +                                  mode returned successfully.
> > +**/
> > +EFI_STATUS
> > +LcdPlatformQueryMode (
> > +  IN  CONST UINT32                                 ModeNumber,
> > +  OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info
> > +  )
> > +{
> > +  ASSERT (ModeNumber < mMaxMode);
> > +  ASSERT (Info != NULL);
> > +
> > +  Info->Version = 0;
> > +  Info->HorizontalResolution =
> > + mDisplayModes[ModeNumber].Horizontal.Resolution;
> > +  Info->VerticalResolution =
> > + mDisplayModes[ModeNumber].Vertical.Resolution;
> > +  Info->PixelsPerScanLine =
> > + mDisplayModes[ModeNumber].Horizontal.Resolution;
> > +
> > +  Info->PixelFormat = FixedPcdGet32 (PcdGopPixelFormat);
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/** Return the display timing information for the requested mode
> number.
> > +
> > +  @param[in]  ModeNumber         Mode Number.
> > +
> > +  @param[out] Horizontal         Pointer to horizontal timing parameters.
> > +                                 (Resolution, Sync, Back porch, Front porch)
> > +  @param[out] Vertical           Pointer to vertical timing parameters.
> > +                                 (Resolution, Sync, Back porch, Front
> > + porch)
> > +
> > +  @retval EFI_SUCCESS             Display timing information of the
> requested
> > +                                  mode returned successfully.
> > +  @retval EFI_INVALID_PARAMETER   Requested mode not found.
> > +**/
> > +EFI_STATUS
> > +LcdPlatformGetTimings (
> > +  IN  UINT32                ModeNumber,
> > +  OUT CONST SCAN_TIMINGS ** Horizontal,
> > +  OUT CONST SCAN_TIMINGS ** Vertical
> > +  )
> > +{
> > +  ASSERT (Horizontal != NULL);
> > +  ASSERT (Vertical != NULL);
> > +
> > +  if (ModeNumber >= mMaxMode) {
> > +    ASSERT (ModeNumber < mMaxMode);
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  *Horizontal = &mDisplayModes[ModeNumber].Horizontal;
> > +  *Vertical = &mDisplayModes[ModeNumber].Vertical;
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/** Return bits per pixel information for a mode number.
> > +
> > +  @param[in]  ModeNumber          Mode Number.
> > +  @param[out] Bpp                 Pointer to value Bytes Per Pixel.
> > +
> > +  @retval EFI_SUCCESS             Bits per pixel information of the display
> > +                                  mode returned successfully.
> > +  @retval EFI_INVALID_PARAMETER   Requested mode not found.
> > +**/
> > +EFI_STATUS
> > +LcdPlatformGetBpp (
> > +  IN  CONST UINT32    ModeNumber,
> > +  OUT LCD_BPP * CONST Bpp
> > +  )
> > +{
> > +  ASSERT (Bpp != NULL);
> > +  if (ModeNumber >= mMaxMode) {
> > +    // Check valid ModeNumber.
> > +    ASSERT (ModeNumber < mMaxMode);
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  *Bpp = LCD_BITS_PER_PIXEL_24;
> > +
> > +  return EFI_SUCCESS;
> > +}
> > diff --git
> >
> a/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> >
> b/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> > index
> >
> 1e8f3205312ebf30fa1666130bc944261384359a..f0b482f91a84d91ac9914
> c18b794
> > 1dd32ab8c8a7 100644
> > ---
> a/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> > +++
> b/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> > @@ -21,9 +21,10 @@
> >  #include <ArmPlatform.h>
> >
> >  #define FRAME_BUFFER_DESCRIPTOR ((FixedPcdGet64
> > (PcdArmLcdDdrFrameBufferBase) != 0) ? 1 : 0)
> > +#define DP_BASE_DESCRIPTOR      ((FixedPcdGet64
> (PcdArmMaliDpBase) != 0) ? 1 : 0)
> >
> >  // Number of Virtual Memory Map Descriptors -#define
> > MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS (9 +
> FRAME_BUFFER_DESCRIPTOR)
> > +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS (9 +
> > +FRAME_BUFFER_DESCRIPTOR + DP_BASE_DESCRIPTOR)
> >
> >  // DDR attributes
> >  #define DDR_ATTRIBUTES_CACHED
> ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
> > @@ -158,6 +159,13 @@ ArmPlatformGetVirtualMemoryMap (
> >    VirtualMemoryTable[Index].Attributes =
> > ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> >  #endif
> >
> > +#if (FixedPcdGet64 (PcdArmMaliDpBase) != 0)
> > +  // DP500/DP550/DP650 peripheral memory region
> > +  VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64
> > +(PcdArmMaliDpBase);
> > +  VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64
> > +(PcdArmMaliDpBase);
> > +  VirtualMemoryTable[Index].Length = FixedPcdGet32
> > +(PcdArmMaliDpMemoryRegionLength);
> > +  VirtualMemoryTable[Index].Attributes =
> > +ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> > +#endif
> >    // Map sparse memory region if present
> >    if (HasSparseMemory) {
> >      VirtualMemoryTable[++Index].PhysicalBase = SparseMemoryBase;
> > --
> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.