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

[Mika Kuoppala]

Chris Wilson <chris at chris-wilson.co.uk> writes:

> 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.

Ok.
>
>> > -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.

Agreed but this is documentation for function. It returns a wakeref.

>
>> >   */
>> > -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? :)

I should have been more specific. My concern was on documenting
the changing return values.
-Mika


> -Chris