| Message ID | 20210621174415.1721198-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda: Release display power reference during shutdown/reboot | expand |
Hi Imre,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on sound/for-next]
[also build test ERROR on v5.13-rc7 next-20210621]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Imre-Deak/ALSA-hda-Release-display-power-reference-during-shutdown-reboot/20210622-014628
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/69033b3b4fd0f59ad73279ee931c5f28e37e6f61
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Imre-Deak/ALSA-hda-Release-display-power-reference-during-shutdown-reboot/20210622-014628
git checkout 69033b3b4fd0f59ad73279ee931c5f28e37e6f61
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=alpha
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
sound/pci/hda/hda_intel.c: In function 'azx_shutdown':
>> sound/pci/hda/hda_intel.c:2388:3: error: implicit declaration of function '__azx_runtime_suspend'; did you mean '__pm_runtime_suspend'? [-Werror=implicit-function-declaration]
2388 | __azx_runtime_suspend(chip);
| ^~~~~~~~~~~~~~~~~~~~~
| __pm_runtime_suspend
cc1: some warnings being treated as errors
vim +2388 sound/pci/hda/hda_intel.c
2378
2379 static void azx_shutdown(struct pci_dev *pci)
2380 {
2381 struct snd_card *card = pci_get_drvdata(pci);
2382 struct azx *chip;
2383
2384 if (!card)
2385 return;
2386 chip = card->private_data;
2387 if (chip && chip->running)
> 2388 __azx_runtime_suspend(chip);
2389 }
2390
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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. I myself have no much preference, but maybe the latter can be easier to be cherry-picked to stable kernels. thanks, Takashi
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?: diff --git a/include/sound/core.h b/include/sound/core.h index c4ade121727df..5228dec658ad6 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -61,6 +61,7 @@ struct snd_device_ops { int (*dev_free)(struct snd_device *dev); int (*dev_register)(struct snd_device *dev); int (*dev_disconnect)(struct snd_device *dev); + void (*dev_shutdown)(struct snd_device *dev); }; struct snd_device { @@ -314,6 +315,7 @@ void snd_device_disconnect(struct snd_card *card, void *device_data); void snd_device_disconnect_all(struct snd_card *card); void snd_device_free(struct snd_card *card, void *device_data); void snd_device_free_all(struct snd_card *card); +void snd_device_shutdown_all(struct snd_card *card); int snd_device_get_state(struct snd_card *card, void *device_data); /* isadma.c */ diff --git a/sound/core/device.c b/sound/core/device.c index bf0b04a7ee797..7c695f8a72f5b 100644 --- a/sound/core/device.c +++ b/sound/core/device.c @@ -238,6 +238,17 @@ void snd_device_free_all(struct snd_card *card) __snd_device_free(dev); } +void snd_device_shutdown_all(struct snd_card *card) +{ + struct snd_device *dev; + + list_for_each_entry_reverse(dev, &card->devices, list) { + if (dev->ops->dev_shutdown) + dev->ops->dev_shutdown(dev); + } +} +EXPORT_SYMBOL_GPL(snd_device_shutdown_all); + /** * snd_device_get_state - Get the current state of the given device * @card: the card instance diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 5462f771c2f90..6da105bc57f58 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -866,6 +866,13 @@ static void snd_hda_codec_dev_release(struct device *dev) kfree(codec); } +static void snd_hda_codec_dev_shutdown(struct snd_device *device) +{ + struct hda_codec *codec = device->device_data; + + codec_display_power(codec, false); +} + #define DEV_NAME_LEN 31 static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card *card, @@ -930,6 +937,7 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, static const struct snd_device_ops dev_ops = { .dev_register = snd_hda_codec_dev_register, .dev_free = snd_hda_codec_dev_free, + .dev_shutdown = snd_hda_codec_dev_shutdown, }; dev_dbg(card->dev, "%s: entry\n", __func__); diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index f5ab0b682adcc..6ca05c6633fc6 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2382,8 +2382,10 @@ static void azx_shutdown(struct pci_dev *pci) if (!card) return; chip = card->private_data; - if (chip && chip->running) + if (chip && chip->running) { + snd_device_shutdown_all(card); azx_shutdown_chip(chip); + } } /* PCI IDs */ > thanks, > > Takashi
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...) \
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...) \
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 7f8f11536a3dc..d0993c22c5e60 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2383,7 +2383,7 @@ static void azx_shutdown(struct pci_dev *pci) return; chip = card->private_data; if (chip && chip->running) - azx_stop_chip(chip); + __azx_runtime_suspend(chip); } /* PCI IDs */
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. Tested on HSW. Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3618 Reported-and-tested-by: Thomas Voegtle <tv@lio96.de> Cc: Takashi Iwai <tiwai@suse.de> Signed-off-by: Imre Deak <imre.deak@intel.com> --- sound/pci/hda/hda_intel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)