Question about hdac_ext_link ref_count management
Hi Takashi,
While exploring some power optimizations on Intel platforms, I noticed that the hdac_ext_link ref_count is incremented during codec probe in hdac_hda_codec_probe() and the ref_count is held until the codec device is removed.
I was wondering if it would be possible to call the get/put for the hdac_ext_link in the codec runtime suspend/resume callbacks so that the link is powered up only when the link is in use. Are there any downsides to doing this?
Thanks, Ranjani
On Fri, 22 Jan 2021 00:23:53 +0100, Ranjani Sridharan wrote:
Hi Takashi,
While exploring some power optimizations on Intel platforms, I noticed that the hdac_ext_link ref_count is incremented during codec probe in hdac_hda_codec_probe() and the ref_count is held until the codec device is removed.
I was wondering if it would be possible to call the get/put for the hdac_ext_link in the codec runtime suspend/resume callbacks so that the link is powered up only when the link is in use. Are there any downsides to doing this?
Wouldn't the snd_hdac_ext_bus_link_power_up() / down() calls do the runtime PM stuff? Maybe we need to revisit those link power management. The ext stuff isn't well managed, I'm afraid.
The get() and put() are obviously for fully enabling and disabling the device, hence it's not suitable for the runtime PM suspend/resume. The power_up() / down() should be adjusted to fit with the runtime PM call, if any.
thanks,
Takashi
On Fri, 2021-01-22 at 15:12 +0100, Takashi Iwai wrote:
On Fri, 22 Jan 2021 00:23:53 +0100, Ranjani Sridharan wrote:
Hi Takashi,
While exploring some power optimizations on Intel platforms, I noticed that the hdac_ext_link ref_count is incremented during codec probe in hdac_hda_codec_probe() and the ref_count is held until the codec device is removed.
I was wondering if it would be possible to call the get/put for the hdac_ext_link in the codec runtime suspend/resume callbacks so that the link is powered up only when the link is in use. Are there any downsides to doing this?
Wouldn't the snd_hdac_ext_bus_link_power_up() / down() calls do the runtime PM stuff? Maybe we need to revisit those link power management. The ext stuff isn't well m and, I'm afraid.
Thanks, Takashi. It looks like snd_hdac_ext_bus_link_power_up/down() are only called during snd_hdac_ext_bus_link_get/put(). Actually, in my observation disabling the CORB/RIRB buffer DMAs is what saves us power and this is done only if snd_hdac_ext_bus_link_put() is called on all links.
The get() and put() are obviously for fully enabling and disabling the device, hence it's not suitable for the runtime PM suspend/resume. The power_up() / down() should be adjusted to fit with the runtime PM call, if any.
The only additional thing that snd_hdac_ext_bus_link_get/put() does on top of snd_hdac_ext_bus_link_power_up/down() is to stop the CORB/RIRB DMA when all the link ref_counts are 0. Do you think it is not advisable to stop the CORB/RIRB DMA during runtime PM?
Thanks, Ranjani
On Fri, 22 Jan 2021 17:40:55 +0100, Ranjani Sridharan wrote:
On Fri, 2021-01-22 at 15:12 +0100, Takashi Iwai wrote:
On Fri, 22 Jan 2021 00:23:53 +0100, Ranjani Sridharan wrote:
Hi Takashi,
While exploring some power optimizations on Intel platforms, I noticed that the hdac_ext_link ref_count is incremented during codec probe in hdac_hda_codec_probe() and the ref_count is held until the codec device is removed.
I was wondering if it would be possible to call the get/put for the hdac_ext_link in the codec runtime suspend/resume callbacks so that the link is powered up only when the link is in use. Are there any downsides to doing this?
Wouldn't the snd_hdac_ext_bus_link_power_up() / down() calls do the runtime PM stuff? Maybe we need to revisit those link power management. The ext stuff isn't well m and, I'm afraid.
Thanks, Takashi. It looks like snd_hdac_ext_bus_link_power_up/down() are only called during snd_hdac_ext_bus_link_get/put(). Actually, in my observation disabling the CORB/RIRB buffer DMAs is what saves us power and this is done only if snd_hdac_ext_bus_link_put() is called on all links.
The get() and put() are obviously for fully enabling and disabling the device, hence it's not suitable for the runtime PM suspend/resume. The power_up() / down() should be adjusted to fit with the runtime PM call, if any.
The only additional thing that snd_hdac_ext_bus_link_get/put() does on top of snd_hdac_ext_bus_link_power_up/down() is to stop the CORB/RIRB DMA when all the link ref_counts are 0. Do you think it is not advisable to stop the CORB/RIRB DMA during runtime PM?
Why do you need to stop CORB/RIRB? For stopping the CORB/RIRB DMA, you need to disable the IRQ and other stuff at first, in anyway.
Takashi
It looks like snd_hdac_ext_bus_link_power_up/down() are only called during snd_hdac_ext_bus_link_get/put(). Actually, in my observation disabling the CORB/RIRB buffer DMAs is what saves us power and this is done only if snd_hdac_ext_bus_link_put() is called on all links.
The get() and put() are obviously for fully enabling and disabling the device, hence it's not suitable for the runtime PM suspend/resume. The power_up() / down() should be adjusted to fit with the runtime PM call, if any.
The only additional thing that snd_hdac_ext_bus_link_get/put() does on top of snd_hdac_ext_bus_link_power_up/down() is to stop the CORB/RIRB DMA when all the link ref_counts are 0. Do you think it is not advisable to stop the CORB/RIRB DMA during runtime PM?
Why do you need to stop CORB/RIRB? For stopping the CORB/RIRB DMA, you need to disable the IRQ and other stuff at first, in anyway.
I see. Let me go back and double check if powering down the link during codec runtime suspend yields the same results in terms of power as well and get back to you.
Thanks, Ranjani
On Fri, 2021-01-22 at 17:50 +0100, Takashi Iwai wrote:
On Fri, 22 Jan 2021 17:40:55 +0100, Ranjani Sridharan wrote:
On Fri, 2021-01-22 at 15:12 +0100, Takashi Iwai wrote:
On Fri, 22 Jan 2021 00:23:53 +0100, Ranjani Sridharan wrote:
Hi Takashi,
While exploring some power optimizations on Intel platforms, I noticed that the hdac_ext_link ref_count is incremented during codec probe in hdac_hda_codec_probe() and the ref_count is held until the codec device is removed.
I was wondering if it would be possible to call the get/put for the hdac_ext_link in the codec runtime suspend/resume callbacks so that the link is powered up only when the link is in use. Are there any downsides to doing this?
Wouldn't the snd_hdac_ext_bus_link_power_up() / down() calls do the runtime PM stuff? Maybe we need to revisit those link power management. The ext stuff isn't well m and, I'm afraid.
Thanks, Takashi. It looks like snd_hdac_ext_bus_link_power_up/down() are only called during snd_hdac_ext_bus_link_get/put(). Actually, in my observation disabling the CORB/RIRB buffer DMAs is what saves us power and this is done only if snd_hdac_ext_bus_link_put() is called on all links.
The get() and put() are obviously for fully enabling and disabling the device, hence it's not suitable for the runtime PM suspend/resume. The power_up() / down() should be adjusted to fit with the runtime PM call, if any.
The only additional thing that snd_hdac_ext_bus_link_get/put() does on top of snd_hdac_ext_bus_link_power_up/down() is to stop the CORB/RIRB DMA when all the link ref_counts are 0. Do you think it is not advisable to stop the CORB/RIRB DMA during runtime PM?
Why do you need to stop CORB/RIRB? For stopping the CORB/RIRB DMA, you need to disable the IRQ and other stuff at first, in anyway.
Hi Takashi,
I've confirmed that turning off the link and stopping CORB/RIRB is what yields the maximum power savings. Just powering off the link without stopping CORB/RIRB does not yield meaningful savings.
If I may ask a question, we already stop CORB/RIRB and turn off the links in the SOF runtime_suspend callback. The usecase we're trying to optimize is when wake-on-voice is the only active stream. There is no HDMI playback and the codec driver is runtime suspended. So, cant we do the same thing as runtime suspend and turn off the CORB/RIRB as well as the links too? What adverse impacts am I missing here?
Thanks, Ranjani
participants (2)
-
Ranjani Sridharan -
Takashi Iwai