All of lore.kernel.org
 help / color / mirror / Atom feed
* 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.