| Message ID | 20190409212741.28593-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | snd/hda: Balance hda->need_i915_power across runtime_suspend | expand |
On Tue, 09 Apr 2019 23:27:41 +0200, Chris Wilson wrote: > > In runtime_resume, we release the local display_power wakeref if we can > rely on i915 providing a wakeref along the component. On suspend > therefore, we should only release the display_power if we kept it from > runtime_resume. Hrm, it shouldn't matter. After the recent code rewrite, the control isn't refcount any longer, hence it's fine to call display_power(false) again even if it were already powered off. thanks, Takashi > > Fixes: e454ff8e89b6 ("ALSA: hda/intel: Drop superfluous AZX_DCAPS_I915_POWERWELL checks") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Takashi Iwai <tiwai@suse.de> > Cc: Imre Deak <imre.deak@intel.com> > --- > This appears to fix the glk-dsi as it performs a pm_runtime_suspend in > the middle of azx_probe_contime(). Hopefully. > --- > sound/pci/hda/hda_intel.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > index 2ec91085fa3e..a9faf95c88b5 100644 > --- a/sound/pci/hda/hda_intel.c > +++ b/sound/pci/hda/hda_intel.c > @@ -941,10 +941,14 @@ static bool azx_is_pm_ready(struct snd_card *card) > > static void __azx_runtime_suspend(struct azx *chip) > { > + struct hda_intel *hda = container_of(chip, struct hda_intel, chip); > + > azx_stop_chip(chip); > azx_enter_link_reset(chip); > azx_clear_irq_pending(chip); > - display_power(chip, false); > + > + if (hda->need_i915_power) > + display_power(chip, false); > } > > static void __azx_runtime_resume(struct azx *chip, bool from_rt) > -- > 2.20.1 >
Quoting Takashi Iwai (2019-04-09 22:35:28) > On Tue, 09 Apr 2019 23:27:41 +0200, > Chris Wilson wrote: > > > > In runtime_resume, we release the local display_power wakeref if we can > > rely on i915 providing a wakeref along the component. On suspend > > therefore, we should only release the display_power if we kept it from > > runtime_resume. > > Hrm, it shouldn't matter. After the recent code rewrite, the control > isn't refcount any longer, hence it's fine to call > display_power(false) again even if it were already powered off. That is the puzzle. Have a look at the glk-dsi results, https://patchwork.freedesktop.org/series/59253/ something does appear to go wrong in azx_probe_continue (and seems to be avoided by this patch). Perhaps something like https://patchwork.freedesktop.org/patch/297656/?series=59257&rev=1 if the pm_runtime_autosuspend is occurring from a workqueue at the same time as we call display_power(false). -Chris
On Wed, 10 Apr 2019 00:53:31 +0200, Chris Wilson wrote: > > Quoting Takashi Iwai (2019-04-09 22:35:28) > > On Tue, 09 Apr 2019 23:27:41 +0200, > > Chris Wilson wrote: > > > > > > In runtime_resume, we release the local display_power wakeref if we can > > > rely on i915 providing a wakeref along the component. On suspend > > > therefore, we should only release the display_power if we kept it from > > > runtime_resume. > > > > Hrm, it shouldn't matter. After the recent code rewrite, the control > > isn't refcount any longer, hence it's fine to call > > display_power(false) again even if it were already powered off. > > That is the puzzle. Have a look at the glk-dsi results, > https://patchwork.freedesktop.org/series/59253/ > something does appear to go wrong in azx_probe_continue (and seems to be > avoided by this patch). > > Perhaps something like https://patchwork.freedesktop.org/patch/297656/?series=59257&rev=1 > if the pm_runtime_autosuspend is occurring from a workqueue at the same > time as we call display_power(false). Then how about rather a patch like below? thanks, Takashi --- a/sound/hda/hdac_component.c +++ b/sound/hda/hdac_component.c @@ -78,18 +78,16 @@ void snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable) return; if (bus->display_power_status) { - if (!bus->display_power_active) { + if (!cmpxchg(&bus->display_power_active, false, true)) { if (acomp->ops->get_power) 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; } } else { - if (bus->display_power_active) { + if (cmpxchg(&bus->display_power_active, true, false)) { if (acomp->ops->put_power) acomp->ops->put_power(acomp->dev); - bus->display_power_active = false; } } }
Quoting Takashi Iwai (2019-04-10 06:29:07) > On Wed, 10 Apr 2019 00:53:31 +0200, > Chris Wilson wrote: > > > > Quoting Takashi Iwai (2019-04-09 22:35:28) > > > On Tue, 09 Apr 2019 23:27:41 +0200, > > > Chris Wilson wrote: > > > > > > > > In runtime_resume, we release the local display_power wakeref if we can > > > > rely on i915 providing a wakeref along the component. On suspend > > > > therefore, we should only release the display_power if we kept it from > > > > runtime_resume. > > > > > > Hrm, it shouldn't matter. After the recent code rewrite, the control > > > isn't refcount any longer, hence it's fine to call > > > display_power(false) again even if it were already powered off. > > > > That is the puzzle. Have a look at the glk-dsi results, > > https://patchwork.freedesktop.org/series/59253/ > > something does appear to go wrong in azx_probe_continue (and seems to be > > avoided by this patch). > > > > Perhaps something like https://patchwork.freedesktop.org/patch/297656/?series=59257&rev=1 > > if the pm_runtime_autosuspend is occurring from a workqueue at the same > > time as we call display_power(false). > > Then how about rather a patch like below? > > > thanks, > > Takashi > > --- a/sound/hda/hdac_component.c > +++ b/sound/hda/hdac_component.c > @@ -78,18 +78,16 @@ void snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable) > return; > > if (bus->display_power_status) { > - if (!bus->display_power_active) { > + if (!cmpxchg(&bus->display_power_active, false, true)) { Except that display_power_active is the wakeref cookie returned by get_power to be passed back to put_power. It seems that the cmpxchg is happy so we can conclude this is a race between display_power(false) and pm_runtime_suspend. -Chris
On Wed, 10 Apr 2019 09:59:19 +0200, Chris Wilson wrote: > > Quoting Takashi Iwai (2019-04-10 06:29:07) > > On Wed, 10 Apr 2019 00:53:31 +0200, > > Chris Wilson wrote: > > > > > > Quoting Takashi Iwai (2019-04-09 22:35:28) > > > > On Tue, 09 Apr 2019 23:27:41 +0200, > > > > Chris Wilson wrote: > > > > > > > > > > In runtime_resume, we release the local display_power wakeref if we can > > > > > rely on i915 providing a wakeref along the component. On suspend > > > > > therefore, we should only release the display_power if we kept it from > > > > > runtime_resume. > > > > > > > > Hrm, it shouldn't matter. After the recent code rewrite, the control > > > > isn't refcount any longer, hence it's fine to call > > > > display_power(false) again even if it were already powered off. > > > > > > That is the puzzle. Have a look at the glk-dsi results, > > > https://patchwork.freedesktop.org/series/59253/ > > > something does appear to go wrong in azx_probe_continue (and seems to be > > > avoided by this patch). > > > > > > Perhaps something like https://patchwork.freedesktop.org/patch/297656/?series=59257&rev=1 > > > if the pm_runtime_autosuspend is occurring from a workqueue at the same > > > time as we call display_power(false). > > > > Then how about rather a patch like below? > > > > > > thanks, > > > > Takashi > > > > --- a/sound/hda/hdac_component.c > > +++ b/sound/hda/hdac_component.c > > @@ -78,18 +78,16 @@ void snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable) > > return; > > > > if (bus->display_power_status) { > > - if (!bus->display_power_active) { > > + if (!cmpxchg(&bus->display_power_active, false, true)) { > > Except that display_power_active is the wakeref cookie returned by > get_power to be passed back to put_power. My patch is for 5.1. > It seems that the cmpxchg is happy so we can conclude this is a race > between display_power(false) and pm_runtime_suspend. I believe it worth to fix this in 5.1 at first. Let me submit and queue the fix for 5.1-rc5. thanks, Takashi
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 2ec91085fa3e..a9faf95c88b5 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -941,10 +941,14 @@ static bool azx_is_pm_ready(struct snd_card *card) static void __azx_runtime_suspend(struct azx *chip) { + struct hda_intel *hda = container_of(chip, struct hda_intel, chip); + azx_stop_chip(chip); azx_enter_link_reset(chip); azx_clear_irq_pending(chip); - display_power(chip, false); + + if (hda->need_i915_power) + display_power(chip, false); } static void __azx_runtime_resume(struct azx *chip, bool from_rt)
In runtime_resume, we release the local display_power wakeref if we can rely on i915 providing a wakeref along the component. On suspend therefore, we should only release the display_power if we kept it from runtime_resume. Fixes: e454ff8e89b6 ("ALSA: hda/intel: Drop superfluous AZX_DCAPS_I915_POWERWELL checks") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Takashi Iwai <tiwai@suse.de> Cc: Imre Deak <imre.deak@intel.com> --- This appears to fix the glk-dsi as it performs a pm_runtime_suspend in the middle of azx_probe_contime(). Hopefully. --- sound/pci/hda/hda_intel.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)