| Message ID | 20190114173753.472-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915/audio: Track temporary rpm wakerefs | expand |
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
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;
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(+)