[alsa-devel] [PATCH 1/3] drm/i915/audio: Track temporary rpm wakerefs
Track the temporary rpm wakerefs used within audio so that they may be marked as complete and the tracking cancelled upon release.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/intel_audio.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index de26cd0a5497..92e27359c2e3 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -756,12 +756,13 @@ static void i915_audio_component_codec_wake_override(struct device *kdev, bool enable) { struct drm_i915_private *dev_priv = kdev_to_i915(kdev); + intel_wakeref_t wakeref; u32 tmp;
if (!IS_GEN(dev_priv, 9)) return;
- i915_audio_component_get_power(kdev); + wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
/* * 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); + intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO, 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; + intel_wakeref_t wakeref; int err = 0;
if (!HAS_DDI(dev_priv)) return 0;
- i915_audio_component_get_power(kdev); + wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO); 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); + intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO, wakeref); return err; }
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 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 | 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. */ - 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; + 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);
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 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 | 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;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
Quoting Takashi Iwai (2019-01-14 17:54:08)
On Mon, 14 Jan 2019 18:37:52 +0100, Chris Wilson wrote:
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.
It kept the length of the two variables the same, while matching the type as used with set_bit/clear_bit :) -Chris
Just in case the audio linkage is swapped between components during the runtime pm sequence, we need to protect the rpm tracking with a mutex.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Takashi Iwai tiwai@suse.de Cc: Jani Nikula jani.nikula@intel.com --- include/sound/hdaudio.h | 1 + sound/hda/hdac_component.c | 7 +++++++ 2 files changed, 8 insertions(+)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 39761120ee76..497335b24e18 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -368,6 +368,7 @@ struct hdac_bus { struct drm_audio_component *audio_component; unsigned long display_power_status; unsigned long display_power_active; + struct mutex display_power_lock;
/* parameters required for enhanced capabilities */ int num_streams; diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c index 2702548b788a..ea76c1de2927 100644 --- a/sound/hda/hdac_component.c +++ b/sound/hda/hdac_component.c @@ -77,6 +77,7 @@ void snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable) if (!acomp || !acomp->ops) return;
+ mutex_lock(&bus->display_power_lock); if (bus->display_power_status) { if (!bus->display_power_active) { unsigned long cookie = -1; @@ -98,6 +99,7 @@ void snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable) bus->display_power_active = 0; } } + mutex_unlock(&bus->display_power_lock); } EXPORT_SYMBOL_GPL(snd_hdac_display_power);
@@ -290,6 +292,9 @@ int snd_hdac_acomp_init(struct hdac_bus *bus, GFP_KERNEL); if (!acomp) return -ENOMEM; + + mutex_init(&bus->display_power_lock); + acomp->audio_ops = aops; bus->audio_component = acomp; devres_add(dev, acomp); @@ -336,6 +341,8 @@ int snd_hdac_acomp_exit(struct hdac_bus *bus) bus->display_power_active = 0; bus->display_power_status = 0;
+ mutex_destroy(&bus->display_power_lock); + component_master_del(dev, &hdac_component_master_ops);
bus->audio_component = NULL;
On Mon, 14 Jan 2019 18:37:53 +0100, Chris Wilson wrote:
Just in case the audio linkage is swapped between components during the runtime pm sequence, we need to protect the rpm tracking with a mutex.
It's not clear to me how does this happens. Could you elaborate a bit more the scenario?
thanks,
Takashi
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Takashi Iwai tiwai@suse.de Cc: Jani Nikula jani.nikula@intel.com
include/sound/hdaudio.h | 1 + sound/hda/hdac_component.c | 7 +++++++ 2 files changed, 8 insertions(+)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 39761120ee76..497335b24e18 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -368,6 +368,7 @@ struct hdac_bus { struct drm_audio_component *audio_component; unsigned long display_power_status; unsigned long display_power_active;
struct mutex display_power_lock;
/* parameters required for enhanced capabilities */ int num_streams;
diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c index 2702548b788a..ea76c1de2927 100644 --- a/sound/hda/hdac_component.c +++ b/sound/hda/hdac_component.c @@ -77,6 +77,7 @@ void snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable) if (!acomp || !acomp->ops) return;
- mutex_lock(&bus->display_power_lock); if (bus->display_power_status) { if (!bus->display_power_active) { unsigned long cookie = -1;
@@ -98,6 +99,7 @@ void snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable) bus->display_power_active = 0; } }
- mutex_unlock(&bus->display_power_lock);
} EXPORT_SYMBOL_GPL(snd_hdac_display_power);
@@ -290,6 +292,9 @@ int snd_hdac_acomp_init(struct hdac_bus *bus, GFP_KERNEL); if (!acomp) return -ENOMEM;
- mutex_init(&bus->display_power_lock);
- acomp->audio_ops = aops; bus->audio_component = acomp; devres_add(dev, acomp);
@@ -336,6 +341,8 @@ int snd_hdac_acomp_exit(struct hdac_bus *bus) bus->display_power_active = 0; bus->display_power_status = 0;
mutex_destroy(&bus->display_power_lock);
component_master_del(dev, &hdac_component_master_ops);
bus->audio_component = NULL;
-- 2.20.1
Quoting Takashi Iwai (2019-01-14 17:46:57)
On Mon, 14 Jan 2019 18:37:53 +0100, Chris Wilson wrote:
Just in case the audio linkage is swapped between components during the runtime pm sequence, we need to protect the rpm tracking with a mutex.
It's not clear to me how does this happens. Could you elaborate a bit more the scenario?
The code is written such that multiple bits within display_power_status can be set and cleared simultaneously. There was no serialisation mentioned in the routine, so I was fearful that the display_power_active here was being accessed concurrently -- and if that was explaining why snd/hda appears to be leaking the runtime pm (or at least is holding on to the wakeref longer than igt expects, > 10s). -Chris
On Mon, 14 Jan 2019 18:51:31 +0100, Chris Wilson wrote:
Quoting Takashi Iwai (2019-01-14 17:46:57)
On Mon, 14 Jan 2019 18:37:53 +0100, Chris Wilson wrote:
Just in case the audio linkage is swapped between components during the runtime pm sequence, we need to protect the rpm tracking with a mutex.
It's not clear to me how does this happens. Could you elaborate a bit more the scenario?
The code is written such that multiple bits within display_power_status can be set and cleared simultaneously. There was no serialisation mentioned in the routine, so I was fearful that the display_power_active here was being accessed concurrently -- and if that was explaining why snd/hda appears to be leaking the runtime pm (or at least is holding on to the wakeref longer than igt expects, > 10s).
Aha, and does patch actually "fix" the issue...?
It's a simple mutex addition, so no big show, and I'm fine with it, but just wonder whether it really helped.
thanks,
Takashi
Quoting Takashi Iwai (2019-01-14 18:00:02)
On Mon, 14 Jan 2019 18:51:31 +0100, Chris Wilson wrote:
Quoting Takashi Iwai (2019-01-14 17:46:57)
On Mon, 14 Jan 2019 18:37:53 +0100, Chris Wilson wrote:
Just in case the audio linkage is swapped between components during the runtime pm sequence, we need to protect the rpm tracking with a mutex.
It's not clear to me how does this happens. Could you elaborate a bit more the scenario?
The code is written such that multiple bits within display_power_status can be set and cleared simultaneously. There was no serialisation mentioned in the routine, so I was fearful that the display_power_active here was being accessed concurrently -- and if that was explaining why snd/hda appears to be leaking the runtime pm (or at least is holding on to the wakeref longer than igt expects, > 10s).
Aha, and does patch actually "fix" the issue...?
Not sure yet; it's a temperamental issue in CI and my chief suspect is that it's just a misconfiguration of the rpm autosuspend. What may point towards the scenario above is if i915.ko module unloading has any impact on it.
It's a simple mutex addition, so no big show, and I'm fine with it, but just wonder whether it really helped.
Yeah, it just looked suspicious when reviewing the code, will throw it at CI a few times to see if it sticks. -Chris
On Mon, 14 Jan 2019 21:57:15 +0100, Chris Wilson wrote:
Quoting Takashi Iwai (2019-01-14 18:00:02)
On Mon, 14 Jan 2019 18:51:31 +0100, Chris Wilson wrote:
Quoting Takashi Iwai (2019-01-14 17:46:57)
On Mon, 14 Jan 2019 18:37:53 +0100, Chris Wilson wrote:
Just in case the audio linkage is swapped between components during the runtime pm sequence, we need to protect the rpm tracking with a mutex.
It's not clear to me how does this happens. Could you elaborate a bit more the scenario?
The code is written such that multiple bits within display_power_status can be set and cleared simultaneously. There was no serialisation mentioned in the routine, so I was fearful that the display_power_active here was being accessed concurrently -- and if that was explaining why snd/hda appears to be leaking the runtime pm (or at least is holding on to the wakeref longer than igt expects, > 10s).
Aha, and does patch actually "fix" the issue...?
Not sure yet; it's a temperamental issue in CI and my chief suspect is that it's just a misconfiguration of the rpm autosuspend. What may point towards the scenario above is if i915.ko module unloading has any impact on it.
It's a simple mutex addition, so no big show, and I'm fine with it, but just wonder whether it really helped.
Yeah, it just looked suspicious when reviewing the code, will throw it at CI a few times to see if it sticks.
OK, would you re-submit the patchset if it's confirmed to fix the issue? Then we should mark it Cc-to-stable as a real fix, at least.
thanks,
Takashi
On 1/14/2019 09:37, Chris Wilson wrote:
Track the temporary rpm wakerefs used within audio so that they may be marked as complete and the tracking cancelled upon release.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk
drivers/gpu/drm/i915/intel_audio.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index de26cd0a5497..92e27359c2e3 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -756,12 +756,13 @@ static void i915_audio_component_codec_wake_override(struct device *kdev, bool enable) { struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
intel_wakeref_t wakeref; u32 tmp;
if (!IS_GEN(dev_priv, 9)) return;
- i915_audio_component_get_power(kdev);
- wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
Why remove the audio power abstraction from these functions when the next patch is to update the abstraction to use the wakeref interface?
John.
/* * 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);
intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO, 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;
intel_wakeref_t wakeref; int err = 0;
if (!HAS_DDI(dev_priv)) return 0;
- i915_audio_component_get_power(kdev);
wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO); 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);
- intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO, wakeref); return err; }
participants (3)
-
Chris Wilson -
John Harrison -
Takashi Iwai