Question about hdac_ext_link ref_count management

[Ranjani Sridharan]

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