diff mbox series

[v2,2/2] drm/i915/gt: Extract function to apply media fuses

Message ID 20220909-media-v2-2-6f20f322b4ef@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Media fuses future-proofing | expand

Commit Message

Lucas De Marchi Sept. 9, 2022, 11:18 p.m. UTC 
Just like is done for compute and copy engines, extract a function to
handle media engines. While at it, be consistent on using or not the
uncore/gt/info variable aliases.

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Comments

Andrzej Hajda Sept. 12, 2022, 8:56 a.m. UTC | #1 
On 10.09.2022 01:18, Lucas De Marchi wrote:
> Just like is done for compute and copy engines, extract a function to
> handle media engines. While at it, be consistent on using or not the
> uncore/gt/info variable aliases.
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index b6602439224d..814f83b5fe59 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -663,6 +663,74 @@ bool gen11_vdbox_has_sfc(struct intel_gt *gt,
>   	return false;
>   }
>   
> +static void engine_mask_apply_media_fuses(struct intel_gt *gt)
> +{
> +	struct drm_i915_private *i915 = gt->i915;
> +	unsigned int logical_vdbox = 0;
> +	unsigned int i;
> +	u32 media_fuse, fuse1;
> +	u16 vdbox_mask;
> +	u16 vebox_mask;
> +
> +	if (MEDIA_VER(gt->i915) < 11)
> +		return;
> +
> +	/*
> +	 * On newer platforms the fusing register is called 'enable' and has
> +	 * enable semantics, while on older platforms it is called 'disable'
> +	 * and bits have disable semantices.
> +	 */
> +	media_fuse = intel_uncore_read(gt->uncore, GEN11_GT_VEBOX_VDBOX_DISABLE);
> +	if (MEDIA_VER_FULL(i915) < IP_VER(12, 50))
> +		media_fuse = ~media_fuse;
> +
> +	vdbox_mask = media_fuse & GEN11_GT_VDBOX_DISABLE_MASK;
> +	vebox_mask = (media_fuse & GEN11_GT_VEBOX_DISABLE_MASK) >>
> +		      GEN11_GT_VEBOX_DISABLE_SHIFT;
> +
> +	if (MEDIA_VER_FULL(i915) >= IP_VER(12, 50)) {
> +		fuse1 = intel_uncore_read(gt->uncore, HSW_PAVP_FUSE1);
> +		gt->info.sfc_mask = REG_FIELD_GET(XEHP_SFC_ENABLE_MASK, fuse1);
> +	} else {
> +		gt->info.sfc_mask = ~0;
> +	}
> +
> +	for (i = 0; i < I915_MAX_VCS; i++) {
> +		if (!HAS_ENGINE(gt, _VCS(i))) {
> +			vdbox_mask &= ~BIT(i);
> +			continue;
> +		}
> +
> +		if (!(BIT(i) & vdbox_mask)) {
> +			gt->info.engine_mask &= ~BIT(_VCS(i));
> +			drm_dbg(&i915->drm, "vcs%u fused off\n", i);
> +			continue;
> +		}
> +
> +		if (gen11_vdbox_has_sfc(gt, i, logical_vdbox, vdbox_mask))
> +			gt->info.vdbox_sfc_access |= BIT(i);
> +		logical_vdbox++;
> +	}
> +	drm_dbg(&i915->drm, "vdbox enable: %04x, instances: %04lx\n",
> +		vdbox_mask, VDBOX_MASK(gt));
> +	GEM_BUG_ON(vdbox_mask != VDBOX_MASK(gt));
> +
> +	for (i = 0; i < I915_MAX_VECS; i++) {
> +		if (!HAS_ENGINE(gt, _VECS(i))) {
> +			vebox_mask &= ~BIT(i);
> +			continue;
> +		}
> +
> +		if (!(BIT(i) & vebox_mask)) {
> +			gt->info.engine_mask &= ~BIT(_VECS(i));
> +			drm_dbg(&i915->drm, "vecs%u fused off\n", i);
> +		}
> +	}
> +	drm_dbg(&i915->drm, "vebox enable: %04x, instances: %04lx\n",
> +		vebox_mask, VEBOX_MASK(gt));
> +	GEM_BUG_ON(vebox_mask != VEBOX_MASK(gt));
> +}
> +
>   static void engine_mask_apply_compute_fuses(struct intel_gt *gt)
>   {
>   	struct drm_i915_private *i915 = gt->i915;
> @@ -671,6 +739,9 @@ static void engine_mask_apply_compute_fuses(struct intel_gt *gt)
>   	unsigned long ccs_mask;
>   	unsigned int i;
>   
> +	if (GRAPHICS_VER(i915) < 11)
> +		return;
> +

Why there is no similar sentinel in case of engine_mask_apply_copy_fuses?
Beside this:
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej



>   	if (hweight32(CCS_MASK(gt)) <= 1)
>   		return;
>   
> @@ -726,75 +797,11 @@ static void engine_mask_apply_copy_fuses(struct intel_gt *gt)
>    */
>   static intel_engine_mask_t init_engine_mask(struct intel_gt *gt)
>   {
> -	struct drm_i915_private *i915 = gt->i915;
>   	struct intel_gt_info *info = &gt->info;
> -	struct intel_uncore *uncore = gt->uncore;
> -	unsigned int logical_vdbox = 0;
> -	unsigned int i;
> -	u32 media_fuse, fuse1;
> -	u16 vdbox_mask;
> -	u16 vebox_mask;
>   
>   	GEM_BUG_ON(!info->engine_mask);
>   
> -	if (GRAPHICS_VER(i915) < 11)
> -		return info->engine_mask;
> -
> -	/*
> -	 * On newer platforms the fusing register is called 'enable' and has
> -	 * enable semantics, while on older platforms it is called 'disable'
> -	 * and bits have disable semantices.
> -	 */
> -	media_fuse = intel_uncore_read(uncore, GEN11_GT_VEBOX_VDBOX_DISABLE);
> -	if (MEDIA_VER_FULL(i915) < IP_VER(12, 50))
> -		media_fuse = ~media_fuse;
> -
> -	vdbox_mask = media_fuse & GEN11_GT_VDBOX_DISABLE_MASK;
> -	vebox_mask = (media_fuse & GEN11_GT_VEBOX_DISABLE_MASK) >>
> -		      GEN11_GT_VEBOX_DISABLE_SHIFT;
> -
> -	if (MEDIA_VER_FULL(i915) >= IP_VER(12, 50)) {
> -		fuse1 = intel_uncore_read(uncore, HSW_PAVP_FUSE1);
> -		gt->info.sfc_mask = REG_FIELD_GET(XEHP_SFC_ENABLE_MASK, fuse1);
> -	} else {
> -		gt->info.sfc_mask = ~0;
> -	}
> -
> -	for (i = 0; i < I915_MAX_VCS; i++) {
> -		if (!HAS_ENGINE(gt, _VCS(i))) {
> -			vdbox_mask &= ~BIT(i);
> -			continue;
> -		}
> -
> -		if (!(BIT(i) & vdbox_mask)) {
> -			info->engine_mask &= ~BIT(_VCS(i));
> -			drm_dbg(&i915->drm, "vcs%u fused off\n", i);
> -			continue;
> -		}
> -
> -		if (gen11_vdbox_has_sfc(gt, i, logical_vdbox, vdbox_mask))
> -			gt->info.vdbox_sfc_access |= BIT(i);
> -		logical_vdbox++;
> -	}
> -	drm_dbg(&i915->drm, "vdbox enable: %04x, instances: %04lx\n",
> -		vdbox_mask, VDBOX_MASK(gt));
> -	GEM_BUG_ON(vdbox_mask != VDBOX_MASK(gt));
> -
> -	for (i = 0; i < I915_MAX_VECS; i++) {
> -		if (!HAS_ENGINE(gt, _VECS(i))) {
> -			vebox_mask &= ~BIT(i);
> -			continue;
> -		}
> -
> -		if (!(BIT(i) & vebox_mask)) {
> -			info->engine_mask &= ~BIT(_VECS(i));
> -			drm_dbg(&i915->drm, "vecs%u fused off\n", i);
> -		}
> -	}
> -	drm_dbg(&i915->drm, "vebox enable: %04x, instances: %04lx\n",
> -		vebox_mask, VEBOX_MASK(gt));
> -	GEM_BUG_ON(vebox_mask != VEBOX_MASK(gt));
> -
> +	engine_mask_apply_media_fuses(gt);
>   	engine_mask_apply_compute_fuses(gt);
>   	engine_mask_apply_copy_fuses(gt);
>   
>
Lucas De Marchi Sept. 12, 2022, 3:32 p.m. UTC | #2 
On Mon, Sep 12, 2022 at 10:56:16AM +0200, Andrzej Hajda wrote:
>On 10.09.2022 01:18, Lucas De Marchi wrote:
>>Just like is done for compute and copy engines, extract a function to
>>handle media engines. While at it, be consistent on using or not the
>>uncore/gt/info variable aliases.
>>
>>Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>
>>diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>index b6602439224d..814f83b5fe59 100644
>>--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>@@ -663,6 +663,74 @@ bool gen11_vdbox_has_sfc(struct intel_gt *gt,
>>  	return false;
>>  }
>>+static void engine_mask_apply_media_fuses(struct intel_gt *gt)
>>+{
>>+	struct drm_i915_private *i915 = gt->i915;
>>+	unsigned int logical_vdbox = 0;
>>+	unsigned int i;
>>+	u32 media_fuse, fuse1;
>>+	u16 vdbox_mask;
>>+	u16 vebox_mask;
>>+
>>+	if (MEDIA_VER(gt->i915) < 11)
>>+		return;
>>+
>>+	/*
>>+	 * On newer platforms the fusing register is called 'enable' and has
>>+	 * enable semantics, while on older platforms it is called 'disable'
>>+	 * and bits have disable semantices.
>>+	 */
>>+	media_fuse = intel_uncore_read(gt->uncore, GEN11_GT_VEBOX_VDBOX_DISABLE);
>>+	if (MEDIA_VER_FULL(i915) < IP_VER(12, 50))
>>+		media_fuse = ~media_fuse;
>>+
>>+	vdbox_mask = media_fuse & GEN11_GT_VDBOX_DISABLE_MASK;
>>+	vebox_mask = (media_fuse & GEN11_GT_VEBOX_DISABLE_MASK) >>
>>+		      GEN11_GT_VEBOX_DISABLE_SHIFT;
>>+
>>+	if (MEDIA_VER_FULL(i915) >= IP_VER(12, 50)) {
>>+		fuse1 = intel_uncore_read(gt->uncore, HSW_PAVP_FUSE1);
>>+		gt->info.sfc_mask = REG_FIELD_GET(XEHP_SFC_ENABLE_MASK, fuse1);
>>+	} else {
>>+		gt->info.sfc_mask = ~0;
>>+	}
>>+
>>+	for (i = 0; i < I915_MAX_VCS; i++) {
>>+		if (!HAS_ENGINE(gt, _VCS(i))) {
>>+			vdbox_mask &= ~BIT(i);
>>+			continue;
>>+		}
>>+
>>+		if (!(BIT(i) & vdbox_mask)) {
>>+			gt->info.engine_mask &= ~BIT(_VCS(i));
>>+			drm_dbg(&i915->drm, "vcs%u fused off\n", i);
>>+			continue;
>>+		}
>>+
>>+		if (gen11_vdbox_has_sfc(gt, i, logical_vdbox, vdbox_mask))
>>+			gt->info.vdbox_sfc_access |= BIT(i);
>>+		logical_vdbox++;
>>+	}
>>+	drm_dbg(&i915->drm, "vdbox enable: %04x, instances: %04lx\n",
>>+		vdbox_mask, VDBOX_MASK(gt));
>>+	GEM_BUG_ON(vdbox_mask != VDBOX_MASK(gt));
>>+
>>+	for (i = 0; i < I915_MAX_VECS; i++) {
>>+		if (!HAS_ENGINE(gt, _VECS(i))) {
>>+			vebox_mask &= ~BIT(i);
>>+			continue;
>>+		}
>>+
>>+		if (!(BIT(i) & vebox_mask)) {
>>+			gt->info.engine_mask &= ~BIT(_VECS(i));
>>+			drm_dbg(&i915->drm, "vecs%u fused off\n", i);
>>+		}
>>+	}
>>+	drm_dbg(&i915->drm, "vebox enable: %04x, instances: %04lx\n",
>>+		vebox_mask, VEBOX_MASK(gt));
>>+	GEM_BUG_ON(vebox_mask != VEBOX_MASK(gt));
>>+}
>>+
>>  static void engine_mask_apply_compute_fuses(struct intel_gt *gt)
>>  {
>>  	struct drm_i915_private *i915 = gt->i915;
>>@@ -671,6 +739,9 @@ static void engine_mask_apply_compute_fuses(struct intel_gt *gt)
>>  	unsigned long ccs_mask;
>>  	unsigned int i;
>>+	if (GRAPHICS_VER(i915) < 11)
>>+		return;
>>+
>
>Why there is no similar sentinel in case of engine_mask_apply_copy_fuses?
>Beside this:
>Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

I noticed it too while doing these patches. I have a pending one for
that, but it seems I failed to send it on Friday.

drm/i915: Skip applying copy engine fuses

Support for reading the fuses to check what are the Link Copy engines
was added in commit ad5f74f34201 ("drm/i915/pvc: read fuses for link
copy engines"). However they were added unconditionally because the
FUSE3 register is present since graphics version 10.

However the bitfield with meml3 fuses only exists since graphics version
12. Moreover, Link Copy engines are currently only available in PVC.
Tying additional copy engines to the meml3 fuses is not correct for
other platforms.

Make sure there is a check for  `12.60 <= ver < 12.70`. Later platforms
may extend this function later if it's needed to fuse off copy engines.

Currently it's harmless as the Link Copy engines are still not exported:
info->engine_mask only has BCS0 set and the register is only read for
platforms that do have it.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
  drivers/gpu/drm/i915/gt/intel_engine_cs.c | 4 ++++
  1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 6e0122b3dca2..ac5ff17888cd 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -694,6 +694,10 @@ static void engine_mask_apply_copy_fuses(struct intel_gt *gt)
  	unsigned long meml3_mask;
  	unsigned long quad;
  
+	if (!(GRAPHICS_VER_FULL(i915) >= IP_VER(12, 60) &&
+	      GRAPHICS_VER_FULL(i915) < IP_VER(12, 70)))
+		return;
+
  	meml3_mask = intel_uncore_read(gt->uncore, GEN10_MIRROR_FUSE3);
  	meml3_mask = REG_FIELD_GET(GEN12_MEML3_EN_MASK, meml3_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 b6602439224d..814f83b5fe59 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -663,6 +663,74 @@  bool gen11_vdbox_has_sfc(struct intel_gt *gt,
 	return false;
 }
 
+static void engine_mask_apply_media_fuses(struct intel_gt *gt)
+{
+	struct drm_i915_private *i915 = gt->i915;
+	unsigned int logical_vdbox = 0;
+	unsigned int i;
+	u32 media_fuse, fuse1;
+	u16 vdbox_mask;
+	u16 vebox_mask;
+
+	if (MEDIA_VER(gt->i915) < 11)
+		return;
+
+	/*
+	 * On newer platforms the fusing register is called 'enable' and has
+	 * enable semantics, while on older platforms it is called 'disable'
+	 * and bits have disable semantices.
+	 */
+	media_fuse = intel_uncore_read(gt->uncore, GEN11_GT_VEBOX_VDBOX_DISABLE);
+	if (MEDIA_VER_FULL(i915) < IP_VER(12, 50))
+		media_fuse = ~media_fuse;
+
+	vdbox_mask = media_fuse & GEN11_GT_VDBOX_DISABLE_MASK;
+	vebox_mask = (media_fuse & GEN11_GT_VEBOX_DISABLE_MASK) >>
+		      GEN11_GT_VEBOX_DISABLE_SHIFT;
+
+	if (MEDIA_VER_FULL(i915) >= IP_VER(12, 50)) {
+		fuse1 = intel_uncore_read(gt->uncore, HSW_PAVP_FUSE1);
+		gt->info.sfc_mask = REG_FIELD_GET(XEHP_SFC_ENABLE_MASK, fuse1);
+	} else {
+		gt->info.sfc_mask = ~0;
+	}
+
+	for (i = 0; i < I915_MAX_VCS; i++) {
+		if (!HAS_ENGINE(gt, _VCS(i))) {
+			vdbox_mask &= ~BIT(i);
+			continue;
+		}
+
+		if (!(BIT(i) & vdbox_mask)) {
+			gt->info.engine_mask &= ~BIT(_VCS(i));
+			drm_dbg(&i915->drm, "vcs%u fused off\n", i);
+			continue;
+		}
+
+		if (gen11_vdbox_has_sfc(gt, i, logical_vdbox, vdbox_mask))
+			gt->info.vdbox_sfc_access |= BIT(i);
+		logical_vdbox++;
+	}
+	drm_dbg(&i915->drm, "vdbox enable: %04x, instances: %04lx\n",
+		vdbox_mask, VDBOX_MASK(gt));
+	GEM_BUG_ON(vdbox_mask != VDBOX_MASK(gt));
+
+	for (i = 0; i < I915_MAX_VECS; i++) {
+		if (!HAS_ENGINE(gt, _VECS(i))) {
+			vebox_mask &= ~BIT(i);
+			continue;
+		}
+
+		if (!(BIT(i) & vebox_mask)) {
+			gt->info.engine_mask &= ~BIT(_VECS(i));
+			drm_dbg(&i915->drm, "vecs%u fused off\n", i);
+		}
+	}
+	drm_dbg(&i915->drm, "vebox enable: %04x, instances: %04lx\n",
+		vebox_mask, VEBOX_MASK(gt));
+	GEM_BUG_ON(vebox_mask != VEBOX_MASK(gt));
+}
+
 static void engine_mask_apply_compute_fuses(struct intel_gt *gt)
 {
 	struct drm_i915_private *i915 = gt->i915;
@@ -671,6 +739,9 @@  static void engine_mask_apply_compute_fuses(struct intel_gt *gt)
 	unsigned long ccs_mask;
 	unsigned int i;
 
+	if (GRAPHICS_VER(i915) < 11)
+		return;
+
 	if (hweight32(CCS_MASK(gt)) <= 1)
 		return;
 
@@ -726,75 +797,11 @@  static void engine_mask_apply_copy_fuses(struct intel_gt *gt)
  */
 static intel_engine_mask_t init_engine_mask(struct intel_gt *gt)
 {
-	struct drm_i915_private *i915 = gt->i915;
 	struct intel_gt_info *info = &gt->info;
-	struct intel_uncore *uncore = gt->uncore;
-	unsigned int logical_vdbox = 0;
-	unsigned int i;
-	u32 media_fuse, fuse1;
-	u16 vdbox_mask;
-	u16 vebox_mask;
 
 	GEM_BUG_ON(!info->engine_mask);
 
-	if (GRAPHICS_VER(i915) < 11)
-		return info->engine_mask;
-
-	/*
-	 * On newer platforms the fusing register is called 'enable' and has
-	 * enable semantics, while on older platforms it is called 'disable'
-	 * and bits have disable semantices.
-	 */
-	media_fuse = intel_uncore_read(uncore, GEN11_GT_VEBOX_VDBOX_DISABLE);
-	if (MEDIA_VER_FULL(i915) < IP_VER(12, 50))
-		media_fuse = ~media_fuse;
-
-	vdbox_mask = media_fuse & GEN11_GT_VDBOX_DISABLE_MASK;
-	vebox_mask = (media_fuse & GEN11_GT_VEBOX_DISABLE_MASK) >>
-		      GEN11_GT_VEBOX_DISABLE_SHIFT;
-
-	if (MEDIA_VER_FULL(i915) >= IP_VER(12, 50)) {
-		fuse1 = intel_uncore_read(uncore, HSW_PAVP_FUSE1);
-		gt->info.sfc_mask = REG_FIELD_GET(XEHP_SFC_ENABLE_MASK, fuse1);
-	} else {
-		gt->info.sfc_mask = ~0;
-	}
-
-	for (i = 0; i < I915_MAX_VCS; i++) {
-		if (!HAS_ENGINE(gt, _VCS(i))) {
-			vdbox_mask &= ~BIT(i);
-			continue;
-		}
-
-		if (!(BIT(i) & vdbox_mask)) {
-			info->engine_mask &= ~BIT(_VCS(i));
-			drm_dbg(&i915->drm, "vcs%u fused off\n", i);
-			continue;
-		}
-
-		if (gen11_vdbox_has_sfc(gt, i, logical_vdbox, vdbox_mask))
-			gt->info.vdbox_sfc_access |= BIT(i);
-		logical_vdbox++;
-	}
-	drm_dbg(&i915->drm, "vdbox enable: %04x, instances: %04lx\n",
-		vdbox_mask, VDBOX_MASK(gt));
-	GEM_BUG_ON(vdbox_mask != VDBOX_MASK(gt));
-
-	for (i = 0; i < I915_MAX_VECS; i++) {
-		if (!HAS_ENGINE(gt, _VECS(i))) {
-			vebox_mask &= ~BIT(i);
-			continue;
-		}
-
-		if (!(BIT(i) & vebox_mask)) {
-			info->engine_mask &= ~BIT(_VECS(i));
-			drm_dbg(&i915->drm, "vecs%u fused off\n", i);
-		}
-	}
-	drm_dbg(&i915->drm, "vebox enable: %04x, instances: %04lx\n",
-		vebox_mask, VEBOX_MASK(gt));
-	GEM_BUG_ON(vebox_mask != VEBOX_MASK(gt));
-
+	engine_mask_apply_media_fuses(gt);
 	engine_mask_apply_compute_fuses(gt);
 	engine_mask_apply_copy_fuses(gt);