On Fri, 8 Feb 2019, Takashi Iwai wrote:
On Fri, 08 Feb 2019 12:33:14 +0100, Guennadi Liakhovetski wrote:
Hi Takashi,
On Fri, 8 Feb 2019, Takashi Iwai wrote:
On Fri, 08 Feb 2019 11:50:25 +0100, Guennadi Liakhovetski wrote:
From: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com
If HDA probing on Intel platforms fails, all the devices, that have previously been powered up, have to be suspended again.
Signed-off-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com
I came across HDA not suspending when no codec driver has successfully probed while working on SOF. Then I decided to look how this is handled in the mainline Intel HDA code and I think something like this patch is needed, however, I don't have a set up to test it. I haven't looked at the tegra HDA implementation, possibly they need something similar. Please, comment.
This will likely lead to the runtime PM refcount unbalance at azx_free(), I'm afraid. chip->running is used as a flag to judge whether the runtime PM is applied or not, and if you jump to the error path, the flag won't be set.
Ok, understand, indeed, that would be a problem now. But isn't that also a problem, if azx_probe_continue() doesn't run at all before the azx_free()?
In that case, it won't go suspend, so it won't need resume at that point.
Mmh, ok, then we have two different things there - see below.
Wouldn't this fix it?
Not really. The chip->running is also checked in azx_runtime_idle().
Also, what you're trying to change is the behavior of HD-audio controller, not about codecs.
When talking about codecs I meant the path
azx_probe_continue() -> set_default_power_save() -> snd_hda_set_power_save() -> codec_set_power_save() -> pm_runtime_suspended()
and those codecs are first runtime-enabled via
azx_probe_continue() -> azx_probe_codecs() -> snd_hda_codec_new() -> snd_hda_codec_device_init() -> snd_hdac_device_init() -> pm_runtime_get_noresume()
So for symmetry they should also be runtime-disabled if probing doesn't complete successfully, right? And currently this isn't the case, so, there is a problem?
As for the controller, alright, I guess, we shouldn't touch its runtime PM if probe_continue() didn't succeed. So we only have to change the codec runtime PM status in case of a failure or have I misunderstood something about them and also their status is balanced in such cases?
Thanks Guennadi
thanks,
Takashi
@@ -1300,7 +1300,7 @@ static int azx_free(struct azx *chip) struct hda_intel *hda = container_of(chip, struct hda_intel, chip); struct hdac_bus *bus = azx_bus(chip);
- if (azx_has_pm_runtime(chip) && chip->running)
- if (azx_has_pm_runtime(chip)) pm_runtime_get_noresume(&pci->dev); chip->running = 0;
Thanks Guennadi
thanks,
Takashi
sound/pci/hda/hda_intel.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index e784130..d3caa37 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2229,14 +2229,14 @@ static int azx_probe_continue(struct azx *chip) /* create codec instances */ err = azx_probe_codecs(chip, azx_max_codecs[chip->driver_type]); if (err < 0)
goto out_free;
goto out_power_down;#ifdef CONFIG_SND_HDA_PATCH_LOADER if (chip->fw) { err = snd_hda_load_patch(&chip->bus, chip->fw->size, chip->fw->data); if (err < 0)
goto out_free;
goto out_power_down;#ifndef CONFIG_PM release_firmware(chip->fw); /* no longer needed */ chip->fw = NULL; @@ -2246,12 +2246,12 @@ static int azx_probe_continue(struct azx *chip) if ((probe_only[dev] & 1) == 0) { err = azx_codec_configure(chip); if (err < 0)
goto out_free;
goto out_power_down;}
err = snd_card_register(chip->card); if (err < 0)
goto out_free;
goto out_power_down;setup_vga_switcheroo_runtime_pm(chip);
@@ -2260,6 +2260,7 @@ static int azx_probe_continue(struct azx *chip)
set_default_power_save(chip);
+out_power_down: if (azx_has_pm_runtime(chip)) pm_runtime_put_autosuspend(&pci->dev);
-- 1.9.3