| Message ID | 20210812183433.6330-2-vitalyr@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ALSA: hda/cs8409: Prevent pops and clicks during suspend | expand |
On Thu, 12 Aug 2021 20:34:33 +0200, Vitaly Rodionov wrote: > > From: Stefan Binding <sbinding@opensource.cirrus.com> > > During reboot, when the CS42L42 powers down, pops and clicks > may occur due to the codec not being shutdown gracefully. > This can be fixed by going through the suspend sequence, > which shuts down the codec cleanly inside the reboot_notify > hook, which is called on reboot. > > Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> > Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> I hold this one for now, as there is a fix series that deprecates the reboot_notify callback of HD-audio by forcibly doing runtime-suspend at shutdown. Please check the three patches in https://bugzilla.kernel.org/show_bug.cgi?id=214045 I'm going to submit those soon in anyway. thanks, Takashi
On Fri, 13 Aug 2021 08:10:47 +0200, Takashi Iwai wrote: > > On Thu, 12 Aug 2021 20:34:33 +0200, > Vitaly Rodionov wrote: > > > > From: Stefan Binding <sbinding@opensource.cirrus.com> > > > > During reboot, when the CS42L42 powers down, pops and clicks > > may occur due to the codec not being shutdown gracefully. > > This can be fixed by going through the suspend sequence, > > which shuts down the codec cleanly inside the reboot_notify > > hook, which is called on reboot. > > > > Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> > > Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > > I hold this one for now, as there is a fix series that deprecates the > reboot_notify callback of HD-audio by forcibly doing runtime-suspend > at shutdown. Please check the three patches in > https://bugzilla.kernel.org/show_bug.cgi?id=214045 > > I'm going to submit those soon in anyway. The removal of reboot_notifier landed in my for-next branch now. Please rebase and adapt the changes appropriately. In short, the runtime suspend is applied at the shutdown, so the workaround is needed only for suspend. thanks, Takashi
On 14/08/2021 7:41 am, Takashi Iwai wrote: > On Fri, 13 Aug 2021 08:10:47 +0200, > Takashi Iwai wrote: >> On Thu, 12 Aug 2021 20:34:33 +0200, >> Vitaly Rodionov wrote: >>> From: Stefan Binding <sbinding@opensource.cirrus.com> >>> >>> During reboot, when the CS42L42 powers down, pops and clicks >>> may occur due to the codec not being shutdown gracefully. >>> This can be fixed by going through the suspend sequence, >>> which shuts down the codec cleanly inside the reboot_notify >>> hook, which is called on reboot. >>> >>> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> >>> Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> >> I hold this one for now, as there is a fix series that deprecates the >> reboot_notify callback of HD-audio by forcibly doing runtime-suspend >> at shutdown. Please check the three patches in >> https://bugzilla.kernel.org/show_bug.cgi?id=214045 >> >> I'm going to submit those soon in anyway. Hi Takashi, Thanks for letting us know. We have tested against for-next branch and we have an issue. Loud pops on reboot. It looks like suspend have never been called on reboot or shutdown for us. > The removal of reboot_notifier landed in my for-next branch now. > Please rebase and adapt the changes appropriately. In short, the > runtime suspend is applied at the shutdown, so the workaround is > needed only for suspend. > > > thanks, > > Takashi
On Tue, 17 Aug 2021 13:28:21 +0200, Vitaly Rodionov wrote: > > On 14/08/2021 7:41 am, Takashi Iwai wrote: > > On Fri, 13 Aug 2021 08:10:47 +0200, > > Takashi Iwai wrote: > >> On Thu, 12 Aug 2021 20:34:33 +0200, > >> Vitaly Rodionov wrote: > >>> From: Stefan Binding <sbinding@opensource.cirrus.com> > >>> > >>> During reboot, when the CS42L42 powers down, pops and clicks > >>> may occur due to the codec not being shutdown gracefully. > >>> This can be fixed by going through the suspend sequence, > >>> which shuts down the codec cleanly inside the reboot_notify > >>> hook, which is called on reboot. > >>> > >>> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> > >>> Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > >> I hold this one for now, as there is a fix series that deprecates the > >> reboot_notify callback of HD-audio by forcibly doing runtime-suspend > >> at shutdown. Please check the three patches in > >> https://bugzilla.kernel.org/show_bug.cgi?id=214045 > >> > >> I'm going to submit those soon in anyway. > > Hi Takashi, > > Thanks for letting us know. We have tested against for-next branch and > we have an issue. > > Loud pops on reboot. It looks like suspend have never been called on > reboot or shutdown for us. OK, we need to track down the cause. Does the noise persist if the codec has been runtime-suspended beforehand? You can check the status in sysfs. thanks, Takashi
On 17/08/2021 1:16 pm, Takashi Iwai wrote: > On Tue, 17 Aug 2021 13:28:21 +0200, > Vitaly Rodionov wrote: >> On 14/08/2021 7:41 am, Takashi Iwai wrote: >>> On Fri, 13 Aug 2021 08:10:47 +0200, >>> Takashi Iwai wrote: >>>> On Thu, 12 Aug 2021 20:34:33 +0200, >>>> Vitaly Rodionov wrote: >>>>> From: Stefan Binding <sbinding@opensource.cirrus.com> >>>>> >>>>> During reboot, when the CS42L42 powers down, pops and clicks >>>>> may occur due to the codec not being shutdown gracefully. >>>>> This can be fixed by going through the suspend sequence, >>>>> which shuts down the codec cleanly inside the reboot_notify >>>>> hook, which is called on reboot. >>>>> >>>>> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> >>>>> Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> >>>> I hold this one for now, as there is a fix series that deprecates the >>>> reboot_notify callback of HD-audio by forcibly doing runtime-suspend >>>> at shutdown. Please check the three patches in >>>> https://bugzilla.kernel.org/show_bug.cgi?id=214045 >>>> >>>> I'm going to submit those soon in anyway. >> Hi Takashi, >> >> Thanks for letting us know. We have tested against for-next branch and >> we have an issue. >> >> Loud pops on reboot. It looks like suspend have never been called on >> reboot or shutdown for us. > OK, we need to track down the cause. > > Does the noise persist if the codec has been runtime-suspended > beforehand? You can check the status in sysfs. Hi Takashi, Sorry for the delay. We just wanted to get as much information as possible. Now we can see what causing pops on reboot. Actually when codec is suspended and we do reboot from UI, then sometimes we see suspend() calls in kernel log and no pops, but sometimes we still have no suspend() on reboot and we hear pops. But when we do reboot from command line: > sudo reboot then we always have pops and no suspend() called. Then we have added extra logging and we can see that on reboot codec somehow getting resume() call and we run jack detect on resume that causing pops. We were thinking about possible solution for that and we would propose some changes in generic code hda_bind.c: static void hda_codec_driver_shutdown(struct device *dev) { + if (codec->patch_ops.suspend) + codec->patch_ops.suspend(codec); snd_hda_codec_shutdown(dev_to_hda_codec(dev)); + hda_codec_driver_remove(dev_to_hda_codec(dev)); } This have been tested on all our platforms without regression and it fixes pops issue on dolphin HW as well for reboot from UI and > sudo reboot. We always getting suspend() calls on reboot. Thanks, Vitaly > > > thanks, > > Takashi
On Wed, 25 Aug 2021 20:04:05 +0200, Vitaly Rodionov wrote: > Actually when codec is suspended and we do reboot from UI, then sometimes we > see suspend() calls in kernel log and no pops, but sometimes > > we still have no suspend() on reboot and we hear pops. But when we do reboot > from command line: > sudo reboot then we always have pops and no suspend() > called. > > Then we have added extra logging and we can see that on reboot codec somehow > getting resume() call and we run jack detect on resume that causing pops. Hm, it's interesting who triggers the runtime resume. > We were thinking about possible solution for that and we would propose some > changes in generic code hda_bind.c: > > static void hda_codec_driver_shutdown(struct device *dev) { + if (codec-> > patch_ops.suspend) + codec->patch_ops.suspend(codec); > snd_hda_codec_shutdown(dev_to_hda_codec(dev)); + hda_codec_driver_remove > (dev_to_hda_codec(dev)); } Sorry, it's no-no. The suspend can't be called unconditionally, and the driver unbind must not be called in the callback itself. Does the patch below work instead? thanks, Takashi --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2356,8 +2356,11 @@ static void azx_shutdown(struct pci_dev *pci) if (!card) return; chip = card->private_data; - if (chip && chip->running) + if (chip && chip->running) { + chip->bus.shutdown = 1; + cancel_work_sync(&bus->unsol_work); azx_shutdown_chip(chip); + } } /* PCI IDs */
On Thu, 26 Aug 2021 08:03:45 +0200, Takashi Iwai wrote: > > On Wed, 25 Aug 2021 20:04:05 +0200, > Vitaly Rodionov wrote: > > Actually when codec is suspended and we do reboot from UI, then sometimes we > > see suspend() calls in kernel log and no pops, but sometimes > > > > we still have no suspend() on reboot and we hear pops. But when we do reboot > > from command line: > sudo reboot then we always have pops and no suspend() > > called. > > > > Then we have added extra logging and we can see that on reboot codec somehow > > getting resume() call and we run jack detect on resume that causing pops. > > Hm, it's interesting who triggers the runtime resume. > > > We were thinking about possible solution for that and we would propose some > > changes in generic code hda_bind.c: > > > > static void hda_codec_driver_shutdown(struct device *dev) { + if (codec-> > > patch_ops.suspend) + codec->patch_ops.suspend(codec); > > snd_hda_codec_shutdown(dev_to_hda_codec(dev)); + hda_codec_driver_remove > > (dev_to_hda_codec(dev)); } > > Sorry, it's no-no. The suspend can't be called unconditionally, and > the driver unbind must not be called in the callback itself. > > Does the patch below work instead? Sorry there was a typo. A bit more revised patch is below. Takashi --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1383,14 +1383,17 @@ static void azx_free(struct azx *chip) hda->freed = 1; } -static int azx_dev_disconnect(struct snd_device *device) +static void __azx_disconnect(struct azx *chip) { - struct azx *chip = device->device_data; struct hdac_bus *bus = azx_bus(chip); chip->bus.shutdown = 1; cancel_work_sync(&bus->unsol_work); +} +static int azx_dev_disconnect(struct snd_device *device) +{ + __azx_disconnect(device->device_data); return 0; } @@ -2356,8 +2359,10 @@ static void azx_shutdown(struct pci_dev *pci) if (!card) return; chip = card->private_data; - if (chip && chip->running) + if (chip && chip->running) { + __azx_disconnect(chip); azx_shutdown_chip(chip); + } } /* PCI IDs */
On 26/08/2021 11:49 am, Takashi Iwai wrote: > On Thu, 26 Aug 2021 08:03:45 +0200, > Takashi Iwai wrote: >> On Wed, 25 Aug 2021 20:04:05 +0200, >> Vitaly Rodionov wrote: >>> Actually when codec is suspended and we do reboot from UI, then sometimes we >>> see suspend() calls in kernel log and no pops, but sometimes >>> >>> we still have no suspend() on reboot and we hear pops. But when we do reboot >>> from command line: > sudo reboot then we always have pops and no suspend() >>> called. >>> >>> Then we have added extra logging and we can see that on reboot codec somehow >>> getting resume() call and we run jack detect on resume that causing pops. >> Hm, it's interesting who triggers the runtime resume. >> >>> We were thinking about possible solution for that and we would propose some >>> changes in generic code hda_bind.c: >>> >>> static void hda_codec_driver_shutdown(struct device *dev) { + if (codec-> >>> patch_ops.suspend) + codec->patch_ops.suspend(codec); >>> snd_hda_codec_shutdown(dev_to_hda_codec(dev)); + hda_codec_driver_remove >>> (dev_to_hda_codec(dev)); } >> Sorry, it's no-no. The suspend can't be called unconditionally, and >> the driver unbind must not be called in the callback itself. >> >> Does the patch below work instead? > Sorry there was a typo. A bit more revised patch is below. > > > Takashi Hi Takashi, Thanks a lot for quick response. I have tested previous patch and it did not fix an issue, as suspend() was not called. Now I will test new revised patch and let you know ASAP. I am adding some extra logging, unfortunately on reboot these messages are missing from kernel log, however I managed to capture reboot screen and I will attach an image where last messages shown. Thanks, Vitaly > > --- a/sound/pci/hda/hda_intel.c > +++ b/sound/pci/hda/hda_intel.c > @@ -1383,14 +1383,17 @@ static void azx_free(struct azx *chip) > hda->freed = 1; > } > > -static int azx_dev_disconnect(struct snd_device *device) > +static void __azx_disconnect(struct azx *chip) > { > - struct azx *chip = device->device_data; > struct hdac_bus *bus = azx_bus(chip); > > chip->bus.shutdown = 1; > cancel_work_sync(&bus->unsol_work); > +} > > +static int azx_dev_disconnect(struct snd_device *device) > +{ > + __azx_disconnect(device->device_data); > return 0; > } > > @@ -2356,8 +2359,10 @@ static void azx_shutdown(struct pci_dev *pci) > if (!card) > return; > chip = card->private_data; > - if (chip && chip->running) > + if (chip && chip->running) { > + __azx_disconnect(chip); > azx_shutdown_chip(chip); > + } > } > > /* PCI IDs */
On Thu, 26 Aug 2021 13:49:32 +0200, Vitaly Rodionov wrote: > > On 26/08/2021 11:49 am, Takashi Iwai wrote: > > On Thu, 26 Aug 2021 08:03:45 +0200, > > Takashi Iwai wrote: > >> On Wed, 25 Aug 2021 20:04:05 +0200, > >> Vitaly Rodionov wrote: > >>> Actually when codec is suspended and we do reboot from UI, then sometimes we > >>> see suspend() calls in kernel log and no pops, but sometimes > >>> > >>> we still have no suspend() on reboot and we hear pops. But when we do reboot > >>> from command line: > sudo reboot then we always have pops and no suspend() > >>> called. > >>> > >>> Then we have added extra logging and we can see that on reboot codec somehow > >>> getting resume() call and we run jack detect on resume that causing pops. > >> Hm, it's interesting who triggers the runtime resume. > >> > >>> We were thinking about possible solution for that and we would propose some > >>> changes in generic code hda_bind.c: > >>> > >>> static void hda_codec_driver_shutdown(struct device *dev) { + if (codec-> > >>> patch_ops.suspend) + codec->patch_ops.suspend(codec); > >>> snd_hda_codec_shutdown(dev_to_hda_codec(dev)); + hda_codec_driver_remove > >>> (dev_to_hda_codec(dev)); } > >> Sorry, it's no-no. The suspend can't be called unconditionally, and > >> the driver unbind must not be called in the callback itself. > >> > >> Does the patch below work instead? > > Sorry there was a typo. A bit more revised patch is below. > > > > > > Takashi > > > > --- a/sound/pci/hda/hda_intel.c > > +++ b/sound/pci/hda/hda_intel.c > > @@ -1383,14 +1383,17 @@ static void azx_free(struct azx *chip) > > hda->freed = 1; > > } > > -static int azx_dev_disconnect(struct snd_device *device) > > +static void __azx_disconnect(struct azx *chip) > > { > > - struct azx *chip = device->device_data; > > struct hdac_bus *bus = azx_bus(chip); > > chip->bus.shutdown = 1; > > cancel_work_sync(&bus->unsol_work); > > +} > > +static int azx_dev_disconnect(struct snd_device *device) > > +{ > > + __azx_disconnect(device->device_data); > > return 0; > > } > > @@ -2356,8 +2359,10 @@ static void azx_shutdown(struct pci_dev > > *pci) > > if (!card) > > return; > > chip = card->private_data; > > - if (chip && chip->running) > > + if (chip && chip->running) { > > + __azx_disconnect(chip); > > azx_shutdown_chip(chip); > > + } > > } > > /* PCI IDs */ > > Hi Takashi, > > Applied fix and tested on dolphin HW. Issue still there, here is > captured screen on reboot from command line: > > reboot capture > > Reboot from UI works differently, no resume() call in this case. Thanks for quick testing. After reconsideration, I believe we can even take a simpler path. Use pm_runtime_force_suspend(), and keep suspended by pm_runtime_disable() call afterwards. Below is another test patch. Could you check whether this works better? Takashi --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -2986,13 +2986,11 @@ void snd_hda_codec_shutdown(struct hda_codec *codec) { struct hda_pcm *cpcm; - if (pm_runtime_suspended(hda_codec_dev(codec))) - return; - list_for_each_entry(cpcm, &codec->pcm_list_head, list) snd_pcm_suspend_all(cpcm->pcm); - pm_runtime_suspend(hda_codec_dev(codec)); + pm_runtime_force_suspend(hda_codec_dev(codec)); + pm_runtime_disable(hda_codec_dev(codec)); } /*
On 26/08/2021 1:20 pm, Takashi Iwai wrote: > On Thu, 26 Aug 2021 13:49:32 +0200, > Vitaly Rodionov wrote: >> On 26/08/2021 11:49 am, Takashi Iwai wrote: >>> On Thu, 26 Aug 2021 08:03:45 +0200, >>> Takashi Iwai wrote: >>>> On Wed, 25 Aug 2021 20:04:05 +0200, >>>> Vitaly Rodionov wrote: >>>>> Actually when codec is suspended and we do reboot from UI, then sometimes we >>>>> see suspend() calls in kernel log and no pops, but sometimes >>>>> >>>>> we still have no suspend() on reboot and we hear pops. But when we do reboot >>>>> from command line: > sudo reboot then we always have pops and no suspend() >>>>> called. >>>>> >>>>> Then we have added extra logging and we can see that on reboot codec somehow >>>>> getting resume() call and we run jack detect on resume that causing pops. >>>> Hm, it's interesting who triggers the runtime resume. >>>> >>>>> We were thinking about possible solution for that and we would propose some >>>>> changes in generic code hda_bind.c: >>>>> >>>>> static void hda_codec_driver_shutdown(struct device *dev) { + if (codec-> >>>>> patch_ops.suspend) + codec->patch_ops.suspend(codec); >>>>> snd_hda_codec_shutdown(dev_to_hda_codec(dev)); + hda_codec_driver_remove >>>>> (dev_to_hda_codec(dev)); } >>>> Sorry, it's no-no. The suspend can't be called unconditionally, and >>>> the driver unbind must not be called in the callback itself. >>>> >>>> Does the patch below work instead? >>> Sorry there was a typo. A bit more revised patch is below. >>> >>> >>> Takashi >>> >>> --- a/sound/pci/hda/hda_intel.c >>> +++ b/sound/pci/hda/hda_intel.c >>> @@ -1383,14 +1383,17 @@ static void azx_free(struct azx *chip) >>> hda->freed = 1; >>> } >>> -static int azx_dev_disconnect(struct snd_device *device) >>> +static void __azx_disconnect(struct azx *chip) >>> { >>> - struct azx *chip = device->device_data; >>> struct hdac_bus *bus = azx_bus(chip); >>> chip->bus.shutdown = 1; >>> cancel_work_sync(&bus->unsol_work); >>> +} >>> +static int azx_dev_disconnect(struct snd_device *device) >>> +{ >>> + __azx_disconnect(device->device_data); >>> return 0; >>> } >>> @@ -2356,8 +2359,10 @@ static void azx_shutdown(struct pci_dev >>> *pci) >>> if (!card) >>> return; >>> chip = card->private_data; >>> - if (chip && chip->running) >>> + if (chip && chip->running) { >>> + __azx_disconnect(chip); >>> azx_shutdown_chip(chip); >>> + } >>> } >>> /* PCI IDs */ >> Hi Takashi, >> >> Applied fix and tested on dolphin HW. Issue still there, here is >> captured screen on reboot from command line: >> >> reboot capture >> >> Reboot from UI works differently, no resume() call in this case. > Thanks for quick testing. > > After reconsideration, I believe we can even take a simpler path. > Use pm_runtime_force_suspend(), and keep suspended by > pm_runtime_disable() call afterwards. > > Below is another test patch. Could you check whether this works > better? > > > Takashi > > --- a/sound/pci/hda/hda_codec.c > +++ b/sound/pci/hda/hda_codec.c > @@ -2986,13 +2986,11 @@ void snd_hda_codec_shutdown(struct hda_codec *codec) > { > struct hda_pcm *cpcm; > > - if (pm_runtime_suspended(hda_codec_dev(codec))) > - return; > - > list_for_each_entry(cpcm, &codec->pcm_list_head, list) > snd_pcm_suspend_all(cpcm->pcm); > > - pm_runtime_suspend(hda_codec_dev(codec)); > + pm_runtime_force_suspend(hda_codec_dev(codec)); > + pm_runtime_disable(hda_codec_dev(codec)); > } > > /* Hi Takashi, Thank you very much! Yes, that works fine. suspend() has been called on reboot and no pops. I still have previous patch in the code, so let me remove it and test it again with only snd_hda_codec_shutdown() changes. Thanks, Vitaly
On 26/08/2021 2:00 pm, Vitaly Rodionov wrote: > On 26/08/2021 1:20 pm, Takashi Iwai wrote: >> On Thu, 26 Aug 2021 13:49:32 +0200, >> Vitaly Rodionov wrote: >>> On 26/08/2021 11:49 am, Takashi Iwai wrote: >>>> On Thu, 26 Aug 2021 08:03:45 +0200, >>>> Takashi Iwai wrote: >>>>> On Wed, 25 Aug 2021 20:04:05 +0200, >>>>> Vitaly Rodionov wrote: >>>>>> Actually when codec is suspended and we do reboot from UI, then >>>>>> sometimes we >>>>>> see suspend() calls in kernel log and no pops, but sometimes >>>>>> >>>>>> we still have no suspend() on reboot and we hear pops. But when >>>>>> we do reboot >>>>>> from command line: > sudo reboot then we always have pops and no >>>>>> suspend() >>>>>> called. >>>>>> >>>>>> Then we have added extra logging and we can see that on reboot >>>>>> codec somehow >>>>>> getting resume() call and we run jack detect on resume that >>>>>> causing pops. >>>>> Hm, it's interesting who triggers the runtime resume. >>>>> >>>>>> We were thinking about possible solution for that and we would >>>>>> propose some >>>>>> changes in generic code hda_bind.c: >>>>>> >>>>>> static void hda_codec_driver_shutdown(struct device *dev) { + >>>>>> if (codec-> >>>>>> patch_ops.suspend) + codec->patch_ops.suspend(codec); >>>>>> snd_hda_codec_shutdown(dev_to_hda_codec(dev)); + >>>>>> hda_codec_driver_remove >>>>>> (dev_to_hda_codec(dev)); } >>>>> Sorry, it's no-no. The suspend can't be called unconditionally, and >>>>> the driver unbind must not be called in the callback itself. >>>>> >>>>> Does the patch below work instead? >>>> Sorry there was a typo. A bit more revised patch is below. >>>> >>>> >>>> Takashi >>>> >>>> --- a/sound/pci/hda/hda_intel.c >>>> +++ b/sound/pci/hda/hda_intel.c >>>> @@ -1383,14 +1383,17 @@ static void azx_free(struct azx *chip) >>>> hda->freed = 1; >>>> } >>>> -static int azx_dev_disconnect(struct snd_device *device) >>>> +static void __azx_disconnect(struct azx *chip) >>>> { >>>> - struct azx *chip = device->device_data; >>>> struct hdac_bus *bus = azx_bus(chip); >>>> chip->bus.shutdown = 1; >>>> cancel_work_sync(&bus->unsol_work); >>>> +} >>>> +static int azx_dev_disconnect(struct snd_device *device) >>>> +{ >>>> + __azx_disconnect(device->device_data); >>>> return 0; >>>> } >>>> @@ -2356,8 +2359,10 @@ static void azx_shutdown(struct pci_dev >>>> *pci) >>>> if (!card) >>>> return; >>>> chip = card->private_data; >>>> - if (chip && chip->running) >>>> + if (chip && chip->running) { >>>> + __azx_disconnect(chip); >>>> azx_shutdown_chip(chip); >>>> + } >>>> } >>>> /* PCI IDs */ >>> Hi Takashi, >>> >>> Applied fix and tested on dolphin HW. Issue still there, here is >>> captured screen on reboot from command line: >>> >>> reboot capture >>> >>> Reboot from UI works differently, no resume() call in this case. >> Thanks for quick testing. >> >> After reconsideration, I believe we can even take a simpler path. >> Use pm_runtime_force_suspend(), and keep suspended by >> pm_runtime_disable() call afterwards. >> >> Below is another test patch. Could you check whether this works >> better? >> >> >> Takashi >> >> --- a/sound/pci/hda/hda_codec.c >> +++ b/sound/pci/hda/hda_codec.c >> @@ -2986,13 +2986,11 @@ void snd_hda_codec_shutdown(struct hda_codec >> *codec) >> { >> struct hda_pcm *cpcm; >> - if (pm_runtime_suspended(hda_codec_dev(codec))) >> - return; >> - >> list_for_each_entry(cpcm, &codec->pcm_list_head, list) >> snd_pcm_suspend_all(cpcm->pcm); >> - pm_runtime_suspend(hda_codec_dev(codec)); >> + pm_runtime_force_suspend(hda_codec_dev(codec)); >> + pm_runtime_disable(hda_codec_dev(codec)); >> } >> /* > > Hi Takashi, > > Thank you very much! Yes, that works fine. suspend() has been called > on reboot and no pops. > > I still have previous patch in the code, so let me remove it and test > it again with only snd_hda_codec_shutdown() changes. > > Thanks, > > Vitaly > > Hi Takashi, I have finished regression tests on all our platforms and results are positive, latest patch has fixed an issue with pops on reboot and no regression on other platforms as well. Thanks, Vitaly
On Thu, 26 Aug 2021 17:14:31 +0200, Vitaly Rodionov wrote: > > On 26/08/2021 2:00 pm, Vitaly Rodionov wrote: > > On 26/08/2021 1:20 pm, Takashi Iwai wrote: > >> On Thu, 26 Aug 2021 13:49:32 +0200, > >> Vitaly Rodionov wrote: > >>> On 26/08/2021 11:49 am, Takashi Iwai wrote: > >>>> On Thu, 26 Aug 2021 08:03:45 +0200, > >>>> Takashi Iwai wrote: > >>>>> On Wed, 25 Aug 2021 20:04:05 +0200, > >>>>> Vitaly Rodionov wrote: > >>>>>> Actually when codec is suspended and we do reboot from UI, then > >>>>>> sometimes we > >>>>>> see suspend() calls in kernel log and no pops, but sometimes > >>>>>> > >>>>>> we still have no suspend() on reboot and we hear pops. But when > >>>>>> we do reboot > >>>>>> from command line: > sudo reboot then we always have pops and > >>>>>> no suspend() > >>>>>> called. > >>>>>> > >>>>>> Then we have added extra logging and we can see that on reboot > >>>>>> codec somehow > >>>>>> getting resume() call and we run jack detect on resume that > >>>>>> causing pops. > >>>>> Hm, it's interesting who triggers the runtime resume. > >>>>> > >>>>>> We were thinking about possible solution for that and we would > >>>>>> propose some > >>>>>> changes in generic code hda_bind.c: > >>>>>> > >>>>>> static void hda_codec_driver_shutdown(struct device *dev) { + > >>>>>> if (codec-> > >>>>>> patch_ops.suspend) + codec->patch_ops.suspend(codec); > >>>>>> snd_hda_codec_shutdown(dev_to_hda_codec(dev)); + > >>>>>> hda_codec_driver_remove > >>>>>> (dev_to_hda_codec(dev)); } > >>>>> Sorry, it's no-no. The suspend can't be called unconditionally, and > >>>>> the driver unbind must not be called in the callback itself. > >>>>> > >>>>> Does the patch below work instead? > >>>> Sorry there was a typo. A bit more revised patch is below. > >>>> > >>>> > >>>> Takashi > >>>> > >>>> --- a/sound/pci/hda/hda_intel.c > >>>> +++ b/sound/pci/hda/hda_intel.c > >>>> @@ -1383,14 +1383,17 @@ static void azx_free(struct azx *chip) > >>>> hda->freed = 1; > >>>> } > >>>> -static int azx_dev_disconnect(struct snd_device *device) > >>>> +static void __azx_disconnect(struct azx *chip) > >>>> { > >>>> - struct azx *chip = device->device_data; > >>>> struct hdac_bus *bus = azx_bus(chip); > >>>> chip->bus.shutdown = 1; > >>>> cancel_work_sync(&bus->unsol_work); > >>>> +} > >>>> +static int azx_dev_disconnect(struct snd_device *device) > >>>> +{ > >>>> + __azx_disconnect(device->device_data); > >>>> return 0; > >>>> } > >>>> @@ -2356,8 +2359,10 @@ static void azx_shutdown(struct pci_dev > >>>> *pci) > >>>> if (!card) > >>>> return; > >>>> chip = card->private_data; > >>>> - if (chip && chip->running) > >>>> + if (chip && chip->running) { > >>>> + __azx_disconnect(chip); > >>>> azx_shutdown_chip(chip); > >>>> + } > >>>> } > >>>> /* PCI IDs */ > >>> Hi Takashi, > >>> > >>> Applied fix and tested on dolphin HW. Issue still there, here is > >>> captured screen on reboot from command line: > >>> > >>> reboot capture > >>> > >>> Reboot from UI works differently, no resume() call in this case. > >> Thanks for quick testing. > >> > >> After reconsideration, I believe we can even take a simpler path. > >> Use pm_runtime_force_suspend(), and keep suspended by > >> pm_runtime_disable() call afterwards. > >> > >> Below is another test patch. Could you check whether this works > >> better? > >> > >> > >> Takashi > >> > >> --- a/sound/pci/hda/hda_codec.c > >> +++ b/sound/pci/hda/hda_codec.c > >> @@ -2986,13 +2986,11 @@ void snd_hda_codec_shutdown(struct > >> hda_codec *codec) > >> { > >> struct hda_pcm *cpcm; > >> - if (pm_runtime_suspended(hda_codec_dev(codec))) > >> - return; > >> - > >> list_for_each_entry(cpcm, &codec->pcm_list_head, list) > >> snd_pcm_suspend_all(cpcm->pcm); > >> - pm_runtime_suspend(hda_codec_dev(codec)); > >> + pm_runtime_force_suspend(hda_codec_dev(codec)); > >> + pm_runtime_disable(hda_codec_dev(codec)); > >> } > >> /* > > > > Hi Takashi, > > > > Thank you very much! Yes, that works fine. suspend() has been > > called on reboot and no pops. > > > > I still have previous patch in the code, so let me remove it and > > test it again with only snd_hda_codec_shutdown() changes. > > > > Thanks, > > > > Vitaly > > > > > Hi Takashi, > > I have finished regression tests on all our platforms and results are > positive, latest patch has fixed an issue with pops on reboot and no > > regression on other platforms as well. Great, thanks for testing again! I'll submit and merge the patch with your reported-and-tested-by tag. Takashi
diff --git a/sound/pci/hda/patch_cs8409.c b/sound/pci/hda/patch_cs8409.c index 9db16b6292f4..f51fc4a1545a 100644 --- a/sound/pci/hda/patch_cs8409.c +++ b/sound/pci/hda/patch_cs8409.c @@ -753,7 +753,6 @@ static void cs42l42_resume(struct sub_codec *cs42l42) cs42l42_enable_jack_detect(cs42l42); } -#ifdef CONFIG_PM static void cs42l42_suspend(struct sub_codec *cs42l42) { struct hda_codec *codec = cs42l42->codec; @@ -773,6 +772,9 @@ static void cs42l42_suspend(struct sub_codec *cs42l42) { 0x1101, 0xFF }, }; + if (cs42l42->suspended) + return; + cs8409_i2c_bulk_write(cs42l42, cs42l42_pwr_down_seq, ARRAY_SIZE(cs42l42_pwr_down_seq)); if (read_poll_timeout(cs8409_i2c_read, reg_cdc_status, @@ -790,7 +792,6 @@ static void cs42l42_suspend(struct sub_codec *cs42l42) gpio_data &= ~cs42l42->reset_gpio; snd_hda_codec_write(codec, CS8409_PIN_AFG, 0, AC_VERB_SET_GPIO_DATA, gpio_data); } -#endif static void cs8409_free(struct hda_codec *codec) { @@ -803,6 +804,33 @@ static void cs8409_free(struct hda_codec *codec) snd_hda_gen_free(codec); } +/* Manage PDREF, when transition to D3hot */ +static int cs8409_cs42l42_suspend(struct hda_codec *codec) +{ + struct cs8409_spec *spec = codec->spec; + int i; + + cs8409_enable_ur(codec, 0); + + for (i = 0; i < spec->num_scodecs; i++) + cs42l42_suspend(spec->scodecs[i]); + + /* Cancel i2c clock disable timer, and disable clock if left enabled */ + cancel_delayed_work_sync(&spec->i2c_clk_work); + cs8409_disable_i2c_clock(codec); + + snd_hda_shutup_pins(codec); + + return 0; +} + +static void cs8409_reboot_notify(struct hda_codec *codec) +{ + cs8409_cs42l42_suspend(codec); + snd_hda_gen_reboot_notify(codec); + codec->patch_ops.free(codec); +} + /****************************************************************************** * BULLSEYE / WARLOCK / CYBORG Specific Functions * CS8409/CS42L42 @@ -845,28 +873,6 @@ static void cs8409_cs42l42_jack_unsol_event(struct hda_codec *codec, unsigned in } } -#ifdef CONFIG_PM -/* Manage PDREF, when transition to D3hot */ -static int cs8409_cs42l42_suspend(struct hda_codec *codec) -{ - struct cs8409_spec *spec = codec->spec; - int i; - - cs8409_enable_ur(codec, 0); - - for (i = 0; i < spec->num_scodecs; i++) - cs42l42_suspend(spec->scodecs[i]); - - /* Cancel i2c clock disable timer, and disable clock if left enabled */ - cancel_delayed_work_sync(&spec->i2c_clk_work); - cs8409_disable_i2c_clock(codec); - - snd_hda_shutup_pins(codec); - - return 0; -} -#endif - /* Vendor specific HW configuration * PLL, ASP, I2C, SPI, GPIOs, DMIC etc... */ @@ -910,6 +916,7 @@ static const struct hda_codec_ops cs8409_cs42l42_patch_ops = { .init = cs8409_init, .free = cs8409_free, .unsol_event = cs8409_cs42l42_jack_unsol_event, + .reboot_notify = cs8409_reboot_notify, #ifdef CONFIG_PM .suspend = cs8409_cs42l42_suspend, #endif @@ -1121,6 +1128,7 @@ static const struct hda_codec_ops cs8409_dolphin_patch_ops = { .init = cs8409_init, .free = cs8409_free, .unsol_event = dolphin_jack_unsol_event, + .reboot_notify = cs8409_reboot_notify, #ifdef CONFIG_PM .suspend = cs8409_cs42l42_suspend, #endif