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

Wed Jun 23 13:44:55 UTC 2021

On Wed, Jun 23, 2021 at 10:07:21AM +0200, Takashi Iwai wrote:
> 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?

Yes, makes sense, I missed that the codec driver has a shutdown hook
already:/ It also fixes the problem.

I wondered about the the 1. codec -> 2. i915 shutdown hook ordering
guarantee, but I think that's given by the device_link we add between
i915 and the HDA controller dev and the codec dev being located on the
HDA controller's bus.

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

Ok, will send them as separate patches, if they look ok, could you merge
them via the audio tree?

Thanks for the review,
Imre

> 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...) \