Re: [PATCH edk2-platforms v2 14/18] ARM/VExpressPkg: Reserving framebuffer at build

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 22 December 2017 at 19:08,  <evan.lloyd@arm.com> wrote:
> From: Girish Pathak <girish.pathak at arm.com>
>
> This change uses two PCDs, PcdArmLcdFrameBufferBase and
> PcdArmLcdFrameBufferSize introduced in correspondiong EDK2 patch to
> reserve framebuffer in DRAM if these values are defined in platform
> specific DSC file, avoiding the need to allocate dynamically.
> This allows the framebuffer to appear as "I/O memory" outside of the
> normal RAM map, which is similar to the "VRAM" case.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> ---
>  Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf |  4 +++-
>  Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c          | 20 ++++++++++++++++++--
>  2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf b/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf
> index 4cbd2ff4b4faf11ccd4fe30117708d7cc4c1bf0e..c70c4ce64826174e6d15611de640ad3b902835b9 100644
> --- a/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf
> +++ b/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf
> @@ -1,5 +1,5 @@
>  #/* @file
> -#  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
> +#  Copyright (c) 2011-2017, ARM Limited. All rights reserved.
>  #
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD License
> @@ -57,6 +57,8 @@ [FixedPcd]
>    gArmTokenSpaceGuid.PcdArmPrimaryCore
>
>    gArmPlatformTokenSpaceGuid.PcdCoreCount
> +  gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferBase
> +  gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferSize
>

As mentioned in the review of the other patch, please move these into
VExpressPkg

>  [Ppis]
>    gArmMpCoreInfoPpiGuid
> diff --git a/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> index 6379e81751fca5e7972c5c30f305be65fd1ae71d..1e8f3205312ebf30fa1666130bc944261384359a 100644
> --- a/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> +++ b/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2011-2016, ARM Limited. All rights reserved.
> +*  Copyright (c) 2011-2017, ARM Limited. All rights reserved.
>  *
>  *  This program and the accompanying materials
>  *  are licensed and made available under the terms and conditions of the BSD License
> @@ -20,8 +20,10 @@
>  #include <Library/MemoryAllocationLib.h>
>  #include <ArmPlatform.h>
>
> +#define FRAME_BUFFER_DESCRIPTOR ((FixedPcdGet64 (PcdArmLcdDdrFrameBufferBase) != 0) ? 1 : 0)
> +
>  // Number of Virtual Memory Map Descriptors
> -#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          9
> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS (9 + FRAME_BUFFER_DESCRIPTOR)
>
>  // DDR attributes
>  #define DDR_ATTRIBUTES_CACHED   ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
> @@ -142,6 +144,20 @@ ArmPlatformGetVirtualMemoryMap (
>    //
>    VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
>
> +  // Map region for the frame buffer in the system RAM
> +#if (FixedPcdGet32 (PcdArmLcdDdrFrameBufferSize) != 0)
> +  VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64 (PcdArmLcdDdrFrameBufferBase);
> +  VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdArmLcdDdrFrameBufferBase);
> +  VirtualMemoryTable[Index].Length = FixedPcdGet32 (PcdArmLcdDdrFrameBufferSize);
> +  // Map as Normal Non-Cacheable memory, so that we can use the accelerated
> +  // SetMem/CopyMem routines that may use unaligned accesses or
> +  // DC ZVA instructions. If mapped as device memory, these routine may cause
> +  // alignment faults.
> +  // NOTE: The attribute value is misleading, it indicates memory map type as
> +  // an un-cached, un-buffered but allows buffering and reordering.
> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> +#endif
> +

Whose responsibility is it to ensure that this region is removed from
SystemMemoryBase/SystemMemorySize? If it is up to the .DSC author,
could you please add an ASSERT() here that they don't overlap?

>    // Map sparse memory region if present
>    if (HasSparseMemory) {
>      VirtualMemoryTable[++Index].PhysicalBase = SparseMemoryBase;
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>