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

[Takashi Iwai]

On Thu, 28 Mar 2019 14:52:19 +0100,
Yang, Libin wrote:
> 
> 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?

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