Re: [PATCH 1/2] drm/i915: Make sure pipe interrupts are processed before turning off power well on BDW+

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

On Mon, Feb 22, 2016 at 03:59:28PM +0200, Imre Deak wrote:
> On pe, 2016-02-19 at 20:47 +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Starting from BDW the DE_PIPE interrupts for pipe B and C belong to the
> > relevant display power well. So we should make sure we've finished
> > processing them before turning off the power well.
> > 
> > The pipe interrupts shouldn't really happen at this point anymore since
> > we've already shut down the planes/pipes/whatnot, but being a bit
> > paranoid shouldn't hurt.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c         | 16 ++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h        |  2 ++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 19 +++++++++++++++++++
> >  3 files changed, 37 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index d56c261ad867..a9048e1b96e5 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -3366,6 +3366,22 @@ void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
> >  	spin_unlock_irq(&dev_priv->irq_lock);
> >  }
> >  
> > +void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
> > +				     unsigned int pipe_mask)
> > +{
> > +	spin_lock_irq(&dev_priv->irq_lock);
> > +	if (pipe_mask & 1 << PIPE_A)
> > +		GEN8_IRQ_RESET_NDX(DE_PIPE, PIPE_A);
> > +	if (pipe_mask & 1 << PIPE_B)
> > +		GEN8_IRQ_RESET_NDX(DE_PIPE, PIPE_B);
> > +	if (pipe_mask & 1 << PIPE_C)
> > +		GEN8_IRQ_RESET_NDX(DE_PIPE, PIPE_C);
> > +	spin_unlock_irq(&dev_priv->irq_lock);
> > +
> > +	/* make sure we're done processing display irqs */
> > +	synchronize_irq(dev_priv->dev->irq);
> > +}
> > +
> >  static void cherryview_irq_preinstall(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 285b0570be9c..2618e3026e8b 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -993,6 +993,8 @@ static inline bool intel_irqs_enabled(struct drm_i915_private *dev_priv)
> >  int intel_get_crtc_scanline(struct intel_crtc *crtc);
> >  void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
> >  				     unsigned int pipe_mask);
> > +void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
> > +				     unsigned int pipe_mask);
> >  
> >  /* intel_crt.c */
> >  void intel_crt_init(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 59e9222223c9..1afa00855ed8 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -284,6 +284,13 @@ static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
> >  						1 << PIPE_C | 1 << PIPE_B);
> >  }
> >  
> > +static void hsw_power_well_pre_disable(struct drm_i915_private *dev_priv)
> > +{
> > +	if (IS_BROADWELL(dev_priv))
> > +		gen8_irq_power_well_pre_disable(dev_priv,
> > +						1 << PIPE_C | 1 << PIPE_B);
> > +}
> > +
> >  static void skl_power_well_post_enable(struct drm_i915_private *dev_priv,
> >  				       struct i915_power_well *power_well)
> >  {
> > @@ -309,6 +316,14 @@ static void skl_power_well_post_enable(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >  
> > +static void skl_power_well_pre_disable(struct drm_i915_private *dev_priv,
> > +				       struct i915_power_well *power_well)
> > +{
> > +	if (power_well->data == SKL_DISP_PW_2)
> > +		gen8_irq_power_well_pre_disable(dev_priv,
> > +						1 << PIPE_C | 1 << PIPE_B);
> > +}
> > +
> >  static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> >  			       struct i915_power_well *power_well, bool enable)
> >  {
> > @@ -334,6 +349,7 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> >  
> >  	} else {
> >  		if (enable_requested) {
> > +			hsw_power_well_pre_disable(dev_priv);
> >  			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
> >  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> >  			DRM_DEBUG_KMS("Requesting to disable the power well\n");
> > @@ -663,6 +679,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >  	state_mask = SKL_POWER_WELL_STATE(power_well->data);
> >  	is_enabled = tmp & state_mask;
> >  
> > +	if (!enable && enable_requested)
> > +		skl_power_well_pre_disable(dev_priv, power_well);
> > +
> 
> Why not putting this in the existing if branch? Either way:
> Reviewed-by: Imre Deak <imre.deak@intel.com>

I figured it's better if it mirrors the way the post_enable is done.

> 
> >  	if (enable) {
> >  		if (!enable_requested) {
> >  			WARN((tmp & state_mask) &&

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