[alsa-devel] link between HDMI ELD and PCM devices

[Stephen Warren]

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 at 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 at 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