[alsa-devel] [PATCH] ALSA: hda - Fix runtime PM accounting

[Takashi Iwai]

At Tue, 28 Aug 2012 08:14:43 +0000,
Lin, Mengdong wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Sunday, August 26, 2012 10:26 PM
> 
> > The flag codec->d3_clk_stop_ok isn't always set when the codec is powered
> > off.  If it's not set, the controller driver doesn't change the runtime PM
> > count.  This causes an unblanced accounting when multiple codecs are
> > present.  
> 
> If the flag codec->d3_clk_stop_ok is not set on changes to D3, this flag will keep 0 and the same codec will not call pm_runtime_get_sync() on change back to D0. So I feel the runtime PM count is still balanced although the controller may lose a chance to suspend itself.

Yeah, my analysis was wrong.  The refcounting is OK during the driver
is being operated.  But it messes up in some corner cases.  See below.

> Usually, the first codec is powered down with CLK_STOP_OK=0,
> > then the next codec with CLK_STOP_OK=1.  At this moment, the first codec
> > is changed also to CLK_STOP_OK=1, but this isn't informed to the controller.
> > Thus it still thinks as if one codec is active.
> 
> Does this conflict with spec? The spec says that PS-ClkStopOk should be set or cleared *before* entering D3: "if the codec requires BCLK to be available in D3 state, while a pass through loop is active, then it must indicate so by clearing the PS-ClkStopOk bit before entering D3 state, otherwise it must set that bit." If this is not true, then checking later is necessary.

I did a deeper look, and it seems that the codec on my machine (IDT)
requires the longer delay even though it advertises EPSS.  This means
we break potentially others, and we need some way to override the EPSS
check.

Now I introduced codec->epss flag that is initialized once in
snd_hda_codec_new(), and let the codec driver overrides this flag.
One bonus is that you can avoid the unnecessary codec parameter read
at each power change.

> > For fixing this issue, move the check of CLK_STOP_OK bit at the runtime
> > suspend time, and simple up/down by the codec power save in
> > azx_power_notifier().  For CLK_STOP_OK bit checks of all codecs, a new
> > helper function is introduced.
> 
> I've tested your patch but with a little change. Because one of my codecs cannot support stop-clock, I pretend it can by setting codec->d3_clk_stop. Then I found snd_hda_bus_ready_for_d3() blocks when getting this codec's power state, and so the runtime PM status keeps SUSPENDING. I'm not sure it's because this codec cannot support 'get state' after entering D3 for some time, or other reason. 
> I've just got a new codec that should support 'stop-clk' in D3 and I'll test the new codec. My platform is not stable and it may take some time to do the test. Thanks!

Drop my previous patch.  It's a crap.  It shouldn't call the parameter
read at that place at all...

In anyway, there are a few issues:
- The refcounting gets broken after module unload.  It's because we
  leave the codec's pm_runtime usages after removal.

- The D3-stop-clock check isn't done properly for the codecs providing
  own set_power_state ops.

- Ditto for the repeated power-set sequence, the error check, etc.

Now I fixed these things and uploaded to topic/hda-fix branch in sound
git tree.  Please take a look and check whether it's OK.

Meanwhile, ESSP fix must go to 3.6, so I queued it to for-linus branch
and merged back to for-next branch (due to merge conflicts).


thanks,

Takashi