[alsa-devel] [PATCH] ALSA: hda - verify pin:cvt connection on preparing a stream for Intel HDMI codec
From: Mengdong Lin mengdong.lin@intel.com
The pin:converter connection may get reset after S3 on Intel Broadwell HDMI codec. And this can cause multiple pins share a same convertor and mute control will affect each other. We observed a resumed audio playback become silent after S3.
This patch verifies pin:cvt connection on preparing a stream, to assure the pin selects the right convetor and an assigned convertor is not shared by other unused pins. Apply this fix-up on Haswell, Broadwell and Baytrail.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 3ab7063..6000ce9 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -68,6 +68,7 @@ struct hdmi_spec_per_pin { hda_nid_t pin_nid; int num_mux_nids; hda_nid_t mux_nids[HDA_MAX_CONNECTIONS]; + int mux_idx; hda_nid_t cvt_nid;
struct hda_codec *codec; @@ -1342,6 +1343,8 @@ static int hdmi_choose_cvt(struct hda_codec *codec, if (cvt_idx == spec->num_cvts) return -ENODEV;
+ per_pin->mux_idx = mux_idx; + if (cvt_id) *cvt_id = cvt_idx; if (mux_id) @@ -1350,6 +1353,22 @@ static int hdmi_choose_cvt(struct hda_codec *codec, return 0; }
+/* Assure the pin select the right convetor */ +static void intel_verify_pin_cvt_connect(struct hda_codec *codec, + struct hdmi_spec_per_pin *per_pin) +{ + hda_nid_t pin_nid = per_pin->pin_nid; + int mux_idx, curr; + + mux_idx = per_pin->mux_idx; + curr = snd_hda_codec_read(codec, pin_nid, 0, + AC_VERB_GET_CONNECT_SEL, 0); + if (curr != mux_idx) + snd_hda_codec_write_cache(codec, pin_nid, 0, + AC_VERB_SET_CONNECT_SEL, + mux_idx); +} + /* Intel HDMI workaround to fix audio routing issue: * For some Intel display codecs, pins share the same connection list. * So a conveter can be selected by multiple pins and playback on any of these @@ -1751,6 +1770,12 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo, bool non_pcm; int pinctl;
+ if (is_haswell_plus(codec) || is_valleyview(codec)) { + /* the pin:cvt connection may get reset after S3 */ + intel_verify_pin_cvt_connect(codec, per_pin); + intel_not_share_assigned_cvt(codec, pin_nid, per_pin->mux_idx); + } + non_pcm = check_non_pcm_per_cvt(codec, cvt_nid); mutex_lock(&per_pin->lock); per_pin->channels = substream->runtime->channels;
At Tue, 18 Mar 2014 17:35:02 +0800, mengdong.lin@intel.com wrote:
From: Mengdong Lin mengdong.lin@intel.com
The pin:converter connection may get reset after S3 on Intel Broadwell HDMI codec. And this can cause multiple pins share a same convertor and mute control will affect each other. We observed a resumed audio playback become silent after S3.
This patch verifies pin:cvt connection on preparing a stream, to assure the pin selects the right convetor and an assigned convertor is not shared by other unused pins. Apply this fix-up on Haswell, Broadwell and Baytrail.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
Hm, it's still not clear to me why this happens. The connections are recorded via snd_hda_codec_write_cache(), and these values should be restored in the resume. Where the things went wrong?
Takashi
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 3ab7063..6000ce9 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -68,6 +68,7 @@ struct hdmi_spec_per_pin { hda_nid_t pin_nid; int num_mux_nids; hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
int mux_idx; hda_nid_t cvt_nid;
struct hda_codec *codec;
@@ -1342,6 +1343,8 @@ static int hdmi_choose_cvt(struct hda_codec *codec, if (cvt_idx == spec->num_cvts) return -ENODEV;
- per_pin->mux_idx = mux_idx;
- if (cvt_id) *cvt_id = cvt_idx; if (mux_id)
@@ -1350,6 +1353,22 @@ static int hdmi_choose_cvt(struct hda_codec *codec, return 0; }
+/* Assure the pin select the right convetor */ +static void intel_verify_pin_cvt_connect(struct hda_codec *codec,
struct hdmi_spec_per_pin *per_pin)+{
- hda_nid_t pin_nid = per_pin->pin_nid;
- int mux_idx, curr;
- mux_idx = per_pin->mux_idx;
- curr = snd_hda_codec_read(codec, pin_nid, 0,
AC_VERB_GET_CONNECT_SEL, 0);- if (curr != mux_idx)
snd_hda_codec_write_cache(codec, pin_nid, 0,AC_VERB_SET_CONNECT_SEL,mux_idx);+}
/* Intel HDMI workaround to fix audio routing issue:
- For some Intel display codecs, pins share the same connection list.
- So a conveter can be selected by multiple pins and playback on any of these
@@ -1751,6 +1770,12 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo, bool non_pcm; int pinctl;
- if (is_haswell_plus(codec) || is_valleyview(codec)) {
/* the pin:cvt connection may get reset after S3 */intel_verify_pin_cvt_connect(codec, per_pin);intel_not_share_assigned_cvt(codec, pin_nid, per_pin->mux_idx);- }
- non_pcm = check_non_pcm_per_cvt(codec, cvt_nid); mutex_lock(&per_pin->lock); per_pin->channels = substream->runtime->channels;
-- 1.8.1.2
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, March 18, 2014 5:59 PM
The pin:converter connection may get reset after S3 on Intel Broadwell HDMI codec. And this can cause multiple pins share a same convertor and mute control will affect each other. We observed a resumed audio playback become silent after S3.
This patch verifies pin:cvt connection on preparing a stream, to assure the pin selects the right convetor and an assigned convertor is not shared by other unused pins. Apply this fix-up on Haswell,
Broadwell and Baytrail.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
Hm, it's still not clear to me why this happens. The connections are recorded via snd_hda_codec_write_cache(), and these values should be restored in the resume. Where the things went wrong?
I don't think it's the problem of snd_hda_codec_write_cache().
When the pcm stream is opened, the convertor to pin connection is configured by the driver, not interrupted by S3, and then playback start. But after S3, HW forgets this configuration and pin can select the default convertor. Thus a used pin and an unused pin can share a same convertor and a muted pin can make the other used pin no sound output. As a result, a resumed playback after S3 can have no sound output.
The commit f82d7d16aee5eb4c2315ba11a90f2f3c662d45b8 (ALSA : hda - not use assigned converters for all unused pins) is to avoid convertor sharing when a pcm stream is opened. Now this patch will verify pin:converter connection and avoid convertor sharing when preparing the stream, for resuming a stream after S3.
Although this issue after S3 is only observed on Broadwell, we want to apply it to Haswell and Baytrail (Valleyview) for safety consideration.
Thanks Mengdong
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 3ab7063..6000ce9 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -68,6 +68,7 @@ struct hdmi_spec_per_pin { hda_nid_t pin_nid; int num_mux_nids; hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
int mux_idx; hda_nid_t cvt_nid;
struct hda_codec *codec;
@@ -1342,6 +1343,8 @@ static int hdmi_choose_cvt(struct hda_codec
*codec,
if (cvt_idx == spec->num_cvts) return -ENODEV;
- per_pin->mux_idx = mux_idx;
- if (cvt_id) *cvt_id = cvt_idx; if (mux_id)
@@ -1350,6 +1353,22 @@ static int hdmi_choose_cvt(struct hda_codec
*codec,
return 0; }
+/* Assure the pin select the right convetor */ static void +intel_verify_pin_cvt_connect(struct hda_codec *codec,
struct hdmi_spec_per_pin *per_pin) {- hda_nid_t pin_nid = per_pin->pin_nid;
- int mux_idx, curr;
- mux_idx = per_pin->mux_idx;
- curr = snd_hda_codec_read(codec, pin_nid, 0,
AC_VERB_GET_CONNECT_SEL, 0);- if (curr != mux_idx)
snd_hda_codec_write_cache(codec, pin_nid, 0,AC_VERB_SET_CONNECT_SEL,mux_idx);+}
/* Intel HDMI workaround to fix audio routing issue:
- For some Intel display codecs, pins share the same connection list.
- So a conveter can be selected by multiple pins and playback on any
of these @@ -1751,6 +1770,12 @@ static int
generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
bool non_pcm; int pinctl;
- if (is_haswell_plus(codec) || is_valleyview(codec)) {
/* the pin:cvt connection may get reset after S3 */intel_verify_pin_cvt_connect(codec, per_pin);intel_not_share_assigned_cvt(codec, pin_nid,per_pin->mux_idx);
- }
- non_pcm = check_non_pcm_per_cvt(codec, cvt_nid); mutex_lock(&per_pin->lock); per_pin->channels = substream->runtime->channels;
-- 1.8.1.2
At Tue, 18 Mar 2014 14:53:07 +0000, Lin, Mengdong wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, March 18, 2014 5:59 PM
The pin:converter connection may get reset after S3 on Intel Broadwell HDMI codec. And this can cause multiple pins share a same convertor and mute control will affect each other. We observed a resumed audio playback become silent after S3.
This patch verifies pin:cvt connection on preparing a stream, to assure the pin selects the right convetor and an assigned convertor is not shared by other unused pins. Apply this fix-up on Haswell,
Broadwell and Baytrail.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
Hm, it's still not clear to me why this happens. The connections are recorded via snd_hda_codec_write_cache(), and these values should be restored in the resume. Where the things went wrong?
I don't think it's the problem of snd_hda_codec_write_cache().
When the pcm stream is opened, the convertor to pin connection is configured by the driver, not interrupted by S3, and then playback start.
Yes. And at this moment, the connection was already saved in the cache via snd_hda_codec_write_cache().
But after S3, HW forgets this configuration and pin can select the default convertor.
... but the driver restores the old connection read from the cache. So, after S3 resume, this connection should have been already restored. Also the connections of other converters should be also covered, as they are set with snd_hda_codec_write_cache() in intel_not_share_assigned_cvt().
Thus a used pin and an unused pin can share a same convertor and a muted pin can make the other used pin no sound output. As a result, a resumed playback after S3 can have no sound output.
I'm afraid that there is a big missing piece here. Check whether the cached values are really restored properly in the resume. snd_hda_codec_resume_cache() does it.
Takashi
The commit f82d7d16aee5eb4c2315ba11a90f2f3c662d45b8 (ALSA : hda - not use assigned converters for all unused pins) is to avoid convertor sharing when a pcm stream is opened. Now this patch will verify pin:converter connection and avoid convertor sharing when preparing the stream, for resuming a stream after S3.
Although this issue after S3 is only observed on Broadwell, we want to apply it to Haswell and Baytrail (Valleyview) for safety consideration.
Thanks Mengdong
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 3ab7063..6000ce9 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -68,6 +68,7 @@ struct hdmi_spec_per_pin { hda_nid_t pin_nid; int num_mux_nids; hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
int mux_idx; hda_nid_t cvt_nid;
struct hda_codec *codec;
@@ -1342,6 +1343,8 @@ static int hdmi_choose_cvt(struct hda_codec
*codec,
if (cvt_idx == spec->num_cvts) return -ENODEV;
- per_pin->mux_idx = mux_idx;
- if (cvt_id) *cvt_id = cvt_idx; if (mux_id)
@@ -1350,6 +1353,22 @@ static int hdmi_choose_cvt(struct hda_codec
*codec,
return 0; }
+/* Assure the pin select the right convetor */ static void +intel_verify_pin_cvt_connect(struct hda_codec *codec,
struct hdmi_spec_per_pin *per_pin) {- hda_nid_t pin_nid = per_pin->pin_nid;
- int mux_idx, curr;
- mux_idx = per_pin->mux_idx;
- curr = snd_hda_codec_read(codec, pin_nid, 0,
AC_VERB_GET_CONNECT_SEL, 0);- if (curr != mux_idx)
snd_hda_codec_write_cache(codec, pin_nid, 0,AC_VERB_SET_CONNECT_SEL,mux_idx);+}
/* Intel HDMI workaround to fix audio routing issue:
- For some Intel display codecs, pins share the same connection list.
- So a conveter can be selected by multiple pins and playback on any
of these @@ -1751,6 +1770,12 @@ static int
generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
bool non_pcm; int pinctl;
- if (is_haswell_plus(codec) || is_valleyview(codec)) {
/* the pin:cvt connection may get reset after S3 */intel_verify_pin_cvt_connect(codec, per_pin);intel_not_share_assigned_cvt(codec, pin_nid,per_pin->mux_idx);
- }
- non_pcm = check_non_pcm_per_cvt(codec, cvt_nid); mutex_lock(&per_pin->lock); per_pin->channels = substream->runtime->channels;
-- 1.8.1.2
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, March 18, 2014 11:06 PM
The pin:converter connection may get reset after S3 on Intel Broadwell HDMI codec. And this can cause multiple pins share a same convertor and mute control will affect each other. We observed a resumed audio playback become silent after S3.
This patch verifies pin:cvt connection on preparing a stream, to assure the pin selects the right convetor and an assigned convertor is not shared by other unused pins. Apply this fix-up on Haswell,
Broadwell and Baytrail.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
Hm, it's still not clear to me why this happens. The connections are recorded via snd_hda_codec_write_cache(), and these values should be restored in the resume. Where the things went wrong?
I don't think it's the problem of snd_hda_codec_write_cache().
When the pcm stream is opened, the convertor to pin connection is configured by the driver, not interrupted by S3, and then playback start.
Yes. And at this moment, the connection was already saved in the cache via snd_hda_codec_write_cache().
But after S3, HW forgets this configuration and pin can select the default convertor.
... but the driver restores the old connection read from the cache. So, after S3 resume, this connection should have been already restored. Also the connections of other converters should be also covered, as they are set with snd_hda_codec_write_cache() in intel_not_share_assigned_cvt().
Thus a used pin and an unused pin can share a same convertor and a
muted pin can make the other used pin no sound output.
As a result, a resumed playback after S3 can have no sound output.
I'm afraid that there is a big missing piece here. Check whether the cached values are really restored properly in the resume. snd_hda_codec_resume_cache() does it.
Thanks for your tips! snd_hda_codec_resume_cache() works well as expected, it does apply the convertor selections for all pins with the right cached value.
So the big missing piece here is the audio driver restores things earlier than gfx side is ready, and such connect selection is overlooked by HW. After Gfx is ready, the pins make the default selection again.
I think the better solution is to let gfx driver tell audio when it's ready by the new communication channel as we discussed before. And probably we could delay generic_hdmi_resume until gfx notify it's ready.
Not only the pin:cvt connection, but pin's power state/muting status also depend on gfx status as we observed before. Unsolicited event is not reliable and will be lost when the audio controller in D3 and ELD in HW buffer can be broken if the display power well has on/off. A SW channel will be more reliable than using unsol event and checking ELD by pin_sense.
Sorry that the implementation on audio/gfx channel has been pending so long due to other internal tasks. But now we'd better do this asap since several features is blocked by this.
Thanks Mengdong
The commit f82d7d16aee5eb4c2315ba11a90f2f3c662d45b8 (ALSA : hda -
not use assigned converters for all unused pins) is to avoid convertor sharing when a pcm stream is opened.
Now this patch will verify pin:converter connection and avoid convertor
sharing when preparing the stream, for resuming a stream after S3.
Although this issue after S3 is only observed on Broadwell, we want to
apply it to Haswell and Baytrail (Valleyview) for safety consideration.
Thanks Mengdong
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 3ab7063..6000ce9 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -68,6 +68,7 @@ struct hdmi_spec_per_pin { hda_nid_t pin_nid; int num_mux_nids; hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
int mux_idx; hda_nid_t cvt_nid;
struct hda_codec *codec;
@@ -1342,6 +1343,8 @@ static int hdmi_choose_cvt(struct
hda_codec
*codec,
if (cvt_idx == spec->num_cvts) return -ENODEV;
- per_pin->mux_idx = mux_idx;
- if (cvt_id) *cvt_id = cvt_idx; if (mux_id)
@@ -1350,6 +1353,22 @@ static int hdmi_choose_cvt(struct
hda_codec
*codec,
return 0; }
+/* Assure the pin select the right convetor */ static void +intel_verify_pin_cvt_connect(struct hda_codec *codec,
struct hdmi_spec_per_pin *per_pin) {- hda_nid_t pin_nid = per_pin->pin_nid;
- int mux_idx, curr;
- mux_idx = per_pin->mux_idx;
- curr = snd_hda_codec_read(codec, pin_nid, 0,
AC_VERB_GET_CONNECT_SEL, 0);- if (curr != mux_idx)
snd_hda_codec_write_cache(codec, pin_nid, 0,AC_VERB_SET_CONNECT_SEL,mux_idx);+}
/* Intel HDMI workaround to fix audio routing issue:
- For some Intel display codecs, pins share the same connection
list.
- So a conveter can be selected by multiple pins and playback on
any of these @@ -1751,6 +1770,12 @@ static int
generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
bool non_pcm; int pinctl;
- if (is_haswell_plus(codec) || is_valleyview(codec)) {
/* the pin:cvt connection may get reset after S3 */intel_verify_pin_cvt_connect(codec, per_pin);intel_not_share_assigned_cvt(codec, pin_nid,per_pin->mux_idx);
- }
- non_pcm = check_non_pcm_per_cvt(codec, cvt_nid); mutex_lock(&per_pin->lock); per_pin->channels = substream->runtime->channels;
-- 1.8.1.2
At Wed, 19 Mar 2014 04:00:41 +0000, Lin, Mengdong wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, March 18, 2014 11:06 PM
The pin:converter connection may get reset after S3 on Intel Broadwell HDMI codec. And this can cause multiple pins share a same convertor and mute control will affect each other. We observed a resumed audio playback become silent after S3.
This patch verifies pin:cvt connection on preparing a stream, to assure the pin selects the right convetor and an assigned convertor is not shared by other unused pins. Apply this fix-up on Haswell,
Broadwell and Baytrail.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
Hm, it's still not clear to me why this happens. The connections are recorded via snd_hda_codec_write_cache(), and these values should be restored in the resume. Where the things went wrong?
I don't think it's the problem of snd_hda_codec_write_cache().
When the pcm stream is opened, the convertor to pin connection is configured by the driver, not interrupted by S3, and then playback start.
Yes. And at this moment, the connection was already saved in the cache via snd_hda_codec_write_cache().
But after S3, HW forgets this configuration and pin can select the default convertor.
... but the driver restores the old connection read from the cache. So, after S3 resume, this connection should have been already restored. Also the connections of other converters should be also covered, as they are set with snd_hda_codec_write_cache() in intel_not_share_assigned_cvt().
Thus a used pin and an unused pin can share a same convertor and a
muted pin can make the other used pin no sound output.
As a result, a resumed playback after S3 can have no sound output.
I'm afraid that there is a big missing piece here. Check whether the cached values are really restored properly in the resume. snd_hda_codec_resume_cache() does it.
Thanks for your tips! snd_hda_codec_resume_cache() works well as expected, it does apply the convertor selections for all pins with the right cached value.
So the big missing piece here is the audio driver restores things earlier than gfx side is ready, and such connect selection is overlooked by HW. After Gfx is ready, the pins make the default selection again.
That explains. Thanks for finding out.
I think the better solution is to let gfx driver tell audio when it's ready by the new communication channel as we discussed before. And probably we could delay generic_hdmi_resume until gfx notify it's ready.
Not only the pin:cvt connection, but pin's power state/muting status also depend on gfx status as we observed before. Unsolicited event is not reliable and will be lost when the audio controller in D3 and ELD in HW buffer can be broken if the display power well has on/off. A SW channel will be more reliable than using unsol event and checking ELD by pin_sense.
Right, this should be the best way to go. But the question is what we can do for now. I find your patch OK as a temporary workaround. The only problem was to understand why it was really needed.
Sorry that the implementation on audio/gfx channel has been pending so long due to other internal tasks. But now we'd better do this asap since several features is blocked by this.
Yes, but I'm afraid this won't be in 3.15. As a temporary fix, could you resubmit the patch with a more correct description and a patch mentioning that it's a temporary fix?
thanks,
Takashi
Thanks Mengdong
The commit f82d7d16aee5eb4c2315ba11a90f2f3c662d45b8 (ALSA : hda -
not use assigned converters for all unused pins) is to avoid convertor sharing when a pcm stream is opened.
Now this patch will verify pin:converter connection and avoid convertor
sharing when preparing the stream, for resuming a stream after S3.
Although this issue after S3 is only observed on Broadwell, we want to
apply it to Haswell and Baytrail (Valleyview) for safety consideration.
Thanks Mengdong
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 3ab7063..6000ce9 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -68,6 +68,7 @@ struct hdmi_spec_per_pin { hda_nid_t pin_nid; int num_mux_nids; hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
int mux_idx; hda_nid_t cvt_nid;
struct hda_codec *codec;
@@ -1342,6 +1343,8 @@ static int hdmi_choose_cvt(struct
hda_codec
*codec,
if (cvt_idx == spec->num_cvts) return -ENODEV;
- per_pin->mux_idx = mux_idx;
- if (cvt_id) *cvt_id = cvt_idx; if (mux_id)
@@ -1350,6 +1353,22 @@ static int hdmi_choose_cvt(struct
hda_codec
*codec,
return 0; }
+/* Assure the pin select the right convetor */ static void +intel_verify_pin_cvt_connect(struct hda_codec *codec,
struct hdmi_spec_per_pin *per_pin) {- hda_nid_t pin_nid = per_pin->pin_nid;
- int mux_idx, curr;
- mux_idx = per_pin->mux_idx;
- curr = snd_hda_codec_read(codec, pin_nid, 0,
AC_VERB_GET_CONNECT_SEL, 0);- if (curr != mux_idx)
snd_hda_codec_write_cache(codec, pin_nid, 0,AC_VERB_SET_CONNECT_SEL,mux_idx);+}
/* Intel HDMI workaround to fix audio routing issue:
- For some Intel display codecs, pins share the same connection
list.
- So a conveter can be selected by multiple pins and playback on
any of these @@ -1751,6 +1770,12 @@ static int
generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
bool non_pcm; int pinctl;
- if (is_haswell_plus(codec) || is_valleyview(codec)) {
/* the pin:cvt connection may get reset after S3 */intel_verify_pin_cvt_connect(codec, per_pin);intel_not_share_assigned_cvt(codec, pin_nid,per_pin->mux_idx);
- }
- non_pcm = check_non_pcm_per_cvt(codec, cvt_nid); mutex_lock(&per_pin->lock); per_pin->channels = substream->runtime->channels;
-- 1.8.1.2
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, March 19, 2014 2:55 PM To: Lin, Mengdong Cc: alsa-devel@alsa-project.org Subject: Re: [PATCH] ALSA: hda - verify pin:cvt connection on preparing a stream for Intel HDMI codec
At Wed, 19 Mar 2014 04:00:41 +0000, Lin, Mengdong wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, March 18, 2014 11:06 PM
The pin:converter connection may get reset after S3 on Intel Broadwell HDMI codec. And this can cause multiple pins share a same convertor and mute control will affect each other. We observed a resumed audio playback become silent after S3.
This patch verifies pin:cvt connection on preparing a stream, to assure the pin selects the right convetor and an assigned convertor is not shared by other unused pins. Apply this fix-up on Haswell,
Broadwell and Baytrail.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
Hm, it's still not clear to me why this happens. The connections are recorded via snd_hda_codec_write_cache(), and these values should be restored in the resume. Where the things
went wrong?
I don't think it's the problem of snd_hda_codec_write_cache().
When the pcm stream is opened, the convertor to pin connection is configured by the driver, not interrupted by S3, and then playback
start.
Yes. And at this moment, the connection was already saved in the cache via snd_hda_codec_write_cache().
But after S3, HW forgets this configuration and pin can select the default convertor.
... but the driver restores the old connection read from the cache. So, after S3 resume, this connection should have been already
restored.
Also the connections of other converters should be also covered, as they are set with snd_hda_codec_write_cache() in intel_not_share_assigned_cvt().
Thus a used pin and an unused pin can share a same convertor and a
muted pin can make the other used pin no sound output.
As a result, a resumed playback after S3 can have no sound output.
I'm afraid that there is a big missing piece here. Check whether the cached values are really restored properly in the resume. snd_hda_codec_resume_cache() does it.
Thanks for your tips! snd_hda_codec_resume_cache() works well as expected, it does apply
the convertor selections for all pins with the right cached value.
So the big missing piece here is the audio driver restores things earlier
than gfx side is ready, and such connect selection is overlooked by HW.
After Gfx is ready, the pins make the default selection again.
That explains. Thanks for finding out.
I think the better solution is to let gfx driver tell audio when it's ready by
the new communication channel as we discussed before.
And probably we could delay generic_hdmi_resume until gfx notify it's
ready.
Not only the pin:cvt connection, but pin's power state/muting status
also depend on gfx status as we observed before.
Unsolicited event is not reliable and will be lost when the audio
controller in D3 and ELD in HW buffer can be broken if the display power well has on/off.
A SW channel will be more reliable than using unsol event and checking
ELD by pin_sense.
Right, this should be the best way to go. But the question is what we can do for now. I find your patch OK as a temporary workaround. The only problem was to understand why it was really needed.
Sorry that the implementation on audio/gfx channel has been pending
so long due to other internal tasks.
But now we'd better do this asap since several features is blocked by
this.
Yes, but I'm afraid this won't be in 3.15. As a temporary fix, could you resubmit the patch with a more correct description and a patch mentioning that it's a temporary fix?
Okay, I'll do this tomorrow.
Thanks Mengdong
From: Mengdong Lin mengdong.lin@intel.com
This is a temporary fix for some Intel HDMI codecs to avoid no sound output for a resuming playback after S3.
After S3, the audio driver restores pin:cvt connection selections by snd_hda_codec_resume_cache(). However this can happen before the gfx side is ready and such connect selection is overlooked by HW. After gfx is ready, the pins make the default selection again. And this will cause multiple pins share a same convertor and mute control will affect each other. Thus a resumed audio playback become silent after S3.
This patch verifies pin:cvt connection on preparing a stream, to assure the pin selects the right convetor and an assigned convertor is not shared by other unused pins. Apply this fix-up on Haswell, Broadwell and Valleyview (Baytrail).
We need this temporary fix before a reliable software communication channel is established between audio and gfx, to sync audio/gfx operations.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 3ab7063..585c271 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -68,6 +68,7 @@ struct hdmi_spec_per_pin { hda_nid_t pin_nid; int num_mux_nids; hda_nid_t mux_nids[HDA_MAX_CONNECTIONS]; + int mux_idx; hda_nid_t cvt_nid;
struct hda_codec *codec; @@ -1342,6 +1343,8 @@ static int hdmi_choose_cvt(struct hda_codec *codec, if (cvt_idx == spec->num_cvts) return -ENODEV;
+ per_pin->mux_idx = mux_idx; + if (cvt_id) *cvt_id = cvt_idx; if (mux_id) @@ -1350,6 +1353,22 @@ static int hdmi_choose_cvt(struct hda_codec *codec, return 0; }
+/* Assure the pin select the right convetor */ +static void intel_verify_pin_cvt_connect(struct hda_codec *codec, + struct hdmi_spec_per_pin *per_pin) +{ + hda_nid_t pin_nid = per_pin->pin_nid; + int mux_idx, curr; + + mux_idx = per_pin->mux_idx; + curr = snd_hda_codec_read(codec, pin_nid, 0, + AC_VERB_GET_CONNECT_SEL, 0); + if (curr != mux_idx) + snd_hda_codec_write_cache(codec, pin_nid, 0, + AC_VERB_SET_CONNECT_SEL, + mux_idx); +} + /* Intel HDMI workaround to fix audio routing issue: * For some Intel display codecs, pins share the same connection list. * So a conveter can be selected by multiple pins and playback on any of these @@ -1751,6 +1770,19 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo, bool non_pcm; int pinctl;
+ if (is_haswell_plus(codec) || is_valleyview(codec)) { + /* Verify pin:cvt selections to avoid silent audio after S3. + * After S3, the audio driver restores pin:cvt selections + * but this can happen before gfx is ready and such selection + * is overlooked by HW. Thus multiple pins can share a same + * default convertor and mute control will affect each other, + * which can cause a resumed audio playback become silent + * after S3. + */ + intel_verify_pin_cvt_connect(codec, per_pin); + intel_not_share_assigned_cvt(codec, pin_nid, per_pin->mux_idx); + } + non_pcm = check_non_pcm_per_cvt(codec, cvt_nid); mutex_lock(&per_pin->lock); per_pin->channels = substream->runtime->channels;
At Thu, 20 Mar 2014 13:01:06 +0800, mengdong.lin@intel.com wrote:
From: Mengdong Lin mengdong.lin@intel.com
This is a temporary fix for some Intel HDMI codecs to avoid no sound output for a resuming playback after S3.
After S3, the audio driver restores pin:cvt connection selections by snd_hda_codec_resume_cache(). However this can happen before the gfx side is ready and such connect selection is overlooked by HW. After gfx is ready, the pins make the default selection again. And this will cause multiple pins share a same convertor and mute control will affect each other. Thus a resumed audio playback become silent after S3.
This patch verifies pin:cvt connection on preparing a stream, to assure the pin selects the right convetor and an assigned convertor is not shared by other unused pins. Apply this fix-up on Haswell, Broadwell and Valleyview (Baytrail).
We need this temporary fix before a reliable software communication channel is established between audio and gfx, to sync audio/gfx operations.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
Thanks, applied.
Takashi
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 3ab7063..585c271 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -68,6 +68,7 @@ struct hdmi_spec_per_pin { hda_nid_t pin_nid; int num_mux_nids; hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
int mux_idx; hda_nid_t cvt_nid;
struct hda_codec *codec;
@@ -1342,6 +1343,8 @@ static int hdmi_choose_cvt(struct hda_codec *codec, if (cvt_idx == spec->num_cvts) return -ENODEV;
- per_pin->mux_idx = mux_idx;
- if (cvt_id) *cvt_id = cvt_idx; if (mux_id)
@@ -1350,6 +1353,22 @@ static int hdmi_choose_cvt(struct hda_codec *codec, return 0; }
+/* Assure the pin select the right convetor */ +static void intel_verify_pin_cvt_connect(struct hda_codec *codec,
struct hdmi_spec_per_pin *per_pin)+{
- hda_nid_t pin_nid = per_pin->pin_nid;
- int mux_idx, curr;
- mux_idx = per_pin->mux_idx;
- curr = snd_hda_codec_read(codec, pin_nid, 0,
AC_VERB_GET_CONNECT_SEL, 0);- if (curr != mux_idx)
snd_hda_codec_write_cache(codec, pin_nid, 0,AC_VERB_SET_CONNECT_SEL,mux_idx);+}
/* Intel HDMI workaround to fix audio routing issue:
- For some Intel display codecs, pins share the same connection list.
- So a conveter can be selected by multiple pins and playback on any of these
@@ -1751,6 +1770,19 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo, bool non_pcm; int pinctl;
- if (is_haswell_plus(codec) || is_valleyview(codec)) {
/* Verify pin:cvt selections to avoid silent audio after S3.* After S3, the audio driver restores pin:cvt selections* but this can happen before gfx is ready and such selection* is overlooked by HW. Thus multiple pins can share a same* default convertor and mute control will affect each other,* which can cause a resumed audio playback become silent* after S3.*/intel_verify_pin_cvt_connect(codec, per_pin);intel_not_share_assigned_cvt(codec, pin_nid, per_pin->mux_idx);- }
- non_pcm = check_non_pcm_per_cvt(codec, cvt_nid); mutex_lock(&per_pin->lock); per_pin->channels = substream->runtime->channels;
-- 1.8.1.2
On 03/20/2014 07:37 AM, Takashi Iwai wrote:
At Thu, 20 Mar 2014 13:01:06 +0800, mengdong.lin@intel.com wrote:
From: Mengdong Lin mengdong.lin@intel.com
This is a temporary fix for some Intel HDMI codecs to avoid no sound output for a resuming playback after S3.
After S3, the audio driver restores pin:cvt connection selections by snd_hda_codec_resume_cache(). However this can happen before the gfx side is ready and such connect selection is overlooked by HW. After gfx is ready, the pins make the default selection again. And this will cause multiple pins share a same convertor and mute control will affect each other. Thus a resumed audio playback become silent after S3.
This patch verifies pin:cvt connection on preparing a stream, to assure the pin selects the right convetor and an assigned convertor is not shared by other unused pins. Apply this fix-up on Haswell, Broadwell and Valleyview (Baytrail).
We need this temporary fix before a reliable software communication channel is established between audio and gfx, to sync audio/gfx operations.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
Thanks, applied.
Hi,
Any reason this commit isn't sent to stable? Looks like a candidate.
Takashi
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 3ab7063..585c271 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -68,6 +68,7 @@ struct hdmi_spec_per_pin { hda_nid_t pin_nid; int num_mux_nids; hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
int mux_idx; hda_nid_t cvt_nid;
struct hda_codec *codec;
@@ -1342,6 +1343,8 @@ static int hdmi_choose_cvt(struct hda_codec *codec, if (cvt_idx == spec->num_cvts) return -ENODEV;
- per_pin->mux_idx = mux_idx;
- if (cvt_id) *cvt_id = cvt_idx; if (mux_id)
@@ -1350,6 +1353,22 @@ static int hdmi_choose_cvt(struct hda_codec *codec, return 0; }
+/* Assure the pin select the right convetor */ +static void intel_verify_pin_cvt_connect(struct hda_codec *codec,
struct hdmi_spec_per_pin *per_pin)+{
- hda_nid_t pin_nid = per_pin->pin_nid;
- int mux_idx, curr;
- mux_idx = per_pin->mux_idx;
- curr = snd_hda_codec_read(codec, pin_nid, 0,
AC_VERB_GET_CONNECT_SEL, 0);- if (curr != mux_idx)
snd_hda_codec_write_cache(codec, pin_nid, 0,AC_VERB_SET_CONNECT_SEL,mux_idx);+}
/* Intel HDMI workaround to fix audio routing issue:
- For some Intel display codecs, pins share the same connection list.
- So a conveter can be selected by multiple pins and playback on any of these
@@ -1751,6 +1770,19 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo, bool non_pcm; int pinctl;
- if (is_haswell_plus(codec) || is_valleyview(codec)) {
/* Verify pin:cvt selections to avoid silent audio after S3.* After S3, the audio driver restores pin:cvt selections* but this can happen before gfx is ready and such selection* is overlooked by HW. Thus multiple pins can share a same* default convertor and mute control will affect each other,* which can cause a resumed audio playback become silent* after S3.*/intel_verify_pin_cvt_connect(codec, per_pin);intel_not_share_assigned_cvt(codec, pin_nid, per_pin->mux_idx);- }
- non_pcm = check_non_pcm_per_cvt(codec, cvt_nid); mutex_lock(&per_pin->lock); per_pin->channels = substream->runtime->channels;
-- 1.8.1.2
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
At Fri, 28 Mar 2014 09:24:53 +0100, David Henningsson wrote:
On 03/20/2014 07:37 AM, Takashi Iwai wrote:
At Thu, 20 Mar 2014 13:01:06 +0800, mengdong.lin@intel.com wrote:
From: Mengdong Lin mengdong.lin@intel.com
This is a temporary fix for some Intel HDMI codecs to avoid no sound output for a resuming playback after S3.
After S3, the audio driver restores pin:cvt connection selections by snd_hda_codec_resume_cache(). However this can happen before the gfx side is ready and such connect selection is overlooked by HW. After gfx is ready, the pins make the default selection again. And this will cause multiple pins share a same convertor and mute control will affect each other. Thus a resumed audio playback become silent after S3.
This patch verifies pin:cvt connection on preparing a stream, to assure the pin selects the right convetor and an assigned convertor is not shared by other unused pins. Apply this fix-up on Haswell, Broadwell and Valleyview (Baytrail).
We need this temporary fix before a reliable software communication channel is established between audio and gfx, to sync audio/gfx operations.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
Thanks, applied.
Hi,
Any reason this commit isn't sent to stable? Looks like a candidate.
No particular reason from my side. Feel free to ask for stable once when the patch hits to Linus tree.
thanks,
Takashi
participants (4)
-
David Henningsson -
Lin, Mengdong -
mengdong.lin＠intel.com
-
Takashi Iwai