[PATCH v2] ALSA: HDA: Early Forbid of runtime PM

[PATCH v2] ALSA: HDA: Early Forbid of runtime PM

[Harsha Priya]

For certain codecs (like Realtek), pm_runtime_forbid() is invoked 
in the probe function after build_controls(). In a stress test, 
its observed occasionally that runtime PM calls are invoked 
before controls are built. This causes the codec to be
runtime suspended before probe completes. Because of this, not all
controls are enumerated correctly, and audio does not work until
system is rebooted.

This issue being common across all codecs, pm_runtime_forbid() is 
called when the codec object is created to fix this issue. 
A codec enables or disables runtime pm in its own probe function.

Multiple stress tests of 2000+ cycles has been done to test the fix.

Signed-off-by: Harsha Priya <harshapriya.n@intel.com>
Signed-off-by: Emmanuel Jillela <emmanuel.jillela@intel.com>
Reviewed-by: Kailang Yang <kailang@realtek.com>
---
 sound/pci/hda/hda_codec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index e96a87f1b611..a356c21edb90 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -1000,6 +1000,9 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
 	if (err < 0)
 		goto error;
 
+	/* PM runtime needs to be enabled later after binding codec */
+	pm_runtime_forbid(&codec->core.dev);
+
 	return 0;
 
  error:
-- 
2.17.1

Re: [PATCH v2] ALSA: HDA: Early Forbid of runtime PM

[Takashi Iwai]

On Fri, 28 Aug 2020 01:05:36 +0200,
Harsha Priya wrote:
> 
> For certain codecs (like Realtek), pm_runtime_forbid() is invoked 
> in the probe function after build_controls(). In a stress test, 
> its observed occasionally that runtime PM calls are invoked 
> before controls are built. This causes the codec to be
> runtime suspended before probe completes. Because of this, not all
> controls are enumerated correctly, and audio does not work until
> system is rebooted.
> 
> This issue being common across all codecs, pm_runtime_forbid() is 
> called when the codec object is created to fix this issue. 
> A codec enables or disables runtime pm in its own probe function.
> 
> Multiple stress tests of 2000+ cycles has been done to test the fix.
> 
> Signed-off-by: Harsha Priya <harshapriya.n@intel.com>
> Signed-off-by: Emmanuel Jillela <emmanuel.jillela@intel.com>
> Reviewed-by: Kailang Yang <kailang@realtek.com>

Thanks.  The only concern is about the influence on the relevant ASoC
code, especially hdac_hda.c.

Kai, could you check whether this still works?


Takashi

> ---
>  sound/pci/hda/hda_codec.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index e96a87f1b611..a356c21edb90 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -1000,6 +1000,9 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
>  	if (err < 0)
>  		goto error;
>  
> +	/* PM runtime needs to be enabled later after binding codec */
> +	pm_runtime_forbid(&codec->core.dev);
> +
>  	return 0;
>  
>   error:
> -- 
> 2.17.1
> 

Re: [PATCH v2] ALSA: HDA: Early Forbid of runtime PM

[Kai Vehmanen]

Hey,

On Fri, 28 Aug 2020, Takashi Iwai wrote:

> On Fri, 28 Aug 2020 01:05:36 +0200, Harsha Priya wrote:
> > This issue being common across all codecs, pm_runtime_forbid() is 
> > called when the codec object is created to fix this issue. 
> > A codec enables or disables runtime pm in its own probe function.
> 
> Thanks.  The only concern is about the influence on the relevant ASoC
> code, especially hdac_hda.c.
> 
> Kai, could you check whether this still works?

I believe Harsha is testing mostly with ASoC, so hdac_hda.c should be 
covered. 

I did queue a SOF CI job for this v2 patch and I'm seeing some failures
in module load/unload test that might be related and need checking before
we merge:

https://sof-ci.01.org/linuxpr/PR2403/build4400/devicetest/

The actual runtime-PM tests in our test set pass, but module load/unload 
has failures on some platforms. I'll follow-up when I have a better 
understanding what goes wrong.

Br, Kai

Re: [PATCH v2] ALSA: HDA: Early Forbid of runtime PM

[Takashi Iwai]

On Fri, 28 Aug 2020 19:06:51 +0200,
Jillela, Emmanuel wrote:
> 
> 
> > Subject: Re: [PATCH v2] ALSA: HDA: Early Forbid of runtime PM
> > 
> > Hey,
> > 
> > On Fri, 28 Aug 2020, Takashi Iwai wrote:
> > 
> > > On Fri, 28 Aug 2020 01:05:36 +0200, Harsha Priya wrote:
> > > > This issue being common across all codecs, pm_runtime_forbid() is
> > > > called when the codec object is created to fix this issue.
> > > > A codec enables or disables runtime pm in its own probe function.
> > >
> > > Thanks.  The only concern is about the influence on the relevant ASoC
> > > code, especially hdac_hda.c.
> > >
> > > Kai, could you check whether this still works?
> > 
> > I believe Harsha is testing mostly with ASoC, so hdac_hda.c should be covered.
> > 
> > I did queue a SOF CI job for this v2 patch and I'm seeing some failures in module
> > load/unload test that might be related and need checking before we merge:
> > 
> > https://sof-ci.01.org/linuxpr/PR2403/build4400/devicetest/
> > 
> > The actual runtime-PM tests in our test set pass, but module load/unload has failures >on
> > some platforms. I'll follow-up when I have a better understanding what goes wrong.
> > 
> > Br, Kai
> I am just trying to think ahead to get something that may work. We can add an if check around the pm_runtime_forbid like below
> If (!codec->auto_runtime_pm)
> 	Pm_runtime_forbid(&codec->core.dev)
> Do you think that will work??

snd_hda_codec_device_new() is called before setting the flag, so it's
no use for adding the conditional.


thanks,

Takashi

Re: [PATCH v2] ALSA: HDA: Early Forbid of runtime PM

[Kai Vehmanen]

Hi,

On Fri, 28 Aug 2020, Kai Vehmanen wrote:

> On Fri, 28 Aug 2020, Takashi Iwai wrote:
> > Thanks.  The only concern is about the influence on the relevant ASoC
> > code, especially hdac_hda.c.
> > 
> > Kai, could you check whether this still works?
> 
> I did queue a SOF CI job for this v2 patch and I'm seeing some failures
> in module load/unload test that might be related and need checking before
> we merge:
> 
> https://sof-ci.01.org/linuxpr/PR2403/build4400/devicetest/

ok this took a bit longer than expected, but we've been debugging this 
with Emmanuel and found the rootcause.

So if we take this patch in and forbid runtime PM in device_new, it then 
becomes mandatory for all controllers to call set_default_power_save(). In 
SOF, this is only added recently to some machine drivers (notably the 
widely used generic HDA machine driver), which explains why tests pass on 
many targets. However, on platforms that use other machine drivers, 
set_default_power_save()/snd_hda_set_power_save() is not called, and 
device will never hit suspend (and this fails the tests).

To further complicate the matter, recently merged "ASoC: Intel: 
skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay" will 
cause tests to fail on more platforms as HDMI/DP codec's powersave 
settings are not set.

The original patch is ok, but to merge it without a lot of regressions, 
we need to rework how powersave initial settings are done in SOF and other 
ASoC controllers using HDAC. I do think this is the right solution.

Br, Kai