Quoting John Harrison (2019-01-10 23:15:31)
> On 1/10/2019 02:11, Chris Wilson wrote:
> > @@ -4054,18 +4055,20 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
> > * resources powered until display HW readout is complete. We drop
> > * this reference in intel_power_domains_enable().
> > */
> > - intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > + power_domains->wakeref =
> > + intel_display_power_get(i915, POWER_DOMAIN_INIT);
> > +
> > /* Disable power support if the user asked so. */
> > if (!i915_modparams.disable_power_well)
> > - intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > - intel_power_domains_sync_hw(dev_priv);
> > + intel_display_power_get(i915, POWER_DOMAIN_INIT);
> Why not save this cookie away somewhere too, e.g.
> power_domains->wakeref_disable? That way you can also get rid of the
> put_unchecked calls below.
Just seemed like a bit more hassle for the case where rpm was
intentionally disabled by the user. The system is going to leak the
wakerefs, tracking seemed pointless, so I just threw my hands up in the
air and gave up.
> > + intel_power_domains_sync_hw(i915);
> >
> > power_domains->initializing = false;
> > }
> >
> > /**
> > * intel_power_domains_fini_hw - deinitialize hw power domain state
> > - * @dev_priv: i915 device instance
> > + * @i915: i915 device instance
> > *
> > * De-initializes the display power domain HW state. It also ensures that the
> > * device stays powered up so that the driver can be reloaded.
> > @@ -4074,21 +4077,24 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
> > * intel_power_domains_disable()) and must be paired with
> > * intel_power_domains_init_hw().
> > */
> > -void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv)
> > +void intel_power_domains_fini_hw(struct drm_i915_private *i915)
> > {
> > - /* Keep the power well enabled, but cancel its rpm wakeref. */
> > - intel_runtime_pm_put_unchecked(dev_priv);
> > + intel_wakeref_t wakeref __maybe_unused =
> > + fetch_and_zero(&i915->power_domains.wakeref);
> Why the '__maybe_unused'? The call to put is unconditional. Or is it
> about keeping the compiler happy in the case where the wakeref tracking
> is disabled? Although I don't recall seeing any 'maybe's in the earlier
> patches.
Just about keeping the compiler happy when the compiler complained.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
