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

2018-01-03 Thread Evan Lloyd 


> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: 23 December 2017 16:03
> To: Evan Lloyd <evan.ll...@arm.com>
> Cc: edk2-devel@lists.01.org; Arvind Chauhan <arvind.chau...@arm.com>;
> Daniil Egranov <daniil.egra...@arm.com>; Thomas Abraham
> <thomas.abra...@arm.com>; "ard.biesheu...@linaro.org"@arm.com;
> "leif.lindh...@linaro.org"@arm.com;
> "matteo.carl...@arm.com"@arm.com; "n...@arm.com"@arm.com
> Subject: Re: [PATCH edk2-platforms v2 14/18] ARM/VExpressPkg:
> Reserving framebuffer at build
>
> On 22 December 2017 at 19:08,  <evan.ll...@arm.com> wrote:
> > From: Girish Pathak 
> >
> > 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
...
> > +  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?

 [[Evan Lloyd]] That is an excellent suggestion, thank you - we'll add that.

>
> >// 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.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2017-12-23 Thread Ard Biesheuvel 
On 22 December 2017 at 19:08,   wrote:
> From: Girish Pathak 
>
> 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 
> Signed-off-by: Evan Lloyd 
> ---
>  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 
>  #include 
>
> +#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")
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2017-12-22 Thread evan . lloyd 
From: Girish Pathak 

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 
Signed-off-by: Evan Lloyd 
---
 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
 
 [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 
 #include 
 
+#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
+
   // 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