[Intel-gfx] [RFC 1/8] drm/i915: Introduce intel_runtime_pm_disable to pair intel_runtime_pm_enable

[Imre Deak]

On Fri, Aug 10, 2018 at 01:33:19PM +0100, Chris Wilson wrote:
> Quoting Imre Deak (2018-08-10 13:22:43)
> > On Fri, Aug 10, 2018 at 08:11:31AM +0100, Chris Wilson wrote:
> > > @@ -1474,6 +1476,8 @@ void i915_driver_unload(struct drm_device *dev)
> > >       i915_driver_cleanup_mmio(dev_priv);
> > >  
> > >       intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > > +
> > > +     WARN_ON(atomic_read(&dev_priv->runtime_pm.wakeref_count));
> > 
> > This probably WARNs atm at least due to having a low-level
> > pm_runtime_put_autosuspend() in intel_runtime_pm_enable(); but a
> > corresponding intel_runtime_pm_get() in intel_power_domains_fini_hw() (via
> > intel_display_set_init_power) which is i915 specific. The following
> > would fix that:
> 
> It gets caught out by the very last display_set_init_power indeed. I
> mean to have a word with you about that function ;)
> 
> The issue we have with intel_display_set_init_power() at the moment is
> that we don't always clean up it correctly as it not obvious who is
> responsible for it at any time. Would it be possible to make that into
> local POWER_DOMAIN_INIT grabs so the onion is always clear? (It wasn't
> obvious to me why ownership was being transferred fairly arbitrary.)

Yes, would be much clearer. One thing that depends on it is driver
loading / resume expecting any power wells enabled by BIOS to stay enabled
until the end of display HW readout. I can take a look at the exact
dependencies there and remove the use of intel_display_set_init_power().

> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index e209edbc561d..1623c0d2cf57 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -3806,6 +3806,10 @@ void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv)
> >          */
> >         intel_display_set_init_power(dev_priv, true);
> 
> In this case this would be much clearer as a raw
> 	intel_power_domain_get(POWER_DOMAIN_INIT)
> I think.

Yes, but would have to change it in sync with the
intel_display_set_init_power() in intel_power_domains_init_hw(). An
error somewhere after that and before the end of HW readout would make
the above intel_display_set_init_power() a nop.

--Imre