| Message ID | 20230317055239.1313175-1-tejas.upadhyay@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gt: Consider multi-gt at all places | expand |
On 17/03/2023 05:52, Tejas Upadhyay wrote: > In order to make igt_live_test work in proper > way, we need to consider multi-gt in all tests > where igt_live_test is used as well as at other > random places where multi-gt should be considered. Description is a bit vague - is this for Meteorlake in general? What is the "proper way" ie what is broken? > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com> > --- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ++-- > .../drm/i915/gem/selftests/i915_gem_context.c | 28 ++++---- > drivers/gpu/drm/i915/gt/intel_engine_user.c | 2 +- > drivers/gpu/drm/i915/gt/selftest_execlists.c | 68 +++++++++---------- > drivers/gpu/drm/i915/selftests/i915_request.c | 36 +++++----- > .../gpu/drm/i915/selftests/igt_live_test.c | 10 +-- > .../gpu/drm/i915/selftests/igt_live_test.h | 4 +- > 7 files changed, 81 insertions(+), 80 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index 9dce2957b4e5..289b75ac39e1 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -2449,9 +2449,9 @@ static int eb_submit(struct i915_execbuffer *eb) > return err; > } > > -static int num_vcs_engines(struct drm_i915_private *i915) > +static int num_vcs_engines(struct intel_gt *gt) > { > - return hweight_long(VDBOX_MASK(to_gt(i915))); > + return hweight_long(VDBOX_MASK(gt)); > } > > /* > @@ -2459,7 +2459,7 @@ static int num_vcs_engines(struct drm_i915_private *i915) > * The engine index is returned. > */ > static unsigned int > -gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv, > +gen8_dispatch_bsd_engine(struct intel_gt *gt, > struct drm_file *file) > { > struct drm_i915_file_private *file_priv = file->driver_priv; > @@ -2467,7 +2467,7 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv, > /* Check whether the file_priv has already selected one ring. */ > if ((int)file_priv->bsd_engine < 0) > file_priv->bsd_engine = > - get_random_u32_below(num_vcs_engines(dev_priv)); > + get_random_u32_below(num_vcs_engines(gt)); > > return file_priv->bsd_engine; > } > @@ -2644,6 +2644,7 @@ static unsigned int > eb_select_legacy_ring(struct i915_execbuffer *eb) > { > struct drm_i915_private *i915 = eb->i915; > + struct intel_gt *gt = eb->gt; > struct drm_i915_gem_execbuffer2 *args = eb->args; > unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK; > > @@ -2655,11 +2656,11 @@ eb_select_legacy_ring(struct i915_execbuffer *eb) > return -1; > } > > - if (user_ring_id == I915_EXEC_BSD && num_vcs_engines(i915) > 1) { > + if (user_ring_id == I915_EXEC_BSD && num_vcs_engines(gt) > 1) { > unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK; > > if (bsd_idx == I915_EXEC_BSD_DEFAULT) { > - bsd_idx = gen8_dispatch_bsd_engine(i915, eb->file); > + bsd_idx = gen8_dispatch_bsd_engine(gt, eb->file); > } else if (bsd_idx >= I915_EXEC_BSD_RING1 && > bsd_idx <= I915_EXEC_BSD_RING2) { > bsd_idx >>= I915_EXEC_BSD_SHIFT; The hunks above I don't think are correct. Execbuf is in principle based on uabi engines, and that is not a per GT concept. There is also no functional change above so I can only guess it is a prep work for something? [snip] > -int igt_live_test_end(struct igt_live_test *t) > +int igt_live_test_end(struct igt_live_test *t, struct intel_gt *gt) > { > - struct drm_i915_private *i915 = t->i915; > + struct drm_i915_private *i915 = gt->i915; > struct intel_engine_cs *engine; > enum intel_engine_id id; > > @@ -57,7 +57,7 @@ int igt_live_test_end(struct igt_live_test *t) > return -EIO; > } > > - for_each_engine(engine, to_gt(i915), id) { > + for_each_engine(engine, gt, id) { > if (t->reset_engine[id] == > i915_reset_engine_count(&i915->gpu_error, engine)) > continue; > diff --git a/drivers/gpu/drm/i915/selftests/igt_live_test.h b/drivers/gpu/drm/i915/selftests/igt_live_test.h > index 36ed42736c52..209b0548c603 100644 > --- a/drivers/gpu/drm/i915/selftests/igt_live_test.h > +++ b/drivers/gpu/drm/i915/selftests/igt_live_test.h > @@ -27,9 +27,9 @@ struct igt_live_test { > * e.g. if the GPU was reset. > */ > int igt_live_test_begin(struct igt_live_test *t, > - struct drm_i915_private *i915, > + struct intel_gt *gt, > const char *func, > const char *name); > -int igt_live_test_end(struct igt_live_test *t); > +int igt_live_test_end(struct igt_live_test *t, struct intel_gt *gt); Back in the day the plan was that live selftests are device focused and then we also have intel_gt_live_subtests, which are obviously GT focused. So in that sense adding a single GT parameter to igt_live_test_begin isn't something I immediately understand. Could you explain in one or two practical examples what is not working properly and how is this patch fixing it? Regards, Tvrtko
Hi Tejas, On Fri, Mar 17, 2023 at 11:22:39AM +0530, Tejas Upadhyay wrote: > In order to make igt_live_test work in proper > way, we need to consider multi-gt in all tests > where igt_live_test is used as well as at other > random places where multi-gt should be considered. > > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com> Everything looks good, but many things could potentially go wrong when changing i915 to gt. I would like to see some positive results from the CI before proceeding. I will take care of resubmitting the tests once CI is back from holiday. Thanks Tejas, Andi
Hi Tejas, On Tue, Mar 21, 2023 at 01:51:36PM -0000, Patchwork wrote: > == Series Details == > > Series: drm/i915/gt: Consider multi-gt at all places (rev2) > URL : https://patchwork.freedesktop.org/series/115302/ > State : failure > > == Summary == > > Error: patch https://patchwork.freedesktop.org/api/1.0/series/115302/revisions/2/mbox/ not applied > Applying: drm/i915/gt: Consider multi-gt at all places > Using index info to reconstruct a base tree... > M drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > Falling back to patching base and 3-way merge... > Auto-merging drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > error: Failed to merge in the changes. > hint: Use 'git am --show-current-patch=diff' to see the failed patch > Patch failed at 0001 drm/i915/gt: Consider multi-gt at all places > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > Build failed, no error log produced I think you need a rebase here. Andi
Hi, > > diff --git a/drivers/gpu/drm/i915/selftests/igt_live_test.h b/drivers/gpu/drm/i915/selftests/igt_live_test.h > > index 36ed42736c52..209b0548c603 100644 > > --- a/drivers/gpu/drm/i915/selftests/igt_live_test.h > > +++ b/drivers/gpu/drm/i915/selftests/igt_live_test.h > > @@ -27,9 +27,9 @@ struct igt_live_test { > > * e.g. if the GPU was reset. > > */ > > int igt_live_test_begin(struct igt_live_test *t, > > - struct drm_i915_private *i915, > > + struct intel_gt *gt, > > const char *func, > > const char *name); > > -int igt_live_test_end(struct igt_live_test *t); > > +int igt_live_test_end(struct igt_live_test *t, struct intel_gt *gt); > > Back in the day the plan was that live selftests are device focused and then > we also have intel_gt_live_subtests, which are obviously GT focused. So in > that sense adding a single GT parameter to igt_live_test_begin isn't > something I immediately understand. > > Could you explain in one or two practical examples what is not working > properly and how is this patch fixing it? Tejas, would it help to split this patch in several patches? Andi
Sorry for late response. Inline responses below, > -----Original Message----- > From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Sent: Friday, March 17, 2023 2:46 PM > To: Upadhyay, Tejas <tejas.upadhyay@intel.com>; Intel- > GFX@Lists.FreeDesktop.Org > Subject: Re: [Intel-gfx] [PATCH] drm/i915/gt: Consider multi-gt at all places > > > On 17/03/2023 05:52, Tejas Upadhyay wrote: > > In order to make igt_live_test work in proper way, we need to consider > > multi-gt in all tests where igt_live_test is used as well as at other > > random places where multi-gt should be considered. > > Description is a bit vague - is this for Meteorlake in general? What is the > "proper way" ie what is broken? > > > Cc: Andi Shyti <andi.shyti@linux.intel.com> > > Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com> > > --- > > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ++-- > > .../drm/i915/gem/selftests/i915_gem_context.c | 28 ++++---- > > drivers/gpu/drm/i915/gt/intel_engine_user.c | 2 +- > > drivers/gpu/drm/i915/gt/selftest_execlists.c | 68 +++++++++---------- > > drivers/gpu/drm/i915/selftests/i915_request.c | 36 +++++----- > > .../gpu/drm/i915/selftests/igt_live_test.c | 10 +-- > > .../gpu/drm/i915/selftests/igt_live_test.h | 4 +- > > 7 files changed, 81 insertions(+), 80 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > index 9dce2957b4e5..289b75ac39e1 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > @@ -2449,9 +2449,9 @@ static int eb_submit(struct i915_execbuffer *eb) > > return err; > > } > > > > -static int num_vcs_engines(struct drm_i915_private *i915) > > +static int num_vcs_engines(struct intel_gt *gt) > > { > > - return hweight_long(VDBOX_MASK(to_gt(i915))); > > + return hweight_long(VDBOX_MASK(gt)); > > } > > > > /* > > @@ -2459,7 +2459,7 @@ static int num_vcs_engines(struct > drm_i915_private *i915) > > * The engine index is returned. > > */ > > static unsigned int > > -gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv, > > +gen8_dispatch_bsd_engine(struct intel_gt *gt, > > struct drm_file *file) > > { > > struct drm_i915_file_private *file_priv = file->driver_priv; @@ > > -2467,7 +2467,7 @@ gen8_dispatch_bsd_engine(struct drm_i915_private > *dev_priv, > > /* Check whether the file_priv has already selected one ring. */ > > if ((int)file_priv->bsd_engine < 0) > > file_priv->bsd_engine = > > - > get_random_u32_below(num_vcs_engines(dev_priv)); > > + get_random_u32_below(num_vcs_engines(gt)); > > > > return file_priv->bsd_engine; > > } > > @@ -2644,6 +2644,7 @@ static unsigned int > > eb_select_legacy_ring(struct i915_execbuffer *eb) > > { > > struct drm_i915_private *i915 = eb->i915; > > + struct intel_gt *gt = eb->gt; > > struct drm_i915_gem_execbuffer2 *args = eb->args; > > unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK; > > > > @@ -2655,11 +2656,11 @@ eb_select_legacy_ring(struct i915_execbuffer > *eb) > > return -1; > > } > > > > - if (user_ring_id == I915_EXEC_BSD && num_vcs_engines(i915) > 1) { > > + if (user_ring_id == I915_EXEC_BSD && num_vcs_engines(gt) > 1) { > > unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK; > > > > if (bsd_idx == I915_EXEC_BSD_DEFAULT) { > > - bsd_idx = gen8_dispatch_bsd_engine(i915, eb->file); > > + bsd_idx = gen8_dispatch_bsd_engine(gt, eb->file); > > } else if (bsd_idx >= I915_EXEC_BSD_RING1 && > > bsd_idx <= I915_EXEC_BSD_RING2) { > > bsd_idx >>= I915_EXEC_BSD_SHIFT; > > The hunks above I don't think are correct. Execbuf is in principle based on > uabi engines, and that is not a per GT concept. > > There is also no functional change above so I can only guess it is a prep work > for something? This I think you remove with below patch, so no more discussion required : commit 927fb9c5ef6ae385c65ae04b181cc2ee94663e28 Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Date: Thu Mar 16 14:27:28 2023 +0000 drm/i915: Simplify vcs/bsd engine selection > > [snip] > > > -int igt_live_test_end(struct igt_live_test *t) > > +int igt_live_test_end(struct igt_live_test *t, struct intel_gt *gt) > > { > > - struct drm_i915_private *i915 = t->i915; > > + struct drm_i915_private *i915 = gt->i915; > > struct intel_engine_cs *engine; > > enum intel_engine_id id; > > > > @@ -57,7 +57,7 @@ int igt_live_test_end(struct igt_live_test *t) > > return -EIO; > > } > > > > - for_each_engine(engine, to_gt(i915), id) { > > + for_each_engine(engine, gt, id) { > > if (t->reset_engine[id] == > > i915_reset_engine_count(&i915->gpu_error, engine)) > > continue; > > diff --git a/drivers/gpu/drm/i915/selftests/igt_live_test.h > > b/drivers/gpu/drm/i915/selftests/igt_live_test.h > > index 36ed42736c52..209b0548c603 100644 > > --- a/drivers/gpu/drm/i915/selftests/igt_live_test.h > > +++ b/drivers/gpu/drm/i915/selftests/igt_live_test.h > > @@ -27,9 +27,9 @@ struct igt_live_test { > > * e.g. if the GPU was reset. > > */ > > int igt_live_test_begin(struct igt_live_test *t, > > - struct drm_i915_private *i915, > > + struct intel_gt *gt, > > const char *func, > > const char *name); > > -int igt_live_test_end(struct igt_live_test *t); > > +int igt_live_test_end(struct igt_live_test *t, struct intel_gt *gt); > > Back in the day the plan was that live selftests are device focused and then > we also have intel_gt_live_subtests, which are obviously GT focused. So in > that sense adding a single GT parameter to igt_live_test_begin isn't > something I immediately understand. > > Could you explain in one or two practical examples what is not working > properly and how is this patch fixing it? For example you are running test "live_all_engines(void *arg)", -- Below test begin, will reset counters for primary GT - Tile0 -- err = igt_live_test_begin(&t, to_gt(i915), __func__, ""); if (err) goto out_free; --- Now we loop for all engines, note here for MTL vcs, vecs engines are not on primary GT or tile 0, So counters did not reset on test begin does not cover them. --- In test_begin, below will not reset count for vcs, vecs engines on MTL, for_each_engine(engine, gt, id) t->reset_engine[id] = i915_reset_engine_count(&i915->gpu_error, engine); --- Then below will end test, again for primary GT where above mentioned engines are not there --- err = igt_live_test_end(&t, to_gt(i915)); In short to me it looks like igt_live_test for device needs attention when we have different engines on different GTs like MTL. Regards, Tejas > > Regards, > > Tvrtko
On 05/04/2023 07:56, Upadhyay, Tejas wrote: >>> -int igt_live_test_end(struct igt_live_test *t) >>> +int igt_live_test_end(struct igt_live_test *t, struct intel_gt *gt) >>> { >>> - struct drm_i915_private *i915 = t->i915; >>> + struct drm_i915_private *i915 = gt->i915; >>> struct intel_engine_cs *engine; >>> enum intel_engine_id id; >>> >>> @@ -57,7 +57,7 @@ int igt_live_test_end(struct igt_live_test *t) >>> return -EIO; >>> } >>> >>> - for_each_engine(engine, to_gt(i915), id) { >>> + for_each_engine(engine, gt, id) { >>> if (t->reset_engine[id] == >>> i915_reset_engine_count(&i915->gpu_error, engine)) >>> continue; >>> diff --git a/drivers/gpu/drm/i915/selftests/igt_live_test.h >>> b/drivers/gpu/drm/i915/selftests/igt_live_test.h >>> index 36ed42736c52..209b0548c603 100644 >>> --- a/drivers/gpu/drm/i915/selftests/igt_live_test.h >>> +++ b/drivers/gpu/drm/i915/selftests/igt_live_test.h >>> @@ -27,9 +27,9 @@ struct igt_live_test { >>> * e.g. if the GPU was reset. >>> */ >>> int igt_live_test_begin(struct igt_live_test *t, >>> - struct drm_i915_private *i915, >>> + struct intel_gt *gt, >>> const char *func, >>> const char *name); >>> -int igt_live_test_end(struct igt_live_test *t); >>> +int igt_live_test_end(struct igt_live_test *t, struct intel_gt *gt); >> >> Back in the day the plan was that live selftests are device focused and then >> we also have intel_gt_live_subtests, which are obviously GT focused. So in >> that sense adding a single GT parameter to igt_live_test_begin isn't >> something I immediately understand. >> >> Could you explain in one or two practical examples what is not working >> properly and how is this patch fixing it? > > For example you are running test "live_all_engines(void *arg)", > > -- Below test begin, will reset counters for primary GT - Tile0 -- > err = igt_live_test_begin(&t, to_gt(i915), __func__, ""); > if (err) > goto out_free; > > --- Now we loop for all engines, note here for MTL vcs, vecs engines are not on primary GT or tile 0, > So counters did not reset on test begin does not cover them. --- > > In test_begin, below will not reset count for vcs, vecs engines on MTL, > for_each_engine(engine, gt, id) > t->reset_engine[id] = > i915_reset_engine_count(&i915->gpu_error, engine); > > --- Then below will end test, again for primary GT where above mentioned engines are not there --- > err = igt_live_test_end(&t, to_gt(i915)); > > In short to me it looks like igt_live_test for device needs attention when we have different engines on different GTs like MTL. If you either add for_each_gt to that for_each_engine in igt_live_test_begin and igt_live_test_end, or alternatively for_each_uabi_engine (maybe misses some internal engines?), everything works? That would be much smaller patch and wouldn't falsely associate live tests with a single gt. Regards, Tvrtko
> -----Original Message----- > From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Sent: Wednesday, April 5, 2023 2:01 PM > To: Upadhyay, Tejas <tejas.upadhyay@intel.com>; Intel- > GFX@Lists.FreeDesktop.Org > Subject: Re: [Intel-gfx] [PATCH] drm/i915/gt: Consider multi-gt at all places > > > On 05/04/2023 07:56, Upadhyay, Tejas wrote: > >>> -int igt_live_test_end(struct igt_live_test *t) > >>> +int igt_live_test_end(struct igt_live_test *t, struct intel_gt *gt) > >>> { > >>> - struct drm_i915_private *i915 = t->i915; > >>> + struct drm_i915_private *i915 = gt->i915; > >>> struct intel_engine_cs *engine; > >>> enum intel_engine_id id; > >>> > >>> @@ -57,7 +57,7 @@ int igt_live_test_end(struct igt_live_test *t) > >>> return -EIO; > >>> } > >>> > >>> - for_each_engine(engine, to_gt(i915), id) { > >>> + for_each_engine(engine, gt, id) { > >>> if (t->reset_engine[id] == > >>> i915_reset_engine_count(&i915->gpu_error, engine)) > >>> continue; > >>> diff --git a/drivers/gpu/drm/i915/selftests/igt_live_test.h > >>> b/drivers/gpu/drm/i915/selftests/igt_live_test.h > >>> index 36ed42736c52..209b0548c603 100644 > >>> --- a/drivers/gpu/drm/i915/selftests/igt_live_test.h > >>> +++ b/drivers/gpu/drm/i915/selftests/igt_live_test.h > >>> @@ -27,9 +27,9 @@ struct igt_live_test { > >>> * e.g. if the GPU was reset. > >>> */ > >>> int igt_live_test_begin(struct igt_live_test *t, > >>> - struct drm_i915_private *i915, > >>> + struct intel_gt *gt, > >>> const char *func, > >>> const char *name); > >>> -int igt_live_test_end(struct igt_live_test *t); > >>> +int igt_live_test_end(struct igt_live_test *t, struct intel_gt > >>> +*gt); > >> > >> Back in the day the plan was that live selftests are device focused > >> and then we also have intel_gt_live_subtests, which are obviously GT > >> focused. So in that sense adding a single GT parameter to > >> igt_live_test_begin isn't something I immediately understand. > >> > >> Could you explain in one or two practical examples what is not > >> working properly and how is this patch fixing it? > > > > For example you are running test "live_all_engines(void *arg)", > > > > -- Below test begin, will reset counters for primary GT - Tile0 -- err > > = igt_live_test_begin(&t, to_gt(i915), __func__, ""); > > if (err) > > goto out_free; > > > > --- Now we loop for all engines, note here for MTL vcs, vecs engines are not > on primary GT or tile 0, > > So counters did not reset on test begin does not cover them. --- > > > > In test_begin, below will not reset count for vcs, vecs engines on MTL, > > for_each_engine(engine, gt, id) > > t->reset_engine[id] = > > i915_reset_engine_count(&i915->gpu_error, > > engine); > > > > --- Then below will end test, again for primary GT where above > > mentioned engines are not there --- err = igt_live_test_end(&t, > > to_gt(i915)); > > > > In short to me it looks like igt_live_test for device needs attention when we > have different engines on different GTs like MTL. > > If you either add for_each_gt to that for_each_engine in igt_live_test_begin > and igt_live_test_end, or alternatively for_each_uabi_engine (maybe misses > some internal engines?), everything works? That would be much smaller > patch and wouldn't falsely associate live tests with a single gt. > Below would work, if you agree I will rearrange and send patch. --- a/drivers/gpu/drm/i915/selftests/igt_live_test.c +++ b/drivers/gpu/drm/i915/selftests/igt_live_test.c @@ -16,28 +16,31 @@ int igt_live_test_begin(struct igt_live_test *t, const char *func, const char *name) { - struct intel_gt *gt = to_gt(i915); + struct intel_gt *gt; struct intel_engine_cs *engine; enum intel_engine_id id; int err; + unsigned int i; - t->i915 = i915; - t->func = func; - t->name = name; + for_each_gt(gt, i915, i) { + t->i915 = i915; + t->func = func; + t->name = name; - err = intel_gt_wait_for_idle(gt, MAX_SCHEDULE_TIMEOUT); - if (err) { - pr_err("%s(%s): failed to idle before, with err=%d!", - func, name, err); - return err; - } + err = intel_gt_wait_for_idle(gt, MAX_SCHEDULE_TIMEOUT); + if (err) { + pr_err("%s(%s): failed to idle before, with err=%d!", + func, name, err); + return err; + } - t->reset_global = i915_reset_count(&i915->gpu_error); + t->reset_global = i915_reset_count(&i915->gpu_error); - for_each_engine(engine, gt, id) - t->reset_engine[id] = + for_each_engine(engine, gt, id) + t->reset_engine[id] = i915_reset_engine_count(&i915->gpu_error, engine); + } return 0; } @@ -46,6 +49,7 @@ int igt_live_test_end(struct igt_live_test *t) struct drm_i915_private *i915 = t->i915; struct intel_engine_cs *engine; enum intel_engine_id id; + unsigned int i; if (igt_flush_test(i915)) return -EIO; @@ -57,17 +61,18 @@ int igt_live_test_end(struct igt_live_test *t) return -EIO; } - for_each_engine(engine, to_gt(i915), id) { - if (t->reset_engine[id] == - i915_reset_engine_count(&i915->gpu_error, engine)) - continue; + for_each_gt(gt, i915, i) { + for_each_engine(engine, gt, id) { + if (t->reset_engine[id] == + i915_reset_engine_count(&i915->gpu_error, engine)) + continue; - pr_err("%s(%s): engine '%s' was reset %d times!\n", - t->func, t->name, engine->name, - i915_reset_engine_count(&i915->gpu_error, engine) - - t->reset_engine[id]); - return -EIO; + pr_err("%s(%s): engine '%s' was reset %d times!\n", + t->func, t->name, engine->name, + i915_reset_engine_count(&i915->gpu_error, engine) - + t->reset_engine[id]); + return -EIO; + } } Regards, Tejas > Regards, > > Tvrtko
On 17/04/2023 19:13, Upadhyay, Tejas wrote: > > >> -----Original Message----- >> From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >> Sent: Wednesday, April 5, 2023 2:01 PM >> To: Upadhyay, Tejas <tejas.upadhyay@intel.com>; Intel- >> GFX@Lists.FreeDesktop.Org >> Subject: Re: [Intel-gfx] [PATCH] drm/i915/gt: Consider multi-gt at all places >> >> >> On 05/04/2023 07:56, Upadhyay, Tejas wrote: >>>>> -int igt_live_test_end(struct igt_live_test *t) >>>>> +int igt_live_test_end(struct igt_live_test *t, struct intel_gt *gt) >>>>> { >>>>> - struct drm_i915_private *i915 = t->i915; >>>>> + struct drm_i915_private *i915 = gt->i915; >>>>> struct intel_engine_cs *engine; >>>>> enum intel_engine_id id; >>>>> >>>>> @@ -57,7 +57,7 @@ int igt_live_test_end(struct igt_live_test *t) >>>>> return -EIO; >>>>> } >>>>> >>>>> - for_each_engine(engine, to_gt(i915), id) { >>>>> + for_each_engine(engine, gt, id) { >>>>> if (t->reset_engine[id] == >>>>> i915_reset_engine_count(&i915->gpu_error, engine)) >>>>> continue; >>>>> diff --git a/drivers/gpu/drm/i915/selftests/igt_live_test.h >>>>> b/drivers/gpu/drm/i915/selftests/igt_live_test.h >>>>> index 36ed42736c52..209b0548c603 100644 >>>>> --- a/drivers/gpu/drm/i915/selftests/igt_live_test.h >>>>> +++ b/drivers/gpu/drm/i915/selftests/igt_live_test.h >>>>> @@ -27,9 +27,9 @@ struct igt_live_test { >>>>> * e.g. if the GPU was reset. >>>>> */ >>>>> int igt_live_test_begin(struct igt_live_test *t, >>>>> - struct drm_i915_private *i915, >>>>> + struct intel_gt *gt, >>>>> const char *func, >>>>> const char *name); >>>>> -int igt_live_test_end(struct igt_live_test *t); >>>>> +int igt_live_test_end(struct igt_live_test *t, struct intel_gt >>>>> +*gt); >>>> >>>> Back in the day the plan was that live selftests are device focused >>>> and then we also have intel_gt_live_subtests, which are obviously GT >>>> focused. So in that sense adding a single GT parameter to >>>> igt_live_test_begin isn't something I immediately understand. >>>> >>>> Could you explain in one or two practical examples what is not >>>> working properly and how is this patch fixing it? >>> >>> For example you are running test "live_all_engines(void *arg)", >>> >>> -- Below test begin, will reset counters for primary GT - Tile0 -- err >>> = igt_live_test_begin(&t, to_gt(i915), __func__, ""); >>> if (err) >>> goto out_free; >>> >>> --- Now we loop for all engines, note here for MTL vcs, vecs engines are not >> on primary GT or tile 0, >>> So counters did not reset on test begin does not cover them. --- >>> >>> In test_begin, below will not reset count for vcs, vecs engines on MTL, >>> for_each_engine(engine, gt, id) >>> t->reset_engine[id] = >>> i915_reset_engine_count(&i915->gpu_error, >>> engine); >>> >>> --- Then below will end test, again for primary GT where above >>> mentioned engines are not there --- err = igt_live_test_end(&t, >>> to_gt(i915)); >>> >>> In short to me it looks like igt_live_test for device needs attention when we >> have different engines on different GTs like MTL. >> >> If you either add for_each_gt to that for_each_engine in igt_live_test_begin >> and igt_live_test_end, or alternatively for_each_uabi_engine (maybe misses >> some internal engines?), everything works? That would be much smaller >> patch and wouldn't falsely associate live tests with a single gt. >> > > Below would work, if you agree I will rearrange and send patch. > > --- a/drivers/gpu/drm/i915/selftests/igt_live_test.c > +++ b/drivers/gpu/drm/i915/selftests/igt_live_test.c > @@ -16,28 +16,31 @@ int igt_live_test_begin(struct igt_live_test *t, > const char *func, > const char *name) > { > - struct intel_gt *gt = to_gt(i915); > + struct intel_gt *gt; > struct intel_engine_cs *engine; > enum intel_engine_id id; > int err; > + unsigned int i; > > - t->i915 = i915; > - t->func = func; > - t->name = name; > + for_each_gt(gt, i915, i) { > + t->i915 = i915; > + t->func = func; > + t->name = name; These three assignments should stay outside the outer loop. > > - err = intel_gt_wait_for_idle(gt, MAX_SCHEDULE_TIMEOUT); > - if (err) { > - pr_err("%s(%s): failed to idle before, with err=%d!", > - func, name, err); > - return err; > - } > + err = intel_gt_wait_for_idle(gt, MAX_SCHEDULE_TIMEOUT); > + if (err) { > + pr_err("%s(%s): failed to idle before, with err=%d!", > + func, name, err); > + return err; > + } > > - t->reset_global = i915_reset_count(&i915->gpu_error); > + t->reset_global = i915_reset_count(&i915->gpu_error); Ditto. Rest looks good to me. Regards, Tvrtko > > - for_each_engine(engine, gt, id) > - t->reset_engine[id] = > + for_each_engine(engine, gt, id) > + t->reset_engine[id] = > i915_reset_engine_count(&i915->gpu_error, engine); > > + } > return 0; > } > > @@ -46,6 +49,7 @@ int igt_live_test_end(struct igt_live_test *t) > struct drm_i915_private *i915 = t->i915; > struct intel_engine_cs *engine; > enum intel_engine_id id; > + unsigned int i; > > if (igt_flush_test(i915)) > return -EIO; > @@ -57,17 +61,18 @@ int igt_live_test_end(struct igt_live_test *t) > return -EIO; > } > > - for_each_engine(engine, to_gt(i915), id) { > - if (t->reset_engine[id] == > - i915_reset_engine_count(&i915->gpu_error, engine)) > - continue; > + for_each_gt(gt, i915, i) { > + for_each_engine(engine, gt, id) { > + if (t->reset_engine[id] == > + i915_reset_engine_count(&i915->gpu_error, engine)) > + continue; > > - pr_err("%s(%s): engine '%s' was reset %d times!\n", > - t->func, t->name, engine->name, > - i915_reset_engine_count(&i915->gpu_error, engine) - > - t->reset_engine[id]); > - return -EIO; > + pr_err("%s(%s): engine '%s' was reset %d times!\n", > + t->func, t->name, engine->name, > + i915_reset_engine_count(&i915->gpu_error, engine) - > + t->reset_engine[id]); > + return -EIO; > + } > } > > Regards, > Tejas >> Regards, >> >> Tvrtko
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 9dce2957b4e5..289b75ac39e1 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -2449,9 +2449,9 @@ static int eb_submit(struct i915_execbuffer *eb) return err; } -static int num_vcs_engines(struct drm_i915_private *i915) +static int num_vcs_engines(struct intel_gt *gt) { - return hweight_long(VDBOX_MASK(to_gt(i915))); + return hweight_long(VDBOX_MASK(gt)); } /* @@ -2459,7 +2459,7 @@ static int num_vcs_engines(struct drm_i915_private *i915) * The engine index is returned. */ static unsigned int -gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv, +gen8_dispatch_bsd_engine(struct intel_gt *gt, struct drm_file *file) { struct drm_i915_file_private *file_priv = file->driver_priv; @@ -2467,7 +2467,7 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv, /* Check whether the file_priv has already selected one ring. */ if ((int)file_priv->bsd_engine < 0) file_priv->bsd_engine = - get_random_u32_below(num_vcs_engines(dev_priv)); + get_random_u32_below(num_vcs_engines(gt)); return file_priv->bsd_engine; } @@ -2644,6 +2644,7 @@ static unsigned int eb_select_legacy_ring(struct i915_execbuffer *eb) { struct drm_i915_private *i915 = eb->i915; + struct intel_gt *gt = eb->gt; struct drm_i915_gem_execbuffer2 *args = eb->args; unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK; @@ -2655,11 +2656,11 @@ eb_select_legacy_ring(struct i915_execbuffer *eb) return -1; } - if (user_ring_id == I915_EXEC_BSD && num_vcs_engines(i915) > 1) { + if (user_ring_id == I915_EXEC_BSD && num_vcs_engines(gt) > 1) { unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK; if (bsd_idx == I915_EXEC_BSD_DEFAULT) { - bsd_idx = gen8_dispatch_bsd_engine(i915, eb->file); + bsd_idx = gen8_dispatch_bsd_engine(gt, eb->file); } else if (bsd_idx >= I915_EXEC_BSD_RING1 && bsd_idx <= I915_EXEC_BSD_RING2) { bsd_idx >>= I915_EXEC_BSD_SHIFT; diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c index a81fa6a20f5a..b2695aab54e5 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c @@ -93,7 +93,7 @@ static int live_nop_switch(void *arg) } if (i915_request_wait(rq, 0, 10 * HZ) < 0) { pr_err("Failed to populated %d contexts\n", nctx); - intel_gt_set_wedged(to_gt(i915)); + intel_gt_set_wedged(engine->gt); i915_request_put(rq); err = -EIO; goto out_file; @@ -105,7 +105,7 @@ static int live_nop_switch(void *arg) pr_info("Populated %d contexts on %s in %lluns\n", nctx, engine->name, ktime_to_ns(times[1] - times[0])); - err = igt_live_test_begin(&t, i915, __func__, engine->name); + err = igt_live_test_begin(&t, engine->gt, __func__, engine->name); if (err) goto out_file; @@ -149,7 +149,7 @@ static int live_nop_switch(void *arg) if (i915_request_wait(rq, 0, HZ / 5) < 0) { pr_err("Switching between %ld contexts timed out\n", prime); - intel_gt_set_wedged(to_gt(i915)); + intel_gt_set_wedged(engine->gt); i915_request_put(rq); break; } @@ -163,7 +163,7 @@ static int live_nop_switch(void *arg) break; } - err = igt_live_test_end(&t); + err = igt_live_test_end(&t, engine->gt); if (err) goto out_file; @@ -376,7 +376,7 @@ static int live_parallel_switch(void *arg) for (fn = func; !err && *fn; fn++) { struct igt_live_test t; - err = igt_live_test_begin(&t, i915, __func__, ""); + err = igt_live_test_begin(&t, to_gt(i915), __func__, ""); if (err) break; @@ -397,7 +397,7 @@ static int live_parallel_switch(void *arg) } } - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, to_gt(i915))) err = -EIO; } @@ -682,7 +682,7 @@ static int igt_ctx_exec(void *arg) if (IS_ERR(file)) return PTR_ERR(file); - err = igt_live_test_begin(&t, i915, __func__, engine->name); + err = igt_live_test_begin(&t, engine->gt, __func__, engine->name); if (err) goto out_file; @@ -760,7 +760,7 @@ static int igt_ctx_exec(void *arg) out_file: throttle_release(tq, ARRAY_SIZE(tq)); - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, engine->gt)) err = -EIO; fput(file); @@ -806,7 +806,7 @@ static int igt_shared_ctx_exec(void *arg) goto out_file; } - err = igt_live_test_begin(&t, i915, __func__, ""); + err = igt_live_test_begin(&t, to_gt(i915), __func__, ""); if (err) goto out_file; @@ -895,7 +895,7 @@ static int igt_shared_ctx_exec(void *arg) } out_test: throttle_release(tq, ARRAY_SIZE(tq)); - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, to_gt(i915))) err = -EIO; out_file: fput(file); @@ -1382,7 +1382,7 @@ static int igt_ctx_readonly(void *arg) if (IS_ERR(file)) return PTR_ERR(file); - err = igt_live_test_begin(&t, i915, __func__, ""); + err = igt_live_test_begin(&t, to_gt(i915), __func__, ""); if (err) goto out_file; @@ -1472,7 +1472,7 @@ static int igt_ctx_readonly(void *arg) out_file: throttle_release(tq, ARRAY_SIZE(tq)); - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, to_gt(i915))) err = -EIO; fput(file); @@ -1785,7 +1785,7 @@ static int igt_vm_isolation(void *arg) if (IS_ERR(file)) return PTR_ERR(file); - err = igt_live_test_begin(&t, i915, __func__, ""); + err = igt_live_test_begin(&t, to_gt(i915), __func__, ""); if (err) goto out_file; @@ -1882,7 +1882,7 @@ static int igt_vm_isolation(void *arg) put_a: i915_gem_object_put(obj_a); out_file: - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, to_gt(i915))) err = -EIO; fput(file); return err; diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c b/drivers/gpu/drm/i915/gt/intel_engine_user.c index cd4f1b126f75..dcedff41a825 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c @@ -117,7 +117,7 @@ static void set_scheduler_caps(struct drm_i915_private *i915) disabled |= (I915_SCHEDULER_CAP_ENABLED | I915_SCHEDULER_CAP_PRIORITY); - if (intel_uc_uses_guc_submission(&to_gt(i915)->uc)) + if (intel_uc_uses_guc_submission(&engine->gt->uc)) enabled |= I915_SCHEDULER_CAP_STATIC_PRIORITY_MAP; for (i = 0; i < ARRAY_SIZE(map); i++) { diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c index 736b89a8ecf5..4384d466a632 100644 --- a/drivers/gpu/drm/i915/gt/selftest_execlists.c +++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c @@ -189,7 +189,7 @@ static int live_unlite_restore(struct intel_gt *gt, int prio) if (!intel_engine_can_store_dword(engine)) continue; - if (igt_live_test_begin(&t, gt->i915, __func__, engine->name)) { + if (igt_live_test_begin(&t, gt, __func__, engine->name)) { err = -EIO; break; } @@ -302,7 +302,7 @@ static int live_unlite_restore(struct intel_gt *gt, int prio) } st_engine_heartbeat_enable(engine); - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, gt)) err = -EIO; if (err) break; @@ -351,7 +351,7 @@ static int live_unlite_ring(void *arg) if (!intel_engine_can_store_dword(engine)) continue; - if (igt_live_test_begin(&t, gt->i915, __func__, engine->name)) { + if (igt_live_test_begin(&t, gt, __func__, engine->name)) { err = -EIO; break; } @@ -462,7 +462,7 @@ static int live_unlite_ring(void *arg) intel_context_put(ce[n]); } st_engine_heartbeat_enable(engine); - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, gt)) err = -EIO; if (err) break; @@ -494,7 +494,7 @@ static int live_pin_rewind(void *arg) struct intel_ring *ring; struct igt_live_test t; - if (igt_live_test_begin(&t, gt->i915, __func__, engine->name)) { + if (igt_live_test_begin(&t, gt, __func__, engine->name)) { err = -EIO; break; } @@ -541,7 +541,7 @@ static int live_pin_rewind(void *arg) i915_request_add(rq); /* Expect not to hang! */ - if (igt_live_test_end(&t)) { + if (igt_live_test_end(&t, gt)) { err = -EIO; break; } @@ -1585,7 +1585,7 @@ static int live_busywait_preempt(void *arg) if (!intel_engine_can_store_dword(engine)) continue; - if (igt_live_test_begin(&t, gt->i915, __func__, engine->name)) { + if (igt_live_test_begin(&t, gt, __func__, engine->name)) { err = -EIO; goto err_vma; } @@ -1687,7 +1687,7 @@ static int live_busywait_preempt(void *arg) GEM_BUG_ON(READ_ONCE(*map)); i915_request_put(lo); - if (igt_live_test_end(&t)) { + if (igt_live_test_end(&t, gt)) { err = -EIO; goto err_vma; } @@ -1757,7 +1757,7 @@ static int live_preempt(void *arg) if (!intel_engine_has_preemption(engine)) continue; - if (igt_live_test_begin(&t, gt->i915, __func__, engine->name)) { + if (igt_live_test_begin(&t, gt, __func__, engine->name)) { err = -EIO; goto err_spin_lo; } @@ -1798,7 +1798,7 @@ static int live_preempt(void *arg) igt_spinner_end(&spin_hi); igt_spinner_end(&spin_lo); - if (igt_live_test_end(&t)) { + if (igt_live_test_end(&t, gt)) { err = -EIO; goto err_spin_lo; } @@ -1850,7 +1850,7 @@ static int live_late_preempt(void *arg) if (!intel_engine_has_preemption(engine)) continue; - if (igt_live_test_begin(&t, gt->i915, __func__, engine->name)) { + if (igt_live_test_begin(&t, gt, __func__, engine->name)) { err = -EIO; goto err_spin_lo; } @@ -1894,7 +1894,7 @@ static int live_late_preempt(void *arg) igt_spinner_end(&spin_hi); igt_spinner_end(&spin_lo); - if (igt_live_test_end(&t)) { + if (igt_live_test_end(&t, gt)) { err = -EIO; goto err_spin_lo; } @@ -2057,7 +2057,7 @@ static int __cancel_active0(struct live_preempt_cancel *arg) /* Preempt cancel of ELSP0 */ GEM_TRACE("%s(%s)\n", __func__, arg->engine->name); - if (igt_live_test_begin(&t, arg->engine->i915, + if (igt_live_test_begin(&t, arg->engine->gt, __func__, arg->engine->name)) return -EIO; @@ -2088,7 +2088,7 @@ static int __cancel_active0(struct live_preempt_cancel *arg) out: i915_request_put(rq); - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, arg->engine->gt)) err = -EIO; return err; } @@ -2101,7 +2101,7 @@ static int __cancel_active1(struct live_preempt_cancel *arg) /* Preempt cancel of ELSP1 */ GEM_TRACE("%s(%s)\n", __func__, arg->engine->name); - if (igt_live_test_begin(&t, arg->engine->i915, + if (igt_live_test_begin(&t, arg->engine->gt, __func__, arg->engine->name)) return -EIO; @@ -2159,7 +2159,7 @@ static int __cancel_active1(struct live_preempt_cancel *arg) out: i915_request_put(rq[1]); i915_request_put(rq[0]); - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, arg->engine->gt)) err = -EIO; return err; } @@ -2172,7 +2172,7 @@ static int __cancel_queued(struct live_preempt_cancel *arg) /* Full ELSP and one in the wings */ GEM_TRACE("%s(%s)\n", __func__, arg->engine->name); - if (igt_live_test_begin(&t, arg->engine->i915, + if (igt_live_test_begin(&t, arg->engine->gt, __func__, arg->engine->name)) return -EIO; @@ -2254,7 +2254,7 @@ static int __cancel_queued(struct live_preempt_cancel *arg) i915_request_put(rq[2]); i915_request_put(rq[1]); i915_request_put(rq[0]); - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, arg->engine->gt)) err = -EIO; return err; } @@ -2599,7 +2599,7 @@ static int live_chain_preempt(void *arg) } i915_request_put(rq); - if (igt_live_test_begin(&t, gt->i915, __func__, engine->name)) { + if (igt_live_test_begin(&t, gt, __func__, engine->name)) { err = -EIO; goto err_wedged; } @@ -2673,7 +2673,7 @@ static int live_chain_preempt(void *arg) i915_request_put(rq); } - if (igt_live_test_end(&t)) { + if (igt_live_test_end(&t, gt)) { err = -EIO; goto err_wedged; } @@ -2799,7 +2799,7 @@ static int __live_preempt_ring(struct intel_engine_cs *engine, int err = 0; int n; - if (igt_live_test_begin(&t, engine->i915, __func__, engine->name)) + if (igt_live_test_begin(&t, engine->gt, __func__, engine->name)) return -EIO; for (n = 0; n < ARRAY_SIZE(ce); n++) { @@ -2902,7 +2902,7 @@ static int __live_preempt_ring(struct intel_engine_cs *engine, intel_context_unpin(ce[n]); intel_context_put(ce[n]); } - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, engine->gt)) err = -EIO; return err; } @@ -2978,7 +2978,7 @@ static int live_preempt_gang(void *arg) if (!intel_engine_has_preemption(engine)) continue; - if (igt_live_test_begin(&t, gt->i915, __func__, engine->name)) + if (igt_live_test_begin(&t, gt, __func__, engine->name)) return -EIO; do { @@ -3030,7 +3030,7 @@ static int live_preempt_gang(void *arg) rq = n; } - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, gt)) err = -EIO; if (err) return err; @@ -3284,7 +3284,7 @@ static int live_preempt_user(void *arg) if (GRAPHICS_VER(gt->i915) == 8 && engine->class != RENDER_CLASS) continue; /* we need per-context GPR */ - if (igt_live_test_begin(&t, gt->i915, __func__, engine->name)) { + if (igt_live_test_begin(&t, gt, __func__, engine->name)) { err = -EIO; break; } @@ -3347,7 +3347,7 @@ static int live_preempt_user(void *arg) /* Flush the semaphores on error */ smp_store_mb(result[0], -1); - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, gt)) err = -EIO; if (err) break; @@ -3665,7 +3665,7 @@ static int live_preempt_smoke(void *arg) i915_gem_object_flush_map(smoke.batch); i915_gem_object_unpin_map(smoke.batch); - if (igt_live_test_begin(&t, smoke.gt->i915, __func__, "all")) { + if (igt_live_test_begin(&t, smoke.gt, __func__, "all")) { err = -EIO; goto err_batch; } @@ -3687,7 +3687,7 @@ static int live_preempt_smoke(void *arg) } err_ctx: - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, smoke.gt)) err = -EIO; for (n = 0; n < smoke.ncontext; n++) { @@ -3737,7 +3737,7 @@ static int nop_virtual_engine(struct intel_gt *gt, } } - err = igt_live_test_begin(&t, gt->i915, __func__, ve[0]->engine->name); + err = igt_live_test_begin(&t, gt, __func__, ve[0]->engine->name); if (err) goto out; @@ -3810,7 +3810,7 @@ static int nop_virtual_engine(struct intel_gt *gt, break; } - err = igt_live_test_end(&t); + err = igt_live_test_end(&t, gt); if (err) goto out; @@ -3928,7 +3928,7 @@ static int mask_virtual_engine(struct intel_gt *gt, if (err) goto out_put; - err = igt_live_test_begin(&t, gt->i915, __func__, ve->engine->name); + err = igt_live_test_begin(&t, gt, __func__, ve->engine->name); if (err) goto out_unpin; @@ -3973,7 +3973,7 @@ static int mask_virtual_engine(struct intel_gt *gt, } } - err = igt_live_test_end(&t); + err = igt_live_test_end(&t, gt); out: if (igt_flush_test(gt->i915)) err = -EIO; @@ -4213,7 +4213,7 @@ static int preserved_virtual_engine(struct intel_gt *gt, if (err) goto out_put; - err = igt_live_test_begin(&t, gt->i915, __func__, ve->engine->name); + err = igt_live_test_begin(&t, gt, __func__, ve->engine->name); if (err) goto out_unpin; @@ -4277,7 +4277,7 @@ static int preserved_virtual_engine(struct intel_gt *gt, i915_gem_object_unpin_map(scratch->obj); out_end: - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, gt)) err = -EIO; i915_request_put(last); out_unpin: diff --git a/drivers/gpu/drm/i915/selftests/i915_request.c b/drivers/gpu/drm/i915/selftests/i915_request.c index a9b79888c193..8f71c4bd66d9 100644 --- a/drivers/gpu/drm/i915/selftests/i915_request.c +++ b/drivers/gpu/drm/i915/selftests/i915_request.c @@ -582,7 +582,7 @@ static int live_nop_request(void *arg) IGT_TIMEOUT(end_time); ktime_t times[2] = {}; - err = igt_live_test_begin(&t, i915, __func__, engine->name); + err = igt_live_test_begin(&t, engine->gt, __func__, engine->name); if (err) return err; @@ -627,7 +627,7 @@ static int live_nop_request(void *arg) } intel_engine_pm_put(engine); - err = igt_live_test_end(&t); + err = igt_live_test_end(&t, engine->gt); if (err) return err; @@ -929,7 +929,7 @@ static int live_cancel_request(void *arg) if (!intel_engine_has_preemption(engine)) continue; - err = igt_live_test_begin(&t, i915, __func__, engine->name); + err = igt_live_test_begin(&t, engine->gt, __func__, engine->name); if (err) return err; @@ -939,7 +939,7 @@ static int live_cancel_request(void *arg) if (err == 0) err = __cancel_completed(engine); - err2 = igt_live_test_end(&t); + err2 = igt_live_test_end(&t, engine->gt); if (err) return err; if (err2) @@ -1058,7 +1058,7 @@ static int live_empty_request(void *arg) if (IS_ERR(batch)) return PTR_ERR(batch); - err = igt_live_test_begin(&t, i915, __func__, engine->name); + err = igt_live_test_begin(&t, engine->gt, __func__, engine->name); if (err) goto out_batch; @@ -1097,7 +1097,7 @@ static int live_empty_request(void *arg) i915_request_put(request); intel_engine_pm_put(engine); - err = igt_live_test_end(&t); + err = igt_live_test_end(&t, engine->gt); if (err) goto out_batch; @@ -1206,7 +1206,7 @@ static int live_all_engines(void *arg) if (!request) return -ENOMEM; - err = igt_live_test_begin(&t, i915, __func__, ""); + err = igt_live_test_begin(&t, to_gt(i915), __func__, ""); if (err) goto out_free; @@ -1292,7 +1292,7 @@ static int live_all_engines(void *arg) idx++; } - err = igt_live_test_end(&t); + err = igt_live_test_end(&t, to_gt(i915)); out_request: idx = 0; @@ -1336,7 +1336,7 @@ static int live_sequential_engines(void *arg) if (!request) return -ENOMEM; - err = igt_live_test_begin(&t, i915, __func__, ""); + err = igt_live_test_begin(&t, to_gt(i915), __func__, ""); if (err) goto out_free; @@ -1423,7 +1423,7 @@ static int live_sequential_engines(void *arg) idx++; } - err = igt_live_test_end(&t); + err = igt_live_test_end(&t, to_gt(i915)); out_request: idx = 0; @@ -1635,7 +1635,7 @@ static int live_parallel_engines(void *arg) unsigned int idx; snprintf(name, sizeof(name), "%ps", *fn); - err = igt_live_test_begin(&t, i915, __func__, name); + err = igt_live_test_begin(&t, to_gt(i915), __func__, name); if (err) break; @@ -1676,7 +1676,7 @@ static int live_parallel_engines(void *arg) kthread_destroy_worker(threads[idx++].worker); } - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, to_gt(i915))) err = -EIO; } @@ -1783,7 +1783,7 @@ static int live_breadcrumbs_smoketest(void *arg) } } - ret = igt_live_test_begin(&live, i915, __func__, ""); + ret = igt_live_test_begin(&live, to_gt(i915), __func__, ""); if (ret) goto out_contexts; @@ -1853,7 +1853,7 @@ static int live_breadcrumbs_smoketest(void *arg) pr_info("Completed %lu waits for %lu fences across %d engines and %d cpus\n", num_waits, num_fences, idx, ncpus); - ret = igt_live_test_end(&live) ?: ret; + ret = igt_live_test_end(&live, to_gt(i915)) ?: ret; out_contexts: kfree(smoke[0].contexts); out_threads: @@ -2877,7 +2877,7 @@ static int perf_series_engines(void *arg) struct igt_live_test t; snprintf(name, sizeof(name), "%ps", *fn); - err = igt_live_test_begin(&t, i915, __func__, name); + err = igt_live_test_begin(&t, to_gt(i915), __func__, name); if (err) break; @@ -2898,7 +2898,7 @@ static int perf_series_engines(void *arg) } err = (*fn)(ps); - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, to_gt(i915))) err = -EIO; for (idx = 0; idx < nengines; idx++) { @@ -3205,7 +3205,7 @@ static int perf_parallel_engines(void *arg) unsigned int idx; snprintf(name, sizeof(name), "%ps", *fn); - err = igt_live_test_begin(&t, i915, __func__, name); + err = igt_live_test_begin(&t, to_gt(i915), __func__, name); if (err) break; @@ -3254,7 +3254,7 @@ static int perf_parallel_engines(void *arg) idx++; } - if (igt_live_test_end(&t)) + if (igt_live_test_end(&t, to_gt(i915))) err = -EIO; if (err) break; diff --git a/drivers/gpu/drm/i915/selftests/igt_live_test.c b/drivers/gpu/drm/i915/selftests/igt_live_test.c index 72b58b66692a..0e7a16962549 100644 --- a/drivers/gpu/drm/i915/selftests/igt_live_test.c +++ b/drivers/gpu/drm/i915/selftests/igt_live_test.c @@ -12,11 +12,11 @@ #include "igt_live_test.h" int igt_live_test_begin(struct igt_live_test *t, - struct drm_i915_private *i915, + struct intel_gt *gt, const char *func, const char *name) { - struct intel_gt *gt = to_gt(i915); + struct drm_i915_private *i915 = gt->i915; struct intel_engine_cs *engine; enum intel_engine_id id; int err; @@ -41,9 +41,9 @@ int igt_live_test_begin(struct igt_live_test *t, return 0; } -int igt_live_test_end(struct igt_live_test *t) +int igt_live_test_end(struct igt_live_test *t, struct intel_gt *gt) { - struct drm_i915_private *i915 = t->i915; + struct drm_i915_private *i915 = gt->i915; struct intel_engine_cs *engine; enum intel_engine_id id; @@ -57,7 +57,7 @@ int igt_live_test_end(struct igt_live_test *t) return -EIO; } - for_each_engine(engine, to_gt(i915), id) { + for_each_engine(engine, gt, id) { if (t->reset_engine[id] == i915_reset_engine_count(&i915->gpu_error, engine)) continue; diff --git a/drivers/gpu/drm/i915/selftests/igt_live_test.h b/drivers/gpu/drm/i915/selftests/igt_live_test.h index 36ed42736c52..209b0548c603 100644 --- a/drivers/gpu/drm/i915/selftests/igt_live_test.h +++ b/drivers/gpu/drm/i915/selftests/igt_live_test.h @@ -27,9 +27,9 @@ struct igt_live_test { * e.g. if the GPU was reset. */ int igt_live_test_begin(struct igt_live_test *t, - struct drm_i915_private *i915, + struct intel_gt *gt, const char *func, const char *name); -int igt_live_test_end(struct igt_live_test *t); +int igt_live_test_end(struct igt_live_test *t, struct intel_gt *gt); #endif /* IGT_LIVE_TEST_H */
In order to make igt_live_test work in proper way, we need to consider multi-gt in all tests where igt_live_test is used as well as at other random places where multi-gt should be considered. Cc: Andi Shyti <andi.shyti@linux.intel.com> Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com> --- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ++-- .../drm/i915/gem/selftests/i915_gem_context.c | 28 ++++---- drivers/gpu/drm/i915/gt/intel_engine_user.c | 2 +- drivers/gpu/drm/i915/gt/selftest_execlists.c | 68 +++++++++---------- drivers/gpu/drm/i915/selftests/i915_request.c | 36 +++++----- .../gpu/drm/i915/selftests/igt_live_test.c | 10 +-- .../gpu/drm/i915/selftests/igt_live_test.h | 4 +- 7 files changed, 81 insertions(+), 80 deletions(-)