Re: [PATCH] drm/i915: Only enable hotplug interrupts if the display interrupts are enabled

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

On Wed, Feb 15, 2017 at 12:04:18PM +0000, Chris Wilson wrote:
> On Wed, Feb 15, 2017 at 01:23:35PM +0200, Ville Syrjälä wrote:
> > On Wed, Feb 15, 2017 at 09:06:53AM +0000, Chris Wilson wrote:
> > > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> > > index 6a9c16508ab5..bad4f14858e3 100644
> > > --- a/drivers/gpu/drm/i915/intel_hotplug.c
> > > +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> > > @@ -224,7 +224,7 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
> > >  			}
> > >  		}
> > >  	}
> > > -	if (dev_priv->display.hpd_irq_setup)
> > > +	if (dev_priv->display_irqs_enabled && dev_priv->display.hpd_irq_setup)
> > 
> > I don't think we're setting display_irqs_enabled on any other platform
> > than VLV/CHV. So either we have to push this check down into
> > hpd_irq_setup() or just set display_irqs_enable=true somewhere for all
> > the other platforms.
> 
> Yup, only vlv/chv set display_irqs_enable. Would it not be safe to
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 99e2d659202c..7e205747f18d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4314,6 +4314,8 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>         if (!IS_GEN2(dev_priv))
>                 dev->vblank_disable_immediate = true;
>  
> +       dev_priv->display_irqs_enabled = true;
> +
>         dev_priv->hotplug.hpd_storm_threshold = HPD_STORM_DEFAULT_THRESHOLD;
> 
> which may then cause us to enable the display irq mask before the
> powerwell is powered, but as soon as we go through a display powerwell
> cycle will be back in sync? That presumes we complete such a cycle
> before hotplug init touches the register - and that we can touch the irq
> registers outside of the powerwell.

I guess that should work. Or we could just do it for !VLV/CHV if we want
to strictly maintain the current behaviour on those platforms.

> 
> Or we can do something a bit more complete:

Not sure such extra complexitry is worth the hassle for this little
thing,

> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c1b400f1ede4..681196987450 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1381,6 +1381,12 @@ struct i915_power_well;
>  
>  struct i915_power_well_ops {
>  	/*
> +	 * Initialize drm_i915_private or struct i915_power_well state,
> +	 * called on driver load.
> +	 */
> +	void (*init)(struct drm_i915_private *dev_priv,
> +		     struct i915_power_well *power_well);
> +	/*
>  	 * Synchronize the well's hw state to match the current sw state, for
>  	 * example enable/disable it based on the current refcount. Called
>  	 * during driver init and resume time, possibly after first calling
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 879567987201..4cb2b099f6c1 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1126,7 +1126,7 @@ static void vlv_init_display_clock_gating(struct drm_i915_private *dev_priv)
>  		   DIV_ROUND_CLOSEST(dev_priv->rawclk_freq, 1000));
>  }
>  
> -static void vlv_display_power_well_init(struct drm_i915_private *dev_priv)
> +static void vlv_display_power_well_install(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_encoder *encoder;
>  	enum pipe pipe;
> @@ -1175,7 +1175,7 @@ static void vlv_display_power_well_init(struct drm_i915_private *dev_priv)
>  	intel_pps_unlock_regs_wa(dev_priv);
>  }
>  
> -static void vlv_display_power_well_deinit(struct drm_i915_private *dev_priv)
> +static void vlv_display_power_well_uninstall(struct drm_i915_private *dev_priv)
>  {
>  	spin_lock_irq(&dev_priv->irq_lock);
>  	valleyview_disable_display_irqs(dev_priv);
> @@ -1191,6 +1191,12 @@ static void vlv_display_power_well_deinit(struct drm_i915_private *dev_priv)
>  		intel_hpd_poll_init(dev_priv);
>  }
>  
> +static void vlv_display_power_well_init(struct drm_i915_private *dev_priv,
> +					struct i915_power_well *power_well)
> +{
> +	dev_priv->display_irqs_enabled = false;
> +}
> +
>  static void vlv_display_power_well_enable(struct drm_i915_private *dev_priv,
>  					  struct i915_power_well *power_well)
>  {
> @@ -1198,7 +1204,7 @@ static void vlv_display_power_well_enable(struct drm_i915_private *dev_priv,
>  
>  	vlv_set_power_well(dev_priv, power_well, true);
>  
> -	vlv_display_power_well_init(dev_priv);
> +	vlv_display_power_well_install(dev_priv);
>  }
>  
>  static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv,
> @@ -1206,7 +1212,7 @@ static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv,
>  {
>  	WARN_ON_ONCE(power_well->id != PUNIT_POWER_WELL_DISP2D);
>  
> -	vlv_display_power_well_deinit(dev_priv);
> +	vlv_display_power_well_uninstall(dev_priv);
>  
>  	vlv_set_power_well(dev_priv, power_well, false);
>  }
> @@ -1661,6 +1667,12 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> +static void chv_pipe_power_well_init(struct drm_i915_private *dev_priv,
> +				     struct i915_power_well *power_well)
> +{
> +	dev_priv->display_irqs_enabled = false;
> +}
> +
>  static void chv_pipe_power_well_sync_hw(struct drm_i915_private *dev_priv,
>  					struct i915_power_well *power_well)
>  {
> @@ -1676,7 +1688,7 @@ static void chv_pipe_power_well_enable(struct drm_i915_private *dev_priv,
>  
>  	chv_set_pipe_power_well(dev_priv, power_well, true);
>  
> -	vlv_display_power_well_init(dev_priv);
> +	vlv_display_power_well_install(dev_priv);
>  }
>  
>  static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
> @@ -1684,7 +1696,7 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
>  {
>  	WARN_ON_ONCE(power_well->id != PIPE_A);
>  
> -	vlv_display_power_well_deinit(dev_priv);
> +	vlv_display_power_well_uninstall(dev_priv);
>  
>  	chv_set_pipe_power_well(dev_priv, power_well, false);
>  }
> @@ -1921,6 +1933,7 @@ static const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
>  };
>  
>  static const struct i915_power_well_ops chv_pipe_power_well_ops = {
> +	.init = chv_pipe_power_well_init,
>  	.sync_hw = chv_pipe_power_well_sync_hw,
>  	.enable = chv_pipe_power_well_enable,
>  	.disable = chv_pipe_power_well_disable,
> @@ -2000,6 +2013,7 @@ static struct i915_power_well bdw_power_wells[] = {
>  };
>  
>  static const struct i915_power_well_ops vlv_display_power_well_ops = {
> +	.init = vlv_display_power_well_init,
>  	.sync_hw = vlv_power_well_sync_hw,
>  	.enable = vlv_display_power_well_enable,
>  	.disable = vlv_display_power_well_disable,
> @@ -2367,6 +2381,19 @@ static uint32_t get_allowed_dc_mask(const struct drm_i915_private *dev_priv,
>  	return mask;
>  }
>  
> +static void __intel_power_domains_init(struct drm_i915_private *dev_priv)
> +{
> +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +	struct i915_power_well *power_well;
> +	int i;
> +
> +	dev_priv->display_irqs_enabled = true;
> +	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
> +		if (power_well->ops->init)
> +			power_well->ops->init(dev_priv, power_well);
> +	}
> +}
> +
>  #define set_power_wells(power_domains, __power_wells) ({		\
>  	(power_domains)->power_wells = (__power_wells);			\
>  	(power_domains)->power_well_count = ARRAY_SIZE(__power_wells);	\
> @@ -2414,6 +2441,8 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
>  		set_power_wells(power_domains, i9xx_always_on_power_well);
>  	}
>  
> +	__intel_power_domains_init(dev_priv);
> +
>  	return 0;
>  }
>  
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ville Syrjälä
Intel OTC