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