[edk2] [PATCH 18/19] ArmPlatformPkg: Reserving framebuffer at build

evan.lloyd@arm.com posted 19 patches 7 years, 3 months ago
There is a newer version of this series
[edk2] [PATCH 18/19] ArmPlatformPkg: Reserving framebuffer at build
Posted by evan.lloyd@arm.com 7 years, 3 months ago
From: Girish Pathak <girish.pathak@arm.com>

Currently frame buffer memory is either reserved in special VRAM or
dynamically allocated using boot services memory allocation functions.
When allocated using boot services calls the memory has to be allocated
as EfiBootServicesData. Unfortunately failures have been seen with this
case.  There is also an unfortunate lack of control on the placement of
the frmae buffer.

This change introduces two PCDs, PcdArmLcdFrameBufferBase and
PcdArmLcdFrameBufferSize which enable build time reservation of the
frame buffer, avoiding the need to allocate dynamically.  This allows
the frame buffer to appear as "I/O memory" outside of the normal RAM
map, which is similar to the "VRAM" case.

This change has no impact on current code, only enables the option
of build time reservation of frame buffers.

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>
---
 ArmPlatformPkg/ArmPlatformPkg.dec                                           |  4 ++++
 ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf |  4 +++-
 ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c          | 21 ++++++++++++++++++--
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
index 77eb789ad8fe4ddcbf25abefad2e7b7d3d5e1722..0174f63e77f5b8430e106289366feb9a6577fb99 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dec
+++ b/ArmPlatformPkg/ArmPlatformPkg.dec
@@ -111,6 +111,10 @@ [PcdsFixedAtBuild.common]
   gArmPlatformTokenSpaceGuid.PcdPL111LcdBase|0x0|UINT32|0x00000026
   gArmPlatformTokenSpaceGuid.PcdArmHdLcdBase|0x0|UINT32|0x00000027
 
+  ## If set, frame buffer memory will be reserved and mapped in the system RAM
+  gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferSize|0x0|UINT32|0x00000033
+  gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferBase|0x0|UINT64|0x00000034
+
   ## PL180 MCI
   gArmPlatformTokenSpaceGuid.PcdPL180SysMciRegAddress|0x00000000|UINT32|0x00000028
   gArmPlatformTokenSpaceGuid.PcdPL180MciBaseAddress|0x00000000|UINT32|0x00000029
diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf
index 9b16f7f0c4731ab72bfb1008a073e81842bae82b..60789e9b8ff1b936db04953a765fb164b0e85a40 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf
+++ b/ArmPlatformPkg/ArmVExpressPkg/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
 
 [Ppis]
   gArmMpCoreInfoPpiGuid
diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
index 6379e81751fca5e7972c5c30f305be65fd1ae71d..5cd529750a3d2d3b0d381b58d875d378afaba2c2 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/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,21 @@ 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
+
   // Map sparse memory region if present
   if (HasSparseMemory) {
     VirtualMemoryTable[++Index].PhysicalBase = SparseMemoryBase;
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 18/19] ArmPlatformPkg: Reserving framebuffer at build
Posted by Leif Lindholm 7 years, 2 months ago
On Tue, Sep 26, 2017 at 09:15:28PM +0100, evan.lloyd@arm.com wrote:
> From: Girish Pathak <girish.pathak@arm.com>
> 
> Currently frame buffer memory is either reserved in special VRAM or
> dynamically allocated using boot services memory allocation functions.
> When allocated using boot services calls the memory has to be allocated
> as EfiBootServicesData. Unfortunately failures have been seen with this
> case.  There is also an unfortunate lack of control on the placement of
> the frmae buffer.
> 
> This change introduces two PCDs, PcdArmLcdFrameBufferBase and
> PcdArmLcdFrameBufferSize which enable build time reservation of the
> frame buffer, avoiding the need to allocate dynamically.  This allows
> the frame buffer to appear as "I/O memory" outside of the normal RAM
> map, which is similar to the "VRAM" case.
> 
> This change has no impact on current code, only enables the option
> of build time reservation of frame buffers.
> 
> 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>
> ---
>  ArmPlatformPkg/ArmPlatformPkg.dec                                           |  4 ++++
>  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf |  4 +++-
>  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c          | 21 ++++++++++++++++++--
>  3 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
> index 77eb789ad8fe4ddcbf25abefad2e7b7d3d5e1722..0174f63e77f5b8430e106289366feb9a6577fb99 100644
> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -111,6 +111,10 @@ [PcdsFixedAtBuild.common]
>    gArmPlatformTokenSpaceGuid.PcdPL111LcdBase|0x0|UINT32|0x00000026
>    gArmPlatformTokenSpaceGuid.PcdArmHdLcdBase|0x0|UINT32|0x00000027
>  
> +  ## If set, frame buffer memory will be reserved and mapped in the system RAM
> +  gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferSize|0x0|UINT32|0x00000033
> +  gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferBase|0x0|UINT64|0x00000034
> +
>    ## PL180 MCI
>    gArmPlatformTokenSpaceGuid.PcdPL180SysMciRegAddress|0x00000000|UINT32|0x00000028
>    gArmPlatformTokenSpaceGuid.PcdPL180MciBaseAddress|0x00000000|UINT32|0x00000029
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf
> index 9b16f7f0c4731ab72bfb1008a073e81842bae82b..60789e9b8ff1b936db04953a765fb164b0e85a40 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf
> +++ b/ArmPlatformPkg/ArmVExpressPkg/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
>  
>  [Ppis]
>    gArmMpCoreInfoPpiGuid
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> index 6379e81751fca5e7972c5c30f305be65fd1ae71d..5cd529750a3d2d3b0d381b58d875d378afaba2c2 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> +++ b/ArmPlatformPkg/ArmVExpressPkg/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,21 @@ 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.

Well done on that note, I was about to object, because I keep
forgetting about that twist :)

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> +   */
> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> +#endif
> +
>    // Map sparse memory region if present
>    if (HasSparseMemory) {
>      VirtualMemoryTable[++Index].PhysicalBase = SparseMemoryBase;
> -- 
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 18/19] ArmPlatformPkg: Reserving framebuffer at build
Posted by Ard Biesheuvel 7 years, 2 months ago
On 26 September 2017 at 21:15,  <evan.lloyd@arm.com> wrote:
> From: Girish Pathak <girish.pathak@arm.com>
>
> Currently frame buffer memory is either reserved in special VRAM or
> dynamically allocated using boot services memory allocation functions.
> When allocated using boot services calls the memory has to be allocated
> as EfiBootServicesData. Unfortunately failures have been seen with this
> case.  There is also an unfortunate lack of control on the placement of
> the frmae buffer.
>
> This change introduces two PCDs, PcdArmLcdFrameBufferBase and
> PcdArmLcdFrameBufferSize which enable build time reservation of the
> frame buffer, avoiding the need to allocate dynamically.  This allows
> the frame buffer to appear as "I/O memory" outside of the normal RAM
> map, which is similar to the "VRAM" case.
>
> This change has no impact on current code, only enables the option
> of build time reservation of frame buffers.
>

Where is the memory actually being reserved? And if it is reserved,
how can the OS reclaim it if it is not interested in using the GOP?


> 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>
> ---
>  ArmPlatformPkg/ArmPlatformPkg.dec                                           |  4 ++++
>  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf |  4 +++-
>  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c          | 21 ++++++++++++++++++--
>  3 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
> index 77eb789ad8fe4ddcbf25abefad2e7b7d3d5e1722..0174f63e77f5b8430e106289366feb9a6577fb99 100644
> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -111,6 +111,10 @@ [PcdsFixedAtBuild.common]
>    gArmPlatformTokenSpaceGuid.PcdPL111LcdBase|0x0|UINT32|0x00000026
>    gArmPlatformTokenSpaceGuid.PcdArmHdLcdBase|0x0|UINT32|0x00000027
>
> +  ## If set, frame buffer memory will be reserved and mapped in the system RAM
> +  gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferSize|0x0|UINT32|0x00000033
> +  gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferBase|0x0|UINT64|0x00000034
> +
>    ## PL180 MCI
>    gArmPlatformTokenSpaceGuid.PcdPL180SysMciRegAddress|0x00000000|UINT32|0x00000028
>    gArmPlatformTokenSpaceGuid.PcdPL180MciBaseAddress|0x00000000|UINT32|0x00000029
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf
> index 9b16f7f0c4731ab72bfb1008a073e81842bae82b..60789e9b8ff1b936db04953a765fb164b0e85a40 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf
> +++ b/ArmPlatformPkg/ArmVExpressPkg/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
>
>  [Ppis]
>    gArmMpCoreInfoPpiGuid
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> index 6379e81751fca5e7972c5c30f305be65fd1ae71d..5cd529750a3d2d3b0d381b58d875d378afaba2c2 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> +++ b/ArmPlatformPkg/ArmVExpressPkg/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,21 @@ 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
> +
>    // Map sparse memory region if present
>    if (HasSparseMemory) {
>      VirtualMemoryTable[++Index].PhysicalBase = SparseMemoryBase;
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 18/19] ArmPlatformPkg: Reserving framebuffer at build
Posted by Evan Lloyd 7 years ago
Responses inline

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: 25 October 2017 19:10
> To: Evan Lloyd <Evan.Lloyd@arm.com>
> Cc: edk2-devel@lists.01.org; "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 18/19] ArmPlatformPkg: Reserving framebuffer at
> build
>
> On 26 September 2017 at 21:15,  <evan.lloyd@arm.com> wrote:
> > From: Girish Pathak <girish.pathak@arm.com>
> >
> > Currently frame buffer memory is either reserved in special VRAM or
> > dynamically allocated using boot services memory allocation functions.
> > When allocated using boot services calls the memory has to be
> > allocated as EfiBootServicesData. Unfortunately failures have been
> > seen with this case.  There is also an unfortunate lack of control on
> > the placement of the frmae buffer.
> >
> > This change introduces two PCDs, PcdArmLcdFrameBufferBase and
> > PcdArmLcdFrameBufferSize which enable build time reservation of the
> > frame buffer, avoiding the need to allocate dynamically.  This allows
> > the frame buffer to appear as "I/O memory" outside of the normal RAM
> > map, which is similar to the "VRAM" case.
> >
> > This change has no impact on current code, only enables the option of
> > build time reservation of frame buffers.
> >
>
> Where is the memory actually being reserved? And if it is reserved, how can
> the OS reclaim it if it is not interested in using the GOP?

[[Evan Lloyd]] The memory is reserved by whatever sets the Pcd - normally the .dsc.  It may or may not be system RAM.
If it is excised from system memory, then there is no way for the OS to reclaim it, as it can't differentiate that from the VRAM case.
I'm not sure I see the relevance of that anyway.  Is our objective to provide restricted firmware tightly tuned for a specific operating system in an unrealistic mode, or to provide a generic example of firmware exercising the standard interfaces and testing real use cases?

Regards,
Evan

>
>
> > 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>
> > ---
> >  ArmPlatformPkg/ArmPlatformPkg.dec                                           |  4 ++++
> >
...
> > --
> > 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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 18/19] ArmPlatformPkg: Reserving framebuffer at build
Posted by Ard Biesheuvel 7 years ago
On 1 December 2017 at 16:56, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
> Responses inline
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: 25 October 2017 19:10
>> To: Evan Lloyd <Evan.Lloyd@arm.com>
>> Cc: edk2-devel@lists.01.org; "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 18/19] ArmPlatformPkg: Reserving framebuffer at
>> build
>>
>> On 26 September 2017 at 21:15,  <evan.lloyd@arm.com> wrote:
>> > From: Girish Pathak <girish.pathak@arm.com>
>> >
>> > Currently frame buffer memory is either reserved in special VRAM or
>> > dynamically allocated using boot services memory allocation functions.
>> > When allocated using boot services calls the memory has to be
>> > allocated as EfiBootServicesData. Unfortunately failures have been
>> > seen with this case.  There is also an unfortunate lack of control on
>> > the placement of the frmae buffer.
>> >
>> > This change introduces two PCDs, PcdArmLcdFrameBufferBase and
>> > PcdArmLcdFrameBufferSize which enable build time reservation of the
>> > frame buffer, avoiding the need to allocate dynamically.  This allows
>> > the frame buffer to appear as "I/O memory" outside of the normal RAM
>> > map, which is similar to the "VRAM" case.
>> >
>> > This change has no impact on current code, only enables the option of
>> > build time reservation of frame buffers.
>> >
>>
>> Where is the memory actually being reserved? And if it is reserved, how can
>> the OS reclaim it if it is not interested in using the GOP?
>
> [[Evan Lloyd]] The memory is reserved by whatever sets the Pcd - normally the .dsc.  It may or may not be system RAM.
> If it is excised from system memory, then there is no way for the OS to reclaim it, as it can't differentiate that from the VRAM case.
> I'm not sure I see the relevance of that anyway.  Is our objective to provide restricted firmware tightly tuned for a specific operating system in an unrealistic mode, or to provide a generic example of firmware exercising the standard interfaces and testing real use cases?
>

Is that intended as a rhetorical question? If you introduce a feature
that may take away RAM from the OS without a way to reclaim it, that
deserves a comment. At the very least, it would have saved me from
having to ask about it specifically.

Whether doing so is justified is another question, but that depends on
the expected users of this code (hence the need to document it)

-- 
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel