| Message ID | 20230124142212.18498-1-nirmoy.das@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/xehpsdv/selftests: Flush all tiles on test exit | expand |
Hi Nirmoy, On Tue, Jan 24, 2023 at 03:22:12PM +0100, Nirmoy Das wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > We want to idle all tiles when exiting selftests. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> ... > --- > .../gpu/drm/i915/selftests/igt_flush_test.c | 28 +++++++++++-------- > 1 file changed, 17 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/selftests/igt_flush_test.c b/drivers/gpu/drm/i915/selftests/igt_flush_test.c > index b484e12df417..29110abb4fe0 100644 > --- a/drivers/gpu/drm/i915/selftests/igt_flush_test.c > +++ b/drivers/gpu/drm/i915/selftests/igt_flush_test.c > @@ -14,21 +14,27 @@ > > int igt_flush_test(struct drm_i915_private *i915) > { > - struct intel_gt *gt = to_gt(i915); > - int ret = intel_gt_is_wedged(gt) ? -EIO : 0; > + struct intel_gt *gt; > + unsigned int i; > + int ret = 0; > > - cond_resched(); > + for_each_gt(gt, i915, i) { > + if (intel_gt_is_wedged(gt)) > + ret = -EIO; I'm just wondering if it makes sense to check if the gt is wedged. Andi > - if (intel_gt_wait_for_idle(gt, HZ * 3) == -ETIME) { > - pr_err("%pS timed out, cancelling all further testing.\n", > - __builtin_return_address(0)); > + cond_resched(); > > - GEM_TRACE("%pS timed out.\n", > - __builtin_return_address(0)); > - GEM_TRACE_DUMP(); > + if (intel_gt_wait_for_idle(gt, HZ * 3) == -ETIME) { > + pr_err("%pS timed out, cancelling all further testing.\n", > + __builtin_return_address(0)); > > - intel_gt_set_wedged(gt); > - ret = -EIO; > + GEM_TRACE("%pS timed out.\n", > + __builtin_return_address(0)); > + GEM_TRACE_DUMP(); > + > + intel_gt_set_wedged(gt); > + ret = -EIO; > + } > } > > return ret; > -- > 2.39.0
On Tue, Jan 24, 2023 at 03:22:12PM +0100, Nirmoy Das wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > We want to idle all tiles when exiting selftests. This test doesn't sound like it's specific to "xehpsdv." Was the patch title supposed to be "xehp" (the architecture, not the platform)? For that matter, we haven't even enabled multiple tiles on xehpsdv/pvc yet, so MTL is actually the only platform with multiple GTs at the moment. Matt > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> > --- > .../gpu/drm/i915/selftests/igt_flush_test.c | 28 +++++++++++-------- > 1 file changed, 17 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/selftests/igt_flush_test.c b/drivers/gpu/drm/i915/selftests/igt_flush_test.c > index b484e12df417..29110abb4fe0 100644 > --- a/drivers/gpu/drm/i915/selftests/igt_flush_test.c > +++ b/drivers/gpu/drm/i915/selftests/igt_flush_test.c > @@ -14,21 +14,27 @@ > > int igt_flush_test(struct drm_i915_private *i915) > { > - struct intel_gt *gt = to_gt(i915); > - int ret = intel_gt_is_wedged(gt) ? -EIO : 0; > + struct intel_gt *gt; > + unsigned int i; > + int ret = 0; > > - cond_resched(); > + for_each_gt(gt, i915, i) { > + if (intel_gt_is_wedged(gt)) > + ret = -EIO; > > - if (intel_gt_wait_for_idle(gt, HZ * 3) == -ETIME) { > - pr_err("%pS timed out, cancelling all further testing.\n", > - __builtin_return_address(0)); > + cond_resched(); > > - GEM_TRACE("%pS timed out.\n", > - __builtin_return_address(0)); > - GEM_TRACE_DUMP(); > + if (intel_gt_wait_for_idle(gt, HZ * 3) == -ETIME) { > + pr_err("%pS timed out, cancelling all further testing.\n", > + __builtin_return_address(0)); > > - intel_gt_set_wedged(gt); > - ret = -EIO; > + GEM_TRACE("%pS timed out.\n", > + __builtin_return_address(0)); > + GEM_TRACE_DUMP(); > + > + intel_gt_set_wedged(gt); > + ret = -EIO; > + } > } > > return ret; > -- > 2.39.0 >
On 1/24/2023 6:01 PM, Matt Roper wrote: > On Tue, Jan 24, 2023 at 03:22:12PM +0100, Nirmoy Das wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> We want to idle all tiles when exiting selftests. > This test doesn't sound like it's specific to "xehpsdv." Was the patch > title supposed to be "xehp" (the architecture, not the platform)? For > that matter, we haven't even enabled multiple tiles on xehpsdv/pvc yet, > so MTL is actually the only platform with multiple GTs at the moment. Thanks for correcting that. It is needed for MTL. I will resend with proper subject. Regards, Nirmoy > > > Matt > >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> >> --- >> .../gpu/drm/i915/selftests/igt_flush_test.c | 28 +++++++++++-------- >> 1 file changed, 17 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/selftests/igt_flush_test.c b/drivers/gpu/drm/i915/selftests/igt_flush_test.c >> index b484e12df417..29110abb4fe0 100644 >> --- a/drivers/gpu/drm/i915/selftests/igt_flush_test.c >> +++ b/drivers/gpu/drm/i915/selftests/igt_flush_test.c >> @@ -14,21 +14,27 @@ >> >> int igt_flush_test(struct drm_i915_private *i915) >> { >> - struct intel_gt *gt = to_gt(i915); >> - int ret = intel_gt_is_wedged(gt) ? -EIO : 0; >> + struct intel_gt *gt; >> + unsigned int i; >> + int ret = 0; >> >> - cond_resched(); >> + for_each_gt(gt, i915, i) { >> + if (intel_gt_is_wedged(gt)) >> + ret = -EIO; >> >> - if (intel_gt_wait_for_idle(gt, HZ * 3) == -ETIME) { >> - pr_err("%pS timed out, cancelling all further testing.\n", >> - __builtin_return_address(0)); >> + cond_resched(); >> >> - GEM_TRACE("%pS timed out.\n", >> - __builtin_return_address(0)); >> - GEM_TRACE_DUMP(); >> + if (intel_gt_wait_for_idle(gt, HZ * 3) == -ETIME) { >> + pr_err("%pS timed out, cancelling all further testing.\n", >> + __builtin_return_address(0)); >> >> - intel_gt_set_wedged(gt); >> - ret = -EIO; >> + GEM_TRACE("%pS timed out.\n", >> + __builtin_return_address(0)); >> + GEM_TRACE_DUMP(); >> + >> + intel_gt_set_wedged(gt); >> + ret = -EIO; >> + } >> } >> >> return ret; >> -- >> 2.39.0 >>
Hi Andi, On 1/24/2023 5:10 PM, Andi Shyti wrote: > Hi Nirmoy, > > On Tue, Jan 24, 2023 at 03:22:12PM +0100, Nirmoy Das wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> We want to idle all tiles when exiting selftests. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Thanks. > > ... > >> --- >> .../gpu/drm/i915/selftests/igt_flush_test.c | 28 +++++++++++-------- >> 1 file changed, 17 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/selftests/igt_flush_test.c b/drivers/gpu/drm/i915/selftests/igt_flush_test.c >> index b484e12df417..29110abb4fe0 100644 >> --- a/drivers/gpu/drm/i915/selftests/igt_flush_test.c >> +++ b/drivers/gpu/drm/i915/selftests/igt_flush_test.c >> @@ -14,21 +14,27 @@ >> >> int igt_flush_test(struct drm_i915_private *i915) >> { >> - struct intel_gt *gt = to_gt(i915); >> - int ret = intel_gt_is_wedged(gt) ? -EIO : 0; >> + struct intel_gt *gt; >> + unsigned int i; >> + int ret = 0; >> >> - cond_resched(); >> + for_each_gt(gt, i915, i) { >> + if (intel_gt_is_wedged(gt)) >> + ret = -EIO; > I'm just wondering if it makes sense to check if the gt is > wedged. Could you please expand more, what are your thoughts about this ? Nirmoy > > Andi > >> - if (intel_gt_wait_for_idle(gt, HZ * 3) == -ETIME) { >> - pr_err("%pS timed out, cancelling all further testing.\n", >> - __builtin_return_address(0)); >> + cond_resched(); >> >> - GEM_TRACE("%pS timed out.\n", >> - __builtin_return_address(0)); >> - GEM_TRACE_DUMP(); >> + if (intel_gt_wait_for_idle(gt, HZ * 3) == -ETIME) { >> + pr_err("%pS timed out, cancelling all further testing.\n", >> + __builtin_return_address(0)); >> >> - intel_gt_set_wedged(gt); >> - ret = -EIO; >> + GEM_TRACE("%pS timed out.\n", >> + __builtin_return_address(0)); >> + GEM_TRACE_DUMP(); >> + >> + intel_gt_set_wedged(gt); >> + ret = -EIO; >> + } >> } >> >> return ret; >> -- >> 2.39.0
Hi Nirmoy, > > > + struct intel_gt *gt; > > > + unsigned int i; > > > + int ret = 0; > > > - cond_resched(); > > > + for_each_gt(gt, i915, i) { > > > + if (intel_gt_is_wedged(gt)) > > > + ret = -EIO; > > I'm just wondering if it makes sense to check if the gt is > > wedged. > > Could you please expand more, what are your thoughts about this ? if we are wedged, I do expect the wait_for_idle to fail and not having any pending job. But nevertheless it's not that important, it's just the way this function is organized that makes me raise an eyebrow. My r-b stands, still. Andi [...] > > > + cond_resched(); > > > - GEM_TRACE("%pS timed out.\n", > > > - __builtin_return_address(0)); > > > - GEM_TRACE_DUMP(); > > > + if (intel_gt_wait_for_idle(gt, HZ * 3) == -ETIME) { > > > + pr_err("%pS timed out, cancelling all further testing.\n", > > > + __builtin_return_address(0)); > > > - intel_gt_set_wedged(gt); > > > - ret = -EIO; > > > + GEM_TRACE("%pS timed out.\n", > > > + __builtin_return_address(0)); > > > + GEM_TRACE_DUMP(); > > > + > > > + intel_gt_set_wedged(gt); > > > + ret = -EIO; > > > + } > > > } > > > return ret; > > > -- > > > 2.39.0
diff --git a/drivers/gpu/drm/i915/selftests/igt_flush_test.c b/drivers/gpu/drm/i915/selftests/igt_flush_test.c index b484e12df417..29110abb4fe0 100644 --- a/drivers/gpu/drm/i915/selftests/igt_flush_test.c +++ b/drivers/gpu/drm/i915/selftests/igt_flush_test.c @@ -14,21 +14,27 @@ int igt_flush_test(struct drm_i915_private *i915) { - struct intel_gt *gt = to_gt(i915); - int ret = intel_gt_is_wedged(gt) ? -EIO : 0; + struct intel_gt *gt; + unsigned int i; + int ret = 0; - cond_resched(); + for_each_gt(gt, i915, i) { + if (intel_gt_is_wedged(gt)) + ret = -EIO; - if (intel_gt_wait_for_idle(gt, HZ * 3) == -ETIME) { - pr_err("%pS timed out, cancelling all further testing.\n", - __builtin_return_address(0)); + cond_resched(); - GEM_TRACE("%pS timed out.\n", - __builtin_return_address(0)); - GEM_TRACE_DUMP(); + if (intel_gt_wait_for_idle(gt, HZ * 3) == -ETIME) { + pr_err("%pS timed out, cancelling all further testing.\n", + __builtin_return_address(0)); - intel_gt_set_wedged(gt); - ret = -EIO; + GEM_TRACE("%pS timed out.\n", + __builtin_return_address(0)); + GEM_TRACE_DUMP(); + + intel_gt_set_wedged(gt); + ret = -EIO; + } } return ret;