Re: HDA, power saving and recording

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2023-05-17 4:52 PM, Takashi Iwai wrote:
> On Wed, 17 May 2023 15:15:13 +0200,
> Cezary Rojewski wrote:

...

>> After reading this conversation few times I came to conclusion that
>> codec device should be kept up as long as its runtime_status=0
>> (RPM_ACTIVE), regardless if usage_count is 0 or not. Basically, in
>> autosuspend case usage_count and runtime_status for the device are
>> both 0 so, if we are not ignoring the former by calling
>> pm_runtime_get_if_in_use() then we end up caching the writes during
>> the autosuspend period - period when the device is no longer used but
>> there is still some time left before it's suspended.
>>
>>
>> --- a/sound/hda/hdac_device.c
>> +++ b/sound/hda/hdac_device.c
>> @@ -611,7 +611,7 @@ 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);
>> +               int ret = pm_runtime_get_if_active(&codec->dev, true);
>>                  if (!ret)
>>                          return -1;
>>                  if (ret < 0)
>>
>>
>> Results for the above look good.
> 
> OK, this seems to be a workaround.
> 
> I took a deeper look at the issue now, and noticed that it's a messy
> problem.


While we want to address this issue first - I've even messaged you about 
this for the very first time early 2021 :D but it's not marked as high 
prio so it remained unaddressed till now - me and Amadeo spent some time 
analyzing most of the pm-related code for sound/hda and we believe most 
of it could be replaced by native pm_runtime_xxx code and fields such as 
->in_pm could be dropped. However, this won't take a day or two and it's 
best if first we get to know the background what/why/when some of those 
specific functions/members exist in the hda code.

> The check with pm_runtime_get_if_in_use() itself isn't wrong, but it
> needs the exceptional handling.
> 
> In addition to that, however, we need to work around the case where
> the register gets updated twice with -EAGAIN handling; at the first
> update, the regcache gets updated while the actual write gives an
> error.  Then at the second update, it checks only the cache and
> believes as if it's been already written, and the write is skipped.
> This was what Amadeusz experienced with my previous patch.
> So, we need to paper over those two.
> 
> OTOH, if we replace the primary check with
> pm_runtime_get_if_active(true), this makes the things *mostly*
> working, too.  This increases the usage count forcibly when the device
> is active, and we'll continue to write the register.
> The caveat is that the auto-suspend timer is still ticking in this
> case.  But, this won't be a big problem, as the timer callback does
> check the state and cancel itself if the device is actually in use.
> 
> So, I guess we should go for pm_runtime_get_if_in_use().
> But please give it more tests.

I believe you meant pm_runtime_get_if_active(true) in the last one. If 
yes, then indeed I'm delaying the patch upload until more tests are run.

Once again, thank you for the input. Ramping up and addressing this 
problem wouldn't happen so quickly without your guidance.


Regards,
Czarek