From: "Yang, Libin" <libin.yang@intel.com>
To: Takashi Iwai <tiwai@suse.de>
Cc: "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
"broonie@kernel.org" <broonie@kernel.org>
Subject: Re: [PATCH V2] ASoC: fix hdmi codec driver contest in S3
Date: Tue, 26 Mar 2019 07:58:34 +0000 [thread overview]
Message-ID: <96A12704CE18D347B625EE2D4A099D19527FB25E@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <s5hr2aum6i5.wl-tiwai@suse.de>
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
next prev parent reply other threads:[~2019-03-26 7:58 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-22 6:24 [PATCH V2] ASoC: fix hdmi codec driver contest in S3 libin.yang
2019-03-22 9:04 ` Takashi Iwai
2019-03-22 9:12 ` Yang, Libin
2019-03-22 9:56 ` Takashi Iwai
2019-03-22 11:46 ` Yang, Libin
2019-03-22 20:25 ` Takashi Iwai
2019-03-25 6:51 ` Yang, Libin
2019-03-25 10:08 ` Takashi Iwai
2019-03-25 14:27 ` Yang, Libin
2019-03-25 14:38 ` Takashi Iwai
2019-03-26 5:42 ` Yang, Libin
2019-03-26 7:36 ` Takashi Iwai
2019-03-26 7:58 ` Yang, Libin [this message]
2019-03-26 8:59 ` Takashi Iwai
2019-03-26 11:02 ` Yang, Libin
2019-03-26 11:10 ` Takashi Iwai
2019-03-26 11:19 ` Yang, Libin
2019-03-26 11:22 ` Takashi Iwai
2019-03-26 11:28 ` Yang, Libin
2019-03-26 8:46 ` Yang, Libin
2019-03-26 8:53 ` Takashi Iwai
2019-03-26 11:15 ` Yang, Libin
2019-03-26 11:19 ` Takashi Iwai
2019-03-26 11:21 ` Yang, Libin
2019-03-27 6:14 ` Yang, Libin
2019-03-28 7:10 ` Yang, Libin
2019-03-28 7:15 ` Takashi Iwai
2019-03-28 7:28 ` Yang, Libin
2019-03-28 7:31 ` Takashi Iwai
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=96A12704CE18D347B625EE2D4A099D19527FB25E@SHSMSX103.ccr.corp.intel.com \
--to=libin.yang@intel.com \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=tiwai@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.