[PATCH 0/4] Haswell Display audio routing bug fix

[PATCH 0/4] Haswell Display audio routing bug fix

[Wang Xingchao]

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(-)

-- 
1.8.1.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/4] ALSA: hda - Haswell converter power state D0 verify

[Wang Xingchao]

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);
-- 
1.8.1.2

Re: [PATCH 1/4] ALSA: hda - Haswell converter power state D0 verify

[Takashi Iwai]

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
> 

Re: [PATCH 1/4] ALSA: hda - Haswell converter power state D0 verify

[Wang, Xingchao]

> -----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
> >

[PATCH 2/4] ALSA: hda - Return error when open empty hdmi device

[Wang Xingchao]

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);
 		
-- 
1.8.1.2

Re: [PATCH 2/4] ALSA: hda - Return error when open empty hdmi device

[David Henningsson]

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);
>   		
>



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH 2/4] ALSA: hda - Return error when open empty hdmi device

[Wang, Xingchao]

> -----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:
> 
>   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.

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.
> 
>   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.

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

Re: [PATCH 2/4] ALSA: hda - Return error when open empty hdmi device

[David Henningsson]

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:
>>
>>    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.
>
> 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.

>>    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.
>
> 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

Re: [PATCH 2/4] ALSA: hda - Return error when open empty hdmi device

[Wang, 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:
> >>
> >>    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.
> >
> > 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.
> >>    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.
> >
> > 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

Re: [PATCH 2/4] ALSA: hda - Return error when open empty hdmi device

[Wang, Xingchao]

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:
> >>
> >>    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.
> >
> > 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.
> 
> >>    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.
> >
> > 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

[PATCH 3/4] drm/i915: Add display audio routing APIs for ALSA

[Wang Xingchao]

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_ */
-- 
1.8.1.2

Re: [PATCH 3/4] drm/i915: Add display audio routing APIs for ALSA

[Daniel Vetter]

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

Re: [PATCH 3/4] drm/i915: Add display audio routing APIs for ALSA

[Wang, Xingchao]

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

Re: [PATCH 3/4] drm/i915: Add display audio routing APIs for ALSA

[Daniel Vetter]

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:
> 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.

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

Re: [PATCH 3/4] drm/i915: Add display audio routing APIs for ALSA

[Wang, Xingchao]

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:
> > 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.
> 
> 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

[PATCH 4/4] ALSA: hda - Add display audio routing API for haswell

[Wang Xingchao]

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;
-- 
1.8.1.2

Re: [PATCH 4/4] ALSA: hda - Add display audio routing API for haswell

[Takashi Iwai]

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
> 

Re: [PATCH 4/4] ALSA: hda - Add display audio routing API for haswell

[Wang, Xingchao]

> -----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
> >