Re: [alsa-devel] link between HDMI ELD and PCM devices
On 09/21/2011 03:20 PM, Pierre-Louis Bossart wrote:
This won't work - in the case of several HDMI codecs (common with NVidia) there will be more than one "HDMI 0".
No, the code doesn't use a constant,
It's actually four constants, please see generic_hdmi_pcm_names in patch_hdmi.c. But my point is that this restarts from 0 on every codec.
it's assigned in the same way as the device name. You would have HDMI 0, HDMI 1..4. I don't have this kind of hardware, it'd be good if someone could verify that the name is indeed dynamic.
On a 3.0 based kernel, here's part of my aplay -l output:
card 3: NVidia [HDA NVidia], device 3: HDMI 0 [HDMI 0] Subdevices: 1/1 Subdevice #0: subdevice #0 card 3: NVidia [HDA NVidia], device 7: HDMI 0 [HDMI 0] Subdevices: 1/1 Subdevice #0: subdevice #0 card 3: NVidia [HDA NVidia], device 8: HDMI 0 [HDMI 0] Subdevices: 1/1 Subdevice #0: subdevice #0 card 3: NVidia [HDA NVidia], device 9: HDMI 0 [HDMI 0] Subdevices: 1/1 Subdevice #0: subdevice #0
The solution is to actually find the device number out, like this:
struct hdmi_spec *spec = codec->spec; int pcmdev = spec->pcm_rec[pin_idx].device;
That's what I wanted initially, but this device field is not used/initialized, and like I said the device assignment comes after the pin sensing and all the ELD handling
A good place to do this is generic_hdmi_build_controls, both because it is normally used to add controls (which is what you do), and because it is triggered after the pcm device is assigned.
(A side note to Takashi: for some reason hda-emu has the calls to build_controls and build_pcms switched, compared to the real implementation.)
No, the code doesn't use a constant,
It's actually four constants, please see generic_hdmi_pcm_names in patch_hdmi.c. But my point is that this restarts from 0 on every codec. On a 3.0 based kernel, here's part of my aplay -l output:
card 3: NVidia [HDA NVidia], device 3: HDMI 0 [HDMI 0] Subdevices: 1/1 Subdevice #0: subdevice #0 card 3: NVidia [HDA NVidia], device 7: HDMI 0 [HDMI 0] Subdevices: 1/1 Subdevice #0: subdevice #0
Thanks for this feedback. I was obviously trying to build on broken code... Will try to see if I can add the control in build_controls as you suggested. -Pierre
David Henningsson wrote at Wednesday, September 21, 2011 7:55 AM:
On 09/21/2011 03:20 PM, Pierre-Louis Bossart wrote:
This won't work - in the case of several HDMI codecs (common with NVidia) there will be more than one "HDMI 0".
No, the code doesn't use a constant,
It's actually four constants, please see generic_hdmi_pcm_names in patch_hdmi.c. But my point is that this restarts from 0 on every codec.
it's assigned in the same way as the device name. You would have HDMI 0, HDMI 1..4. I don't have this kind of hardware, it'd be good if someone could verify that the name is indeed dynamic.
On a 3.0 based kernel, here's part of my aplay -l output:
card 3: NVidia [HDA NVidia], device 3: HDMI 0 [HDMI 0] Subdevices: 1/1 Subdevice #0: subdevice #0 card 3: NVidia [HDA NVidia], device 7: HDMI 0 [HDMI 0] Subdevices: 1/1 Subdevice #0: subdevice #0 card 3: NVidia [HDA NVidia], device 8: HDMI 0 [HDMI 0] Subdevices: 1/1 Subdevice #0: subdevice #0 card 3: NVidia [HDA NVidia], device 9: HDMI 0 [HDMI 0] Subdevices: 1/1 Subdevice #0: subdevice #0
Just as an FYI, this is a little HW-dependent.
For GPUs that have multiple codecs, everything David said is true.
Very recent NVIDIA GPUs such as GeForce 520, and at least some Intel GPUs which Pierre-Louis may be testing with (e.g. my wife's Ibex Peak) have just one codec, which supports multiple PCMs, and then are numbered like:
card 1: NVidia_1 [HDA NVidia], device 3: HDMI 0 [HDMI 0] Subdevices: 1/1 Subdevice #0: subdevice #0 card 1: NVidia_1 [HDA NVidia], device 7: HDMI 1 [HDMI 1] Subdevices: 1/1 Subdevice #0: subdevice #0
Just as an FYI, this is a little HW-dependent.
For GPUs that have multiple codecs, everything David said is true.
Very recent NVIDIA GPUs such as GeForce 520, and at least some Intel GPUs which Pierre-Louis may be testing with (e.g. my wife's Ibex Peak) have just one codec, which supports multiple PCMs, and then are numbered like:
card 1: NVidia_1 [HDA NVidia], device 3: HDMI 0 [HDMI 0] Subdevices: 1/1 Subdevice #0: subdevice #0 card 1: NVidia_1 [HDA NVidia], device 7: HDMI 1 [HDMI 1] Subdevices: 1/1 Subdevice #0: subdevice #0
Good feedback. Looks like the device number is the only reliable piece of information here. I simplified my initial patch based on David's suggestions, can you guys let me know how it behaves with multiple codecs/multiple PCMs? On my laptop, I get this result
$ amixer -c0 controls numid=22,iface=PCM,name='ELD',device=3 $ amixer -c0 cget numid=22 numid=22,iface=PCM,name='ELD',device=3 ; type=BYTES,access=rw------,values=84 : values=0x10,0x00,0x0d,0x00,0x68,0x82,0x00,0x0f,<snip> Thanks!
Pierre-Louis Bossart wrote at Wednesday, September 21, 2011 11:31 AM: ...
I simplified my initial patch based on David's suggestions, can you guys let me know how it behaves with multiple codecs/multiple PCMs? On my laptop, I get this result
$ amixer -c0 controls numid=22,iface=PCM,name='ELD',device=3 $ amixer -c0 cget numid=22 numid=22,iface=PCM,name='ELD',device=3 ; type=BYTES,access=rw------,values=84 : values=0x10,0x00,0x0d,0x00,0x68,0x82,0x00,0x0f,<snip>
GeForce 520 (1 codec, up to 4 pins per, but 2 pins disabled):
[swarren@swarren-lx2 alsa-driver-build]$ sudo amixer -c1 controls | grep ELD numid=5,iface=PCM,name='ELD',device=3 numid=10,iface=PCM,name='ELD',device=7
GeForce 200 series (4 codecs, 1 pin per):
[swarren@swarren-lx2 alsa-driver-build]$ sudo amixer -c2 controls | grep ELD numid=5,iface=PCM,name='ELD',device=3 numid=10,iface=PCM,name='ELD',device=7 numid=15,iface=PCM,name='ELD',device=8 numid=20,iface=PCM,name='ELD',device=9
That looks pretty sane to me.
A few comments on the patch:
It doesn't apply to ToT; it conflicts slightly with Takashi's recent ELD bytes cleanup.
diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
...
@@ -335,21 +335,20 @@ int snd_hdmi_get_eld(struct hdmi_eld *eld, snd_printd(KERN_INFO "HDMI: ELD buf size is 0, force 128\n"); size = 128; }
- if (size < ELD_FIXED_BYTES || size > PAGE_SIZE) {
- if (size < ELD_FIXED_BYTES || size > ELD_MAX_SIZE || size > PAGE_SIZE) {
Note that eld->eld_size are checked against ELD_MAX_SIZE here.
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
...
+static int hdmi_eld_ctl_info(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_info *uinfo)+{
...
- pin_idx = kcontrol->private_value;
...
- if (pin_idx < 0) {
uinfo->count = 0;
I don't think you need that conditional. I'd just assume that pin_idx was set correctly when the control was created, and not create the control if that wasn't the case. At most, BUG_ON here?
Same comment in hdmi_eld_ctl_get below.
+static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
...
- if (pin_idx < 0) {
memset(ucontrol->value.bytes.data, 0, ELD_MAX_SIZE);- } else {
per_pin = &spec->pins[pin_idx];eld = &per_pin->sink_eld;memcpy(ucontrol->value.bytes.data, eld->eld_buffer,(eld->eld_size > ELD_MAX_SIZE) ?ELD_MAX_SIZE : eld->eld_size);
Given the check on eld->eld_size in snd_hdmi_get_eld, I don't think you need the conditional here to calculate the number of bytes to copy.
One question: How do we know that ucontrol is large enough to accept eld->eld_size; are all BYTES controls always big enough, or do we need a check here on ucontrol->value.bytes.size or something like that?
+static int hdmi_create_eld_ctl(struct hda_codec *codec, int pin_idx,
int device)+{
- struct snd_kcontrol *kctl;
- struct hdmi_spec *spec = codec->spec;
- int err;
- if (pin_idx < 0)
return -EINVAL;
Oh, the pin_idx validity check is already here...
GeForce 520 (1 codec, up to 4 pins per, but 2 pins disabled):
[swarren@swarren-lx2 alsa-driver-build]$ sudo amixer -c1 controls | grep ELD numid=5,iface=PCM,name='ELD',device=3 numid=10,iface=PCM,name='ELD',device=7
GeForce 200 series (4 codecs, 1 pin per):
[swarren@swarren-lx2 alsa-driver-build]$ sudo amixer -c2 controls | grep ELD numid=5,iface=PCM,name='ELD',device=3 numid=10,iface=PCM,name='ELD',device=7 numid=15,iface=PCM,name='ELD',device=8 numid=20,iface=PCM,name='ELD',device=9
That looks pretty sane to me.
Excellent. Thanks for your time!
It doesn't apply to ToT; it conflicts slightly with Takashi's recent ELD bytes cleanup.
Ah, yes. Haven't updated since kernel.org went down.
pin_idx = kcontrol->private_value; ...
- if (pin_idx < 0) {
uinfo->count = 0;I don't think you need that conditional. I'd just assume that pin_idx was set correctly when the control was created, and not create the control if that wasn't the case. At most, BUG_ON here?
Agree. All this was copy/pasted debug code in case I modified things.
One question: How do we know that ucontrol is large enough to accept eld->eld_size; are all BYTES controls always big enough, or do we need a check here on ucontrol->value.bytes.size or something like that?
The ELD is limited to 256 bytes. Same as the size of a BYTES control.
At Wed, 21 Sep 2011 15:55:06 +0200, David Henningsson wrote:
(A side note to Takashi: for some reason hda-emu has the calls to build_controls and build_pcms switched, compared to the real implementation.)
It's just overlooked. Fixed now.
FYI, I set up hda-emu.git in github.com: git://github.com/tiwai/hda-emu.git
thanks,
Takashi
participants (4)
-
David Henningsson -
Pierre-Louis Bossart -
Stephen Warren -
Takashi Iwai