diff mbox series

[2/6] drm/i915: Trust programmed MCR in read_subslice_reg

Message ID 20190717180624.20354-3-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series MCR fixes and more | expand

Commit Message

Tvrtko Ursulin July 17, 2019, 6:06 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c | 37 ++++++++---------------
 1 file changed, 12 insertions(+), 25 deletions(-)

Comments

Chris Wilson July 17, 2019, 7:31 p.m. UTC | #1 
Quoting Tvrtko Ursulin (2019-07-17 19:06:20)
> From: Tvrtko Ursulin <tvrtko.ursulin@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.

I successfully worked back from implementation to changelog.
> 
> 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@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Summers, Stuart July 17, 2019, 8:47 p.m. UTC | #2 
On Wed, 2019-07-17 at 19:06 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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@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@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 */
Summers, Stuart July 17, 2019, 8:49 p.m. UTC | #3 
On Wed, 2019-07-17 at 20:47 +0000, Summers, Stuart wrote:
> On Wed, 2019-07-17 at 19:06 +0100, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@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@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.

I see you already did this, what I get for reading these one at a
time... Please ignore the above comment.

Thanks,
Stuart

> 
> Reviewed-by: Stuart Summers <stuart.summers@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 */
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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 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);
-
 	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 */