[alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3

[Takashi Iwai]

On Fri, 22 Mar 2019 07:24:56 +0100,
libin.yang at intel.com wrote:
> 
> From: Libin Yang <libin.yang at intel.com>
> 
> In S3, there is a contest between dapm event and getting display power.
> 
> When resume, the sequence is:
> 
> hdac_hdmi_runtime_resume()
>   => snd_hdac_display_power()
> hdac_hdmi_xxx_widget_event()
> 
> The hdac_hdmi_xxx_widget_event() are based on the power being on.
> Otherwise, the operation on the codec register will fail.
> 
> However, in snd_hdac_display_power(), it may sleep because of the
> mutex_lock() in display driver. In this case, hdac_hdmi_xxx_widget_event()
> will be called before the power is on.
> 
> Let's not operate the registers and wait for the power in
> hdac_hdmi_xxx_widget_event()
> 
> Signed-off-by: Libin Yang <libin.yang at intel.com>

IMO the workaround looks a bit too fragile.  The substantial problem
is the race between codec and DAPM.  Can this be controlled better,
e.g. by defining the proper PM dependency?  I thought we may rearrange
the PM topology dynamically.


thanks,

Takashi

> ---
>  sound/soc/codecs/hdac_hdmi.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> index 5eeb0fe..3c5c122 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -144,6 +144,7 @@ struct hdac_hdmi_priv {
>  	int num_pin;
>  	int num_cvt;
>  	int num_ports;
> +	int display_power;	/* 0: power off; 1 power on */
>  	struct mutex pin_mutex;
>  	struct hdac_chmap chmap;
>  	struct hdac_hdmi_drv_data *drv_data;
> @@ -742,12 +743,27 @@ static void hdac_hdmi_set_amp(struct hdac_device *hdev,
>  					AC_VERB_SET_AMP_GAIN_MUTE, val);
>  }
>  
> +static int wait_for_display_power(struct hdac_hdmi_priv *hdmi)
> +{
> +	int i = 0;
> +
> +	/* sleep for totally 80ms ~ 200ms should be enough */
> +	while (i < 40) {
> +		if (!hdmi->display_power)
> +			usleep_range(2000, 5000);
> +		else
> +			return 0;
> +		i++;
> +	}
> +	return -EAGAIN;
> +}
>  
>  static int hdac_hdmi_pin_output_widget_event(struct snd_soc_dapm_widget *w,
>  					struct snd_kcontrol *kc, int event)
>  {
>  	struct hdac_hdmi_port *port = w->priv;
>  	struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev);
> +	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
>  	struct hdac_hdmi_pcm *pcm;
>  
>  	dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n",
> @@ -757,6 +773,11 @@ static int hdac_hdmi_pin_output_widget_event(struct snd_soc_dapm_widget *w,
>  	if (!pcm)
>  		return -EIO;
>  
> +	if (wait_for_display_power(hdmi)) {
> +		dev_err(&hdev->dev, "%s: hdmi power is not ready\n", __func__);
> +		return -EAGAIN;
> +	}
> +
>  	/* set the device if pin is mst_capable */
>  	if (hdac_hdmi_port_select_set(hdev, port) < 0)
>  		return -EIO;
> @@ -803,6 +824,11 @@ static int hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w,
>  	if (!pcm)
>  		return -EIO;
>  
> +	if (wait_for_display_power(hdmi)) {
> +		dev_err(&hdev->dev, "%s: hdmi power is not ready\n", __func__);
> +		return -EAGAIN;
> +	}
> +
>  	switch (event) {
>  	case SND_SOC_DAPM_PRE_PMU:
>  		hdac_hdmi_set_power_state(hdev, cvt->nid, AC_PWRST_D0);
> @@ -840,6 +866,7 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w,
>  {
>  	struct hdac_hdmi_port *port = w->priv;
>  	struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev);
> +	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
>  	int mux_idx;
>  
>  	dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n",
> @@ -850,6 +877,11 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w,
>  
>  	mux_idx = dapm_kcontrol_get_value(kc);
>  
> +	if (wait_for_display_power(hdmi)) {
> +		dev_err(&hdev->dev, "%s: hdmi power is not ready\n", __func__);
> +		return -EAGAIN;
> +	}
> +
>  	/* set the device if pin is mst_capable */
>  	if (hdac_hdmi_port_select_set(hdev, port) < 0)
>  		return -EIO;
> @@ -2068,6 +2100,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
>  {
>  	struct hdac_device *hdev = dev_to_hdac_dev(dev);
>  	struct hdac_bus *bus = hdev->bus;
> +	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
>  	struct hdac_ext_link *hlink = NULL;
>  
>  	dev_dbg(dev, "Enter: %s\n", __func__);
> @@ -2095,6 +2128,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
>  	snd_hdac_ext_bus_link_put(bus, hlink);
>  
>  	snd_hdac_display_power(bus, hdev->addr, false);
> +	hdmi->display_power = 0;
>  
>  	return 0;
>  }
> @@ -2102,6 +2136,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
>  static int hdac_hdmi_runtime_resume(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;
>  
> @@ -2128,6 +2163,7 @@ 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);
>  
> +	hdmi->display_power = 1;
>  	return 0;
>  }
>  #else
> -- 
> 2.7.4
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>