[alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
Yang, Libin
libin.yang at intel.com
Tue Mar 26 08:58:34 CET 2019
Hi Takashi,
>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai at suse.de]
>Sent: Tuesday, March 26, 2019 3:37 PM
>To: Yang, Libin <libin.yang at intel.com>
>Cc: alsa-devel at alsa-project.org; broonie at kernel.org
>Subject: Re: [alsa-devel] [PATCH 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
More information about the Alsa-devel
mailing list