diff mbox series

drm/i915/selftests: Set always_coherent to false when reading from CPU

Message ID 20240516151403.2875-1-nirmoy.das@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/selftests: Set always_coherent to false when reading from CPU | expand

Commit Message

Nirmoy Das May 16, 2024, 3:14 p.m. UTC
The previous commit 'commit 8d4ba9fc1c6c ("drm/i915/selftests: Pick
correct caching mode.")' was not complete as for non LLC  sharing platforms
cpu read can happen from LLC which probably doesn't have the latest
changes made by GPU.

Cc: Andi Shyti <andi.shyti@linux.intel.com>
Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Jonathan Cavitt <jonathan.cavitt@intel.com>
Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Cavitt, Jonathan May 16, 2024, 4:22 p.m. UTC | #1
-----Original Message-----
From: Das, Nirmoy <nirmoy.das@intel.com> 
Sent: Thursday, May 16, 2024 8:14 AM
To: intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org; Das, Nirmoy <nirmoy.das@intel.com>; Andi Shyti <andi.shyti@linux.intel.com>; Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>; Cavitt, Jonathan <jonathan.cavitt@intel.com>
Subject: [PATCH] drm/i915/selftests: Set always_coherent to false when reading from CPU
> 
> The previous commit 'commit 8d4ba9fc1c6c ("drm/i915/selftests: Pick
> correct caching mode.")' was not complete as for non LLC  sharing platforms
> cpu read can happen from LLC which probably doesn't have the latest
> changes made by GPU.
> 
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>

I see no problem with this
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
-Jonathan Cavitt

> ---
>  drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
> index 65a931ea80e9..3527b8f446fe 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
> @@ -196,7 +196,7 @@ static int verify_access(struct drm_i915_private *i915,
>  	if (err)
>  		goto out_file;
>  
> -	mode = intel_gt_coherent_map_type(to_gt(i915), native_obj, true);
> +	mode = intel_gt_coherent_map_type(to_gt(i915), native_obj, false);
>  	vaddr = i915_gem_object_pin_map_unlocked(native_obj, mode);
>  	if (IS_ERR(vaddr)) {
>  		err = PTR_ERR(vaddr);
> -- 
> 2.42.0
> 
>
Jani Nikula May 17, 2024, 7:39 a.m. UTC | #2
On Thu, 16 May 2024, Nirmoy Das <nirmoy.das@intel.com> wrote:
> The previous commit 'commit 8d4ba9fc1c6c ("drm/i915/selftests: Pick

"previous commit" is a fairly vague reference once this gets
committed. It's not going to be "previous" in any meaningful sense.

Please just start with:

Commit 8d4ba9fc1c6c ("drm/i915/selftests: Pick correct caching mode.")
was not complete...

And probably add:

Fixes: 8d4ba9fc1c6c ("drm/i915/selftests: Pick correct caching mode.")

BR,
Jani.

> correct caching mode.")' was not complete as for non LLC  sharing platforms
> cpu read can happen from LLC which probably doesn't have the latest
> changes made by GPU.
>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
> index 65a931ea80e9..3527b8f446fe 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
> @@ -196,7 +196,7 @@ static int verify_access(struct drm_i915_private *i915,
>  	if (err)
>  		goto out_file;
>  
> -	mode = intel_gt_coherent_map_type(to_gt(i915), native_obj, true);
> +	mode = intel_gt_coherent_map_type(to_gt(i915), native_obj, false);
>  	vaddr = i915_gem_object_pin_map_unlocked(native_obj, mode);
>  	if (IS_ERR(vaddr)) {
>  		err = PTR_ERR(vaddr);
Nirmoy Das May 17, 2024, 8:40 a.m. UTC | #3
Hi Jani,

On 5/17/2024 9:39 AM, Jani Nikula wrote:
> On Thu, 16 May 2024, Nirmoy Das <nirmoy.das@intel.com> wrote:
>> The previous commit 'commit 8d4ba9fc1c6c ("drm/i915/selftests: Pick
> "previous commit" is a fairly vague reference once this gets
> committed. It's not going to be "previous" in any meaningful sense.
>
> Please just start with:
>
> Commit 8d4ba9fc1c6c ("drm/i915/selftests: Pick correct caching mode.")
> was not complete...

Will do that.


>
> And probably add:
>
> Fixes: 8d4ba9fc1c6c ("drm/i915/selftests: Pick correct caching mode.")

Do we need Fixes for selftest ? I always assumed it is not required as 
this code is for debug/CI


Thanks,

Nirmoy

>
> BR,
> Jani.
>
>> correct caching mode.")' was not complete as for non LLC  sharing platforms
>> cpu read can happen from LLC which probably doesn't have the latest
>> changes made by GPU.
>>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>> index 65a931ea80e9..3527b8f446fe 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>> @@ -196,7 +196,7 @@ static int verify_access(struct drm_i915_private *i915,
>>   	if (err)
>>   		goto out_file;
>>   
>> -	mode = intel_gt_coherent_map_type(to_gt(i915), native_obj, true);
>> +	mode = intel_gt_coherent_map_type(to_gt(i915), native_obj, false);
>>   	vaddr = i915_gem_object_pin_map_unlocked(native_obj, mode);
>>   	if (IS_ERR(vaddr)) {
>>   		err = PTR_ERR(vaddr);
Andi Shyti May 17, 2024, 10:30 a.m. UTC | #4
Hi Nirmoy,

On Thu, May 16, 2024 at 05:14:03PM +0200, Nirmoy Das wrote:
> The previous commit 'commit 8d4ba9fc1c6c ("drm/i915/selftests: Pick
> correct caching mode.")' was not complete as for non LLC  sharing platforms
> cpu read can happen from LLC which probably doesn't have the latest
> changes made by GPU.
> 
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>

you can add:

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi
Jani Nikula May 17, 2024, 11:53 a.m. UTC | #5
On Fri, 17 May 2024, Nirmoy Das <nirmoy.das@linux.intel.com> wrote:
> Hi Jani,
>
> On 5/17/2024 9:39 AM, Jani Nikula wrote:
>> On Thu, 16 May 2024, Nirmoy Das <nirmoy.das@intel.com> wrote:
>>> The previous commit 'commit 8d4ba9fc1c6c ("drm/i915/selftests: Pick
>> "previous commit" is a fairly vague reference once this gets
>> committed. It's not going to be "previous" in any meaningful sense.
>>
>> Please just start with:
>>
>> Commit 8d4ba9fc1c6c ("drm/i915/selftests: Pick correct caching mode.")
>> was not complete...
>
> Will do that.
>
>
>>
>> And probably add:
>>
>> Fixes: 8d4ba9fc1c6c ("drm/i915/selftests: Pick correct caching mode.")
>
> Do we need Fixes for selftest ? I always assumed it is not required as 
> this code is for debug/CI

Maybe not for stuff that's already in stable, but we do run CI on
drm-next and -rc kernels, and if this causes issues there, why not have
them fixed?

BR,
Jani.

>
>
> Thanks,
>
> Nirmoy
>
>>
>> BR,
>> Jani.
>>
>>> correct caching mode.")' was not complete as for non LLC  sharing platforms
>>> cpu read can happen from LLC which probably doesn't have the latest
>>> changes made by GPU.
>>>
>>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>>> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>>> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>>> index 65a931ea80e9..3527b8f446fe 100644
>>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>>> @@ -196,7 +196,7 @@ static int verify_access(struct drm_i915_private *i915,
>>>   	if (err)
>>>   		goto out_file;
>>>   
>>> -	mode = intel_gt_coherent_map_type(to_gt(i915), native_obj, true);
>>> +	mode = intel_gt_coherent_map_type(to_gt(i915), native_obj, false);
>>>   	vaddr = i915_gem_object_pin_map_unlocked(native_obj, mode);
>>>   	if (IS_ERR(vaddr)) {
>>>   		err = PTR_ERR(vaddr);
Nirmoy Das May 17, 2024, 12:45 p.m. UTC | #6
On 5/17/2024 1:53 PM, Jani Nikula wrote:
> On Fri, 17 May 2024, Nirmoy Das <nirmoy.das@linux.intel.com> wrote:
>> Hi Jani,
>>
>> On 5/17/2024 9:39 AM, Jani Nikula wrote:
>>> On Thu, 16 May 2024, Nirmoy Das <nirmoy.das@intel.com> wrote:
>>>> The previous commit 'commit 8d4ba9fc1c6c ("drm/i915/selftests: Pick
>>> "previous commit" is a fairly vague reference once this gets
>>> committed. It's not going to be "previous" in any meaningful sense.
>>>
>>> Please just start with:
>>>
>>> Commit 8d4ba9fc1c6c ("drm/i915/selftests: Pick correct caching mode.")
>>> was not complete...
>> Will do that.
>>
>>
>>> And probably add:
>>>
>>> Fixes: 8d4ba9fc1c6c ("drm/i915/selftests: Pick correct caching mode.")
>> Do we need Fixes for selftest ? I always assumed it is not required as
>> this code is for debug/CI
> Maybe not for stuff that's already in stable, but we do run CI on
> drm-next and -rc kernels, and if this causes issues there, why not have
> them fixed?

Not sure a commit with Fixes flows from drm-intel-next to drm-next/-rc 
but I see no issue adding Fixes without CC-ing to stable.

Pushed it to drm-intel-next with above modifications.  b4-shazam picked 
Fixes as well which was nice.


Thanks,

Nirmoy

>
> BR,
> Jani.
>
>>
>> Thanks,
>>
>> Nirmoy
>>
>>> BR,
>>> Jani.
>>>
>>>> correct caching mode.")' was not complete as for non LLC  sharing platforms
>>>> cpu read can happen from LLC which probably doesn't have the latest
>>>> changes made by GPU.
>>>>
>>>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>>>> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>>>> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>>>> index 65a931ea80e9..3527b8f446fe 100644
>>>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>>>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
>>>> @@ -196,7 +196,7 @@ static int verify_access(struct drm_i915_private *i915,
>>>>    	if (err)
>>>>    		goto out_file;
>>>>    
>>>> -	mode = intel_gt_coherent_map_type(to_gt(i915), native_obj, true);
>>>> +	mode = intel_gt_coherent_map_type(to_gt(i915), native_obj, false);
>>>>    	vaddr = i915_gem_object_pin_map_unlocked(native_obj, mode);
>>>>    	if (IS_ERR(vaddr)) {
>>>>    		err = PTR_ERR(vaddr);
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
index 65a931ea80e9..3527b8f446fe 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
@@ -196,7 +196,7 @@  static int verify_access(struct drm_i915_private *i915,
 	if (err)
 		goto out_file;
 
-	mode = intel_gt_coherent_map_type(to_gt(i915), native_obj, true);
+	mode = intel_gt_coherent_map_type(to_gt(i915), native_obj, false);
 	vaddr = i915_gem_object_pin_map_unlocked(native_obj, mode);
 	if (IS_ERR(vaddr)) {
 		err = PTR_ERR(vaddr);