* [Intel-gfx] [PATCH] drm/i915/selftests: keep same cache settings as timeline
@ 2023-03-08 23:28 fei.yang
2023-03-15 14:02 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
0 siblings, 1 reply; 6+ messages in thread
From: fei.yang @ 2023-03-08 23:28 UTC (permalink / raw)
To: intel-gfx; +Cc: Matt Roper, Jonathan Cavitt, Chris Wilson, dri-devel
From: Fei Yang <fei.yang@intel.com>
On MTL, objects allocated through i915_gem_object_create_internal() are
mapped as uncached in GPU by default because HAS_LLC is false. However
in the live_hwsp_read selftest these watcher objects are mapped as WB
on CPU side. The conseqence is that the updates done by the GPU are not
immediately visible to CPU, thus the selftest is randomly failing due to
the stale data in CPU cache. Solution can be either setting WC for CPU +
UC for GPU, or WB for CPU + 1-way coherent WB for GPU.
To keep the consistency, let's simply inherit the same cache settings
from the timeline, which is WB for CPU + 1-way coherent WB for GPU,
because this test is supposed to emulate the behavior of the timeline
anyway.
v2: copy cache settings from timeline instead of setting it to WC
(Suggested by Chris)
Signed-off-by: Fei Yang <fei.yang@intel.com>
Reviewed-by: Chris Wilson <chris.p.wilson@linux.intel.com>
Acked-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
Acked-by: Matt Roper <matthew.d.roper@intel.com>
---
drivers/gpu/drm/i915/gt/selftest_timeline.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/selftest_timeline.c b/drivers/gpu/drm/i915/gt/selftest_timeline.c
index 522d0190509c..631aaed9bc3d 100644
--- a/drivers/gpu/drm/i915/gt/selftest_timeline.c
+++ b/drivers/gpu/drm/i915/gt/selftest_timeline.c
@@ -825,7 +825,8 @@ static bool cmp_gte(u32 a, u32 b)
return a >= b;
}
-static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt)
+static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt,
+ struct intel_timeline *tl)
{
struct drm_i915_gem_object *obj;
struct i915_vma *vma;
@@ -834,7 +835,10 @@ static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt)
if (IS_ERR(obj))
return PTR_ERR(obj);
- w->map = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
+ /* keep the same cache settings as timeline */
+ i915_gem_object_set_cache_coherency(obj, tl->hwsp_ggtt->obj->cache_level);
+ w->map = i915_gem_object_pin_map_unlocked(obj,
+ page_unmask_bits(tl->hwsp_ggtt->obj->mm.mapping));
if (IS_ERR(w->map)) {
i915_gem_object_put(obj);
return PTR_ERR(w->map);
@@ -1004,8 +1008,10 @@ static int live_hwsp_read(void *arg)
if (!tl->has_initial_breadcrumb)
goto out_free;
+ selftest_tl_pin(tl);
+
for (i = 0; i < ARRAY_SIZE(watcher); i++) {
- err = setup_watcher(&watcher[i], gt);
+ err = setup_watcher(&watcher[i], gt, tl);
if (err)
goto out;
}
@@ -1160,6 +1166,8 @@ static int live_hwsp_read(void *arg)
for (i = 0; i < ARRAY_SIZE(watcher); i++)
cleanup_watcher(&watcher[i]);
+ intel_timeline_unpin(tl);
+
if (igt_flush_test(gt->i915))
err = -EIO;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Intel-gfx] [PATCH] drm/i915/selftests: keep same cache settings as timeline
@ 2023-03-15 18:08 fei.yang
2023-03-17 0:21 ` Matt Roper
0 siblings, 1 reply; 6+ messages in thread
From: fei.yang @ 2023-03-15 18:08 UTC (permalink / raw)
To: intel-gfx; +Cc: matthew.d.roper, dri-devel, chris
From: Fei Yang <fei.yang@intel.com>
On MTL, objects allocated through i915_gem_object_create_internal() are
mapped as uncached in GPU by default because HAS_LLC is false. However
in the live_hwsp_read selftest these watcher objects are mapped as WB
on CPU side. The conseqence is that the updates done by the GPU are not
immediately visible to CPU, thus the selftest is randomly failing due to
the stale data in CPU cache. Solution can be either setting WC for CPU +
UC for GPU, or WB for CPU + 1-way coherent WB for GPU.
To keep the consistency, let's simply inherit the same cache settings
from the timeline, which is WB for CPU + 1-way coherent WB for GPU,
because this test is supposed to emulate the behavior of the timeline
anyway.
Signed-off-by: Fei Yang <fei.yang@intel.com>
---
drivers/gpu/drm/i915/gt/selftest_timeline.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/selftest_timeline.c b/drivers/gpu/drm/i915/gt/selftest_timeline.c
index 522d0190509c..631aaed9bc3d 100644
--- a/drivers/gpu/drm/i915/gt/selftest_timeline.c
+++ b/drivers/gpu/drm/i915/gt/selftest_timeline.c
@@ -825,7 +825,8 @@ static bool cmp_gte(u32 a, u32 b)
return a >= b;
}
-static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt)
+static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt,
+ struct intel_timeline *tl)
{
struct drm_i915_gem_object *obj;
struct i915_vma *vma;
@@ -834,7 +835,10 @@ static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt)
if (IS_ERR(obj))
return PTR_ERR(obj);
- w->map = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
+ /* keep the same cache settings as timeline */
+ i915_gem_object_set_cache_coherency(obj, tl->hwsp_ggtt->obj->cache_level);
+ w->map = i915_gem_object_pin_map_unlocked(obj,
+ page_unmask_bits(tl->hwsp_ggtt->obj->mm.mapping));
if (IS_ERR(w->map)) {
i915_gem_object_put(obj);
return PTR_ERR(w->map);
@@ -1004,8 +1008,10 @@ static int live_hwsp_read(void *arg)
if (!tl->has_initial_breadcrumb)
goto out_free;
+ selftest_tl_pin(tl);
+
for (i = 0; i < ARRAY_SIZE(watcher); i++) {
- err = setup_watcher(&watcher[i], gt);
+ err = setup_watcher(&watcher[i], gt, tl);
if (err)
goto out;
}
@@ -1160,6 +1166,8 @@ static int live_hwsp_read(void *arg)
for (i = 0; i < ARRAY_SIZE(watcher); i++)
cleanup_watcher(&watcher[i]);
+ intel_timeline_unpin(tl);
+
if (igt_flush_test(gt->i915))
err = -EIO;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/selftests: keep same cache settings as timeline
2023-03-15 18:08 [Intel-gfx] [PATCH] " fei.yang
@ 2023-03-17 0:21 ` Matt Roper
2023-03-17 3:43 ` Yang, Fei
0 siblings, 1 reply; 6+ messages in thread
From: Matt Roper @ 2023-03-17 0:21 UTC (permalink / raw)
To: fei.yang; +Cc: intel-gfx, dri-devel, chris
On Wed, Mar 15, 2023 at 11:08:00AM -0700, fei.yang@intel.com wrote:
> From: Fei Yang <fei.yang@intel.com>
>
> On MTL, objects allocated through i915_gem_object_create_internal() are
> mapped as uncached in GPU by default because HAS_LLC is false. However
> in the live_hwsp_read selftest these watcher objects are mapped as WB
> on CPU side. The conseqence is that the updates done by the GPU are not
> immediately visible to CPU, thus the selftest is randomly failing due to
> the stale data in CPU cache. Solution can be either setting WC for CPU +
> UC for GPU, or WB for CPU + 1-way coherent WB for GPU.
> To keep the consistency, let's simply inherit the same cache settings
> from the timeline, which is WB for CPU + 1-way coherent WB for GPU,
> because this test is supposed to emulate the behavior of the timeline
> anyway.
>
> Signed-off-by: Fei Yang <fei.yang@intel.com>
It looks like there might be an indentation mistake on the second line
of the i915_gem_object_pin_map_unlocked() call, but we can fix that up
while applying; no need to re-send.
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Is there an FDO issue # for the random failures thar were being seen?
If so, we should add a Closes: or References: tag here.
Matt
> ---
> drivers/gpu/drm/i915/gt/selftest_timeline.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/selftest_timeline.c b/drivers/gpu/drm/i915/gt/selftest_timeline.c
> index 522d0190509c..631aaed9bc3d 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_timeline.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_timeline.c
> @@ -825,7 +825,8 @@ static bool cmp_gte(u32 a, u32 b)
> return a >= b;
> }
>
> -static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt)
> +static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt,
> + struct intel_timeline *tl)
> {
> struct drm_i915_gem_object *obj;
> struct i915_vma *vma;
> @@ -834,7 +835,10 @@ static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt)
> if (IS_ERR(obj))
> return PTR_ERR(obj);
>
> - w->map = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
> + /* keep the same cache settings as timeline */
> + i915_gem_object_set_cache_coherency(obj, tl->hwsp_ggtt->obj->cache_level);
> + w->map = i915_gem_object_pin_map_unlocked(obj,
> + page_unmask_bits(tl->hwsp_ggtt->obj->mm.mapping));
> if (IS_ERR(w->map)) {
> i915_gem_object_put(obj);
> return PTR_ERR(w->map);
> @@ -1004,8 +1008,10 @@ static int live_hwsp_read(void *arg)
> if (!tl->has_initial_breadcrumb)
> goto out_free;
>
> + selftest_tl_pin(tl);
> +
> for (i = 0; i < ARRAY_SIZE(watcher); i++) {
> - err = setup_watcher(&watcher[i], gt);
> + err = setup_watcher(&watcher[i], gt, tl);
> if (err)
> goto out;
> }
> @@ -1160,6 +1166,8 @@ static int live_hwsp_read(void *arg)
> for (i = 0; i < ARRAY_SIZE(watcher); i++)
> cleanup_watcher(&watcher[i]);
>
> + intel_timeline_unpin(tl);
> +
> if (igt_flush_test(gt->i915))
> err = -EIO;
>
> --
> 2.25.1
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/selftests: keep same cache settings as timeline
2023-03-17 0:21 ` Matt Roper
@ 2023-03-17 3:43 ` Yang, Fei
2023-03-17 16:38 ` Matt Roper
0 siblings, 1 reply; 6+ messages in thread
From: Yang, Fei @ 2023-03-17 3:43 UTC (permalink / raw)
To: Roper, Matthew D
Cc: intel-gfx@lists.freedesktop.org, Chris Wilson,
dri-devel@lists.freedesktop.org
>> From: Fei Yang <fei.yang@intel.com>
>>
>> On MTL, objects allocated through i915_gem_object_create_internal() are
>> mapped as uncached in GPU by default because HAS_LLC is false. However
>> in the live_hwsp_read selftest these watcher objects are mapped as WB
>> on CPU side. The conseqence is that the updates done by the GPU are not
>> immediately visible to CPU, thus the selftest is randomly failing due to
>> the stale data in CPU cache. Solution can be either setting WC for CPU +
>> UC for GPU, or WB for CPU + 1-way coherent WB for GPU.
>> To keep the consistency, let's simply inherit the same cache settings
>> from the timeline, which is WB for CPU + 1-way coherent WB for GPU,
>> because this test is supposed to emulate the behavior of the timeline
>> anyway.
>>
>> Signed-off-by: Fei Yang <fei.yang@intel.com>
>
> It looks like there might be an indentation mistake on the second line
> of the i915_gem_object_pin_map_unlocked() call, but we can fix that up
> while applying; no need to re-send.
>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Thanks for reviewing.
> Is there an FDO issue # for the random failures thar were being seen?
> If so, we should add a Closes: or References: tag here.
I'm not aware of a FDO filed for this failure. That might be because the
issue is reproduced on MTL which might not be widely available to the
community yet.
> Matt
>> ---
>> drivers/gpu/drm/i915/gt/selftest_timeline.c | 14 +++++++++++---
>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_timeline.c b/drivers/gpu/drm/i915/gt/selftest_timeline.c
>> index 522d0190509c..631aaed9bc3d 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_timeline.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_timeline.c
>> @@ -825,7 +825,8 @@ static bool cmp_gte(u32 a, u32 b)
>> return a >= b;
>> }
>>
>> -static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt)
>> +static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt,
>> + struct intel_timeline *tl)
>> {
>> struct drm_i915_gem_object *obj;
>> struct i915_vma *vma;
>> @@ -834,7 +835,10 @@ static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt)
>> if (IS_ERR(obj))
>> return PTR_ERR(obj);
>>
>> - w->map = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
>> + /* keep the same cache settings as timeline */
>> + i915_gem_object_set_cache_coherency(obj, tl->hwsp_ggtt->obj->cache_level);
>> + w->map = i915_gem_object_pin_map_unlocked(obj,
>> + page_unmask_bits(tl->hwsp_ggtt->obj->mm.mapping));
>> if (IS_ERR(w->map)) {
>> i915_gem_object_put(obj);
>> return PTR_ERR(w->map);
>> @@ -1004,8 +1008,10 @@ static int live_hwsp_read(void *arg)
>> if (!tl->has_initial_breadcrumb)
>> goto out_free;
>>
>> + selftest_tl_pin(tl);
>> +
>> for (i = 0; i < ARRAY_SIZE(watcher); i++) {
>> - err = setup_watcher(&watcher[i], gt);
>> + err = setup_watcher(&watcher[i], gt, tl);
>> if (err)
>> goto out;
>> }
>> @@ -1160,6 +1166,8 @@ static int live_hwsp_read(void *arg)
>> for (i = 0; i < ARRAY_SIZE(watcher); i++)
>> cleanup_watcher(&watcher[i]);
>>
>> + intel_timeline_unpin(tl);
>> +
>> if (igt_flush_test(gt->i915))
>> err = -EIO;
>>
>> --
>> 2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/selftests: keep same cache settings as timeline
2023-03-17 3:43 ` Yang, Fei
@ 2023-03-17 16:38 ` Matt Roper
0 siblings, 0 replies; 6+ messages in thread
From: Matt Roper @ 2023-03-17 16:38 UTC (permalink / raw)
To: Yang, Fei
Cc: intel-gfx@lists.freedesktop.org, Chris Wilson,
dri-devel@lists.freedesktop.org
On Thu, Mar 16, 2023 at 08:43:46PM -0700, Yang, Fei wrote:
> >> From: Fei Yang <fei.yang@intel.com>
> >>
> >> On MTL, objects allocated through i915_gem_object_create_internal() are
> >> mapped as uncached in GPU by default because HAS_LLC is false. However
> >> in the live_hwsp_read selftest these watcher objects are mapped as WB
> >> on CPU side. The conseqence is that the updates done by the GPU are not
> >> immediately visible to CPU, thus the selftest is randomly failing due to
> >> the stale data in CPU cache. Solution can be either setting WC for CPU +
> >> UC for GPU, or WB for CPU + 1-way coherent WB for GPU.
> >> To keep the consistency, let's simply inherit the same cache settings
> >> from the timeline, which is WB for CPU + 1-way coherent WB for GPU,
> >> because this test is supposed to emulate the behavior of the timeline
> >> anyway.
> >>
> >> Signed-off-by: Fei Yang <fei.yang@intel.com>
> >
> > It looks like there might be an indentation mistake on the second line
> > of the i915_gem_object_pin_map_unlocked() call, but we can fix that up
> > while applying; no need to re-send.
> >
> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>
> Thanks for reviewing.
>
> > Is there an FDO issue # for the random failures thar were being seen?
> > If so, we should add a Closes: or References: tag here.
>
> I'm not aware of a FDO filed for this failure. That might be because the
> issue is reproduced on MTL which might not be widely available to the
> community yet.
Yeah, I was thinking CI would have filed some, but I just remembered we
don't have public CI setup yet for MTL, so no automated bugs are coming
in yet.
Applied to drm-intel-gt-next. Thanks for the patch.
Matt
>
> > Matt
> >> ---
> >> drivers/gpu/drm/i915/gt/selftest_timeline.c | 14 +++++++++++---
> >> 1 file changed, 11 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gt/selftest_timeline.c b/drivers/gpu/drm/i915/gt/selftest_timeline.c
> >> index 522d0190509c..631aaed9bc3d 100644
> >> --- a/drivers/gpu/drm/i915/gt/selftest_timeline.c
> >> +++ b/drivers/gpu/drm/i915/gt/selftest_timeline.c
> >> @@ -825,7 +825,8 @@ static bool cmp_gte(u32 a, u32 b)
> >> return a >= b;
> >> }
> >>
> >> -static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt)
> >> +static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt,
> >> + struct intel_timeline *tl)
> >> {
> >> struct drm_i915_gem_object *obj;
> >> struct i915_vma *vma;
> >> @@ -834,7 +835,10 @@ static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt)
> >> if (IS_ERR(obj))
> >> return PTR_ERR(obj);
> >>
> >> - w->map = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
> >> + /* keep the same cache settings as timeline */
> >> + i915_gem_object_set_cache_coherency(obj, tl->hwsp_ggtt->obj->cache_level);
> >> + w->map = i915_gem_object_pin_map_unlocked(obj,
> >> + page_unmask_bits(tl->hwsp_ggtt->obj->mm.mapping));
> >> if (IS_ERR(w->map)) {
> >> i915_gem_object_put(obj);
> >> return PTR_ERR(w->map);
> >> @@ -1004,8 +1008,10 @@ static int live_hwsp_read(void *arg)
> >> if (!tl->has_initial_breadcrumb)
> >> goto out_free;
> >>
> >> + selftest_tl_pin(tl);
> >> +
> >> for (i = 0; i < ARRAY_SIZE(watcher); i++) {
> >> - err = setup_watcher(&watcher[i], gt);
> >> + err = setup_watcher(&watcher[i], gt, tl);
> >> if (err)
> >> goto out;
> >> }
> >> @@ -1160,6 +1166,8 @@ static int live_hwsp_read(void *arg)
> >> for (i = 0; i < ARRAY_SIZE(watcher); i++)
> >> cleanup_watcher(&watcher[i]);
> >>
> >> + intel_timeline_unpin(tl);
> >> +
> >> if (igt_flush_test(gt->i915))
> >> err = -EIO;
> >>
> >> --
> >> 2.25.1
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-17 16:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-08 23:28 [Intel-gfx] [PATCH] drm/i915/selftests: keep same cache settings as timeline fei.yang
2023-03-15 14:02 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
-- strict thread matches above, loose matches on Subject: below --
2023-03-15 18:08 [Intel-gfx] [PATCH] " fei.yang
2023-03-17 0:21 ` Matt Roper
2023-03-17 3:43 ` Yang, Fei
2023-03-17 16:38 ` Matt Roper
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox