Re: [PATCH 2/2] ALSA: hda/cs8409: Prevent pops and clicks during reboot

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux