[Intel-gfx] [PATCH 5/8] drm/i915/icl: Add reset control register changes

[Daniele Ceraolo Spurio]

On 16/03/18 05:14, Mika Kuoppala wrote:
> From: Michel Thierry <michel.thierry at intel.com>
> 
> The bits used to reset the different engines/domains have changed in
> GEN11, this patch maps the reset engine mask bits with the new bits
> in the reset control register.
> 
> v2: Use shift-left instead of BIT macro to match the file style (Paulo).
> v3: Reuse gen8_reset_engines (Daniele).
> v4: Do not call intel_uncore_forcewake_reset after reset, we may be
> using the forcewake to read protected registers elsewhere and those
> results may be clobbered by the concurrent dropping of forcewake.
> 
> bspec: 19212
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> Cc: Antonio Argenziano <antonio.argenziano at intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Acked-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Signed-off-by: Michel Thierry <michel.thierry at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h     | 11 ++++++++
>   drivers/gpu/drm/i915/intel_uncore.c | 53 +++++++++++++++++++++++++++++++++++--
>   2 files changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9eaaa96287ec..f3cc77690124 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -301,6 +301,17 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>   #define  GEN6_GRDOM_VECS		(1 << 4)
>   #define  GEN9_GRDOM_GUC			(1 << 5)
>   #define  GEN8_GRDOM_MEDIA2		(1 << 7)
> +/* GEN11 changed all bit defs except for FULL & RENDER */
> +#define  GEN11_GRDOM_FULL		GEN6_GRDOM_FULL
> +#define  GEN11_GRDOM_RENDER		GEN6_GRDOM_RENDER
> +#define  GEN11_GRDOM_BLT		(1 << 2)
> +#define  GEN11_GRDOM_GUC		(1 << 3)
> +#define  GEN11_GRDOM_MEDIA		(1 << 5)
> +#define  GEN11_GRDOM_MEDIA2		(1 << 6)
> +#define  GEN11_GRDOM_MEDIA3		(1 << 7)
> +#define  GEN11_GRDOM_MEDIA4		(1 << 8)
> +#define  GEN11_GRDOM_VECS		(1 << 13)
> +#define  GEN11_GRDOM_VECS2		(1 << 14)
>   
>   #define RING_PP_DIR_BASE(engine)	_MMIO((engine)->mmio_base+0x228)
>   #define RING_PP_DIR_BASE_READ(engine)	_MMIO((engine)->mmio_base+0x518)
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 4c616d074a97..cabbf0e682e7 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1909,6 +1909,50 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
>   	return gen6_hw_domain_reset(dev_priv, hw_mask);
>   }
>   
> +/**
> + * gen11_reset_engines - reset individual engines
> + * @dev_priv: i915 device
> + * @engine_mask: mask of intel_ring_flag() engines or ALL_ENGINES for full reset
> + *
> + * This function will reset the individual engines that are set in engine_mask.
> + * If you provide ALL_ENGINES as mask, full global domain reset will be issued.
> + *
> + * Note: It is responsibility of the caller to handle the difference between
> + * asking full domain reset versus reset for all available individual engines.
> + *
> + * Returns 0 on success, nonzero on error.
> + */
> +static int gen11_reset_engines(struct drm_i915_private *dev_priv,
> +			       unsigned engine_mask)
> +{
> +	struct intel_engine_cs *engine;
> +	const u32 hw_engine_mask[I915_NUM_ENGINES] = {
> +		[RCS] = GEN11_GRDOM_RENDER,
> +		[BCS] = GEN11_GRDOM_BLT,
> +		[VCS] = GEN11_GRDOM_MEDIA,
> +		[VCS2] = GEN11_GRDOM_MEDIA2,
> +		[VCS3] = GEN11_GRDOM_MEDIA3,
> +		[VCS4] = GEN11_GRDOM_MEDIA4,
> +		[VECS] = GEN11_GRDOM_VECS,
> +		[VECS2] = GEN11_GRDOM_VECS2,
> +	};

Just a thought, but since this function is a copy of gen6_reset_engines 
with the only difference being the array (GEN11_GRDOM_FULL is also the 
same as GEN6_GRDOM_FULL), would it make sense to just add the array to 
the gen6 function? e.g.:

	const u32 gen6_hw_engine_mask[] = {
	....
	}
	const u32 gen11_hw_engine_mask[] = {
	....
	}

	const u32 *hw_engine_mask = INTEL_GEN(dev_priv) >= 11 ?
		gen11_hw_engine_mask : gen6_hw_engine_mask;


My Ack still stands regardless and I also agree with renaming the 
defines to be closer to the specs.

Daniele

> +	u32 hw_mask;
> +
> +	BUILD_BUG_ON(VECS2 + 1 != I915_NUM_ENGINES);
> +
> +	if (engine_mask == ALL_ENGINES) {
> +		hw_mask = GEN11_GRDOM_FULL;
> +	} else {
> +		unsigned int tmp;
> +
> +		hw_mask = 0;
> +		for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
> +			hw_mask |= hw_engine_mask[engine->id];
> +	}
> +
> +	return gen6_hw_domain_reset(dev_priv, hw_mask);
> +}
> +
>   /**
>    * __intel_wait_for_register_fw - wait until register matches expected state
>    * @dev_priv: the i915 device
> @@ -2056,7 +2100,10 @@ static int gen8_reset_engines(struct drm_i915_private *dev_priv,
>   		if (gen8_reset_engine_start(engine))
>   			goto not_ready;
>   
> -	return gen6_reset_engines(dev_priv, engine_mask);
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		return gen11_reset_engines(dev_priv, engine_mask);
> +	else
> +		return gen6_reset_engines(dev_priv, engine_mask);
>   
>   not_ready:
>   	for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
> @@ -2141,12 +2188,14 @@ bool intel_has_reset_engine(struct drm_i915_private *dev_priv)
>   
>   int intel_reset_guc(struct drm_i915_private *dev_priv)
>   {
> +	u32 guc_domain = INTEL_GEN(dev_priv) >= 11 ? GEN11_GRDOM_GUC :
> +						     GEN9_GRDOM_GUC;
>   	int ret;
>   
>   	GEM_BUG_ON(!HAS_GUC(dev_priv));
>   
>   	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
> -	ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
> +	ret = gen6_hw_domain_reset(dev_priv, guc_domain);
>   	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>   
>   	return ret;
>