On Thu, 18 May 2023 11:00:54 +0200,
Cezary Rojewski wrote:
>
> 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.
It's a bit tricky. The in_pm refcount check is mandatory because the
very same code path is called recursively for enabling itself.
> 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.
Yeah, I'd love to clean up / fix the stuff, but this is in some
sensitive area, so let's get it carefully.
> > 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.
Ah correct, sorry for the typo, I mean *_if_active().
> Once again, thank you for the input. Ramping up and addressing this
> problem wouldn't happen so quickly without your guidance.
I'm going to submit your change as a proper fix patch soon later.
Then let's dig down and tidy the rest things up.
thanks,
Takashi
