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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
