On Mon, 14 Jan 2019 18:37:52 +0100,
Chris Wilson wrote:
> 
> 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.
> 
> 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 | 10 +++++-----
>  include/drm/drm_audio_component.h  |  7 +++++--
>  include/sound/hdaudio.h            |  4 ++--
>  sound/hda/hdac_component.c         | 18 ++++++++++++------
>  4 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c 
> b/drivers/gpu/drm/i915/intel_audio.c
> index 92e27359c2e3..efc5fb3c2161 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -741,15 +741,15 @@ 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)
>  {
> -     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,
> 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.

Better to explain that the cookie should be non-zero for active
state.


>        */
> -     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 b4fa1c775251..39761120ee76 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -366,8 +366,8 @@ struct hdac_bus {
>  
>       /* DRM component interface */
>       struct drm_audio_component *audio_component;
> -     long display_power_status;
> -     bool display_power_active;
> +     unsigned long display_power_status;

You don't need to change this field.  It's irrelevant with this patch
itself.


thanks,

Takashi


> +     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 a6d37b9d6413..2702548b788a 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

Reply via email to