[alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
From: Libin Yang libin.yang@intel.com
In S3, there is a contest between dapm event and getting display power.
When resume, the sequence is:
hdac_hdmi_runtime_resume() => snd_hdac_display_power() hdac_hdmi_xxx_widget_event()
The hdac_hdmi_xxx_widget_event() are based on the power being on. Otherwise, the operation on the codec register will fail.
However, in snd_hdac_display_power(), it may sleep because of the mutex_lock() in display driver. In this case, hdac_hdmi_xxx_widget_event() will be called before the power is on.
Let's not operate the registers and wait for the power in hdac_hdmi_xxx_widget_event()
Signed-off-by: Libin Yang libin.yang@intel.com --- 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
On Fri, 22 Mar 2019 07:24:56 +0100, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
In S3, there is a contest between dapm event and getting display power.
When resume, the sequence is:
hdac_hdmi_runtime_resume() => snd_hdac_display_power() hdac_hdmi_xxx_widget_event()
The hdac_hdmi_xxx_widget_event() are based on the power being on. Otherwise, the operation on the codec register will fail.
However, in snd_hdac_display_power(), it may sleep because of the mutex_lock() in display driver. In this case, hdac_hdmi_xxx_widget_event() will be called before the power is on.
Let's not operate the registers and wait for the power in hdac_hdmi_xxx_widget_event()
Signed-off-by: Libin Yang libin.yang@intel.com
IMO the workaround looks a bit too fragile. The substantial problem is the race between codec and DAPM. Can this be controlled better, e.g. by defining the proper PM dependency? I thought we may rearrange the PM topology dynamically.
thanks,
Takashi
sound/soc/codecs/hdac_hdmi.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 5eeb0fe..3c5c122 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -144,6 +144,7 @@ struct hdac_hdmi_priv { int num_pin; int num_cvt; int num_ports;
- int display_power; /* 0: power off; 1 power on */ struct mutex pin_mutex; struct hdac_chmap chmap; struct hdac_hdmi_drv_data *drv_data;
@@ -742,12 +743,27 @@ static void hdac_hdmi_set_amp(struct hdac_device *hdev, AC_VERB_SET_AMP_GAIN_MUTE, val); }
+static int wait_for_display_power(struct hdac_hdmi_priv *hdmi) +{
- int i = 0;
- /* sleep for totally 80ms ~ 200ms should be enough */
- while (i < 40) {
if (!hdmi->display_power)usleep_range(2000, 5000);elsereturn 0;i++;- }
- return -EAGAIN;
+}
static int hdac_hdmi_pin_output_widget_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kc, int event) { struct hdac_hdmi_port *port = w->priv; struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev);
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_hdmi_pcm *pcm;
dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n",
@@ -757,6 +773,11 @@ static int hdac_hdmi_pin_output_widget_event(struct snd_soc_dapm_widget *w, if (!pcm) return -EIO;
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n", __func__);return -EAGAIN;- }
- /* set the device if pin is mst_capable */ if (hdac_hdmi_port_select_set(hdev, port) < 0) return -EIO;
@@ -803,6 +824,11 @@ static int hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w, if (!pcm) return -EIO;
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n", __func__);return -EAGAIN;- }
- switch (event) { case SND_SOC_DAPM_PRE_PMU: hdac_hdmi_set_power_state(hdev, cvt->nid, AC_PWRST_D0);
@@ -840,6 +866,7 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w, { struct hdac_hdmi_port *port = w->priv; struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev);
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); int mux_idx;
dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n",
@@ -850,6 +877,11 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w,
mux_idx = dapm_kcontrol_get_value(kc);
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n", __func__);return -EAGAIN;- }
- /* set the device if pin is mst_capable */ if (hdac_hdmi_port_select_set(hdev, port) < 0) return -EIO;
@@ -2068,6 +2100,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev); struct hdac_bus *bus = hdev->bus;
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_ext_link *hlink = NULL;
dev_dbg(dev, "Enter: %s\n", __func__);
@@ -2095,6 +2128,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev) snd_hdac_ext_bus_link_put(bus, hlink);
snd_hdac_display_power(bus, hdev->addr, false);
hdmi->display_power = 0;
return 0;
} @@ -2102,6 +2136,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev) static int hdac_hdmi_runtime_resume(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev);
- struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_bus *bus = hdev->bus; struct hdac_ext_link *hlink = NULL;
@@ -2128,6 +2163,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev) snd_hdac_codec_read(hdev, hdev->afg, 0, AC_VERB_SET_POWER_STATE, AC_PWRST_D0);
- hdmi->display_power = 1; return 0;
}
#else
2.7.4
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Friday, March 22, 2019 5:04 PM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
On Fri, 22 Mar 2019 07:24:56 +0100, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
In S3, there is a contest between dapm event and getting display power.
When resume, the sequence is:
hdac_hdmi_runtime_resume() => snd_hdac_display_power() hdac_hdmi_xxx_widget_event()
The hdac_hdmi_xxx_widget_event() are based on the power being on. Otherwise, the operation on the codec register will fail.
However, in snd_hdac_display_power(), it may sleep because of the mutex_lock() in display driver. In this case, hdac_hdmi_xxx_widget_event() will be called before the power is on.
Let's not operate the registers and wait for the power in hdac_hdmi_xxx_widget_event()
Signed-off-by: Libin Yang libin.yang@intel.com
IMO the workaround looks a bit too fragile. The substantial problem is the race between codec and DAPM. Can this be controlled better, e.g. by defining the proper PM dependency? I thought we may rearrange the PM topology dynamically.
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);elsereturn 0;i++;- }
- return -EAGAIN;
+}
static int hdac_hdmi_pin_output_widget_event(struct
snd_soc_dapm_widget *w,
struct snd_kcontrol *kc, int event) {struct hdac_hdmi_port *port = w->priv; struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev);
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_hdmi_pcm *pcm;
dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n", @@ -757,6
+773,11
@@ static int hdac_hdmi_pin_output_widget_event(struct
snd_soc_dapm_widget *w,
if (!pcm) return -EIO;
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n",__func__);
return -EAGAIN;- }
- /* set the device if pin is mst_capable */ if (hdac_hdmi_port_select_set(hdev, port) < 0) return -EIO;
@@ -803,6 +824,11 @@ static int
hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w,
if (!pcm) return -EIO;
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n",__func__);
return -EAGAIN;- }
- switch (event) { case SND_SOC_DAPM_PRE_PMU: hdac_hdmi_set_power_state(hdev, cvt->nid, AC_PWRST_D0);
@@ -840,6
+866,7 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w, { struct hdac_hdmi_port *port = w->priv; struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev);
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); int mux_idx;
dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n", @@ -850,6
+877,11
@@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w,
mux_idx = dapm_kcontrol_get_value(kc);
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n",__func__);
return -EAGAIN;- }
- /* set the device if pin is mst_capable */ if (hdac_hdmi_port_select_set(hdev, port) < 0) return -EIO;
@@ -2068,6 +2100,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev); struct hdac_bus *bus = hdev->bus;
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_ext_link *hlink = NULL;
dev_dbg(dev, "Enter: %s\n", __func__); @@ -2095,6 +2128,7 @@
static
int hdac_hdmi_runtime_suspend(struct device *dev) snd_hdac_ext_bus_link_put(bus, hlink);
snd_hdac_display_power(bus, hdev->addr, false);
hdmi->display_power = 0;
return 0;
} @@ -2102,6 +2136,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev) static int hdac_hdmi_runtime_resume(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev);
- struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_bus *bus = hdev->bus; struct hdac_ext_link *hlink = NULL;
@@ -2128,6 +2163,7 @@ static int hdac_hdmi_runtime_resume(struct
device *dev)
snd_hdac_codec_read(hdev, hdev->afg, 0,
AC_VERB_SET_POWER_STATE,
AC_PWRST_D0);
- hdmi->display_power = 1; return 0;
}
#else
2.7.4
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Fri, 22 Mar 2019 10:12:37 +0100, Yang, Libin wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Friday, March 22, 2019 5:04 PM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
On Fri, 22 Mar 2019 07:24:56 +0100, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
In S3, there is a contest between dapm event and getting display power.
When resume, the sequence is:
hdac_hdmi_runtime_resume() => snd_hdac_display_power() hdac_hdmi_xxx_widget_event()
The hdac_hdmi_xxx_widget_event() are based on the power being on. Otherwise, the operation on the codec register will fail.
However, in snd_hdac_display_power(), it may sleep because of the mutex_lock() in display driver. In this case, hdac_hdmi_xxx_widget_event() will be called before the power is on.
Let's not operate the registers and wait for the power in hdac_hdmi_xxx_widget_event()
Signed-off-by: Libin Yang libin.yang@intel.com
IMO the workaround looks a bit too fragile. The substantial problem is the race between codec and DAPM. Can this be controlled better, e.g. by defining the proper PM dependency? I thought we may rearrange the PM topology dynamically.
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);elsereturn 0;i++;- }
- return -EAGAIN;
+}
static int hdac_hdmi_pin_output_widget_event(struct
snd_soc_dapm_widget *w,
struct snd_kcontrol *kc, int event) {struct hdac_hdmi_port *port = w->priv; struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev);
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_hdmi_pcm *pcm;
dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n", @@ -757,6
+773,11
@@ static int hdac_hdmi_pin_output_widget_event(struct
snd_soc_dapm_widget *w,
if (!pcm) return -EIO;
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n",__func__);
return -EAGAIN;- }
- /* set the device if pin is mst_capable */ if (hdac_hdmi_port_select_set(hdev, port) < 0) return -EIO;
@@ -803,6 +824,11 @@ static int
hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w,
if (!pcm) return -EIO;
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n",__func__);
return -EAGAIN;- }
- switch (event) { case SND_SOC_DAPM_PRE_PMU: hdac_hdmi_set_power_state(hdev, cvt->nid, AC_PWRST_D0);
@@ -840,6
+866,7 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w, { struct hdac_hdmi_port *port = w->priv; struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev);
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); int mux_idx;
dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n", @@ -850,6
+877,11
@@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w,
mux_idx = dapm_kcontrol_get_value(kc);
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n",__func__);
return -EAGAIN;- }
- /* set the device if pin is mst_capable */ if (hdac_hdmi_port_select_set(hdev, port) < 0) return -EIO;
@@ -2068,6 +2100,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev); struct hdac_bus *bus = hdev->bus;
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_ext_link *hlink = NULL;
dev_dbg(dev, "Enter: %s\n", __func__); @@ -2095,6 +2128,7 @@
static
int hdac_hdmi_runtime_suspend(struct device *dev) snd_hdac_ext_bus_link_put(bus, hlink);
snd_hdac_display_power(bus, hdev->addr, false);
hdmi->display_power = 0;
return 0;
} @@ -2102,6 +2136,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev) static int hdac_hdmi_runtime_resume(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev);
- struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_bus *bus = hdev->bus; struct hdac_ext_link *hlink = NULL;
@@ -2128,6 +2163,7 @@ static int hdac_hdmi_runtime_resume(struct
device *dev)
snd_hdac_codec_read(hdev, hdev->afg, 0,
AC_VERB_SET_POWER_STATE,
AC_PWRST_D0);
- hdmi->display_power = 1; return 0;
}
#else
2.7.4
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi Takashi,
On Fri, 22 Mar 2019 07:24:56 +0100, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
In S3, there is a contest between dapm event and getting display power.
When resume, the sequence is:
hdac_hdmi_runtime_resume() => snd_hdac_display_power() hdac_hdmi_xxx_widget_event()
The hdac_hdmi_xxx_widget_event() are based on the power being on. Otherwise, the operation on the codec register will fail.
However, in snd_hdac_display_power(), it may sleep because of the mutex_lock() in display driver. In this case, hdac_hdmi_xxx_widget_event() will be called before the power is on.
Let's not operate the registers and wait for the power in hdac_hdmi_xxx_widget_event()
Signed-off-by: Libin Yang libin.yang@intel.com
IMO the workaround looks a bit too fragile. The substantial problem is the race between codec and DAPM. Can this be controlled better, e.g. by defining the proper PM dependency? I thought we may rearrange the PM topology dynamically.
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);elsereturn 0;i++;- }
- return -EAGAIN;
+}
static int hdac_hdmi_pin_output_widget_event(struct
snd_soc_dapm_widget *w,
struct snd_kcontrol *kc, int event) {struct hdac_hdmi_port *port = w->priv; struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev);
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_hdmi_pcm *pcm;
dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n", @@ -757,6
+773,11
@@ static int hdac_hdmi_pin_output_widget_event(struct
snd_soc_dapm_widget *w,
if (!pcm) return -EIO;
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n",__func__);
return -EAGAIN;- }
- /* set the device if pin is mst_capable */ if (hdac_hdmi_port_select_set(hdev, port) < 0) return -EIO;
@@ -803,6 +824,11 @@ static int
hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w,
if (!pcm) return -EIO;
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n",__func__);
return -EAGAIN;- }
- switch (event) { case SND_SOC_DAPM_PRE_PMU: hdac_hdmi_set_power_state(hdev, cvt->nid, AC_PWRST_D0);
@@ -840,6
+866,7 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w, { struct hdac_hdmi_port *port = w->priv; struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev);
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); int mux_idx;
dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n", @@ -850,6
+877,11
@@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w,
mux_idx = dapm_kcontrol_get_value(kc);
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n",__func__);
return -EAGAIN;- }
- /* set the device if pin is mst_capable */ if (hdac_hdmi_port_select_set(hdev, port) < 0) return -EIO;
@@ -2068,6 +2100,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev); struct hdac_bus *bus = hdev->bus;
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_ext_link *hlink = NULL;
dev_dbg(dev, "Enter: %s\n", __func__); @@ -2095,6 +2128,7 @@
static
int hdac_hdmi_runtime_suspend(struct device *dev) snd_hdac_ext_bus_link_put(bus, hlink);
snd_hdac_display_power(bus, hdev->addr, false);
hdmi->display_power = 0;
return 0;
} @@ -2102,6 +2136,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev) static int hdac_hdmi_runtime_resume(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev);
- struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_bus *bus = hdev->bus; struct hdac_ext_link *hlink = NULL;
@@ -2128,6 +2163,7 @@ static int hdac_hdmi_runtime_resume(struct
device *dev)
snd_hdac_codec_read(hdev, hdev->afg, 0,
AC_VERB_SET_POWER_STATE,
AC_PWRST_D0);
- hdmi->display_power = 1; return 0;
}
#else
2.7.4
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Fri, 22 Mar 2019 12:46:25 +0100, Yang, Libin wrote:
Hi Takashi,
On Fri, 22 Mar 2019 07:24:56 +0100, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
In S3, there is a contest between dapm event and getting display power.
When resume, the sequence is:
hdac_hdmi_runtime_resume() => snd_hdac_display_power() hdac_hdmi_xxx_widget_event()
The hdac_hdmi_xxx_widget_event() are based on the power being on. Otherwise, the operation on the codec register will fail.
However, in snd_hdac_display_power(), it may sleep because of the mutex_lock() in display driver. In this case, hdac_hdmi_xxx_widget_event() will be called before the power is on.
Let's not operate the registers and wait for the power in hdac_hdmi_xxx_widget_event()
Signed-off-by: Libin Yang libin.yang@intel.com
IMO the workaround looks a bit too fragile. The substantial problem is the race between codec and DAPM. Can this be controlled better, e.g. by defining the proper PM dependency? I thought we may rearrange the PM topology dynamically.
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);elsereturn 0;i++;- }
- return -EAGAIN;
+}
static int hdac_hdmi_pin_output_widget_event(struct
snd_soc_dapm_widget *w,
struct snd_kcontrol *kc, int event) {struct hdac_hdmi_port *port = w->priv; struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev);
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_hdmi_pcm *pcm;
dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n", @@ -757,6
+773,11
@@ static int hdac_hdmi_pin_output_widget_event(struct
snd_soc_dapm_widget *w,
if (!pcm) return -EIO;
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n",__func__);
return -EAGAIN;- }
- /* set the device if pin is mst_capable */ if (hdac_hdmi_port_select_set(hdev, port) < 0) return -EIO;
@@ -803,6 +824,11 @@ static int
hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w,
if (!pcm) return -EIO;
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n",__func__);
return -EAGAIN;- }
- switch (event) { case SND_SOC_DAPM_PRE_PMU: hdac_hdmi_set_power_state(hdev, cvt->nid, AC_PWRST_D0);
@@ -840,6
+866,7 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w, { struct hdac_hdmi_port *port = w->priv; struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev);
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); int mux_idx;
dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n", @@ -850,6
+877,11
@@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w,
mux_idx = dapm_kcontrol_get_value(kc);
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n",__func__);
return -EAGAIN;- }
- /* set the device if pin is mst_capable */ if (hdac_hdmi_port_select_set(hdev, port) < 0) return -EIO;
@@ -2068,6 +2100,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev); struct hdac_bus *bus = hdev->bus;
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_ext_link *hlink = NULL;
dev_dbg(dev, "Enter: %s\n", __func__); @@ -2095,6 +2128,7 @@
static
int hdac_hdmi_runtime_suspend(struct device *dev) snd_hdac_ext_bus_link_put(bus, hlink);
snd_hdac_display_power(bus, hdev->addr, false);
hdmi->display_power = 0;
return 0;
} @@ -2102,6 +2136,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev) static int hdac_hdmi_runtime_resume(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev);
- struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_bus *bus = hdev->bus; struct hdac_ext_link *hlink = NULL;
@@ -2128,6 +2163,7 @@ static int hdac_hdmi_runtime_resume(struct
device *dev)
snd_hdac_codec_read(hdev, hdev->afg, 0,
AC_VERB_SET_POWER_STATE,
AC_PWRST_D0);
- hdmi->display_power = 1; return 0;
}
#else
2.7.4
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Saturday, March 23, 2019 4:26 AM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org 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, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
In S3, there is a contest between dapm event and getting display
power.
When resume, the sequence is:
hdac_hdmi_runtime_resume() => snd_hdac_display_power() hdac_hdmi_xxx_widget_event()
The hdac_hdmi_xxx_widget_event() are based on the power being on. Otherwise, the operation on the codec register will fail.
However, in snd_hdac_display_power(), it may sleep because of the mutex_lock() in display driver. In this case, hdac_hdmi_xxx_widget_event() will be called before the power is on.
Let's not operate the registers and wait for the power in hdac_hdmi_xxx_widget_event()
Signed-off-by: Libin Yang libin.yang@intel.com
IMO the workaround looks a bit too fragile. The substantial problem is the race between codec and DAPM. Can this be controlled better, e.g. by defining the proper PM dependency? I thought we may rearrange the PM topology dynamically.
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);elsereturn 0;i++;- }
- return -EAGAIN;
+}
static int hdac_hdmi_pin_output_widget_event(struct
snd_soc_dapm_widget *w,
struct snd_kcontrol *kc, intevent) {
struct hdac_hdmi_port *port = w->priv; struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev);
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_hdmi_pcm *pcm;
dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n", @@ -
757,6
+773,11
@@ static int hdac_hdmi_pin_output_widget_event(struct
snd_soc_dapm_widget *w,
if (!pcm) return -EIO;
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n",__func__);
return -EAGAIN;- }
- /* set the device if pin is mst_capable */ if (hdac_hdmi_port_select_set(hdev, port) < 0) return -EIO;
@@ -803,6 +824,11 @@ static int
hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w,
if (!pcm) return -EIO;
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n",__func__);
return -EAGAIN;- }
- switch (event) { case SND_SOC_DAPM_PRE_PMU: hdac_hdmi_set_power_state(hdev, cvt->nid,
AC_PWRST_D0);
@@ -840,6
+866,7 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w, { struct hdac_hdmi_port *port = w->priv; struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev);
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); int mux_idx;
dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n", @@ -
850,6
+877,11
@@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w,
mux_idx = dapm_kcontrol_get_value(kc);
- if (wait_for_display_power(hdmi)) {
dev_err(&hdev->dev, "%s: hdmi power is not ready\n",__func__);
return -EAGAIN;- }
- /* set the device if pin is mst_capable */ if (hdac_hdmi_port_select_set(hdev, port) < 0) return -EIO;
@@ -2068,6 +2100,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev); struct hdac_bus *bus = hdev->bus;
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_ext_link *hlink = NULL;
dev_dbg(dev, "Enter: %s\n", __func__); @@ -2095,6 +2128,7
@@
static
int hdac_hdmi_runtime_suspend(struct device *dev) snd_hdac_ext_bus_link_put(bus, hlink);
snd_hdac_display_power(bus, hdev->addr, false);
hdmi->display_power = 0;
return 0;
} @@ -2102,6 +2136,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev) static int hdac_hdmi_runtime_resume(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev);
- struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_bus *bus = hdev->bus; struct hdac_ext_link *hlink = NULL;
@@ -2128,6 +2163,7 @@ static int
hdac_hdmi_runtime_resume(struct
device *dev)
snd_hdac_codec_read(hdev, hdev->afg, 0,
AC_VERB_SET_POWER_STATE,
AC_PWRST_D0);
- hdmi->display_power = 1; return 0;
}
#else
2.7.4
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Mon, 25 Mar 2019 07:51:11 +0100, Yang, Libin wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Saturday, March 23, 2019 4:26 AM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org 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, libin.yang@intel.com wrote: > > From: Libin Yang libin.yang@intel.com > > In S3, there is a contest between dapm event and getting display
power.
> > When resume, the sequence is: > > hdac_hdmi_runtime_resume() > => snd_hdac_display_power() > hdac_hdmi_xxx_widget_event() > > The hdac_hdmi_xxx_widget_event() are based on the power being on. > Otherwise, the operation on the codec register will fail. > > However, in snd_hdac_display_power(), it may sleep because of > the > mutex_lock() in display driver. In this case, > hdac_hdmi_xxx_widget_event() will be called before the power is on. > > Let's not operate the registers and wait for the power in > hdac_hdmi_xxx_widget_event() > > Signed-off-by: Libin Yang libin.yang@intel.com
IMO the workaround looks a bit too fragile. The substantial problem is the race between codec and DAPM. Can this be controlled better, e.g. by defining the proper PM dependency? I thought we may rearrange the PM topology dynamically.
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 > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Monday, March 25, 2019 6:08 PM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org 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:tiwai@suse.de] Sent: Saturday, March 23, 2019 4:26 AM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org 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, libin.yang@intel.com wrote: >> >> From: Libin Yang libin.yang@intel.com >> >> In S3, there is a contest between dapm event and getting >> display
power.
>> >> When resume, the sequence is: >> >> hdac_hdmi_runtime_resume() >> => snd_hdac_display_power() >> hdac_hdmi_xxx_widget_event() >> >> The hdac_hdmi_xxx_widget_event() are based on the power being
on.
>> Otherwise, the operation on the codec register will fail. >> >> However, in snd_hdac_display_power(), it may sleep because of >> the >> mutex_lock() in display driver. In this case, >> hdac_hdmi_xxx_widget_event() will be called before the power is
on.
>> >> Let's not operate the registers and wait for the power in >> hdac_hdmi_xxx_widget_event() >> >> Signed-off-by: Libin Yang libin.yang@intel.com > >IMO the workaround looks a bit too fragile. The substantial >problem is the race between codec and DAPM. Can this be >controlled better, e.g. by defining the proper PM dependency? >I thought we may rearrange the PM topology dynamically.
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 >> Alsa-devel@alsa-project.org >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel >>
On Mon, 25 Mar 2019 15:27:18 +0100, Yang, Libin wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Monday, March 25, 2019 6:08 PM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org 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:tiwai@suse.de] Sent: Saturday, March 23, 2019 4:26 AM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org 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, libin.yang@intel.com wrote: > >> > >> From: Libin Yang libin.yang@intel.com > >> > >> In S3, there is a contest between dapm event and getting > >> display
power.
> >> > >> When resume, the sequence is: > >> > >> hdac_hdmi_runtime_resume() > >> => snd_hdac_display_power() > >> hdac_hdmi_xxx_widget_event() > >> > >> The hdac_hdmi_xxx_widget_event() are based on the power being
on.
> >> Otherwise, the operation on the codec register will fail. > >> > >> However, in snd_hdac_display_power(), it may sleep because of > >> the > >> mutex_lock() in display driver. In this case, > >> hdac_hdmi_xxx_widget_event() will be called before the power is
on.
> >> > >> Let's not operate the registers and wait for the power in > >> hdac_hdmi_xxx_widget_event() > >> > >> Signed-off-by: Libin Yang libin.yang@intel.com > > > >IMO the workaround looks a bit too fragile. The substantial > >problem is the race between codec and DAPM. Can this be > >controlled better, e.g. by defining the proper PM dependency? > >I thought we may rearrange the PM topology dynamically. > > 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 > >> Alsa-devel@alsa-project.org > >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel > >> >
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 libin.yang@intel.com >> > >> >IMO the workaround looks a bit too fragile. The substantial >> >problem is the race between codec and DAPM. Can this be >> >controlled better, e.g. by defining the proper PM dependency? >> >I thought we may rearrange the PM topology dynamically. >> >> 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 >> >> Alsa-devel@alsa-project.org >> >> https://mailman.alsa-project.org/mailman/listinfo/alsa-dev >> >> el >> >> >>
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
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, March 26, 2019 3:37 PM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org 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
On Tue, 26 Mar 2019 08:58:34 +0100, Yang, Libin wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, March 26, 2019 3:37 PM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org 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
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
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:
- HDMI runtime_resume runs
- 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
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:
- HDMI runtime_resume runs
- 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
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:
- HDMI runtime_resume runs
- 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
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, March 26, 2019 7:23 PM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org 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:
- HDMI runtime_resume runs
- 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
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
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
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, March 26, 2019 4:54 PM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org 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
On Tue, 26 Mar 2019 12:15:06 +0100, Yang, Libin wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, March 26, 2019 4:54 PM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org 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:
- 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
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, March 26, 2019 7:19 PM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org 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:tiwai@suse.de] Sent: Tuesday, March 26, 2019 4:54 PM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org 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:
- 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
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:
- 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
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:
- 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
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:
- 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
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, March 28, 2019 3:15 PM To: Yang, Libin libin.yang@intel.com Cc: 'alsa-devel@alsa-project.org' alsa-devel@alsa-project.org; 'broonie@kernel.org' broonie@kernel.org 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:
- 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
On Thu, 28 Mar 2019 08:28:06 +0100, Yang, Libin wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, March 28, 2019 3:15 PM To: Yang, Libin libin.yang@intel.com Cc: 'alsa-devel@alsa-project.org' alsa-devel@alsa-project.org; 'broonie@kernel.org' broonie@kernel.org 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:
- 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
participants (3)
-
libin.yang＠intel.com -
Takashi Iwai -
Yang, Libin