[PATCH] ALSA: HDA: Add jack detection for HDMI

[PATCH] ALSA: HDA: Add jack detection for HDMI

[David Henningsson]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

Just as for headphones and microphone jacks, this patch adds reporting
of HDMI jack status through the input layer.

I considered making additional SND_JACK_* constants for HDMI and 
Displayport instead of going with SND_JACK_VIDEOOUT, but it didn't seem 
worth the additions, and breakage of compiling with old kernels, etc. 
Let me know if you think otherwise and I'll prepare a second patch for 
that.

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

[-- Attachment #2: 0001-ALSA-HDA-Add-jack-detection-for-HDMI.patch --]
[-- Type: text/x-patch, Size: 2297 bytes --]

>From d47239ae3d352b898649d0c15869dedd032ccbd7 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Tue, 17 May 2011 15:39:19 +0200
Subject: [PATCH] ALSA: HDA: Add jack detection for HDMI

Just as for headphones and microphone jacks, this patch adds reporting
of HDMI jack status through the input layer.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/hda_codec.c  |    2 ++
 sound/pci/hda/patch_hdmi.c |   15 +++++++++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index c63f376..8edd998 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -5055,6 +5055,8 @@ static const char *get_jack_default_name(struct hda_codec *codec, hda_nid_t nid,
 		return "Line-out";
 	case SND_JACK_HEADSET:
 		return "Headset";
+	case SND_JACK_VIDEOOUT:
+		return "HDMI/DP";
 	default:
 		return "Misc";
 	}
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 6eb209d..16c1505 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -33,6 +33,7 @@
 #include <linux/slab.h>
 #include <linux/moduleparam.h>
 #include <sound/core.h>
+#include <sound/jack.h>
 #include "hda_codec.h"
 #include "hda_local.h"
 
@@ -720,6 +721,8 @@ static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
 				  &spec->sink_eld[index]);
 		/* TODO: do real things about ELD */
 	}
+
+	snd_hda_input_jack_report(codec, tag);
 }
 
 static void hdmi_non_intrinsic_event(struct hda_codec *codec, unsigned int res)
@@ -919,6 +922,17 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
 		return -E2BIG;
 	}
 
+#ifdef CONFIG_SND_HDA_INPUT_JACK
+	{
+		int err;
+		err = snd_hda_input_jack_add(codec, pin_nid,
+					     SND_JACK_VIDEOOUT, NULL);
+		if (err < 0)
+			return err;
+		snd_hda_input_jack_report(codec, pin_nid);
+	}
+#endif
+
 	hdmi_present_sense(codec, pin_nid, &spec->sink_eld[spec->num_pins]);
 
 	spec->pin[spec->num_pins] = pin_nid;
@@ -1120,6 +1134,7 @@ static void generic_hdmi_free(struct hda_codec *codec)
 
 	for (i = 0; i < spec->num_pins; i++)
 		snd_hda_eld_proc_free(codec, &spec->sink_eld[i]);
+	snd_hda_input_jack_free(codec);
 
 	kfree(spec);
 }
-- 
1.7.4.1


[-- Attachment #3: 0001-hda-emu-update-jack.h-to-match-latest-kernel-version.patch --]
[-- Type: text/x-patch, Size: 887 bytes --]

>From cdc4030a2028efdd1c36f2dc37402ea6971e1b12 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Tue, 17 May 2011 15:43:01 +0200
Subject: [PATCH] hda-emu: update jack.h to match latest kernel version

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 include/sound/jack.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/sound/jack.h b/include/sound/jack.h
index 77247d0..a6a4f21 100644
--- a/include/sound/jack.h
+++ b/include/sound/jack.h
@@ -12,6 +12,9 @@ enum snd_jack_types {
 	SND_JACK_MICROPHONE	= 0x0002,
 	SND_JACK_HEADSET	= SND_JACK_HEADPHONE | SND_JACK_MICROPHONE,
 	SND_JACK_LINEOUT	= 0x0004,
+	SND_JACK_MECHANICAL	= 0x0008, /* If detected separately */
+	SND_JACK_VIDEOOUT	= 0x0010,
+	SND_JACK_AVOUT		= SND_JACK_LINEOUT | SND_JACK_VIDEOOUT,
 };
 
 struct snd_jack {
-- 
1.7.4.1


[-- Attachment #4: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Takashi Iwai]

At Tue, 17 May 2011 15:46:43 +0200,
David Henningsson wrote:
> 
> Just as for headphones and microphone jacks, this patch adds reporting
> of HDMI jack status through the input layer.
> 
> I considered making additional SND_JACK_* constants for HDMI and 
> Displayport instead of going with SND_JACK_VIDEOOUT, but it didn't seem 
> worth the additions, and breakage of compiling with old kernels, etc. 
> Let me know if you think otherwise and I'll prepare a second patch for 
> that.

Did you test it with the actual machine, right?
If it's working, I'm fine to add it.


thanks,

Takashi

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[David Henningsson]

[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]

On 2011-05-17 17:46, Takashi Iwai wrote:
> At Tue, 17 May 2011 15:46:43 +0200,
> David Henningsson wrote:
>>
>> Just as for headphones and microphone jacks, this patch adds reporting
>> of HDMI jack status through the input layer.
>>
>> I considered making additional SND_JACK_* constants for HDMI and
>> Displayport instead of going with SND_JACK_VIDEOOUT, but it didn't seem
>> worth the additions, and breakage of compiling with old kernels, etc.
>> Let me know if you think otherwise and I'll prepare a second patch for
>> that.
>
> Did you test it with the actual machine, right?
> If it's working, I'm fine to add it.

To be honest; it's partially working, or rather it's working in the 
sense that it follows the eld proc file. It's also working in hda-emu.

I've tried it on one Nvidia (with binary drivers), and one Intel 
Graphics and well, and both seem to have the same problem essentially: 
There is no hotplug event coming in (through hdmi_unsol_event) when a 
monitor is removed. But with this patch in perhaps the graphics driver 
writers will feel more motivated to fix it? :-)

Note that the hotplug event is not coming in when you actually plug the 
cable but when you detect displays and/or apply the monitor 
configuration change.

I'm attaching a new version of the patch according to your preference, 
in hopes that it will be in 2.6.40.

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

[-- Attachment #2: 0001-ALSA-HDA-Add-jack-detection-for-HDMI.patch --]
[-- Type: text/x-patch, Size: 2514 bytes --]

>From 0be95828e59250af983c86b3ec49d1ea81d8ec82 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Thu, 19 May 2011 11:46:03 +0200
Subject: [PATCH] ALSA: HDA: Add jack detection for HDMI

Just as for headphones and microphone jacks, this patch adds reporting
of HDMI jack status through the input layer.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/hda_codec.c  |    2 ++
 sound/pci/hda/patch_hdmi.c |   11 +++++++++++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index c63f376..8edd998 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -5055,6 +5055,8 @@ static const char *get_jack_default_name(struct hda_codec *codec, hda_nid_t nid,
 		return "Line-out";
 	case SND_JACK_HEADSET:
 		return "Headset";
+	case SND_JACK_VIDEOOUT:
+		return "HDMI/DP";
 	default:
 		return "Misc";
 	}
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 6eb209d..7348296 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -33,6 +33,7 @@
 #include <linux/slab.h>
 #include <linux/moduleparam.h>
 #include <sound/core.h>
+#include <sound/jack.h>
 #include "hda_codec.h"
 #include "hda_local.h"
 
@@ -720,6 +721,8 @@ static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
 				  &spec->sink_eld[index]);
 		/* TODO: do real things about ELD */
 	}
+
+	snd_hda_input_jack_report(codec, tag);
 }
 
 static void hdmi_non_intrinsic_event(struct hda_codec *codec, unsigned int res)
@@ -912,6 +915,7 @@ static void hdmi_present_sense(struct hda_codec *codec, hda_nid_t pin_nid,
 static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
 {
 	struct hdmi_spec *spec = codec->spec;
+	int err;
 
 	if (spec->num_pins >= MAX_HDMI_PINS) {
 		snd_printk(KERN_WARNING
@@ -919,6 +923,12 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
 		return -E2BIG;
 	}
 
+	err = snd_hda_input_jack_add(codec, pin_nid,
+				     SND_JACK_VIDEOOUT, NULL);
+	if (err < 0)
+		return err;
+	snd_hda_input_jack_report(codec, pin_nid);
+
 	hdmi_present_sense(codec, pin_nid, &spec->sink_eld[spec->num_pins]);
 
 	spec->pin[spec->num_pins] = pin_nid;
@@ -1120,6 +1130,7 @@ static void generic_hdmi_free(struct hda_codec *codec)
 
 	for (i = 0; i < spec->num_pins; i++)
 		snd_hda_eld_proc_free(codec, &spec->sink_eld[i]);
+	snd_hda_input_jack_free(codec);
 
 	kfree(spec);
 }
-- 
1.7.4.1


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Takashi Iwai]

At Thu, 19 May 2011 11:55:21 +0200,
David Henningsson wrote:
> 
> On 2011-05-17 17:46, Takashi Iwai wrote:
> > At Tue, 17 May 2011 15:46:43 +0200,
> > David Henningsson wrote:
> >>
> >> Just as for headphones and microphone jacks, this patch adds reporting
> >> of HDMI jack status through the input layer.
> >>
> >> I considered making additional SND_JACK_* constants for HDMI and
> >> Displayport instead of going with SND_JACK_VIDEOOUT, but it didn't seem
> >> worth the additions, and breakage of compiling with old kernels, etc.
> >> Let me know if you think otherwise and I'll prepare a second patch for
> >> that.
> >
> > Did you test it with the actual machine, right?
> > If it's working, I'm fine to add it.
> 
> To be honest; it's partially working, or rather it's working in the 
> sense that it follows the eld proc file. It's also working in hda-emu.
> 
> I've tried it on one Nvidia (with binary drivers), and one Intel 
> Graphics and well, and both seem to have the same problem essentially: 
> There is no hotplug event coming in (through hdmi_unsol_event) when a 
> monitor is removed. But with this patch in perhaps the graphics driver 
> writers will feel more motivated to fix it? :-)
> 
> Note that the hotplug event is not coming in when you actually plug the 
> cable but when you detect displays and/or apply the monitor 
> configuration change.
> 
> I'm attaching a new version of the patch according to your preference, 
> in hopes that it will be in 2.6.40.

OK, as the jack report itself doesn't play a big role yet so far for
HD-audio, let's get it in, and give pressure to graphics guys :)

The remove event itself is detected in the graphic side (sent as udev
event), so it should be possible to propagate it somehow.  But, I'm
not quite sure whether this is an issue in the video driver code, or
it's a hardware design.


thanks,

Takashi


> 
> -- 
> David Henningsson, Canonical Ltd.
> http://launchpad.net/~diwic
> [2 0001-ALSA-HDA-Add-jack-detection-for-HDMI.patch <text/x-patch (7bit)>]
> >From 0be95828e59250af983c86b3ec49d1ea81d8ec82 Mon Sep 17 00:00:00 2001
> From: David Henningsson <david.henningsson@canonical.com>
> Date: Thu, 19 May 2011 11:46:03 +0200
> Subject: [PATCH] ALSA: HDA: Add jack detection for HDMI
> 
> Just as for headphones and microphone jacks, this patch adds reporting
> of HDMI jack status through the input layer.
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  sound/pci/hda/hda_codec.c  |    2 ++
>  sound/pci/hda/patch_hdmi.c |   11 +++++++++++
>  2 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index c63f376..8edd998 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -5055,6 +5055,8 @@ static const char *get_jack_default_name(struct hda_codec *codec, hda_nid_t nid,
>  		return "Line-out";
>  	case SND_JACK_HEADSET:
>  		return "Headset";
> +	case SND_JACK_VIDEOOUT:
> +		return "HDMI/DP";
>  	default:
>  		return "Misc";
>  	}
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 6eb209d..7348296 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -33,6 +33,7 @@
>  #include <linux/slab.h>
>  #include <linux/moduleparam.h>
>  #include <sound/core.h>
> +#include <sound/jack.h>
>  #include "hda_codec.h"
>  #include "hda_local.h"
>  
> @@ -720,6 +721,8 @@ static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
>  				  &spec->sink_eld[index]);
>  		/* TODO: do real things about ELD */
>  	}
> +
> +	snd_hda_input_jack_report(codec, tag);
>  }
>  
>  static void hdmi_non_intrinsic_event(struct hda_codec *codec, unsigned int res)
> @@ -912,6 +915,7 @@ static void hdmi_present_sense(struct hda_codec *codec, hda_nid_t pin_nid,
>  static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
>  {
>  	struct hdmi_spec *spec = codec->spec;
> +	int err;
>  
>  	if (spec->num_pins >= MAX_HDMI_PINS) {
>  		snd_printk(KERN_WARNING
> @@ -919,6 +923,12 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
>  		return -E2BIG;
>  	}
>  
> +	err = snd_hda_input_jack_add(codec, pin_nid,
> +				     SND_JACK_VIDEOOUT, NULL);
> +	if (err < 0)
> +		return err;
> +	snd_hda_input_jack_report(codec, pin_nid);
> +
>  	hdmi_present_sense(codec, pin_nid, &spec->sink_eld[spec->num_pins]);
>  
>  	spec->pin[spec->num_pins] = pin_nid;
> @@ -1120,6 +1130,7 @@ static void generic_hdmi_free(struct hda_codec *codec)
>  
>  	for (i = 0; i < spec->num_pins; i++)
>  		snd_hda_eld_proc_free(codec, &spec->sink_eld[i]);
> +	snd_hda_input_jack_free(codec);
>  
>  	kfree(spec);
>  }
> -- 
> 1.7.4.1
> 

Setting invalid samplerate

[Torsten Schenk]

Hello all,

I'm working on the driver for the dmx 6fire usb. On my sourceforge page, users have acknowledged that the driver is working fine. I don't know whether the alsa mailing list is the correct one; but since Ubuntu 11.04, I get complaints about the card not working anymore, dmesg says "invalid samplerate 64000 in prepare". I could reproduce the error on my own Ubuntu. I started to wonder why pulseaudio wants to use 64kHz since the card doesn't support it and I also didn't enable the SNDRV_PCM_RATE_64000 flag.

The hardware description is as follows:
	.rates = SNDRV_PCM_RATE_44100 |
		SNDRV_PCM_RATE_48000 |
		SNDRV_PCM_RATE_88200 |
		SNDRV_PCM_RATE_96000 |
		SNDRV_PCM_RATE_176400 |
		SNDRV_PCM_RATE_192000,

	.rate_min = 44100,
	.rate_max = 192000,

I wrote a small test program to see what happens if I try to set a samplerate of 48000. set_rate_near always returned 64kHz. A nasty workaround is to enable just 88.2kHz. But then not all programs run (f.ex. milkytracker refused to playback sound). So what is going on here and what could be done about it?

Thanks,
Torsten

Re: Setting invalid samplerate

[Torsten Schenk]

As addition: I used the small tool I found somewhere in the internet to display all samplerates that alsa allows to be set on the card. The result was:

Device: hw:1 (type: HW)
Access types: MMAP_INTERLEAVED RW_INTERLEAVED
Formats: S24_LE S32_LE
Channels: 1 2 3 4 5 6
Sample rates: 64000 88200 192000
Interrupt interval: 1041-256000 us
Buffer size: 2083-512000 us

Re: Setting invalid samplerate

[Clemens Ladisch]

Torsten Schenk wrote:
> I'm working on the driver for the dmx 6fire usb.  ... 
> since Ubuntu 11.04, I get complaints about the card not working anymore,

I'd guess that would be commit 82d90313980b:
| ALSA: USB: 6fire: signedness bug in usb6fire_pcm_prepare()
| 
| rt->rate is an unsigned char so it's never equal to -1.  It's not a huge
| problem because the invalid rate is caught inside the call to
| usb6fire_pcm_set_rate() which returns -EINVAL.  But if we fix the test
| then it prints out the correct error message so that's good.
| 
| Signed-off-by: Dan Carpenter <error27@gmail.com>
| 
| --- a/sound/usb/6fire/pcm.c
| +++ b/sound/usb/6fire/pcm.c
| @@ -493,13 +493,12 @@ static int usb6fire_pcm_prepare(struct snd_pcm_substream *alsa_sub)
|  	sub->period_off = 0;
|  
|  	if (rt->stream_state == STREAM_DISABLED) {
| -		rt->rate = -1;
|  		for (i = 0; i < ARRAY_SIZE(rates); i++)
|  			if (alsa_rt->rate == rates[i]) {
|  				rt->rate = i;
|  				break;
|  			}
| -		if (rt->rate == -1) {
| +		if (i == ARRAY_SIZE(rates)) {
|  			mutex_unlock(&rt->stream_mutex);
|  			snd_printk("invalid rate %d in prepare.\n",
|  					alsa_rt->rate);

This fixed not a bug but just a symptom of the actual bug, that rt->rate
is not signed.

There are still "rt->rate = -1" initializations, and usb6fire_pcm_open()
then does:

	if (rt->rate >= 0)
		alsa_rt->hw.rates = rates_alsaid[rt->rate];


Regards,
Clemens

Re: Setting invalid samplerate

[Torsten Schenk]

That did the job indeed. I still wonder why it worked in so many cases... However, I will soon create a patch and submit it.

Thanks,
Torsten

---- On Thu, 19 May 2011 12:55:28 +0200 Clemens Ladisch  wrote ---- 

>Torsten Schenk wrote: 
>> I'm working on the driver for the dmx 6fire usb. ... 
>> since Ubuntu 11.04, I get complaints about the card not working anymore, 
> 
>I'd guess that would be commit 82d90313980b: 
>| ALSA: USB: 6fire: signedness bug in usb6fire_pcm_prepare() 
>| 
>| rt->rate is an unsigned char so it's never equal to -1. It's not a huge 
>| problem because the invalid rate is caught inside the call to 
>| usb6fire_pcm_set_rate() which returns -EINVAL. But if we fix the test 
>| then it prints out the correct error message so that's good. 
>| 
>| Signed-off-by: Dan Carpenter  
>| 
>| --- a/sound/usb/6fire/pcm.c 
>| +++ b/sound/usb/6fire/pcm.c 
>| @@ -493,13 +493,12 @@ static int usb6fire_pcm_prepare(struct snd_pcm_substream *alsa_sub) 
>|     sub->period_off = 0; 
>| 
>|     if (rt->stream_state == STREAM_DISABLED) { 
>| -        rt->rate = -1; 
>|         for (i = 0; i < ARRAY_SIZE(rates); i++) 
>|             if (alsa_rt->rate == rates[i]) { 
>|                 rt->rate = i; 
>|                 break; 
>|             } 
>| -        if (rt->rate == -1) { 
>| +        if (i == ARRAY_SIZE(rates)) { 
>|             mutex_unlock(&rt->stream_mutex); 
>|             snd_printk("invalid rate %d in prepare.", 
>|                     alsa_rt->rate); 
> 
>This fixed not a bug but just a symptom of the actual bug, that rt->rate 
>is not signed. 
> 
>There are still "rt->rate = -1" initializations, and usb6fire_pcm_open() 
>then does: 
> 
>    if (rt->rate >= 0) 
>        alsa_rt->hw.rates = rates_alsaid[rt->rate]; 
> 
> 
>Regards, 
>Clemens 
>_______________________________________________ 
>Alsa-devel mailing list 
>Alsa-devel@alsa-project.org 
>http://mailman.alsa-project.org/mailman/listinfo/alsa-devel 
>

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Setting invalid samplerate

[Daniel Mack]

On Thu, May 19, 2011 at 1:28 PM, Torsten Schenk <torsten.schenk@zoho.com> wrote:
> That did the job indeed. I still wonder why it worked in so many cases... However, I will soon create a patch and submit it.

Remember to add a "Cc: stable@kernel.org" in the commit log if you
want to have it backported to stable series of the kernel.

Daniel

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Stephen Warren]

Takashi Iwai wrote at Thursday, May 19, 2011 4:06 AM:
> At Thu, 19 May 2011 11:55:21 +0200,
> David Henningsson wrote:
> >
> > On 2011-05-17 17:46, Takashi Iwai wrote:
> > > At Tue, 17 May 2011 15:46:43 +0200,
> > > David Henningsson wrote:
> > >>
> > >> Just as for headphones and microphone jacks, this patch adds reporting
> > >> of HDMI jack status through the input layer.
> > >>
>...
> 
> OK, as the jack report itself doesn't play a big role yet so far for
> HD-audio, let's get it in, and give pressure to graphics guys :)

I've been testing a preliminary fix for this internally. I found that
the /proc ELD files correctly report monitor_present and eld_valid when
we hot unplug a monitor, but the other fields in the ELD file are not
cleared out to their default state. As best I can tell, this is an ALSA
driver issue, since our display driver only fills in the other fields
when ELDV==1.

-- 
nvpublic

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Takashi Iwai]

At Mon, 23 May 2011 14:49:15 -0700,
Stephen Warren wrote:
> 
> Takashi Iwai wrote at Thursday, May 19, 2011 4:06 AM:
> > At Thu, 19 May 2011 11:55:21 +0200,
> > David Henningsson wrote:
> > >
> > > On 2011-05-17 17:46, Takashi Iwai wrote:
> > > > At Tue, 17 May 2011 15:46:43 +0200,
> > > > David Henningsson wrote:
> > > >>
> > > >> Just as for headphones and microphone jacks, this patch adds reporting
> > > >> of HDMI jack status through the input layer.
> > > >>
> >...
> > 
> > OK, as the jack report itself doesn't play a big role yet so far for
> > HD-audio, let's get it in, and give pressure to graphics guys :)
> 
> I've been testing a preliminary fix for this internally. I found that
> the /proc ELD files correctly report monitor_present and eld_valid when
> we hot unplug a monitor, but the other fields in the ELD file are not
> cleared out to their default state. As best I can tell, this is an ALSA
> driver issue, since our display driver only fills in the other fields
> when ELDV==1.

Then it's just showing stale data.  The eld fields are updated only
when the data is valid.  The patch below should fix the confusion.


thanks,

Takashi

---
diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
index 74b0560..cd96b1d 100644
--- a/sound/pci/hda/hda_eld.c
+++ b/sound/pci/hda/hda_eld.c
@@ -477,6 +477,8 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry,
 
 	snd_iprintf(buffer, "monitor_present\t\t%d\n", e->monitor_present);
 	snd_iprintf(buffer, "eld_valid\t\t%d\n", e->eld_valid);
+	if (!e->eld_valid)
+		return;
 	snd_iprintf(buffer, "monitor_name\t\t%s\n", e->monitor_name);
 	snd_iprintf(buffer, "connection_type\t\t%s\n",
 				eld_connection_type_names[e->conn_type]);

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Stephen Warren]

Takashi Iwai wrote at Monday, May 23, 2011 11:40 PM:
> At Mon, 23 May 2011 14:49:15 -0700, Stephen Warren wrote:
> > ...
> > I've been testing a preliminary fix for this internally. I found that
> > the /proc ELD files correctly report monitor_present and eld_valid when
> > we hot unplug a monitor, but the other fields in the ELD file are not
> > cleared out to their default state. As best I can tell, this is an ALSA
> > driver issue, since our display driver only fills in the other fields
> > when ELDV==1.
> 
> Then it's just showing stale data.  The eld fields are updated only
> when the data is valid.  The patch below should fix the confusion.

> ---
> diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
> index 74b0560..cd96b1d 100644
> --- a/sound/pci/hda/hda_eld.c
> +++ b/sound/pci/hda/hda_eld.c
> @@ -477,6 +477,8 @@ static void hdmi_print_eld_info(struct snd_info_entry
> *entry,
> 
>  	snd_iprintf(buffer, "monitor_present\t\t%d\n", e->monitor_present);
>  	snd_iprintf(buffer, "eld_valid\t\t%d\n", e->eld_valid);
> +	if (!e->eld_valid)
> +		return;
>  	snd_iprintf(buffer, "monitor_name\t\t%s\n", e->monitor_name);
>  	snd_iprintf(buffer, "connection_type\t\t%s\n",
>  				eld_connection_type_names[e->conn_type]);

Takashi,

Your patch looks fine as far as the reporting side goes.

Should we also modify hdmi_intrinsic_event() as follows:

	if (pind && eldv) {
		hdmi_get_show_eld(codec, spec->pin[index],
				  &spec->sink_eld[index]);
		/* TODO: do real things about ELD */
	}
+	else {
+		memset(&spec->sink_eld[index], 0, sizeof(spec->sink_eld[index]));
+	}

... to make sure that if something accidentally uses the ELD data
without checking eld_valid, it'll get obviously bad data instead of
valid-looking-but-stale data? Right now, this isn't an issue, but if
we start pushing the ELD into user-space through a binary control,
it'd be nice if the ELD content were consistent in this way.

Hmm. Actually, hdmi_eld isn't storing the raw ELD bytes, but a parsed
representation of them, so this may not be entirely relevant.

-- 
nvpublic

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Takashi Iwai]

At Tue, 24 May 2011 10:27:45 -0700,
Stephen Warren wrote:
> 
> Takashi Iwai wrote at Monday, May 23, 2011 11:40 PM:
> > At Mon, 23 May 2011 14:49:15 -0700, Stephen Warren wrote:
> > > ...
> > > I've been testing a preliminary fix for this internally. I found that
> > > the /proc ELD files correctly report monitor_present and eld_valid when
> > > we hot unplug a monitor, but the other fields in the ELD file are not
> > > cleared out to their default state. As best I can tell, this is an ALSA
> > > driver issue, since our display driver only fills in the other fields
> > > when ELDV==1.
> > 
> > Then it's just showing stale data.  The eld fields are updated only
> > when the data is valid.  The patch below should fix the confusion.
> 
> > ---
> > diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
> > index 74b0560..cd96b1d 100644
> > --- a/sound/pci/hda/hda_eld.c
> > +++ b/sound/pci/hda/hda_eld.c
> > @@ -477,6 +477,8 @@ static void hdmi_print_eld_info(struct snd_info_entry
> > *entry,
> > 
> >  	snd_iprintf(buffer, "monitor_present\t\t%d\n", e->monitor_present);
> >  	snd_iprintf(buffer, "eld_valid\t\t%d\n", e->eld_valid);
> > +	if (!e->eld_valid)
> > +		return;
> >  	snd_iprintf(buffer, "monitor_name\t\t%s\n", e->monitor_name);
> >  	snd_iprintf(buffer, "connection_type\t\t%s\n",
> >  				eld_connection_type_names[e->conn_type]);
> 
> Takashi,
> 
> Your patch looks fine as far as the reporting side goes.
> 
> Should we also modify hdmi_intrinsic_event() as follows:
> 
> 	if (pind && eldv) {
> 		hdmi_get_show_eld(codec, spec->pin[index],
> 				  &spec->sink_eld[index]);
> 		/* TODO: do real things about ELD */
> 	}
> +	else {
> +		memset(&spec->sink_eld[index], 0, sizeof(spec->sink_eld[index]));
> +	}
> 
> ... to make sure that if something accidentally uses the ELD data
> without checking eld_valid, it'll get obviously bad data instead of
> valid-looking-but-stale data? Right now, this isn't an issue, but if
> we start pushing the ELD into user-space through a binary control,
> it'd be nice if the ELD content were consistent in this way.

It sounds safer, but I'd call memset before setting monitor_present
and eld_valid as below.

> Hmm. Actually, hdmi_eld isn't storing the raw ELD bytes, but a parsed
> representation of them, so this may not be entirely relevant.

True.  Still clearing it explicitly would be better for avoiding
unintentional leaks in future.


thanks,

Takashi

---
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 3229018..4dbe8d6 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -707,6 +707,7 @@ static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
 	if (index < 0)
 		return;
 
+	memset(&spec->sink_eld[index], 0, sizeof(spec->sink_eld[index]));
 	if (spec->old_pin_detect) {
 		if (pind)
 			hdmi_present_sense(codec, tag, &spec->sink_eld[index]);

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Stephen Warren]

Takashi Iwai wrote at Tuesday, May 24, 2011 12:10 PM:
> > Takashi Iwai wrote at Monday, May 23, 2011 11:40 PM:
> > > ---
> > > diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
> > > index 74b0560..cd96b1d 100644
> > > --- a/sound/pci/hda/hda_eld.c
> > > +++ b/sound/pci/hda/hda_eld.c
> > > @@ -477,6 +477,8 @@ static void hdmi_print_eld_info(struct snd_info_entry
> > > *entry,
> > >
> > >  	snd_iprintf(buffer, "monitor_present\t\t%d\n", e->monitor_present);
> > >  	snd_iprintf(buffer, "eld_valid\t\t%d\n", e->eld_valid);
> > > +	if (!e->eld_valid)
> > > +		return;
> > >  	snd_iprintf(buffer, "monitor_name\t\t%s\n", e->monitor_name);
> > >  	snd_iprintf(buffer, "connection_type\t\t%s\n",
> > >  				eld_connection_type_names[e->conn_type]);

> ---
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 3229018..4dbe8d6 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -707,6 +707,7 @@ static void hdmi_intrinsic_event(struct hda_codec
> *codec, unsigned int res)
>  	if (index < 0)
>  		return;
> 
> +	memset(&spec->sink_eld[index], 0, sizeof(spec->sink_eld[index]));
>  	if (spec->old_pin_detect) {
>  		if (pind)
>  			hdmi_present_sense(codec, tag, &spec->sink_eld[index]);

Both:
Tested-by: Stephen Warren <swarren@nvidia.com>

Thanks.

-- 
nvpublic

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Stephen Warren]

Takashi Iwai wrote at Monday, May 23, 2011 11:40 PM:
> diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
> index 74b0560..cd96b1d 100644
> --- a/sound/pci/hda/hda_eld.c
> +++ b/sound/pci/hda/hda_eld.c
> @@ -477,6 +477,8 @@ static void hdmi_print_eld_info(struct snd_info_entry
> *entry,
> 
>  	snd_iprintf(buffer, "monitor_present\t\t%d\n", e->monitor_present);
>  	snd_iprintf(buffer, "eld_valid\t\t%d\n", e->eld_valid);
> +	if (!e->eld_valid)
> +		return;

Thinking about this more, that condition should be:

    if (!(eld->monitor_present && eld->eld_valid))

I think. Of course, graphics drivers shouldn't be setting ELDV without
PD, but best to be safe within ALSA.

As background, it looks like one of our codecs might be inverting the
PD bit (an issue different to the existing old_pin_detect WAR), which
ends up causing that combination.

Related, there are a few inconsistencies in the way patch_hdmi.c handles
ELD retrieval; hdmi_present_sense looks only at ELDV, whereas
hdmi_intrinsic_event looks at both PD and ELDV. I plan to make a patch
to solve that and the WAR for the NVIDIA HW PD handling above. I can
roll your change into that if you want.

>  	snd_iprintf(buffer, "monitor_name\t\t%s\n", e->monitor_name);
>  	snd_iprintf(buffer, "connection_type\t\t%s\n",
>  				eld_connection_type_names[e->conn_type]);

-- 
nvpublic

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Stephen Warren]

David Henningsson wrote at Thursday, May 19, 2011 3:55 AM:
> On 2011-05-17 17:46, Takashi Iwai wrote:
> > At Tue, 17 May 2011 15:46:43 +0200,
> > David Henningsson wrote:
> >>
> >> Just as for headphones and microphone jacks, this patch adds reporting
> >> of HDMI jack status through the input layer.
>...
> 
> To be honest; it's partially working, or rather it's working in the
> sense that it follows the eld proc file. It's also working in hda-emu.
> 
> I've tried it on one Nvidia (with binary drivers), and one Intel
> Graphics and well, and both seem to have the same problem essentially:
> There is no hotplug event coming in (through hdmi_unsol_event) when a
> monitor is removed. But with this patch in perhaps the graphics driver
> writers will feel more motivated to fix it? :-)
> 
> Note that the hotplug event is not coming in when you actually plug the
> cable but when you detect displays and/or apply the monitor
> configuration change.

That's certainly the expected behavior of our drivers at present.

The ELD data and PD/ELDV bits are only updated when we perform a modeset
operation, which only happens in response to an explicit user request,
either with XRandR or through our nvidia-settings application.

I'm not sure if there's a standard that says what our driver's behavior
should be or not; I could argue that the current behavior makes sense to
some degree, so that if the user unplugs their monitor then replugs it,
we don't end up reprogramming all the display hardware. If we did
disable the HDMI output path when a monitor was unplugged, given that I
think the idea is that new paths don't get auto-programmed by the X
driver when a new device is plugged in, but instead wait until XRandR is
used to configure them, in which case if we'd just turned off all the
display outputs, how would the user configure the new one without being
able to see the desktop?

(While this may or may not be ideal, I'm simply pointing this out to
indicate this behavior doesn't indicate any issue with David's patch)

-- 
nvpublic

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[David Henningsson]

On 2011-05-19 18:57, Stephen Warren wrote:
> David Henningsson wrote at Thursday, May 19, 2011 3:55 AM:
>> On 2011-05-17 17:46, Takashi Iwai wrote:
>>> At Tue, 17 May 2011 15:46:43 +0200,
>>> David Henningsson wrote:
>>>>
>>>> Just as for headphones and microphone jacks, this patch adds reporting
>>>> of HDMI jack status through the input layer.
>> ...
>>
>> To be honest; it's partially working, or rather it's working in the
>> sense that it follows the eld proc file. It's also working in hda-emu.
>>
>> I've tried it on one Nvidia (with binary drivers), and one Intel
>> Graphics and well, and both seem to have the same problem essentially:
>> There is no hotplug event coming in (through hdmi_unsol_event) when a
>> monitor is removed. But with this patch in perhaps the graphics driver
>> writers will feel more motivated to fix it? :-)
>>
>> Note that the hotplug event is not coming in when you actually plug the
>> cable but when you detect displays and/or apply the monitor
>> configuration change.
>
> That's certainly the expected behavior of our drivers at present.
>
> The ELD data and PD/ELDV bits are only updated when we perform a modeset
> operation, which only happens in response to an explicit user request,
> either with XRandR or through our nvidia-settings application.
>
> I'm not sure if there's a standard that says what our driver's behavior
> should be or not; I could argue that the current behavior makes sense to
> some degree, so that if the user unplugs their monitor then replugs it,
> we don't end up reprogramming all the display hardware. If we did
> disable the HDMI output path when a monitor was unplugged, given that I
> think the idea is that new paths don't get auto-programmed by the X
> driver when a new device is plugged in, but instead wait until XRandR is
> used to configure them, in which case if we'd just turned off all the
> display outputs, how would the user configure the new one without being
> able to see the desktop?
>
> (While this may or may not be ideal, I'm simply pointing this out to
> indicate this behavior doesn't indicate any issue with David's patch)

Ok, thanks. Just to clarify my current problem - if I start 
nvidia-settings, I'll find my current screen through DVI and an extra 
HDMI screen that is disabled. If I choose to enable this screen (by 
clicking Apply on the "X Server Display Information" tab), I get a 
hotplug event through patch_hdmi.c:hdmi_unsol_event, telling me that the 
monitor is now present. So far, that's what's expected.
However, if I then disable that second HDMI screen and click Apply 
again, I *don't* get an hdmi_unsol_event telling me that the monitor is 
now not present. This seems inconsistent to me.

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

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Stephen Warren]

David Henningsson wrote at Thursday, May 19, 2011 4:45 PM:
>...
> Ok, thanks. Just to clarify my current problem - if I start
> nvidia-settings, I'll find my current screen through DVI and an extra
> HDMI screen that is disabled. If I choose to enable this screen (by
> clicking Apply on the "X Server Display Information" tab), I get a
> hotplug event through patch_hdmi.c:hdmi_unsol_event, telling me that the
> monitor is now present. So far, that's what's expected.
> However, if I then disable that second HDMI screen and click Apply
> again, I *don't* get an hdmi_unsol_event telling me that the monitor is
> now not present. This seems inconsistent to me.

OK, now that's definitely a bug. I'll file one internally.

Thanks for reporting this.

-- 
nvpublic

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Stephen Warren]

Stephen Warren wrote at Thursday, May 19, 2011 4:51 PM:
> David Henningsson wrote at Thursday, May 19, 2011 4:45 PM:
> >...
> > Ok, thanks. Just to clarify my current problem - if I start
> > nvidia-settings, I'll find my current screen through DVI and an extra
> > HDMI screen that is disabled. If I choose to enable this screen (by
> > clicking Apply on the "X Server Display Information" tab), I get a
> > hotplug event through patch_hdmi.c:hdmi_unsol_event, telling me that the
> > monitor is now present. So far, that's what's expected.
> > However, if I then disable that second HDMI screen and click Apply
> > again, I *don't* get an hdmi_unsol_event telling me that the monitor is
> > now not present. This seems inconsistent to me.
> 
> OK, now that's definitely a bug. I'll file one internally.

This should be fixed in the 275.09.04 pre-release driver that we just
released.

-- 
nvpublic

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Stephen Warren]

David Henningsson wrote at Tuesday, May 17, 2011 7:47 AM:
> Just as for headphones and microphone jacks, this patch adds reporting
> of HDMI jack status through the input layer.

Interesting. I was asked to look into this, but you beat me to it; thanks!

A couple questions though:

a) Is it possible to report more information alongside the plug events,
such as ELD/EDID content? Or, is the idea that the kernel sends a plug
event, and then user-space retrieves that information via some other
API? I don't think there's an API to retrieve ELD information at present
though right? Although certainly it'd make sense for that to be a 
completely separate patch.

b) We might discuss how to tie ALSA jacks/ports/PCMs to X displays in
some way, so that we can represent to the user which specific active
monitor is the one that supports audio (e.g. in a 2-head setup with both
monitors connected over HDMI, yet only 1 monitor supports audio).

I note that the ELD data contains a port_id field, which might be usable
for this purpose. Could this contain an xrandr value, or an NV-CTRL
(NVIDIA's proprietary control protocol) display ID, or something similar?

-- 
nvpublic

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Takashi Iwai]

At Tue, 17 May 2011 09:00:51 -0700,
Stephen Warren wrote:
> 
> David Henningsson wrote at Tuesday, May 17, 2011 7:47 AM:
> > Just as for headphones and microphone jacks, this patch adds reporting
> > of HDMI jack status through the input layer.
> 
> Interesting. I was asked to look into this, but you beat me to it; thanks!
> 
> A couple questions though:
> 
> a) Is it possible to report more information alongside the plug events,
> such as ELD/EDID content? Or, is the idea that the kernel sends a plug
> event, and then user-space retrieves that information via some other
> API? I don't think there's an API to retrieve ELD information at present
> though right? Although certainly it'd make sense for that to be a 
> completely separate patch.

A simple approach would be adding a control element containing
byte-array of ELD/EDID.

> b) We might discuss how to tie ALSA jacks/ports/PCMs to X displays in
> some way, so that we can represent to the user which specific active
> monitor is the one that supports audio (e.g. in a 2-head setup with both
> monitors connected over HDMI, yet only 1 monitor supports audio).

Yeah, most tight coupling with display vs audio-port is needed.

> I note that the ELD data contains a port_id field, which might be usable
> for this purpose. Could this contain an xrandr value, or an NV-CTRL
> (NVIDIA's proprietary control protocol) display ID, or something similar?

Hm, rather it's a question to X side, i.e. what information is exposed
via xrandr.


Takashi

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[pl bossart]

>> a) Is it possible to report more information alongside the plug events,
>> such as ELD/EDID content? Or, is the idea that the kernel sends a plug
>> event, and then user-space retrieves that information via some other
>> API? I don't think there's an API to retrieve ELD information at present
>> though right? Although certainly it'd make sense for that to be a
>> completely separate patch.
>
> A simple approach would be adding a control element containing
> byte-array of ELD/EDID.

Are there any examples of such controls? Or are we talking about a new
kind of control?
We could really use this ELD information in PulseAudio now that the
passthrough mode is in git master; for now the codecs supported by the
receiver are hard-coded, not a very reliable solution...
-Pierre

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Takashi Iwai]

At Tue, 17 May 2011 12:09:47 -0500,
pl bossart wrote:
> 
> >> a) Is it possible to report more information alongside the plug events,
> >> such as ELD/EDID content? Or, is the idea that the kernel sends a plug
> >> event, and then user-space retrieves that information via some other
> >> API? I don't think there's an API to retrieve ELD information at present
> >> though right? Although certainly it'd make sense for that to be a
> >> completely separate patch.
> >
> > A simple approach would be adding a control element containing
> > byte-array of ELD/EDID.
> 
> Are there any examples of such controls? Or are we talking about a new
> kind of control?

Look for SNDRV_CTL_ELEM_TYPE_BYTES.  Some codecs provide these.

> We could really use this ELD information in PulseAudio now that the
> passthrough mode is in git master; for now the codecs supported by the
> receiver are hard-coded, not a very reliable solution...


Takashi

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[pl bossart]

>> >> a) Is it possible to report more information alongside the plug events,
>> >> such as ELD/EDID content? Or, is the idea that the kernel sends a plug
>> >> event, and then user-space retrieves that information via some other
>> >> API? I don't think there's an API to retrieve ELD information at present
>> >> though right? Although certainly it'd make sense for that to be a
>> >> completely separate patch.
>> >
>> > A simple approach would be adding a control element containing
>> > byte-array of ELD/EDID.
>>
>> Are there any examples of such controls? Or are we talking about a new
>> kind of control?
>
> Look for SNDRV_CTL_ELEM_TYPE_BYTES.  Some codecs provide these.

Thanks for the pointer. looks simple enough to expose the ELD bytes.
This type of element can store up to 512 bytes, enough to store the
ELD header+baseline fields (260 bytes tops). I don't think userspace
would want to muck with vendor-specific information?
we may need an array of ELD controls in case there are several
monitors. Not sure how to represent which one is actually used. The
ELD is linked to a specific nid (node id), we'd need to link this to
the audio device #?

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Stephen Warren]

pl bossart wrote at Tuesday, May 17, 2011 2:51 PM:
> >> >> a) Is it possible to report more information alongside the plug events,
> >> >> such as ELD/EDID content? Or, is the idea that the kernel sends a plug
> >> >> event, and then user-space retrieves that information via some other
> >> >> API? I don't think there's an API to retrieve ELD information at present
> >> >> though right? Although certainly it'd make sense for that to be a
> >> >> completely separate patch.
> >> >
> >> > A simple approach would be adding a control element containing
> >> > byte-array of ELD/EDID.
> >>
> >> Are there any examples of such controls? Or are we talking about a new
> >> kind of control?
> >
> > Look for SNDRV_CTL_ELEM_TYPE_BYTES.  Some codecs provide these.
> 
> Thanks for the pointer. looks simple enough to expose the ELD bytes.
> This type of element can store up to 512 bytes, enough to store the
> ELD header+baseline fields (260 bytes tops). I don't think userspace
> would want to muck with vendor-specific information?

Isn't the ELD limited to 256 bytes, since verb F2F has an 8-bit offset
parameter?

I think it'd be a good idea to just dump the entire ELD out to user-space
for future flexibility. It's trivial for user-space to ignore the vendor-
specific data (since it's always after the standardized data).

> we may need an array of ELD controls in case there are several
> monitors. Not sure how to represent which one is actually used. The
> ELD is linked to a specific nid (node id), we'd need to link this to
> the audio device #?

Perhaps the vendor-specific data space could be useful for matching audio
ports to X displays; there is a 64-bit Port_ID in the ELD that might be
used for that, or we could define X/Linux/Unix as the vendor, and specify
the format of the vendor-specific data in a way that defines this mapping.

That said, the internal APIs our graphics driver uses to write the ELD
is currently limited to 96 bytes (the size of the standardized section
of the ELD). I'm not sure yet if that's simply because 96 bytes was all
that was needed, or if that also ended up being encoded as a HW design
limitation.

-- 
nvpublic

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[pl bossart]

> Isn't the ELD limited to 256 bytes, since verb F2F has an 8-bit offset
> parameter?

I guess you are right. I saw some checks in the hda code where the ELD
size is gt PAGE_SIZE. Confusing really.

> I think it'd be a good idea to just dump the entire ELD out to user-space
> for future flexibility. It's trivial for user-space to ignore the vendor-
> specific data (since it's always after the standardized data).

Right, if this fits in 512 bytes no reason to drop anything.

> Perhaps the vendor-specific data space could be useful for matching audio
> ports to X displays; there is a 64-bit Port_ID in the ELD that might be
> used for that, or we could define X/Linux/Unix as the vendor, and specify
> the format of the vendor-specific data in a way that defines this mapping.

Sounds complicated...The spec says this PortID field is "a ""canned""
field that would be populated through implementation specific mean of
default programming before the graphic driver is loaded". Duh?
The standard "Monitor Name String" may be easier to use...

> That said, the internal APIs our graphics driver uses to write the ELD
> is currently limited to 96 bytes (the size of the standardized section
> of the ELD). I'm not sure yet if that's simply because 96 bytes was all
> that was needed, or if that also ended up being encoded as a HW design
> limitation.

Looks like the max for ELDv2 is 80 bytes for the baseline+4 for the header.

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Stephen Warren]

pl bossart wrote at Tuesday, May 17, 2011 4:12 PM:
>...
> > Perhaps the vendor-specific data space could be useful for matching audio
> > ports to X displays; there is a 64-bit Port_ID in the ELD that might be
> > used for that, or we could define X/Linux/Unix as the vendor, and specify
> > the format of the vendor-specific data in a way that defines this mapping.
> 
> Sounds complicated...The spec says this PortID field is "a ""canned""
> field that would be populated through implementation specific mean of
> default programming before the graphic driver is loaded". Duh?

I'm pretty sure our HW doesn't initialize this to anything meaningful before
the video driver runs.

> The standard "Monitor Name String" may be easier to use...

Do you mean co-opt this field and store alternate data in it? The real
monitor name for both monitors in my dual-head setup are the same...

> > That said, the internal APIs our graphics driver uses to write the ELD
> > is currently limited to 96 bytes (the size of the standardized section
> > of the ELD). I'm not sure yet if that's simply because 96 bytes was all
> > that was needed, or if that also ended up being encoded as a HW design
> > limitation.
> 
> Looks like the max for ELDv2 is 80 bytes for the baseline+4 for the
> header.

Oh yeah; I'd calculated assuming SADs were 4 bytes but they're 3.

-- 
nvpublic

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[pl bossart]

>> > A simple approach would be adding a control element containing
>> > byte-array of ELD/EDID.
>>
>> Are there any examples of such controls? Or are we talking about a new
>> kind of control?
>
> Look for SNDRV_CTL_ELEM_TYPE_BYTES.  Some codecs provide these.

I started doing some work on this, adding a persistent 256-byte buffer
to store the ELD data instead of dynamically allocating and freeing
the buffer. I am kind of lost with the controls though, never looked
at this part of ALSA before.
In all the HDA code, all controls have an IFACE_MIXER type, while all
the code that uses ELEM_TYPE_BYTES the type is IFACE_PCM. Does this
matter at all?
Likewise, there's a 'generic_hdmi_build_controls' routine in which I
could add the ELD control, but it uses cvt structures. I think it
would make more sense to relate ELD controls to pins, as done with the
proc information.
Quite a learning experience...

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Takashi Iwai]

At Wed, 18 May 2011 10:43:36 -0500,
pl bossart wrote:
> 
> >> > A simple approach would be adding a control element containing
> >> > byte-array of ELD/EDID.
> >>
> >> Are there any examples of such controls? Or are we talking about a new
> >> kind of control?
> >
> > Look for SNDRV_CTL_ELEM_TYPE_BYTES.  Some codecs provide these.
> 
> I started doing some work on this, adding a persistent 256-byte buffer
> to store the ELD data instead of dynamically allocating and freeing
> the buffer. I am kind of lost with the controls though, never looked
> at this part of ALSA before.
> In all the HDA code, all controls have an IFACE_MIXER type, while all
> the code that uses ELEM_TYPE_BYTES the type is IFACE_PCM. Does this
> matter at all?

Not much.  It's fine to keep it in IFACE_MIXER.
But, IFACE_PCM might be a better choice since ELD is actually bound
with codec-port, i.e. a PCM stream, and all mixer apps look at these
unparseable IFACE_MIXER elements.

> Likewise, there's a 'generic_hdmi_build_controls' routine in which I
> could add the ELD control, but it uses cvt structures. I think it
> would make more sense to relate ELD controls to pins, as done with the
> proc information.

Yeah, it'd make more sense.  Currently it's a bit messy there, as we
handle cvt and pin in mixture somehow.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Takashi Iwai]

At Tue, 17 May 2011 15:46:43 +0200,
David Henningsson wrote:
> 
>  static void hdmi_non_intrinsic_event(struct hda_codec *codec, unsigned int res)
> @@ -919,6 +922,17 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
>  		return -E2BIG;
>  	}
>  
> +#ifdef CONFIG_SND_HDA_INPUT_JACK
> +	{
> +		int err;
> +		err = snd_hda_input_jack_add(codec, pin_nid,
> +					     SND_JACK_VIDEOOUT, NULL);
> +		if (err < 0)
> +			return err;
> +		snd_hda_input_jack_report(codec, pin_nid);
> +	}
> +#endif

You don't need ifdef here since dummy functions are defined when
CONFIG_SND_HDA_INPUT_JACK=n.  Then don't need to make a block here,
as you won't get an unused-variable compile warning.


Takashi

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Stephen Warren]

David Henningsson wrote at Tuesday, May 17, 2011 7:47 AM:
> Just as for headphones and microphone jacks, this patch adds reporting
> of HDMI jack status through the input layer.
> 
> I considered making additional SND_JACK_* constants for HDMI and
> Displayport instead of going with SND_JACK_VIDEOOUT, but it didn't seem
> worth the additions, and breakage of compiling with old kernels, etc.
> Let me know if you think otherwise and I'll prepare a second patch for
> that.

One more thought on this:

Some of our older HW (MCP6x, MCP7x chipsets) doesn't support unsolicited
responses, nor PD/ELDV/ELD data. If PulseAudio is updated to listen to
these new jack events on kernels where the input files are present,
that'll presumably cause it never to allow use of the HDMI sinks on those
chipsets. I don't know if other vendors have this issue or not.

In that case, can we do one of:

* Not create the jack/input files at all.

* Immediately mark the jack as "present".

I assume the latter would be easier for user-space to deal with; it'd be
more in line with the kernel abstracting HW differences.

-- 
nvpublic

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[David Henningsson]

On 2011-05-20 23:59, Stephen Warren wrote:
> David Henningsson wrote at Tuesday, May 17, 2011 7:47 AM:
>> Just as for headphones and microphone jacks, this patch adds reporting
>> of HDMI jack status through the input layer.
>>
>> I considered making additional SND_JACK_* constants for HDMI and
>> Displayport instead of going with SND_JACK_VIDEOOUT, but it didn't seem
>> worth the additions, and breakage of compiling with old kernels, etc.
>> Let me know if you think otherwise and I'll prepare a second patch for
>> that.
>
> One more thought on this:
>
> Some of our older HW (MCP6x, MCP7x chipsets) doesn't support unsolicited
> responses, nor PD/ELDV/ELD data. If PulseAudio is updated to listen to
> these new jack events on kernels where the input files are present,
> that'll presumably cause it never to allow use of the HDMI sinks on those
> chipsets. I don't know if other vendors have this issue or not.
>
> In that case, can we do one of:
>
> * Not create the jack/input files at all.
>
> * Immediately mark the jack as "present".
>
> I assume the latter would be easier for user-space to deal with; it'd be
> more in line with the kernel abstracting HW differences.
>

Ok, thanks for the input (no pun intended ;-) ). If they support 
presence detect, we could create a polling loop. Otherwise, don't create 
the jack.

Is this autodetectable or do we have to quirk it?

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

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Takashi Iwai]

At Sat, 21 May 2011 08:25:57 +0200,
David Henningsson wrote:
> 
> On 2011-05-20 23:59, Stephen Warren wrote:
> > David Henningsson wrote at Tuesday, May 17, 2011 7:47 AM:
> >> Just as for headphones and microphone jacks, this patch adds reporting
> >> of HDMI jack status through the input layer.
> >>
> >> I considered making additional SND_JACK_* constants for HDMI and
> >> Displayport instead of going with SND_JACK_VIDEOOUT, but it didn't seem
> >> worth the additions, and breakage of compiling with old kernels, etc.
> >> Let me know if you think otherwise and I'll prepare a second patch for
> >> that.
> >
> > One more thought on this:
> >
> > Some of our older HW (MCP6x, MCP7x chipsets) doesn't support unsolicited
> > responses, nor PD/ELDV/ELD data. If PulseAudio is updated to listen to
> > these new jack events on kernels where the input files are present,
> > that'll presumably cause it never to allow use of the HDMI sinks on those
> > chipsets. I don't know if other vendors have this issue or not.
> >
> > In that case, can we do one of:
> >
> > * Not create the jack/input files at all.
> >
> > * Immediately mark the jack as "present".
> >
> > I assume the latter would be easier for user-space to deal with; it'd be
> > more in line with the kernel abstracting HW differences.
> >
> 
> Ok, thanks for the input (no pun intended ;-) ). If they support 
> presence detect, we could create a polling loop. Otherwise, don't create 
> the jack.

In that case, input-polldev would be an option, too.
Of course, this will need some change in the input-jack layer.


Takashi

Re: [PATCH] ALSA: HDA: Add jack detection for HDMI

[Stephen Warren]

David Henningsson wrote at Saturday, May 21, 2011 12:26 AM:
> On 2011-05-20 23:59, Stephen Warren wrote:
> > David Henningsson wrote at Tuesday, May 17, 2011 7:47 AM:
> >> Just as for headphones and microphone jacks, this patch adds reporting
> >> of HDMI jack status through the input layer.
> >
> > Some of our older HW (MCP6x, MCP7x chipsets) doesn't support unsolicited
> > responses, nor PD/ELDV/ELD data. If PulseAudio is updated to listen to
> > these new jack events on kernels where the input files are present,
> > that'll presumably cause it never to allow use of the HDMI sinks on those
> > chipsets. I don't know if other vendors have this issue or not.
> 
> Ok, thanks for the input (no pun intended ;-) ). If they support
> presence detect, we could create a polling loop. Otherwise, don't create
> the jack.

There's no unsolicited response, nor is there any way to read any of the
PD/ELD data as far as I know, nor any way for the graphics driver to pass
it into the audio HW I presume.

> Is this autodetectable or do we have to quirk it?

The relevant NVIDIA chips have custom .patch functions in
snd_hda_preset_hdmi[], which end up not calling snd_hda_eld_proc_new()
for the pins in question. I note that many of the ATI/AMD codecs are in
the same boat here. Presumably, that function sets up some data structure
that could easily be keyed off of.

-- 
nvpublic