Re: [Intel-gfx] [PATCH] ALSA: hda: Release display power reference during shutdown/reboot

[Takashi Iwai <tiwai@suse.de>]

On Tue, 22 Jun 2021 21:58:13 +0200,
Imre Deak wrote:
> 
> On Tue, Jun 22, 2021 at 04:18:14PM +0200, Takashi Iwai wrote:
> > On Mon, 21 Jun 2021 19:44:15 +0200,
> > Imre Deak wrote:
> > > 
> > > Make sure the HDA driver's display power reference is released during
> > > shutdown/reboot.
> > > 
> > > During the shutdown/reboot sequence the pci device core calls the
> > > pm_runtime_resume handler for all devices before calling the driver's
> > > shutdown callback and so the HDA driver's runtime resume callback will
> > > acquire a display power reference (on HSW/BDW). This triggers a power
> > > reference held WARN on HSW/BDW in the i915 driver's subsequent shutdown
> > > handler, which expects all display power references to be released by
> > > that time.
> > > 
> > > Since the HDA controller is stopped in the shutdown handler in any case,
> > > let's follow here the same sequence as the one during runtime suspend.
> > > This will also reset the HDA link and drop the display power reference,
> > > getting rid of the above WARN.
> > 
> > As kbuild bot suggested, __azx_runtime_suspend() is defined only with
> > CONFIG_PM.  We need either moving the function out of ifdef CONFIG_PM
> > block, or having CONFIG_PM conditional call there.
> 
> Thanks, missed that. I think we need to drop the power ref in the !CONFIG_PM
> case as well (since AFAICS then the ref is held after the device is probed), so
> I'd move __azx_runtime_suspend() out of the CONFIG_PM block (and perhaps rename
> it to azx_shutdown_chip).
> 
> > I myself have no much preference,  but maybe the latter can be easier
> > to be cherry-picked to stable kernels.
> 
> To my knowledge this only fixes the book-keeping in the i915 driver, so
> not sure if it's a stable material.
> 
> Trying things now with !CONFIG_PM, I noticed that the HDA codec would still
> keep a separate power reference (which was dropped for me with CONFIG_PM, as
> the codec was runtime suspended). To fix that we'd need something like the
> following (on top of the above changes in a separate patch), any
> comments on it?:

Adding the common dev_shutdown sounds a bit like overkill.
Since it's just a missing shutdown handling in the hd-audio codec
side, does the patch like below work instead?

If this works, feel free to fold into your patch.


thanks,

Takashi

---
diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c
index 17a25e453f60..e8dee24c309d 100644
--- a/sound/pci/hda/hda_bind.c
+++ b/sound/pci/hda/hda_bind.c
@@ -167,8 +167,11 @@ static void hda_codec_driver_shutdown(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
 
-	if (!pm_runtime_suspended(dev) && codec->patch_ops.reboot_notify)
-		codec->patch_ops.reboot_notify(codec);
+	if (!pm_runtime_suspended(dev)) {
+		if (codec->patch_ops.reboot_notify)
+			codec->patch_ops.reboot_notify(codec);
+		snd_hda_codec_display_power(codec, false);
+	}
 }
 
 int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 5462f771c2f9..7a717e151156 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -798,7 +798,7 @@ static unsigned int hda_set_power_state(struct hda_codec *codec,
 				unsigned int power_state);
 
 /* enable/disable display power per codec */
-static void codec_display_power(struct hda_codec *codec, bool enable)
+void snd_hda_codec_display_power(struct hda_codec *codec, bool enable)
 {
 	if (codec->display_power_control)
 		snd_hdac_display_power(&codec->bus->core, codec->addr, enable);
@@ -810,7 +810,7 @@ void snd_hda_codec_register(struct hda_codec *codec)
 	if (codec->registered)
 		return;
 	if (device_is_registered(hda_codec_dev(codec))) {
-		codec_display_power(codec, true);
+		snd_hda_codec_display_power(codec, true);
 		pm_runtime_enable(hda_codec_dev(codec));
 		/* it was powered up in snd_hda_codec_new(), now all done */
 		snd_hda_power_down(codec);
@@ -836,7 +836,7 @@ static int snd_hda_codec_dev_free(struct snd_device *device)
 	 */
 	if (codec->core.type == HDA_DEV_LEGACY)
 		snd_hdac_device_unregister(&codec->core);
-	codec_display_power(codec, false);
+	snd_hda_codec_display_power(codec, false);
 
 	/*
 	 * In the case of ASoC HD-audio bus, the device refcount is released in
@@ -2893,7 +2893,7 @@ static int hda_codec_runtime_suspend(struct device *dev)
 	    (codec_has_clkstop(codec) && codec_has_epss(codec) &&
 	     (state & AC_PWRST_CLK_STOP_OK)))
 		snd_hdac_codec_link_down(&codec->core);
-	codec_display_power(codec, false);
+	snd_hda_codec_display_power(codec, false);
 	return 0;
 }
 
@@ -2905,7 +2905,7 @@ static int hda_codec_runtime_resume(struct device *dev)
 	if (!codec->card)
 		return 0;
 
-	codec_display_power(codec, true);
+	snd_hda_codec_display_power(codec, true);
 	snd_hdac_codec_link_up(&codec->core);
 	hda_call_codec_resume(codec);
 	pm_runtime_mark_last_busy(dev);
diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
index 4c5589c10f1d..8d2503e8dad8 100644
--- a/sound/pci/hda/hda_local.h
+++ b/sound/pci/hda/hda_local.h
@@ -709,6 +709,8 @@ void snd_hdmi_write_eld_info(struct hdmi_eld *eld,
 #define SND_PRINT_CHANNEL_ALLOCATION_ADVISED_BUFSIZE 80
 void snd_print_channel_allocation(int spk_alloc, char *buf, int buflen);
 
+void snd_hda_codec_display_power(struct hda_codec *codec, bool enable);
+
 /*
  */
 #define codec_err(codec, fmt, args...) \
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx