snd/hda: Balance hda->need_i915_power across runtime_suspend

Submitted by Takashi Iwai on April 10, 2019, 5:29 a.m.

Details

Message ID s5hpnpu2zto.wl-tiwai@suse.de
State New
Headers show
Series "snd/hda: Balance hda->need_i915_power across runtime_suspend" ( rev: 2 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Takashi Iwai April 10, 2019, 5:29 a.m.
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

Patch hide | download patch | download mbox

--- 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;
 		}
 	}
 }

Comments

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