* link between HDMI ELD and PCM devices @ 2011-09-20 23:12 Pierre-Louis Bossart 2011-09-20 23:33 ` Stephen Warren 0 siblings, 1 reply; 13+ messages in thread From: Pierre-Louis Bossart @ 2011-09-20 23:12 UTC (permalink / raw) To: alsa-devel; +Cc: ; Takashi Iwai, ; Wu Fengguang, Stephen Warren Hi, I've been trying for some time to expose the HDMI ELD information in a new control. The problem I am facing with the HDMI/HDA code is that the ELD is linked to an output pin, and I can't find an elegant way to link this output pin with a PCM device number to be used by my control. For a single output, I could hard-code something but it would probably not work with multiple monitors handled by recent hardware. There is some existing code such as pin_nid_to_pin_index() in patch_hdmi.c, but I can't figure out what this pin_idx is, and how to find the related PCM device number. Any suggestions? Thanks, -Pierre ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: link between HDMI ELD and PCM devices 2011-09-20 23:12 link between HDMI ELD and PCM devices Pierre-Louis Bossart @ 2011-09-20 23:33 ` Stephen Warren 2011-09-21 0:58 ` Pierre-Louis Bossart 0 siblings, 1 reply; 13+ messages in thread From: Stephen Warren @ 2011-09-20 23:33 UTC (permalink / raw) To: Pierre-Louis Bossart, alsa-devel@alsa-project.org Cc: ; Takashi Iwai, ; Wu Fengguang Pierre-Louis Bossart wrote at Tuesday, September 20, 2011 5:13 PM: > Hi, > I've been trying for some time to expose the HDMI ELD information in a > new control. The problem I am facing with the HDMI/HDA code is that the > ELD is linked to an output pin, and I can't find an elegant way to link > this output pin with a PCM device number to be used by my control. For a > single output, I could hard-code something but it would probably not > work with multiple monitors handled by recent hardware. > There is some existing code such as pin_nid_to_pin_index() in > patch_hdmi.c, but I can't figure out what this pin_idx is, and how to > find the related PCM device number. > Any suggestions? That function simply returns the index into the array spec->pins[] that refers to that pin NID. Judging by hinfo_to_pin_index() (e.g. used near the start ofhdmi_pcm_open), that same index is valid for spec->pcm_rec[] too, which I assume will give you access to what you want, possibly through the .stream[0] field. -- nvpublic ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: link between HDMI ELD and PCM devices 2011-09-20 23:33 ` Stephen Warren @ 2011-09-21 0:58 ` Pierre-Louis Bossart 2011-09-21 6:44 ` David Henningsson 0 siblings, 1 reply; 13+ messages in thread From: Pierre-Louis Bossart @ 2011-09-21 0:58 UTC (permalink / raw) To: Stephen Warren Cc: ; Takashi Iwai, alsa-devel@alsa-project.org, ; Wu Fengguang > > There is some existing code such as pin_nid_to_pin_index() in > > patch_hdmi.c, but I can't figure out what this pin_idx is, and how to > > find the related PCM device number. > > Any suggestions? > > That function simply returns the index into the array spec->pins[] that > refers to that pin NID. > > Judging by hinfo_to_pin_index() (e.g. used near the start ofhdmi_pcm_open), > that same index is valid for spec->pcm_rec[] too, which I assume will > give you access to what you want, possibly through the .stream[0] field. Looks like the device number is assigned in hda_codec.c and there's no hooks to create the ELD control then... Instead of creating a PCM_IFACE control, it's probably simpler to create a MIXER control in the build_pcms routines, using the same name for the control and the device, eg: >amixer -c0 controls numid=1,iface=MIXER,name='ELD HDMI 0' >aplay -l card 0: Intel [HDA Intel], device 3: HDMI 0 [HDMI 0] That looks simple enough for apps to figure out which ELD control to use? Feedback welcome. -Pierre ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: link between HDMI ELD and PCM devices 2011-09-21 0:58 ` Pierre-Louis Bossart @ 2011-09-21 6:44 ` David Henningsson 2011-09-21 8:33 ` Takashi Iwai ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: David Henningsson @ 2011-09-21 6:44 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Takashi Iwai, alsa-devel@alsa-project.org, Wu Fengguang, Stephen Warren On 09/21/2011 02:58 AM, Pierre-Louis Bossart wrote: > >>> There is some existing code such as pin_nid_to_pin_index() in >>> patch_hdmi.c, but I can't figure out what this pin_idx is, and how to >>> find the related PCM device number. >>> Any suggestions? >> >> That function simply returns the index into the array spec->pins[] that >> refers to that pin NID. >> >> Judging by hinfo_to_pin_index() (e.g. used near the start ofhdmi_pcm_open), >> that same index is valid for spec->pcm_rec[] too, which I assume will >> give you access to what you want, possibly through the .stream[0] field. > > Looks like the device number is assigned in hda_codec.c and there's no > hooks to create the ELD control then... > Instead of creating a PCM_IFACE control, it's probably simpler to create > a MIXER control in the build_pcms routines, using the same name for the > control and the device, eg: > >> amixer -c0 controls > numid=1,iface=MIXER,name='ELD HDMI 0' >> aplay -l > card 0: Intel [HDA Intel], device 3: HDMI 0 [HDMI 0] > > That looks simple enough for apps to figure out which ELD control to > use? Feedback welcome. This won't work - in the case of several HDMI codecs (common with NVidia) there will be more than one "HDMI 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; (This must be run after build_pcms, e g in build_controls.) -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: link between HDMI ELD and PCM devices 2011-09-21 6:44 ` David Henningsson @ 2011-09-21 8:33 ` Takashi Iwai 2011-09-21 13:20 ` Pierre-Louis Bossart [not found] ` <000001cc7861$2d7895e0$8869c1a0$@bossart@linux.intel.com> 2 siblings, 0 replies; 13+ messages in thread From: Takashi Iwai @ 2011-09-21 8:33 UTC (permalink / raw) To: David Henningsson Cc: alsa-devel@alsa-project.org, Wu Fengguang, Stephen Warren, Pierre-Louis Bossart At Wed, 21 Sep 2011 08:44:15 +0200, David Henningsson wrote: > > On 09/21/2011 02:58 AM, Pierre-Louis Bossart wrote: > > > >>> There is some existing code such as pin_nid_to_pin_index() in > >>> patch_hdmi.c, but I can't figure out what this pin_idx is, and how to > >>> find the related PCM device number. > >>> Any suggestions? > >> > >> That function simply returns the index into the array spec->pins[] that > >> refers to that pin NID. > >> > >> Judging by hinfo_to_pin_index() (e.g. used near the start ofhdmi_pcm_open), > >> that same index is valid for spec->pcm_rec[] too, which I assume will > >> give you access to what you want, possibly through the .stream[0] field. > > > > Looks like the device number is assigned in hda_codec.c and there's no > > hooks to create the ELD control then... > > Instead of creating a PCM_IFACE control, it's probably simpler to create > > a MIXER control in the build_pcms routines, using the same name for the > > control and the device, eg: > > > >> amixer -c0 controls > > numid=1,iface=MIXER,name='ELD HDMI 0' > >> aplay -l > > card 0: Intel [HDA Intel], device 3: HDMI 0 [HDMI 0] > > > > That looks simple enough for apps to figure out which ELD control to > > use? Feedback welcome. > > This won't work - in the case of several HDMI codecs (common with > NVidia) there will be more than one "HDMI 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; > > (This must be run after build_pcms, e g in build_controls.) Yes, currently a device-number assignment is somehow tricky, as it's assigned semi-dynamically. But the method above David mentioned should work. thanks, Takashi ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: link between HDMI ELD and PCM devices 2011-09-21 6:44 ` David Henningsson 2011-09-21 8:33 ` Takashi Iwai @ 2011-09-21 13:20 ` Pierre-Louis Bossart [not found] ` <000001cc7861$2d7895e0$8869c1a0$@bossart@linux.intel.com> 2 siblings, 0 replies; 13+ messages in thread From: Pierre-Louis Bossart @ 2011-09-21 13:20 UTC (permalink / raw) To: 'David Henningsson' Cc: 'Takashi Iwai', alsa-devel, 'Wu Fengguang', 'Stephen Warren' > 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 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. > 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 -Pierre ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <000001cc7861$2d7895e0$8869c1a0$@bossart@linux.intel.com>]
* Re: link between HDMI ELD and PCM devices [not found] ` <000001cc7861$2d7895e0$8869c1a0$@bossart@linux.intel.com> @ 2011-09-21 13:55 ` David Henningsson 2011-09-21 14:32 ` Pierre-Louis Bossart ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: David Henningsson @ 2011-09-21 13:55 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: 'Takashi Iwai', alsa-devel, 'Wu Fengguang', 'Stephen Warren' 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.) -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: link between HDMI ELD and PCM devices 2011-09-21 13:55 ` David Henningsson @ 2011-09-21 14:32 ` Pierre-Louis Bossart 2011-09-21 15:11 ` Stephen Warren 2011-09-26 10:43 ` Takashi Iwai 2 siblings, 0 replies; 13+ messages in thread From: Pierre-Louis Bossart @ 2011-09-21 14:32 UTC (permalink / raw) To: 'David Henningsson' Cc: 'Takashi Iwai', alsa-devel, Wu, Fengguang, 'Stephen Warren' > > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: link between HDMI ELD and PCM devices 2011-09-21 13:55 ` David Henningsson 2011-09-21 14:32 ` Pierre-Louis Bossart @ 2011-09-21 15:11 ` Stephen Warren 2011-09-21 17:30 ` Pierre-Louis Bossart 2011-09-26 10:43 ` Takashi Iwai 2 siblings, 1 reply; 13+ messages in thread From: Stephen Warren @ 2011-09-21 15:11 UTC (permalink / raw) To: David Henningsson, Pierre-Louis Bossart Cc: 'Takashi Iwai', alsa-devel@alsa-project.org, 'Wu Fengguang' 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 -- nvpublic ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: link between HDMI ELD and PCM devices 2011-09-21 15:11 ` Stephen Warren @ 2011-09-21 17:30 ` Pierre-Louis Bossart 2011-09-21 18:29 ` Stephen Warren 0 siblings, 1 reply; 13+ messages in thread From: Pierre-Louis Bossart @ 2011-09-21 17:30 UTC (permalink / raw) To: Stephen Warren Cc: 'Takashi Iwai', alsa-devel@alsa-project.org, 'Wu Fengguang', David Henningsson [-- Attachment #1: Type: text/plain, Size: 1071 bytes --] > 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! [-- Attachment #2: 0001-ALSA-hdmi-expose-ELD-control.patch --] [-- Type: text/x-patch, Size: 4645 bytes --] >From 01320aa954c7968db0d026c0643a0238df590804 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Date: Tue, 20 Sep 2011 20:15:48 -0500 Subject: [PATCH] ALSA: hdmi: expose ELD control Applications may want to read ELD information to understand what codecs are supported on the HDMI receiver and handle the a-v delay for better lip-sync. ELD information is exposed in a device-specific IFACE_PCM kcontrol. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- sound/pci/hda/hda_eld.c | 9 ++--- sound/pci/hda/hda_local.h | 2 + sound/pci/hda/patch_hdmi.c | 85 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 5 deletions(-) diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c index 28ce17d..8f16b20 100644 --- 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) { snd_printd(KERN_INFO "HDMI: invalid ELD buf size %d\n", size); return -ERANGE; } - buf = kmalloc(size, GFP_KERNEL); - if (!buf) - return -ENOMEM; + /* update info */ + eld->eld_size = size; + buf = eld->eld_buffer; for (i = 0; i < size; i++) buf[i] = hdmi_get_eld_byte(codec, nid, i); ret = hdmi_update_eld(eld, buf, size); - kfree(buf); return ret; } diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 9ed4b0d..cd579a6 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -609,6 +609,7 @@ struct cea_sad { }; #define ELD_FIXED_BYTES 20 +#define ELD_MAX_SIZE 256 #define ELD_MAX_MNL 16 #define ELD_MAX_SAD 16 @@ -633,6 +634,7 @@ struct hdmi_eld { int spk_alloc; int sad_count; struct cea_sad sad[ELD_MAX_SAD]; + char eld_buffer[ELD_MAX_SIZE]; #ifdef CONFIG_PROC_FS struct snd_info_entry *proc_entry; #endif diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 19cb72d..5ea9b55 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -324,6 +324,83 @@ static int cvt_nid_to_cvt_index(struct hdmi_spec *spec, hda_nid_t cvt_nid) return -EINVAL; } +static int hdmi_eld_ctl_info(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_info *uinfo) +{ + struct hda_codec *codec = snd_kcontrol_chip(kcontrol); + struct hdmi_spec *spec; + struct hdmi_spec_per_pin *per_pin; + struct hdmi_eld *eld; + int pin_idx; + + uinfo->type = SNDRV_CTL_ELEM_TYPE_BYTES; + pin_idx = kcontrol->private_value; + spec = codec->spec; + + if (pin_idx < 0) { + uinfo->count = 0; + } else { + per_pin = &spec->pins[pin_idx]; + eld = &per_pin->sink_eld; + uinfo->count = eld->eld_size; + } + return 0; +} + +static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct hda_codec *codec = snd_kcontrol_chip(kcontrol); + struct hdmi_spec *spec; + struct hdmi_spec_per_pin *per_pin; + struct hdmi_eld *eld; + int pin_idx; + + spec = codec->spec; + pin_idx = kcontrol->private_value; + + 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); + } + return 0; +} + +static struct snd_kcontrol_new eld_bytes_ctl = { + .iface = SNDRV_CTL_ELEM_IFACE_PCM, + .name = "ELD", + .info = hdmi_eld_ctl_info, + .get = hdmi_eld_ctl_get, +}; + +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; + + kctl = snd_ctl_new1(&eld_bytes_ctl, codec); + if (!kctl) + return -ENOMEM; + kctl->private_value = pin_idx; + kctl->id.device = device; + + err = snd_hda_ctl_add(codec, spec->pins[pin_idx].pin_nid, kctl); + if (err < 0) + return err; + + return 0; +} + #ifdef BE_PARANOID static void hdmi_get_dip_index(struct hda_codec *codec, hda_nid_t pin_nid, int *packet_index, int *byte_index) @@ -1176,6 +1253,14 @@ static int generic_hdmi_build_controls(struct hda_codec *codec) if (err < 0) return err; snd_hda_spdif_ctls_unassign(codec, pin_idx); + + /* add control for ELD Bytes */ + err = hdmi_create_eld_ctl(codec, + pin_idx, + spec->pcm_rec[pin_idx].device); + + if (err < 0) + return err; } return 0; -- 1.7.6.2 [-- Attachment #3: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: link between HDMI ELD and PCM devices 2011-09-21 17:30 ` Pierre-Louis Bossart @ 2011-09-21 18:29 ` Stephen Warren 2011-09-21 18:59 ` Pierre-Louis Bossart 0 siblings, 1 reply; 13+ messages in thread From: Stephen Warren @ 2011-09-21 18:29 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: 'Takashi Iwai', alsa-devel@alsa-project.org, 'Wu Fengguang', David Henningsson 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... -- nvpublic ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: link between HDMI ELD and PCM devices 2011-09-21 18:29 ` Stephen Warren @ 2011-09-21 18:59 ` Pierre-Louis Bossart 0 siblings, 0 replies; 13+ messages in thread From: Pierre-Louis Bossart @ 2011-09-21 18:59 UTC (permalink / raw) To: Stephen Warren Cc: 'Takashi Iwai', alsa-devel@alsa-project.org, 'Wu Fengguang', David Henningsson > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: link between HDMI ELD and PCM devices 2011-09-21 13:55 ` David Henningsson 2011-09-21 14:32 ` Pierre-Louis Bossart 2011-09-21 15:11 ` Stephen Warren @ 2011-09-26 10:43 ` Takashi Iwai 2 siblings, 0 replies; 13+ messages in thread From: Takashi Iwai @ 2011-09-26 10:43 UTC (permalink / raw) To: David Henningsson Cc: alsa-devel, 'Wu Fengguang', 'Stephen Warren', Pierre-Louis Bossart 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-09-26 10:43 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-20 23:12 link between HDMI ELD and PCM devices Pierre-Louis Bossart
2011-09-20 23:33 ` Stephen Warren
2011-09-21 0:58 ` Pierre-Louis Bossart
2011-09-21 6:44 ` David Henningsson
2011-09-21 8:33 ` Takashi Iwai
2011-09-21 13:20 ` Pierre-Louis Bossart
[not found] ` <000001cc7861$2d7895e0$8869c1a0$@bossart@linux.intel.com>
2011-09-21 13:55 ` David Henningsson
2011-09-21 14:32 ` Pierre-Louis Bossart
2011-09-21 15:11 ` Stephen Warren
2011-09-21 17:30 ` Pierre-Louis Bossart
2011-09-21 18:29 ` Stephen Warren
2011-09-21 18:59 ` Pierre-Louis Bossart
2011-09-26 10:43 ` Takashi Iwai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.