* Question about hdac_ext_link ref_count management @ 2021-01-21 23:23 Ranjani Sridharan 2021-01-22 14:12 ` Takashi Iwai 0 siblings, 1 reply; 6+ messages in thread From: Ranjani Sridharan @ 2021-01-21 23:23 UTC (permalink / raw) To: alsa-devel, tiwai; +Cc: kai.vehmanen 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Question about hdac_ext_link ref_count management 2021-01-21 23:23 Question about hdac_ext_link ref_count management Ranjani Sridharan @ 2021-01-22 14:12 ` Takashi Iwai 2021-01-22 16:40 ` Ranjani Sridharan 0 siblings, 1 reply; 6+ messages in thread From: Takashi Iwai @ 2021-01-22 14:12 UTC (permalink / raw) To: Ranjani Sridharan; +Cc: alsa-devel, kai.vehmanen 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Question about hdac_ext_link ref_count management 2021-01-22 14:12 ` Takashi Iwai @ 2021-01-22 16:40 ` Ranjani Sridharan 2021-01-22 16:50 ` Takashi Iwai 0 siblings, 1 reply; 6+ messages in thread From: Ranjani Sridharan @ 2021-01-22 16:40 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, kai.vehmanen 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Question about hdac_ext_link ref_count management 2021-01-22 16:40 ` Ranjani Sridharan @ 2021-01-22 16:50 ` Takashi Iwai 2021-01-22 17:24 ` Ranjani Sridharan 2021-01-22 22:04 ` Ranjani Sridharan 0 siblings, 2 replies; 6+ messages in thread From: Takashi Iwai @ 2021-01-22 16:50 UTC (permalink / raw) To: Ranjani Sridharan; +Cc: alsa-devel, kai.vehmanen 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Question about hdac_ext_link ref_count management 2021-01-22 16:50 ` Takashi Iwai @ 2021-01-22 17:24 ` Ranjani Sridharan 2021-01-22 22:04 ` Ranjani Sridharan 1 sibling, 0 replies; 6+ messages in thread From: Ranjani Sridharan @ 2021-01-22 17:24 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, kai.vehmanen > > > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Question about hdac_ext_link ref_count management 2021-01-22 16:50 ` Takashi Iwai 2021-01-22 17:24 ` Ranjani Sridharan @ 2021-01-22 22:04 ` Ranjani Sridharan 1 sibling, 0 replies; 6+ messages in thread From: Ranjani Sridharan @ 2021-01-22 22:04 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, kai.vehmanen 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-22 22:05 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-21 23:23 Question about hdac_ext_link ref_count management Ranjani Sridharan 2021-01-22 14:12 ` Takashi Iwai 2021-01-22 16:40 ` Ranjani Sridharan 2021-01-22 16:50 ` Takashi Iwai 2021-01-22 17:24 ` Ranjani Sridharan 2021-01-22 22:04 ` Ranjani Sridharan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.