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

[Takashi Iwai]

At Wed, 29 Aug 2012 12:19:32 +0000,
Lin, Mengdong wrote:
> 
> > 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.
> 
> Yes, this is better. And the patch works well for me - [PATCH 1/2] ALSA: hda - Avoid unnecessary parameter read for EPSS.
> 
> > 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.
> 
> Thanks for pointing out this! But I think the following codec can introduce risk of race among multiple codecs to modify ' chip->power_count',
> because only one codec's power on/off is serialized.
> static void azx_power_notify(struct hda_bus *bus, struct hda_codec *codec)
> {
> 	 ...
> 	if (codec->power_on) {
> 		pm_runtime_get_sync(&chip->pci->dev);
> 		chip->power_count++;
> 	} else {
> 		pm_runtime_put_sync(&chip->pci->dev);
> 		chip->power_count--;
> 	}
> }
> 
> How about calling pm_runtime_put_noidle() when freeing a codec in D0? 
> Maybe we can add a bus notify function like this:
> Static void azx_codec_free_notify(struct hda_codec *codec)
> {
> 	If(codec->power_on)
> 		pm_runtime_put_noidle(&codec->bus->pci->dev);
> }
> And snd_hda_codec_free() can trigger this notify.

That's my second idea but I didn't want to introduce yet another op
point.  But maybe it shall be the choice in the end...

> > - The D3-stop-clock check isn't done properly for the codecs providing
> >   own set_power_state ops.
> 
> I cannot test the branch 'if (codec->patch_ops.set_power_state)' in hda_set_power_state() for lack of such a codec. 
> But the logic seems right and the other branch works well for me :-)
> 
> > - Ditto for the repeated power-set sequence, the error check, etc.
> In hda_sync_power_state(), 
> 	if ((state & 0xff) == power_state)   ... should be if ((state & 0xf0)>> 4 == power_state), because PS-Act is bits 7:4
> 		break;

Ah thanks, I forgot that.

> And would you please explain why changing to D3 need not wait? One of my codec does have a delay on change from D0 to D3.

It was skipped because the driver doesn't need to wait.  It's just a
power down and doesn't matter even if you proceed if you go to the
next codec.  But as we are checking stop-clock bit, D3 should be
sync'ed as well, indeed.  I'm going to remove the exception of D3,
too.


thanks,

Takashi