[alsa-devel] [PATCH v2] 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. 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) { 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; }
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); + + 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); + + 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); + + 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) @@ -1193,6 +1273,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
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
Thanks for your review Takashi!
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.
Yes, I wasn't sure what to do here and expected some comments. Should the strategy be that if there's a single error we discard the entire ELD by setting the size to zero? No issues for the other changes.
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.
The intent was to read the ELD after the device is opened, I don't think the ELD can change during playback, the updates can only happen on a pin-sensing event. I can't think of a good reason why a notification would be needed? -Pierre
participants (2)
-
Pierre-Louis Bossart
-
Takashi Iwai