[Intel-gfx] [PATCH 2/6] drm/i915: Trust programmed MCR in read_subslice_reg

[Summers, Stuart]

On Wed, 2019-07-17 at 19:06 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Instead of re-calculating the MCR selector in read_subslice_reg do
> the
> rwm on its existing value and restore it when done.
> 
> This consolidates MCR programming to one place for cnl+, and avoids
> re-calculating its default value on older platforms during hangcheck.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c | 37 ++++++++-------------
> --
>  1 file changed, 12 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 6f93caf7a5a1..cc4d1826173d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -989,27 +989,17 @@ read_subslice_reg(struct intel_engine_cs
> *engine, int slice, int subslice,
>  {
>  	struct drm_i915_private *i915 = engine->i915;
>  	struct intel_uncore *uncore = engine->uncore;
> -	u32 mcr_slice_subslice_mask;
> -	u32 mcr_slice_subslice_select;
> -	u32 default_mcr_s_ss_select;
> -	u32 mcr;
> -	u32 ret;
> +	u32 mcr_mask, mcr_ss, mcr, old_mcr, val;
>  	enum forcewake_domains fw_domains;
>  
>  	if (INTEL_GEN(i915) >= 11) {
> -		mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK |
> -					  GEN11_MCR_SUBSLICE_MASK;
> -		mcr_slice_subslice_select = GEN11_MCR_SLICE(slice) |
> -					    GEN11_MCR_SUBSLICE(subslice
> );
> +		mcr_mask = GEN11_MCR_SLICE_MASK |
> GEN11_MCR_SUBSLICE_MASK;
> +		mcr_ss = GEN11_MCR_SLICE(slice) |
> GEN11_MCR_SUBSLICE(subslice);
>  	} else {
> -		mcr_slice_subslice_mask = GEN8_MCR_SLICE_MASK |
> -					  GEN8_MCR_SUBSLICE_MASK;
> -		mcr_slice_subslice_select = GEN8_MCR_SLICE(slice) |
> -					    GEN8_MCR_SUBSLICE(subslice)
> ;
> +		mcr_mask = GEN8_MCR_SLICE_MASK |
> GEN8_MCR_SUBSLICE_MASK;
> +		mcr_ss = GEN8_MCR_SLICE(slice) |
> GEN8_MCR_SUBSLICE(subslice);
>  	}
>  
> -	default_mcr_s_ss_select =
> intel_calculate_mcr_s_ss_select(i915);

I'm only now reading one user of this function after this change. Does
it make sense to move it to gt/intel_workarounds.c as a static function
instead? Not critical here.

Reviewed-by: Stuart Summers <stuart.summers at intel.com>

> -
>  	fw_domains = intel_uncore_forcewake_for_reg(uncore, reg,
>  						    FW_REG_READ);
>  	fw_domains |= intel_uncore_forcewake_for_reg(uncore,
> @@ -1019,26 +1009,23 @@ read_subslice_reg(struct intel_engine_cs
> *engine, int slice, int subslice,
>  	spin_lock_irq(&uncore->lock);
>  	intel_uncore_forcewake_get__locked(uncore, fw_domains);
>  
> -	mcr = intel_uncore_read_fw(uncore, GEN8_MCR_SELECTOR);
> -
> -	WARN_ON_ONCE((mcr & mcr_slice_subslice_mask) !=
> -		     default_mcr_s_ss_select);
> +	old_mcr = mcr = intel_uncore_read_fw(uncore,
> GEN8_MCR_SELECTOR);
>  
> -	mcr &= ~mcr_slice_subslice_mask;
> -	mcr |= mcr_slice_subslice_select;
> +	mcr &= ~mcr_mask;
> +	mcr |= mcr_ss;
>  	intel_uncore_write_fw(uncore, GEN8_MCR_SELECTOR, mcr);
>  
> -	ret = intel_uncore_read_fw(uncore, reg);
> +	val = intel_uncore_read_fw(uncore, reg);
>  
> -	mcr &= ~mcr_slice_subslice_mask;
> -	mcr |= default_mcr_s_ss_select;
> +	mcr &= ~mcr_mask;
> +	mcr |= old_mcr & mcr_mask;
>  
>  	intel_uncore_write_fw(uncore, GEN8_MCR_SELECTOR, mcr);
>  
>  	intel_uncore_forcewake_put__locked(uncore, fw_domains);
>  	spin_unlock_irq(&uncore->lock);
>  
> -	return ret;
> +	return val;
>  }
>  
>  /* NB: please notice the memset */
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3270 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20190717/f9c803a8/attachment.bin>