Chris Wilson <ch...@chris-wilson.co.uk> writes:
> drm/i915 is tracking all wakeref owners with a cookie in order to
> identify leaks. To that end, each rpm acquisition ops->get_power is
> assigned a cookie which should be passed to ops->put_power to signify
> its release (and removal from the list of wakeref owners). As snd/hda is
> already using a bool to track current status of display_power extending
> that to an unsigned long to hold the boolean cookie is a trivial
> extension, and will quell all doubt that snd/hda is the cause of the
> device runtime pm leaks.
>
> v2: Keep using the power abstraction for local wakeref tracking.
>
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Takashi Iwai <ti...@suse.de>
> Cc: Jani Nikula <jani.nik...@intel.com>
> ---
> drivers/gpu/drm/i915/intel_audio.c | 20 +++++++++++---------
> include/drm/drm_audio_component.h | 7 +++++--
> include/sound/hdaudio.h | 2 +-
> sound/hda/hdac_component.c | 18 ++++++++++++------
> 4 files changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_audio.c
> b/drivers/gpu/drm/i915/intel_audio.c
> index 5104c6bbd66f..a1e60370eb34 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -741,27 +741,28 @@ void intel_init_audio_hooks(struct drm_i915_private
> *dev_priv)
> }
> }
>
> -static void i915_audio_component_get_power(struct device *kdev)
> +static unsigned long i915_audio_component_get_power(struct device *kdev)
> {
> - intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> + return intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> }
>
> -static void i915_audio_component_put_power(struct device *kdev)
> +static void i915_audio_component_put_power(struct device *kdev,
> + unsigned long cookie)
Changing the name and type is warranted as the layer changes.
We discussed on irc about catching the possible future type
mismatches with build bug on.
Reviewed-by: Mika Kuoppala <mika.kuopp...@linux.intel.com>
> {
> - intel_display_power_put_unchecked(kdev_to_i915(kdev),
> - POWER_DOMAIN_AUDIO);
> + intel_display_power_put(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO, cookie);
> }
>
> static void i915_audio_component_codec_wake_override(struct device *kdev,
> bool enable)
> {
> struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> + unsigned long wakeref;
> u32 tmp;
>
> if (!IS_GEN(dev_priv, 9))
> return;
>
> - i915_audio_component_get_power(kdev);
> + wakeref = i915_audio_component_get_power(kdev);
>
> /*
> * Enable/disable generating the codec wake signal, overriding the
> @@ -779,7 +780,7 @@ static void
> i915_audio_component_codec_wake_override(struct device *kdev,
> usleep_range(1000, 1500);
> }
>
> - i915_audio_component_put_power(kdev);
> + i915_audio_component_put_power(kdev, wakeref);
> }
>
> /* Get CDCLK in kHz */
> @@ -850,12 +851,13 @@ static int i915_audio_component_sync_audio_rate(struct
> device *kdev, int port,
> struct i915_audio_component *acomp = dev_priv->audio_component;
> struct intel_encoder *encoder;
> struct intel_crtc *crtc;
> + unsigned long wakeref;
> int err = 0;
>
> if (!HAS_DDI(dev_priv))
> return 0;
>
> - i915_audio_component_get_power(kdev);
> + wakeref = i915_audio_component_get_power(kdev);
> mutex_lock(&dev_priv->av_mutex);
>
> /* 1. get the pipe */
> @@ -875,7 +877,7 @@ static int i915_audio_component_sync_audio_rate(struct
> device *kdev, int port,
>
> unlock:
> mutex_unlock(&dev_priv->av_mutex);
> - i915_audio_component_put_power(kdev);
> + i915_audio_component_put_power(kdev, wakeref);
> return err;
> }
>
> diff --git a/include/drm/drm_audio_component.h
> b/include/drm/drm_audio_component.h
> index 4923b00328c1..d0c7444319f5 100644
> --- a/include/drm/drm_audio_component.h
> +++ b/include/drm/drm_audio_component.h
> @@ -18,14 +18,17 @@ struct drm_audio_component_ops {
> * @get_power: get the POWER_DOMAIN_AUDIO power well
> *
> * Request the power well to be turned on.
> + *
> + * Returns a wakeref cookie to be passed back to the corresponding
> + * call to @put_power.
> */
> - void (*get_power)(struct device *);
> + unsigned long (*get_power)(struct device *);
> /**
> * @put_power: put the POWER_DOMAIN_AUDIO power well
> *
> * Allow the power well to be turned off.
> */
> - void (*put_power)(struct device *);
> + void (*put_power)(struct device *, unsigned long);
> /**
> * @codec_wake_override: Enable/disable codec wake signal
> */
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index 45f944d57982..06f504c10b80 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -367,7 +367,7 @@ struct hdac_bus {
> /* DRM component interface */
> struct drm_audio_component *audio_component;
> long display_power_status;
> - bool display_power_active;
> + unsigned long display_power_active;
>
> /* parameters required for enhanced capabilities */
> int num_streams;
> diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c
> index 5c95933e739a..13915fdc6a54 100644
> --- a/sound/hda/hdac_component.c
> +++ b/sound/hda/hdac_component.c
> @@ -79,17 +79,23 @@ void snd_hdac_display_power(struct hdac_bus *bus,
> unsigned int idx, bool enable)
>
> if (bus->display_power_status) {
> if (!bus->display_power_active) {
> + unsigned long cookie = -1;
> +
> if (acomp->ops->get_power)
> - acomp->ops->get_power(acomp->dev);
> + cookie = acomp->ops->get_power(acomp->dev);
> +
> snd_hdac_set_codec_wakeup(bus, true);
> snd_hdac_set_codec_wakeup(bus, false);
> - bus->display_power_active = true;
> + bus->display_power_active = cookie;
> }
> } else {
> if (bus->display_power_active) {
> + unsigned long cookie = bus->display_power_active;
> +
> if (acomp->ops->put_power)
> - acomp->ops->put_power(acomp->dev);
> - bus->display_power_active = false;
> + acomp->ops->put_power(acomp->dev, cookie);
> +
> + bus->display_power_active = 0;
> }
> }
> }
> @@ -325,9 +331,9 @@ int snd_hdac_acomp_exit(struct hdac_bus *bus)
> return 0;
>
> if (WARN_ON(bus->display_power_active) && acomp->ops)
> - acomp->ops->put_power(acomp->dev);
> + acomp->ops->put_power(acomp->dev, bus->display_power_active);
>
> - bus->display_power_active = false;
> + bus->display_power_active = 0;
> bus->display_power_status = 0;
>
> component_master_del(dev, &hdac_component_master_ops);
> --
> 2.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
