[alsa-devel] [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Takashi Iwai]

On Mon, 25 Jun 2018 16:51:12 +0200,
Sriram Periyasamy wrote:
> 
> On Mon, Jun 25, 2018 at 11:53:59AM +0200, Takashi Iwai wrote:
> > On Mon, 25 Jun 2018 11:23:17 +0200,
> > Sriram Periyasamy wrote:
> > > 
> > > On Mon, Jun 25, 2018 at 11:08:13AM +0200, Takashi Iwai wrote:
> > > > On Mon, 25 Jun 2018 10:53:49 +0200,
> > > > Sriram Periyasamy wrote:
> > > > > 
> > > > > On Fri, Jun 22, 2018 at 07:14:50PM +0200, Takashi Iwai wrote:
> > > > > > On Fri, 22 Jun 2018 18:42:24 +0200,
> > > > > > Pierre-Louis Bossart wrote:
> > > > > > > 
> > > > > > > On 6/22/18 4:25 AM, Sriram Periyasamy wrote:
> > > > > > > > On certain platforms, Display HDMI HDA codec was not going to sleep state
> > > > > > > > after the use when links are powered down after turning off the display
> > > > > > > > power. As per the HW recommendation, links are powered down before turning
> > > > > > > > off the display power to ensure that the codec goes to sleep state.
> > > > > > > 
> > > > > > > The change looks ok, I am still not clear on display power
> > > > > > > management. snd_hdac_display_power is called both from hdac_hdmi.c and
> > > > > > > skl.c in the respective probe/suspend/resume routines, wondering if
> > > > > > > this is redundant or intentional.
> > > > > > 
> > > > > > Yeah, it looks like that skl.c keeps the display power on by assuming
> > > > > > that it's turned off by the codec driver, but it doesn't happen
> > > > > > actually.  I also find it fishy there...
> > > > > > 
> > > > > 
> > > > > snd_hdac_display_power is called intentionally to ensure that the codec is
> > > > > powered up before any communication is initiated from either skl.c or
> > > > > hdac_hdmi.c.
> > > > 
> > > > But who turns it down if the device is not used?
> > > 
> > > Powering down of codec is taken care in codec's suspend routine.
> > 
> > But you'll power up twice, no?  Once in skl_resume() (which is no
> > runtime resume but only for S3/S4) and once in codec resume.
> > I wonder who will turn down when you wake up from S3.
> 
> No. When entering to S3, hdmi_codec_prepare() will invoke the runtime
> resume of codec hdac_hdmi_runtime_resume() which power up the codec and
> skl_suspend power down the codec. When it wakes up from S3, skl_resume()
> powers up the codec and hdmi_codec_complete invokes the runtime suspend of
> codec hdac_hdmi_runtime_suspend() which powers down the codec.

Hm, but what about the i915 power well refcount, not the codec runtime
PM refcount?  Does the refcount reach to zero when the system goes to
runtime suspend?  That puzzles me.

> This design is required to achieve the runtime power reference count for
> codec device when it is put to S3/S4 while playback was going on.

... and this "design" picture is completely missing.
That's why illustrating it was requested instead of a patch :)

> >  The runtime
> > suspend doesn't call snd_hdac_display_power() in skl.c.
> > 
> 
> Display power management for all runtime uses cases are handled in the
> codec driver hdac_hdmi.c.

But it means that hdac_hdmi.c does manage the extra decrement the
refcount that was taken in skl.c?


thanks,

Takashi