[alsa-devel] [PATCH 0/4] Haswell Display audio routing bug fix
This patchset used to fix some display audio issues on Haswell platform.
I tested this patch on Haswell ult board, C0 stepping, with eDP pannel, HDMI monitor, DP monitor.
The fixed issues: 1) when only HDMI or DP monitor connected, hear sound on ALL three HDMI devices. 2) when HDMI + DP monitors connected, hear sound on HDMI and DP at the same time no matter you're playing audio on DP or HDMI device. 3) When DP + HDMI + eDP connected, no sound could be heard. 4) After bootup, play audio on HDMI device 7(use second pin), no sound. If play audio on device 3 at first(use first pin), it has sound again.
There's quite different hardware changes for haswell display audio, compared with previous Ivybridge/Sandybridge. The common hd-a driver and hdmi driver need make improvement to support.
Haswell chip supports Dp1.2 feature(see ref 1). This is very cool feture windows had supported. We get confirm linux gfx team has plan to support Dp1.2 in future too. We're ready to support dp1.2 feature in audio side, as the converter selection dependency is fixed in this patchset. Also we need some improvement in HDA driver side because Haswell added new Verb to support DP1.2.
here’s a video show about DP1.2 feature http://www.club-3d.com/index.php/products/reader.en/product/mst-hub-1-3.html
Please apply the patches based on Daniel's drm-intel-next-queued branch: http://cgit.freedesktop.org/~danvet/drm-intel/log/?h=drm-intel-next-queued last commit 80e83831a64b9a5d49e844691037b2d4be0f14f9
Please feel free to let me know the issues you meet during test.
Wang Xingchao (4): ALSA: hda - Haswell converter power state D0 verify ALSA: hda - Return error when open empty hdmi device drm/i915: Add display audio routing APIs for ALSA ALSA: hda - Add display audio routing API for haswell
drivers/gpu/drm/i915/i915_drv.h | 18 +++++ drivers/gpu/drm/i915/intel_ddi.c | 131 +++++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_display.c | 7 +- drivers/gpu/drm/i915/intel_drv.h | 1 + include/drm/i915_powerwell.h | 5 ++ sound/pci/hda/hda_i915.c | 83 ++++++++++++++++++++++ sound/pci/hda/hda_i915.h | 4 ++ sound/pci/hda/patch_hdmi.c | 37 ++++++++++ 8 files changed, 281 insertions(+), 5 deletions(-)
Haswell converters maybe in wrong power state before usage. i.e. only converter 0 is in D0, converter 1/2 are in D3. When pin choose converter 1/2, there's no audio output.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com --- sound/pci/hda/patch_hdmi.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index e12f7a0..8db5eb6 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1053,6 +1053,23 @@ static void haswell_verify_pin_D0(struct hda_codec *codec, hda_nid_t nid) } }
+static void hsw_verify_cvt_D0(struct hdmi_spec *spec, + struct hda_codec *codec) +{ + struct hdmi_spec_per_cvt *per_cvt; + int pwr, cvt_idx; + + for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) { + per_cvt = get_cvt(spec, cvt_idx); + pwr = snd_hda_codec_read(codec, per_cvt->cvt_nid, 0, + AC_VERB_GET_POWER_STATE, 0); + pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT; + if (pwr != AC_PWRST_D0) + snd_hda_codec_write(codec, per_cvt->cvt_nid, 0, AC_VERB_SET_POWER_STATE, + AC_PWRST_D0); + } +} + /* * Callbacks */ @@ -1122,6 +1139,9 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, per_pin = get_pin(spec, pin_idx); eld = &per_pin->sink_eld;
+ if (codec->vendor_id == 0x80862807) + hsw_verify_cvt_D0(spec, codec); + /* Dynamically assign converter to stream */ for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) { per_cvt = get_cvt(spec, cvt_idx);
At Fri, 14 Jun 2013 23:20:26 +0800, Wang Xingchao wrote:
Haswell converters maybe in wrong power state before usage. i.e. only converter 0 is in D0, converter 1/2 are in D3. When pin choose converter 1/2, there's no audio output.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
Isn't this needed in hdmi_setup_stream() instead of open, like haswell_verify_pin_D0() does? Note that the open callback won't be called at PM resume. Also, if it's just a matter of the connected converter from the specific pin, you can check the power state of the specific converter, instead of checking the all converters at each time.
If my guess above is correct, the better code would be to merge the power check of converter into haswell_verify_pin_D0().
thanks,
Takashi
sound/pci/hda/patch_hdmi.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index e12f7a0..8db5eb6 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1053,6 +1053,23 @@ static void haswell_verify_pin_D0(struct hda_codec *codec, hda_nid_t nid) } }
+static void hsw_verify_cvt_D0(struct hdmi_spec *spec,
struct hda_codec *codec)+{
- struct hdmi_spec_per_cvt *per_cvt;
- int pwr, cvt_idx;
- for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) {
per_cvt = get_cvt(spec, cvt_idx);pwr = snd_hda_codec_read(codec, per_cvt->cvt_nid, 0,AC_VERB_GET_POWER_STATE, 0);pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;if (pwr != AC_PWRST_D0)snd_hda_codec_write(codec, per_cvt->cvt_nid, 0, AC_VERB_SET_POWER_STATE,AC_PWRST_D0);- }
+}
/*
- Callbacks
*/ @@ -1122,6 +1139,9 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, per_pin = get_pin(spec, pin_idx); eld = &per_pin->sink_eld;
- if (codec->vendor_id == 0x80862807)
hsw_verify_cvt_D0(spec, codec);- /* Dynamically assign converter to stream */ for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) { per_cvt = get_cvt(spec, cvt_idx);
-- 1.8.1.2
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Monday, June 17, 2013 5:01 PM To: Wang Xingchao Cc: daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; david.henningsson@canonical.com; Wang, Xingchao Subject: Re: [PATCH 1/4] ALSA: hda - Haswell converter power state D0 verify
At Fri, 14 Jun 2013 23:20:26 +0800, Wang Xingchao wrote:
Haswell converters maybe in wrong power state before usage. i.e. only converter 0 is in D0, converter 1/2 are in D3. When pin choose converter 1/2, there's no audio output.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
Isn't this needed in hdmi_setup_stream() instead of open, like haswell_verify_pin_D0() does? Note that the open callback won't be called at PM resume. Also, if it's just a matter of the connected converter from the specific pin, you can check the power state of the specific converter, instead of checking the all converters at each time.
If my guess above is correct, the better code would be to merge the power check of converter into haswell_verify_pin_D0().
Good point, will merge it into haswell_verify_pin_D0(), should the API name change accordingly?
Thanks --xingchao
thanks,
Takashi
sound/pci/hda/patch_hdmi.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index e12f7a0..8db5eb6 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1053,6 +1053,23 @@ static void haswell_verify_pin_D0(struct
hda_codec *codec, hda_nid_t nid)
} }
+static void hsw_verify_cvt_D0(struct hdmi_spec *spec,
struct hda_codec *codec)+{
- struct hdmi_spec_per_cvt *per_cvt;
- int pwr, cvt_idx;
- for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) {
per_cvt = get_cvt(spec, cvt_idx);pwr = snd_hda_codec_read(codec, per_cvt->cvt_nid, 0,AC_VERB_GET_POWER_STATE, 0);pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;if (pwr != AC_PWRST_D0)snd_hda_codec_write(codec, per_cvt->cvt_nid, 0,AC_VERB_SET_POWER_STATE,
AC_PWRST_D0);- }
+}
/*
- Callbacks
*/ @@ -1122,6 +1139,9 @@ static int hdmi_pcm_open(struct
hda_pcm_stream *hinfo,
per_pin = get_pin(spec, pin_idx); eld = &per_pin->sink_eld;
- if (codec->vendor_id == 0x80862807)
hsw_verify_cvt_D0(spec, codec);- /* Dynamically assign converter to stream */ for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) { per_cvt = get_cvt(spec, cvt_idx);
-- 1.8.1.2
when user open HDMI device 3/7/8, if it has no physical device connected, return error. The bug is from Haswell platform, All pins choose converter 0 by default in hardware level, maybe only pin 1 had valid monitor connected. if user play audio on pin 0/2, pin 1 can get audio data too.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com --- sound/pci/hda/patch_hdmi.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 8db5eb6..d766f40 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1139,6 +1139,9 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, per_pin = get_pin(spec, pin_idx); eld = &per_pin->sink_eld;
+ if (!eld->monitor_present || !eld->eld_valid) + return -EIO; + if (codec->vendor_id == 0x80862807) hsw_verify_cvt_D0(spec, codec);
On 06/14/2013 05:20 PM, Wang Xingchao wrote:
when user open HDMI device 3/7/8, if it has no physical device connected, return error.
This patch will cause regressions in two big use cases:
1) Older chipsets (at least from non-Intel vendors) might not support correct ELD reporting. Thus this will cause HDMI audio to stop working there.
2) In PulseAudio's current design, PulseAudio probes the device at startup and caches the result. Unfortunately, there is no reprobing at plug/unplug, so if the monitor is hotplugged, it will not work unless PulseAudio is restarted afterwards.
The bug is from Haswell platform, All pins choose converter 0 by default in hardware level, maybe only pin 1 had valid monitor connected. if user play audio on pin 0/2, pin 1 can get audio data too.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
sound/pci/hda/patch_hdmi.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 8db5eb6..d766f40 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1139,6 +1139,9 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, per_pin = get_pin(spec, pin_idx); eld = &per_pin->sink_eld;
- if (!eld->monitor_present || !eld->eld_valid)
return -EIO;- if (codec->vendor_id == 0x80862807) hsw_verify_cvt_D0(spec, codec);
-----Original Message----- From: David Henningsson [mailto:david.henningsson@canonical.com] Sent: Monday, June 17, 2013 4:24 PM To: Wang Xingchao Cc: tiwai@suse.de; daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; Wang, Xingchao Subject: Re: [PATCH 2/4] ALSA: hda - Return error when open empty hdmi device
On 06/14/2013 05:20 PM, Wang Xingchao wrote:
when user open HDMI device 3/7/8, if it has no physical device connected, return error.
This patch will cause regressions in two big use cases:
- Older chipsets (at least from non-Intel vendors) might not support correct
ELD reporting. Thus this will cause HDMI audio to stop working there.
Thanks for pointing this out. It's a good case I missed. :) I thought ELD info was monitor specific, and reported on common platforms. But sometimes user get confused when opening a hdmi device and started to play audio,but hear nothing, even there's no error message. Is there any way for old chipset which has no correct ELD reporting but do have audio functionality? App should check before really play audio on the device.
- In PulseAudio's current design, PulseAudio probes the device at startup
and caches the result. Unfortunately, there is no reprobing at plug/unplug, so if the monitor is hotplugged, it will not work unless PulseAudio is restarted afterwards.
For haswell ult board only, DDI port D is not supported, this results in screen flicker when playing on third pin. But for other haswell boards, the DDI port D is normal. So at first glance my idea is to disable opening device without physical device connecting.
Thanks --xingchao
The bug is from Haswell platform, All pins choose converter 0 by default in hardware level, maybe only pin 1 had valid monitor connected. if user play audio on pin 0/2, pin 1 can get audio data too.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
sound/pci/hda/patch_hdmi.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 8db5eb6..d766f40 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1139,6 +1139,9 @@ static int hdmi_pcm_open(struct
hda_pcm_stream *hinfo,
per_pin = get_pin(spec, pin_idx); eld = &per_pin->sink_eld;
- if (!eld->monitor_present || !eld->eld_valid)
return -EIO;- if (codec->vendor_id == 0x80862807) hsw_verify_cvt_D0(spec, codec);
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
On 06/17/2013 01:54 PM, Wang, Xingchao wrote:
-----Original Message----- From: David Henningsson [mailto:david.henningsson@canonical.com] Sent: Monday, June 17, 2013 4:24 PM To: Wang Xingchao Cc: tiwai@suse.de; daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; Wang, Xingchao Subject: Re: [PATCH 2/4] ALSA: hda - Return error when open empty hdmi device
On 06/14/2013 05:20 PM, Wang Xingchao wrote:
when user open HDMI device 3/7/8, if it has no physical device connected, return error.
This patch will cause regressions in two big use cases:
- Older chipsets (at least from non-Intel vendors) might not support correct
ELD reporting. Thus this will cause HDMI audio to stop working there.
Thanks for pointing this out. It's a good case I missed. :) I thought ELD info was monitor specific, and reported on common platforms.
We have a generation of Nvidia cards that does not support presence detect (and no ELD) for the audio codec at all.
I'm not sure if we also have cards where presence detect works, but there never comes any ELD. Or if this can potentially vary depending on the connected monitor. But I think this should also be handled in the best way.
But sometimes user get confused when opening a hdmi device and started to play audio,but hear nothing, even there's no error message. Is there any way for old chipset which has no correct ELD reporting but do have audio functionality? App should check before really play audio on the device.
PulseAudio (with support from some of the GUIs) support looking at the Presence Detect, and "hiding" the device if nothing is currently connected.
I agree in principle with that you should be given some warning if you try to play back to unconnected things, but for me it's not a high priority to improve. E g, the situation is the same for analog audio - imagine a small desktop with only a headphone output: you can still play audio to the headphone output even when there's no headphone connected in the jack.
- In PulseAudio's current design, PulseAudio probes the device at startup
and caches the result. Unfortunately, there is no reprobing at plug/unplug, so if the monitor is hotplugged, it will not work unless PulseAudio is restarted afterwards.
For haswell ult board only, DDI port D is not supported, this results in screen flicker when playing on third pin. But for other haswell boards, the DDI port D is normal. So at first glance my idea is to disable opening device without physical device connecting.
Maybe it's possible to fix on the gfx side? It sounds like a graphic driver bug if the screen flickers due to anything happening on the audio side.
Thanks --xingchao
The bug is from Haswell platform, All pins choose converter 0 by default in hardware level, maybe only pin 1 had valid monitor connected. if user play audio on pin 0/2, pin 1 can get audio data too.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
sound/pci/hda/patch_hdmi.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 8db5eb6..d766f40 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1139,6 +1139,9 @@ static int hdmi_pcm_open(struct
hda_pcm_stream *hinfo,
per_pin = get_pin(spec, pin_idx); eld = &per_pin->sink_eld;
- if (!eld->monitor_present || !eld->eld_valid)
return -EIO;- if (codec->vendor_id == 0x80862807) hsw_verify_cvt_D0(spec, codec);
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
-----Original Message----- From: David Henningsson [mailto:david.henningsson@canonical.com] Sent: Monday, June 17, 2013 8:15 PM To: Wang, Xingchao Cc: Wang Xingchao; tiwai@suse.de; daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org Subject: Re: [PATCH 2/4] ALSA: hda - Return error when open empty hdmi device
On 06/17/2013 01:54 PM, Wang, Xingchao wrote:
-----Original Message----- From: David Henningsson [mailto:david.henningsson@canonical.com] Sent: Monday, June 17, 2013 4:24 PM To: Wang Xingchao Cc: tiwai@suse.de; daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; Wang, Xingchao Subject: Re: [PATCH 2/4] ALSA: hda - Return error when open empty hdmi device
On 06/14/2013 05:20 PM, Wang Xingchao wrote:
when user open HDMI device 3/7/8, if it has no physical device connected, return error.
This patch will cause regressions in two big use cases:
- Older chipsets (at least from non-Intel vendors) might not
support correct ELD reporting. Thus this will cause HDMI audio to stop
working there.
Thanks for pointing this out. It's a good case I missed. :) I thought ELD info was monitor specific, and reported on common platforms.
We have a generation of Nvidia cards that does not support presence detect (and no ELD) for the audio codec at all.
I'm not sure if we also have cards where presence detect works, but there never comes any ELD. Or if this can potentially vary depending on the connected monitor. But I think this should also be handled in the best way.
But sometimes user get confused when opening a hdmi device and started to play audio,but hear nothing, even there's no error message. Is there any way for old chipset which has no correct ELD reporting but do have
audio functionality? App should check before really play audio on the device.
PulseAudio (with support from some of the GUIs) support looking at the Presence Detect, and "hiding" the device if nothing is currently connected.
That's good if pulseaudio can hide the devices. But "aplay -L" still show the full HDMI devices. I get complains from QA when they test audio playback in cmdline with "aplay" or "speaker-test", they are confused that the device exists but cannot hear sounds.
Thanks --xingchao
I agree in principle with that you should be given some warning if you try to play back to unconnected things, but for me it's not a high priority to improve. E g, the situation is the same for analog audio - imagine a small desktop with only a headphone output: you can still play audio to the headphone output even when there's no headphone connected in the jack.
- In PulseAudio's current design, PulseAudio probes the device at
startup and caches the result. Unfortunately, there is no reprobing at plug/unplug, so if the monitor is hotplugged, it will not work unless PulseAudio is restarted afterwards.
For haswell ult board only, DDI port D is not supported, this results in
screen flicker when playing on third pin.
But for other haswell boards, the DDI port D is normal. So at first glance my idea is to disable opening device without physical device
connecting.
Maybe it's possible to fix on the gfx side? It sounds like a graphic driver bug if the screen flickers due to anything happening on the audio side.
Thanks --xingchao
The bug is from Haswell platform, All pins choose converter 0 by default in hardware level, maybe only pin 1 had valid monitor connected. if user play audio on pin 0/2, pin 1 can get audio data too.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
sound/pci/hda/patch_hdmi.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 8db5eb6..d766f40 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1139,6 +1139,9 @@ static int hdmi_pcm_open(struct
hda_pcm_stream *hinfo,
per_pin = get_pin(spec, pin_idx); eld = &per_pin->sink_eld;
- if (!eld->monitor_present || !eld->eld_valid)
return -EIO;- if (codec->vendor_id == 0x80862807) hsw_verify_cvt_D0(spec, codec);
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
Hi Daniel,
Do you know the issue I mentioned below? Is it a known issue in gfx side? On Haswell ULT, when playing audio on the third pin, DP monitor would cause flicker while HDMI monitor is fine.
Thanks --xingchao
-----Original Message----- From: David Henningsson [mailto:david.henningsson@canonical.com] Sent: Monday, June 17, 2013 8:15 PM To: Wang, Xingchao Cc: Wang Xingchao; tiwai@suse.de; daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org Subject: Re: [PATCH 2/4] ALSA: hda - Return error when open empty hdmi device
On 06/17/2013 01:54 PM, Wang, Xingchao wrote:
-----Original Message----- From: David Henningsson [mailto:david.henningsson@canonical.com] Sent: Monday, June 17, 2013 4:24 PM To: Wang Xingchao Cc: tiwai@suse.de; daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; Wang, Xingchao Subject: Re: [PATCH 2/4] ALSA: hda - Return error when open empty hdmi device
On 06/14/2013 05:20 PM, Wang Xingchao wrote:
when user open HDMI device 3/7/8, if it has no physical device connected, return error.
This patch will cause regressions in two big use cases:
- Older chipsets (at least from non-Intel vendors) might not
support correct ELD reporting. Thus this will cause HDMI audio to stop
working there.
Thanks for pointing this out. It's a good case I missed. :) I thought ELD info was monitor specific, and reported on common platforms.
We have a generation of Nvidia cards that does not support presence detect (and no ELD) for the audio codec at all.
I'm not sure if we also have cards where presence detect works, but there never comes any ELD. Or if this can potentially vary depending on the connected monitor. But I think this should also be handled in the best way.
But sometimes user get confused when opening a hdmi device and started to play audio,but hear nothing, even there's no error message. Is there any way for old chipset which has no correct ELD reporting but do have
audio functionality? App should check before really play audio on the device.
PulseAudio (with support from some of the GUIs) support looking at the Presence Detect, and "hiding" the device if nothing is currently connected.
I agree in principle with that you should be given some warning if you try to play back to unconnected things, but for me it's not a high priority to improve. E g, the situation is the same for analog audio - imagine a small desktop with only a headphone output: you can still play audio to the headphone output even when there's no headphone connected in the jack.
- In PulseAudio's current design, PulseAudio probes the device at
startup and caches the result. Unfortunately, there is no reprobing at plug/unplug, so if the monitor is hotplugged, it will not work unless PulseAudio is restarted afterwards.
For haswell ult board only, DDI port D is not supported, this results in
screen flicker when playing on third pin.
But for other haswell boards, the DDI port D is normal. So at first glance my idea is to disable opening device without physical device
connecting.
Maybe it's possible to fix on the gfx side? It sounds like a graphic driver bug if the screen flickers due to anything happening on the audio side.
Thanks --xingchao
The bug is from Haswell platform, All pins choose converter 0 by default in hardware level, maybe only pin 1 had valid monitor connected. if user play audio on pin 0/2, pin 1 can get audio data too.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
sound/pci/hda/patch_hdmi.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 8db5eb6..d766f40 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1139,6 +1139,9 @@ static int hdmi_pcm_open(struct
hda_pcm_stream *hinfo,
per_pin = get_pin(spec, pin_idx); eld = &per_pin->sink_eld;
- if (!eld->monitor_present || !eld->eld_valid)
return -EIO;- if (codec->vendor_id == 0x80862807) hsw_verify_cvt_D0(spec, codec);
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
ALSA audio driver need know current audio routing infomation. i.e. Route map between codec pins(DDI ports) and Transcoders(Pipe).
Also the new API let audio driver disable unused audio pin's output. This fixed the bug when three pins *ALL* have monitors connected, playing audio on one pin would cause audio output to all minitors.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com --- drivers/gpu/drm/i915/i915_drv.h | 18 +++++ drivers/gpu/drm/i915/intel_ddi.c | 131 +++++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_display.c | 7 +- drivers/gpu/drm/i915/intel_drv.h | 1 + include/drm/i915_powerwell.h | 5 ++ 5 files changed, 157 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 10a56c9..8248048 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -87,6 +87,7 @@ enum port { I915_MAX_PORTS }; #define port_name(p) ((p) + 'A') +#define pin2port(p) ((p) + PORT_B)
enum intel_display_power_domain { POWER_DOMAIN_PIPE_A, @@ -785,6 +786,20 @@ struct i915_power_well { int i915_request; };
+#define DEFAULT_PIPE -1 +/* Dp1.2 mode, one DDI port can choose multiple pipes */ +struct dp12_port { + int pipes[I915_MAX_PIPES]; + int count; +}; + +/* audio routing info for haswell */ +struct i915_audio { + /* route map between pipe and DDI port */ + struct dp12_port active_pipes[I915_MAX_PORTS]; + int pin_eld; +}; + struct i915_dri1_state { unsigned allow_batchbuffer : 1; u32 __iomem *gfx_hws_cpu_addr; @@ -1147,6 +1162,9 @@ typedef struct drm_i915_private { /* Haswell power well */ struct i915_power_well power_well;
+ /* Haswell audio routing */ + struct i915_audio audio_route; + enum no_fbc_reason no_fbc_reason;
struct drm_mm_node *compressed_fb; diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 224ce25..82823b8 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -286,8 +286,11 @@ static void intel_ddi_mode_set(struct drm_encoder *encoder, struct drm_display_mode *adjusted_mode) { struct drm_crtc *crtc = encoder->crtc; + struct drm_i915_private *dev_priv = crtc->dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_encoder *intel_encoder = to_intel_encoder(encoder); + struct i915_audio *hsw_audio = &dev_priv->audio_route; + struct dp12_port *hsw_port; int port = intel_ddi_get_encoder_port(intel_encoder); int pipe = intel_crtc->pipe; int type = intel_encoder->type; @@ -296,6 +299,12 @@ static void intel_ddi_mode_set(struct drm_encoder *encoder, port_name(port), pipe_name(pipe));
intel_crtc->eld_vld = false; + + /* store pipe routing info */ + hsw_port = &hsw_audio->active_pipes[port]; + hsw_port->pipes[0] = pipe; + hsw_port->count = 1; + if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) { struct intel_dp *intel_dp = enc_to_intel_dp(encoder); struct intel_digital_port *intel_dig_port = @@ -306,8 +315,8 @@ static void intel_ddi_mode_set(struct drm_encoder *encoder, intel_dp->DP |= DDI_PORT_WIDTH(intel_dp->lane_count);
if (intel_dp->has_audio) { - DRM_DEBUG_DRIVER("DP audio on pipe %c on DDI\n", - pipe_name(intel_crtc->pipe)); + DRM_DEBUG_DRIVER("DP audio on pipe %c on DDI %c\n", + pipe_name(intel_crtc->pipe), port_name(port));
/* write eld */ DRM_DEBUG_DRIVER("DP audio: write eld information\n"); @@ -324,8 +333,8 @@ static void intel_ddi_mode_set(struct drm_encoder *encoder, * and a new set of registers, so we leave it for future * patch bombing. */ - DRM_DEBUG_DRIVER("HDMI audio on pipe %c on DDI\n", - pipe_name(intel_crtc->pipe)); + DRM_DEBUG_DRIVER("HDMI audio on pipe %c on DDI %c \n", + pipe_name(intel_crtc->pipe), port_name(port));
/* write eld */ DRM_DEBUG_DRIVER("HDMI audio: write eld information\n"); @@ -1135,6 +1144,9 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) int type = intel_encoder->type; struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private; + struct i915_audio *hsw_audio = &dev_priv->audio_route; + struct dp12_port *hsw_port; + enum port port = intel_ddi_get_encoder_port(intel_encoder); uint32_t tmp;
if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) { @@ -1149,6 +1161,11 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
ironlake_edp_backlight_off(intel_dp); } + + /* clear pipe routing info */ + hsw_port = &hsw_audio->active_pipes[port]; + hsw_port->pipes[0] = 0; + hsw_port->count = 0; }
int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv) @@ -1312,6 +1329,24 @@ static const struct drm_encoder_helper_funcs intel_ddi_helper_funcs = { .mode_set = intel_ddi_mode_set, };
+static struct drm_i915_private *hdmi_dev_priv; +void intel_ddi_audio_init(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct i915_audio *hsw_audio = &dev_priv->audio_route; + struct dp12_port *hsw_port; + enum port port; + + for (port = PORT_A; port < I915_MAX_PORTS; port++) { + hsw_port = &hsw_audio->active_pipes[port]; + hsw_port->count = 0; + memset(hsw_port->pipes, DEFAULT_PIPE, sizeof(hsw_port->pipes)); + } + + hsw_audio->pin_eld = 0; + hdmi_dev_priv = dev_priv; +} + void intel_ddi_init(struct drm_device *dev, enum port port) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -1369,3 +1404,91 @@ void intel_ddi_init(struct drm_device *dev, enum port port) intel_hdmi_init_connector(intel_dig_port, hdmi_connector); } } + +int i915_using_pipe(int pin) +{ + enum port port; + int pipe; + struct i915_audio *hsw_audio; + struct dp12_port *hsw_port; + + if (!hdmi_dev_priv) + return -EINVAL; + + hsw_audio = &hdmi_dev_priv->audio_route; + + port = pin2port(pin); + hsw_port = &hsw_audio->active_pipes[port]; + + /* only first element is valid for non Dp1.2 */ + pipe = hsw_port->pipes[0]; + + if (hsw_port->count) + DRM_DEBUG_DRIVER("HDMI: i915 is using pipe %c for pin %d now\n", pipe_name(pipe), pin); + + return pipe; +} +EXPORT_SYMBOL_GPL(i915_using_pipe); + +int i915_disable_pipe(int pipe, int *busy_pins) +{ + struct drm_i915_private *dev_priv = hdmi_dev_priv; + struct i915_audio *hsw_audio; + struct dp12_port *hsw_port; + int aud_cntrl_st2 = HSW_AUD_PIN_ELD_CP_VLD; + int port, active_pipe; + int tmp; + int pin; + + if (!hdmi_dev_priv) + return -EINVAL; + + hsw_audio = &dev_priv->audio_route; + + if (pipe == DEFAULT_PIPE) { + DRM_DEBUG_DRIVER("HDMI: disable all active pipes\n"); + } + + tmp = I915_READ(aud_cntrl_st2); + DRM_DEBUG_DRIVER("HDMI: current pin eld 0x%x\n", tmp); + /* Clear all output enable bits */ + tmp &= ~(AUDIO_OUTPUT_ENABLE_A | + AUDIO_OUTPUT_ENABLE_B | + AUDIO_OUTPUT_ENABLE_C); + + if (pipe != DEFAULT_PIPE) { + for (pin = 0; pin < 3; pin++) { + port = pin2port(pin); + hsw_port = &hsw_audio->active_pipes[port]; + active_pipe = hsw_port->pipes[0]; + + if (busy_pins[pin] && active_pipe != DEFAULT_PIPE) { + tmp |= (AUDIO_OUTPUT_ENABLE_A << (active_pipe * 4)); + DRM_DEBUG_DRIVER("pin %d is busy now, keep its using pipe %c\n", pin, pipe_name(active_pipe)); + } + } + } + + I915_WRITE(aud_cntrl_st2, tmp); + DRM_DEBUG_DRIVER("HDMI: new pin eld 0x%x -- disable unuesd pin's audio output\n", tmp); + + return 0; +} +EXPORT_SYMBOL_GPL(i915_disable_pipe); + +int i915_restore_pineld(void) +{ + struct drm_i915_private *dev_priv = hdmi_dev_priv; + struct i915_audio *hsw_audio; + int aud_cntrl_st2 = HSW_AUD_PIN_ELD_CP_VLD; + + if (!hdmi_dev_priv) + return -EINVAL; + + hsw_audio = &dev_priv->audio_route; + + I915_WRITE(aud_cntrl_st2, hsw_audio->pin_eld); + DRM_DEBUG_DRIVER("HDMI: restore pin eld 0x%x\n", hsw_audio->pin_eld); + return 0; +} +EXPORT_SYMBOL_GPL(i915_restore_pineld); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b23937b..1080750 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6121,6 +6121,7 @@ static void haswell_write_eld(struct drm_connector *connector, uint8_t *eld = connector->eld; struct drm_device *dev = crtc->dev; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct i915_audio *hsw_audio = &dev_priv->audio_route; uint32_t eldv; uint32_t i; int len; @@ -6137,6 +6138,7 @@ static void haswell_write_eld(struct drm_connector *connector,
/* Audio output enable */ DRM_DEBUG_DRIVER("HDMI audio: enable codec\n"); + tmp = I915_READ(aud_cntrl_st2); tmp |= (AUDIO_OUTPUT_ENABLE_A << (pipe * 4)); I915_WRITE(aud_cntrl_st2, tmp); @@ -6171,6 +6173,7 @@ static void haswell_write_eld(struct drm_connector *connector, } else I915_WRITE(aud_config, 0);
+ hsw_audio->pin_eld = I915_READ(aud_cntrl_st2); if (intel_eld_uptodate(connector, aud_cntrl_st2, eldv, aud_cntl_st, IBX_ELD_ADDRESS, @@ -6198,7 +6201,6 @@ static void haswell_write_eld(struct drm_connector *connector, i = I915_READ(aud_cntrl_st2); i |= eldv; I915_WRITE(aud_cntrl_st2, i); - }
static void ironlake_write_eld(struct drm_connector *connector, @@ -8969,6 +8971,9 @@ static void intel_setup_outputs(struct drm_device *dev) if (HAS_DDI(dev)) { int found;
+ /* Init Haswell audio route map */ + intel_ddi_audio_init(dev); + /* Haswell uses DDI functions to detect digital outputs */ found = I915_READ(DDI_BUF_CTL_A) & DDI_INIT_DISPLAY_DETECTED; /* DDI A only supports eDP */ diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index ffe9d35..6f8e680 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -758,6 +758,7 @@ extern void intel_write_eld(struct drm_encoder *encoder, extern void intel_prepare_ddi(struct drm_device *dev); extern void hsw_fdi_link_train(struct drm_crtc *crtc); extern void intel_ddi_init(struct drm_device *dev, enum port port); +extern void intel_ddi_audio_init(struct drm_device *dev);
/* For use by IVB LP watermark workaround in intel_sprite.c */ extern void intel_update_watermarks(struct drm_device *dev); diff --git a/include/drm/i915_powerwell.h b/include/drm/i915_powerwell.h index cfdc884..aef3ec0 100644 --- a/include/drm/i915_powerwell.h +++ b/include/drm/i915_powerwell.h @@ -33,4 +33,9 @@ extern void i915_request_power_well(void); extern void i915_release_power_well(void);
+/* used by hdmi driver for audio routing */ +extern int i915_using_pipe(int pin); +extern int i915_disable_pipe(int pipe, int *busy_pins); +extern int i915_restore_pineld(void); + #endif /* _I915_POWERWELL_H_ */
On Fri, Jun 14, 2013 at 5:20 PM, Wang Xingchao xingchao.wang@linux.intel.com wrote:
ALSA audio driver need know current audio routing infomation. i.e. Route map between codec pins(DDI ports) and Transcoders(Pipe).
Also the new API let audio driver disable unused audio pin's output. This fixed the bug when three pins *ALL* have monitors connected, playing audio on one pin would cause audio output to all minitors.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
So I've started to have a real look at audio on haswell and audio on i915 in general, and I'm seriously confused. Random observations, but I fear this isn't the real story by far yet:
- On haswell we now have a hooping 3 places that set up these audio routing register: haswell_write_eld (called from the connector enable callbacks), intel_enable/disable_ddi and now the new hooks you're adding here. That's 2 places too many. If we really need all three places those need to be refactored so that the bit-frobbing logic is all in one place. But I seriously doubt that we need them all.
- I've quickly read through the haswell audio modeset sequence. On a glance I could see no reason why we need to have 3 different places to set up the gfx side of the audio support, since it's much simpler apparently:
Enable sequence: 1. gfx side sets up registers 2. gfx side sets the audio enable bit, which generates the interrup 3. audio side completes the setup on its side.
Disable sequence is just the inverse. So I don't think we need 3 different places for this.
- Both on ibx/cpt and also on haswell here we seem to rely on BIOS preset values way too often. Or at least I didn't figure out where these registes get initialized (pretty sure nowhere in i915.ko). We have countless bugs where the BIOS tried to be fancy and set up something we don't actually support. So please, if there's no really good reason why we need to do things differently, have explicit register setups. If there's something we need to preserve from the BIOS, it needs to be done explicitly and must have a comment.
- No global state or stuffing random things into dev_priv/crtc any more. Our modeset infrastructure has a transactional state machine now: First we compute state parameters in the various compute_config functions and store all that into a struct intel_crtc_config pipe_config. Then the modeset functions apply that state. Finally at the end the hw state is cross-checked with the sw state. We need this to properly support atomic modeset and fastboot. Yes, this means that recent additions for haswell audio support like crtc->eld_vld need to go away and be moved into pipe_config.
- Changing the OUTPUT_ENABLE bits will result in interrupts on the audio side. But these functions here are only called from the audio side, so we have a really complicated feedback loop. Given that your patches need much better explanation of what's going on (preferably with time/state diagrams). Also I think I need a more detailed explanation of what exactly is broken currently on Haswell audio and how these patches fix this.
Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Daniel,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] Sent: Saturday, June 15, 2013 3:18 AM To: Wang Xingchao Cc: Takashi Iwai; alsa-devel@alsa-project.org; intel-gfx; David Henningsson; Wang, Xingchao Subject: Re: [PATCH 3/4] drm/i915: Add display audio routing APIs for ALSA
On Fri, Jun 14, 2013 at 5:20 PM, Wang Xingchao xingchao.wang@linux.intel.com wrote:
ALSA audio driver need know current audio routing infomation. i.e. Route map between codec pins(DDI ports) and Transcoders(Pipe).
Also the new API let audio driver disable unused audio pin's output. This fixed the bug when three pins *ALL* have monitors connected, playing audio on one pin would cause audio output to all minitors.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
So I've started to have a real look at audio on haswell and audio on i915 in general, and I'm seriously confused. Random observations, but I fear this isn't the real story by far yet:
- On haswell we now have a hooping 3 places that set up these audio routing
register: haswell_write_eld (called from the connector enable callbacks), intel_enable/disable_ddi and now the new hooks you're adding here. That's 2 places too many. If we really need all three places those need to be refactored so that the bit-frobbing logic is all in one place. But I seriously doubt that we need them all.
Yeah I agree to put audio stuff in One place. Let me give you some background about the issue this Patch fixed. Basically audio side need such API in gfx side to enable/disable audio, that's three PIPE based audio enable bits. The issue comes when two monitors connected on DDI port B and port C, like: DDI B(pin 0) --> DP monitor DDI C(pin 1) --> HDMI minitor
In haswell the pins default choose converter 0(hardware level) as data source, so it's like:
Converter 0 --> pin 0 + pin 1 + pin2
And when play audio on pin 0, pin 1 could get audio data too. Meanwhile pin 1 has HDMi minitor connected, you can hear audio. To fix this issue, I tried several solutions: 1) Disable pin 1/2 when play audio on pin 0, or disable pin 0/2 when play audio on pin 1 2) mute pin 1/2 when play audio on pin 0, or mute pin 0/2 when play audio on pin 1 3) configure pin 1/2 to choose other converters when play audio on pin 0. 4) disable audio output enable in gfx side, this is implemented in current patchset.
I prefer 1) or 2) it's very simple, but it doesnot work on Haswell. It's the issue I'm trying to fix: Pin 0 must be output-eanbe/unmute when playing audio on pin 1. Seems pin 1 has dependency on pin 0, and I have to disable audio output in gfx side.
I don't think it's bad if we implement such API in gfx side, the question is you point out, to make it clean and simple. If Dp1.2 is needed in future, audio side need the pipe->DDI port connections map too. We can reuse power-well module to export such information.
To implement solution 4) above, audio side handle request to disable audio output in gfx side: - if only pin 0 is busy playing audio and using valid pipe, disable audio output for pin 1/2. It's the same logic when only pin1 or pin 2 is busy playing audio. - if both pin0 and pin 1 are busy playing audio, only disable pin 2. - if all pins are playing audio, do nothing.
In above case when both Pin 0 and pin 1 are connected with Dp and HDMI monitor, if only playing audio on pin 0, disable pin 1's audio output in gfx side Would cause eld info refresh in audio driver side, but that doesnot matter as when you need playing audio on pin 1, the audio output enable bit would be set again.
The patch could fix the issue when playing audio on one pin, audio would route to another pin too. In such case, we can drop the second patch in fact.
- I've quickly read through the haswell audio modeset sequence. On a glance I
could see no reason why we need to have 3 different places to set up the gfx side of the audio support, since it's much simpler apparently:
Enable sequence: 1. gfx side sets up registers 2. gfx side sets the audio enable bit, which generates the interrup 3. audio side completes the setup on its side.
Disable sequence is just the inverse. So I don't think we need 3 different places for this.
- Both on ibx/cpt and also on haswell here we seem to rely on BIOS preset
values way too often. Or at least I didn't figure out where these registes get initialized (pretty sure nowhere in i915.ko). We have countless bugs where the BIOS tried to be fancy and set up something we don't actually support. So please, if there's no really good reason why we need to do things differently, have explicit register setups. If there's something we need to preserve from the BIOS, it needs to be done explicitly and must have a comment.
- No global state or stuffing random things into dev_priv/crtc any more. Our
modeset infrastructure has a transactional state machine now: First we compute state parameters in the various compute_config functions and store all that into a struct intel_crtc_config pipe_config. Then the modeset functions apply that state. Finally at the end the hw state is cross-checked with the sw state. We need this to properly support atomic modeset and fastboot. Yes, this means that recent additions for haswell audio support like crtc->eld_vld need to go away and be moved into pipe_config.
- Changing the OUTPUT_ENABLE bits will result in interrupts on the audio side.
But these functions here are only called from the audio side, so we have a really complicated feedback loop. Given that your patches need much better explanation of what's going on (preferably with time/state diagrams). Also I think I need a more detailed explanation of what exactly is broken currently on Haswell audio and how these patches fix this.
Hope above explanation is helpful for you, if it's not enough, I would draw some graph case by case to let you know. :)
Thanks --xingchao
Cheers, Daniel
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Jun 17, 2013 at 12:52:41PM +0000, Wang, Xingchao wrote:
Hi Daniel,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] Sent: Saturday, June 15, 2013 3:18 AM To: Wang Xingchao Cc: Takashi Iwai; alsa-devel@alsa-project.org; intel-gfx; David Henningsson; Wang, Xingchao Subject: Re: [PATCH 3/4] drm/i915: Add display audio routing APIs for ALSA
On Fri, Jun 14, 2013 at 5:20 PM, Wang Xingchao xingchao.wang@linux.intel.com wrote:
ALSA audio driver need know current audio routing infomation. i.e. Route map between codec pins(DDI ports) and Transcoders(Pipe).
Also the new API let audio driver disable unused audio pin's output. This fixed the bug when three pins *ALL* have monitors connected, playing audio on one pin would cause audio output to all minitors.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
So I've started to have a real look at audio on haswell and audio on i915 in general, and I'm seriously confused. Random observations, but I fear this isn't the real story by far yet:
- On haswell we now have a hooping 3 places that set up these audio routing
register: haswell_write_eld (called from the connector enable callbacks), intel_enable/disable_ddi and now the new hooks you're adding here. That's 2 places too many. If we really need all three places those need to be refactored so that the bit-frobbing logic is all in one place. But I seriously doubt that we need them all.
Yeah I agree to put audio stuff in One place. Let me give you some background about the issue this Patch fixed. Basically audio side need such API in gfx side to enable/disable audio, that's three PIPE based audio enable bits. The issue comes when two monitors connected on DDI port B and port C, like: DDI B(pin 0) --> DP monitor DDI C(pin 1) --> HDMI minitor
In haswell the pins default choose converter 0(hardware level) as data source, so it's like:
Converter 0 --> pin 0 + pin 1 + pin2
And when play audio on pin 0, pin 1 could get audio data too. Meanwhile pin 1 has HDMi minitor connected, you can hear audio. To fix this issue, I tried several solutions:
- Disable pin 1/2 when play audio on pin 0, or disable pin 0/2 when play audio on pin 1
- mute pin 1/2 when play audio on pin 0, or mute pin 0/2 when play audio on pin 1
- configure pin 1/2 to choose other converters when play audio on pin 0.
- disable audio output enable in gfx side, this is implemented in current patchset.
I prefer 1) or 2) it's very simple, but it doesnot work on Haswell. It's the issue I'm trying to fix: Pin 0 must be output-eanbe/unmute when playing audio on pin 1. Seems pin 1 has dependency on pin 0, and I have to disable audio output in gfx side.
I don't think it's bad if we implement such API in gfx side, the question is you point out, to make it clean and simple. If Dp1.2 is needed in future, audio side need the pipe->DDI port connections map too. We can reuse power-well module to export such information.
To implement solution 4) above, audio side handle request to disable audio output in gfx side:
- if only pin 0 is busy playing audio and using valid pipe, disable audio output for pin 1/2.
It's the same logic when only pin1 or pin 2 is busy playing audio.
- if both pin0 and pin 1 are busy playing audio, only disable pin 2.
- if all pins are playing audio, do nothing.
In above case when both Pin 0 and pin 1 are connected with Dp and HDMI monitor, if only playing audio on pin 0, disable pin 1's audio output in gfx side Would cause eld info refresh in audio driver side, but that doesnot matter as when you need playing audio on pin 1, the audio output enable bit would be set again.
The patch could fix the issue when playing audio on one pin, audio would route to another pin too. In such case, we can drop the second patch in fact.
Just reading through your description I prefer option 3) since that should be possible to implement in the audio side only.
To reiterate why I don't really like 4) is that touching these bits will result in unsolicted even interrupts on the audio side. So your patch doesn't seem to just disable/enable audio, but there's a big chain of follow-up events going on. So I'm afraid that there's some really subtile dependency in there making your current solution fragile.
So what's the downside of option 3?
Yours, Daniel
- I've quickly read through the haswell audio modeset sequence. On a glance I
could see no reason why we need to have 3 different places to set up the gfx side of the audio support, since it's much simpler apparently:
Enable sequence: 1. gfx side sets up registers 2. gfx side sets the audio enable bit, which generates the interrup 3. audio side completes the setup on its side.
Disable sequence is just the inverse. So I don't think we need 3 different places for this.
- Both on ibx/cpt and also on haswell here we seem to rely on BIOS preset
values way too often. Or at least I didn't figure out where these registes get initialized (pretty sure nowhere in i915.ko). We have countless bugs where the BIOS tried to be fancy and set up something we don't actually support. So please, if there's no really good reason why we need to do things differently, have explicit register setups. If there's something we need to preserve from the BIOS, it needs to be done explicitly and must have a comment.
- No global state or stuffing random things into dev_priv/crtc any more. Our
modeset infrastructure has a transactional state machine now: First we compute state parameters in the various compute_config functions and store all that into a struct intel_crtc_config pipe_config. Then the modeset functions apply that state. Finally at the end the hw state is cross-checked with the sw state. We need this to properly support atomic modeset and fastboot. Yes, this means that recent additions for haswell audio support like crtc->eld_vld need to go away and be moved into pipe_config.
- Changing the OUTPUT_ENABLE bits will result in interrupts on the audio side.
But these functions here are only called from the audio side, so we have a really complicated feedback loop. Given that your patches need much better explanation of what's going on (preferably with time/state diagrams). Also I think I need a more detailed explanation of what exactly is broken currently on Haswell audio and how these patches fix this.
Hope above explanation is helpful for you, if it's not enough, I would draw some graph case by case to let you know. :)
Thanks --xingchao
Cheers, Daniel
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Daniel,
-----Original Message----- From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Tuesday, June 18, 2013 3:13 PM To: Wang, Xingchao Cc: Daniel Vetter; Wang Xingchao; Takashi Iwai; alsa-devel@alsa-project.org; intel-gfx; David Henningsson Subject: Re: [PATCH 3/4] drm/i915: Add display audio routing APIs for ALSA
On Mon, Jun 17, 2013 at 12:52:41PM +0000, Wang, Xingchao wrote:
Hi Daniel,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] Sent: Saturday, June 15, 2013 3:18 AM To: Wang Xingchao Cc: Takashi Iwai; alsa-devel@alsa-project.org; intel-gfx; David Henningsson; Wang, Xingchao Subject: Re: [PATCH 3/4] drm/i915: Add display audio routing APIs for ALSA
On Fri, Jun 14, 2013 at 5:20 PM, Wang Xingchao xingchao.wang@linux.intel.com wrote:
ALSA audio driver need know current audio routing infomation. i.e. Route map between codec pins(DDI ports) and Transcoders(Pipe).
Also the new API let audio driver disable unused audio pin's output. This fixed the bug when three pins *ALL* have monitors connected, playing audio on one pin would cause audio output to all minitors.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
So I've started to have a real look at audio on haswell and audio on i915 in general, and I'm seriously confused. Random observations, but I fear this isn't the real story by far yet:
- On haswell we now have a hooping 3 places that set up these audio
routing register: haswell_write_eld (called from the connector enable callbacks), intel_enable/disable_ddi and now the new hooks you're adding here. That's 2 places too many. If we really need all three places those need to be refactored so that the bit-frobbing logic is all in one place. But I seriously doubt that we need them all.
Yeah I agree to put audio stuff in One place. Let me give you some background about the issue this Patch fixed. Basically audio side need such API in gfx side to enable/disable audio, that's three PIPE based audio enable bits. The issue comes when two monitors connected on DDI port B and port C, like: DDI B(pin 0) --> DP monitor DDI C(pin 1) --> HDMI minitor
In haswell the pins default choose converter 0(hardware level) as data source,
so it's like:
Converter 0 --> pin 0 + pin 1 + pin2
And when play audio on pin 0, pin 1 could get audio data too. Meanwhile pin 1 has HDMi minitor connected, you can hear audio. To fix this issue, I tried several solutions:
- Disable pin 1/2 when play audio on pin 0, or disable pin 0/2 when
play audio on pin 1 2) mute pin 1/2 when play audio on pin 0, or mute pin 0/2 when play audio on pin 1 3) configure pin 1/2 to choose other converters when play audio on pin 0. 4) disable audio output enable in gfx side, this is implemented in current
patchset.
I prefer 1) or 2) it's very simple, but it doesnot work on Haswell. It's the issue I'm trying to fix: Pin 0 must be output-eanbe/unmute when playing audio on pin 1. Seems pin 1 has dependency on pin 0, and I have to disable audio output in gfx side.
I don't think it's bad if we implement such API in gfx side, the question is you point out, to make it clean and simple. If Dp1.2 is needed in future, audio side need the pipe->DDI port connections map too. We can reuse power-well module to export such information.
To implement solution 4) above, audio side handle request to disable audio
output in gfx side:
- if only pin 0 is busy playing audio and using valid pipe, disable audio output
for pin 1/2.
It's the same logic when only pin1 or pin 2 is busy playing audio.
- if both pin0 and pin 1 are busy playing audio, only disable pin 2.
- if all pins are playing audio, do nothing.
In above case when both Pin 0 and pin 1 are connected with Dp and HDMI monitor, if only playing audio on pin 0, disable pin 1's audio output in gfx side Would cause eld info refresh in audio driver side, but that doesnot matter as when you need playing audio on pin 1, the audio output enable bit would be set again.
The patch could fix the issue when playing audio on one pin, audio would route to another pin too. In such case, we can drop the second patch in fact.
Just reading through your description I prefer option 3) since that should be possible to implement in the audio side only.
To reiterate why I don't really like 4) is that touching these bits will result in unsolicted even interrupts on the audio side. So your patch doesn't seem to just disable/enable audio, but there's a big chain of follow-up events going on. So I'm afraid that there's some really subtile dependency in there making your current solution fragile.
So what's the downside of option 3?
I've rework the patch and sent for Takashi's review. The new patch only has changes in audio side.
Thanks --xingchao
Yours, Daniel
- I've quickly read through the haswell audio modeset sequence. On a
glance I could see no reason why we need to have 3 different places to set up the gfx side of the audio support, since it's much simpler apparently:
Enable sequence: 1. gfx side sets up registers 2. gfx side sets the audio enable bit, which generates the interrup 3. audio side completes the
setup on its side.
Disable sequence is just the inverse. So I don't think we need 3 different places for this.
- Both on ibx/cpt and also on haswell here we seem to rely on BIOS
preset values way too often. Or at least I didn't figure out where these registes get initialized (pretty sure nowhere in i915.ko). We have countless bugs where the BIOS tried to be fancy and set up something we don't actually support. So please, if there's no really good reason why we need to do things differently, have explicit register setups. If there's something we need to preserve from the BIOS, it
needs to be done explicitly and must have a comment.
- No global state or stuffing random things into dev_priv/crtc any
more. Our modeset infrastructure has a transactional state machine now: First we compute state parameters in the various compute_config functions and store all that into a struct intel_crtc_config pipe_config. Then the modeset functions apply that state. Finally at the end the hw state is cross-checked with the sw state. We need this to properly support atomic modeset and fastboot. Yes, this means that recent additions for haswell audio support like crtc->eld_vld
need to go away and be moved into pipe_config.
- Changing the OUTPUT_ENABLE bits will result in interrupts on the audio
side.
But these functions here are only called from the audio side, so we have a really complicated feedback loop. Given that your patches need much better explanation of what's going on (preferably with time/state diagrams). Also I think I need a more detailed explanation of what exactly is broken currently on Haswell audio and how
these patches fix this.
Hope above explanation is helpful for you, if it's not enough, I would draw some graph case by case to let you know. :)
Thanks --xingchao
Cheers, Daniel
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
ALSA side use these apis to know display audio routing map in gfx side. And use the API to disable unused pin's audio output.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com --- sound/pci/hda/hda_i915.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++ sound/pci/hda/hda_i915.h | 4 +++ sound/pci/hda/patch_hdmi.c | 20 +++++++++-- 3 files changed, 104 insertions(+), 3 deletions(-)
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index 76c13d5..7ac446f 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -22,9 +22,73 @@ #include <drm/i915_powerwell.h> #include "hda_i915.h"
+/* Haswell power well */ static void (*get_power)(void); static void (*put_power)(void);
+/* Haswell audio routing */ +static int (*get_using_pipe)(int); +static int (*disable_unused_pipe)(int, int *); +static int (*restore_eld)(void); + +#define i915_pipe_name(p) ((p) + 'A') + +static int busy_pins[3] = {0, 0, 0}; + +int hdmi_disable_unused_pipe(int pin_idx, int pipe_idx) +{ + busy_pins[pin_idx] = 1; + if (disable_unused_pipe) + disable_unused_pipe(pipe_idx, busy_pins); + + return 0; +} +EXPORT_SYMBOL(hdmi_disable_unused_pipe); + +void hdmi_restore_pineld(int pin_idx) +{ + busy_pins[pin_idx] = 0; + if (restore_eld) + restore_eld(); +} +EXPORT_SYMBOL(hdmi_restore_pineld); + +int hdmi_get_using_pipe(int pin_idx) +{ + int pipe = -1; + + if (get_using_pipe) + pipe = get_using_pipe(pin_idx); + + if (pipe != -1) + snd_printd("HDMI: pin %d get using pipe %c\n", pin_idx, i915_pipe_name(pipe)); + + return pipe; +} +EXPORT_SYMBOL(hdmi_get_using_pipe); + +static int init_audio_routing(void) +{ + get_using_pipe = symbol_request(i915_using_pipe); + if (!get_using_pipe) + return -ENODEV; + + disable_unused_pipe = symbol_request(i915_disable_pipe); + if (!disable_unused_pipe) { + get_using_pipe = NULL; + return -ENODEV; + } + + restore_eld = symbol_request(i915_restore_pineld); + if (!restore_eld) { + restore_eld = NULL; + get_using_pipe = NULL; + return -ENODEV; + } + + return 0; +} + void hda_display_power(bool enable) { if (!get_power || !put_power) @@ -57,6 +121,10 @@ int hda_i915_init(void)
snd_printd("HDA driver get symbol successfully from i915 module\n");
+ err = init_audio_routing(); + if (err < 0) + snd_printd("HDA driver get audior routing APIs failed!\n"); + return err; }
@@ -71,5 +139,20 @@ int hda_i915_exit(void) put_power = NULL; }
+ if (get_using_pipe) { + symbol_put(get_using_pipe); + get_using_pipe = NULL; + } + + if (disable_unused_pipe) { + symbol_put(disable_unused_pipe); + disable_unused_pipe = NULL; + } + + if (restore_eld) { + symbol_put(restore_eld); + restore_eld = NULL; + } + return 0; } diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h index 5a63da2..52d6f09 100644 --- a/sound/pci/hda/hda_i915.h +++ b/sound/pci/hda/hda_i915.h @@ -32,4 +32,8 @@ static inline int hda_i915_exit(void) } #endif
+extern int hdmi_get_using_pipe(int pin_idx); +extern int hdmi_disable_unused_pipe(int pin_idx, int pipe_idx); +extern void hdmi_restore_pineld(int pin_idx); + #endif diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index d766f40..2a1e977 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -39,6 +39,7 @@ #include "hda_codec.h" #include "hda_local.h" #include "hda_jack.h" +#include "hda_i915.h"
static bool static_hdmi_pcm; module_param(static_hdmi_pcm, bool, 0644); @@ -1131,6 +1132,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, struct hdmi_spec_per_pin *per_pin; struct hdmi_eld *eld; struct hdmi_spec_per_cvt *per_cvt = NULL; + int pipe_idx;
/* Validate hinfo */ pin_idx = hinfo_to_pin_index(spec, hinfo); @@ -1139,12 +1141,21 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, per_pin = get_pin(spec, pin_idx); eld = &per_pin->sink_eld;
+ if (codec->vendor_id == 0x80862807) { + hsw_verify_cvt_D0(spec, codec); + + pipe_idx = hdmi_get_using_pipe(pin_idx); + if (pipe_idx < 0) + snd_printdd("HDMI: Pin %d has no valid pipe in use\n", pin_idx); + else { + hdmi_disable_unused_pipe(pin_idx, pipe_idx); + msleep(10); + } + } + if (!eld->monitor_present || !eld->eld_valid) return -EIO;
- if (codec->vendor_id == 0x80862807) - hsw_verify_cvt_D0(spec, codec); - /* Dynamically assign converter to stream */ for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) { per_cvt = get_cvt(spec, cvt_idx); @@ -1514,6 +1525,9 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo, snd_hda_spdif_ctls_unassign(codec, pin_idx); per_pin->chmap_set = false; memset(per_pin->chmap, 0, sizeof(per_pin->chmap)); + + if (codec->vendor_id == 0x80862807) + hdmi_restore_pineld(pin_idx); }
return 0;
At Fri, 14 Jun 2013 23:20:29 +0800, Wang Xingchao wrote:
ALSA side use these apis to know display audio routing map in gfx side. And use the API to disable unused pin's audio output.
Adding more and more such exported functions doesn't look scaling. Better to define an ops struct and export it.
Takashi
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
sound/pci/hda/hda_i915.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++ sound/pci/hda/hda_i915.h | 4 +++ sound/pci/hda/patch_hdmi.c | 20 +++++++++-- 3 files changed, 104 insertions(+), 3 deletions(-)
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index 76c13d5..7ac446f 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -22,9 +22,73 @@ #include <drm/i915_powerwell.h> #include "hda_i915.h"
+/* Haswell power well */ static void (*get_power)(void); static void (*put_power)(void);
+/* Haswell audio routing */ +static int (*get_using_pipe)(int); +static int (*disable_unused_pipe)(int, int *); +static int (*restore_eld)(void);
+#define i915_pipe_name(p) ((p) + 'A')
+static int busy_pins[3] = {0, 0, 0};
+int hdmi_disable_unused_pipe(int pin_idx, int pipe_idx) +{
- busy_pins[pin_idx] = 1;
- if (disable_unused_pipe)
disable_unused_pipe(pipe_idx, busy_pins);- return 0;
+} +EXPORT_SYMBOL(hdmi_disable_unused_pipe);
+void hdmi_restore_pineld(int pin_idx) +{
- busy_pins[pin_idx] = 0;
- if (restore_eld)
restore_eld();+} +EXPORT_SYMBOL(hdmi_restore_pineld);
+int hdmi_get_using_pipe(int pin_idx) +{
- int pipe = -1;
- if (get_using_pipe)
pipe = get_using_pipe(pin_idx);- if (pipe != -1)
snd_printd("HDMI: pin %d get using pipe %c\n", pin_idx, i915_pipe_name(pipe));- return pipe;
+} +EXPORT_SYMBOL(hdmi_get_using_pipe);
+static int init_audio_routing(void) +{
- get_using_pipe = symbol_request(i915_using_pipe);
- if (!get_using_pipe)
return -ENODEV;- disable_unused_pipe = symbol_request(i915_disable_pipe);
- if (!disable_unused_pipe) {
get_using_pipe = NULL;return -ENODEV;- }
- restore_eld = symbol_request(i915_restore_pineld);
- if (!restore_eld) {
restore_eld = NULL;get_using_pipe = NULL;return -ENODEV;- }
- return 0;
+}
void hda_display_power(bool enable) { if (!get_power || !put_power) @@ -57,6 +121,10 @@ int hda_i915_init(void)
snd_printd("HDA driver get symbol successfully from i915 module\n");
- err = init_audio_routing();
- if (err < 0)
snd_printd("HDA driver get audior routing APIs failed!\n");- return err;
}
@@ -71,5 +139,20 @@ int hda_i915_exit(void) put_power = NULL; }
- if (get_using_pipe) {
symbol_put(get_using_pipe);get_using_pipe = NULL;- }
- if (disable_unused_pipe) {
symbol_put(disable_unused_pipe);disable_unused_pipe = NULL;- }
- if (restore_eld) {
symbol_put(restore_eld);restore_eld = NULL;- }
- return 0;
} diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h index 5a63da2..52d6f09 100644 --- a/sound/pci/hda/hda_i915.h +++ b/sound/pci/hda/hda_i915.h @@ -32,4 +32,8 @@ static inline int hda_i915_exit(void) } #endif
+extern int hdmi_get_using_pipe(int pin_idx); +extern int hdmi_disable_unused_pipe(int pin_idx, int pipe_idx); +extern void hdmi_restore_pineld(int pin_idx);
#endif diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index d766f40..2a1e977 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -39,6 +39,7 @@ #include "hda_codec.h" #include "hda_local.h" #include "hda_jack.h" +#include "hda_i915.h"
static bool static_hdmi_pcm; module_param(static_hdmi_pcm, bool, 0644); @@ -1131,6 +1132,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, struct hdmi_spec_per_pin *per_pin; struct hdmi_eld *eld; struct hdmi_spec_per_cvt *per_cvt = NULL;
int pipe_idx;
/* Validate hinfo */ pin_idx = hinfo_to_pin_index(spec, hinfo);
@@ -1139,12 +1141,21 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, per_pin = get_pin(spec, pin_idx); eld = &per_pin->sink_eld;
- if (codec->vendor_id == 0x80862807) {
hsw_verify_cvt_D0(spec, codec);pipe_idx = hdmi_get_using_pipe(pin_idx);if (pipe_idx < 0)snd_printdd("HDMI: Pin %d has no valid pipe in use\n", pin_idx);else {hdmi_disable_unused_pipe(pin_idx, pipe_idx);msleep(10);}- }
- if (!eld->monitor_present || !eld->eld_valid) return -EIO;
- if (codec->vendor_id == 0x80862807)
hsw_verify_cvt_D0(spec, codec);- /* Dynamically assign converter to stream */ for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) { per_cvt = get_cvt(spec, cvt_idx);
@@ -1514,6 +1525,9 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo, snd_hda_spdif_ctls_unassign(codec, pin_idx); per_pin->chmap_set = false; memset(per_pin->chmap, 0, sizeof(per_pin->chmap));
if (codec->vendor_id == 0x80862807)hdmi_restore_pineld(pin_idx);}
return 0;
-- 1.8.1.2
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Monday, June 17, 2013 5:04 PM To: Wang Xingchao Cc: daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; david.henningsson@canonical.com; Wang, Xingchao Subject: Re: [PATCH 4/4] ALSA: hda - Add display audio routing API for haswell
At Fri, 14 Jun 2013 23:20:29 +0800, Wang Xingchao wrote:
ALSA side use these apis to know display audio routing map in gfx side. And use the API to disable unused pin's audio output.
Adding more and more such exported functions doesn't look scaling. Better to define an ops struct and export it.
Thanks Takashi, under discussion with Daniel in another thread, need improvement to make the API clean. I will rework the patch when get some agreement.
Thanks --xingchao
Takashi
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
sound/pci/hda/hda_i915.c | 83
++++++++++++++++++++++++++++++++++++++++++++++
sound/pci/hda/hda_i915.h | 4 +++ sound/pci/hda/patch_hdmi.c | 20 +++++++++-- 3 files changed, 104 insertions(+), 3 deletions(-)
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index 76c13d5..7ac446f 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -22,9 +22,73 @@ #include <drm/i915_powerwell.h> #include "hda_i915.h"
+/* Haswell power well */ static void (*get_power)(void); static void (*put_power)(void);
+/* Haswell audio routing */ +static int (*get_using_pipe)(int); +static int (*disable_unused_pipe)(int, int *); static int +(*restore_eld)(void);
+#define i915_pipe_name(p) ((p) + 'A')
+static int busy_pins[3] = {0, 0, 0};
+int hdmi_disable_unused_pipe(int pin_idx, int pipe_idx) {
- busy_pins[pin_idx] = 1;
- if (disable_unused_pipe)
disable_unused_pipe(pipe_idx, busy_pins);- return 0;
+} +EXPORT_SYMBOL(hdmi_disable_unused_pipe);
+void hdmi_restore_pineld(int pin_idx) {
- busy_pins[pin_idx] = 0;
- if (restore_eld)
restore_eld();+} +EXPORT_SYMBOL(hdmi_restore_pineld);
+int hdmi_get_using_pipe(int pin_idx) +{
- int pipe = -1;
- if (get_using_pipe)
pipe = get_using_pipe(pin_idx);- if (pipe != -1)
snd_printd("HDMI: pin %d get using pipe %c\n", pin_idx,+i915_pipe_name(pipe));
- return pipe;
+} +EXPORT_SYMBOL(hdmi_get_using_pipe);
+static int init_audio_routing(void) +{
- get_using_pipe = symbol_request(i915_using_pipe);
- if (!get_using_pipe)
return -ENODEV;- disable_unused_pipe = symbol_request(i915_disable_pipe);
- if (!disable_unused_pipe) {
get_using_pipe = NULL;return -ENODEV;- }
- restore_eld = symbol_request(i915_restore_pineld);
- if (!restore_eld) {
restore_eld = NULL;get_using_pipe = NULL;return -ENODEV;- }
- return 0;
+}
void hda_display_power(bool enable) { if (!get_power || !put_power) @@ -57,6 +121,10 @@ int hda_i915_init(void)
snd_printd("HDA driver get symbol successfully from i915 module\n");
- err = init_audio_routing();
- if (err < 0)
snd_printd("HDA driver get audior routing APIs failed!\n");- return err;
}
@@ -71,5 +139,20 @@ int hda_i915_exit(void) put_power = NULL; }
- if (get_using_pipe) {
symbol_put(get_using_pipe);get_using_pipe = NULL;- }
- if (disable_unused_pipe) {
symbol_put(disable_unused_pipe);disable_unused_pipe = NULL;- }
- if (restore_eld) {
symbol_put(restore_eld);restore_eld = NULL;- }
- return 0;
} diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h index 5a63da2..52d6f09 100644 --- a/sound/pci/hda/hda_i915.h +++ b/sound/pci/hda/hda_i915.h @@ -32,4 +32,8 @@ static inline int hda_i915_exit(void) } #endif
+extern int hdmi_get_using_pipe(int pin_idx); extern int +hdmi_disable_unused_pipe(int pin_idx, int pipe_idx); extern void +hdmi_restore_pineld(int pin_idx);
#endif diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index d766f40..2a1e977 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -39,6 +39,7 @@ #include "hda_codec.h" #include "hda_local.h" #include "hda_jack.h" +#include "hda_i915.h"
static bool static_hdmi_pcm; module_param(static_hdmi_pcm, bool, 0644); @@ -1131,6 +1132,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, struct hdmi_spec_per_pin *per_pin; struct hdmi_eld *eld; struct hdmi_spec_per_cvt *per_cvt = NULL;
int pipe_idx;
/* Validate hinfo */ pin_idx = hinfo_to_pin_index(spec, hinfo); @@ -1139,12 +1141,21 @@
static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, per_pin = get_pin(spec, pin_idx); eld = &per_pin->sink_eld;
- if (codec->vendor_id == 0x80862807) {
hsw_verify_cvt_D0(spec, codec);pipe_idx = hdmi_get_using_pipe(pin_idx);if (pipe_idx < 0)snd_printdd("HDMI: Pin %d has no valid pipe in use\n", pin_idx);else {hdmi_disable_unused_pipe(pin_idx, pipe_idx);msleep(10);}- }
- if (!eld->monitor_present || !eld->eld_valid) return -EIO;
- if (codec->vendor_id == 0x80862807)
hsw_verify_cvt_D0(spec, codec);- /* Dynamically assign converter to stream */ for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) { per_cvt = get_cvt(spec, cvt_idx);
@@ -1514,6 +1525,9 @@ static int hdmi_pcm_close(struct
hda_pcm_stream *hinfo,
snd_hda_spdif_ctls_unassign(codec, pin_idx); per_pin->chmap_set = false; memset(per_pin->chmap, 0, sizeof(per_pin->chmap));
if (codec->vendor_id == 0x80862807)hdmi_restore_pineld(pin_idx);}
return 0;
-- 1.8.1.2
participants (6)
-
Daniel Vetter -
Daniel Vetter -
David Henningsson -
Takashi Iwai -
Wang Xingchao -
Wang, Xingchao