[PATCH] snd/hda: Balance hda->need_i915_power across runtime_suspend

[PATCH] snd/hda: Balance hda->need_i915_power across runtime_suspend

[Chris Wilson]

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(-)

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] snd/hda: Balance hda->need_i915_power across runtime_suspend

[Takashi Iwai]

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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for snd/hda: Balance hda->need_i915_power across runtime_suspend

[Patchwork]

== Series Details ==

Series: snd/hda: Balance hda->need_i915_power across runtime_suspend
URL   : https://patchwork.freedesktop.org/series/59253/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5897 -> Patchwork_12746
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12746 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12746, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/59253/revisions/1/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12746:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
    - fi-glk-dsi:         PASS -> FAIL

  
Known issues
------------

  Here are the changes found in Patchwork_12746 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@query-info:
    - fi-glk-dsi:         NOTRUN -> SKIP [fdo#109271] +17

  * igt@amdgpu/amd_basic@userptr:
    - fi-kbl-8809g:       PASS -> DMESG-WARN [fdo#108965]

  * igt@gem_exec_store@basic-bsd1:
    - fi-kbl-r:           NOTRUN -> SKIP [fdo#109271] +41

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         PASS -> INCOMPLETE [fdo#103927] / [fdo#109720]

  * igt@i915_selftest@live_hangcheck:
    - fi-bxt-dsi:         PASS -> INCOMPLETE [fdo#103927]

  * igt@kms_busy@basic-flip-a:
    - fi-bsw-n3050:       NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1

  * igt@kms_busy@basic-flip-c:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] / [fdo#109278]
    - fi-byt-j1900:       NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_chamelium@hdmi-crc-fast:
    - fi-bsw-n3050:       NOTRUN -> SKIP [fdo#109271] +62
    - fi-byt-j1900:       NOTRUN -> SKIP [fdo#109271] +52

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-c:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +48

  * igt@runner@aborted:
    - fi-apl-guc:         NOTRUN -> FAIL [fdo#108622] / [fdo#109720]

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  * igt@i915_pm_rpm@module-reload:
    - fi-glk-dsi:         INCOMPLETE [fdo#103359] / [k.org#198133] -> PASS

  * igt@i915_selftest@live_contexts:
    - fi-bdw-gvtdvm:      DMESG-FAIL [fdo#110235 ] -> PASS
    - fi-skl-gvtdvm:      DMESG-FAIL [fdo#110235 ] -> PASS

  
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
  [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720
  [fdo#110235 ]: https://bugs.freedesktop.org/show_bug.cgi?id=110235 
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (44 -> 39)
------------------------------

  Additional (3): fi-byt-j1900 fi-kbl-r fi-bsw-n3050 
  Missing    (8): fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-kbl-7500u fi-icl-u3 fi-icl-y fi-bdw-samus fi-snb-2600 


Build changes
-------------

    * Linux: CI_DRM_5897 -> Patchwork_12746

  CI_DRM_5897: 7d07e025e78603d6270bc115fdb6c1efea6e66a5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4934: dc4f45eb6874331daec870dc1e4cfc3ac5c49311 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12746: ded06fca89e4d1bd756e47f9cfdfe798e4ff7977 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

ded06fca89e4 snd/hda: Balance hda->need_i915_power across runtime_suspend

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12746/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] snd/hda: Balance hda->need_i915_power across runtime_suspend

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] snd/hda: Balance hda->need_i915_power across runtime_suspend

[Takashi Iwai]

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;
 		}
 	}
 }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for snd/hda: Balance hda->need_i915_power across runtime_suspend (rev2)

[Patchwork]

== Series Details ==

Series: snd/hda: Balance hda->need_i915_power across runtime_suspend (rev2)
URL   : https://patchwork.freedesktop.org/series/59253/
State : failure

== Summary ==

Applying: snd/hda: Balance hda->need_i915_power across runtime_suspend
error: sha1 information is lacking or useless (sound/hda/hdac_component.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 snd/hda: Balance hda->need_i915_power across runtime_suspend
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] snd/hda: Balance hda->need_i915_power across runtime_suspend

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] snd/hda: Balance hda->need_i915_power across runtime_suspend

[Takashi Iwai]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx