Re: HDA, power saving and recording

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

On 5/12/2023 1:33 PM, Takashi Iwai wrote:
> On Fri, 12 May 2023 13:23:49 +0200,
> Takashi Iwai wrote:
>>
>> On Thu, 11 May 2023 19:20:17 +0200,
>> Amadeusz Sławiński wrote:
>>>
>>> On 5/11/2023 5:58 PM, Takashi Iwai wrote:
>>>> On Thu, 11 May 2023 17:31:37 +0200,
>>>> Amadeusz Sławiński wrote:
>>>>>
>>>>> On 5/10/2023 2:21 PM, Takashi Iwai wrote:
>>>>>> On Tue, 09 May 2023 12:10:06 +0200,
>>>>>> Amadeusz Sławiński wrote:
>>>>> Then capture stream starts and seems to assume that
>>>>> registers were already set, so it doesn't write them to hw.
>>>>
>>>> ... it seems this didn't happen, and that's the inconsistency.
>>>>
>>>> So the further question is:
>>>> At the point just before you start recording, is the codec in runtime
>>>> suspended?  Or it's running?
>>>>
>>>> If it's runtime-suspended, snd_hda_regmap_sync() must be called from
>>>> alc269_resume() via runtime-resume, and this must write out the
>>>> cached values.  Then the bug can be along with that line.
>>>>
>>>> Or if it's running, it means that the previous check of
>>>> snd_hdac_keep_power_up() was bogus (or racy).
>>>>
>>>
>>> Well, it is in... let's call it semi powered state. When snd_hda_intel
>>> driver is loaded with power_save=X option it sets timeout to X seconds
>>> and problem only happens when I start the stream before those X
>>> seconds pass and it runs first runtime suspend. After it suspends it
>>> then uses standard pm_runtime_resume and works correctly. That's why
>>> the pm_runtime_force_suspend(&codec->core.dev);  mentioned in first
>>> email in thread "fixes" the problem, as it forces it to be instantly
>>> suspended instead of waiting for timeout and then later normal
>>> resume-play/record-suspend flow can be followed.
>>
>> Hm, then maybe it's a bad idea to rely on the usage count there.
>> Even if the usage is 0, the device can be still active, and the update
>> can be missed.
>>
>> How about the patch like below?
> 
> Scratch that, it returns a wrong value.
> A simpler version like below works instead?
> 

Yes it was broken, arecord didn't even start capturing ;)

> 
> Takashi
> 
> --- a/sound/hda/hdac_device.c
> +++ b/sound/hda/hdac_device.c
> @@ -611,10 +611,9 @@ EXPORT_SYMBOL_GPL(snd_hdac_power_up_pm);
>   int snd_hdac_keep_power_up(struct hdac_device *codec)
>   {
>   	if (!atomic_inc_not_zero(&codec->in_pm)) {
> -		int ret = pm_runtime_get_if_in_use(&codec->dev);
> -		if (!ret)
> +		if (!pm_runtime_active(&codec->dev))
>   			return -1;
> -		if (ret < 0)
> +		if (pm_runtime_get_sync(&codec->dev) < 0)
>   			return 0;
>   	}
>   	return 1;


This one seems to work, as in I'm able to record before first suspend 
hits. However device stays in D0 when no stream is running...
# cat /sys/devices/pci0000\:00/0000\:00\:0e.0/power_state
D0