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

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

[Sriram Periyasamy]

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.

Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c  | 12 +++++-------
 sound/soc/intel/skylake/skl.c | 11 ++++++-----
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 84f7a7a36e4b..b78fd84bf591 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -2127,12 +2127,6 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
 	 */
 	snd_hdac_codec_read(hdev, hdev->afg, 0,	AC_VERB_SET_POWER_STATE,
 							AC_PWRST_D3);
-	err = snd_hdac_display_power(bus, false);
-	if (err < 0) {
-		dev_err(bus->dev, "Cannot turn on display power on i915\n");
-		return err;
-	}
-
 	hlink = snd_hdac_ext_bus_get_link(ebus, dev_name(dev));
 	if (!hlink) {
 		dev_err(dev, "hdac link not found\n");
@@ -2141,7 +2135,11 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
 
 	snd_hdac_ext_bus_link_put(ebus, hlink);
 
-	return 0;
+	err = snd_hdac_display_power(bus, false);
+	if (err < 0)
+		dev_err(bus->dev, "Cannot turn off display power on i915\n");
+
+	return err;
 }
 
 static int hdac_hdmi_runtime_resume(struct device *dev)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 670ff9aaca55..c62e474717ff 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -762,6 +762,12 @@ static void skl_probe_work(struct work_struct *work)
 		}
 	}
 
+	/*
+	 * we are done probing so decrement link counts
+	 */
+	list_for_each_entry(hlink, &ebus->hlink_list, list)
+		snd_hdac_ext_bus_link_put(ebus, hlink);
+
 	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
 		err = snd_hdac_display_power(bus, false);
 		if (err < 0) {
@@ -771,11 +777,6 @@ static void skl_probe_work(struct work_struct *work)
 		}
 	}
 
-	/*
-	 * we are done probing so decrement link counts
-	 */
-	list_for_each_entry(hlink, &ebus->hlink_list, list)
-		snd_hdac_ext_bus_link_put(ebus, hlink);
 
 	/* configure PM */
 	pm_runtime_put_noidle(bus->dev);
-- 
2.7.4

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Pierre-Louis Bossart]

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.

> 
> Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
> Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
> ---
>   sound/soc/codecs/hdac_hdmi.c  | 12 +++++-------
>   sound/soc/intel/skylake/skl.c | 11 ++++++-----
>   2 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> index 84f7a7a36e4b..b78fd84bf591 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -2127,12 +2127,6 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
>   	 */
>   	snd_hdac_codec_read(hdev, hdev->afg, 0,	AC_VERB_SET_POWER_STATE,
>   							AC_PWRST_D3);
> -	err = snd_hdac_display_power(bus, false);
> -	if (err < 0) {
> -		dev_err(bus->dev, "Cannot turn on display power on i915\n");
> -		return err;
> -	}
> -
>   	hlink = snd_hdac_ext_bus_get_link(ebus, dev_name(dev));
>   	if (!hlink) {
>   		dev_err(dev, "hdac link not found\n");
> @@ -2141,7 +2135,11 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
>   
>   	snd_hdac_ext_bus_link_put(ebus, hlink);
>   
> -	return 0;
> +	err = snd_hdac_display_power(bus, false);
> +	if (err < 0)
> +		dev_err(bus->dev, "Cannot turn off display power on i915\n");
> +
> +	return err;
>   }
>   
>   static int hdac_hdmi_runtime_resume(struct device *dev)
> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> index 670ff9aaca55..c62e474717ff 100644
> --- a/sound/soc/intel/skylake/skl.c
> +++ b/sound/soc/intel/skylake/skl.c
> @@ -762,6 +762,12 @@ static void skl_probe_work(struct work_struct *work)
>   		}
>   	}
>   
> +	/*
> +	 * we are done probing so decrement link counts
> +	 */
> +	list_for_each_entry(hlink, &ebus->hlink_list, list)
> +		snd_hdac_ext_bus_link_put(ebus, hlink);
> +
>   	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
>   		err = snd_hdac_display_power(bus, false);
>   		if (err < 0) {
> @@ -771,11 +777,6 @@ static void skl_probe_work(struct work_struct *work)
>   		}
>   	}
>   
> -	/*
> -	 * we are done probing so decrement link counts
> -	 */
> -	list_for_each_entry(hlink, &ebus->hlink_list, list)
> -		snd_hdac_ext_bus_link_put(ebus, hlink);
>   
>   	/* configure PM */
>   	pm_runtime_put_noidle(bus->dev);
> 

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Takashi Iwai]

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


thanks,

Takashi

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Sriram Periyasamy]

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.

Thanks,
Sriram.

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Takashi Iwai]

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?
It'd be helpful if you can illustrate the PM scheme in a big picture.


thanks,

Takashi

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Sriram Periyasamy]

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.

> It'd be helpful if you can illustrate the PM scheme in a big picture.
>

We will add separate patch for providing PM sequence.

Thanks,
Sriram.

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Takashi Iwai]

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.  The runtime
suspend doesn't call snd_hdac_display_power() in skl.c.

> > It'd be helpful if you can illustrate the PM scheme in a big picture.
> >
> 
> We will add separate patch for providing PM sequence.

Well, no patch is needed but rather an explanation is...


thanks,

Takashi

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Sriram Periyasamy]

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.

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.

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

Thanks,
Sriram.

Re: [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

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Sriram Periyasamy]

On Mon, Jun 25, 2018 at 05:18:27PM +0200, Takashi Iwai wrote:
> 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.
> 

Yes, i915 power well refcount goes to zero when system goes to runtime
suspend. The value of bus->i915_power_refcount in snd_hdac_display_power
is zero.

> > 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?
>

No, snd_hdac_display_power is invoked in skl.c for the following cases,

skl_probe_work - display power on, enumurate the codec and display power
                 off
skl_suspend & skl_resume - display power off/on for S3/S0 cycle 
	                   respectively

snd_hdac_display_power is invoked in hdac_hdmi.c for the following cases,

hdac_hdmi_dev_probe - display power on, probe the device and put the
		      device to runtime suspend explicitly

runtime suspend & resume - display power off and on respectively

Thanks,
Sriram.

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Takashi Iwai]

On Tue, 26 Jun 2018 13:24:01 +0200,
Sriram Periyasamy wrote:
> 
> On Mon, Jun 25, 2018 at 05:18:27PM +0200, Takashi Iwai wrote:
> > 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.
> > 
> 
> Yes, i915 power well refcount goes to zero when system goes to runtime
> suspend. The value of bus->i915_power_refcount in snd_hdac_display_power
> is zero.
> 
> > > 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?
> >
> 
> No, snd_hdac_display_power is invoked in skl.c for the following cases,
> 
> skl_probe_work - display power on, enumurate the codec and display power
>                  off
> skl_suspend & skl_resume - display power off/on for S3/S0 cycle 
> 	                   respectively
> 
> snd_hdac_display_power is invoked in hdac_hdmi.c for the following cases,
> 
> hdac_hdmi_dev_probe - display power on, probe the device and put the
> 		      device to runtime suspend explicitly
> 
> runtime suspend & resume - display power off and on respectively

That puzzled me.  How does display refcount goes to zero at runtime
suspend of skl?  skl_suspend() is only for S3, not for runtime PM.

Suppose you probe skl, hdac_hdmi, then do runtime suspend hdac_hdmi
and skl respectively.  How does the display pm refcount go to zero?


Takashi

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Sriram Periyasamy]

On Tue, Jun 26, 2018 at 01:33:36PM +0200, Takashi Iwai wrote:
> On Tue, 26 Jun 2018 13:24:01 +0200,
> Sriram Periyasamy wrote:
> > 
> > On Mon, Jun 25, 2018 at 05:18:27PM +0200, Takashi Iwai wrote:
> > > 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.
> > > 
> > 
> > Yes, i915 power well refcount goes to zero when system goes to runtime
> > suspend. The value of bus->i915_power_refcount in snd_hdac_display_power
> > is zero.
> > 
> > > > 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?
> > >
> > 
> > No, snd_hdac_display_power is invoked in skl.c for the following cases,
> > 
> > skl_probe_work - display power on, enumurate the codec and display power
> >                  off
> > skl_suspend & skl_resume - display power off/on for S3/S0 cycle 
> > 	                   respectively
> > 
> > snd_hdac_display_power is invoked in hdac_hdmi.c for the following cases,
> > 
> > hdac_hdmi_dev_probe - display power on, probe the device and put the
> > 		      device to runtime suspend explicitly
> > 
> > runtime suspend & resume - display power off and on respectively
> 
> That puzzled me.  How does display refcount goes to zero at runtime
> suspend of skl?  skl_suspend() is only for S3, not for runtime PM.
> 

Runtime suspend and resume of skylake doesn’t perform reference counting
of display. For an HDMI playback stream, below is the sequence.

Open:
1. skylake runtime resume
2. hdac_hdmi runtime resume --> ref count is incremented

Close:
1. hdac_hdmi runtime suspend --> ref count is decremented
2. skylake runtime suspend

> Suppose you probe skl, hdac_hdmi, then do runtime suspend hdac_hdmi
> and skl respectively.  How does the display pm refcount go to zero?
>

In skylake probe, the display ref count is incremented at start of probe
and decremented at end of probe. The ref count decrement is not
performed in skylake runtime suspend. In hdac_hdmi probe, the display
refcount is incremented in probe, and decremented in hdac_hdmi runtime
suspend.

Thanks,
Sriram.
 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Takashi Iwai]

On Wed, 27 Jun 2018 10:35:35 +0200,
Sriram Periyasamy wrote:
> 
> On Tue, Jun 26, 2018 at 01:33:36PM +0200, Takashi Iwai wrote:
> > On Tue, 26 Jun 2018 13:24:01 +0200,
> > Sriram Periyasamy wrote:
> > > 
> > > On Mon, Jun 25, 2018 at 05:18:27PM +0200, Takashi Iwai wrote:
> > > > 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.
> > > > 
> > > 
> > > Yes, i915 power well refcount goes to zero when system goes to runtime
> > > suspend. The value of bus->i915_power_refcount in snd_hdac_display_power
> > > is zero.
> > > 
> > > > > 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?
> > > >
> > > 
> > > No, snd_hdac_display_power is invoked in skl.c for the following cases,
> > > 
> > > skl_probe_work - display power on, enumurate the codec and display power
> > >                  off
> > > skl_suspend & skl_resume - display power off/on for S3/S0 cycle 
> > > 	                   respectively
> > > 
> > > snd_hdac_display_power is invoked in hdac_hdmi.c for the following cases,
> > > 
> > > hdac_hdmi_dev_probe - display power on, probe the device and put the
> > > 		      device to runtime suspend explicitly
> > > 
> > > runtime suspend & resume - display power off and on respectively
> > 
> > That puzzled me.  How does display refcount goes to zero at runtime
> > suspend of skl?  skl_suspend() is only for S3, not for runtime PM.
> > 
> 
> Runtime suspend and resume of skylake doesn’t perform reference counting
> of display. For an HDMI playback stream, below is the sequence.
> 
> Open:
> 1. skylake runtime resume
> 2. hdac_hdmi runtime resume --> ref count is incremented
> 
> Close:
> 1. hdac_hdmi runtime suspend --> ref count is decremented
> 2. skylake runtime suspend
> 
> > Suppose you probe skl, hdac_hdmi, then do runtime suspend hdac_hdmi
> > and skl respectively.  How does the display pm refcount go to zero?
> >
> 
> In skylake probe, the display ref count is incremented at start of probe
> and decremented at end of probe. The ref count decrement is not
> performed in skylake runtime suspend. In hdac_hdmi probe, the display
> refcount is incremented in probe, and decremented in hdac_hdmi runtime
> suspend.

Well, the display refcount is incremented in S3 resume in skl.c.
Who decrements it at runtime suspend?


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Sriram Periyasamy]

On Wed, Jun 27, 2018 at 10:48:05AM +0200, Takashi Iwai wrote:
> On Wed, 27 Jun 2018 10:35:35 +0200,
> Sriram Periyasamy wrote:
> > 
> > On Tue, Jun 26, 2018 at 01:33:36PM +0200, Takashi Iwai wrote:
> > > On Tue, 26 Jun 2018 13:24:01 +0200,
> > > Sriram Periyasamy wrote:
> > > > 
> > > > On Mon, Jun 25, 2018 at 05:18:27PM +0200, Takashi Iwai wrote:
> > > > > 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.
> > > > > 
> > > > 
> > > > Yes, i915 power well refcount goes to zero when system goes to runtime
> > > > suspend. The value of bus->i915_power_refcount in snd_hdac_display_power
> > > > is zero.
> > > > 
> > > > > > 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?
> > > > >
> > > > 
> > > > No, snd_hdac_display_power is invoked in skl.c for the following cases,
> > > > 
> > > > skl_probe_work - display power on, enumurate the codec and display power
> > > >                  off
> > > > skl_suspend & skl_resume - display power off/on for S3/S0 cycle 
> > > > 	                   respectively
> > > > 
> > > > snd_hdac_display_power is invoked in hdac_hdmi.c for the following cases,
> > > > 
> > > > hdac_hdmi_dev_probe - display power on, probe the device and put the
> > > > 		      device to runtime suspend explicitly
> > > > 
> > > > runtime suspend & resume - display power off and on respectively
> > > 
> > > That puzzled me.  How does display refcount goes to zero at runtime
> > > suspend of skl?  skl_suspend() is only for S3, not for runtime PM.
> > > 
> > 
> > Runtime suspend and resume of skylake doesn’t perform reference counting
> > of display. For an HDMI playback stream, below is the sequence.
> > 
> > Open:
> > 1. skylake runtime resume
> > 2. hdac_hdmi runtime resume --> ref count is incremented
> > 
> > Close:
> > 1. hdac_hdmi runtime suspend --> ref count is decremented
> > 2. skylake runtime suspend
> > 
> > > Suppose you probe skl, hdac_hdmi, then do runtime suspend hdac_hdmi
> > > and skl respectively.  How does the display pm refcount go to zero?
> > >
> > 
> > In skylake probe, the display ref count is incremented at start of probe
> > and decremented at end of probe. The ref count decrement is not
> > performed in skylake runtime suspend. In hdac_hdmi probe, the display
> > refcount is incremented in probe, and decremented in hdac_hdmi runtime
> > suspend.
> 
> Well, the display refcount is incremented in S3 resume in skl.c.
> Who decrements it at runtime suspend?
>

It is decremented at runtime suspend of codec hdac_hdmi_runtime_suspend().
Below are sequence,

S3 cycle without playback going on,

Entering S3,

1. hdmi_codec_prepare() invoke the runtime resume of codec
2. hdac_hdmi_runtime_resume() --> increments the refcount 
3. skl_suspend() -- > decrements the refcount

Wake up from S3,

1. skl_resume() --> increments the refcount 
2. hdmi_codec_complete() invokes the runtime suspend of codec 
3. hdac_hdmi_runtime_suspend() -- > decrements the refcount

S3 cycle with playback going on where the device is runtime active as the
sequence mentioned above and refcount is 1 already,

Entering S3,

1. hdmi_codec_prepare() invoke the runtime resume of codec which just
increments the pm runtime usage count of the codec
2. skl_suspend() -- > decrements the refcount

Wake up from S3,

1. skl_resume() --> increments the refcount 
2. hdmi_codec_complete() invokes the runtime suspend of codec which just
decrements the pm runtime usage count of the codec

Once playback is stopped, the refcount back to 0.

Thanks,
Sriram.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Takashi Iwai]

On Wed, 27 Jun 2018 11:17:55 +0200,
Sriram Periyasamy wrote:
> 
> On Wed, Jun 27, 2018 at 10:48:05AM +0200, Takashi Iwai wrote:
> > On Wed, 27 Jun 2018 10:35:35 +0200,
> > Sriram Periyasamy wrote:
> > > 
> > > On Tue, Jun 26, 2018 at 01:33:36PM +0200, Takashi Iwai wrote:
> > > > On Tue, 26 Jun 2018 13:24:01 +0200,
> > > > Sriram Periyasamy wrote:
> > > > > 
> > > > > On Mon, Jun 25, 2018 at 05:18:27PM +0200, Takashi Iwai wrote:
> > > > > > 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.
> > > > > > 
> > > > > 
> > > > > Yes, i915 power well refcount goes to zero when system goes to runtime
> > > > > suspend. The value of bus->i915_power_refcount in snd_hdac_display_power
> > > > > is zero.
> > > > > 
> > > > > > > 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?
> > > > > >
> > > > > 
> > > > > No, snd_hdac_display_power is invoked in skl.c for the following cases,
> > > > > 
> > > > > skl_probe_work - display power on, enumurate the codec and display power
> > > > >                  off
> > > > > skl_suspend & skl_resume - display power off/on for S3/S0 cycle 
> > > > > 	                   respectively
> > > > > 
> > > > > snd_hdac_display_power is invoked in hdac_hdmi.c for the following cases,
> > > > > 
> > > > > hdac_hdmi_dev_probe - display power on, probe the device and put the
> > > > > 		      device to runtime suspend explicitly
> > > > > 
> > > > > runtime suspend & resume - display power off and on respectively
> > > > 
> > > > That puzzled me.  How does display refcount goes to zero at runtime
> > > > suspend of skl?  skl_suspend() is only for S3, not for runtime PM.
> > > > 
> > > 
> > > Runtime suspend and resume of skylake doesn’t perform reference counting
> > > of display. For an HDMI playback stream, below is the sequence.
> > > 
> > > Open:
> > > 1. skylake runtime resume
> > > 2. hdac_hdmi runtime resume --> ref count is incremented
> > > 
> > > Close:
> > > 1. hdac_hdmi runtime suspend --> ref count is decremented
> > > 2. skylake runtime suspend
> > > 
> > > > Suppose you probe skl, hdac_hdmi, then do runtime suspend hdac_hdmi
> > > > and skl respectively.  How does the display pm refcount go to zero?
> > > >
> > > 
> > > In skylake probe, the display ref count is incremented at start of probe
> > > and decremented at end of probe. The ref count decrement is not
> > > performed in skylake runtime suspend. In hdac_hdmi probe, the display
> > > refcount is incremented in probe, and decremented in hdac_hdmi runtime
> > > suspend.
> > 
> > Well, the display refcount is incremented in S3 resume in skl.c.
> > Who decrements it at runtime suspend?
> >
> 
> It is decremented at runtime suspend of codec hdac_hdmi_runtime_suspend().
> Below are sequence,
> 
> S3 cycle without playback going on,
> 
> Entering S3,
> 
> 1. hdmi_codec_prepare() invoke the runtime resume of codec
> 2. hdac_hdmi_runtime_resume() --> increments the refcount 
> 3. skl_suspend() -- > decrements the refcount
> 
> Wake up from S3,
> 
> 1. skl_resume() --> increments the refcount 
> 2. hdmi_codec_complete() invokes the runtime suspend of codec 
> 3. hdac_hdmi_runtime_suspend() -- > decrements the refcount

So you're calling the runtime suspend even though it's not runtime
resumed?  That confuses readers and needs a clear explanation
somewhere in the code.

And what's the case if multiple codecs are present?


Takashi

> S3 cycle with playback going on where the device is runtime active as the
> sequence mentioned above and refcount is 1 already,
> 
> Entering S3,
> 
> 1. hdmi_codec_prepare() invoke the runtime resume of codec which just
> increments the pm runtime usage count of the codec
> 2. skl_suspend() -- > decrements the refcount
> 
> Wake up from S3,
> 
> 1. skl_resume() --> increments the refcount 
> 2. hdmi_codec_complete() invokes the runtime suspend of codec which just
> decrements the pm runtime usage count of the codec
> 
> Once playback is stopped, the refcount back to 0.
> 
> Thanks,
> Sriram.
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Sriram Periyasamy]

On Wed, Jun 27, 2018 at 11:34:28AM +0200, Takashi Iwai wrote:
> On Wed, 27 Jun 2018 11:17:55 +0200,
> Sriram Periyasamy wrote:
> > 
> > On Wed, Jun 27, 2018 at 10:48:05AM +0200, Takashi Iwai wrote:
> > > On Wed, 27 Jun 2018 10:35:35 +0200,
> > > Sriram Periyasamy wrote:
> > > > 
> > > > On Tue, Jun 26, 2018 at 01:33:36PM +0200, Takashi Iwai wrote:
> > > > > On Tue, 26 Jun 2018 13:24:01 +0200,
> > > > > Sriram Periyasamy wrote:
> > > > > > 
> > > > > > On Mon, Jun 25, 2018 at 05:18:27PM +0200, Takashi Iwai wrote:
> > > > > > > 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.
> > > > > > > 
> > > > > > 
> > > > > > Yes, i915 power well refcount goes to zero when system goes to runtime
> > > > > > suspend. The value of bus->i915_power_refcount in snd_hdac_display_power
> > > > > > is zero.
> > > > > > 
> > > > > > > > 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?
> > > > > > >
> > > > > > 
> > > > > > No, snd_hdac_display_power is invoked in skl.c for the following cases,
> > > > > > 
> > > > > > skl_probe_work - display power on, enumurate the codec and display power
> > > > > >                  off
> > > > > > skl_suspend & skl_resume - display power off/on for S3/S0 cycle 
> > > > > > 	                   respectively
> > > > > > 
> > > > > > snd_hdac_display_power is invoked in hdac_hdmi.c for the following cases,
> > > > > > 
> > > > > > hdac_hdmi_dev_probe - display power on, probe the device and put the
> > > > > > 		      device to runtime suspend explicitly
> > > > > > 
> > > > > > runtime suspend & resume - display power off and on respectively
> > > > > 
> > > > > That puzzled me.  How does display refcount goes to zero at runtime
> > > > > suspend of skl?  skl_suspend() is only for S3, not for runtime PM.
> > > > > 
> > > > 
> > > > Runtime suspend and resume of skylake doesn’t perform reference counting
> > > > of display. For an HDMI playback stream, below is the sequence.
> > > > 
> > > > Open:
> > > > 1. skylake runtime resume
> > > > 2. hdac_hdmi runtime resume --> ref count is incremented
> > > > 
> > > > Close:
> > > > 1. hdac_hdmi runtime suspend --> ref count is decremented
> > > > 2. skylake runtime suspend
> > > > 
> > > > > Suppose you probe skl, hdac_hdmi, then do runtime suspend hdac_hdmi
> > > > > and skl respectively.  How does the display pm refcount go to zero?
> > > > >
> > > > 
> > > > In skylake probe, the display ref count is incremented at start of probe
> > > > and decremented at end of probe. The ref count decrement is not
> > > > performed in skylake runtime suspend. In hdac_hdmi probe, the display
> > > > refcount is incremented in probe, and decremented in hdac_hdmi runtime
> > > > suspend.
> > > 
> > > Well, the display refcount is incremented in S3 resume in skl.c.
> > > Who decrements it at runtime suspend?
> > >
> > 
> > It is decremented at runtime suspend of codec hdac_hdmi_runtime_suspend().
> > Below are sequence,
> > 
> > S3 cycle without playback going on,
> > 
> > Entering S3,
> > 
> > 1. hdmi_codec_prepare() invoke the runtime resume of codec
> > 2. hdac_hdmi_runtime_resume() --> increments the refcount 
> > 3. skl_suspend() -- > decrements the refcount
> > 
> > Wake up from S3,
> > 
> > 1. skl_resume() --> increments the refcount 
> > 2. hdmi_codec_complete() invokes the runtime suspend of codec 
> > 3. hdac_hdmi_runtime_suspend() -- > decrements the refcount
> 
> So you're calling the runtime suspend even though it's not runtime
> resumed?  That confuses readers and needs a clear explanation
> somewhere in the code.
>

Yes, we will add a separate patch for this.

> And what's the case if multiple codecs are present?
>

We need to check on multiple codecs scenario. Do you think any issues
upfront?

Thanks,
Sriram.

> 
> > S3 cycle with playback going on where the device is runtime active as the
> > sequence mentioned above and refcount is 1 already,
> > 
> > Entering S3,
> > 
> > 1. hdmi_codec_prepare() invoke the runtime resume of codec which just
> > increments the pm runtime usage count of the codec
> > 2. skl_suspend() -- > decrements the refcount
> > 
> > Wake up from S3,
> > 
> > 1. skl_resume() --> increments the refcount 
> > 2. hdmi_codec_complete() invokes the runtime suspend of codec which just
> > decrements the pm runtime usage count of the codec
> > 
> > Once playback is stopped, the refcount back to 0.
> > 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Takashi Iwai]

On Wed, 27 Jun 2018 12:57:22 +0200,
Sriram Periyasamy wrote:
> 
> On Wed, Jun 27, 2018 at 11:34:28AM +0200, Takashi Iwai wrote:
> > On Wed, 27 Jun 2018 11:17:55 +0200,
> > Sriram Periyasamy wrote:
> > > 
> > > On Wed, Jun 27, 2018 at 10:48:05AM +0200, Takashi Iwai wrote:
> > > > On Wed, 27 Jun 2018 10:35:35 +0200,
> > > > Sriram Periyasamy wrote:
> > > > > 
> > > > > On Tue, Jun 26, 2018 at 01:33:36PM +0200, Takashi Iwai wrote:
> > > > > > On Tue, 26 Jun 2018 13:24:01 +0200,
> > > > > > Sriram Periyasamy wrote:
> > > > > > > 
> > > > > > > On Mon, Jun 25, 2018 at 05:18:27PM +0200, Takashi Iwai wrote:
> > > > > > > > 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.
> > > > > > > > 
> > > > > > > 
> > > > > > > Yes, i915 power well refcount goes to zero when system goes to runtime
> > > > > > > suspend. The value of bus->i915_power_refcount in snd_hdac_display_power
> > > > > > > is zero.
> > > > > > > 
> > > > > > > > > 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?
> > > > > > > >
> > > > > > > 
> > > > > > > No, snd_hdac_display_power is invoked in skl.c for the following cases,
> > > > > > > 
> > > > > > > skl_probe_work - display power on, enumurate the codec and display power
> > > > > > >                  off
> > > > > > > skl_suspend & skl_resume - display power off/on for S3/S0 cycle 
> > > > > > > 	                   respectively
> > > > > > > 
> > > > > > > snd_hdac_display_power is invoked in hdac_hdmi.c for the following cases,
> > > > > > > 
> > > > > > > hdac_hdmi_dev_probe - display power on, probe the device and put the
> > > > > > > 		      device to runtime suspend explicitly
> > > > > > > 
> > > > > > > runtime suspend & resume - display power off and on respectively
> > > > > > 
> > > > > > That puzzled me.  How does display refcount goes to zero at runtime
> > > > > > suspend of skl?  skl_suspend() is only for S3, not for runtime PM.
> > > > > > 
> > > > > 
> > > > > Runtime suspend and resume of skylake doesn’t perform reference counting
> > > > > of display. For an HDMI playback stream, below is the sequence.
> > > > > 
> > > > > Open:
> > > > > 1. skylake runtime resume
> > > > > 2. hdac_hdmi runtime resume --> ref count is incremented
> > > > > 
> > > > > Close:
> > > > > 1. hdac_hdmi runtime suspend --> ref count is decremented
> > > > > 2. skylake runtime suspend
> > > > > 
> > > > > > Suppose you probe skl, hdac_hdmi, then do runtime suspend hdac_hdmi
> > > > > > and skl respectively.  How does the display pm refcount go to zero?
> > > > > >
> > > > > 
> > > > > In skylake probe, the display ref count is incremented at start of probe
> > > > > and decremented at end of probe. The ref count decrement is not
> > > > > performed in skylake runtime suspend. In hdac_hdmi probe, the display
> > > > > refcount is incremented in probe, and decremented in hdac_hdmi runtime
> > > > > suspend.
> > > > 
> > > > Well, the display refcount is incremented in S3 resume in skl.c.
> > > > Who decrements it at runtime suspend?
> > > >
> > > 
> > > It is decremented at runtime suspend of codec hdac_hdmi_runtime_suspend().
> > > Below are sequence,
> > > 
> > > S3 cycle without playback going on,
> > > 
> > > Entering S3,
> > > 
> > > 1. hdmi_codec_prepare() invoke the runtime resume of codec
> > > 2. hdac_hdmi_runtime_resume() --> increments the refcount 
> > > 3. skl_suspend() -- > decrements the refcount
> > > 
> > > Wake up from S3,
> > > 
> > > 1. skl_resume() --> increments the refcount 
> > > 2. hdmi_codec_complete() invokes the runtime suspend of codec 
> > > 3. hdac_hdmi_runtime_suspend() -- > decrements the refcount
> > 
> > So you're calling the runtime suspend even though it's not runtime
> > resumed?  That confuses readers and needs a clear explanation
> > somewhere in the code.
> >
> 
> Yes, we will add a separate patch for this.

That'd be helpful.

> > And what's the case if multiple codecs are present?
> >
> 
> We need to check on multiple codecs scenario. Do you think any issues
> upfront?

No, and maybe my question was moot - it's only a single GPU, so always
a single codec, supposedly.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Intel: Power down links before turning off display audio power

[Pierre-Louis Bossart]

>   
> -	return 0;
> +	err = snd_hdac_display_power(bus, false);
> +	if (err < 0) {
> +		dev_err(dev, "Cannot turn off display power on i915\n");
> +		return err;
> +	}
> +
> +	return err;

oops, no need for these two returns. this was a bad copy/paste that 
wasn't detected in the reviews.

please ignore this one, will send a v2 shortly.

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

[Pierre-Louis Bossart]

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.

This patch was updated from an earlier version submitted upstream [1]
which conflicted with the changes merged for HDaudio codec support
with the Intel DSP.

[1] https://patchwork.kernel.org/patch/10540213/

Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/codecs/hdac_hdmi.c  | 13 +++++++------
 sound/soc/intel/skylake/skl.c | 12 ++++++------
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 4e9854889a95..e0fe682cea04 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -2187,11 +2187,6 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
 	 */
 	snd_hdac_codec_read(hdev, hdev->afg, 0,	AC_VERB_SET_POWER_STATE,
 							AC_PWRST_D3);
-	err = snd_hdac_display_power(bus, false);
-	if (err < 0) {
-		dev_err(dev, "Cannot turn on display power on i915\n");
-		return err;
-	}
 
 	hlink = snd_hdac_ext_bus_get_link(bus, dev_name(dev));
 	if (!hlink) {
@@ -2201,7 +2196,13 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
 
 	snd_hdac_ext_bus_link_put(bus, hlink);
 
-	return 0;
+	err = snd_hdac_display_power(bus, false);
+	if (err < 0) {
+		dev_err(dev, "Cannot turn off display power on i915\n");
+		return err;
+	}
+
+	return err;
 }
 
 static int hdac_hdmi_runtime_resume(struct device *dev)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 29225623b4b4..1586c97d9450 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -815,6 +815,12 @@ static void skl_probe_work(struct work_struct *work)
 		}
 	}
 
+	/*
+	 * we are done probing so decrement link counts
+	 */
+	list_for_each_entry(hlink, &bus->hlink_list, list)
+		snd_hdac_ext_bus_link_put(bus, hlink);
+
 	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
 		err = snd_hdac_display_power(bus, false);
 		if (err < 0) {
@@ -824,12 +830,6 @@ static void skl_probe_work(struct work_struct *work)
 		}
 	}
 
-	/*
-	 * we are done probing so decrement link counts
-	 */
-	list_for_each_entry(hlink, &bus->hlink_list, list)
-		snd_hdac_ext_bus_link_put(bus, hlink);
-
 	/* configure PM */
 	pm_runtime_put_noidle(bus->dev);
 	pm_runtime_allow(bus->dev);
-- 
2.17.1