Re: [Intel-gfx] [PATCH v2 2/2] drm/i915/gt: Extract function to apply media fuses

[Andrzej Hajda <andrzej.hajda@intel.com>]

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);
>   
>