[Intel-gfx] [PATCH 04/46] drm/i915: Markup paired operations on wakerefs

[Chris Wilson]

Quoting Mika Kuoppala (2019-01-08 16:23:18)
> > @@ -3965,7 +4014,7 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
> >  void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv)
> >  {
> >       /* Keep the power well enabled, but cancel its rpm wakeref. */
> > -     intel_runtime_pm_put(dev_priv);
> > +     intel_runtime_pm_put_unchecked(dev_priv);
> >  
> >       /* Remove the refcount we took to keep power well support disabled. */
> >       if (!i915_modparams.disable_power_well)
> > @@ -4179,7 +4228,7 @@ static void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
> >   * Any runtime pm reference obtained by this function must have a symmetric
> >   * call to intel_runtime_pm_put() to release the reference again.
> >   */
> 
> Need to update the documentation.

No. You are expected to pair intel_runtime_pm_get with intel_runtime_pm_put.
The _unchecked version is temporary and not expected to be used in new code.
Once the dust has settled it will be gone.

* Any runtime pm reference obtained by this function must have a symmetric
* call to intel_runtime_pm_put() to release the reference again.

is accurate.

> > -void intel_runtime_pm_get(struct drm_i915_private *i915)
> > +intel_wakeref_t intel_runtime_pm_get(struct drm_i915_private *i915)
> >  {
> >       struct pci_dev *pdev = i915->drm.pdev;
> >       struct device *kdev = &pdev->dev;
> > @@ -4191,7 +4240,7 @@ void intel_runtime_pm_get(struct drm_i915_private *i915)
> >       atomic_inc(&i915->runtime_pm.wakeref_count);
> >       assert_rpm_wakelock_held(i915);
> >  
> > -     track_intel_runtime_pm_wakeref(i915);
> > +     return track_intel_runtime_pm_wakeref(i915);
> >  }
> >  
> >  /**
> > @@ -4207,7 +4256,7 @@ void intel_runtime_pm_get(struct drm_i915_private *i915)
> >   *
> >   * Returns: True if the wakeref was acquired, or False otherwise.
> 
> For practical purposes this could still be the case but please update
> the return value type.

Still should be used as a boolean (true/false) though.

> >   */
> > -bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *i915)
> > +intel_wakeref_t intel_runtime_pm_get_if_in_use(struct drm_i915_private *i915)
> >  {
> >       if (IS_ENABLED(CONFIG_PM)) {
> >               struct pci_dev *pdev = i915->drm.pdev;
> > @@ -4220,15 +4269,13 @@ bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *i915)
> >                * atm to the late/early system suspend/resume handlers.
> >                */
> >               if (pm_runtime_get_if_in_use(kdev) <= 0)
> > -                     return false;
> > +                     return 0;
> >       }
> >  
> >       atomic_inc(&i915->runtime_pm.wakeref_count);
> >       assert_rpm_wakelock_held(i915);
> >  
> > -     track_intel_runtime_pm_wakeref(i915);
> > -
> > -     return true;
> > +     return track_intel_runtime_pm_wakeref(i915);
> >  }
> >  
> >  /**
> > @@ -4248,7 +4295,7 @@ bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *i915)
> >   * Any runtime pm reference obtained by this function must have a symmetric
> >   * call to intel_runtime_pm_put() to release the reference again.
> >   */
> 
> Document update needed here also.

Nope.

> > -void intel_runtime_pm_get_noresume(struct drm_i915_private *i915)
> > +intel_wakeref_t intel_runtime_pm_get_noresume(struct drm_i915_private *i915)
> >  {
> >       struct pci_dev *pdev = i915->drm.pdev;
> >       struct device *kdev = &pdev->dev;
> > @@ -4258,7 +4305,7 @@ void intel_runtime_pm_get_noresume(struct drm_i915_private *i915)
> >  
> >       atomic_inc(&i915->runtime_pm.wakeref_count);
> >  
> > -     track_intel_runtime_pm_wakeref(i915);
> > +     return track_intel_runtime_pm_wakeref(i915);
> >  }
> >  
> >  /**
> > @@ -4269,7 +4316,7 @@ void intel_runtime_pm_get_noresume(struct drm_i915_private *i915)
> >   * intel_runtime_pm_get() and might power down the corresponding
> >   * hardware block right away if this is the last reference.
> >   */
> 
> Documentation part needs updating.

I either don't get your point, or you missed the point of the wakeref
tracking? :)
-Chris