Re: [PATCH 1/1] drm/i915: Save PM interrupt register offsets in device info

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 24, 2017 at 08:42:41PM +0300, Jani Nikula wrote:
> On Tue, 24 Oct 2017, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote:
> > PM interrupt register offsets are constant per platforms and saving those
> > in device info is more appropriate than getting those through functions.
> > This patch removes functions gen6_pm_iir/imr/ier and saves those offsets
> > in device info.
> 
> Do we *really* want to start on this path? We have the *_offsets in
> device info for some groups of registers, but this is a whole another
> ballgame. You could make the same argument for half the registers in
> i915_reg.h!
> 
> If you don't want to use a function to get the correct
> register... almost all of the rest of the driver uses if-else in code
> for this...
> 
> The issue is not this single patch per se. The issue is the precedence
> it sets, and the apparent lack of thought on where we'll end up with
> this.

I definitely think we shouldn't do this for individual registers. I even
removed some _offsets[] arrays in the past because they were effectively
covering just one or two registers, and thus didn't seem justified.

In this case we could actually make do with a single offset that would
cover the four interrupt registers. So it's maybe on the edge of what
seems sensible to me.

> 
> 
> BR,
> Jani.
> 
> >
> > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h          |  5 +++++
> >  drivers/gpu/drm/i915/i915_irq.c          | 30 ++++++++----------------------
> >  drivers/gpu/drm/i915/intel_device_info.c | 11 +++++++++++
> >  3 files changed, 24 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 54b5d4c..2f77d26 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -888,6 +888,11 @@ struct intel_device_info {
> >  		u16 degamma_lut_size;
> >  		u16 gamma_lut_size;
> >  	} color;
> > +
> > +	/* PM interrupt register offsets */
> > +	i915_reg_t pm_iir_offset;
> > +	i915_reg_t pm_imr_offset;
> > +	i915_reg_t pm_ier_offset;
> >  };
> >  
> >  struct intel_display_error_state;
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index b1296a5..68c6f44 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -306,21 +306,6 @@ void gen5_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> >  	ilk_update_gt_irq(dev_priv, mask, 0);
> >  }
> >  
> > -static i915_reg_t gen6_pm_iir(struct drm_i915_private *dev_priv)
> > -{
> > -	return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IIR(2) : GEN6_PMIIR;
> > -}
> > -
> > -static i915_reg_t gen6_pm_imr(struct drm_i915_private *dev_priv)
> > -{
> > -	return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IMR(2) : GEN6_PMIMR;
> > -}
> > -
> > -static i915_reg_t gen6_pm_ier(struct drm_i915_private *dev_priv)
> > -{
> > -	return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IER(2) : GEN6_PMIER;
> > -}
> > -
> >  /**
> >   * snb_update_pm_irq - update GEN6_PMIMR
> >   * @dev_priv: driver private
> > @@ -343,8 +328,8 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
> >  
> >  	if (new_val != dev_priv->pm_imr) {
> >  		dev_priv->pm_imr = new_val;
> > -		I915_WRITE(gen6_pm_imr(dev_priv), dev_priv->pm_imr);
> > -		POSTING_READ(gen6_pm_imr(dev_priv));
> > +		I915_WRITE(dev_priv->info.pm_imr_offset, dev_priv->pm_imr);
> > +		POSTING_READ(dev_priv->info.pm_imr_offset);
> >  	}
> >  }
> >  
> > @@ -371,7 +356,7 @@ void gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
> >  
> >  static void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 reset_mask)
> >  {
> > -	i915_reg_t reg = gen6_pm_iir(dev_priv);
> > +	i915_reg_t reg = dev_priv->info.pm_iir_offset;
> >  
> >  	lockdep_assert_held(&dev_priv->irq_lock);
> >  
> > @@ -385,7 +370,7 @@ static void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, u32 enable_mas
> >  	lockdep_assert_held(&dev_priv->irq_lock);
> >  
> >  	dev_priv->pm_ier |= enable_mask;
> > -	I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier);
> > +	I915_WRITE(dev_priv->info.pm_ier_offset, dev_priv->pm_ier);
> >  	gen6_unmask_pm_irq(dev_priv, enable_mask);
> >  	/* unmask_pm_irq provides an implicit barrier (POSTING_READ) */
> >  }
> > @@ -396,7 +381,7 @@ static void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, u32 disable_m
> >  
> >  	dev_priv->pm_ier &= ~disable_mask;
> >  	__gen6_mask_pm_irq(dev_priv, disable_mask);
> > -	I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier);
> > +	I915_WRITE(dev_priv->info.pm_ier_offset, dev_priv->pm_ier);
> >  	/* though a barrier is missing here, but don't really need a one */
> >  }
> >  
> > @@ -417,7 +402,8 @@ void gen6_enable_rps_interrupts(struct drm_i915_private *dev_priv)
> >  
> >  	spin_lock_irq(&dev_priv->irq_lock);
> >  	WARN_ON_ONCE(rps->pm_iir);
> > -	WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & dev_priv->pm_rps_events);
> > +	WARN_ON_ONCE(I915_READ(dev_priv->info.pm_iir_offset) &
> > +			       dev_priv->pm_rps_events);
> >  	rps->interrupts_enabled = true;
> >  	gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> >  
> > @@ -461,7 +447,7 @@ void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
> >  {
> >  	spin_lock_irq(&dev_priv->irq_lock);
> >  	if (!dev_priv->guc.interrupts_enabled) {
> > -		WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
> > +		WARN_ON_ONCE(I915_READ(dev_priv->info.pm_iir_offset) &
> >  				       dev_priv->pm_guc_events);
> >  		dev_priv->guc.interrupts_enabled = true;
> >  		gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> > index 875d428..d1a4911 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
> >  			 info->sseu.has_subslice_pg ? "y" : "n");
> >  	DRM_DEBUG_DRIVER("has EU power gating: %s\n",
> >  			 info->sseu.has_eu_pg ? "y" : "n");
> > +
> > +	/* Initialize PM interrupt register offsets */
> > +	if (INTEL_GEN(dev_priv) >= 8) {
> > +		info->pm_iir_offset = GEN8_GT_IIR(2);
> > +		info->pm_imr_offset = GEN8_GT_IMR(2);
> > +		info->pm_ier_offset = GEN8_GT_IER(2);
> > +	} else {
> > +		info->pm_iir_offset = GEN6_PMIIR;
> > +		info->pm_imr_offset = GEN6_PMIMR;
> > +		info->pm_ier_offset = GEN6_PMIER;
> > +	}
> >  }
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx