ASoC: hdmi: implement component supsend/resume callback

Commit Message

Yang, Libin March 28, 2019, 8:40 a.m. UTC

From: Libin Yang <[e-mail]>

Implement the hdmi codec component driver's suspend/resume callback.
This can make sure that the dapm event is triggered after the display
audio power domain is turned on if bus_control is set.

Also this patch removes the codec device PM suspend/resume.

Signed-off-by: Libin Yang <[e-mail]>
---
 sound/soc/codecs/hdac_hdmi.c | 51 +++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 24 deletions(-)

Comments

Takashi Iwai March 28, 2019, 9:22 a.m. UTC | #1

On Thu, 28 Mar 2019 09:40:05 +0100,
[e-mail] wrote:
> 
> From: Libin Yang <[e-mail]>
> 
> Implement the hdmi codec component driver's suspend/resume callback.
> This can make sure that the dapm event is triggered after the display
> audio power domain is turned on if bus_control is set.
> 
> Also this patch removes the codec device PM suspend/resume.
> 
> Signed-off-by: Libin Yang <[e-mail]>
> ---
>  sound/soc/codecs/hdac_hdmi.c | 51 +++++++++++++++++++++++---------------------
>  1 file changed, 27 insertions(+), 24 deletions(-)
> 
> diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> index 5eeb0fe..478c6f2 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -1873,36 +1873,27 @@ static void hdmi_codec_remove(struct snd_soc_component *component)
>  	pm_runtime_disable(&hdev->dev);
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
> -static int hdmi_codec_resume(struct device *dev)
> +static int hdmi_component_suspend(struct snd_soc_component *component)
>  {
> -	struct hdac_device *hdev = dev_to_hdac_dev(dev);
> -	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
> -	int ret;
> +	struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component);
> +	struct hdac_device *hdev = hdmi->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
> -	 * all pins here. pin capablity change is not support, so use the
> -	 * already set pin caps.
> -	 *
> -	 * NOTE: this is safe to call even if the codec doesn't actually resume.
> -	 * The pin check involves only with DRM audio component hooks, so it
> -	 * works even if the HD-audio side is still dreaming peacefully.
> -	 */
> -	hdac_hdmi_present_sense_all_pins(hdev, hdmi, false);
> -	return 0;
> +	return pm_runtime_force_suspend(&hdev->dev);
> +}
> +
> +static int hdmi_component_resume(struct snd_soc_component *component)
> +{
> +	struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component);
> +	struct hdac_device *hdev = hdmi->hdev;
> +
> +	return pm_runtime_force_resume(&hdev->dev);
>  }
> -#else
> -#define hdmi_codec_resume NULL
> -#endif
>  
>  static const struct snd_soc_component_driver hdmi_hda_codec = {
>  	.probe			= hdmi_codec_probe,
>  	.remove			= hdmi_codec_remove,
> +	.suspend		= hdmi_component_suspend,
> +	.resume			= hdmi_component_resume,
>  	.use_pmdown_time	= 1,
>  	.endianness		= 1,
>  	.non_legacy_dai_naming	= 1,
> @@ -2104,6 +2095,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
>  	struct hdac_device *hdev = dev_to_hdac_dev(dev);
>  	struct hdac_bus *bus = hdev->bus;
>  	struct hdac_ext_link *hlink = NULL;
> +	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
>  
>  	dev_dbg(dev, "Enter: %s\n", __func__);
>  
> @@ -2128,6 +2120,18 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
>  	snd_hdac_codec_read(hdev, hdev->afg, 0,	AC_VERB_SET_POWER_STATE,
>  							AC_PWRST_D0);
>  
> +	/*
> +	 * As the ELD notify callback request is not entertained while the
> +	 * device is in suspend state. Need to manually check detection of
> +	 * all pins here. pin capablity change is not support, so use the
> +	 * already set pin caps.
> +	 *
> +	 * NOTE: this is safe to call even if the codec doesn't actually resume.
> +	 * The pin check involves only with DRM audio component hooks, so it
> +	 * works even if the HD-audio side is still dreaming peacefully.
> +	 */
> +	hdac_hdmi_present_sense_all_pins(hdev, hdmi, false);

Do we need to move this into runtime_resume?  The forced detection is
needed basically only for the system resume, i.e. the detection is
performed while the system is running.

Other than that, one thing I overlooked is that this approach might
hit another race when another problem is calling a path that includes
pm_runtime_get*().  Since the whole ASoC resume is done in an async
work, this would conflict.  That's rather a fundamental bug in ASoC
resume framework, though...

So, I'm interested in whether another approach really works: namely
defining the PM dependency via device_link_add().  This should be
safer.


thanks,

Takashi

Yang, Libin March 28, 2019, 1:52 p.m. UTC | #2

Hi Takashi,

>-----Original Message-----
>From: Takashi Iwai [mailto:[e-mail]]
>Sent: Thursday, March 28, 2019 5:23 PM
>To: Yang, Libin <[e-mail]>
>Cc: [e-mail]; [e-mail]
>Subject: Re: [alsa-devel] [PATCH] ASoC: hdmi: implement component
>supsend/resume callback
>
>On Thu, 28 Mar 2019 09:40:05 +0100,
>[e-mail] wrote:
>>
>> From: Libin Yang <[e-mail]>
>>
>> Implement the hdmi codec component driver's suspend/resume callback.
>> This can make sure that the dapm event is triggered after the display
>> audio power domain is turned on if bus_control is set.
>>
>> Also this patch removes the codec device PM suspend/resume.
>>
>> Signed-off-by: Libin Yang <[e-mail]>
>> ---
>>  sound/soc/codecs/hdac_hdmi.c | 51
>> +++++++++++++++++++++++---------------------
>>  1 file changed, 27 insertions(+), 24 deletions(-)
>>
>> diff --git a/sound/soc/codecs/hdac_hdmi.c
>> b/sound/soc/codecs/hdac_hdmi.c index 5eeb0fe..478c6f2 100644
>> --- a/sound/soc/codecs/hdac_hdmi.c
>> +++ b/sound/soc/codecs/hdac_hdmi.c
>> @@ -1873,36 +1873,27 @@ static void hdmi_codec_remove(struct
>snd_soc_component *component)
>>  	pm_runtime_disable(&hdev->dev);
>>  }
>>
>> -#ifdef CONFIG_PM_SLEEP
>> -static int hdmi_codec_resume(struct device *dev)
>> +static int hdmi_component_suspend(struct snd_soc_component
>> +*component)
>>  {
>> -	struct hdac_device *hdev = dev_to_hdac_dev(dev);
>> -	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
>> -	int ret;
>> +	struct hdac_hdmi_priv *hdmi =
>snd_soc_component_get_drvdata(component);
>> +	struct hdac_device *hdev = hdmi->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
>> -	 * all pins here. pin capablity change is not support, so use the
>> -	 * already set pin caps.
>> -	 *
>> -	 * NOTE: this is safe to call even if the codec doesn't actually resume.
>> -	 * The pin check involves only with DRM audio component hooks, so it
>> -	 * works even if the HD-audio side is still dreaming peacefully.
>> -	 */
>> -	hdac_hdmi_present_sense_all_pins(hdev, hdmi, false);
>> -	return 0;
>> +	return pm_runtime_force_suspend(&hdev->dev);
>> +}
>> +
>> +static int hdmi_component_resume(struct snd_soc_component
>*component)
>> +{
>> +	struct hdac_hdmi_priv *hdmi =
>snd_soc_component_get_drvdata(component);
>> +	struct hdac_device *hdev = hdmi->hdev;
>> +
>> +	return pm_runtime_force_resume(&hdev->dev);
>>  }
>> -#else
>> -#define hdmi_codec_resume NULL
>> -#endif
>>
>>  static const struct snd_soc_component_driver hdmi_hda_codec = {
>>  	.probe			= hdmi_codec_probe,
>>  	.remove			= hdmi_codec_remove,
>> +	.suspend		= hdmi_component_suspend,
>> +	.resume			= hdmi_component_resume,
>>  	.use_pmdown_time	= 1,
>>  	.endianness		= 1,
>>  	.non_legacy_dai_naming	= 1,
>> @@ -2104,6 +2095,7 @@ static int hdac_hdmi_runtime_resume(struct
>device *dev)
>>  	struct hdac_device *hdev = dev_to_hdac_dev(dev);
>>  	struct hdac_bus *bus = hdev->bus;
>>  	struct hdac_ext_link *hlink = NULL;
>> +	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
>>
>>  	dev_dbg(dev, "Enter: %s\n", __func__);
>>
>> @@ -2128,6 +2120,18 @@ static int hdac_hdmi_runtime_resume(struct
>device *dev)
>>  	snd_hdac_codec_read(hdev, hdev->afg, 0,
>	AC_VERB_SET_POWER_STATE,
>>  							AC_PWRST_D0);
>>
>> +	/*
>> +	 * As the ELD notify callback request is not entertained while the
>> +	 * device is in suspend state. Need to manually check detection of
>> +	 * all pins here. pin capablity change is not support, so use the
>> +	 * already set pin caps.
>> +	 *
>> +	 * NOTE: this is safe to call even if the codec doesn't actually resume.
>> +	 * The pin check involves only with DRM audio component hooks, so it
>> +	 * works even if the HD-audio side is still dreaming peacefully.
>> +	 */
>> +	hdac_hdmi_present_sense_all_pins(hdev, hdmi, false);
>
>Do we need to move this into runtime_resume?  The forced detection is
>needed basically only for the system resume, i.e. the detection is performed
>while the system is running.

Right. So move it into component resume?

>
>Other than that, one thing I overlooked is that this approach might hit another
>race when another problem is calling a path that includes pm_runtime_get*().
>Since the whole ASoC resume is done in an async work, this would conflict.
>That's rather a fundamental bug in ASoC resume framework, though...
>
>So, I'm interested in whether another approach really works: namely defining
>the PM dependency via device_link_add().  This should be safer.

OK. I will try to use device_link_add() and have a test tomorrow.

Regards,
Libin

>
>
>thanks,
>
>Takashi

Takashi Iwai March 28, 2019, 2:02 p.m. UTC | #3

On Thu, 28 Mar 2019 14:52:19 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:[e-mail]]
> >Sent: Thursday, March 28, 2019 5:23 PM
> >To: Yang, Libin <[e-mail]>
> >Cc: [e-mail]; [e-mail]
> >Subject: Re: [alsa-devel] [PATCH] ASoC: hdmi: implement component
> >supsend/resume callback
> >
> >On Thu, 28 Mar 2019 09:40:05 +0100,
> >[e-mail] wrote:
> >>
> >> From: Libin Yang <[e-mail]>
> >>
> >> Implement the hdmi codec component driver's suspend/resume callback.
> >> This can make sure that the dapm event is triggered after the display
> >> audio power domain is turned on if bus_control is set.
> >>
> >> Also this patch removes the codec device PM suspend/resume.
> >>
> >> Signed-off-by: Libin Yang <[e-mail]>
> >> ---
> >>  sound/soc/codecs/hdac_hdmi.c | 51
> >> +++++++++++++++++++++++---------------------
> >>  1 file changed, 27 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/sound/soc/codecs/hdac_hdmi.c
> >> b/sound/soc/codecs/hdac_hdmi.c index 5eeb0fe..478c6f2 100644
> >> --- a/sound/soc/codecs/hdac_hdmi.c
> >> +++ b/sound/soc/codecs/hdac_hdmi.c
> >> @@ -1873,36 +1873,27 @@ static void hdmi_codec_remove(struct
> >snd_soc_component *component)
> >>  	pm_runtime_disable(&hdev->dev);
> >>  }
> >>
> >> -#ifdef CONFIG_PM_SLEEP
> >> -static int hdmi_codec_resume(struct device *dev)
> >> +static int hdmi_component_suspend(struct snd_soc_component
> >> +*component)
> >>  {
> >> -	struct hdac_device *hdev = dev_to_hdac_dev(dev);
> >> -	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
> >> -	int ret;
> >> +	struct hdac_hdmi_priv *hdmi =
> >snd_soc_component_get_drvdata(component);
> >> +	struct hdac_device *hdev = hdmi->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
> >> -	 * all pins here. pin capablity change is not support, so use the
> >> -	 * already set pin caps.
> >> -	 *
> >> -	 * NOTE: this is safe to call even if the codec doesn't actually resume.
> >> -	 * The pin check involves only with DRM audio component hooks, so it
> >> -	 * works even if the HD-audio side is still dreaming peacefully.
> >> -	 */
> >> -	hdac_hdmi_present_sense_all_pins(hdev, hdmi, false);
> >> -	return 0;
> >> +	return pm_runtime_force_suspend(&hdev->dev);
> >> +}
> >> +
> >> +static int hdmi_component_resume(struct snd_soc_component
> >*component)
> >> +{
> >> +	struct hdac_hdmi_priv *hdmi =
> >snd_soc_component_get_drvdata(component);
> >> +	struct hdac_device *hdev = hdmi->hdev;
> >> +
> >> +	return pm_runtime_force_resume(&hdev->dev);
> >>  }
> >> -#else
> >> -#define hdmi_codec_resume NULL
> >> -#endif
> >>
> >>  static const struct snd_soc_component_driver hdmi_hda_codec = {
> >>  	.probe			= hdmi_codec_probe,
> >>  	.remove			= hdmi_codec_remove,
> >> +	.suspend		= hdmi_component_suspend,
> >> +	.resume			= hdmi_component_resume,
> >>  	.use_pmdown_time	= 1,
> >>  	.endianness		= 1,
> >>  	.non_legacy_dai_naming	= 1,
> >> @@ -2104,6 +2095,7 @@ static int hdac_hdmi_runtime_resume(struct
> >device *dev)
> >>  	struct hdac_device *hdev = dev_to_hdac_dev(dev);
> >>  	struct hdac_bus *bus = hdev->bus;
> >>  	struct hdac_ext_link *hlink = NULL;
> >> +	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
> >>
> >>  	dev_dbg(dev, "Enter: %s\n", __func__);
> >>
> >> @@ -2128,6 +2120,18 @@ static int hdac_hdmi_runtime_resume(struct
> >device *dev)
> >>  	snd_hdac_codec_read(hdev, hdev->afg, 0,
> >	AC_VERB_SET_POWER_STATE,
> >>  							AC_PWRST_D0);
> >>
> >> +	/*
> >> +	 * As the ELD notify callback request is not entertained while the
> >> +	 * device is in suspend state. Need to manually check detection of
> >> +	 * all pins here. pin capablity change is not support, so use the
> >> +	 * already set pin caps.
> >> +	 *
> >> +	 * NOTE: this is safe to call even if the codec doesn't actually resume.
> >> +	 * The pin check involves only with DRM audio component hooks, so it
> >> +	 * works even if the HD-audio side is still dreaming peacefully.
> >> +	 */
> >> +	hdac_hdmi_present_sense_all_pins(hdev, hdmi, false);
> >
> >Do we need to move this into runtime_resume?  The forced detection is
> >needed basically only for the system resume, i.e. the detection is performed
> >while the system is running.
> 
> Right. So move it into component resume?

Yes, that'll make more sense, although keeping it in runtime resume
would be relatively harmless in practice.

> >Other than that, one thing I overlooked is that this approach might hit another
> >race when another problem is calling a path that includes pm_runtime_get*().
> >Since the whole ASoC resume is done in an async work, this would conflict.
> >That's rather a fundamental bug in ASoC resume framework, though...
> >
> >So, I'm interested in whether another approach really works: namely defining
> >the PM dependency via device_link_add().  This should be safer.
> 
> OK. I will try to use device_link_add() and have a test tomorrow.

Thanks!


Takashi

Patch

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 5eeb0fe..478c6f2 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -1873,36 +1873,27 @@  static void hdmi_codec_remove(struct snd_soc_component *component)
 	pm_runtime_disable(&hdev->dev);
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int hdmi_codec_resume(struct device *dev)
+static int hdmi_component_suspend(struct snd_soc_component *component)
 {
-	struct hdac_device *hdev = dev_to_hdac_dev(dev);
-	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
-	int ret;
+	struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component);
+	struct hdac_device *hdev = hdmi->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
-	 * all pins here. pin capablity change is not support, so use the
-	 * already set pin caps.
-	 *
-	 * NOTE: this is safe to call even if the codec doesn't actually resume.
-	 * The pin check involves only with DRM audio component hooks, so it
-	 * works even if the HD-audio side is still dreaming peacefully.
-	 */
-	hdac_hdmi_present_sense_all_pins(hdev, hdmi, false);
-	return 0;
+	return pm_runtime_force_suspend(&hdev->dev);
+}
+
+static int hdmi_component_resume(struct snd_soc_component *component)
+{
+	struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component);
+	struct hdac_device *hdev = hdmi->hdev;
+
+	return pm_runtime_force_resume(&hdev->dev);
 }
-#else
-#define hdmi_codec_resume NULL
-#endif
 
 static const struct snd_soc_component_driver hdmi_hda_codec = {
 	.probe			= hdmi_codec_probe,
 	.remove			= hdmi_codec_remove,
+	.suspend		= hdmi_component_suspend,
+	.resume			= hdmi_component_resume,
 	.use_pmdown_time	= 1,
 	.endianness		= 1,
 	.non_legacy_dai_naming	= 1,
@@ -2104,6 +2095,7 @@  static int hdac_hdmi_runtime_resume(struct device *dev)
 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
 	struct hdac_bus *bus = hdev->bus;
 	struct hdac_ext_link *hlink = NULL;
+	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
 
 	dev_dbg(dev, "Enter: %s\n", __func__);
 
@@ -2128,6 +2120,18 @@  static int hdac_hdmi_runtime_resume(struct device *dev)
 	snd_hdac_codec_read(hdev, hdev->afg, 0,	AC_VERB_SET_POWER_STATE,
 							AC_PWRST_D0);
 
+	/*
+	 * As the ELD notify callback request is not entertained while the
+	 * device is in suspend state. Need to manually check detection of
+	 * all pins here. pin capablity change is not support, so use the
+	 * already set pin caps.
+	 *
+	 * NOTE: this is safe to call even if the codec doesn't actually resume.
+	 * The pin check involves only with DRM audio component hooks, so it
+	 * works even if the HD-audio side is still dreaming peacefully.
+	 */
+	hdac_hdmi_present_sense_all_pins(hdev, hdmi, false);
+
 	return 0;
 }
 #else
@@ -2137,7 +2141,6 @@  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)
-	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, hdmi_codec_resume)
 };
 
 static const struct hda_device_id hdmi_list[] = {