Re: [Intel-gfx] [PATCH] drm/i915/gt: Consider multi-gt at all places

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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