* 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.