diff mbox series

[v6,2/3] drm/i915/gt: Do not generate the command streamer for all the CCS

Message ID 20240313201955.95716-3-andi.shyti@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Disable automatic load CCS load balancing | expand

Commit Message

Andi Shyti March 13, 2024, 8:19 p.m. UTC 
We want a fixed load CCS balancing consisting in all slices
sharing one single user engine. For this reason do not create the
intel_engine_cs structure with its dedicated command streamer for
CCS slices beyond the first.

Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")
Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: <stable@vger.kernel.org> # v6.2+
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Comments

Matt Roper March 26, 2024, 4:03 p.m. UTC | #1 
On Wed, Mar 13, 2024 at 09:19:50PM +0100, Andi Shyti wrote:
> We want a fixed load CCS balancing consisting in all slices
> sharing one single user engine. For this reason do not create the
> intel_engine_cs structure with its dedicated command streamer for
> CCS slices beyond the first.
> 
> Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")
> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: <stable@vger.kernel.org> # v6.2+
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index f553cf4e6449..c4fb31bb6e72 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -966,6 +966,7 @@ int intel_engines_init_mmio(struct intel_gt *gt)
>  	const unsigned int engine_mask = init_engine_mask(gt);
>  	unsigned int mask = 0;
>  	unsigned int i, class;
> +	u8 ccs_instance = 0;
>  	u8 logical_ids[MAX_ENGINE_INSTANCE + 1];
>  	int err;
>  
> @@ -986,6 +987,19 @@ int intel_engines_init_mmio(struct intel_gt *gt)
>  			    !HAS_ENGINE(gt, i))
>  				continue;
>  
> +			/*
> +			 * Do not create the command streamer for CCS slices
> +			 * beyond the first. All the workload submitted to the
> +			 * first engine will be shared among all the slices.
> +			 *
> +			 * Once the user will be allowed to customize the CCS
> +			 * mode, then this check needs to be removed.
> +			 */
> +			if (IS_DG2(i915) &&
> +			    class == COMPUTE_CLASS &&
> +			    ccs_instance++)
> +				continue;

Wouldn't it be more intuitive to drop the non-lowest CCS engines in
init_engine_mask() since that's the function that's dedicated to
building the list of engines we'll use?  Then we don't need to kill the
assertion farther down either.


Matt

> +
>  			err = intel_engine_setup(gt, i,
>  						 logical_ids[instance]);
>  			if (err)
> @@ -996,11 +1010,9 @@ int intel_engines_init_mmio(struct intel_gt *gt)
>  	}
>  
>  	/*
> -	 * Catch failures to update intel_engines table when the new engines
> -	 * are added to the driver by a warning and disabling the forgotten
> -	 * engines.
> +	 * Update the intel_engines table.
>  	 */
> -	if (drm_WARN_ON(&i915->drm, mask != engine_mask))
> +	if (mask != engine_mask)
>  		gt->info.engine_mask = mask;
>  
>  	gt->info.num_engines = hweight32(mask);
> -- 
> 2.43.0
>
Andi Shyti March 26, 2024, 6:42 p.m. UTC | #2 
Hi Matt,

On Tue, Mar 26, 2024 at 09:03:10AM -0700, Matt Roper wrote:
> On Wed, Mar 13, 2024 at 09:19:50PM +0100, Andi Shyti wrote:
> > +			/*
> > +			 * Do not create the command streamer for CCS slices
> > +			 * beyond the first. All the workload submitted to the
> > +			 * first engine will be shared among all the slices.
> > +			 *
> > +			 * Once the user will be allowed to customize the CCS
> > +			 * mode, then this check needs to be removed.
> > +			 */
> > +			if (IS_DG2(i915) &&
> > +			    class == COMPUTE_CLASS &&
> > +			    ccs_instance++)
> > +				continue;
> 
> Wouldn't it be more intuitive to drop the non-lowest CCS engines in
> init_engine_mask() since that's the function that's dedicated to
> building the list of engines we'll use?  Then we don't need to kill the
> assertion farther down either.

Because we don't check the result of init_engine_mask() while
creating the engine's structure. We check it only after and
indeed I removed the drm_WARN_ON() check.

I think the whole process of creating the engine's structure in
the intel_engines_init_mmio() can be simplified, but this goes
beyong the scope of the series.

Or am I missing something?

Thanks,
Andi
Matt Roper March 26, 2024, 9:30 p.m. UTC | #3 
On Tue, Mar 26, 2024 at 07:42:34PM +0100, Andi Shyti wrote:
> Hi Matt,
> 
> On Tue, Mar 26, 2024 at 09:03:10AM -0700, Matt Roper wrote:
> > On Wed, Mar 13, 2024 at 09:19:50PM +0100, Andi Shyti wrote:
> > > +			/*
> > > +			 * Do not create the command streamer for CCS slices
> > > +			 * beyond the first. All the workload submitted to the
> > > +			 * first engine will be shared among all the slices.
> > > +			 *
> > > +			 * Once the user will be allowed to customize the CCS
> > > +			 * mode, then this check needs to be removed.
> > > +			 */
> > > +			if (IS_DG2(i915) &&
> > > +			    class == COMPUTE_CLASS &&
> > > +			    ccs_instance++)
> > > +				continue;
> > 
> > Wouldn't it be more intuitive to drop the non-lowest CCS engines in
> > init_engine_mask() since that's the function that's dedicated to
> > building the list of engines we'll use?  Then we don't need to kill the
> > assertion farther down either.
> 
> Because we don't check the result of init_engine_mask() while
> creating the engine's structure. We check it only after and
> indeed I removed the drm_WARN_ON() check.
> 
> I think the whole process of creating the engine's structure in
> the intel_engines_init_mmio() can be simplified, but this goes
> beyong the scope of the series.
> 
> Or am I missing something?

The important part of init_engine_mask isn't the return value, but
rather that it's what sets up gt->info.engine_mask.  The HAS_ENGINE()
check that intel_engines_init_mmio() uses is based on the value stored
there, so updating that function will also ensure that we skip the
engines we don't want in the loop.


Matt

> 
> Thanks,
> Andi
Andi Shyti March 26, 2024, 11:16 p.m. UTC | #4 
Hi Matt,

On Tue, Mar 26, 2024 at 02:30:33PM -0700, Matt Roper wrote:
> On Tue, Mar 26, 2024 at 07:42:34PM +0100, Andi Shyti wrote:
> > On Tue, Mar 26, 2024 at 09:03:10AM -0700, Matt Roper wrote:
> > > On Wed, Mar 13, 2024 at 09:19:50PM +0100, Andi Shyti wrote:
> > > > +			/*
> > > > +			 * Do not create the command streamer for CCS slices
> > > > +			 * beyond the first. All the workload submitted to the
> > > > +			 * first engine will be shared among all the slices.
> > > > +			 *
> > > > +			 * Once the user will be allowed to customize the CCS
> > > > +			 * mode, then this check needs to be removed.
> > > > +			 */
> > > > +			if (IS_DG2(i915) &&
> > > > +			    class == COMPUTE_CLASS &&
> > > > +			    ccs_instance++)
> > > > +				continue;
> > > 
> > > Wouldn't it be more intuitive to drop the non-lowest CCS engines in
> > > init_engine_mask() since that's the function that's dedicated to
> > > building the list of engines we'll use?  Then we don't need to kill the
> > > assertion farther down either.
> > 
> > Because we don't check the result of init_engine_mask() while
> > creating the engine's structure. We check it only after and
> > indeed I removed the drm_WARN_ON() check.
> > 
> > I think the whole process of creating the engine's structure in
> > the intel_engines_init_mmio() can be simplified, but this goes
> > beyong the scope of the series.
> > 
> > Or am I missing something?
> 
> The important part of init_engine_mask isn't the return value, but
> rather that it's what sets up gt->info.engine_mask.  The HAS_ENGINE()
> check that intel_engines_init_mmio() uses is based on the value stored
> there, so updating that function will also ensure that we skip the
> engines we don't want in the loop.

Yes, can do like this, as well. After all this is done I'm going
to do some cleanup here, as well.

Thanks,
Andi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index f553cf4e6449..c4fb31bb6e72 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -966,6 +966,7 @@  int intel_engines_init_mmio(struct intel_gt *gt)
 	const unsigned int engine_mask = init_engine_mask(gt);
 	unsigned int mask = 0;
 	unsigned int i, class;
+	u8 ccs_instance = 0;
 	u8 logical_ids[MAX_ENGINE_INSTANCE + 1];
 	int err;
 
@@ -986,6 +987,19 @@  int intel_engines_init_mmio(struct intel_gt *gt)
 			    !HAS_ENGINE(gt, i))
 				continue;
 
+			/*
+			 * Do not create the command streamer for CCS slices
+			 * beyond the first. All the workload submitted to the
+			 * first engine will be shared among all the slices.
+			 *
+			 * Once the user will be allowed to customize the CCS
+			 * mode, then this check needs to be removed.
+			 */
+			if (IS_DG2(i915) &&
+			    class == COMPUTE_CLASS &&
+			    ccs_instance++)
+				continue;
+
 			err = intel_engine_setup(gt, i,
 						 logical_ids[instance]);
 			if (err)
@@ -996,11 +1010,9 @@  int intel_engines_init_mmio(struct intel_gt *gt)
 	}
 
 	/*
-	 * Catch failures to update intel_engines table when the new engines
-	 * are added to the driver by a warning and disabling the forgotten
-	 * engines.
+	 * Update the intel_engines table.
 	 */
-	if (drm_WARN_ON(&i915->drm, mask != engine_mask))
+	if (mask != engine_mask)
 		gt->info.engine_mask = mask;
 
 	gt->info.num_engines = hweight32(mask);