diff mbox series

[11/11] drm/i915/pvc: read fuses for link copy engines

Message ID 20220502163417.2635462-12-matthew.d.roper@intel.com (mailing list archive)
State New, archived
Headers show
Series i915: Introduce Ponte Vecchio | expand

Commit Message

Matt Roper May 2, 2022, 4:34 p.m. UTC 
From: Lucas De Marchi <lucas.demarchi@intel.com>

The new Link Copy engines in PVC may be fused off according to the
mslice_mask. Each bit of the MEML3_EN_MASK we read from the
GEN10_MIRROR_FUSE3 register disables a pair of link copy engines.

Bspec: 44483
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c | 28 +++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Souza, Jose May 2, 2022, 6:48 p.m. UTC | #1 
On Mon, 2022-05-02 at 09:34 -0700, Matt Roper wrote:
> From: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> The new Link Copy engines in PVC may be fused off according to the
> mslice_mask. Each bit of the MEML3_EN_MASK we read from the
> GEN10_MIRROR_FUSE3 register disables a pair of link copy engines.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> Bspec: 44483
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c | 28 +++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index c6e93db134b1..d10cdeff5072 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -686,6 +686,33 @@ static void engine_mask_apply_compute_fuses(struct intel_gt *gt)
>  	}
>  }
>  
> +static void engine_mask_apply_copy_fuses(struct intel_gt *gt)
> +{
> +	struct drm_i915_private *i915 = gt->i915;
> +	struct intel_gt_info *info = &gt->info;
> +	unsigned long meml3_mask;
> +	u8 quad;
> +
> +	meml3_mask = intel_uncore_read(gt->uncore, GEN10_MIRROR_FUSE3);
> +	meml3_mask = REG_FIELD_GET(GEN12_MEML3_EN_MASK, meml3_mask);
> +
> +	/*
> +	 * Link Copy engines may be fused off according to meml3_mask. Each
> +	 * bit is a quad that houses 2 Link Copy and two Sub Copy engines.
> +	 */
> +	for_each_clear_bit(quad, &meml3_mask, GEN12_MAX_MSLICES) {
> +		intel_engine_mask_t mask = GENMASK(BCS1 + quad * 2 + 1,
> +						   BCS1 + quad * 2);
> +
> +		if (mask & info->engine_mask) {
> +			drm_dbg(&i915->drm, "bcs%u fused off\n", quad * 2 + 1);
> +			drm_dbg(&i915->drm, "bcs%u fused off\n", quad * 2 + 2);
> +
> +			info->engine_mask &= ~mask;
> +		}
> +	}
> +}
> +
>  /*
>   * Determine which engines are fused off in our particular hardware.
>   * Note that we have a catch-22 situation where we need to be able to access
> @@ -768,6 +795,7 @@ static intel_engine_mask_t init_engine_mask(struct intel_gt *gt)
>  	GEM_BUG_ON(vebox_mask != VEBOX_MASK(gt));
>  
>  	engine_mask_apply_compute_fuses(gt);
> +	engine_mask_apply_copy_fuses(gt);
>  
>  	return info->engine_mask;
>  }
Tvrtko Ursulin May 3, 2022, 8:19 a.m. UTC | #2 
On 02/05/2022 17:34, Matt Roper wrote:
> From: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> The new Link Copy engines in PVC may be fused off according to the
> mslice_mask. Each bit of the MEML3_EN_MASK we read from the
> GEN10_MIRROR_FUSE3 register disables a pair of link copy engines.
> 
> Bspec: 44483
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c | 28 +++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index c6e93db134b1..d10cdeff5072 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -686,6 +686,33 @@ static void engine_mask_apply_compute_fuses(struct intel_gt *gt)
>   	}
>   }
>   
> +static void engine_mask_apply_copy_fuses(struct intel_gt *gt)
> +{
> +	struct drm_i915_private *i915 = gt->i915;
> +	struct intel_gt_info *info = &gt->info;
> +	unsigned long meml3_mask;
> +	u8 quad;

Any hidden reason u8 is the right type here and not unsigned long like bitops expect? (Yes I did notice GEN12_MAX_MSLICES only goes to 4 but generally u8 sucks.)

> +
> +	meml3_mask = intel_uncore_read(gt->uncore, GEN10_MIRROR_FUSE3);
> +	meml3_mask = REG_FIELD_GET(GEN12_MEML3_EN_MASK, meml3_mask);
> +
> +	/*
> +	 * Link Copy engines may be fused off according to meml3_mask. Each
> +	 * bit is a quad that houses 2 Link Copy and two Sub Copy engines.
> +	 */
> +	for_each_clear_bit(quad, &meml3_mask, GEN12_MAX_MSLICES) {
> +		intel_engine_mask_t mask = GENMASK(BCS1 + quad * 2 + 1,
> +						   BCS1 + quad * 2);

So internally we will be sure BCS1 to BCS9 are link copy engines? I mean enum names hardcoded/fixed to function. Should we have a comment to that effect somewhere? In intel_engine_types.h maybe?

> +
> +		if (mask & info->engine_mask) {
> +			drm_dbg(&i915->drm, "bcs%u fused off\n", quad * 2 + 1);
> +			drm_dbg(&i915->drm, "bcs%u fused off\n", quad * 2 + 2);

Bikeshed - I'd be tempted to decrease the amount of "quad * 2 + 1" by having a local variable.

   unsigned int instance = quad * 2 + 1;
   intel_engine_mask_t mask = GENMASK(_BCS(instance + 1), _BCS(instance));

Etc.

Regards,

Tvrtko

> +
> +			info->engine_mask &= ~mask;
> +		}
> +	}
> +}
> +
>   /*
>    * Determine which engines are fused off in our particular hardware.
>    * Note that we have a catch-22 situation where we need to be able to access
> @@ -768,6 +795,7 @@ static intel_engine_mask_t init_engine_mask(struct intel_gt *gt)
>   	GEM_BUG_ON(vebox_mask != VEBOX_MASK(gt));
>   
>   	engine_mask_apply_compute_fuses(gt);
> +	engine_mask_apply_copy_fuses(gt);
>   
>   	return info->engine_mask;
>   }
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 c6e93db134b1..d10cdeff5072 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -686,6 +686,33 @@  static void engine_mask_apply_compute_fuses(struct intel_gt *gt)
 	}
 }
 
+static void engine_mask_apply_copy_fuses(struct intel_gt *gt)
+{
+	struct drm_i915_private *i915 = gt->i915;
+	struct intel_gt_info *info = &gt->info;
+	unsigned long meml3_mask;
+	u8 quad;
+
+	meml3_mask = intel_uncore_read(gt->uncore, GEN10_MIRROR_FUSE3);
+	meml3_mask = REG_FIELD_GET(GEN12_MEML3_EN_MASK, meml3_mask);
+
+	/*
+	 * Link Copy engines may be fused off according to meml3_mask. Each
+	 * bit is a quad that houses 2 Link Copy and two Sub Copy engines.
+	 */
+	for_each_clear_bit(quad, &meml3_mask, GEN12_MAX_MSLICES) {
+		intel_engine_mask_t mask = GENMASK(BCS1 + quad * 2 + 1,
+						   BCS1 + quad * 2);
+
+		if (mask & info->engine_mask) {
+			drm_dbg(&i915->drm, "bcs%u fused off\n", quad * 2 + 1);
+			drm_dbg(&i915->drm, "bcs%u fused off\n", quad * 2 + 2);
+
+			info->engine_mask &= ~mask;
+		}
+	}
+}
+
 /*
  * Determine which engines are fused off in our particular hardware.
  * Note that we have a catch-22 situation where we need to be able to access
@@ -768,6 +795,7 @@  static intel_engine_mask_t init_engine_mask(struct intel_gt *gt)
 	GEM_BUG_ON(vebox_mask != VEBOX_MASK(gt));
 
 	engine_mask_apply_compute_fuses(gt);
+	engine_mask_apply_copy_fuses(gt);
 
 	return info->engine_mask;
 }