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