[PATCH] drm/ttm: add workaround for some arm hardware issue

[PATCH] drm/ttm: add workaround for some arm hardware issue

[Victor Zhao]

Some Arm based platform has hardware issue which may
generate incorrect addresses when receiving writes from the CPU
with a discontiguous set of byte enables. This affects the writes
with write combine property.

Workaround by change PROT_NORMAL_NC to PROT_DEVICE_nGnRE on arm.
As this is an issue with some specific arm based cpu, adding
a ttm parameter to control.

Signed-off-by: Victor Zhao <Victor.Zhao@amd.com>
---
 drivers/gpu/drm/ttm/ttm_module.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ttm/ttm_module.c b/drivers/gpu/drm/ttm/ttm_module.c
index e87f40674a4d..b27473cbbd52 100644
--- a/drivers/gpu/drm/ttm/ttm_module.c
+++ b/drivers/gpu/drm/ttm/ttm_module.c
@@ -41,6 +41,12 @@
 
 #include "ttm_module.h"
 
+static int enable_use_wc = 1;
+
+MODULE_PARM_DESC(enable_use_wc,
+	"control write combine usage on arm platform due to hardware issue with write combine found on some specific arm cpu (1 = enable(default), 0 = disable)");
+module_param(enable_use_wc, int, 0644);
+
 /**
  * ttm_prot_from_caching - Modify the page protection according to the
  * ttm cacing mode
@@ -63,7 +69,7 @@ pgprot_t ttm_prot_from_caching(enum ttm_caching caching, pgprot_t tmp)
 #endif
 #if defined(__ia64__) || defined(__arm__) || defined(__aarch64__) || \
 	defined(__powerpc__) || defined(__mips__)
-	if (caching == ttm_write_combined)
+	if (caching == ttm_write_combined && enable_use_wc != 0)
 		tmp = pgprot_writecombine(tmp);
 	else
 		tmp = pgprot_noncached(tmp);
-- 
2.25.1

Re: [PATCH] drm/ttm: add workaround for some arm hardware issue

[Christian König]

Am 22.12.21 um 06:51 schrieb Victor Zhao:
> Some Arm based platform has hardware issue which may
> generate incorrect addresses when receiving writes from the CPU
> with a discontiguous set of byte enables. This affects the writes
> with write combine property.

Can you point out which arm platforms are that exactly?

> Workaround by change PROT_NORMAL_NC to PROT_DEVICE_nGnRE on arm.
> As this is an issue with some specific arm based cpu, adding
> a ttm parameter to control.

Something as fundamental as this should not be made controllable by an 
module parameter.

Write combining is very important for good performance and so we should 
only disable it on boards where we know that this won't work correctly.

Regards,
Christian.

>
> Signed-off-by: Victor Zhao <Victor.Zhao@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_module.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_module.c b/drivers/gpu/drm/ttm/ttm_module.c
> index e87f40674a4d..b27473cbbd52 100644
> --- a/drivers/gpu/drm/ttm/ttm_module.c
> +++ b/drivers/gpu/drm/ttm/ttm_module.c
> @@ -41,6 +41,12 @@
>   
>   #include "ttm_module.h"
>   
> +static int enable_use_wc = 1;
> +
> +MODULE_PARM_DESC(enable_use_wc,
> +	"control write combine usage on arm platform due to hardware issue with write combine found on some specific arm cpu (1 = enable(default), 0 = disable)");
> +module_param(enable_use_wc, int, 0644);
> +
>   /**
>    * ttm_prot_from_caching - Modify the page protection according to the
>    * ttm cacing mode
> @@ -63,7 +69,7 @@ pgprot_t ttm_prot_from_caching(enum ttm_caching caching, pgprot_t tmp)
>   #endif
>   #if defined(__ia64__) || defined(__arm__) || defined(__aarch64__) || \
>   	defined(__powerpc__) || defined(__mips__)
> -	if (caching == ttm_write_combined)
> +	if (caching == ttm_write_combined && enable_use_wc != 0)
>   		tmp = pgprot_writecombine(tmp);
>   	else
>   		tmp = pgprot_noncached(tmp);

RE: [PATCH] drm/ttm: add workaround for some arm hardware issue

[Deng, Emily]

[AMD Official Use Only]

Currently, only ampere found this issue, but it is hard to detect ampere board, especially on arm passthrough environment.

Best wishes
Emily Deng



>-----Original Message-----
>From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>Christian König
>Sent: Wednesday, December 22, 2021 4:11 PM
>To: Zhao, Victor <Victor.Zhao@amd.com>; amd-gfx@lists.freedesktop.org
>Subject: Re: [PATCH] drm/ttm: add workaround for some arm hardware issue
>
>Am 22.12.21 um 06:51 schrieb Victor Zhao:
>> Some Arm based platform has hardware issue which may generate
>> incorrect addresses when receiving writes from the CPU with a
>> discontiguous set of byte enables. This affects the writes with write
>> combine property.
>
>Can you point out which arm platforms are that exactly?
>
>> Workaround by change PROT_NORMAL_NC to PROT_DEVICE_nGnRE on arm.
>> As this is an issue with some specific arm based cpu, adding a ttm
>> parameter to control.
>
>Something as fundamental as this should not be made controllable by an
>module parameter.
>
>Write combining is very important for good performance and so we should
>only disable it on boards where we know that this won't work correctly.
>
>Regards,
>Christian.
>
>>
>> Signed-off-by: Victor Zhao <Victor.Zhao@amd.com>
>> ---
>>   drivers/gpu/drm/ttm/ttm_module.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_module.c
>> b/drivers/gpu/drm/ttm/ttm_module.c
>> index e87f40674a4d..b27473cbbd52 100644
>> --- a/drivers/gpu/drm/ttm/ttm_module.c
>> +++ b/drivers/gpu/drm/ttm/ttm_module.c
>> @@ -41,6 +41,12 @@
>>
>>   #include "ttm_module.h"
>>
>> +static int enable_use_wc = 1;
>> +
>> +MODULE_PARM_DESC(enable_use_wc,
>> +    "control write combine usage on arm platform due to hardware issue
>> +with write combine found on some specific arm cpu (1 =
>> +enable(default), 0 = disable)"); module_param(enable_use_wc, int,
>> +0644);
>> +
>>   /**
>>    * ttm_prot_from_caching - Modify the page protection according to the
>>    * ttm cacing mode
>> @@ -63,7 +69,7 @@ pgprot_t ttm_prot_from_caching(enum ttm_caching
>caching, pgprot_t tmp)
>>   #endif
>>   #if defined(__ia64__) || defined(__arm__) || defined(__aarch64__) || \
>>      defined(__powerpc__) || defined(__mips__)
>> -    if (caching == ttm_write_combined)
>> +    if (caching == ttm_write_combined && enable_use_wc != 0)
>>              tmp = pgprot_writecombine(tmp);
>>      else
>>              tmp = pgprot_noncached(tmp);

Re: [PATCH] drm/ttm: add workaround for some arm hardware issue

[Alex Deucher]

On Wed, Dec 22, 2021 at 3:18 AM Deng, Emily <Emily.Deng@amd.com> wrote:
>
> [AMD Official Use Only]
>
> Currently, only ampere found this issue, but it is hard to detect ampere board, especially on arm passthrough environment.

Isn't this already handled in drm_arch_can_wc_memory()?

Alex

>
> Best wishes
> Emily Deng
>
>
>
> >-----Original Message-----
> >From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> >Christian König
> >Sent: Wednesday, December 22, 2021 4:11 PM
> >To: Zhao, Victor <Victor.Zhao@amd.com>; amd-gfx@lists.freedesktop.org
> >Subject: Re: [PATCH] drm/ttm: add workaround for some arm hardware issue
> >
> >Am 22.12.21 um 06:51 schrieb Victor Zhao:
> >> Some Arm based platform has hardware issue which may generate
> >> incorrect addresses when receiving writes from the CPU with a
> >> discontiguous set of byte enables. This affects the writes with write
> >> combine property.
> >
> >Can you point out which arm platforms are that exactly?
> >
> >> Workaround by change PROT_NORMAL_NC to PROT_DEVICE_nGnRE on arm.
> >> As this is an issue with some specific arm based cpu, adding a ttm
> >> parameter to control.
> >
> >Something as fundamental as this should not be made controllable by an
> >module parameter.
> >
> >Write combining is very important for good performance and so we should
> >only disable it on boards where we know that this won't work correctly.
> >
> >Regards,
> >Christian.
> >
> >>
> >> Signed-off-by: Victor Zhao <Victor.Zhao@amd.com>
> >> ---
> >>   drivers/gpu/drm/ttm/ttm_module.c | 8 +++++++-
> >>   1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/ttm/ttm_module.c
> >> b/drivers/gpu/drm/ttm/ttm_module.c
> >> index e87f40674a4d..b27473cbbd52 100644
> >> --- a/drivers/gpu/drm/ttm/ttm_module.c
> >> +++ b/drivers/gpu/drm/ttm/ttm_module.c
> >> @@ -41,6 +41,12 @@
> >>
> >>   #include "ttm_module.h"
> >>
> >> +static int enable_use_wc = 1;
> >> +
> >> +MODULE_PARM_DESC(enable_use_wc,
> >> +    "control write combine usage on arm platform due to hardware issue
> >> +with write combine found on some specific arm cpu (1 =
> >> +enable(default), 0 = disable)"); module_param(enable_use_wc, int,
> >> +0644);
> >> +
> >>   /**
> >>    * ttm_prot_from_caching - Modify the page protection according to the
> >>    * ttm cacing mode
> >> @@ -63,7 +69,7 @@ pgprot_t ttm_prot_from_caching(enum ttm_caching
> >caching, pgprot_t tmp)
> >>   #endif
> >>   #if defined(__ia64__) || defined(__arm__) || defined(__aarch64__) || \
> >>      defined(__powerpc__) || defined(__mips__)
> >> -    if (caching == ttm_write_combined)
> >> +    if (caching == ttm_write_combined && enable_use_wc != 0)
> >>              tmp = pgprot_writecombine(tmp);
> >>      else
> >>              tmp = pgprot_noncached(tmp);
>

Re: [PATCH] drm/ttm: add workaround for some arm hardware issue

[Christian König]

Am 22.12.21 um 15:11 schrieb Alex Deucher:
> On Wed, Dec 22, 2021 at 3:18 AM Deng, Emily <Emily.Deng@amd.com> wrote:
>> [AMD Official Use Only]
>>
>> Currently, only ampere found this issue, but it is hard to detect ampere board, especially on arm passthrough environment.
> Isn't this already handled in drm_arch_can_wc_memory()?

Could be, that function controls if we are trying to setup USWC in the 
first place.

Christian

>
> Alex
>
>> Best wishes
>> Emily Deng
>>
>>
>>
>>> -----Original Message-----
>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>> Christian König
>>> Sent: Wednesday, December 22, 2021 4:11 PM
>>> To: Zhao, Victor <Victor.Zhao@amd.com>; amd-gfx@lists.freedesktop.org
>>> Subject: Re: [PATCH] drm/ttm: add workaround for some arm hardware issue
>>>
>>> Am 22.12.21 um 06:51 schrieb Victor Zhao:
>>>> Some Arm based platform has hardware issue which may generate
>>>> incorrect addresses when receiving writes from the CPU with a
>>>> discontiguous set of byte enables. This affects the writes with write
>>>> combine property.
>>> Can you point out which arm platforms are that exactly?
>>>
>>>> Workaround by change PROT_NORMAL_NC to PROT_DEVICE_nGnRE on arm.
>>>> As this is an issue with some specific arm based cpu, adding a ttm
>>>> parameter to control.
>>> Something as fundamental as this should not be made controllable by an
>>> module parameter.
>>>
>>> Write combining is very important for good performance and so we should
>>> only disable it on boards where we know that this won't work correctly.
>>>
>>> Regards,
>>> Christian.
>>>
>>>> Signed-off-by: Victor Zhao <Victor.Zhao@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/ttm/ttm_module.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_module.c
>>>> b/drivers/gpu/drm/ttm/ttm_module.c
>>>> index e87f40674a4d..b27473cbbd52 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_module.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_module.c
>>>> @@ -41,6 +41,12 @@
>>>>
>>>>    #include "ttm_module.h"
>>>>
>>>> +static int enable_use_wc = 1;
>>>> +
>>>> +MODULE_PARM_DESC(enable_use_wc,
>>>> +    "control write combine usage on arm platform due to hardware issue
>>>> +with write combine found on some specific arm cpu (1 =
>>>> +enable(default), 0 = disable)"); module_param(enable_use_wc, int,
>>>> +0644);
>>>> +
>>>>    /**
>>>>     * ttm_prot_from_caching - Modify the page protection according to the
>>>>     * ttm cacing mode
>>>> @@ -63,7 +69,7 @@ pgprot_t ttm_prot_from_caching(enum ttm_caching
>>> caching, pgprot_t tmp)
>>>>    #endif
>>>>    #if defined(__ia64__) || defined(__arm__) || defined(__aarch64__) || \
>>>>       defined(__powerpc__) || defined(__mips__)
>>>> -    if (caching == ttm_write_combined)
>>>> +    if (caching == ttm_write_combined && enable_use_wc != 0)
>>>>               tmp = pgprot_writecombine(tmp);
>>>>       else
>>>>               tmp = pgprot_noncached(tmp);