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