Re: HDA, power saving and recording

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

On 2023-05-15 5:33 PM, Takashi Iwai wrote:
> On Mon, 15 May 2023 16:49:48 +0200,
> Amadeusz Sławiński wrote:

...

>> I think there are two problems:
>>
>> 1. After probe codec is powered down
>> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/pci/hda/hda_codec.c#n833),
>> even though according to power management it is still running
> 
> I guess it's in the auto-suspend state, so it's still not suspended
> but the device itself is active, while the usage count is 0.
> 
> That's fine, and I thought my second patch handling it.  That is, if
> the usage count is 0 and the device is not suspended, it should return
> -EAGAIN and make the caller retry with the full power up.
> The code path is with CALL_RUN_FUNC() macro in hdac_regmap.c, and with
> -EAGAIN return value, it tries snd_hdac_power_up_pm() and call the
> function again.
> 
>> 2. When stream is started before first suspend, resume function
>> doesn't run and it is a function which syncs cached registers. By
>> resume function I mean
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/pci/hda/hda_codec.c#n2899
>> which calls snd_hda_regmap_sync() or through in case of the platform I
>> test it on codec->patch_ops.resume(codec) -> alc269_resume, which also
>> calls snd_hda_regmap_sync().
> 
> It's also expected, per se.  Since it's been not suspended, it assumes
> that the value got already written, and no resume is needed.


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.