On Wed, 17 May 2023 15:15:13 +0200, Cezary Rojewski wrote: > > 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. OK, this seems to be a workaround. I took a deeper look at the issue now, and noticed that it's a messy problem. 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. thanks, Takashi
