| Message ID | 20190213152109.16997-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | snd/hda, drm/i915: Track the display_power_status using a cookie | expand |
On Wed, 13 Feb 2019 16:21:09 +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. > > v2: Keep using the power abstraction for local wakeref tracking. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Takashi Iwai <tiwai@suse.de> > Cc: Jani Nikula <jani.nikula@intel.com> Feel free to take my ack: Reviewed-by: Takashi Iwai <tiwai@suse.de> Or let me know if you guys want to apply this through sound tree for 5.1 merge. thanks, Takashi
Quoting Takashi Iwai (2019-02-13 21:48:50) > On Wed, 13 Feb 2019 16:21:09 +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. > > > > v2: Keep using the power abstraction for local wakeref tracking. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Takashi Iwai <tiwai@suse.de> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Feel free to take my ack: > Reviewed-by: Takashi Iwai <tiwai@suse.de> > > Or let me know if you guys want to apply this through sound tree for > 5.1 merge. It's a debug featurette so it's not urgent in any way; it's just for identifying bugs in CI. I think we're good to go in through drm-intel-next-queued for 5.2 -Chris
Chris Wilson <chris@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 <chris@chris-wilson.co.uk> > Cc: Takashi Iwai <tiwai@suse.de> > Cc: Jani Nikula <jani.nikula@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.kuoppala@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
Quoting Mika Kuoppala (2019-02-14 13:14:08) > Chris Wilson <chris@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 <chris@chris-wilson.co.uk> > > Cc: Takashi Iwai <tiwai@suse.de> > > Cc: Jani Nikula <jani.nikula@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. Turns out depot_stack_handle_t (aka intel_wakeref_t) is just a u32, so I went with your suggestion of sizeof(unsigned long) < sizeof(intel_wakeref_t). Thank, -Chris
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) { - 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);
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 <chris@chris-wilson.co.uk> Cc: Takashi Iwai <tiwai@suse.de> Cc: Jani Nikula <jani.nikula@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(-)