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