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

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

[Ard Biesheuvel]

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(-)

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

[PATCH 2/2] ArmPlatformPkg/PL111LcdArmVExpressLib: use write-combine mapping for VRAM

[Ard Biesheuvel]

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/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
index 2000c9bdf436..d18d6b3e1665 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
@@ -192,7 +192,7 @@ LcdPlatformGetVram (
     ASSERT_EFI_ERROR(Status);
 
     // Mark the VRAM as un-cachable. The VRAM is inside the DRAM, which is cachable.
-    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

Re: [PATCH 2/2] ArmPlatformPkg/PL111LcdArmVExpressLib: use write-combine mapping for VRAM

[Ryan Harkin]

On 5 April 2017 at 21:38, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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>

Well, ... PL111 isn't usually enabled for me. And if I enable it,
neither Foundation nor AEMv8 models boot with or without this patch.

So it's no worse than before....


> ---
>  ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> index 2000c9bdf436..d18d6b3e1665 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> @@ -192,7 +192,7 @@ LcdPlatformGetVram (
>      ASSERT_EFI_ERROR(Status);
>
>      // Mark the VRAM as un-cachable. The VRAM is inside the DRAM, which is cachable.
> -    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

Re: [PATCH 2/2] ArmPlatformPkg/PL111LcdArmVExpressLib: use write-combine mapping for VRAM

[Ard Biesheuvel]

On 6 April 2017 at 12:14, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> On 5 April 2017 at 21:38, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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>
>
> Well, ... PL111 isn't usually enabled for me. And if I enable it,
> neither Foundation nor AEMv8 models boot with or without this patch.
>
> So it's no worse than before....
>

Not even foundation model? That is strange ...

>
>> ---
>>  ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
>> index 2000c9bdf436..d18d6b3e1665 100644
>> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
>> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
>> @@ -192,7 +192,7 @@ LcdPlatformGetVram (
>>      ASSERT_EFI_ERROR(Status);
>>
>>      // Mark the VRAM as un-cachable. The VRAM is inside the DRAM, which is cachable.
>> -    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

Re: [PATCH 2/2] ArmPlatformPkg/PL111LcdArmVExpressLib: use write-combine mapping for VRAM

[Ryan Harkin]

On 6 April 2017 at 12:32, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 6 April 2017 at 12:14, Ryan Harkin <ryan.harkin@linaro.org> wrote:
>> On 5 April 2017 at 21:38, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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>
>>
>> Well, ... PL111 isn't usually enabled for me. And if I enable it,
>> neither Foundation nor AEMv8 models boot with or without this patch.
>>
>> So it's no worse than before....
>>
>
> Not even foundation model? That is strange ...
>

The reason we created a config without PL111 was because Foundation
didn't originally have a PL111. And I wanted a single binary to run on
Foundation and AEMv8. But ARM added it to Foundation about a year ago
and I've never tried it.

I'm happy for you to submit the patch if it works for you.


>>
>>> ---
>>>  ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
>>> index 2000c9bdf436..d18d6b3e1665 100644
>>> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
>>> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
>>> @@ -192,7 +192,7 @@ LcdPlatformGetVram (
>>>      ASSERT_EFI_ERROR(Status);
>>>
>>>      // Mark the VRAM as un-cachable. The VRAM is inside the DRAM, which is cachable.
>>> -    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

Re: [PATCH 1/2] ArmPlatformPkg/HdLcdArmVExpressLib: use write-combine mapping for VRAM

[Leif Lindholm]

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
> 

Re: [PATCH 1/2] ArmPlatformPkg/HdLcdArmVExpressLib: use write-combine mapping for VRAM

[Ard Biesheuvel]

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

Re: [PATCH 1/2] ArmPlatformPkg/HdLcdArmVExpressLib: use write-combine mapping for VRAM

[Ryan Harkin]

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.