[V2] ASoC: fix hdmi codec driver contest in S3

Commit Message

Yang, Libin March 22, 2019, 6:24 a.m. UTC

From: Libin Yang <[e-mail]>

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 <[e-mail]>
---
 sound/soc/codecs/hdac_hdmi.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Comments

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

On Fri, 22 Mar 2019 07:24:56 +0100,
[e-mail] wrote:
> 
> From: Libin Yang <[e-mail]>
> 
> 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 <[e-mail]>

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
> [e-mail]
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Yang, Libin March 22, 2019, 9:12 a.m. UTC | #2

Hi Takashi,


>-----Original Message-----
>From: Takashi Iwai [mailto:[e-mail]]
>Sent: Friday, March 22, 2019 5:04 PM
>To: Yang, Libin <[e-mail]>
>Cc: [e-mail]; [e-mail]
>Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
>
>On Fri, 22 Mar 2019 07:24:56 +0100,
>[e-mail] wrote:
>>
>> From: Libin Yang <[e-mail]>
>>
>> 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 <[e-mail]>
>
>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.

It seems codec and DAPM is OK so far. Codec resume will be called firstly
and then DAPM. But in HDMI codec runtime resume, it will call 
snd_hdac_display_power(). This function will try to get a mutex_lock.
If it fails to get the lock, it will sleep. This will cause dapm event handler
to be called before audio power domain in display is on in the HDMI driver.

Regards,
Libin

>
>
>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
>> [e-mail]
>> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>

Takashi Iwai March 22, 2019, 9:56 a.m. UTC | #3

On Fri, 22 Mar 2019 10:12:37 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:[e-mail]]
> >Sent: Friday, March 22, 2019 5:04 PM
> >To: Yang, Libin <[e-mail]>
> >Cc: [e-mail]; [e-mail]
> >Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
> >
> >On Fri, 22 Mar 2019 07:24:56 +0100,
> >[e-mail] wrote:
> >>
> >> From: Libin Yang <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >
> >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.
> 
> It seems codec and DAPM is OK so far. Codec resume will be called firstly
> and then DAPM. But in HDMI codec runtime resume, it will call 
> snd_hdac_display_power(). This function will try to get a mutex_lock.
> If it fails to get the lock, it will sleep. This will cause dapm event handler
> to be called before audio power domain in display is on in the HDMI driver.

It implies that the calls are racy.  If they have to be called in some
order, they have to be serialized in a more better way than inserting
a random delay...


thanks,

Takashi

> 
> Regards,
> Libin
> 
> >
> >
> >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
> >> [e-mail]
> >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> >>
>

Yang, Libin March 22, 2019, 11:46 a.m. UTC | #4

Hi Takashi,

>> >
>> >On Fri, 22 Mar 2019 07:24:56 +0100,
>> >[e-mail] wrote:
>> >>
>> >> From: Libin Yang <[e-mail]>
>> >>
>> >> 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 <[e-mail]>
>> >
>> >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.
>>
>> It seems codec and DAPM is OK so far. Codec resume will be called
>> firstly and then DAPM. But in HDMI codec runtime resume, it will call
>> snd_hdac_display_power(). This function will try to get a mutex_lock.
>> If it fails to get the lock, it will sleep. This will cause dapm event
>> handler to be called before audio power domain in display is on in the HDMI
>driver.
>
>It implies that the calls are racy.  If they have to be called in some order, they
>have to be serialized in a more better way than inserting a random delay...

Yes. The code does seem to be misleading. What do you think of using
something like "wait_for_complete()"? This will be more readable.

Regards,
Libin

>
>
>thanks,
>
>Takashi
>
>>
>> Regards,
>> Libin
>>
>> >
>> >
>> >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
>> >> [e-mail]
>> >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>> >>
>>

Takashi Iwai March 22, 2019, 8:25 p.m. UTC | #5

On Fri, 22 Mar 2019 12:46:25 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> >> >
> >> >On Fri, 22 Mar 2019 07:24:56 +0100,
> >> >[e-mail] wrote:
> >> >>
> >> >> From: Libin Yang <[e-mail]>
> >> >>
> >> >> 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 <[e-mail]>
> >> >
> >> >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.
> >>
> >> It seems codec and DAPM is OK so far. Codec resume will be called
> >> firstly and then DAPM. But in HDMI codec runtime resume, it will call
> >> snd_hdac_display_power(). This function will try to get a mutex_lock.
> >> If it fails to get the lock, it will sleep. This will cause dapm event
> >> handler to be called before audio power domain in display is on in the HDMI
> >driver.
> >
> >It implies that the calls are racy.  If they have to be called in some order, they
> >have to be serialized in a more better way than inserting a random delay...
> 
> Yes. The code does seem to be misleading. What do you think of using
> something like "wait_for_complete()"? This will be more readable.

Well, maybe we should create a proper device link for assuring the PM
dependency instead?  Then PM core will take care of everything like
that ugly stuff.


Takashi

> 
> Regards,
> Libin
> 
> >
> >
> >thanks,
> >
> >Takashi
> >
> >>
> >> Regards,
> >> Libin
> >>
> >> >
> >> >
> >> >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
> >> >> [e-mail]
> >> >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> >> >>
> >>
>

Yang, Libin March 25, 2019, 6:51 a.m. UTC | #6

Hi Takashi,

>-----Original Message-----
>From: Takashi Iwai [mailto:[e-mail]]
>Sent: Saturday, March 23, 2019 4:26 AM
>To: Yang, Libin <[e-mail]>
>Cc: [e-mail]; [e-mail]
>Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
>
>On Fri, 22 Mar 2019 12:46:25 +0100,
>Yang, Libin wrote:
>>
>> Hi Takashi,
>>
>> >> >
>> >> >On Fri, 22 Mar 2019 07:24:56 +0100, [e-mail] wrote:
>> >> >>
>> >> >> From: Libin Yang <[e-mail]>
>> >> >>
>> >> >> 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 <[e-mail]>
>> >> >
>> >> >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.
>> >>
>> >> It seems codec and DAPM is OK so far. Codec resume will be called
>> >> firstly and then DAPM. But in HDMI codec runtime resume, it will
>> >> call snd_hdac_display_power(). This function will try to get a mutex_lock.
>> >> If it fails to get the lock, it will sleep. This will cause dapm
>> >> event handler to be called before audio power domain in display is
>> >> on in the HDMI
>> >driver.
>> >
>> >It implies that the calls are racy.  If they have to be called in
>> >some order, they have to be serialized in a more better way than inserting
>a random delay...
>>
>> Yes. The code does seem to be misleading. What do you think of using
>> something like "wait_for_complete()"? This will be more readable.
>
>Well, maybe we should create a proper device link for assuring the PM
>dependency instead?  Then PM core will take care of everything like that ugly
>stuff.

Thanks for the suggestion. I did some investigation based on your suggestion
today. Below is the result on my sof platform:
There are 3 devices we concerned in this case:
machine_device: this is the device of the machine, its parent is pci_device
hdmi_device: this is the device of the hdmi codec, its parent is pci_device
pci_device: this is the device of the pci

When resume, pci_device will be resumed firstly, and then machine_device 
and hdmi_device.
When machine_device is resumed, it will schedule a work to call the
xxx_event(). So the xxx_event() will be run later.
When hdmi_device is resumed, it will try to turn on display power. However,
it may sleep if it can't get the mutex_lock. 

If hdmi_device sleeps, xxx_event() will be called firstly. However, this is wrong,
because xxx_event() depends on the display power.

So if we want to make sure machine_device resume is called always after
hdmi_device resume, we must set hdmi_device being the parent machine_device.
This seems a little weird because codecs are different for different platforms.
And what's more, one platform may have several codecs. It's hard to say which
codec should be the parent.

There is another solution I can figure out. We can do power on display power
in each xxx_event() manually. However, this may not be a very good solution.
There are several xxx_event() functions. So each time power on, the 
display power will be turned on several time. 

As we can make sure that display power is turned on already or will be turned
on very soon in the future. What about we just wait for completion of the 
display power on?

Regards,
Libin

>
>
>Takashi
>
>>
>> Regards,
>> Libin
>>
>> >
>> >
>> >thanks,
>> >
>> >Takashi
>> >
>> >>
>> >> Regards,
>> >> Libin
>> >>
>> >> >
>> >> >
>> >> >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
>> >> >> [e-mail]
>> >> >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>> >> >>
>> >>
>>

Takashi Iwai March 25, 2019, 10:08 a.m. UTC | #7

On Mon, 25 Mar 2019 07:51:11 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:[e-mail]]
> >Sent: Saturday, March 23, 2019 4:26 AM
> >To: Yang, Libin <[e-mail]>
> >Cc: [e-mail]; [e-mail]
> >Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
> >
> >On Fri, 22 Mar 2019 12:46:25 +0100,
> >Yang, Libin wrote:
> >>
> >> Hi Takashi,
> >>
> >> >> >
> >> >> >On Fri, 22 Mar 2019 07:24:56 +0100, [e-mail] wrote:
> >> >> >>
> >> >> >> From: Libin Yang <[e-mail]>
> >> >> >>
> >> >> >> 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 <[e-mail]>
> >> >> >
> >> >> >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.
> >> >>
> >> >> It seems codec and DAPM is OK so far. Codec resume will be called
> >> >> firstly and then DAPM. But in HDMI codec runtime resume, it will
> >> >> call snd_hdac_display_power(). This function will try to get a mutex_lock.
> >> >> If it fails to get the lock, it will sleep. This will cause dapm
> >> >> event handler to be called before audio power domain in display is
> >> >> on in the HDMI
> >> >driver.
> >> >
> >> >It implies that the calls are racy.  If they have to be called in
> >> >some order, they have to be serialized in a more better way than inserting
> >a random delay...
> >>
> >> Yes. The code does seem to be misleading. What do you think of using
> >> something like "wait_for_complete()"? This will be more readable.
> >
> >Well, maybe we should create a proper device link for assuring the PM
> >dependency instead?  Then PM core will take care of everything like that ugly
> >stuff.
> 
> Thanks for the suggestion. I did some investigation based on your suggestion
> today. Below is the result on my sof platform:
> There are 3 devices we concerned in this case:
> machine_device: this is the device of the machine, its parent is pci_device
> hdmi_device: this is the device of the hdmi codec, its parent is pci_device
> pci_device: this is the device of the pci
> 
> When resume, pci_device will be resumed firstly, and then machine_device 
> and hdmi_device.
> When machine_device is resumed, it will schedule a work to call the
> xxx_event(). So the xxx_event() will be run later.
> When hdmi_device is resumed, it will try to turn on display power. However,
> it may sleep if it can't get the mutex_lock. 
> 
> If hdmi_device sleeps, xxx_event() will be called firstly. However, this is wrong,
> because xxx_event() depends on the display power.
> 
> So if we want to make sure machine_device resume is called always after
> hdmi_device resume, we must set hdmi_device being the parent machine_device.
> This seems a little weird because codecs are different for different platforms.
> And what's more, one platform may have several codecs. It's hard to say which
> codec should be the parent.

Ah, it's an async work.  Then the device link won't work as expected.
(FWIW, you can define the PM dependency and call order via
 device_link_add() calls even if they are no device parent/child
 relationship.  I thought it would work in this case, but it's
 effective only for the direct invocation from the PM callbacks.)

> There is another solution I can figure out. We can do power on display power
> in each xxx_event() manually. However, this may not be a very good solution.
> There are several xxx_event() functions. So each time power on, the 
> display power will be turned on several time. 
> 
> As we can make sure that display power is turned on already or will be turned
> on very soon in the future. What about we just wait for completion of the 
> display power on?

I don't think that's the overhead, rather it's the right solution.
You just need to assure the runtime PM via wrapping
pm_runtime_get_sync() and the counterpart in each place where you need
the display power in an async code path.


thanks,

Takashi

> 
> Regards,
> Libin
> 
> >
> >
> >Takashi
> >
> >>
> >> Regards,
> >> Libin
> >>
> >> >
> >> >
> >> >thanks,
> >> >
> >> >Takashi
> >> >
> >> >>
> >> >> Regards,
> >> >> Libin
> >> >>
> >> >> >
> >> >> >
> >> >> >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
> >> >> >> [e-mail]
> >> >> >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> >> >> >>
> >> >>
> >>
>

Yang, Libin March 25, 2019, 2:27 p.m. UTC | #8

Hi Takashi,

>-----Original Message-----
>From: Takashi Iwai [mailto:[e-mail]]
>Sent: Monday, March 25, 2019 6:08 PM
>To: Yang, Libin <[e-mail]>
>Cc: [e-mail]; [e-mail]
>Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
>
>On Mon, 25 Mar 2019 07:51:11 +0100,
>Yang, Libin wrote:
>>
>> Hi Takashi,
>>
>> >-----Original Message-----
>> >From: Takashi Iwai [mailto:[e-mail]]
>> >Sent: Saturday, March 23, 2019 4:26 AM
>> >To: Yang, Libin <[e-mail]>
>> >Cc: [e-mail]; [e-mail]
>> >Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver
>> >contest in S3
>> >
>> >On Fri, 22 Mar 2019 12:46:25 +0100,
>> >Yang, Libin wrote:
>> >>
>> >> Hi Takashi,
>> >>
>> >> >> >
>> >> >> >On Fri, 22 Mar 2019 07:24:56 +0100, [e-mail] wrote:
>> >> >> >>
>> >> >> >> From: Libin Yang <[e-mail]>
>> >> >> >>
>> >> >> >> 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 <[e-mail]>
>> >> >> >
>> >> >> >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.
>> >> >>
>> >> >> It seems codec and DAPM is OK so far. Codec resume will be
>> >> >> called firstly and then DAPM. But in HDMI codec runtime resume,
>> >> >> it will call snd_hdac_display_power(). This function will try to get a
>mutex_lock.
>> >> >> If it fails to get the lock, it will sleep. This will cause dapm
>> >> >> event handler to be called before audio power domain in display
>> >> >> is on in the HDMI
>> >> >driver.
>> >> >
>> >> >It implies that the calls are racy.  If they have to be called in
>> >> >some order, they have to be serialized in a more better way than
>> >> >inserting
>> >a random delay...
>> >>
>> >> Yes. The code does seem to be misleading. What do you think of
>> >> using something like "wait_for_complete()"? This will be more readable.
>> >
>> >Well, maybe we should create a proper device link for assuring the PM
>> >dependency instead?  Then PM core will take care of everything like
>> >that ugly stuff.
>>
>> Thanks for the suggestion. I did some investigation based on your
>> suggestion today. Below is the result on my sof platform:
>> There are 3 devices we concerned in this case:
>> machine_device: this is the device of the machine, its parent is
>> pci_device
>> hdmi_device: this is the device of the hdmi codec, its parent is
>> pci_device
>> pci_device: this is the device of the pci
>>
>> When resume, pci_device will be resumed firstly, and then
>> machine_device and hdmi_device.
>> When machine_device is resumed, it will schedule a work to call the
>> xxx_event(). So the xxx_event() will be run later.
>> When hdmi_device is resumed, it will try to turn on display power.
>> However, it may sleep if it can't get the mutex_lock.
>>
>> If hdmi_device sleeps, xxx_event() will be called firstly. However,
>> this is wrong, because xxx_event() depends on the display power.
>>
>> So if we want to make sure machine_device resume is called always
>> after hdmi_device resume, we must set hdmi_device being the parent
>machine_device.
>> This seems a little weird because codecs are different for different platforms.
>> And what's more, one platform may have several codecs. It's hard to
>> say which codec should be the parent.
>
>Ah, it's an async work.  Then the device link won't work as expected.
>(FWIW, you can define the PM dependency and call order via
> device_link_add() calls even if they are no device parent/child  relationship.  I
>thought it would work in this case, but it's  effective only for the direct
>invocation from the PM callbacks.)
>
>> There is another solution I can figure out. We can do power on display
>> power in each xxx_event() manually. However, this may not be a very good
>solution.
>> There are several xxx_event() functions. So each time power on, the
>> display power will be turned on several time.
>>
>> As we can make sure that display power is turned on already or will be
>> turned on very soon in the future. What about we just wait for
>> completion of the display power on?
>
>I don't think that's the overhead, rather it's the right solution.
>You just need to assure the runtime PM via wrapping
>pm_runtime_get_sync() and the counterpart in each place where you need
>the display power in an async code path.

OK. I will try to get display power in each xxx_event(). I tried to call 
pm_runtime_get_sync() in xxx_event() to turn on the display power
before. As it's already in the resume process, the runtime PM will
not be called again. So it doesn't help. It seems I have to call
snd_hdac_display_power() manually. Another potential issue is
if I called snd_hdac_display_power(on) in xxx_event(), I also
need call snd_hdac_display_power(off) at the end of xxx_event()?
If so, the display power may really be turned off as currently
snd_hdac_display_power() doesn't use counter, but itis using 
" set_bit(idx, &bus->display_power_status)" and 
" clear_bit(idx, &bus->display_power_status)" to track the power.

Let's assume such scenario:
Runtime pm turns on display power
Xxx_event() turns on the display power
Xxx_event() turns off the display power
Now display power may be really turned off. But actually we
don't want to turn off display power off here. This will be
wrong.

Regards,
Libin

>
>
>thanks,
>
>Takashi
>
>>
>> Regards,
>> Libin
>>
>> >
>> >
>> >Takashi
>> >
>> >>
>> >> Regards,
>> >> Libin
>> >>
>> >> >
>> >> >
>> >> >thanks,
>> >> >
>> >> >Takashi
>> >> >
>> >> >>
>> >> >> Regards,
>> >> >> Libin
>> >> >>
>> >> >> >
>> >> >> >
>> >> >> >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
>> >> >> >> [e-mail]
>> >> >> >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>> >> >> >>
>> >> >>
>> >>
>>

Takashi Iwai March 25, 2019, 2:38 p.m. UTC | #9

On Mon, 25 Mar 2019 15:27:18 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:[e-mail]]
> >Sent: Monday, March 25, 2019 6:08 PM
> >To: Yang, Libin <[e-mail]>
> >Cc: [e-mail]; [e-mail]
> >Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
> >
> >On Mon, 25 Mar 2019 07:51:11 +0100,
> >Yang, Libin wrote:
> >>
> >> Hi Takashi,
> >>
> >> >-----Original Message-----
> >> >From: Takashi Iwai [mailto:[e-mail]]
> >> >Sent: Saturday, March 23, 2019 4:26 AM
> >> >To: Yang, Libin <[e-mail]>
> >> >Cc: [e-mail]; [e-mail]
> >> >Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver
> >> >contest in S3
> >> >
> >> >On Fri, 22 Mar 2019 12:46:25 +0100,
> >> >Yang, Libin wrote:
> >> >>
> >> >> Hi Takashi,
> >> >>
> >> >> >> >
> >> >> >> >On Fri, 22 Mar 2019 07:24:56 +0100, [e-mail] wrote:
> >> >> >> >>
> >> >> >> >> From: Libin Yang <[e-mail]>
> >> >> >> >>
> >> >> >> >> 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 <[e-mail]>
> >> >> >> >
> >> >> >> >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.
> >> >> >>
> >> >> >> It seems codec and DAPM is OK so far. Codec resume will be
> >> >> >> called firstly and then DAPM. But in HDMI codec runtime resume,
> >> >> >> it will call snd_hdac_display_power(). This function will try to get a
> >mutex_lock.
> >> >> >> If it fails to get the lock, it will sleep. This will cause dapm
> >> >> >> event handler to be called before audio power domain in display
> >> >> >> is on in the HDMI
> >> >> >driver.
> >> >> >
> >> >> >It implies that the calls are racy.  If they have to be called in
> >> >> >some order, they have to be serialized in a more better way than
> >> >> >inserting
> >> >a random delay...
> >> >>
> >> >> Yes. The code does seem to be misleading. What do you think of
> >> >> using something like "wait_for_complete()"? This will be more readable.
> >> >
> >> >Well, maybe we should create a proper device link for assuring the PM
> >> >dependency instead?  Then PM core will take care of everything like
> >> >that ugly stuff.
> >>
> >> Thanks for the suggestion. I did some investigation based on your
> >> suggestion today. Below is the result on my sof platform:
> >> There are 3 devices we concerned in this case:
> >> machine_device: this is the device of the machine, its parent is
> >> pci_device
> >> hdmi_device: this is the device of the hdmi codec, its parent is
> >> pci_device
> >> pci_device: this is the device of the pci
> >>
> >> When resume, pci_device will be resumed firstly, and then
> >> machine_device and hdmi_device.
> >> When machine_device is resumed, it will schedule a work to call the
> >> xxx_event(). So the xxx_event() will be run later.
> >> When hdmi_device is resumed, it will try to turn on display power.
> >> However, it may sleep if it can't get the mutex_lock.
> >>
> >> If hdmi_device sleeps, xxx_event() will be called firstly. However,
> >> this is wrong, because xxx_event() depends on the display power.
> >>
> >> So if we want to make sure machine_device resume is called always
> >> after hdmi_device resume, we must set hdmi_device being the parent
> >machine_device.
> >> This seems a little weird because codecs are different for different platforms.
> >> And what's more, one platform may have several codecs. It's hard to
> >> say which codec should be the parent.
> >
> >Ah, it's an async work.  Then the device link won't work as expected.
> >(FWIW, you can define the PM dependency and call order via
> > device_link_add() calls even if they are no device parent/child  relationship.  I
> >thought it would work in this case, but it's  effective only for the direct
> >invocation from the PM callbacks.)
> >
> >> There is another solution I can figure out. We can do power on display
> >> power in each xxx_event() manually. However, this may not be a very good
> >solution.
> >> There are several xxx_event() functions. So each time power on, the
> >> display power will be turned on several time.
> >>
> >> As we can make sure that display power is turned on already or will be
> >> turned on very soon in the future. What about we just wait for
> >> completion of the display power on?
> >
> >I don't think that's the overhead, rather it's the right solution.
> >You just need to assure the runtime PM via wrapping
> >pm_runtime_get_sync() and the counterpart in each place where you need
> >the display power in an async code path.
> 
> OK. I will try to get display power in each xxx_event(). I tried to call 
> pm_runtime_get_sync() in xxx_event() to turn on the display power
> before. As it's already in the resume process, the runtime PM will
> not be called again. So it doesn't help.

Isn't it called from an async work?  If yes, it should wait until the
runtime resume finishes, hence it must be already in the sanely
powered up state.  OTOH, if it's called from the same context as the
runtime resume itself, we shoot our foot.  Then it must be done
differently.

> It seems I have to call
> snd_hdac_display_power() manually.

Something goes really wrong, then.

Again, if it's the direct PM callback path, managing the PM device
order via device_link_add() should be the way to go.  If it's an async
work context, an explicit sync with pm_runtime_get_sync() is needed.


> Another potential issue is
> if I called snd_hdac_display_power(on) in xxx_event(), I also
> need call snd_hdac_display_power(off) at the end of xxx_event()?
> If so, the display power may really be turned off as currently
> snd_hdac_display_power() doesn't use counter, but itis using 
> " set_bit(idx, &bus->display_power_status)" and 
> " clear_bit(idx, &bus->display_power_status)" to track the power.
> 
> Let's assume such scenario:
> Runtime pm turns on display power
> Xxx_event() turns on the display power
> Xxx_event() turns off the display power
> Now display power may be really turned off. But actually we
> don't want to turn off display power off here. This will be
> wrong.

If the display power is solely managed by the codec driver, DAPM event
callback needs to call pm_runtime_get*() / put*() wrt the codec
driver, too.  The usage is ref-counted, so it won't go to the power
off until all users are gone.

Of course, the above assumes that xxx_event() gets called in an async
context.

Please check it again.


thanks,

Takashi

> 
> Regards,
> Libin
> 
> >
> >
> >thanks,
> >
> >Takashi
> >
> >>
> >> Regards,
> >> Libin
> >>
> >> >
> >> >
> >> >Takashi
> >> >
> >> >>
> >> >> Regards,
> >> >> Libin
> >> >>
> >> >> >
> >> >> >
> >> >> >thanks,
> >> >> >
> >> >> >Takashi
> >> >> >
> >> >> >>
> >> >> >> Regards,
> >> >> >> Libin
> >> >> >>
> >> >> >> >
> >> >> >> >
> >> >> >> >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
> >> >> >> >> [e-mail]
> >> >> >> >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> >> >> >> >>
> >> >> >>
> >> >>
> >>
>

Yang, Libin March 26, 2019, 5:42 a.m. UTC | #10

Hi Takashi,

>> >> >> >> >>
>> >> >> >> >> 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 <[e-mail]>
>> >> >> >> >
>> >> >> >> >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.
>> >> >> >>
>> >> >> >> It seems codec and DAPM is OK so far. Codec resume will be
>> >> >> >> called firstly and then DAPM. But in HDMI codec runtime
>> >> >> >> resume, it will call snd_hdac_display_power(). This function
>> >> >> >> will try to get a
>> >mutex_lock.
>> >> >> >> If it fails to get the lock, it will sleep. This will cause
>> >> >> >> dapm event handler to be called before audio power domain in
>> >> >> >> display is on in the HDMI
>> >> >> >driver.
>> >> >> >
>> >> >> >It implies that the calls are racy.  If they have to be called
>> >> >> >in some order, they have to be serialized in a more better way
>> >> >> >than inserting
>> >> >a random delay...
>> >> >>
>> >> >> Yes. The code does seem to be misleading. What do you think of
>> >> >> using something like "wait_for_complete()"? This will be more
>readable.
>> >> >
>> >> >Well, maybe we should create a proper device link for assuring the
>> >> >PM dependency instead?  Then PM core will take care of everything
>> >> >like that ugly stuff.
>> >>
>> >> Thanks for the suggestion. I did some investigation based on your
>> >> suggestion today. Below is the result on my sof platform:
>> >> There are 3 devices we concerned in this case:
>> >> machine_device: this is the device of the machine, its parent is
>> >> pci_device
>> >> hdmi_device: this is the device of the hdmi codec, its parent is
>> >> pci_device
>> >> pci_device: this is the device of the pci
>> >>
>> >> When resume, pci_device will be resumed firstly, and then
>> >> machine_device and hdmi_device.
>> >> When machine_device is resumed, it will schedule a work to call the
>> >> xxx_event(). So the xxx_event() will be run later.
>> >> When hdmi_device is resumed, it will try to turn on display power.
>> >> However, it may sleep if it can't get the mutex_lock.
>> >>
>> >> If hdmi_device sleeps, xxx_event() will be called firstly. However,
>> >> this is wrong, because xxx_event() depends on the display power.
>> >>
>> >> So if we want to make sure machine_device resume is called always
>> >> after hdmi_device resume, we must set hdmi_device being the parent
>> >machine_device.
>> >> This seems a little weird because codecs are different for different
>platforms.
>> >> And what's more, one platform may have several codecs. It's hard to
>> >> say which codec should be the parent.
>> >
>> >Ah, it's an async work.  Then the device link won't work as expected.
>> >(FWIW, you can define the PM dependency and call order via
>> > device_link_add() calls even if they are no device parent/child
>> >relationship.  I thought it would work in this case, but it's
>> >effective only for the direct invocation from the PM callbacks.)
>> >
>> >> There is another solution I can figure out. We can do power on
>> >> display power in each xxx_event() manually. However, this may not
>> >> be a very good
>> >solution.
>> >> There are several xxx_event() functions. So each time power on, the
>> >> display power will be turned on several time.
>> >>
>> >> As we can make sure that display power is turned on already or will
>> >> be turned on very soon in the future. What about we just wait for
>> >> completion of the display power on?
>> >
>> >I don't think that's the overhead, rather it's the right solution.
>> >You just need to assure the runtime PM via wrapping
>> >pm_runtime_get_sync() and the counterpart in each place where you
>> >need the display power in an async code path.
>>
>> OK. I will try to get display power in each xxx_event(). I tried to
>> call
>> pm_runtime_get_sync() in xxx_event() to turn on the display power
>> before. As it's already in the resume process, the runtime PM will not
>> be called again. So it doesn't help.
>
>Isn't it called from an async work?  If yes, it should wait until the runtime
>resume finishes, hence it must be already in the sanely powered up state.
>OTOH, if it's called from the same context as the runtime resume itself, we
>shoot our foot.  Then it must be done differently.

It's async work.

>
>> It seems I have to call
>> snd_hdac_display_power() manually.
>
>Something goes really wrong, then.
>
>Again, if it's the direct PM callback path, managing the PM device order via
>device_link_add() should be the way to go.  If it's an async work context, an
>explicit sync with pm_runtime_get_sync() is needed.

As it's async, we can't use PM device order to manage this sequence.
I tried to use pm_runtime_get_sync() and had a test, but it seems it doesn't
help. xxx_event() setting register will still be called before display power
is turned on.

>
>
>> Another potential issue is
>> if I called snd_hdac_display_power(on) in xxx_event(), I also need
>> call snd_hdac_display_power(off) at the end of xxx_event()?
>> If so, the display power may really be turned off as currently
>> snd_hdac_display_power() doesn't use counter, but itis using "
>> set_bit(idx, &bus->display_power_status)" and " clear_bit(idx,
>> &bus->display_power_status)" to track the power.
>>
>> Let's assume such scenario:
>> Runtime pm turns on display power
>> Xxx_event() turns on the display power
>> Xxx_event() turns off the display power Now display power may be
>> really turned off. But actually we don't want to turn off display
>> power off here. This will be wrong.
>
>If the display power is solely managed by the codec driver, DAPM event
>callback needs to call pm_runtime_get*() / put*() wrt the codec driver, too.
>The usage is ref-counted, so it won't go to the power off until all users are
>gone.
>
>Of course, the above assumes that xxx_event() gets called in an async context.
>
>Please check it again.

Below is my draft patch, which doesn't work with pm_runtime_get_sync(). Is
there anything wrong in my code?

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 5eeb0fe..429a831 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -763,6 +763,8 @@ static int hdac_hdmi_pin_output_widget_event(struct snd_soc_dapm_widget *w,

        switch (event) {
        case SND_SOC_DAPM_PRE_PMU:
+               pm_runtime_get_sync(&hdev->dev);
+               dev_err(&hdev->dev, "in %s %d ylb\n", __func__, __LINE__);
                hdac_hdmi_set_power_state(hdev, port->pin->nid, AC_PWRST_D0);

                /* Enable out path for this pin widget */
@@ -781,6 +783,7 @@ static int hdac_hdmi_pin_output_widget_event(struct snd_soc_dapm_widget *w,
                                AC_VERB_SET_PIN_WIDGET_CONTROL, 0);

                hdac_hdmi_set_power_state(hdev, port->pin->nid, AC_PWRST_D3);
+               pm_runtime_put_sync(&hdev->dev);
                break;

        }
@@ -805,6 +808,8 @@ static int hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w,

        switch (event) {
        case SND_SOC_DAPM_PRE_PMU:
+               pm_runtime_get_sync(&hdev->dev);
+               dev_err(&hdev->dev, "in %s %d ylb\n", __func__, __LINE__);
                hdac_hdmi_set_power_state(hdev, cvt->nid, AC_PWRST_D0);

                /* Enable transmission */
@@ -828,6 +833,7 @@ static int hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w,
                                AC_VERB_SET_STREAM_FORMAT, 0);

                hdac_hdmi_set_power_state(hdev, cvt->nid, AC_PWRST_D3);
+               pm_runtime_put_sync(&hdev->dev);
                break;

        }
@@ -847,7 +853,8 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w,

        if (!kc)
                kc  = w->kcontrols[0];
-
+       pm_runtime_get_sync(&hdev->dev);
+       dev_err(&hdev->dev, "in %s %d ylb\n", __func__, __LINE__);
        mux_idx = dapm_kcontrol_get_value(kc);

        /* set the device if pin is mst_capable */
@@ -858,6 +865,7 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w,
                snd_hdac_codec_write(hdev, port->pin->nid, 0,
                        AC_VERB_SET_CONNECT_SEL, (mux_idx - 1));
        }
+       pm_runtime_put_sync(&hdev->dev);

        return 0;
 }
@@ -1879,7 +1887,7 @@ 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;
-
+       dev_err(&hdev->dev, "in %s %d ylb\n", __func__, __LINE__);
        ret = pm_runtime_force_resume(dev);
        if (ret < 0)
                return ret;
@@ -2111,6 +2119,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
        if (!bus)
                return 0;

+       dev_err(&hdev->dev, "in %s %d ylb 1\n", __func__, __LINE__);
        hlink = snd_hdac_ext_bus_get_link(bus, dev_name(dev));
        if (!hlink) {
                dev_err(dev, "hdac link not found\n");
@@ -2123,7 +2132,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev)

        hdac_hdmi_skl_enable_all_pins(hdev);
        hdac_hdmi_skl_enable_dp12(hdev);
-
+       dev_err(&hdev->dev, "in %s %d ylb 2\n", __func__, __LINE__);
        /* Power up afg */
        snd_hdac_codec_read(hdev, hdev->afg, 0, AC_VERB_SET_POWER_STATE,
                                                        AC_PWRST_D0);
--
2.7.4

And the dmesg is:
[   36.087224] HDMI HDA Codec ehdaudio0D2: in hdmi_codec_resume 1890 ylb
[   36.087230] HDMI HDA Codec ehdaudio0D2: in hdac_hdmi_runtime_resume 2122 ylb 1
[   36.087335] HDMI HDA Codec ehdaudio0D2: in hdac_hdmi_cvt_output_widget_event 812 ylb
[   40.097586] HDMI HDA Codec ehdaudio0D2: in hdac_hdmi_runtime_resume 2135 ylb 2
[   40.097766] HDMI HDA Codec ehdaudio0D2: in hdac_hdmi_pin_output_widget_event 767 ylb
[   45.108632] HDMI HDA Codec ehdaudio0D2: in hdac_hdmi_pin_mux_widget_event 857 ylb

From the dmesg, hdac_hdmi_cvt_output_widget_event() 
is called between "ylb 1" and "ylb 2" in runtime_resume().
This means xxx_event() registers setting runs before display
power is turned on.

Regards,
Libin

>
>
>thanks,
>
>Takashi
>
>>
>> Regards,
>> Libin
>>
>> >
>> >
>> >thanks,
>> >
>> >Takashi
>> >
>> >>
>> >> Regards,
>> >> Libin
>> >>
>> >> >
>> >> >
>> >> >Takashi
>> >> >
>> >> >>
>> >> >> Regards,
>> >> >> Libin
>> >> >>
>> >> >> >
>> >> >> >
>> >> >> >thanks,
>> >> >> >
>> >> >> >Takashi
>> >> >> >
>> >> >> >>
>> >> >> >> Regards,
>> >> >> >> Libin
>> >> >> >>
>> >> >> >> >
>> >> >> >> >
>> >> >> >> >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
>> >> >> >> >> [e-mail]
>> >> >> >> >> https://mailman.alsa-project.org/mailman/listinfo/alsa-dev
>> >> >> >> >> el
>> >> >> >> >>
>> >> >> >>
>> >> >>
>> >>
>>

Takashi Iwai March 26, 2019, 7:36 a.m. UTC | #11

On Tue, 26 Mar 2019 06:42:15 +0100,
Yang, Libin wrote:
(snip)
> Hi Takashi,
> Below is my draft patch, which doesn't work with pm_runtime_get_sync(). Is
> there anything wrong in my code?
(snip)
> And the dmesg is:
> [   36.087224] HDMI HDA Codec ehdaudio0D2: in hdmi_codec_resume 1890 ylb
> [   36.087230] HDMI HDA Codec ehdaudio0D2: in hdac_hdmi_runtime_resume 2122 ylb 1
> [   36.087335] HDMI HDA Codec ehdaudio0D2: in hdac_hdmi_cvt_output_widget_event 812 ylb
> [   40.097586] HDMI HDA Codec ehdaudio0D2: in hdac_hdmi_runtime_resume 2135 ylb 2
> [   40.097766] HDMI HDA Codec ehdaudio0D2: in hdac_hdmi_pin_output_widget_event 767 ylb
> [   45.108632] HDMI HDA Codec ehdaudio0D2: in hdac_hdmi_pin_mux_widget_event 857 ylb
> 
> >From the dmesg, hdac_hdmi_cvt_output_widget_event() 
> is called between "ylb 1" and "ylb 2" in runtime_resume().
> This means xxx_event() registers setting runs before display
> power is turned on.

OK, now I understood what went wrong.  It's a similar issue as we've
hit for the legacy HD-audio and figured out recently.

If my understanding is correct, the problem is the call of
pm_runtime_force_resume() in PM resume callback combined with an async
work.  pm_runtime_force_resume() sets the runtime state immediately to
RPM_ACTIVE even before finishing the task.  The code seems assuming
blindly that there is no other conflicting task while S3/S4 resume,
but this is a problem.  That's why the next pm_runtime_get_sync()
wasn't blocked but just went through; since the RPM flag was already
set to RPM_ACTIVE in pm_runtime_force_resume(), it thought as if it
were already active, while the real runtime resume code was still
processing the callback.

In the case of legacy HD-audio, we "fixed" the problem by avoiding the
trigger of async work at resume, but let it be triggered from runtime
resume.  In ASoC case, however, it's no option.

Maybe a possible solution in the sound driver side would be to move
from system suspend/resume to ASoC component suspend/resume.  The
runtime suspend/resume can be kept as is, and call
pm_runtime_force_suspend and resume from the component suspend/resume
callbacks.  Since the component callbacks are certainly processed
before DAPM events, this should assure the order.


thanks,

Takashi

Yang, Libin March 26, 2019, 7:58 a.m. UTC | #12

Hi Takashi,


>-----Original Message-----
>From: Takashi Iwai [mailto:[e-mail]]
>Sent: Tuesday, March 26, 2019 3:37 PM
>To: Yang, Libin <[e-mail]>
>Cc: [e-mail]; [e-mail]
>Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
>
>On Tue, 26 Mar 2019 06:42:15 +0100,
>Yang, Libin wrote:
>(snip)
>> Hi Takashi,
>> Below is my draft patch, which doesn't work with
>> pm_runtime_get_sync(). Is there anything wrong in my code?
>(snip)
>> And the dmesg is:
>> [   36.087224] HDMI HDA Codec ehdaudio0D2: in hdmi_codec_resume 1890
>ylb
>> [   36.087230] HDMI HDA Codec ehdaudio0D2: in
>hdac_hdmi_runtime_resume 2122 ylb 1
>> [   36.087335] HDMI HDA Codec ehdaudio0D2: in
>hdac_hdmi_cvt_output_widget_event 812 ylb
>> [   40.097586] HDMI HDA Codec ehdaudio0D2: in
>hdac_hdmi_runtime_resume 2135 ylb 2
>> [   40.097766] HDMI HDA Codec ehdaudio0D2: in
>hdac_hdmi_pin_output_widget_event 767 ylb
>> [   45.108632] HDMI HDA Codec ehdaudio0D2: in
>hdac_hdmi_pin_mux_widget_event 857 ylb
>>
>> >From the dmesg, hdac_hdmi_cvt_output_widget_event()
>> is called between "ylb 1" and "ylb 2" in runtime_resume().
>> This means xxx_event() registers setting runs before display power is
>> turned on.
>
>OK, now I understood what went wrong.  It's a similar issue as we've hit for
>the legacy HD-audio and figured out recently.
>
>If my understanding is correct, the problem is the call of
>pm_runtime_force_resume() in PM resume callback combined with an async
>work.  pm_runtime_force_resume() sets the runtime state immediately to
>RPM_ACTIVE even before finishing the task.  The code seems assuming blindly
>that there is no other conflicting task while S3/S4 resume, but this is a
>problem.  That's why the next pm_runtime_get_sync() wasn't blocked but just
>went through; since the RPM flag was already set to RPM_ACTIVE in
>pm_runtime_force_resume(), it thought as if it were already active, while the
>real runtime resume code was still processing the callback.

Yes, exactly right. And I have checked dev->power.runtime_status, it's
RPM_ACTIVE when xxx_event() calls pm_runtime_get_sync().

>
>In the case of legacy HD-audio, we "fixed" the problem by avoiding the trigger
>of async work at resume, but let it be triggered from runtime resume.  In ASoC
>case, however, it's no option.
>
>Maybe a possible solution in the sound driver side would be to move from
>system suspend/resume to ASoC component suspend/resume.  The runtime
>suspend/resume can be kept as is, and call pm_runtime_force_suspend and
>resume from the component suspend/resume callbacks.  Since the
>component callbacks are certainly processed before DAPM events, this should
>assure the order.

I have worked out another patch. This patch decouples the xxx_event() and
runtime suspend/resume. This means in whichever case, xxx_event() can make
sure display power is in the correct status. What do you think?
On the same time, I will try the component suspend/resume. My understanding
is that snd_hdac_display_power() should be called either in runtime_suspend/
resume or in component suspend/resume. 

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 5eeb0fe..2acb7f1 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -144,7 +144,9 @@ struct hdac_hdmi_priv {
        int num_pin;
        int num_cvt;
        int num_ports;
+       int power_cnt;
        struct mutex pin_mutex;
+       struct mutex power_mutex;
        struct hdac_chmap chmap;
        struct hdac_hdmi_drv_data *drv_data;
        struct snd_soc_dai_driver *dai_drv;
@@ -286,6 +288,79 @@ static struct hdac_hdmi_pcm *get_hdmi_pcm_from_id(struct hdac_hdmi_priv *hdmi,
        return NULL;
 }

+#define INTEL_VENDOR_NID_0x2 0x02
+#define INTEL_VENDOR_NID_0x8 0x08
+#define INTEL_VENDOR_NID_0xb 0x0b
+#define INTEL_GET_VENDOR_VERB 0xf81
+#define INTEL_SET_VENDOR_VERB 0x781
+#define INTEL_EN_DP12          0x02 /* enable DP 1.2 features */
+#define INTEL_EN_ALL_PIN_CVTS  0x01 /* enable 2nd & 3rd pins and convertors */
+
+static void hdac_hdmi_skl_enable_all_pins(struct hdac_device *hdev)
+{
+       unsigned int vendor_param;
+       struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
+       unsigned int vendor_nid = hdmi->drv_data->vendor_nid;
+
+       vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0,
+                               INTEL_GET_VENDOR_VERB, 0);
+       if (vendor_param == -1 || vendor_param & INTEL_EN_ALL_PIN_CVTS)
+               return;
+
+       vendor_param |= INTEL_EN_ALL_PIN_CVTS;
+       vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0,
+                               INTEL_SET_VENDOR_VERB, vendor_param);
+       if (vendor_param == -1)
+               return;
+}
+
+static void hdac_hdmi_skl_enable_dp12(struct hdac_device *hdev)
+{
+       unsigned int vendor_param;
+       struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
+       unsigned int vendor_nid = hdmi->drv_data->vendor_nid;
+
+       vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0,
+                               INTEL_GET_VENDOR_VERB, 0);
+       if (vendor_param == -1 || vendor_param & INTEL_EN_DP12)
+               return;
+
+       /* enable DP1.2 mode */
+       vendor_param |= INTEL_EN_DP12;
+       vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0,
+                               INTEL_SET_VENDOR_VERB, vendor_param);
+       if (vendor_param == -1)
+               return;
+
+}
+
+static void put_display_power(struct hdac_device *hdev)
+{
+       struct hdac_bus *bus = hdev->bus;
+       struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
+
+       mutex_lock(&hdmi->power_mutex);
+       hdmi->power_cnt--;
+       if (hdmi->power_cnt == 0)
+               snd_hdac_display_power(bus, hdev->addr, false);
+       mutex_unlock(&hdmi->power_mutex);
+}
+
+static void get_display_power(struct hdac_device *hdev)
+{
+       struct hdac_bus *bus = hdev->bus;
+       struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
+
+       mutex_lock(&hdmi->power_mutex);
+       if (hdmi->power_cnt == 0) {
+               snd_hdac_display_power(bus, hdev->addr, true);
+               hdac_hdmi_skl_enable_all_pins(hdev);
+               hdac_hdmi_skl_enable_dp12(hdev);
+       }
+       hdmi->power_cnt++;
+       mutex_unlock(&hdmi->power_mutex);
+}
+
 static unsigned int sad_format(const u8 *sad)
 {
        return ((sad[0] >> 0x3) & 0x1f);
@@ -749,17 +824,21 @@ static int hdac_hdmi_pin_output_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_pcm *pcm;
+       int ret = 0;

        dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n",
                        __func__, w->name, event);

+       get_display_power(hdev);
        pcm = hdac_hdmi_get_pcm(hdev, port);
        if (!pcm)
                return -EIO;

        /* set the device if pin is mst_capable */
-       if (hdac_hdmi_port_select_set(hdev, port) < 0)
+       if (hdac_hdmi_port_select_set(hdev, port) < 0) {
+               put_display_power(hdev);
                return -EIO;
+       }

        switch (event) {
        case SND_SOC_DAPM_PRE_PMU:
@@ -771,7 +850,8 @@ static int hdac_hdmi_pin_output_widget_event(struct snd_soc_dapm_widget *w,

                hdac_hdmi_set_amp(hdev, port->pin->nid, AMP_OUT_UNMUTE);

-               return hdac_hdmi_setup_audio_infoframe(hdev, pcm, port);
+               ret = hdac_hdmi_setup_audio_infoframe(hdev, pcm, port);
+               break;

        case SND_SOC_DAPM_POST_PMD:
                hdac_hdmi_set_amp(hdev, port->pin->nid, AMP_OUT_MUTE);
@@ -784,8 +864,9 @@ static int hdac_hdmi_pin_output_widget_event(struct snd_soc_dapm_widget *w,
                break;

        }
+       put_display_power(hdev);

-       return 0;
+       return ret;
 }

 static int hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w,
@@ -803,6 +884,7 @@ static int hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w,
        if (!pcm)
                return -EIO;

+       get_display_power(hdev);
        switch (event) {
        case SND_SOC_DAPM_PRE_PMU:
                hdac_hdmi_set_power_state(hdev, cvt->nid, AC_PWRST_D0);
@@ -831,6 +913,7 @@ static int hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w,
                break;

        }
+       put_display_power(hdev);

        return 0;
}
@@ -850,15 +933,20 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w,

        mux_idx = dapm_kcontrol_get_value(kc);

+       get_display_power(hdev);
        /* set the device if pin is mst_capable */
-       if (hdac_hdmi_port_select_set(hdev, port) < 0)
+       if (hdac_hdmi_port_select_set(hdev, port) < 0) {
+               put_display_power(hdev);
                return -EIO;
+       }

        if (mux_idx > 0) {
                snd_hdac_codec_write(hdev, port->pin->nid, 0,
                        AC_VERB_SET_CONNECT_SEL, (mux_idx - 1));
        }

+       put_display_power(hdev);
+
        return 0;
 }

@@ -1339,52 +1427,6 @@ static int hdac_hdmi_add_pin(struct hdac_device *hdev, hda_nid_t nid)
        return 0;
 }

-#define INTEL_VENDOR_NID_0x2 0x02
-#define INTEL_VENDOR_NID_0x8 0x08
-#define INTEL_VENDOR_NID_0xb 0x0b
-#define INTEL_GET_VENDOR_VERB 0xf81
-#define INTEL_SET_VENDOR_VERB 0x781
-#define INTEL_EN_DP12          0x02 /* enable DP 1.2 features */
-#define INTEL_EN_ALL_PIN_CVTS  0x01 /* enable 2nd & 3rd pins and convertors */
-
-static void hdac_hdmi_skl_enable_all_pins(struct hdac_device *hdev)
-{
-       unsigned int vendor_param;
-       struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
-       unsigned int vendor_nid = hdmi->drv_data->vendor_nid;
-
-       vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0,
-                               INTEL_GET_VENDOR_VERB, 0);
-       if (vendor_param == -1 || vendor_param & INTEL_EN_ALL_PIN_CVTS)
-               return;
-
-       vendor_param |= INTEL_EN_ALL_PIN_CVTS;
-       vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0,
-                               INTEL_SET_VENDOR_VERB, vendor_param);
-       if (vendor_param == -1)
-               return;
-}
-
-static void hdac_hdmi_skl_enable_dp12(struct hdac_device *hdev)
-{
-       unsigned int vendor_param;
-       struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
-       unsigned int vendor_nid = hdmi->drv_data->vendor_nid;
-
-       vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0,
-                               INTEL_GET_VENDOR_VERB, 0);
-       if (vendor_param == -1 || vendor_param & INTEL_EN_DP12)
-               return;
-
-       /* enable DP1.2 mode */
-       vendor_param |= INTEL_EN_DP12;
-       vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0,
-                               INTEL_SET_VENDOR_VERB, vendor_param);
-       if (vendor_param == -1)
-               return;
-
-}
-
static const struct snd_soc_dai_ops hdmi_dai_ops = {
        .startup = hdac_hdmi_pcm_open,
        .shutdown = hdac_hdmi_pcm_close,
@@ -1473,9 +1515,6 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_device *hdev,
        struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
        int ret;

-       hdac_hdmi_skl_enable_all_pins(hdev);
-       hdac_hdmi_skl_enable_dp12(hdev);
-
        num_nodes = snd_hdac_get_sub_nodes(hdev, hdev->afg, &nid);
        if (!nid || num_nodes <= 0) {
                dev_warn(&hdev->dev, "HDMI: failed to get afg sub nodes\n");
@@ -2032,12 +2071,13 @@ static int hdac_hdmi_dev_probe(struct hdac_device *hdev)
        INIT_LIST_HEAD(&hdmi_priv->cvt_list);
        INIT_LIST_HEAD(&hdmi_priv->pcm_list);
        mutex_init(&hdmi_priv->pin_mutex);
+       mutex_init(&hdmi_priv->power_mutex);

        /*
         * Turned off in the runtime_suspend during the first explicit
         * pm_runtime_suspend call.
         */
-       snd_hdac_display_power(hdev->bus, hdev->addr, true);
+       get_display_power(hdev);

        ret = hdac_hdmi_parse_and_map_nid(hdev, &hdmi_dais, &num_dais);
        if (ret < 0) {
@@ -2058,7 +2098,7 @@ static int hdac_hdmi_dev_probe(struct hdac_device *hdev)

 static int hdac_hdmi_dev_remove(struct hdac_device *hdev)
 {
-       snd_hdac_display_power(hdev->bus, hdev->addr, false);
+       put_display_power(hdev);

        return 0;
}
@@ -2094,7 +2134,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);
+       put_display_power(hdev);

        return 0;
 }
@@ -2119,11 +2159,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev)

        snd_hdac_ext_bus_link_get(bus, hlink);

-       snd_hdac_display_power(bus, hdev->addr, true);
-
-       hdac_hdmi_skl_enable_all_pins(hdev);
-       hdac_hdmi_skl_enable_dp12(hdev);
-
+       get_display_power(hdev);
        /* Power up afg */
        snd_hdac_codec_read(hdev, hdev->afg, 0, AC_VERB_SET_POWER_STATE,
                                                        AC_PWRST_D0);

>
>
>thanks,
>
>Takashi

Yang, Libin March 26, 2019, 8:46 a.m. UTC | #13

Hi Takashi

>
>In the case of legacy HD-audio, we "fixed" the problem by avoiding the trigger
>of async work at resume, but let it be triggered from runtime resume.  In ASoC
>case, however, it's no option.
>
>Maybe a possible solution in the sound driver side would be to move from
>system suspend/resume to ASoC component suspend/resume.  The runtime
>suspend/resume can be kept as is, and call pm_runtime_force_suspend and
>resume from the component suspend/resume callbacks.  Since the
>component callbacks are certainly processed before DAPM events, this should
>assure the order.

I tried to move display power setting from runtime suspend/resume to 
component suspend/resume. I found there may be another issue:
playback will NOT call component suspend/resume.
This  means we will never have chance to set the display power if
we don't do the S3.

Regards,
Libin

>
>
>thanks,
>
>Takashi

Takashi Iwai March 26, 2019, 8:53 a.m. UTC | #14

On Tue, 26 Mar 2019 09:46:31 +0100,
Yang, Libin wrote:
> 
> Hi Takashi
> 
> >
> >In the case of legacy HD-audio, we "fixed" the problem by avoiding the trigger
> >of async work at resume, but let it be triggered from runtime resume.  In ASoC
> >case, however, it's no option.
> >
> >Maybe a possible solution in the sound driver side would be to move from
> >system suspend/resume to ASoC component suspend/resume.  The runtime
> >suspend/resume can be kept as is, and call pm_runtime_force_suspend and
> >resume from the component suspend/resume callbacks.  Since the
> >component callbacks are certainly processed before DAPM events, this should
> >assure the order.
> 
> I tried to move display power setting from runtime suspend/resume to 
> component suspend/resume. I found there may be another issue:
> playback will NOT call component suspend/resume.
> This  means we will never have chance to set the display power if
> we don't do the S3.

The runtime suspend/resume are fine and need to be kept, I suppose.
The only problem is the system suspend/resume callbacks that call
pm_runtime_force_*().  Just moving these two to component would work?


Takashi

Takashi Iwai March 26, 2019, 8:59 a.m. UTC | #15

On Tue, 26 Mar 2019 08:58:34 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:[e-mail]]
> >Sent: Tuesday, March 26, 2019 3:37 PM
> >To: Yang, Libin <[e-mail]>
> >Cc: [e-mail]; [e-mail]
> >Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
> >
> >On Tue, 26 Mar 2019 06:42:15 +0100,
> >Yang, Libin wrote:
> >(snip)
> >> Hi Takashi,
> >> Below is my draft patch, which doesn't work with
> >> pm_runtime_get_sync(). Is there anything wrong in my code?
> >(snip)
> >> And the dmesg is:
> >> [   36.087224] HDMI HDA Codec ehdaudio0D2: in hdmi_codec_resume 1890
> >ylb
> >> [   36.087230] HDMI HDA Codec ehdaudio0D2: in
> >hdac_hdmi_runtime_resume 2122 ylb 1
> >> [   36.087335] HDMI HDA Codec ehdaudio0D2: in
> >hdac_hdmi_cvt_output_widget_event 812 ylb
> >> [   40.097586] HDMI HDA Codec ehdaudio0D2: in
> >hdac_hdmi_runtime_resume 2135 ylb 2
> >> [   40.097766] HDMI HDA Codec ehdaudio0D2: in
> >hdac_hdmi_pin_output_widget_event 767 ylb
> >> [   45.108632] HDMI HDA Codec ehdaudio0D2: in
> >hdac_hdmi_pin_mux_widget_event 857 ylb
> >>
> >> >From the dmesg, hdac_hdmi_cvt_output_widget_event()
> >> is called between "ylb 1" and "ylb 2" in runtime_resume().
> >> This means xxx_event() registers setting runs before display power is
> >> turned on.
> >
> >OK, now I understood what went wrong.  It's a similar issue as we've hit for
> >the legacy HD-audio and figured out recently.
> >
> >If my understanding is correct, the problem is the call of
> >pm_runtime_force_resume() in PM resume callback combined with an async
> >work.  pm_runtime_force_resume() sets the runtime state immediately to
> >RPM_ACTIVE even before finishing the task.  The code seems assuming blindly
> >that there is no other conflicting task while S3/S4 resume, but this is a
> >problem.  That's why the next pm_runtime_get_sync() wasn't blocked but just
> >went through; since the RPM flag was already set to RPM_ACTIVE in
> >pm_runtime_force_resume(), it thought as if it were already active, while the
> >real runtime resume code was still processing the callback.
> 
> Yes, exactly right. And I have checked dev->power.runtime_status, it's
> RPM_ACTIVE when xxx_event() calls pm_runtime_get_sync().
> 
> >
> >In the case of legacy HD-audio, we "fixed" the problem by avoiding the trigger
> >of async work at resume, but let it be triggered from runtime resume.  In ASoC
> >case, however, it's no option.
> >
> >Maybe a possible solution in the sound driver side would be to move from
> >system suspend/resume to ASoC component suspend/resume.  The runtime
> >suspend/resume can be kept as is, and call pm_runtime_force_suspend and
> >resume from the component suspend/resume callbacks.  Since the
> >component callbacks are certainly processed before DAPM events, this should
> >assure the order.
> 
> I have worked out another patch. This patch decouples the xxx_event() and
> runtime suspend/resume. This means in whichever case, xxx_event() can make
> sure display power is in the correct status. What do you think?
> On the same time, I will try the component suspend/resume. My understanding
> is that snd_hdac_display_power() should be called either in runtime_suspend/
> resume or in component suspend/resume. 

This might work around the particular case, yes.  However it still
makes me uneasy as the root cause isn't full covered -- the other part
of runtime resume might be still pending while executing the DAPM
event.

Now, considering the idea with device_link_add() again: I guess it's
snd_soc_resume() who kicks off the DAPM even work?  If so, and if
snd_soc_resume() gets called from the machine driver, we can assure
the PM order via device_link_add() so that the codec driver is resumed
before the machine driver.  Then at the time the deferred resume work
is executed, the codec is ready, so the display power is on.


thanks,

Takashi

Yang, Libin March 26, 2019, 11:02 a.m. UTC | #16

Hi Takashi,

>> >
>> >On Tue, 26 Mar 2019 06:42:15 +0100,
>> >Yang, Libin wrote:
>> >(snip)
>> >> Hi Takashi,
>> >> Below is my draft patch, which doesn't work with
>> >> pm_runtime_get_sync(). Is there anything wrong in my code?
>> >(snip)
>> >> And the dmesg is:
>> >> [   36.087224] HDMI HDA Codec ehdaudio0D2: in hdmi_codec_resume
>1890
>> >ylb
>> >> [   36.087230] HDMI HDA Codec ehdaudio0D2: in
>> >hdac_hdmi_runtime_resume 2122 ylb 1
>> >> [   36.087335] HDMI HDA Codec ehdaudio0D2: in
>> >hdac_hdmi_cvt_output_widget_event 812 ylb
>> >> [   40.097586] HDMI HDA Codec ehdaudio0D2: in
>> >hdac_hdmi_runtime_resume 2135 ylb 2
>> >> [   40.097766] HDMI HDA Codec ehdaudio0D2: in
>> >hdac_hdmi_pin_output_widget_event 767 ylb
>> >> [   45.108632] HDMI HDA Codec ehdaudio0D2: in
>> >hdac_hdmi_pin_mux_widget_event 857 ylb
>> >>
>> >> >From the dmesg, hdac_hdmi_cvt_output_widget_event()
>> >> is called between "ylb 1" and "ylb 2" in runtime_resume().
>> >> This means xxx_event() registers setting runs before display power
>> >> is turned on.
>> >
>> >OK, now I understood what went wrong.  It's a similar issue as we've
>> >hit for the legacy HD-audio and figured out recently.
>> >
>> >If my understanding is correct, the problem is the call of
>> >pm_runtime_force_resume() in PM resume callback combined with an
>> >async work.  pm_runtime_force_resume() sets the runtime state
>> >immediately to RPM_ACTIVE even before finishing the task.  The code
>> >seems assuming blindly that there is no other conflicting task while
>> >S3/S4 resume, but this is a problem.  That's why the next
>> >pm_runtime_get_sync() wasn't blocked but just went through; since the
>> >RPM flag was already set to RPM_ACTIVE in pm_runtime_force_resume(),
>> >it thought as if it were already active, while the real runtime resume code
>was still processing the callback.
>>
>> Yes, exactly right. And I have checked dev->power.runtime_status, it's
>> RPM_ACTIVE when xxx_event() calls pm_runtime_get_sync().
>>
>> >
>> >In the case of legacy HD-audio, we "fixed" the problem by avoiding
>> >the trigger of async work at resume, but let it be triggered from
>> >runtime resume.  In ASoC case, however, it's no option.
>> >
>> >Maybe a possible solution in the sound driver side would be to move
>> >from system suspend/resume to ASoC component suspend/resume.  The
>> >runtime suspend/resume can be kept as is, and call
>> >pm_runtime_force_suspend and resume from the component
>suspend/resume
>> >callbacks.  Since the component callbacks are certainly processed
>> >before DAPM events, this should assure the order.
>>
>> I have worked out another patch. This patch decouples the xxx_event()
>> and runtime suspend/resume. This means in whichever case, xxx_event()
>> can make sure display power is in the correct status. What do you think?
>> On the same time, I will try the component suspend/resume. My
>> understanding is that snd_hdac_display_power() should be called either
>> in runtime_suspend/ resume or in component suspend/resume.
>
>This might work around the particular case, yes.  However it still makes me
>uneasy as the root cause isn't full covered -- the other part of runtime resume
>might be still pending while executing the DAPM event.
>
>Now, considering the idea with device_link_add() again: I guess it's
>snd_soc_resume() who kicks off the DAPM even work?  If so, and if
>snd_soc_resume() gets called from the machine driver, we can assure the PM
>order via device_link_add() so that the codec driver is resumed before the
>machine driver.  Then at the time the deferred resume work is executed, the
>codec is ready, so the display power is on.

Yes, snd_soc_resume() kicks off the DAPM event work. The
device PM sequence is good. The root cause is when hdmi runtime_resume
calls snd_hdac_display_power(), it may sleep. The flow is:

1. HDMI runtime_resume runs
2. HDMI runtime_resume calls snd_hdac_display_power(), but display
driver is also operating display power and the mutex_lock is hold. So
HDMI runtime_resume sleeps.
3. the deferred work is scheduled and xxx_event() is called.

This is wrong. All these happen because of the mutex_lock is hold
by display driver, which causes HDMI runtime_resume sleep.

Regards,
Libin

>
>
>thanks,
>
>Takashi

Takashi Iwai March 26, 2019, 11:10 a.m. UTC | #17

On Tue, 26 Mar 2019 12:02:13 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> >> >
> >> >On Tue, 26 Mar 2019 06:42:15 +0100,
> >> >Yang, Libin wrote:
> >> >(snip)
> >> >> Hi Takashi,
> >> >> Below is my draft patch, which doesn't work with
> >> >> pm_runtime_get_sync(). Is there anything wrong in my code?
> >> >(snip)
> >> >> And the dmesg is:
> >> >> [   36.087224] HDMI HDA Codec ehdaudio0D2: in hdmi_codec_resume
> >1890
> >> >ylb
> >> >> [   36.087230] HDMI HDA Codec ehdaudio0D2: in
> >> >hdac_hdmi_runtime_resume 2122 ylb 1
> >> >> [   36.087335] HDMI HDA Codec ehdaudio0D2: in
> >> >hdac_hdmi_cvt_output_widget_event 812 ylb
> >> >> [   40.097586] HDMI HDA Codec ehdaudio0D2: in
> >> >hdac_hdmi_runtime_resume 2135 ylb 2
> >> >> [   40.097766] HDMI HDA Codec ehdaudio0D2: in
> >> >hdac_hdmi_pin_output_widget_event 767 ylb
> >> >> [   45.108632] HDMI HDA Codec ehdaudio0D2: in
> >> >hdac_hdmi_pin_mux_widget_event 857 ylb
> >> >>
> >> >> >From the dmesg, hdac_hdmi_cvt_output_widget_event()
> >> >> is called between "ylb 1" and "ylb 2" in runtime_resume().
> >> >> This means xxx_event() registers setting runs before display power
> >> >> is turned on.
> >> >
> >> >OK, now I understood what went wrong.  It's a similar issue as we've
> >> >hit for the legacy HD-audio and figured out recently.
> >> >
> >> >If my understanding is correct, the problem is the call of
> >> >pm_runtime_force_resume() in PM resume callback combined with an
> >> >async work.  pm_runtime_force_resume() sets the runtime state
> >> >immediately to RPM_ACTIVE even before finishing the task.  The code
> >> >seems assuming blindly that there is no other conflicting task while
> >> >S3/S4 resume, but this is a problem.  That's why the next
> >> >pm_runtime_get_sync() wasn't blocked but just went through; since the
> >> >RPM flag was already set to RPM_ACTIVE in pm_runtime_force_resume(),
> >> >it thought as if it were already active, while the real runtime resume code
> >was still processing the callback.
> >>
> >> Yes, exactly right. And I have checked dev->power.runtime_status, it's
> >> RPM_ACTIVE when xxx_event() calls pm_runtime_get_sync().
> >>
> >> >
> >> >In the case of legacy HD-audio, we "fixed" the problem by avoiding
> >> >the trigger of async work at resume, but let it be triggered from
> >> >runtime resume.  In ASoC case, however, it's no option.
> >> >
> >> >Maybe a possible solution in the sound driver side would be to move
> >> >from system suspend/resume to ASoC component suspend/resume.  The
> >> >runtime suspend/resume can be kept as is, and call
> >> >pm_runtime_force_suspend and resume from the component
> >suspend/resume
> >> >callbacks.  Since the component callbacks are certainly processed
> >> >before DAPM events, this should assure the order.
> >>
> >> I have worked out another patch. This patch decouples the xxx_event()
> >> and runtime suspend/resume. This means in whichever case, xxx_event()
> >> can make sure display power is in the correct status. What do you think?
> >> On the same time, I will try the component suspend/resume. My
> >> understanding is that snd_hdac_display_power() should be called either
> >> in runtime_suspend/ resume or in component suspend/resume.
> >
> >This might work around the particular case, yes.  However it still makes me
> >uneasy as the root cause isn't full covered -- the other part of runtime resume
> >might be still pending while executing the DAPM event.
> >
> >Now, considering the idea with device_link_add() again: I guess it's
> >snd_soc_resume() who kicks off the DAPM even work?  If so, and if
> >snd_soc_resume() gets called from the machine driver, we can assure the PM
> >order via device_link_add() so that the codec driver is resumed before the
> >machine driver.  Then at the time the deferred resume work is executed, the
> >codec is ready, so the display power is on.
> 
> Yes, snd_soc_resume() kicks off the DAPM event work. The
> device PM sequence is good. The root cause is when hdmi runtime_resume
> calls snd_hdac_display_power(), it may sleep. The flow is:
> 
> 1. HDMI runtime_resume runs
> 2. HDMI runtime_resume calls snd_hdac_display_power(), but display
> driver is also operating display power and the mutex_lock is hold. So
> HDMI runtime_resume sleeps.
> 3. the deferred work is scheduled and xxx_event() is called.
> 
> This is wrong. All these happen because of the mutex_lock is hold
> by display driver, which causes HDMI runtime_resume sleep.

Well, the mutex lock itself is OK, and it's designed to behave so.

As mentioned, the root cause is pm_runtime_force_resume() that sets
RPM_ACTIVE while a concurrent task is running.

OTOH, this can be seen indeed as a PM dependency between devices, and
if we set the order explicitly, the problem can be avoided, too.  Then
the runtime resume of codec finishes before snd_soc_resume() is called
from the machine driver resume.

Also, moving to component PM would solve the problem, too, since it
also assures finishing the codec resume before the schedule_work()
call in snd_soc_resume().


thanks,

Takashi

Yang, Libin March 26, 2019, 11:15 a.m. UTC | #18

Hi Takashi,

>-----Original Message-----
>From: Takashi Iwai [mailto:[e-mail]]
>Sent: Tuesday, March 26, 2019 4:54 PM
>To: Yang, Libin <[e-mail]>
>Cc: [e-mail]; [e-mail]
>Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
>
>On Tue, 26 Mar 2019 09:46:31 +0100,
>Yang, Libin wrote:
>>
>> Hi Takashi
>>
>> >
>> >In the case of legacy HD-audio, we "fixed" the problem by avoiding
>> >the trigger of async work at resume, but let it be triggered from
>> >runtime resume.  In ASoC case, however, it's no option.
>> >
>> >Maybe a possible solution in the sound driver side would be to move
>> >from system suspend/resume to ASoC component suspend/resume.  The
>> >runtime suspend/resume can be kept as is, and call
>> >pm_runtime_force_suspend and resume from the component
>suspend/resume
>> >callbacks.  Since the component callbacks are certainly processed
>> >before DAPM events, this should assure the order.
>>
>> I tried to move display power setting from runtime suspend/resume to
>> component suspend/resume. I found there may be another issue:
>> playback will NOT call component suspend/resume.
>> This  means we will never have chance to set the display power if we
>> don't do the S3.
>
>The runtime suspend/resume are fine and need to be kept, I suppose.
>The only problem is the system suspend/resume callbacks that call
>pm_runtime_force_*().  Just moving these two to component would work?
>

Let's see the flow of the suspend:
1. Component suspend is called. It will call snd_hdac_display_power()
to turn off the display power.
2. runtime suspend is called. It will operate on the registers. This
is wrong. We can't operate on registers when the power is off.

So I think we can only call snd_hdac_display_power() either in
component suspend/resume or runtime suspend/resume.
We can't call snd_hdac_display_power() in both.

And based on the previous test, component suspend/resume
will not be called when playback. So we can only call
snd_hdac_display_power() in runtime suspend/resume?

Regards,
Libin

>
>Takashi

Takashi Iwai March 26, 2019, 11:19 a.m. UTC | #19

On Tue, 26 Mar 2019 12:15:06 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:[e-mail]]
> >Sent: Tuesday, March 26, 2019 4:54 PM
> >To: Yang, Libin <[e-mail]>
> >Cc: [e-mail]; [e-mail]
> >Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
> >
> >On Tue, 26 Mar 2019 09:46:31 +0100,
> >Yang, Libin wrote:
> >>
> >> Hi Takashi
> >>
> >> >
> >> >In the case of legacy HD-audio, we "fixed" the problem by avoiding
> >> >the trigger of async work at resume, but let it be triggered from
> >> >runtime resume.  In ASoC case, however, it's no option.
> >> >
> >> >Maybe a possible solution in the sound driver side would be to move
> >> >from system suspend/resume to ASoC component suspend/resume.  The
> >> >runtime suspend/resume can be kept as is, and call
> >> >pm_runtime_force_suspend and resume from the component
> >suspend/resume
> >> >callbacks.  Since the component callbacks are certainly processed
> >> >before DAPM events, this should assure the order.
> >>
> >> I tried to move display power setting from runtime suspend/resume to
> >> component suspend/resume. I found there may be another issue:
> >> playback will NOT call component suspend/resume.
> >> This  means we will never have chance to set the display power if we
> >> don't do the S3.
> >
> >The runtime suspend/resume are fine and need to be kept, I suppose.
> >The only problem is the system suspend/resume callbacks that call
> >pm_runtime_force_*().  Just moving these two to component would work?
> >
> 
> Let's see the flow of the suspend:
> 1. Component suspend is called. It will call snd_hdac_display_power()
> to turn off the display power.

No, what I meant was to make the component suspend calling
pm_runtime_force_suspend() instead of the system suspend.
Ditto for the component resume calling pm_runtime_force_resume().


Takashi

Yang, Libin March 26, 2019, 11:19 a.m. UTC | #20

Hi Takashi,

>> Hi Takashi,
>>
>> >> >
>> >> >On Tue, 26 Mar 2019 06:42:15 +0100, Yang, Libin wrote:
>> >> >(snip)
>> >> >> Hi Takashi,
>> >> >> Below is my draft patch, which doesn't work with
>> >> >> pm_runtime_get_sync(). Is there anything wrong in my code?
>> >> >(snip)
>> >> >> And the dmesg is:
>> >> >> [   36.087224] HDMI HDA Codec ehdaudio0D2: in hdmi_codec_resume
>> >1890
>> >> >ylb
>> >> >> [   36.087230] HDMI HDA Codec ehdaudio0D2: in
>> >> >hdac_hdmi_runtime_resume 2122 ylb 1
>> >> >> [   36.087335] HDMI HDA Codec ehdaudio0D2: in
>> >> >hdac_hdmi_cvt_output_widget_event 812 ylb
>> >> >> [   40.097586] HDMI HDA Codec ehdaudio0D2: in
>> >> >hdac_hdmi_runtime_resume 2135 ylb 2
>> >> >> [   40.097766] HDMI HDA Codec ehdaudio0D2: in
>> >> >hdac_hdmi_pin_output_widget_event 767 ylb
>> >> >> [   45.108632] HDMI HDA Codec ehdaudio0D2: in
>> >> >hdac_hdmi_pin_mux_widget_event 857 ylb
>> >> >>
>> >> >> >From the dmesg, hdac_hdmi_cvt_output_widget_event()
>> >> >> is called between "ylb 1" and "ylb 2" in runtime_resume().
>> >> >> This means xxx_event() registers setting runs before display
>> >> >> power is turned on.
>> >> >
>> >> >OK, now I understood what went wrong.  It's a similar issue as
>> >> >we've hit for the legacy HD-audio and figured out recently.
>> >> >
>> >> >If my understanding is correct, the problem is the call of
>> >> >pm_runtime_force_resume() in PM resume callback combined with an
>> >> >async work.  pm_runtime_force_resume() sets the runtime state
>> >> >immediately to RPM_ACTIVE even before finishing the task.  The
>> >> >code seems assuming blindly that there is no other conflicting
>> >> >task while
>> >> >S3/S4 resume, but this is a problem.  That's why the next
>> >> >pm_runtime_get_sync() wasn't blocked but just went through; since
>> >> >the RPM flag was already set to RPM_ACTIVE in
>> >> >pm_runtime_force_resume(), it thought as if it were already
>> >> >active, while the real runtime resume code
>> >was still processing the callback.
>> >>
>> >> Yes, exactly right. And I have checked dev->power.runtime_status,
>> >> it's RPM_ACTIVE when xxx_event() calls pm_runtime_get_sync().
>> >>
>> >> >
>> >> >In the case of legacy HD-audio, we "fixed" the problem by avoiding
>> >> >the trigger of async work at resume, but let it be triggered from
>> >> >runtime resume.  In ASoC case, however, it's no option.
>> >> >
>> >> >Maybe a possible solution in the sound driver side would be to
>> >> >move from system suspend/resume to ASoC component
>suspend/resume.
>> >> >The runtime suspend/resume can be kept as is, and call
>> >> >pm_runtime_force_suspend and resume from the component
>> >suspend/resume
>> >> >callbacks.  Since the component callbacks are certainly processed
>> >> >before DAPM events, this should assure the order.
>> >>
>> >> I have worked out another patch. This patch decouples the
>> >> xxx_event() and runtime suspend/resume. This means in whichever
>> >> case, xxx_event() can make sure display power is in the correct status.
>What do you think?
>> >> On the same time, I will try the component suspend/resume. My
>> >> understanding is that snd_hdac_display_power() should be called
>> >> either in runtime_suspend/ resume or in component suspend/resume.
>> >
>> >This might work around the particular case, yes.  However it still
>> >makes me uneasy as the root cause isn't full covered -- the other
>> >part of runtime resume might be still pending while executing the DAPM
>event.
>> >
>> >Now, considering the idea with device_link_add() again: I guess it's
>> >snd_soc_resume() who kicks off the DAPM even work?  If so, and if
>> >snd_soc_resume() gets called from the machine driver, we can assure
>> >the PM order via device_link_add() so that the codec driver is
>> >resumed before the machine driver.  Then at the time the deferred
>> >resume work is executed, the codec is ready, so the display power is on.
>>
>> Yes, snd_soc_resume() kicks off the DAPM event work. The device PM
>> sequence is good. The root cause is when hdmi runtime_resume calls
>> snd_hdac_display_power(), it may sleep. The flow is:
>>
>> 1. HDMI runtime_resume runs
>> 2. HDMI runtime_resume calls snd_hdac_display_power(), but display
>> driver is also operating display power and the mutex_lock is hold. So
>> HDMI runtime_resume sleeps.
>> 3. the deferred work is scheduled and xxx_event() is called.
>>
>> This is wrong. All these happen because of the mutex_lock is hold by
>> display driver, which causes HDMI runtime_resume sleep.
>
>Well, the mutex lock itself is OK, and it's designed to behave so.
>
>As mentioned, the root cause is pm_runtime_force_resume() that sets
>RPM_ACTIVE while a concurrent task is running.
>
>OTOH, this can be seen indeed as a PM dependency between devices, and if
>we set the order explicitly, the problem can be avoided, too.  Then the
>runtime resume of codec finishes before snd_soc_resume() is called from the
>machine driver resume.

Do you have any idea to set the order explicitly? What I can think of is that
we set HDMI device to be the machine device parent?

Regards,
Libin

>
>Also, moving to component PM would solve the problem, too, since it also
>assures finishing the codec resume before the schedule_work() call in
>snd_soc_resume().
>
>
>thanks,
>
>Takashi

Yang, Libin March 26, 2019, 11:21 a.m. UTC | #21

Hi Takashi,

>-----Original Message-----
>From: Takashi Iwai [mailto:[e-mail]]
>Sent: Tuesday, March 26, 2019 7:19 PM
>To: Yang, Libin <[e-mail]>
>Cc: [e-mail]; [e-mail]
>Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
>
>On Tue, 26 Mar 2019 12:15:06 +0100,
>Yang, Libin wrote:
>>
>> Hi Takashi,
>>
>> >-----Original Message-----
>> >From: Takashi Iwai [mailto:[e-mail]]
>> >Sent: Tuesday, March 26, 2019 4:54 PM
>> >To: Yang, Libin <[e-mail]>
>> >Cc: [e-mail]; [e-mail]
>> >Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver
>> >contest in S3
>> >
>> >On Tue, 26 Mar 2019 09:46:31 +0100,
>> >Yang, Libin wrote:
>> >>
>> >> Hi Takashi
>> >>
>> >> >
>> >> >In the case of legacy HD-audio, we "fixed" the problem by avoiding
>> >> >the trigger of async work at resume, but let it be triggered from
>> >> >runtime resume.  In ASoC case, however, it's no option.
>> >> >
>> >> >Maybe a possible solution in the sound driver side would be to
>> >> >move from system suspend/resume to ASoC component
>suspend/resume.
>> >> >The runtime suspend/resume can be kept as is, and call
>> >> >pm_runtime_force_suspend and resume from the component
>> >suspend/resume
>> >> >callbacks.  Since the component callbacks are certainly processed
>> >> >before DAPM events, this should assure the order.
>> >>
>> >> I tried to move display power setting from runtime suspend/resume
>> >> to component suspend/resume. I found there may be another issue:
>> >> playback will NOT call component suspend/resume.
>> >> This  means we will never have chance to set the display power if
>> >> we don't do the S3.
>> >
>> >The runtime suspend/resume are fine and need to be kept, I suppose.
>> >The only problem is the system suspend/resume callbacks that call
>> >pm_runtime_force_*().  Just moving these two to component would work?
>> >
>>
>> Let's see the flow of the suspend:
>> 1. Component suspend is called. It will call snd_hdac_display_power()
>> to turn off the display power.
>
>No, what I meant was to make the component suspend calling
>pm_runtime_force_suspend() instead of the system suspend.
>Ditto for the component resume calling pm_runtime_force_resume().

Aha, I get your idea now. I will have a try tomorrow as I don't have
the device on hand now.

Regards,
Libin

>
>
>Takashi

Takashi Iwai March 26, 2019, 11:22 a.m. UTC | #22

On Tue, 26 Mar 2019 12:19:15 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> >> Hi Takashi,
> >>
> >> >> >
> >> >> >On Tue, 26 Mar 2019 06:42:15 +0100, Yang, Libin wrote:
> >> >> >(snip)
> >> >> >> Hi Takashi,
> >> >> >> Below is my draft patch, which doesn't work with
> >> >> >> pm_runtime_get_sync(). Is there anything wrong in my code?
> >> >> >(snip)
> >> >> >> And the dmesg is:
> >> >> >> [   36.087224] HDMI HDA Codec ehdaudio0D2: in hdmi_codec_resume
> >> >1890
> >> >> >ylb
> >> >> >> [   36.087230] HDMI HDA Codec ehdaudio0D2: in
> >> >> >hdac_hdmi_runtime_resume 2122 ylb 1
> >> >> >> [   36.087335] HDMI HDA Codec ehdaudio0D2: in
> >> >> >hdac_hdmi_cvt_output_widget_event 812 ylb
> >> >> >> [   40.097586] HDMI HDA Codec ehdaudio0D2: in
> >> >> >hdac_hdmi_runtime_resume 2135 ylb 2
> >> >> >> [   40.097766] HDMI HDA Codec ehdaudio0D2: in
> >> >> >hdac_hdmi_pin_output_widget_event 767 ylb
> >> >> >> [   45.108632] HDMI HDA Codec ehdaudio0D2: in
> >> >> >hdac_hdmi_pin_mux_widget_event 857 ylb
> >> >> >>
> >> >> >> >From the dmesg, hdac_hdmi_cvt_output_widget_event()
> >> >> >> is called between "ylb 1" and "ylb 2" in runtime_resume().
> >> >> >> This means xxx_event() registers setting runs before display
> >> >> >> power is turned on.
> >> >> >
> >> >> >OK, now I understood what went wrong.  It's a similar issue as
> >> >> >we've hit for the legacy HD-audio and figured out recently.
> >> >> >
> >> >> >If my understanding is correct, the problem is the call of
> >> >> >pm_runtime_force_resume() in PM resume callback combined with an
> >> >> >async work.  pm_runtime_force_resume() sets the runtime state
> >> >> >immediately to RPM_ACTIVE even before finishing the task.  The
> >> >> >code seems assuming blindly that there is no other conflicting
> >> >> >task while
> >> >> >S3/S4 resume, but this is a problem.  That's why the next
> >> >> >pm_runtime_get_sync() wasn't blocked but just went through; since
> >> >> >the RPM flag was already set to RPM_ACTIVE in
> >> >> >pm_runtime_force_resume(), it thought as if it were already
> >> >> >active, while the real runtime resume code
> >> >was still processing the callback.
> >> >>
> >> >> Yes, exactly right. And I have checked dev->power.runtime_status,
> >> >> it's RPM_ACTIVE when xxx_event() calls pm_runtime_get_sync().
> >> >>
> >> >> >
> >> >> >In the case of legacy HD-audio, we "fixed" the problem by avoiding
> >> >> >the trigger of async work at resume, but let it be triggered from
> >> >> >runtime resume.  In ASoC case, however, it's no option.
> >> >> >
> >> >> >Maybe a possible solution in the sound driver side would be to
> >> >> >move from system suspend/resume to ASoC component
> >suspend/resume.
> >> >> >The runtime suspend/resume can be kept as is, and call
> >> >> >pm_runtime_force_suspend and resume from the component
> >> >suspend/resume
> >> >> >callbacks.  Since the component callbacks are certainly processed
> >> >> >before DAPM events, this should assure the order.
> >> >>
> >> >> I have worked out another patch. This patch decouples the
> >> >> xxx_event() and runtime suspend/resume. This means in whichever
> >> >> case, xxx_event() can make sure display power is in the correct status.
> >What do you think?
> >> >> On the same time, I will try the component suspend/resume. My
> >> >> understanding is that snd_hdac_display_power() should be called
> >> >> either in runtime_suspend/ resume or in component suspend/resume.
> >> >
> >> >This might work around the particular case, yes.  However it still
> >> >makes me uneasy as the root cause isn't full covered -- the other
> >> >part of runtime resume might be still pending while executing the DAPM
> >event.
> >> >
> >> >Now, considering the idea with device_link_add() again: I guess it's
> >> >snd_soc_resume() who kicks off the DAPM even work?  If so, and if
> >> >snd_soc_resume() gets called from the machine driver, we can assure
> >> >the PM order via device_link_add() so that the codec driver is
> >> >resumed before the machine driver.  Then at the time the deferred
> >> >resume work is executed, the codec is ready, so the display power is on.
> >>
> >> Yes, snd_soc_resume() kicks off the DAPM event work. The device PM
> >> sequence is good. The root cause is when hdmi runtime_resume calls
> >> snd_hdac_display_power(), it may sleep. The flow is:
> >>
> >> 1. HDMI runtime_resume runs
> >> 2. HDMI runtime_resume calls snd_hdac_display_power(), but display
> >> driver is also operating display power and the mutex_lock is hold. So
> >> HDMI runtime_resume sleeps.
> >> 3. the deferred work is scheduled and xxx_event() is called.
> >>
> >> This is wrong. All these happen because of the mutex_lock is hold by
> >> display driver, which causes HDMI runtime_resume sleep.
> >
> >Well, the mutex lock itself is OK, and it's designed to behave so.
> >
> >As mentioned, the root cause is pm_runtime_force_resume() that sets
> >RPM_ACTIVE while a concurrent task is running.
> >
> >OTOH, this can be seen indeed as a PM dependency between devices, and if
> >we set the order explicitly, the problem can be avoided, too.  Then the
> >runtime resume of codec finishes before snd_soc_resume() is called from the
> >machine driver resume.
> 
> Do you have any idea to set the order explicitly? What I can think of is that
> we set HDMI device to be the machine device parent?

device_link_add() should serve for defining the explicit PM
dependency.


Takashi

Yang, Libin March 26, 2019, 11:28 a.m. UTC | #23

Hi Takashi,

>-----Original Message-----
>From: Takashi Iwai [mailto:[e-mail]]
>Sent: Tuesday, March 26, 2019 7:23 PM
>To: Yang, Libin <[e-mail]>
>Cc: [e-mail]; [e-mail]
>Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
>
>On Tue, 26 Mar 2019 12:19:15 +0100,
>Yang, Libin wrote:
>>
>> Hi Takashi,
>>
>> >> Hi Takashi,
>> >>
>> >> >> >
>> >> >> >On Tue, 26 Mar 2019 06:42:15 +0100, Yang, Libin wrote:
>> >> >> >(snip)
>> >> >> >> Hi Takashi,
>> >> >> >> Below is my draft patch, which doesn't work with
>> >> >> >> pm_runtime_get_sync(). Is there anything wrong in my code?
>> >> >> >(snip)
>> >> >> >> And the dmesg is:
>> >> >> >> [   36.087224] HDMI HDA Codec ehdaudio0D2: in
>hdmi_codec_resume
>> >> >1890
>> >> >> >ylb
>> >> >> >> [   36.087230] HDMI HDA Codec ehdaudio0D2: in
>> >> >> >hdac_hdmi_runtime_resume 2122 ylb 1
>> >> >> >> [   36.087335] HDMI HDA Codec ehdaudio0D2: in
>> >> >> >hdac_hdmi_cvt_output_widget_event 812 ylb
>> >> >> >> [   40.097586] HDMI HDA Codec ehdaudio0D2: in
>> >> >> >hdac_hdmi_runtime_resume 2135 ylb 2
>> >> >> >> [   40.097766] HDMI HDA Codec ehdaudio0D2: in
>> >> >> >hdac_hdmi_pin_output_widget_event 767 ylb
>> >> >> >> [   45.108632] HDMI HDA Codec ehdaudio0D2: in
>> >> >> >hdac_hdmi_pin_mux_widget_event 857 ylb
>> >> >> >>
>> >> >> >> >From the dmesg, hdac_hdmi_cvt_output_widget_event()
>> >> >> >> is called between "ylb 1" and "ylb 2" in runtime_resume().
>> >> >> >> This means xxx_event() registers setting runs before display
>> >> >> >> power is turned on.
>> >> >> >
>> >> >> >OK, now I understood what went wrong.  It's a similar issue as
>> >> >> >we've hit for the legacy HD-audio and figured out recently.
>> >> >> >
>> >> >> >If my understanding is correct, the problem is the call of
>> >> >> >pm_runtime_force_resume() in PM resume callback combined with
>> >> >> >an async work.  pm_runtime_force_resume() sets the runtime
>> >> >> >state immediately to RPM_ACTIVE even before finishing the task.
>> >> >> >The code seems assuming blindly that there is no other
>> >> >> >conflicting task while
>> >> >> >S3/S4 resume, but this is a problem.  That's why the next
>> >> >> >pm_runtime_get_sync() wasn't blocked but just went through;
>> >> >> >since the RPM flag was already set to RPM_ACTIVE in
>> >> >> >pm_runtime_force_resume(), it thought as if it were already
>> >> >> >active, while the real runtime resume code
>> >> >was still processing the callback.
>> >> >>
>> >> >> Yes, exactly right. And I have checked
>> >> >> dev->power.runtime_status, it's RPM_ACTIVE when xxx_event() calls
>pm_runtime_get_sync().
>> >> >>
>> >> >> >
>> >> >> >In the case of legacy HD-audio, we "fixed" the problem by
>> >> >> >avoiding the trigger of async work at resume, but let it be
>> >> >> >triggered from runtime resume.  In ASoC case, however, it's no
>option.
>> >> >> >
>> >> >> >Maybe a possible solution in the sound driver side would be to
>> >> >> >move from system suspend/resume to ASoC component
>> >suspend/resume.
>> >> >> >The runtime suspend/resume can be kept as is, and call
>> >> >> >pm_runtime_force_suspend and resume from the component
>> >> >suspend/resume
>> >> >> >callbacks.  Since the component callbacks are certainly
>> >> >> >processed before DAPM events, this should assure the order.
>> >> >>
>> >> >> I have worked out another patch. This patch decouples the
>> >> >> xxx_event() and runtime suspend/resume. This means in whichever
>> >> >> case, xxx_event() can make sure display power is in the correct status.
>> >What do you think?
>> >> >> On the same time, I will try the component suspend/resume. My
>> >> >> understanding is that snd_hdac_display_power() should be called
>> >> >> either in runtime_suspend/ resume or in component suspend/resume.
>> >> >
>> >> >This might work around the particular case, yes.  However it still
>> >> >makes me uneasy as the root cause isn't full covered -- the other
>> >> >part of runtime resume might be still pending while executing the
>> >> >DAPM
>> >event.
>> >> >
>> >> >Now, considering the idea with device_link_add() again: I guess
>> >> >it's
>> >> >snd_soc_resume() who kicks off the DAPM even work?  If so, and if
>> >> >snd_soc_resume() gets called from the machine driver, we can
>> >> >assure the PM order via device_link_add() so that the codec driver
>> >> >is resumed before the machine driver.  Then at the time the
>> >> >deferred resume work is executed, the codec is ready, so the display
>power is on.
>> >>
>> >> Yes, snd_soc_resume() kicks off the DAPM event work. The device PM
>> >> sequence is good. The root cause is when hdmi runtime_resume calls
>> >> snd_hdac_display_power(), it may sleep. The flow is:
>> >>
>> >> 1. HDMI runtime_resume runs
>> >> 2. HDMI runtime_resume calls snd_hdac_display_power(), but display
>> >> driver is also operating display power and the mutex_lock is hold.
>> >> So HDMI runtime_resume sleeps.
>> >> 3. the deferred work is scheduled and xxx_event() is called.
>> >>
>> >> This is wrong. All these happen because of the mutex_lock is hold
>> >> by display driver, which causes HDMI runtime_resume sleep.
>> >
>> >Well, the mutex lock itself is OK, and it's designed to behave so.
>> >
>> >As mentioned, the root cause is pm_runtime_force_resume() that sets
>> >RPM_ACTIVE while a concurrent task is running.
>> >
>> >OTOH, this can be seen indeed as a PM dependency between devices, and
>> >if we set the order explicitly, the problem can be avoided, too.
>> >Then the runtime resume of codec finishes before snd_soc_resume() is
>> >called from the machine driver resume.
>>
>> Do you have any idea to set the order explicitly? What I can think of
>> is that we set HDMI device to be the machine device parent?
>
>device_link_add() should serve for defining the explicit PM dependency.

Get it. I will check this function. Thanks.

Regards,
Libin

>
>
>Takashi

Yang, Libin March 27, 2019, 6:14 a.m. UTC | #24

Hi Takashi,

>> >>
>> >> >
>> >> >In the case of legacy HD-audio, we "fixed" the problem by avoiding
>> >> >the trigger of async work at resume, but let it be triggered from
>> >> >runtime resume.  In ASoC case, however, it's no option.
>> >> >
>> >> >Maybe a possible solution in the sound driver side would be to
>> >> >move from system suspend/resume to ASoC component
>suspend/resume.
>> >> >The runtime suspend/resume can be kept as is, and call
>> >> >pm_runtime_force_suspend and resume from the component
>> >suspend/resume
>> >> >callbacks.  Since the component callbacks are certainly processed
>> >> >before DAPM events, this should assure the order.
>> >>
>> >> I tried to move display power setting from runtime suspend/resume
>> >> to component suspend/resume. I found there may be another issue:
>> >> playback will NOT call component suspend/resume.
>> >> This  means we will never have chance to set the display power if
>> >> we don't do the S3.
>> >
>> >The runtime suspend/resume are fine and need to be kept, I suppose.
>> >The only problem is the system suspend/resume callbacks that call
>> >pm_runtime_force_*().  Just moving these two to component would work?
>> >
>>
>> Let's see the flow of the suspend:
>> 1. Component suspend is called. It will call snd_hdac_display_power()
>> to turn off the display power.
>
>No, what I meant was to make the component suspend calling
>pm_runtime_force_suspend() instead of the system suspend.
>Ditto for the component resume calling pm_runtime_force_resume().

It seems this component suspend/resume method works well. 
I will do more test and send out the patch later.

Regards,
Libin

>
>
>Takashi

Yang, Libin March 28, 2019, 7:10 a.m. UTC | #25

Hi Takashi,

>>> >> >In the case of legacy HD-audio, we "fixed" the problem by
>>> >> >avoiding the trigger of async work at resume, but let it be
>>> >> >triggered from runtime resume.  In ASoC case, however, it's no option.
>>> >> >
>>> >> >Maybe a possible solution in the sound driver side would be to
>>> >> >move from system suspend/resume to ASoC component
>>suspend/resume.
>>> >> >The runtime suspend/resume can be kept as is, and call
>>> >> >pm_runtime_force_suspend and resume from the component
>>> >suspend/resume
>>> >> >callbacks.  Since the component callbacks are certainly processed
>>> >> >before DAPM events, this should assure the order.
>>> >>
>>> >> I tried to move display power setting from runtime suspend/resume
>>> >> to component suspend/resume. I found there may be another issue:
>>> >> playback will NOT call component suspend/resume.
>>> >> This  means we will never have chance to set the display power if
>>> >> we don't do the S3.
>>> >
>>> >The runtime suspend/resume are fine and need to be kept, I suppose.
>>> >The only problem is the system suspend/resume callbacks that call
>>> >pm_runtime_force_*().  Just moving these two to component would work?
>>> >
>>>
>>> Let's see the flow of the suspend:
>>> 1. Component suspend is called. It will call snd_hdac_display_power()
>>> to turn off the display power.
>>
>>No, what I meant was to make the component suspend calling
>>pm_runtime_force_suspend() instead of the system suspend.
>>Ditto for the component resume calling pm_runtime_force_resume().
>
>It seems this component suspend/resume method works well.
>I will do more test and send out the patch later.

I did some stress test. And I found if I remove the debug message,
the issue can still be reproduced with component suspend/resume
added. If I add the debug message, it will be much better.
I'm not sure whether my patch is wrong or not. I've set the bus_control = 1
in the dai driver.

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 5eeb0fe..505efd9 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -1900,9 +1900,31 @@ static int hdmi_codec_resume(struct device *dev)
 #define hdmi_codec_resume NULL
 #endif

+static int hdmi_component_suspend(struct snd_soc_component *component)
+{
+       struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component);
+       struct hdac_device *hdev = hdmi->hdev;
+
+       pm_runtime_force_suspend(&hdev->dev);
+
+       return 0;
+}
+
+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;
+
+       pm_runtime_force_resume(&hdev->dev);
+
+       return 0;
+}
+
 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,

>

Regards,
Libin

Takashi Iwai March 28, 2019, 7:15 a.m. UTC | #26

On Thu, 28 Mar 2019 08:10:34 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> >>> >> >In the case of legacy HD-audio, we "fixed" the problem by
> >>> >> >avoiding the trigger of async work at resume, but let it be
> >>> >> >triggered from runtime resume.  In ASoC case, however, it's no option.
> >>> >> >
> >>> >> >Maybe a possible solution in the sound driver side would be to
> >>> >> >move from system suspend/resume to ASoC component
> >>suspend/resume.
> >>> >> >The runtime suspend/resume can be kept as is, and call
> >>> >> >pm_runtime_force_suspend and resume from the component
> >>> >suspend/resume
> >>> >> >callbacks.  Since the component callbacks are certainly processed
> >>> >> >before DAPM events, this should assure the order.
> >>> >>
> >>> >> I tried to move display power setting from runtime suspend/resume
> >>> >> to component suspend/resume. I found there may be another issue:
> >>> >> playback will NOT call component suspend/resume.
> >>> >> This  means we will never have chance to set the display power if
> >>> >> we don't do the S3.
> >>> >
> >>> >The runtime suspend/resume are fine and need to be kept, I suppose.
> >>> >The only problem is the system suspend/resume callbacks that call
> >>> >pm_runtime_force_*().  Just moving these two to component would work?
> >>> >
> >>>
> >>> Let's see the flow of the suspend:
> >>> 1. Component suspend is called. It will call snd_hdac_display_power()
> >>> to turn off the display power.
> >>
> >>No, what I meant was to make the component suspend calling
> >>pm_runtime_force_suspend() instead of the system suspend.
> >>Ditto for the component resume calling pm_runtime_force_resume().
> >
> >It seems this component suspend/resume method works well.
> >I will do more test and send out the patch later.
> 
> I did some stress test. And I found if I remove the debug message,
> the issue can still be reproduced with component suspend/resume
> added. If I add the debug message, it will be much better.
> I'm not sure whether my patch is wrong or not. I've set the bus_control = 1
> in the dai driver.

I guess you forgot to remove the suspend/resume call from the codec
device PM?  Otherwise the codec device resume is still executed
concurrently.


Takashi

> 
> diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> index 5eeb0fe..505efd9 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -1900,9 +1900,31 @@ static int hdmi_codec_resume(struct device *dev)
>  #define hdmi_codec_resume NULL
>  #endif
> 
> +static int hdmi_component_suspend(struct snd_soc_component *component)
> +{
> +       struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component);
> +       struct hdac_device *hdev = hdmi->hdev;
> +
> +       pm_runtime_force_suspend(&hdev->dev);
> +
> +       return 0;
> +}
> +
> +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;
> +
> +       pm_runtime_force_resume(&hdev->dev);
> +
> +       return 0;
> +}
> +
>  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,
> 
> >
> 
> Regards,
> Libin
>

Yang, Libin March 28, 2019, 7:28 a.m. UTC | #27

>-----Original Message-----
>From: Takashi Iwai [mailto:[e-mail]]
>Sent: Thursday, March 28, 2019 3:15 PM
>To: Yang, Libin <[e-mail]>
>Cc: '[e-mail]' <[e-mail]>;
>'[e-mail]' <[e-mail]>
>Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
>
>On Thu, 28 Mar 2019 08:10:34 +0100,
>Yang, Libin wrote:
>>
>> Hi Takashi,
>>
>> >>> >> >In the case of legacy HD-audio, we "fixed" the problem by
>> >>> >> >avoiding the trigger of async work at resume, but let it be
>> >>> >> >triggered from runtime resume.  In ASoC case, however, it's no
>option.
>> >>> >> >
>> >>> >> >Maybe a possible solution in the sound driver side would be to
>> >>> >> >move from system suspend/resume to ASoC component
>> >>suspend/resume.
>> >>> >> >The runtime suspend/resume can be kept as is, and call
>> >>> >> >pm_runtime_force_suspend and resume from the component
>> >>> >suspend/resume
>> >>> >> >callbacks.  Since the component callbacks are certainly
>> >>> >> >processed before DAPM events, this should assure the order.
>> >>> >>
>> >>> >> I tried to move display power setting from runtime
>> >>> >> suspend/resume to component suspend/resume. I found there may
>be another issue:
>> >>> >> playback will NOT call component suspend/resume.
>> >>> >> This  means we will never have chance to set the display power
>> >>> >> if we don't do the S3.
>> >>> >
>> >>> >The runtime suspend/resume are fine and need to be kept, I suppose.
>> >>> >The only problem is the system suspend/resume callbacks that call
>> >>> >pm_runtime_force_*().  Just moving these two to component would
>work?
>> >>> >
>> >>>
>> >>> Let's see the flow of the suspend:
>> >>> 1. Component suspend is called. It will call
>> >>> snd_hdac_display_power() to turn off the display power.
>> >>
>> >>No, what I meant was to make the component suspend calling
>> >>pm_runtime_force_suspend() instead of the system suspend.
>> >>Ditto for the component resume calling pm_runtime_force_resume().
>> >
>> >It seems this component suspend/resume method works well.
>> >I will do more test and send out the patch later.
>>
>> I did some stress test. And I found if I remove the debug message, the
>> issue can still be reproduced with component suspend/resume added. If
>> I add the debug message, it will be much better.
>> I'm not sure whether my patch is wrong or not. I've set the
>> bus_control = 1 in the dai driver.
>
>I guess you forgot to remove the suspend/resume call from the codec device
>PM?  Otherwise the codec device resume is still executed concurrently.

... Yes, I forgot it. Thanks for reminder. By the way, do you think it is better to
use pm_runtime_force_resume() or pm_runtime_get_sync()?

Regards,
Libin

>
>
>Takashi
>
>>
>> diff --git a/sound/soc/codecs/hdac_hdmi.c
>> b/sound/soc/codecs/hdac_hdmi.c index 5eeb0fe..505efd9 100644
>> --- a/sound/soc/codecs/hdac_hdmi.c
>> +++ b/sound/soc/codecs/hdac_hdmi.c
>> @@ -1900,9 +1900,31 @@ static int hdmi_codec_resume(struct device
>> *dev)  #define hdmi_codec_resume NULL  #endif
>>
>> +static int hdmi_component_suspend(struct snd_soc_component
>> +*component) {
>> +       struct hdac_hdmi_priv *hdmi =
>snd_soc_component_get_drvdata(component);
>> +       struct hdac_device *hdev = hdmi->hdev;
>> +
>> +       pm_runtime_force_suspend(&hdev->dev);
>> +
>> +       return 0;
>> +}
>> +
>> +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;
>> +
>> +       pm_runtime_force_resume(&hdev->dev);
>> +
>> +       return 0;
>> +}
>> +
>>  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,
>>
>> >
>>
>> Regards,
>> Libin
>>

Takashi Iwai March 28, 2019, 7:31 a.m. UTC | #28

On Thu, 28 Mar 2019 08:28:06 +0100,
Yang, Libin wrote:
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:[e-mail]]
> >Sent: Thursday, March 28, 2019 3:15 PM
> >To: Yang, Libin <[e-mail]>
> >Cc: '[e-mail]' <[e-mail]>;
> >'[e-mail]' <[e-mail]>
> >Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
> >
> >On Thu, 28 Mar 2019 08:10:34 +0100,
> >Yang, Libin wrote:
> >>
> >> Hi Takashi,
> >>
> >> >>> >> >In the case of legacy HD-audio, we "fixed" the problem by
> >> >>> >> >avoiding the trigger of async work at resume, but let it be
> >> >>> >> >triggered from runtime resume.  In ASoC case, however, it's no
> >option.
> >> >>> >> >
> >> >>> >> >Maybe a possible solution in the sound driver side would be to
> >> >>> >> >move from system suspend/resume to ASoC component
> >> >>suspend/resume.
> >> >>> >> >The runtime suspend/resume can be kept as is, and call
> >> >>> >> >pm_runtime_force_suspend and resume from the component
> >> >>> >suspend/resume
> >> >>> >> >callbacks.  Since the component callbacks are certainly
> >> >>> >> >processed before DAPM events, this should assure the order.
> >> >>> >>
> >> >>> >> I tried to move display power setting from runtime
> >> >>> >> suspend/resume to component suspend/resume. I found there may
> >be another issue:
> >> >>> >> playback will NOT call component suspend/resume.
> >> >>> >> This  means we will never have chance to set the display power
> >> >>> >> if we don't do the S3.
> >> >>> >
> >> >>> >The runtime suspend/resume are fine and need to be kept, I suppose.
> >> >>> >The only problem is the system suspend/resume callbacks that call
> >> >>> >pm_runtime_force_*().  Just moving these two to component would
> >work?
> >> >>> >
> >> >>>
> >> >>> Let's see the flow of the suspend:
> >> >>> 1. Component suspend is called. It will call
> >> >>> snd_hdac_display_power() to turn off the display power.
> >> >>
> >> >>No, what I meant was to make the component suspend calling
> >> >>pm_runtime_force_suspend() instead of the system suspend.
> >> >>Ditto for the component resume calling pm_runtime_force_resume().
> >> >
> >> >It seems this component suspend/resume method works well.
> >> >I will do more test and send out the patch later.
> >>
> >> I did some stress test. And I found if I remove the debug message, the
> >> issue can still be reproduced with component suspend/resume added. If
> >> I add the debug message, it will be much better.
> >> I'm not sure whether my patch is wrong or not. I've set the
> >> bus_control = 1 in the dai driver.
> >
> >I guess you forgot to remove the suspend/resume call from the codec device
> >PM?  Otherwise the codec device resume is still executed concurrently.
> 
> ... Yes, I forgot it. Thanks for reminder. By the way, do you think it is better to
> use pm_runtime_force_resume() or pm_runtime_get_sync()?

It needs pm_runtime_force_resume() if it's suspended by
pm_runtime_force_suspend(), I suppose.


Takashi

Patch

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