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

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