[alsa-devel] [PATCH v2] ALSA: hdmi: expose ELD control

[Takashi Iwai]

At Mon, 26 Sep 2011 17:48:43 -0500,
Pierre-Louis Bossart wrote:
> 
> 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. Tested both with amixer and
> PulseAudio.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> ---
>  sound/pci/hda/hda_eld.c    |    9 ++--
>  sound/pci/hda/hda_local.h  |    2 +
>  sound/pci/hda/patch_hdmi.c |   88 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 94 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
> index c34f730..9bf0d8b 100644
> --- a/sound/pci/hda/hda_eld.c
> +++ b/sound/pci/hda/hda_eld.c
> @@ -327,14 +327,14 @@ 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) {

The check of size > PAGE_SIZE is meaningless now.  Let's drop.


>  		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++) {
>  		unsigned int val = hdmi_get_eld_data(codec, nid, i);
> @@ -356,7 +356,6 @@ int snd_hdmi_get_eld(struct hdmi_eld *eld,
>  	ret = hdmi_update_eld(eld, buf, size);
> 
>  error:
> -	kfree(buf);
>  	return ret;
>  }

Wouldn't it be better to strip the buffer data and size in the error
case?  For example, radeon driver gives often size=0.  Then it's
translated as size=128 for a workaround, then again handled as an
error by checking the version.


> diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
> index aaefa7c..04d730f 100644
> --- a/sound/pci/hda/hda_local.h
> +++ b/sound/pci/hda/hda_local.h
> @@ -621,6 +621,7 @@ struct cea_sad {
>  };
> 
>  #define ELD_FIXED_BYTES	20
> +#define ELD_MAX_SIZE    256
>  #define ELD_MAX_MNL	16
>  #define ELD_MAX_SAD	16
> 
> @@ -645,6 +646,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 3f1f6ac..944e0cd 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -324,6 +324,86 @@ 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;
> +
> +	spec = codec->spec;
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_BYTES;
> +
> +	pin_idx = kcontrol->private_value;
> +	snd_BUG_ON(pin_idx < 0);

snd_BUG_ON() is essentially a WARN_ON(), so you should return an error
here like:
	if (snd_BUG_ON(pin_idx < 0))
		return -EINVAL;
> +
> +	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;
> +	int size;
> +
> +	spec = codec->spec;
> +	pin_idx = kcontrol->private_value;
> +
> +	snd_BUG_ON(pin_idx < 0);

Ditto.

> +	memset(ucontrol->value.bytes.data, 0, ELD_MAX_SIZE);
> +	per_pin = &spec->pins[pin_idx];
> +	eld = &per_pin->sink_eld;
> +	size = eld->eld_size;
> +
> +	memcpy(ucontrol->value.bytes.data, eld->eld_buffer, size);
> +
> +	/* reset rest of control */
> +	memset(&ucontrol->value.bytes.data[size], 0, ELD_MAX_SIZE-size);

This is superfluous.

> +	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,

Set the access field to SNDRV_CTL_ELEM_ACCESS_READ.
Also, unless we implement a notification, better to set
SNDRV_CTL_ELEM_ACCESS_VOLATILE flag, too.


thanks,

Takashi