[RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[libin.yang]

From: Libin Yang <libin.yang@intel.com>

hdmi_codec_prepare() will trigger hdmi runtime resume, which will set
the bitmask of hdev->addr. And skl_suspend() will clear the bitmask of
HDA_CODEC_IDX_CONTROLLER. HDMI codec idx is not the same as
HDA_CODEC_IDX_CONTROLLER, which means i915 power will not be released
when suspend.

On the other hand, hdmi_codec_prepare() don't need to call
pm_runtime_get_sync() to wake up the audio subsystem (HDMI auido)
for setting the codec registers. Turning display power on with
snd_hdac_display_power() is enough.

Let's use S3 without playback as an example:
hdmi_codec_prepare() invokes the runtime resume of codec =>
  snd_hdac_display_power(bus, hdev->addr, true)
skl runtime resume
skl_suspend() =>
  snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);

THis means hdev->addr will never release the display power when
suspend.

The new sequence will be:
hdmi_codec_prepare() =>
  snd_hdac_display_power(bus, hdev->addr, true)
  snd_hdac_display_power(bus, hdev->addr, false)
skl runtime resume
skl suspned

Signed-off-by: Libin Yang <libin.yang@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c  | 6 ++++--
 sound/soc/intel/skylake/skl.c | 7 -------
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 3ab2949..782b323 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -1895,7 +1895,7 @@ static int hdmi_codec_prepare(struct device *dev)
 {
 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
 
-	pm_runtime_get_sync(&hdev->dev);
+	snd_hdac_display_power(hdev->bus, hdev->addr, true);
 
 	/*
 	 * Power down afg.
@@ -1906,6 +1906,7 @@ static int hdmi_codec_prepare(struct device *dev)
 	 */
 	snd_hdac_codec_read(hdev, hdev->afg, 0,	AC_VERB_SET_POWER_STATE,
 							AC_PWRST_D3);
+	snd_hdac_display_power(hdev->bus, hdev->addr, false);
 
 	return 0;
 }
@@ -1915,6 +1916,7 @@ static void hdmi_codec_complete(struct device *dev)
 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
 	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
 
+	snd_hdac_display_power(hdev->bus, hdev->addr, true);
 	/* Power up afg */
 	snd_hdac_codec_read(hdev, hdev->afg, 0,	AC_VERB_SET_POWER_STATE,
 							AC_PWRST_D0);
@@ -1930,7 +1932,7 @@ static void hdmi_codec_complete(struct device *dev)
 	 */
 	hdac_hdmi_present_sense_all_pins(hdev, hdmi, false);
 
-	pm_runtime_put_sync(&hdev->dev);
+	snd_hdac_display_power(hdev->bus, hdev->addr, false);
 }
 #else
 #define hdmi_codec_prepare NULL
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 60c9483..89f4d66 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -336,9 +336,6 @@ static int skl_suspend(struct device *dev)
 		skl->skl_sst->fw_loaded = false;
 	}
 
-	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
-		snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
-
 	return 0;
 }
 
@@ -350,10 +347,6 @@ static int skl_resume(struct device *dev)
 	struct hdac_ext_link *hlink = NULL;
 	int ret;
 
-	/* Turned OFF in HDMI codec driver after codec reconfiguration */
-	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
-		snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
-
 	/*
 	 * resume only when we are not in suspend active, otherwise need to
 	 * restore the device
-- 
2.7.4

[RFC PATCH v2 2/2] ASoC: add asoc hdmi audio trigger handler

[libin.yang]

From: Libin Yang <libin.yang@intel.com>

The flow of ASoC hdmi audio suspend when playback is:
hdmi_codec_prepare() =>
  snd_hdac_display_power(bus, hdev->addr, true)
  snd_hdac_display_power(bus, hdev->addr, false)
suspend =>
  trigger(suspend) =>
    snd_hdac_display_power(bus, hdev->addr + HDA_CODEC_RT, false)
now, all bitmasks are cleared and display power is turned off.

Signed-off-by: Libin Yang <libin.yang@intel.com>
---
 include/sound/hda_component.h |  1 +
 sound/soc/codecs/hdac_hdmi.c  | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/include/sound/hda_component.h b/include/sound/hda_component.h
index 2ec31b3..c7136b1 100644
--- a/include/sound/hda_component.h
+++ b/include/sound/hda_component.h
@@ -9,6 +9,7 @@
 
 /* virtual idx for controller */
 #define HDA_CODEC_IDX_CONTROLLER	HDA_MAX_CODECS
+#define HDA_CODEC_RT		    (HDA_CODEC_IDX_CONTROLLER + 1)
 
 #ifdef CONFIG_SND_HDA_COMPONENT
 int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable);
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 782b323..908f83d 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -1383,11 +1383,43 @@ static void hdac_hdmi_skl_enable_dp12(struct hdac_device *hdev)
 
 }
 
+static int hdac_hdmi_pcm_trigger(struct snd_pcm_substream *substream, int cmd,
+				 struct snd_soc_dai *dai)
+{
+	struct hdac_hdmi_priv *hdmi = snd_soc_dai_get_drvdata(dai);
+	struct hdac_device *hdev = hdmi->hdev;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		/*
+		 * Maybe there is a better way to avoid using
+		 * 'hdev->addr + HDA_CODEC_RT' bitmask?
+		 */
+		snd_hdac_display_power(hdev->bus, hdev->addr + HDA_CODEC_RT,
+				       true);
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		snd_hdac_display_power(hdev->bus, hdev->addr + HDA_CODEC_RT,
+				       false);
+		break;
+	default:
+		/* Nothing to do */
+		break;
+	}
+
+	return 0;
+}
+
 static const struct snd_soc_dai_ops hdmi_dai_ops = {
 	.startup = hdac_hdmi_pcm_open,
 	.shutdown = hdac_hdmi_pcm_close,
 	.hw_params = hdac_hdmi_set_hw_params,
 	.set_tdm_slot = hdac_hdmi_set_tdm_slot,
+	.trigger = hdac_hdmi_pcm_trigger,
 };
 
 /*
@@ -1895,6 +1927,11 @@ static int hdmi_codec_prepare(struct device *dev)
 {
 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
 
+	/*
+	 * FIXME: maybe using suspend/resume instead of
+	 * using prepare/complete can make things simpler
+	 * and no need to use 'hdev->addr + HDA_CODEC_RT'?
+	 */
 	snd_hdac_display_power(hdev->bus, hdev->addr, true);
 
 	/*
-- 
2.7.4

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Pierre-Louis Bossart]

On 1/6/19 8:22 PM, libin.yang@intel.com wrote:
> From: Libin Yang <libin.yang@intel.com>
>
> hdmi_codec_prepare() will trigger hdmi runtime resume, which will set
> the bitmask of hdev->addr. And skl_suspend() will clear the bitmask of
> HDA_CODEC_IDX_CONTROLLER. HDMI codec idx is not the same as
> HDA_CODEC_IDX_CONTROLLER, which means i915 power will not be released
> when suspend.
>
> On the other hand, hdmi_codec_prepare() don't need to call
> pm_runtime_get_sync() to wake up the audio subsystem (HDMI auido)
> for setting the codec registers. Turning display power on with
> snd_hdac_display_power() is enough.
>
> Let's use S3 without playback as an example:
> hdmi_codec_prepare() invokes the runtime resume of codec =>
>    snd_hdac_display_power(bus, hdev->addr, true)
> skl runtime resume
> skl_suspend() =>
>    snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
>
> THis means hdev->addr will never release the display power when
> suspend.
>
> The new sequence will be:
> hdmi_codec_prepare() =>
>    snd_hdac_display_power(bus, hdev->addr, true)
>    snd_hdac_display_power(bus, hdev->addr, false)
> skl runtime resume
> skl suspned

I for one find this RFC difficult to follow. The documentation of "Power 
management sequences" seems obsolete after Takashi's changes, you may 
want to start with an update of the documentation which might help point 
out what is broken in the sequences. I also don't see the point in 
trying to bypass the runtime_pm code in codec_prepare and 
codec_complete. if we have the display on/off sequence only in 
probe/remove and suspend/resume it makes it easy to track states, and I 
wonder if it makes sense anyways to be in suspend with the display on or 
vice-versa in D0 with the display off. Simple state machines always win.

>
> Signed-off-by: Libin Yang <libin.yang@intel.com>
> ---
>   sound/soc/codecs/hdac_hdmi.c  | 6 ++++--
>   sound/soc/intel/skylake/skl.c | 7 -------
>   2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> index 3ab2949..782b323 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -1895,7 +1895,7 @@ static int hdmi_codec_prepare(struct device *dev)
>   {
>   	struct hdac_device *hdev = dev_to_hdac_dev(dev);
>   
> -	pm_runtime_get_sync(&hdev->dev);
> +	snd_hdac_display_power(hdev->bus, hdev->addr, true);
>   
>   	/*
>   	 * Power down afg.
> @@ -1906,6 +1906,7 @@ static int hdmi_codec_prepare(struct device *dev)
>   	 */
>   	snd_hdac_codec_read(hdev, hdev->afg, 0,	AC_VERB_SET_POWER_STATE,
>   							AC_PWRST_D3);
> +	snd_hdac_display_power(hdev->bus, hdev->addr, false);
>   
>   	return 0;
>   }
> @@ -1915,6 +1916,7 @@ static void hdmi_codec_complete(struct device *dev)
>   	struct hdac_device *hdev = dev_to_hdac_dev(dev);
>   	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
>   
> +	snd_hdac_display_power(hdev->bus, hdev->addr, true);
>   	/* Power up afg */
>   	snd_hdac_codec_read(hdev, hdev->afg, 0,	AC_VERB_SET_POWER_STATE,
>   							AC_PWRST_D0);
> @@ -1930,7 +1932,7 @@ static void hdmi_codec_complete(struct device *dev)
>   	 */
>   	hdac_hdmi_present_sense_all_pins(hdev, hdmi, false);
>   
> -	pm_runtime_put_sync(&hdev->dev);
> +	snd_hdac_display_power(hdev->bus, hdev->addr, false);
>   }
>   #else
>   #define hdmi_codec_prepare NULL
> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> index 60c9483..89f4d66 100644
> --- a/sound/soc/intel/skylake/skl.c
> +++ b/sound/soc/intel/skylake/skl.c
> @@ -336,9 +336,6 @@ static int skl_suspend(struct device *dev)
>   		skl->skl_sst->fw_loaded = false;
>   	}
>   
> -	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
> -		snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
> -
>   	return 0;
>   }
>   
> @@ -350,10 +347,6 @@ static int skl_resume(struct device *dev)
>   	struct hdac_ext_link *hlink = NULL;
>   	int ret;
>   
> -	/* Turned OFF in HDMI codec driver after codec reconfiguration */
> -	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
> -		snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
> -
>   	/*
>   	 * resume only when we are not in suspend active, otherwise need to
>   	 * restore the device

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Yang, Libin]

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>Sent: Tuesday, January 8, 2019 12:58 AM
>To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org;
>tiwai@suse.de; broonie@kernel.org
>Cc: liam.r.girdwood@linux.intel.com; Lin, Mengdong
><mengdong.lin@intel.com>
>Subject: Re: [alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio
>suspend/resume
>
>
>On 1/6/19 8:22 PM, libin.yang@intel.com wrote:
>> From: Libin Yang <libin.yang@intel.com>
>>
>> hdmi_codec_prepare() will trigger hdmi runtime resume, which will set
>> the bitmask of hdev->addr. And skl_suspend() will clear the bitmask of
>> HDA_CODEC_IDX_CONTROLLER. HDMI codec idx is not the same as
>> HDA_CODEC_IDX_CONTROLLER, which means i915 power will not be
>released
>> when suspend.
>>
>> On the other hand, hdmi_codec_prepare() don't need to call
>> pm_runtime_get_sync() to wake up the audio subsystem (HDMI auido) for
>> setting the codec registers. Turning display power on with
>> snd_hdac_display_power() is enough.
>>
>> Let's use S3 without playback as an example:
>> hdmi_codec_prepare() invokes the runtime resume of codec =>
>>    snd_hdac_display_power(bus, hdev->addr, true) skl runtime resume
>> skl_suspend() =>
>>    snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
>>
>> THis means hdev->addr will never release the display power when
>> suspend.
>>
>> The new sequence will be:
>> hdmi_codec_prepare() =>
>>    snd_hdac_display_power(bus, hdev->addr, true)
>>    snd_hdac_display_power(bus, hdev->addr, false) skl runtime resume
>> skl suspned
>
>I for one find this RFC difficult to follow. The documentation of "Power
>management sequences" seems obsolete after Takashi's changes, you may
>want to start with an update of the documentation which might help point out
>what is broken in the sequences. I also don't see the point in trying to bypass
>the runtime_pm code in codec_prepare and codec_complete. if we have the
>display on/off sequence only in probe/remove and suspend/resume it makes
>it easy to track states, and I wonder if it makes sense anyways to be in
>suspend with the display on or vice-versa in D0 with the display off. Simple
>state machines always win.

Thanks for comments. I will modify the documentation of "Power
Management sequences" when I submit the formal patch. The rough
flow is described in my patch description. Do you think I need add more
details?

Actually, the HDMI driver is a little stranger. pm runtime resume of HDMI
is not called even prepare() return 0. What I understand is that the runtime
resume should be called. I asked a friend of mine who is familiar with kernel
power management. He also can't tell why. Maybe our ALSA experts know
why :).

My patch is based on hdmi codec pm runtime resume not being called.

The normal flow of suspend should be: 
Pm runtime resume => power up
Pm suspend => power down.
Unfortunately, our ASoC HDMI codec driver doesn't have pm suspend
Callback (maybe this is why HDMI runtime resume is not called?)

In original code, the hdmi prepare() will call pm_runtime_get_sync() which
will trigger hdmi pm runtime resume, which will turn on display power:
snd_hdac_display_power(hdev->bus, hdev->addr, true). And
there is no chance to turn off the 'hdev->addr' display power before
suspend, which is wrong.

Regards,
Libin

>
>>
>> Signed-off-by: Libin Yang <libin.yang@intel.com>
>> ---
>>   sound/soc/codecs/hdac_hdmi.c  | 6 ++++--
>>   sound/soc/intel/skylake/skl.c | 7 -------
>>   2 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/sound/soc/codecs/hdac_hdmi.c
>> b/sound/soc/codecs/hdac_hdmi.c index 3ab2949..782b323 100644
>> --- a/sound/soc/codecs/hdac_hdmi.c
>> +++ b/sound/soc/codecs/hdac_hdmi.c
>> @@ -1895,7 +1895,7 @@ static int hdmi_codec_prepare(struct device *dev)
>>   {
>>   	struct hdac_device *hdev = dev_to_hdac_dev(dev);
>>
>> -	pm_runtime_get_sync(&hdev->dev);
>> +	snd_hdac_display_power(hdev->bus, hdev->addr, true);
>>
>>   	/*
>>   	 * Power down afg.
>> @@ -1906,6 +1906,7 @@ static int hdmi_codec_prepare(struct device *dev)
>>   	 */
>>   	snd_hdac_codec_read(hdev, hdev->afg, 0,
>	AC_VERB_SET_POWER_STATE,
>>   							AC_PWRST_D3);
>> +	snd_hdac_display_power(hdev->bus, hdev->addr, false);
>>
>>   	return 0;
>>   }
>> @@ -1915,6 +1916,7 @@ static void hdmi_codec_complete(struct device
>*dev)
>>   	struct hdac_device *hdev = dev_to_hdac_dev(dev);
>>   	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
>>
>> +	snd_hdac_display_power(hdev->bus, hdev->addr, true);
>>   	/* Power up afg */
>>   	snd_hdac_codec_read(hdev, hdev->afg, 0,
>	AC_VERB_SET_POWER_STATE,
>>   							AC_PWRST_D0);
>> @@ -1930,7 +1932,7 @@ static void hdmi_codec_complete(struct device
>*dev)
>>   	 */
>>   	hdac_hdmi_present_sense_all_pins(hdev, hdmi, false);
>>
>> -	pm_runtime_put_sync(&hdev->dev);
>> +	snd_hdac_display_power(hdev->bus, hdev->addr, false);
>>   }
>>   #else
>>   #define hdmi_codec_prepare NULL
>> diff --git a/sound/soc/intel/skylake/skl.c
>> b/sound/soc/intel/skylake/skl.c index 60c9483..89f4d66 100644
>> --- a/sound/soc/intel/skylake/skl.c
>> +++ b/sound/soc/intel/skylake/skl.c
>> @@ -336,9 +336,6 @@ static int skl_suspend(struct device *dev)
>>   		skl->skl_sst->fw_loaded = false;
>>   	}
>>
>> -	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
>> -		snd_hdac_display_power(bus,
>HDA_CODEC_IDX_CONTROLLER, false);
>> -
>>   	return 0;
>>   }
>>
>> @@ -350,10 +347,6 @@ static int skl_resume(struct device *dev)
>>   	struct hdac_ext_link *hlink = NULL;
>>   	int ret;
>>
>> -	/* Turned OFF in HDMI codec driver after codec reconfiguration */
>> -	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
>> -		snd_hdac_display_power(bus,
>HDA_CODEC_IDX_CONTROLLER, true);
>> -
>>   	/*
>>   	 * resume only when we are not in suspend active, otherwise need to
>>   	 * restore the device

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Takashi Iwai]

On Tue, 08 Jan 2019 08:53:49 +0100,
Yang, Libin wrote:
> 
> 
> >-----Original Message-----
> >From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
> >Sent: Tuesday, January 8, 2019 12:58 AM
> >To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org;
> >tiwai@suse.de; broonie@kernel.org
> >Cc: liam.r.girdwood@linux.intel.com; Lin, Mengdong
> ><mengdong.lin@intel.com>
> >Subject: Re: [alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio
> >suspend/resume
> >
> >
> >On 1/6/19 8:22 PM, libin.yang@intel.com wrote:
> >> From: Libin Yang <libin.yang@intel.com>
> >>
> >> hdmi_codec_prepare() will trigger hdmi runtime resume, which will set
> >> the bitmask of hdev->addr. And skl_suspend() will clear the bitmask of
> >> HDA_CODEC_IDX_CONTROLLER. HDMI codec idx is not the same as
> >> HDA_CODEC_IDX_CONTROLLER, which means i915 power will not be
> >released
> >> when suspend.
> >>
> >> On the other hand, hdmi_codec_prepare() don't need to call
> >> pm_runtime_get_sync() to wake up the audio subsystem (HDMI auido) for
> >> setting the codec registers. Turning display power on with
> >> snd_hdac_display_power() is enough.
> >>
> >> Let's use S3 without playback as an example:
> >> hdmi_codec_prepare() invokes the runtime resume of codec =>
> >>    snd_hdac_display_power(bus, hdev->addr, true) skl runtime resume
> >> skl_suspend() =>
> >>    snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
> >>
> >> THis means hdev->addr will never release the display power when
> >> suspend.
> >>
> >> The new sequence will be:
> >> hdmi_codec_prepare() =>
> >>    snd_hdac_display_power(bus, hdev->addr, true)
> >>    snd_hdac_display_power(bus, hdev->addr, false) skl runtime resume
> >> skl suspned
> >
> >I for one find this RFC difficult to follow. The documentation of "Power
> >management sequences" seems obsolete after Takashi's changes, you may
> >want to start with an update of the documentation which might help point out
> >what is broken in the sequences. I also don't see the point in trying to bypass
> >the runtime_pm code in codec_prepare and codec_complete. if we have the
> >display on/off sequence only in probe/remove and suspend/resume it makes
> >it easy to track states, and I wonder if it makes sense anyways to be in
> >suspend with the display on or vice-versa in D0 with the display off. Simple
> >state machines always win.
> 
> Thanks for comments. I will modify the documentation of "Power
> Management sequences" when I submit the formal patch. The rough
> flow is described in my patch description. Do you think I need add more
> details?
> 
> Actually, the HDMI driver is a little stranger. pm runtime resume of HDMI
> is not called even prepare() return 0. What I understand is that the runtime
> resume should be called. I asked a friend of mine who is familiar with kernel
> power management. He also can't tell why. Maybe our ALSA experts know
> why :).
> 
> My patch is based on hdmi codec pm runtime resume not being called.
> 
> The normal flow of suspend should be: 
> Pm runtime resume => power up
> Pm suspend => power down.
> Unfortunately, our ASoC HDMI codec driver doesn't have pm suspend
> Callback (maybe this is why HDMI runtime resume is not called?)
> 
> In original code, the hdmi prepare() will call pm_runtime_get_sync() which
> will trigger hdmi pm runtime resume, which will turn on display power:
> snd_hdac_display_power(hdev->bus, hdev->addr, true). And
> there is no chance to turn off the 'hdev->addr' display power before
> suspend, which is wrong.

Well, the problem is that HD-audio controller takes the display power
unnecessarily at suspend and resume.  Since the refactoring, this is
superfluous and confuses the system.

Also, I see no reason to stick with prepare and complete PM calls in
hdac_hdmi driver; the display power is managed in each domain now, so
we shouldn't rely on the refcount done by the controller driver any
longer.

Below is an untested patch I cooked up for simplification and fix for
this issue.  Could you check whether this works at all?


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ASoC: intel: skl: Fix display power regression

Since the refactoring of HD-audio display power management, the
display power status is managed per domain.  Meanwhile the ASoC
hdac_hdmi driver still keeps and relies (incorrectly) on the
refcounting together with ASoC skl driver, and this leads to the
display state always on.

This patch is an attempt to address the regression by simplifying the
PM code of ASoC skl and hdac_hdmi drivers.  Basically, since the
refactoring, we don't have to manage the display power at HD-audio
controller suspend / resume but only at HD-audio HDMI codec suspend /
resume.  So the patch drops the superfluous snd_hdac_display_power()
calls in skl driver.

Meanwhile, in hdac_hdmi side, we rewrite the PM call just to re-use
the runtime PM callbacks like other drivers do.  Now the logic is
simple: turn off at suspend and turn on at resume.

The patch also fixes the possibly missing display-power off at skl
driver removal as well as some error paths at probe.

Fixes: 029d92c289bd ("ALSA: hda: Refactor display power management")
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/hdac_hdmi.c  | 112 +++-------------------------------
 sound/soc/intel/skylake/skl.c |  13 ++--
 2 files changed, 13 insertions(+), 112 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 3ab2949c1dfa..cb4668b82882 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -1890,38 +1890,16 @@ static void hdmi_codec_remove(struct snd_soc_component *component)
 	pm_runtime_disable(&hdev->dev);
 }
 
-#ifdef CONFIG_PM
-static int hdmi_codec_prepare(struct device *dev)
-{
-	struct hdac_device *hdev = dev_to_hdac_dev(dev);
-
-	pm_runtime_get_sync(&hdev->dev);
-
-	/*
-	 * Power down afg.
-	 * codec_read is preferred over codec_write to set the power state.
-	 * This way verb is send to set the power state and response
-	 * is received. So setting power state is ensured without using loop
-	 * to read the state.
-	 */
-	snd_hdac_codec_read(hdev, hdev->afg, 0,	AC_VERB_SET_POWER_STATE,
-							AC_PWRST_D3);
-
-	return 0;
-}
-
-static void hdmi_codec_complete(struct device *dev)
+#ifdef CONFIG_PM_SLEEP
+static int hdmi_codec_resume(struct device *dev)
 {
 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
 	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
+	int ret;
 
-	/* Power up afg */
-	snd_hdac_codec_read(hdev, hdev->afg, 0,	AC_VERB_SET_POWER_STATE,
-							AC_PWRST_D0);
-
-	hdac_hdmi_skl_enable_all_pins(hdev);
-	hdac_hdmi_skl_enable_dp12(hdev);
-
+	ret = pm_runtime_force_resume(dev);
+	if (ret < 0)
+		return ret;
 	/*
 	 * As the ELD notify callback request is not entertained while the
 	 * device is in suspend state. Need to manually check detection of
@@ -1929,12 +1907,10 @@ static void hdmi_codec_complete(struct device *dev)
 	 * already set pin caps.
 	 */
 	hdac_hdmi_present_sense_all_pins(hdev, hdmi, false);
-
-	pm_runtime_put_sync(&hdev->dev);
+	return 0;
 }
 #else
-#define hdmi_codec_prepare NULL
-#define hdmi_codec_complete NULL
+#define hdmi_codec_resume NULL
 #endif
 
 static const struct snd_soc_component_driver hdmi_hda_codec = {
@@ -2135,75 +2111,6 @@ static int hdac_hdmi_dev_remove(struct hdac_device *hdev)
 }
 
 #ifdef CONFIG_PM
-/*
- * Power management sequences
- * ==========================
- *
- * The following explains the PM handling of HDAC HDMI with its parent
- * device SKL and display power usage
- *
- * Probe
- * -----
- * In SKL probe,
- * 1. skl_probe_work() powers up the display (refcount++ -> 1)
- * 2. enumerates the codecs on the link
- * 3. powers down the display  (refcount-- -> 0)
- *
- * In HDAC HDMI probe,
- * 1. hdac_hdmi_dev_probe() powers up the display (refcount++ -> 1)
- * 2. probe the codec
- * 3. put the HDAC HDMI device to runtime suspend
- * 4. hdac_hdmi_runtime_suspend() powers down the display (refcount-- -> 0)
- *
- * Once children are runtime suspended, SKL device also goes to runtime
- * suspend
- *
- * HDMI Playback
- * -------------
- * Open HDMI device,
- * 1. skl_runtime_resume() invoked
- * 2. hdac_hdmi_runtime_resume() powers up the display (refcount++ -> 1)
- *
- * Close HDMI device,
- * 1. hdac_hdmi_runtime_suspend() powers down the display (refcount-- -> 0)
- * 2. skl_runtime_suspend() invoked
- *
- * S0/S3 Cycle with playback in progress
- * -------------------------------------
- * When the device is opened for playback, the device is runtime active
- * already and the display refcount is 1 as explained above.
- *
- * Entering to S3,
- * 1. hdmi_codec_prepare() invoke the runtime resume of codec which just
- *    increments the PM runtime usage count of the codec since the device
- *    is in use already
- * 2. skl_suspend() powers down the display (refcount-- -> 0)
- *
- * Wakeup from S3,
- * 1. skl_resume() powers up the display (refcount++ -> 1)
- * 2. hdmi_codec_complete() invokes the runtime suspend of codec which just
- *    decrements the PM runtime usage count of the codec since the device
- *    is in use already
- *
- * Once playback is stopped, the display refcount is set to 0 as explained
- * above in the HDMI playback sequence. The PM handlings are designed in
- * such way that to balance the refcount of display power when the codec
- * device put to S3 while playback is going on.
- *
- * S0/S3 Cycle without playback in progress
- * ----------------------------------------
- * Entering to S3,
- * 1. hdmi_codec_prepare() invoke the runtime resume of codec
- * 2. skl_runtime_resume() invoked
- * 3. hdac_hdmi_runtime_resume() powers up the display (refcount++ -> 1)
- * 4. skl_suspend() powers down the display (refcount-- -> 0)
- *
- * Wakeup from S3,
- * 1. skl_resume() powers up the display (refcount++ -> 1)
- * 2. hdmi_codec_complete() invokes the runtime suspend of codec
- * 3. hdac_hdmi_runtime_suspend() powers down the display (refcount-- -> 0)
- * 4. skl_runtime_suspend() invoked
- */
 static int hdac_hdmi_runtime_suspend(struct device *dev)
 {
 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
@@ -2277,8 +2184,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
 
 static const struct dev_pm_ops hdac_hdmi_pm = {
 	SET_RUNTIME_PM_OPS(hdac_hdmi_runtime_suspend, hdac_hdmi_runtime_resume, NULL)
-	.prepare = hdmi_codec_prepare,
-	.complete = hdmi_codec_complete,
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, hdmi_codec_resume)
 };
 
 static const struct hda_device_id hdmi_list[] = {
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 60c94836bf5b..4ed5b7e17d44 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -336,9 +336,6 @@ static int skl_suspend(struct device *dev)
 		skl->skl_sst->fw_loaded = false;
 	}
 
-	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
-		snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
-
 	return 0;
 }
 
@@ -350,10 +347,6 @@ static int skl_resume(struct device *dev)
 	struct hdac_ext_link *hlink = NULL;
 	int ret;
 
-	/* Turned OFF in HDMI codec driver after codec reconfiguration */
-	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
-		snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
-
 	/*
 	 * resume only when we are not in suspend active, otherwise need to
 	 * restore the device
@@ -446,8 +439,10 @@ static int skl_free(struct hdac_bus *bus)
 	snd_hdac_ext_bus_exit(bus);
 
 	cancel_work_sync(&skl->probe_work);
-	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
+	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
+		snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
 		snd_hdac_i915_exit(bus);
+	}
 
 	return 0;
 }
@@ -814,7 +809,7 @@ static void skl_probe_work(struct work_struct *work)
 	err = skl_platform_register(bus->dev);
 	if (err < 0) {
 		dev_err(bus->dev, "platform register failed: %d\n", err);
-		return;
+		goto out_err;
 	}
 
 	err = skl_machine_device_register(skl);
-- 
2.20.1

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Yang, Libin]

Hi Takashi,

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Tuesday, January 8, 2019 7:15 PM
>To: Yang, Libin <libin.yang@intel.com>
>Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; alsa-
>devel@alsa-project.org; broonie@kernel.org;
>liam.r.girdwood@linux.intel.com; Lin, Mengdong <mengdong.lin@intel.com>
>Subject: Re: [alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio
>suspend/resume
>
>On Tue, 08 Jan 2019 08:53:49 +0100,
>Yang, Libin wrote:
>>
>>
>> >-----Original Message-----
>> >From: Pierre-Louis Bossart
>> >[mailto:pierre-louis.bossart@linux.intel.com]
>> >Sent: Tuesday, January 8, 2019 12:58 AM
>> >To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org;
>> >tiwai@suse.de; broonie@kernel.org
>> >Cc: liam.r.girdwood@linux.intel.com; Lin, Mengdong
>> ><mengdong.lin@intel.com>
>> >Subject: Re: [alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi
>> >audio suspend/resume
>> >
>> >
>> >On 1/6/19 8:22 PM, libin.yang@intel.com wrote:
>> >> From: Libin Yang <libin.yang@intel.com>
>> >>
>> >> hdmi_codec_prepare() will trigger hdmi runtime resume, which will
>> >> set the bitmask of hdev->addr. And skl_suspend() will clear the
>> >> bitmask of HDA_CODEC_IDX_CONTROLLER. HDMI codec idx is not the
>same
>> >> as HDA_CODEC_IDX_CONTROLLER, which means i915 power will not be
>> >released
>> >> when suspend.
>> >>
>> >> On the other hand, hdmi_codec_prepare() don't need to call
>> >> pm_runtime_get_sync() to wake up the audio subsystem (HDMI auido)
>> >> for setting the codec registers. Turning display power on with
>> >> snd_hdac_display_power() is enough.
>> >>
>> >> Let's use S3 without playback as an example:
>> >> hdmi_codec_prepare() invokes the runtime resume of codec =>
>> >>    snd_hdac_display_power(bus, hdev->addr, true) skl runtime resume
>> >> skl_suspend() =>
>> >>    snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
>> >>
>> >> THis means hdev->addr will never release the display power when
>> >> suspend.
>> >>
>> >> The new sequence will be:
>> >> hdmi_codec_prepare() =>
>> >>    snd_hdac_display_power(bus, hdev->addr, true)
>> >>    snd_hdac_display_power(bus, hdev->addr, false) skl runtime
>> >> resume skl suspned
>> >
>> >I for one find this RFC difficult to follow. The documentation of
>> >"Power management sequences" seems obsolete after Takashi's changes,
>> >you may want to start with an update of the documentation which might
>> >help point out what is broken in the sequences. I also don't see the
>> >point in trying to bypass the runtime_pm code in codec_prepare and
>> >codec_complete. if we have the display on/off sequence only in
>> >probe/remove and suspend/resume it makes it easy to track states, and
>> >I wonder if it makes sense anyways to be in suspend with the display
>> >on or vice-versa in D0 with the display off. Simple state machines always
>win.
>>
>> Thanks for comments. I will modify the documentation of "Power
>> Management sequences" when I submit the formal patch. The rough flow
>> is described in my patch description. Do you think I need add more
>> details?
>>
>> Actually, the HDMI driver is a little stranger. pm runtime resume of
>> HDMI is not called even prepare() return 0. What I understand is that
>> the runtime resume should be called. I asked a friend of mine who is
>> familiar with kernel power management. He also can't tell why. Maybe
>> our ALSA experts know why :).
>>
>> My patch is based on hdmi codec pm runtime resume not being called.
>>
>> The normal flow of suspend should be:
>> Pm runtime resume => power up
>> Pm suspend => power down.
>> Unfortunately, our ASoC HDMI codec driver doesn't have pm suspend
>> Callback (maybe this is why HDMI runtime resume is not called?)
>>
>> In original code, the hdmi prepare() will call pm_runtime_get_sync()
>> which will trigger hdmi pm runtime resume, which will turn on display
>power:
>> snd_hdac_display_power(hdev->bus, hdev->addr, true). And there is no
>> chance to turn off the 'hdev->addr' display power before suspend,
>> which is wrong.
>
>Well, the problem is that HD-audio controller takes the display power
>unnecessarily at suspend and resume.  Since the refactoring, this is
>superfluous and confuses the system.
>
>Also, I see no reason to stick with prepare and complete PM calls in
>hdac_hdmi driver; the display power is managed in each domain now, so we
>shouldn't rely on the refcount done by the controller driver any longer.
>
>Below is an untested patch I cooked up for simplification and fix for this issue.
>Could you check whether this works at all?

This patch makes the flow more clear now. Thanks for help.

I have done the test on sof audio driver.
Because SOF has issue of suspend when playback, I didn't do the test
of suspend with hdmi playing. I only test the audio suspend/resume
without playing anything.

The test result is:
Suspend/resume works well.
And after suspend/resume, playback works well.

However, I don't have platform to test SST driver. I tried to setup 
the environment for SST driver test, but failed. I will continue to 
setup the SST environment. On the meantime, I will ask Intel internal
team to help on SST driver test if they are willing to help.

And please see my comments below.

[...]

>-
>-static void hdmi_codec_complete(struct device *dev)
>+#ifdef CONFIG_PM_SLEEP
>+static int hdmi_codec_resume(struct device *dev)
> {
> 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
> 	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
>+	int ret;
>
>-	/* Power up afg */
>-	snd_hdac_codec_read(hdev, hdev->afg, 0,
>	AC_VERB_SET_POWER_STATE,
>-							AC_PWRST_D0);
>-
>-	hdac_hdmi_skl_enable_all_pins(hdev);
>-	hdac_hdmi_skl_enable_dp12(hdev);
>-
>+	ret = pm_runtime_force_resume(dev);

The code hopes to call hdmi pm_runtime() whenever this resume()
is called? If so, I'm afraid it will not work. pm_runtime_force_resume()
calls pm_runtime with conditions. Sometimes pm_runtime will not
be called. In my suspend/resume test, it shows that pm_runtime() is
not called. This may cause the following function
hdac_hdmi_present_sense_all_pins() not work well as there is no
power on.

Regards,
Libin

>+	if (ret < 0)
>+		return ret;
> 	/*
> 	 * As the ELD notify callback request is not entertained while the
> 	 * device is in suspend state. Need to manually check detection of @@

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Takashi Iwai]

On Wed, 09 Jan 2019 09:16:34 +0100,
Yang, Libin wrote:
> 
> >-
> >-static void hdmi_codec_complete(struct device *dev)
> >+#ifdef CONFIG_PM_SLEEP
> >+static int hdmi_codec_resume(struct device *dev)
> > {
> > 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
> > 	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
> >+	int ret;
> >
> >-	/* Power up afg */
> >-	snd_hdac_codec_read(hdev, hdev->afg, 0,
> >	AC_VERB_SET_POWER_STATE,
> >-							AC_PWRST_D0);
> >-
> >-	hdac_hdmi_skl_enable_all_pins(hdev);
> >-	hdac_hdmi_skl_enable_dp12(hdev);
> >-
> >+	ret = pm_runtime_force_resume(dev);
> 
> The code hopes to call hdmi pm_runtime() whenever this resume()
> is called? If so, I'm afraid it will not work. pm_runtime_force_resume()
> calls pm_runtime with conditions. Sometimes pm_runtime will not
> be called. In my suspend/resume test, it shows that pm_runtime() is
> not called. This may cause the following function
> hdac_hdmi_present_sense_all_pins() not work well as there is no
> power on.

Hm, right, maybe we should move hdac_hdmi_present_sense_all_pins()
into runtime resume.  It may be called unconditionally, but safer to
call it conditionally only after S3 or so...


thanks,

Takashi

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Takashi Iwai]

On Wed, 09 Jan 2019 09:29:36 +0100,
Takashi Iwai wrote:
> 
> On Wed, 09 Jan 2019 09:16:34 +0100,
> Yang, Libin wrote:
> > 
> > >-
> > >-static void hdmi_codec_complete(struct device *dev)
> > >+#ifdef CONFIG_PM_SLEEP
> > >+static int hdmi_codec_resume(struct device *dev)
> > > {
> > > 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
> > > 	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
> > >+	int ret;
> > >
> > >-	/* Power up afg */
> > >-	snd_hdac_codec_read(hdev, hdev->afg, 0,
> > >	AC_VERB_SET_POWER_STATE,
> > >-							AC_PWRST_D0);
> > >-
> > >-	hdac_hdmi_skl_enable_all_pins(hdev);
> > >-	hdac_hdmi_skl_enable_dp12(hdev);
> > >-
> > >+	ret = pm_runtime_force_resume(dev);
> > 
> > The code hopes to call hdmi pm_runtime() whenever this resume()
> > is called? If so, I'm afraid it will not work. pm_runtime_force_resume()
> > calls pm_runtime with conditions. Sometimes pm_runtime will not
> > be called. In my suspend/resume test, it shows that pm_runtime() is
> > not called. This may cause the following function
> > hdac_hdmi_present_sense_all_pins() not work well as there is no
> > power on.
> 
> Hm, right, maybe we should move hdac_hdmi_present_sense_all_pins()
> into runtime resume.  It may be called unconditionally, but safer to
> call it conditionally only after S3 or so...

Now reading back the code again, no, it's correct to call
hdac_hdmi_present_sense_all_pins() there.  The function doesn't access
any HD-audio codec verbs but only with the i915 audio component.
Hence it's safe to call there no matter whether the device is opened
(thus resumed) or not.  And, it is even mandatory to call there; if a
HDMI connection changed during suspend, we need to notify it right
after the system resume.


thanks,

Takashi

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Yang, Libin]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Wednesday, January 9, 2019 5:14 PM
>To: Yang, Libin <libin.yang@intel.com>
>Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; alsa-
>devel@alsa-project.org; broonie@kernel.org;
>liam.r.girdwood@linux.intel.com; Lin, Mengdong <mengdong.lin@intel.com>
>Subject: Re: [alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio
>suspend/resume
>
>On Wed, 09 Jan 2019 09:29:36 +0100,
>Takashi Iwai wrote:
>>
>> On Wed, 09 Jan 2019 09:16:34 +0100,
>> Yang, Libin wrote:
>> >
>> > >-
>> > >-static void hdmi_codec_complete(struct device *dev)
>> > >+#ifdef CONFIG_PM_SLEEP
>> > >+static int hdmi_codec_resume(struct device *dev)
>> > > {
>> > > 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
>> > > 	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
>> > >+	int ret;
>> > >
>> > >-	/* Power up afg */
>> > >-	snd_hdac_codec_read(hdev, hdev->afg, 0,
>> > >	AC_VERB_SET_POWER_STATE,
>> > >-							AC_PWRST_D0);
>> > >-
>> > >-	hdac_hdmi_skl_enable_all_pins(hdev);
>> > >-	hdac_hdmi_skl_enable_dp12(hdev);
>> > >-
>> > >+	ret = pm_runtime_force_resume(dev);
>> >
>> > The code hopes to call hdmi pm_runtime() whenever this resume() is
>> > called? If so, I'm afraid it will not work.
>> > pm_runtime_force_resume() calls pm_runtime with conditions.
>> > Sometimes pm_runtime will not be called. In my suspend/resume test,
>> > it shows that pm_runtime() is not called. This may cause the
>> > following function
>> > hdac_hdmi_present_sense_all_pins() not work well as there is no
>> > power on.
>>
>> Hm, right, maybe we should move hdac_hdmi_present_sense_all_pins()
>> into runtime resume.  It may be called unconditionally, but safer to
>> call it conditionally only after S3 or so...
>
>Now reading back the code again, no, it's correct to call
>hdac_hdmi_present_sense_all_pins() there.  The function doesn't access any
>HD-audio codec verbs but only with the i915 audio component.
>Hence it's safe to call there no matter whether the device is opened (thus
>resumed) or not.  And, it is even mandatory to call there; if a HDMI connection
>changed during suspend, we need to notify it right after the system resume.

Yes. It seems this is safe. Thanks for clarification. SOF audio driver test passes. 
No reply from our internal SST driver team. From SOF test, this patch can fix
the display power issue.

BTW: I have another question. Maybe it is not related to this topic, but
related the general audio power sequence. I'm not sure my understand
is correct or not.
Currently, codec device's parent is the card device, right? If so, the suspend
sequence is:
codec power off (child)
card power off (parent)

However, I see some cards will call trigger for suspend in card suspend
function. This may cause issue logically (Maybe the real cards are always
OK). The codec has already powered off. So if trigger() operates with
codec may cause issue? Even, if the codec is the master, maybe the clk
is off before trigger() is called. I'm not sure this is always OK.
(This is why I submitted the second patch.)

Regards,
Libin

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Takashi Iwai]

On Thu, 10 Jan 2019 06:30:14 +0100,
Yang, Libin wrote:
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:tiwai@suse.de]
> >Sent: Wednesday, January 9, 2019 5:14 PM
> >To: Yang, Libin <libin.yang@intel.com>
> >Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; alsa-
> >devel@alsa-project.org; broonie@kernel.org;
> >liam.r.girdwood@linux.intel.com; Lin, Mengdong <mengdong.lin@intel.com>
> >Subject: Re: [alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio
> >suspend/resume
> >
> >On Wed, 09 Jan 2019 09:29:36 +0100,
> >Takashi Iwai wrote:
> >>
> >> On Wed, 09 Jan 2019 09:16:34 +0100,
> >> Yang, Libin wrote:
> >> >
> >> > >-
> >> > >-static void hdmi_codec_complete(struct device *dev)
> >> > >+#ifdef CONFIG_PM_SLEEP
> >> > >+static int hdmi_codec_resume(struct device *dev)
> >> > > {
> >> > > 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
> >> > > 	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
> >> > >+	int ret;
> >> > >
> >> > >-	/* Power up afg */
> >> > >-	snd_hdac_codec_read(hdev, hdev->afg, 0,
> >> > >	AC_VERB_SET_POWER_STATE,
> >> > >-							AC_PWRST_D0);
> >> > >-
> >> > >-	hdac_hdmi_skl_enable_all_pins(hdev);
> >> > >-	hdac_hdmi_skl_enable_dp12(hdev);
> >> > >-
> >> > >+	ret = pm_runtime_force_resume(dev);
> >> >
> >> > The code hopes to call hdmi pm_runtime() whenever this resume() is
> >> > called? If so, I'm afraid it will not work.
> >> > pm_runtime_force_resume() calls pm_runtime with conditions.
> >> > Sometimes pm_runtime will not be called. In my suspend/resume test,
> >> > it shows that pm_runtime() is not called. This may cause the
> >> > following function
> >> > hdac_hdmi_present_sense_all_pins() not work well as there is no
> >> > power on.
> >>
> >> Hm, right, maybe we should move hdac_hdmi_present_sense_all_pins()
> >> into runtime resume.  It may be called unconditionally, but safer to
> >> call it conditionally only after S3 or so...
> >
> >Now reading back the code again, no, it's correct to call
> >hdac_hdmi_present_sense_all_pins() there.  The function doesn't access any
> >HD-audio codec verbs but only with the i915 audio component.
> >Hence it's safe to call there no matter whether the device is opened (thus
> >resumed) or not.  And, it is even mandatory to call there; if a HDMI connection
> >changed during suspend, we need to notify it right after the system resume.
> 
> Yes. It seems this is safe. Thanks for clarification. SOF audio driver test passes. 
> No reply from our internal SST driver team. From SOF test, this patch can fix
> the display power issue.

OK, I'm going to queue this, but will delay the upstream merge for
5.0-rc3.

> BTW: I have another question. Maybe it is not related to this topic, but
> related the general audio power sequence. I'm not sure my understand
> is correct or not.
> Currently, codec device's parent is the card device, right? If so, the suspend
> sequence is:
> codec power off (child)
> card power off (parent)
> 
> However, I see some cards will call trigger for suspend in card suspend
> function. This may cause issue logically (Maybe the real cards are always
> OK). The codec has already powered off. So if trigger() operates with
> codec may cause issue? Even, if the codec is the master, maybe the clk
> is off before trigger() is called. I'm not sure this is always OK.
> (This is why I submitted the second patch.)

Indeed this looks like a problem, and it's specific to ASoC.
The legacy HD-audio codec driver has the base resume code that calls
snd_pcm_suspend_all() for the assigned PCM streams, while ASoC calls
it in snd_soc_suspend() that is invoked from the machine driver in
general.

If a codec driver has a strong dependency with the PCM state (like
HD-audio one), it'd need to stop in its suspend callback, something
like below.


thanks,

Takashi

---
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index b19d7a3e7a2c..caf512d807f5 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -106,6 +106,7 @@ struct hdac_hdmi_pcm {
 	struct list_head port_list;
 	struct hdac_hdmi_cvt *cvt;
 	struct snd_soc_jack *jack;
+	struct snd_pcm *snd_pcm;
 	int stream_tag;
 	int channels;
 	int format;
@@ -1792,6 +1793,7 @@ int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int device,
 	mutex_init(&pcm->lock);
 	INIT_LIST_HEAD(&pcm->port_list);
 	snd_pcm = hdac_hdmi_get_pcm_from_id(dai->component->card, device);
+	pcm->snd_pcm = snd_pcm;
 	if (snd_pcm) {
 		err = snd_hdac_add_chmap_ctls(snd_pcm, device, &hdmi->chmap);
 		if (err < 0) {
@@ -2118,8 +2120,10 @@ static int hdac_hdmi_dev_remove(struct hdac_device *hdev)
 static int hdac_hdmi_runtime_suspend(struct device *dev)
 {
 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
+	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
 	struct hdac_bus *bus = hdev->bus;
 	struct hdac_ext_link *hlink = NULL;
+	struct hdac_hdmi_pcm *pcm;
 
 	dev_dbg(dev, "Enter: %s\n", __func__);
 
@@ -2127,6 +2131,12 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
 	if (!bus)
 		return 0;
 
+	/* make sure all streams suspended before power down */
+	list_for_each_entry(pcm, &hdmi->pcm_list, head) {
+		if (pcm->snd_pcm)
+			snd_pcm_suspend_all(pcm->snd_pcm);
+	}
+
 	/*
 	 * Power down afg.
 	 * codec_read is preferred over codec_write to set the power state.

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Yang, Libin]

>> >On Wed, 09 Jan 2019 09:29:36 +0100,
>> >Takashi Iwai wrote:
>> >>
>> >> On Wed, 09 Jan 2019 09:16:34 +0100, Yang, Libin wrote:
>> >> >
>> >> > >-
>> >> > >-static void hdmi_codec_complete(struct device *dev)
>> >> > >+#ifdef CONFIG_PM_SLEEP
>> >> > >+static int hdmi_codec_resume(struct device *dev)
>> >> > > {
>> >> > > 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
>> >> > > 	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
>> >> > >+	int ret;
>> >> > >
>> >> > >-	/* Power up afg */
>> >> > >-	snd_hdac_codec_read(hdev, hdev->afg, 0,
>> >> > >	AC_VERB_SET_POWER_STATE,
>> >> > >-
>	AC_PWRST_D0);
>> >> > >-
>> >> > >-	hdac_hdmi_skl_enable_all_pins(hdev);
>> >> > >-	hdac_hdmi_skl_enable_dp12(hdev);
>> >> > >-
>> >> > >+	ret = pm_runtime_force_resume(dev);
>> >> >
>> >> > The code hopes to call hdmi pm_runtime() whenever this resume()
>> >> > is called? If so, I'm afraid it will not work.
>> >> > pm_runtime_force_resume() calls pm_runtime with conditions.
>> >> > Sometimes pm_runtime will not be called. In my suspend/resume
>> >> > test, it shows that pm_runtime() is not called. This may cause
>> >> > the following function
>> >> > hdac_hdmi_present_sense_all_pins() not work well as there is no
>> >> > power on.
>> >>
>> >> Hm, right, maybe we should move hdac_hdmi_present_sense_all_pins()
>> >> into runtime resume.  It may be called unconditionally, but safer
>> >> to call it conditionally only after S3 or so...
>> >
>> >Now reading back the code again, no, it's correct to call
>> >hdac_hdmi_present_sense_all_pins() there.  The function doesn't
>> >access any HD-audio codec verbs but only with the i915 audio component.
>> >Hence it's safe to call there no matter whether the device is opened
>> >(thus
>> >resumed) or not.  And, it is even mandatory to call there; if a HDMI
>> >connection changed during suspend, we need to notify it right after the
>system resume.
>>
>> Yes. It seems this is safe. Thanks for clarification. SOF audio driver test
>passes.
>> No reply from our internal SST driver team. From SOF test, this patch
>> can fix the display power issue.
>
>OK, I'm going to queue this, but will delay the upstream merge for 5.0-rc3.

That great! Thanks.

>
>> BTW: I have another question. Maybe it is not related to this topic,
>> but related the general audio power sequence. I'm not sure my
>> understand is correct or not.
>> Currently, codec device's parent is the card device, right? If so, the
>> suspend sequence is:
>> codec power off (child)
>> card power off (parent)
>>
>> However, I see some cards will call trigger for suspend in card
>> suspend function. This may cause issue logically (Maybe the real cards
>> are always OK). The codec has already powered off. So if trigger()
>> operates with codec may cause issue? Even, if the codec is the master,
>> maybe the clk is off before trigger() is called. I'm not sure this is always OK.
>> (This is why I submitted the second patch.)
>
>Indeed this looks like a problem, and it's specific to ASoC.
>The legacy HD-audio codec driver has the base resume code that calls
>snd_pcm_suspend_all() for the assigned PCM streams, while ASoC calls it in
>snd_soc_suspend() that is invoked from the machine driver in general.
>
>If a codec driver has a strong dependency with the PCM state (like HD-audio
>one), it'd need to stop in its suspend callback, something like below.

Yes, below patch should be able to fix my concern. I will test it  in HDMI
audio suspend/resume with playback after I fix another issue of hdmi
audio suspend/resume of SOF.

The below patch may have a small confliction that the trigger will be 
called twice as our SOF has already call snd_pcm_suspend() in card
suspend. Anyway, this is a SOF specific issue. I will work on it later.

Regards,
Libin

>
>
>thanks,
>
>Takashi
>
>---
>diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
>index b19d7a3e7a2c..caf512d807f5 100644
>--- a/sound/soc/codecs/hdac_hdmi.c
>+++ b/sound/soc/codecs/hdac_hdmi.c
>@@ -106,6 +106,7 @@ struct hdac_hdmi_pcm {
> 	struct list_head port_list;
> 	struct hdac_hdmi_cvt *cvt;
> 	struct snd_soc_jack *jack;
>+	struct snd_pcm *snd_pcm;
> 	int stream_tag;
> 	int channels;
> 	int format;
>@@ -1792,6 +1793,7 @@ int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int
>device,
> 	mutex_init(&pcm->lock);
> 	INIT_LIST_HEAD(&pcm->port_list);
> 	snd_pcm = hdac_hdmi_get_pcm_from_id(dai->component->card,
>device);
>+	pcm->snd_pcm = snd_pcm;
> 	if (snd_pcm) {
> 		err = snd_hdac_add_chmap_ctls(snd_pcm, device, &hdmi-
>>chmap);
> 		if (err < 0) {
>@@ -2118,8 +2120,10 @@ static int hdac_hdmi_dev_remove(struct
>hdac_device *hdev)  static int hdac_hdmi_runtime_suspend(struct device
>*dev)  {
> 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
>+	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
> 	struct hdac_bus *bus = hdev->bus;
> 	struct hdac_ext_link *hlink = NULL;
>+	struct hdac_hdmi_pcm *pcm;
>
> 	dev_dbg(dev, "Enter: %s\n", __func__);
>
>@@ -2127,6 +2131,12 @@ static int hdac_hdmi_runtime_suspend(struct
>device *dev)
> 	if (!bus)
> 		return 0;
>
>+	/* make sure all streams suspended before power down */
>+	list_for_each_entry(pcm, &hdmi->pcm_list, head) {
>+		if (pcm->snd_pcm)
>+			snd_pcm_suspend_all(pcm->snd_pcm);
>+	}
>+
> 	/*
> 	 * Power down afg.
> 	 * codec_read is preferred over codec_write to set the power state.
>
>
>_______________________________________________
>Alsa-devel mailing list
>Alsa-devel@alsa-project.org
>http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Takashi Iwai]

On Fri, 11 Jan 2019 06:20:23 +0100,
Yang, Libin wrote:
> 
> The below patch may have a small confliction that the trigger will be 
> called twice as our SOF has already call snd_pcm_suspend() in card
> suspend.

It should be no problem, snd_pcm_suspend() can be called multiple
times.  If it's already suspended, just nothing happens.


thanks,

Takashi

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Takashi Iwai]

On Fri, 11 Jan 2019 14:34:34 +0100,
Takashi Iwai wrote:
> 
> On Fri, 11 Jan 2019 06:20:23 +0100,
> Yang, Libin wrote:
> > 
> > The below patch may have a small confliction that the trigger will be 
> > called twice as our SOF has already call snd_pcm_suspend() in card
> > suspend.
> 
> It should be no problem, snd_pcm_suspend() can be called multiple
> times.  If it's already suspended, just nothing happens.

Thinking of this problem again, does a patch like below work instead?
This looks like a better and more generic solution.

What I'm not quite sure is whether the device suspend order between
PCM device and HD-audio codec device is guaranteed.  I guess yes,
because the PCM device is registered always after the codec.  But ASoC
might have another weirdness :)

If this patch works, basically we can get rid of all external callers
of snd_pcm_suspend() & co.  That'll be a nice cleanup.


thanks,

Takashi

---
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 01b9d62eef14..d8d57336b8b8 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -683,6 +683,25 @@ static inline int snd_pcm_substream_proc_done(struct snd_pcm_substream *substrea
 
 static const struct attribute_group *pcm_dev_attr_groups[];
 
+#ifdef CONFIG_PM_SLEEP
+static int do_pcm_suspend(struct device *dev)
+{
+	struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev);
+
+	snd_pcm_suspend_all(pstr->pcm);
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops pcm_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(do_pcm_suspend, NULL)
+};
+
+static const struct device_type pcm_dev_type = {
+	.name = "pcm",
+	.pm = &pcm_dev_pm_ops,
+};
+
 /**
  * snd_pcm_new_stream - create a new PCM stream
  * @pcm: the pcm instance
@@ -713,6 +732,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count)
 
 	snd_device_initialize(&pstr->dev, pcm->card);
 	pstr->dev.groups = pcm_dev_attr_groups;
+	pstr->dev.type = &pcm_dev_type;
 	dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device,
 		     stream == SNDRV_PCM_STREAM_PLAYBACK ? 'p' : 'c');
 

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Takashi Iwai]

On Fri, 11 Jan 2019 15:24:30 +0100,
Takashi Iwai wrote:
> 
> On Fri, 11 Jan 2019 14:34:34 +0100,
> Takashi Iwai wrote:
> > 
> > On Fri, 11 Jan 2019 06:20:23 +0100,
> > Yang, Libin wrote:
> > > 
> > > The below patch may have a small confliction that the trigger will be 
> > > called twice as our SOF has already call snd_pcm_suspend() in card
> > > suspend.
> > 
> > It should be no problem, snd_pcm_suspend() can be called multiple
> > times.  If it's already suspended, just nothing happens.
> 
> Thinking of this problem again, does a patch like below work instead?
> This looks like a better and more generic solution.
> 
> What I'm not quite sure is whether the device suspend order between
> PCM device and HD-audio codec device is guaranteed.  I guess yes,
> because the PCM device is registered always after the codec.  But ASoC
> might have another weirdness :)

... and further reading revealed that my afraid is right -- ASoC is
another beast.  Basically PM in ASoC requires *not* to be done by the
usual device PM callbacks.  snd_soc_suspend() does the whole suspend
jobs sequentially.  So, what you need for Intel ASoC driver is to do
suspend/resume with the component callbacks instead of device PM ops.
Currently the plumbing of hdac-hdmi driver is in a half-baked state.

> If this patch works, basically we can get rid of all external callers
> of snd_pcm_suspend() & co.  That'll be a nice cleanup.

OTOH, the statement above is still true for non-ASoC drivers.  So we'd
end up needing an extra flag or such to prevent the PCM device PM ops
for ASoC, while using this for the rest.


thanks,

Takashi

> 
> 
> thanks,
> 
> Takashi
> 
> ---
> diff --git a/sound/core/pcm.c b/sound/core/pcm.c
> index 01b9d62eef14..d8d57336b8b8 100644
> --- a/sound/core/pcm.c
> +++ b/sound/core/pcm.c
> @@ -683,6 +683,25 @@ static inline int snd_pcm_substream_proc_done(struct snd_pcm_substream *substrea
>  
>  static const struct attribute_group *pcm_dev_attr_groups[];
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int do_pcm_suspend(struct device *dev)
> +{
> +	struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev);
> +
> +	snd_pcm_suspend_all(pstr->pcm);
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops pcm_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(do_pcm_suspend, NULL)
> +};
> +
> +static const struct device_type pcm_dev_type = {
> +	.name = "pcm",
> +	.pm = &pcm_dev_pm_ops,
> +};
> +
>  /**
>   * snd_pcm_new_stream - create a new PCM stream
>   * @pcm: the pcm instance
> @@ -713,6 +732,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count)
>  
>  	snd_device_initialize(&pstr->dev, pcm->card);
>  	pstr->dev.groups = pcm_dev_attr_groups;
> +	pstr->dev.type = &pcm_dev_type;
>  	dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device,
>  		     stream == SNDRV_PCM_STREAM_PLAYBACK ? 'p' : 'c');
>  

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Yang, Libin]

Hi Takashi,

>> >
>> > On Fri, 11 Jan 2019 06:20:23 +0100,
>> > Yang, Libin wrote:
>> > >
>> > > The below patch may have a small confliction that the trigger will
>> > > be called twice as our SOF has already call snd_pcm_suspend() in
>> > > card suspend.
>> >
>> > It should be no problem, snd_pcm_suspend() can be called multiple
>> > times.  If it's already suspended, just nothing happens.
>>
>> Thinking of this problem again, does a patch like below work instead?
>> This looks like a better and more generic solution.
>>
>> What I'm not quite sure is whether the device suspend order between
>> PCM device and HD-audio codec device is guaranteed.  I guess yes,
>> because the PCM device is registered always after the codec.  But ASoC
>> might have another weirdness :)

The suspend order is always a quite hard issue for me. I have to spend
a lot time checking the parent-child relationship. And if they don't have
such relationship, I don't know how to find the order.

My initial idea to get rid of such dependency is: do the pcm suspend before
suspend(), for example put it in prepare(). And set device clk off and power
off in suspend(). This can make sure pcm is always properly suspended.
But, I'm not sure prepare() is a right place because it seems prepare() is not
designed to do such things and prepare() may impact the suspend/resume
sequence based on its return value. If your below patch works, I think it will
be a best solution which can handle such things in ALSA level. I think we
may need to do a lot of test because the different cards drivers are
extremely different.

Regards,
Libin

>
>... and further reading revealed that my afraid is right -- ASoC is another beast.
>Basically PM in ASoC requires *not* to be done by the usual device PM
>callbacks.  snd_soc_suspend() does the whole suspend jobs sequentially.  So,
>what you need for Intel ASoC driver is to do suspend/resume with the
>component callbacks instead of device PM ops.
>Currently the plumbing of hdac-hdmi driver is in a half-baked state.
>
>> If this patch works, basically we can get rid of all external callers
>> of snd_pcm_suspend() & co.  That'll be a nice cleanup.
>
>OTOH, the statement above is still true for non-ASoC drivers.  So we'd end up
>needing an extra flag or such to prevent the PCM device PM ops for ASoC,
>while using this for the rest.
>
>
>thanks,
>
>Takashi
>
>>
>>
>> thanks,
>>
>> Takashi
>>
>> ---
>> diff --git a/sound/core/pcm.c b/sound/core/pcm.c index
>> 01b9d62eef14..d8d57336b8b8 100644
>> --- a/sound/core/pcm.c
>> +++ b/sound/core/pcm.c
>> @@ -683,6 +683,25 @@ static inline int
>> snd_pcm_substream_proc_done(struct snd_pcm_substream *substrea
>>
>>  static const struct attribute_group *pcm_dev_attr_groups[];
>>
>> +#ifdef CONFIG_PM_SLEEP
>> +static int do_pcm_suspend(struct device *dev) {
>> +	struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str,
>> +dev);
>> +
>> +	snd_pcm_suspend_all(pstr->pcm);
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static const struct dev_pm_ops pcm_dev_pm_ops = {
>> +	SET_SYSTEM_SLEEP_PM_OPS(do_pcm_suspend, NULL) };
>> +
>> +static const struct device_type pcm_dev_type = {
>> +	.name = "pcm",
>> +	.pm = &pcm_dev_pm_ops,
>> +};
>> +
>>  /**
>>   * snd_pcm_new_stream - create a new PCM stream
>>   * @pcm: the pcm instance
>> @@ -713,6 +732,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int
>> stream, int substream_count)
>>
>>  	snd_device_initialize(&pstr->dev, pcm->card);
>>  	pstr->dev.groups = pcm_dev_attr_groups;
>> +	pstr->dev.type = &pcm_dev_type;
>>  	dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number,
>pcm->device,
>>  		     stream == SNDRV_PCM_STREAM_PLAYBACK ? 'p' : 'c');
>>

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Takashi Iwai]

On Sat, 12 Jan 2019 02:46:33 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> >> >
> >> > On Fri, 11 Jan 2019 06:20:23 +0100,
> >> > Yang, Libin wrote:
> >> > >
> >> > > The below patch may have a small confliction that the trigger will
> >> > > be called twice as our SOF has already call snd_pcm_suspend() in
> >> > > card suspend.
> >> >
> >> > It should be no problem, snd_pcm_suspend() can be called multiple
> >> > times.  If it's already suspended, just nothing happens.
> >>
> >> Thinking of this problem again, does a patch like below work instead?
> >> This looks like a better and more generic solution.
> >>
> >> What I'm not quite sure is whether the device suspend order between
> >> PCM device and HD-audio codec device is guaranteed.  I guess yes,
> >> because the PCM device is registered always after the codec.  But ASoC
> >> might have another weirdness :)
> 
> The suspend order is always a quite hard issue for me. I have to spend
> a lot time checking the parent-child relationship. And if they don't have
> such relationship, I don't know how to find the order.
> 
> My initial idea to get rid of such dependency is: do the pcm suspend before
> suspend(), for example put it in prepare(). And set device clk off and power
> off in suspend(). This can make sure pcm is always properly suspended.
> But, I'm not sure prepare() is a right place because it seems prepare() is not
> designed to do such things and prepare() may impact the suspend/resume
> sequence based on its return value. If your below patch works, I think it will
> be a best solution which can handle such things in ALSA level. I think we
> may need to do a lot of test because the different cards drivers are
> extremely different.

As mentioned below, ASoC is another thing.  Its PM sequence is found
in snd_soc_suspend().


Takashi


> 
> Regards,
> Libin
> 
> >
> >... and further reading revealed that my afraid is right -- ASoC is another beast.
> >Basically PM in ASoC requires *not* to be done by the usual device PM
> >callbacks.  snd_soc_suspend() does the whole suspend jobs sequentially.  So,
> >what you need for Intel ASoC driver is to do suspend/resume with the
> >component callbacks instead of device PM ops.
> >Currently the plumbing of hdac-hdmi driver is in a half-baked state.
> >
> >> If this patch works, basically we can get rid of all external callers
> >> of snd_pcm_suspend() & co.  That'll be a nice cleanup.
> >
> >OTOH, the statement above is still true for non-ASoC drivers.  So we'd end up
> >needing an extra flag or such to prevent the PCM device PM ops for ASoC,
> >while using this for the rest.
> >
> >
> >thanks,
> >
> >Takashi
> >
> >>
> >>
> >> thanks,
> >>
> >> Takashi
> >>
> >> ---
> >> diff --git a/sound/core/pcm.c b/sound/core/pcm.c index
> >> 01b9d62eef14..d8d57336b8b8 100644
> >> --- a/sound/core/pcm.c
> >> +++ b/sound/core/pcm.c
> >> @@ -683,6 +683,25 @@ static inline int
> >> snd_pcm_substream_proc_done(struct snd_pcm_substream *substrea
> >>
> >>  static const struct attribute_group *pcm_dev_attr_groups[];
> >>
> >> +#ifdef CONFIG_PM_SLEEP
> >> +static int do_pcm_suspend(struct device *dev) {
> >> +	struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str,
> >> +dev);
> >> +
> >> +	snd_pcm_suspend_all(pstr->pcm);
> >> +	return 0;
> >> +}
> >> +#endif
> >> +
> >> +static const struct dev_pm_ops pcm_dev_pm_ops = {
> >> +	SET_SYSTEM_SLEEP_PM_OPS(do_pcm_suspend, NULL) };
> >> +
> >> +static const struct device_type pcm_dev_type = {
> >> +	.name = "pcm",
> >> +	.pm = &pcm_dev_pm_ops,
> >> +};
> >> +
> >>  /**
> >>   * snd_pcm_new_stream - create a new PCM stream
> >>   * @pcm: the pcm instance
> >> @@ -713,6 +732,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int
> >> stream, int substream_count)
> >>
> >>  	snd_device_initialize(&pstr->dev, pcm->card);
> >>  	pstr->dev.groups = pcm_dev_attr_groups;
> >> +	pstr->dev.type = &pcm_dev_type;
> >>  	dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number,
> >pcm->device,
> >>  		     stream == SNDRV_PCM_STREAM_PLAYBACK ? 'p' : 'c');
> >>
> 

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Keyon Jie]

On 2019/1/12 下午3:43, Takashi Iwai wrote:
> On Sat, 12 Jan 2019 02:46:33 +0100,
> Yang, Libin wrote:
>>
>> Hi Takashi,
>>
>>>>>
>>>>> On Fri, 11 Jan 2019 06:20:23 +0100,
>>>>> Yang, Libin wrote:
>>>>>>
>>>>>> The below patch may have a small confliction that the trigger will
>>>>>> be called twice as our SOF has already call snd_pcm_suspend() in
>>>>>> card suspend.
>>>>>
>>>>> It should be no problem, snd_pcm_suspend() can be called multiple
>>>>> times.  If it's already suspended, just nothing happens.
>>>>
>>>> Thinking of this problem again, does a patch like below work instead?
>>>> This looks like a better and more generic solution.
>>>>
>>>> What I'm not quite sure is whether the device suspend order between
>>>> PCM device and HD-audio codec device is guaranteed.  I guess yes,
>>>> because the PCM device is registered always after the codec.  But ASoC
>>>> might have another weirdness :)
>>
>> The suspend order is always a quite hard issue for me. I have to spend
>> a lot time checking the parent-child relationship. And if they don't have
>> such relationship, I don't know how to find the order.
>>
>> My initial idea to get rid of such dependency is: do the pcm suspend before
>> suspend(), for example put it in prepare(). And set device clk off and power
>> off in suspend(). This can make sure pcm is always properly suspended.
>> But, I'm not sure prepare() is a right place because it seems prepare() is not
>> designed to do such things and prepare() may impact the suspend/resume
>> sequence based on its return value. If your below patch works, I think it will
>> be a best solution which can handle such things in ALSA level. I think we
>> may need to do a lot of test because the different cards drivers are
>> extremely different.
> 
> As mentioned below, ASoC is another thing.  Its PM sequence is found
> in snd_soc_suspend().
> 
> 
> Takashi

This is an interesting topic, it worth to do a surgery to remove those 
ASoC snd_pcm_suspend_all() invoking and do unified PCM suspend in ALSA 
PCM level?

Thanks,
~Keyon


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

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Takashi Iwai]

On Mon, 14 Jan 2019 02:16:02 +0100,
Keyon Jie wrote:
> 
> 
> 
> On 2019/1/12 下午3:43, Takashi Iwai wrote:
> > On Sat, 12 Jan 2019 02:46:33 +0100,
> > Yang, Libin wrote:
> >>
> >> Hi Takashi,
> >>
> >>>>>
> >>>>> On Fri, 11 Jan 2019 06:20:23 +0100,
> >>>>> Yang, Libin wrote:
> >>>>>>
> >>>>>> The below patch may have a small confliction that the trigger will
> >>>>>> be called twice as our SOF has already call snd_pcm_suspend() in
> >>>>>> card suspend.
> >>>>>
> >>>>> It should be no problem, snd_pcm_suspend() can be called multiple
> >>>>> times.  If it's already suspended, just nothing happens.
> >>>>
> >>>> Thinking of this problem again, does a patch like below work instead?
> >>>> This looks like a better and more generic solution.
> >>>>
> >>>> What I'm not quite sure is whether the device suspend order between
> >>>> PCM device and HD-audio codec device is guaranteed.  I guess yes,
> >>>> because the PCM device is registered always after the codec.  But ASoC
> >>>> might have another weirdness :)
> >>
> >> The suspend order is always a quite hard issue for me. I have to spend
> >> a lot time checking the parent-child relationship. And if they don't have
> >> such relationship, I don't know how to find the order.
> >>
> >> My initial idea to get rid of such dependency is: do the pcm suspend before
> >> suspend(), for example put it in prepare(). And set device clk off and power
> >> off in suspend(). This can make sure pcm is always properly suspended.
> >> But, I'm not sure prepare() is a right place because it seems prepare() is not
> >> designed to do such things and prepare() may impact the suspend/resume
> >> sequence based on its return value. If your below patch works, I think it will
> >> be a best solution which can handle such things in ALSA level. I think we
> >> may need to do a lot of test because the different cards drivers are
> >> extremely different.
> >
> > As mentioned below, ASoC is another thing.  Its PM sequence is found
> > in snd_soc_suspend().
> >
> >
> > Takashi
> 
> This is an interesting topic, it worth to do a surgery to remove those
> ASoC snd_pcm_suspend_all() invoking and do unified PCM suspend in ALSA
> PCM level?

This won't work easily, I suppose.  Take a look at snd_soc_suspend()
code.  It's designed to run several procedures sequentially there, and
each component is supposed to process the suspend/resume task via the
ASoC component callback, not the driver PM ops that can be outside
this call.

HD-audio ASoC codec driver is an ASoC codec driver, hence it should
follow that way.


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

Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Yang, Libin]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Friday, January 11, 2019 9:35 PM
>To: Yang, Libin <libin.yang@intel.com>
>Cc: liam.r.girdwood@linux.intel.com; Lin, Mengdong
><mengdong.lin@intel.com>; alsa-devel@alsa-project.org; broonie@kernel.org;
>Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>Subject: Re: [alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio
>suspend/resume
>
>On Fri, 11 Jan 2019 06:20:23 +0100,
>Yang, Libin wrote:
>>
>> The below patch may have a small confliction that the trigger will be
>> called twice as our SOF has already call snd_pcm_suspend() in card
>> suspend.
>
>It should be no problem, snd_pcm_suspend() can be called multiple times.  If
>it's already suspended, just nothing happens.

Got it. Thanks.

Regards,
Libin

>
>
>thanks,
>
>Takashi