[alsa-devel] [PATCH] ASoC: hdmi: implement component supsend/resume callback

[Yang, Libin]

Hi Takashi,

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai at suse.de]
>Sent: Thursday, March 28, 2019 5:23 PM
>To: Yang, Libin <libin.yang at intel.com>
>Cc: alsa-devel at alsa-project.org; broonie at kernel.org
>Subject: Re: [alsa-devel] [PATCH] ASoC: hdmi: implement component
>supsend/resume callback
>
>On Thu, 28 Mar 2019 09:40:05 +0100,
>libin.yang at intel.com wrote:
>>
>> From: Libin Yang <libin.yang at intel.com>
>>
>> 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 <libin.yang at intel.com>
>> ---
>>  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