diff mbox series

[edk2,1/2] ArmPlatformPkg/HdLcdArmVExpressLib: use write-combine mapping for VRAM

Message ID 1491424713-5203-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show
Series [edk2,1/2] ArmPlatformPkg/HdLcdArmVExpressLib: use write-combine mapping for VRAM | expand

Commit Message

Ard Biesheuvel April 5, 2017, 8:38 p.m. UTC 
Replace the uncached memory mapping of the framebuffer with a write-
combining one. This improves performance, and avoids issues with
unaligned accesses and DC ZVA instructions performed by the accelerated
memcpy/memset routines.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.7.4

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

Comments

Leif Lindholm April 6, 2017, 9:37 a.m. UTC | #1 
On Wed, Apr 05, 2017 at 09:38:32PM +0100, Ard Biesheuvel wrote:
> Replace the uncached memory mapping of the framebuffer with a write-

> combining one. This improves performance, and avoids issues with

> unaligned accesses and DC ZVA instructions performed by the accelerated

> memcpy/memset routines.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


If you can get a nod each from Ryan and Evan, for the series:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


> ---

>  ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c

> index a57846715ed7..d6d47545c824 100644

> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c

> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c

> @@ -143,7 +143,7 @@ LcdPlatformGetVram (

>    ASSERT_EFI_ERROR(Status);

>  

>    // Mark the VRAM as un-cacheable. The VRAM is inside the DRAM, which is cacheable.

> -  Status = Cpu->SetMemoryAttributes (Cpu, *VramBaseAddress, *VramSize, EFI_MEMORY_UC);

> +  Status = Cpu->SetMemoryAttributes (Cpu, *VramBaseAddress, *VramSize, EFI_MEMORY_WC);

>    ASSERT_EFI_ERROR(Status);

>    if (EFI_ERROR(Status)) {

>      gBS->FreePool (VramBaseAddress);

> -- 

> 2.7.4

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 6, 2017, 10:40 a.m. UTC | #2 
On 6 April 2017 at 10:37, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Apr 05, 2017 at 09:38:32PM +0100, Ard Biesheuvel wrote:

>> Replace the uncached memory mapping of the framebuffer with a write-

>> combining one. This improves performance, and avoids issues with

>> unaligned accesses and DC ZVA instructions performed by the accelerated

>> memcpy/memset routines.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>

> If you can get a nod each from Ryan and Evan, for the series:

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

>

>> ---

>>  ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c | 2 +-

>>  1 file changed, 1 insertion(+), 1 deletion(-)

>>

>> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c

>> index a57846715ed7..d6d47545c824 100644

>> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c

>> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c

>> @@ -143,7 +143,7 @@ LcdPlatformGetVram (

>>    ASSERT_EFI_ERROR(Status);

>>

>>    // Mark the VRAM as un-cacheable. The VRAM is inside the DRAM, which is cacheable.

>> -  Status = Cpu->SetMemoryAttributes (Cpu, *VramBaseAddress, *VramSize, EFI_MEMORY_UC);

>> +  Status = Cpu->SetMemoryAttributes (Cpu, *VramBaseAddress, *VramSize, EFI_MEMORY_WC);

>>    ASSERT_EFI_ERROR(Status);

>>    if (EFI_ERROR(Status)) {

>>      gBS->FreePool (VramBaseAddress);


Actually, it would be more appropriate for this code to use DXE services, i.e.,

gDS->SetMemorySpaceAttributes (xxx)

which internally calls Cpu->SetMemoryAttributes(), but also checks the
validity of the request against the capabilities of the region
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ryan Harkin April 6, 2017, 11:15 a.m. UTC | #3 
On 6 April 2017 at 11:40, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 6 April 2017 at 10:37, Leif Lindholm <leif.lindholm@linaro.org> wrote:

>> On Wed, Apr 05, 2017 at 09:38:32PM +0100, Ard Biesheuvel wrote:

>>> Replace the uncached memory mapping of the framebuffer with a write-

>>> combining one. This improves performance, and avoids issues with

>>> unaligned accesses and DC ZVA instructions performed by the accelerated

>>> memcpy/memset routines.

>>>

>>> Contributed-under: TianoCore Contribution Agreement 1.0

>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>

>> If you can get a nod each from Ryan and Evan, for the series:

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

>>

>>> ---

>>>  ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c | 2 +-

>>>  1 file changed, 1 insertion(+), 1 deletion(-)

>>>

>>> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c

>>> index a57846715ed7..d6d47545c824 100644

>>> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c

>>> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c

>>> @@ -143,7 +143,7 @@ LcdPlatformGetVram (

>>>    ASSERT_EFI_ERROR(Status);

>>>

>>>    // Mark the VRAM as un-cacheable. The VRAM is inside the DRAM, which is cacheable.

>>> -  Status = Cpu->SetMemoryAttributes (Cpu, *VramBaseAddress, *VramSize, EFI_MEMORY_UC);

>>> +  Status = Cpu->SetMemoryAttributes (Cpu, *VramBaseAddress, *VramSize, EFI_MEMORY_WC);

>>>    ASSERT_EFI_ERROR(Status);

>>>    if (EFI_ERROR(Status)) {

>>>      gBS->FreePool (VramBaseAddress);

>

> Actually, it would be more appropriate for this code to use DXE services, i.e.,

>

> gDS->SetMemorySpaceAttributes (xxx)

>

> which internally calls Cpu->SetMemoryAttributes(), but also checks the

> validity of the request against the capabilities of the region


Ach! I've just tested this patch :-)

Anyway, it works fine on TC2.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
index a57846715ed7..d6d47545c824 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
@@ -143,7 +143,7 @@  LcdPlatformGetVram (
   ASSERT_EFI_ERROR(Status);
 
   // Mark the VRAM as un-cacheable. The VRAM is inside the DRAM, which is cacheable.
-  Status = Cpu->SetMemoryAttributes (Cpu, *VramBaseAddress, *VramSize, EFI_MEMORY_UC);
+  Status = Cpu->SetMemoryAttributes (Cpu, *VramBaseAddress, *VramSize, EFI_MEMORY_WC);
   ASSERT_EFI_ERROR(Status);
   if (EFI_ERROR(Status)) {
     gBS->FreePool (VramBaseAddress);