Re: [alsa-devel] Question about runtime PM for HDA codecs

On Wed, 15 Jan 2020 21:28:20 +0100, Sridharan, Ranjani wrote:

[1 <text/plain; UTF-8 (7bit)>]

On Wed, Jan 15, 2020 at 11:40 AM Takashi Iwai tiwai@suse.de wrote:

On Wed, 15 Jan 2020 19:15:40 +0100,
Sridharan, Ranjani wrote:
>
> Hi Takashi,
>
> Can I please bother you with a question about how to handle the
situation
> with runtime PM for HDA codec on one of the Intel CML-based platforms
with
> an ALC285 codec.
> Basically, the problem we're facing is that when using the SOF driver,
> jack detection doesn't seem to work as intended because the codec gets
> suspended right after the headset is plugged in.

The codec going to the runtime suspend is actually the right
behavior.  Even the bus goes down if the codec has AC_PWRST_CLKSTOP
and AC_PWRST_EPSS capabilities, the link goes also down.  But this
means that the irq is still triggered upon the jack detection event
even in D3 state.
I guess this implementation is missing in SOF DSP side.

We do have the WAKEEN feature implemented in SOF for capturing the jack detecting events when the SOF device is in D3. We have this feature working as expected on some previous platforms.

Which codec was it?

The problem specific to the ALC285 codec.

AFAIK, there is no specific change to ALC285 that is relevant with runtime PM.

> After a bit of experimenting, what we found was that calling
> snd_hda_set_power_save() with a delay of 0 or something > 0, both help
with
> fixing the problem.
>
> But, I have a basic question about this. What is the expectation for
> enabling the codec runtime PM?
> I see that the legacy driver calls snd_hda_set_power_save() based on the
> CONFIG_SND_HDA_POWER_SAVE_DEFAULT value. In the case of SOF, we do not
> explicitly set this value so it is 0 by default. Also, when the codec is
> registered, the runtime PM for the codec is enabled by default without
> checking if the power_save delay is set to 0 or not. And later when the
> snd_hda_set_power_save() is called from the legacy HDA driver probe, it
> sets the use of auto suspend and the delay to be used based on the
config.

The runtime setup purely depends on the user's configuration.  As
default, kernel may set to certain value via Kconfig, and some codecs
(e.g. HDMI) prefer the runtime PM enablement as default.  Other than
that, it's supposed to be set explicitly via sysfs, typically from
udev rules.

The driver initial code does set up some default value from historical
reasons, but basically the setup is done from user-space.

> Would it be correct to remove the pm_runtime_enable() call
> in snd_hda_codec_register() and let the codec runtime PM enabling be set
> with the call to snd_hda_set_power_save() for both the legacy HDA driver
> and the SOF driver?

The basic problem isn't about how the runtime PM is set.  It can be
changed at any time, and the driver should work no matter how it is
set.

In this case then, I suppose it is up to the SOF driver to set the auto suspend delay to define the expected default behaviour?

Yes and no. User can still set the value and the driver still needs to behave correctly no matter what value is set.

thanks,

Takashi