| Message ID | 20230125100003.18243-1-nirmoy.das@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/mtl/selftests: Flush all tiles on test exit | expand |
Hi Nirmoy, On Wed, Jan 25, 2023 at 11:00:03AM +0100, Nirmoy Das wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > We want to idle all tiles when exiting selftests. > > Cc: Matt Roper <matthew.d.roper@intel.com> > 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> except from the tag list and the title I guess this is the same patch as the previous one, right? Can you please add some versioning next time? If it's the same this patch then it's good to be pushed. Just a little failure that is independent from this change. BTW, why is there a "mtl" prfix in the title while in the previous version it was "xehpsdv"? I can understand the latter because originally xehpsdv was a synonymous with multi-gt. But it's not the case anymore. If you don't mind I would remove it before pushing. Andi > --- > .../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/25/2023 3:36 PM, Andi Shyti wrote: > Hi Nirmoy, > > On Wed, Jan 25, 2023 at 11:00:03AM +0100, Nirmoy Das wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> We want to idle all tiles when exiting selftests. >> >> Cc: Matt Roper <matthew.d.roper@intel.com> >> 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> > except from the tag list and the title I guess this is the same > patch as the previous one, right? Can you please add some > versioning next time? I wasn't sure if I should add v2 for the title change. > > If it's the same this patch then it's good to be pushed. Just a > little failure that is independent from this change. > > BTW, why is there a "mtl" prfix in the title while in the > previous version it was "xehpsdv"? I can understand the latter > because originally xehpsdv was a synonymous with multi-gt. But > it's not the case anymore. If you don't mind I would remove it > before pushing. This confusion is because I didn't do the versioning properly :/ Sorry about that. Matt clarified in the v1 that we haven't enabled multi-tile for xehp, only for MTL we have multi-tile enabled. I actually tested this on MTL. Do you want me to resend with a added "v2: fix the title as we only support multi-tile for MTL now(Matt)" ? Regards, Nirmoy > > Andi > >> --- >> .../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 Nirmoy, > > On Wed, Jan 25, 2023 at 11:00:03AM +0100, Nirmoy Das wrote: > > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > > > We want to idle all tiles when exiting selftests. > > > > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > 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> > > except from the tag list and the title I guess this is the same > > patch as the previous one, right? Can you please add some > > versioning next time? > > > I wasn't sure if I should add v2 for the title change. > > > > > If it's the same this patch then it's good to be pushed. Just a > > little failure that is independent from this change. > > > > BTW, why is there a "mtl" prfix in the title while in the > > previous version it was "xehpsdv"? I can understand the latter > > because originally xehpsdv was a synonymous with multi-gt. But > > it's not the case anymore. If you don't mind I would remove it > > before pushing. > > > This confusion is because I didn't do the versioning properly :/ Sorry about > that. > > Matt clarified in the v1 that we haven't enabled multi-tile for xehp, only > for MTL we have multi-tile > > enabled. I actually tested this on MTL. > > > Do you want me to resend with a added "v2: fix the title as we only support > multi-tile for MTL now(Matt)" ? no need, I'll remove the mtl part. I think the title identifies only the file path. As we are at it, I have some patches for adding multitile to xehpdv, but I'm not sure we want them. Any opinion? Matt? Tvrtko? Andi > Regards, > > Nirmoy > > > > > Andi > > > > > --- > > > .../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/25/2023 4:10 PM, Andi Shyti wrote: > Hi Nirmoy, > >>> On Wed, Jan 25, 2023 at 11:00:03AM +0100, Nirmoy Das wrote: >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> We want to idle all tiles when exiting selftests. >>>> >>>> Cc: Matt Roper <matthew.d.roper@intel.com> >>>> 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> >>> except from the tag list and the title I guess this is the same >>> patch as the previous one, right? Can you please add some >>> versioning next time? >> >> I wasn't sure if I should add v2 for the title change. >> >>> If it's the same this patch then it's good to be pushed. Just a >>> little failure that is independent from this change. >>> >>> BTW, why is there a "mtl" prfix in the title while in the >>> previous version it was "xehpsdv"? I can understand the latter >>> because originally xehpsdv was a synonymous with multi-gt. But >>> it's not the case anymore. If you don't mind I would remove it >>> before pushing. >> >> This confusion is because I didn't do the versioning properly :/ Sorry about >> that. >> >> Matt clarified in the v1 that we haven't enabled multi-tile for xehp, only >> for MTL we have multi-tile >> >> enabled. I actually tested this on MTL. >> >> >> Do you want me to resend with a added "v2: fix the title as we only support >> multi-tile for MTL now(Matt)" ? > no need, I'll remove the mtl part. I think the title identifies > only the file path. Fine with me. Thanks Andi! Nirmoy > > As we are at it, I have some patches for adding multitile to > xehpdv, but I'm not sure we want them. Any opinion? Matt? Tvrtko? > > Andi > >> Regards, >> >> Nirmoy >> >>> Andi >>> >>>> --- >>>> .../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
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;