[alsa-devel] [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.
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.
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
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.
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";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
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
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
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
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
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
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.
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]);
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;
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.
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;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]);
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;
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.
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]);
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)
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 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.
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.
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?
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
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
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
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 #?
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.
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.
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.
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...
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
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
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.
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?
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
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.
participants (7)
-
Clemens Ladisch -
Daniel Mack -
David Henningsson -
pl bossart -
Stephen Warren -
Takashi Iwai -
Torsten Schenk