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