[alsa-devel] [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control
Takashi Sakamoto
o-takashi at sakamocchi.jp
Mon Dec 12 13:03:15 CET 2016
On 2016年12月12日 18:38, Arnaud Pouliquen wrote:
>>> + */
>>> +static struct hdmi_codec_cea_spk_alloc hdmi_codec_channel_alloc[] = {
>>> +/* channel: 7 6 5 4 3 2 1 0 */
>>> +{ .ca_index = 0x00, .speakers = { 0, 0, 0, 0, 0, 0, FR, FL } },
>>> + /* 2.1 */
>>> +{ .ca_index = 0x01, .speakers = { 0, 0, 0, 0, 0, LFE, FR, FL } },
>>> + /* Dolby Surround */
>>> +{ .ca_index = 0x02, .speakers = { 0, 0, 0, 0, FC, 0, FR, FL } },
>>> + /* surround51 */
>>> +{ .ca_index = 0x0b, .speakers = { 0, 0, RR, RL, FC, LFE, FR, FL } },
>>> + /* surround40 */
>>> +{ .ca_index = 0x08, .speakers = { 0, 0, RR, RL, 0, 0, FR, FL } },
>>> + /* surround41 */
>>> +{ .ca_index = 0x09, .speakers = { 0, 0, RR, RL, 0, LFE, FR, FL } },
>>> + /* surround50 */
>>> +{ .ca_index = 0x0a, .speakers = { 0, 0, RR, RL, FC, 0, FR, FL } },
>>> + /* 6.1 */
>>> +{ .ca_index = 0x0f, .speakers = { 0, RC, RR, RL, FC, LFE, FR, FL } },
>>> + /* surround71 */
>>> +{ .ca_index = 0x13, .speakers = { RRC, RLC, RR, RL, FC, LFE, FR, FL } },
>>> +
>>> +{ .ca_index = 0x03, .speakers = { 0, 0, 0, 0, FC, LFE, FR, FL } },
>>> +{ .ca_index = 0x04, .speakers = { 0, 0, 0, RC, 0, 0, FR, FL } },
>>> +{ .ca_index = 0x05, .speakers = { 0, 0, 0, RC, 0, LFE, FR, FL } },
>>> +{ .ca_index = 0x06, .speakers = { 0, 0, 0, RC, FC, 0, FR, FL } },
>>> +{ .ca_index = 0x07, .speakers = { 0, 0, 0, RC, FC, LFE, FR, FL } },
>>> +{ .ca_index = 0x0c, .speakers = { 0, RC, RR, RL, 0, 0, FR, FL } },
>>> +{ .ca_index = 0x0d, .speakers = { 0, RC, RR, RL, 0, LFE, FR, FL } },
>>> +{ .ca_index = 0x0e, .speakers = { 0, RC, RR, RL, FC, 0, FR, FL } },
>>> +{ .ca_index = 0x10, .speakers = { RRC, RLC, RR, RL, 0, 0, FR, FL } },
>>> +{ .ca_index = 0x11, .speakers = { RRC, RLC, RR, RL, 0, LFE, FR, FL } },
>>> +{ .ca_index = 0x12, .speakers = { RRC, RLC, RR, RL, FC, 0, FR, FL } },
>>> +{ .ca_index = 0x14, .speakers = { FRC, FLC, 0, 0, 0, 0, FR, FL } },
>>> +{ .ca_index = 0x15, .speakers = { FRC, FLC, 0, 0, 0, LFE, FR, FL } },
>>> +{ .ca_index = 0x16, .speakers = { FRC, FLC, 0, 0, FC, 0, FR, FL } },
>>> +{ .ca_index = 0x17, .speakers = { FRC, FLC, 0, 0, FC, LFE, FR, FL } },
>>> +{ .ca_index = 0x18, .speakers = { FRC, FLC, 0, RC, 0, 0, FR, FL } },
>>> +{ .ca_index = 0x19, .speakers = { FRC, FLC, 0, RC, 0, LFE, FR, FL } },
>>> +{ .ca_index = 0x1a, .speakers = { FRC, FLC, 0, RC, FC, 0, FR, FL } },
>>> +{ .ca_index = 0x1b, .speakers = { FRC, FLC, 0, RC, FC, LFE, FR, FL } },
>>> +{ .ca_index = 0x1c, .speakers = { FRC, FLC, RR, RL, 0, 0, FR, FL } },
>>> +{ .ca_index = 0x1d, .speakers = { FRC, FLC, RR, RL, 0, LFE, FR, FL } },
>>> +{ .ca_index = 0x1e, .speakers = { FRC, FLC, RR, RL, FC, 0, FR, FL } },
>>> +{ .ca_index = 0x1f, .speakers = { FRC, FLC, RR, RL, FC, LFE, FR, FL } },
>>> +};
>>
>> Ditto.
> Sorry not understand this comment vs the previous one, could you please
> clarify?
This table is invariant in lifetime of the storage object, as well.
Let's put into .rodata section, too.
>>> +static int hdmi_codec_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag,
>>> + unsigned int size, unsigned int __user *tlv)
>>> +{
>>> + struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
>>> + struct hdmi_codec_priv *hcp = snd_soc_component_get_drvdata(component);
>>> + unsigned int __user *dst;
>>> + int chs, count = 0;
>>> + int num_ca = ARRAY_SIZE(hdmi_codec_channel_alloc);
>>> + unsigned long max_chs;
>>> + int spk_alloc, spk_mask;
>>> +
>>> + if (size < 8)
>>> + return -ENOMEM;
>>> +
>>> + if (put_user(SNDRV_CTL_TLVT_CONTAINER, tlv))
>>> + return -EFAULT;
>>> + size -= 8;
>>> + dst = tlv + 2;
>>> +
>>> + spk_alloc = drm_eld_get_spk_alloc(hcp->eld);
>>> + spk_mask = hdmi_codec_spk_mask_from_alloc(spk_alloc);
>>> +
>>> + max_chs = hweight_long(spk_mask);
>>> +
>>> + for (chs = 2; chs <= max_chs; chs++) {
>>> + int i;
>>> + struct hdmi_codec_cea_spk_alloc *cap;
>>> +
>>> + cap = hdmi_codec_channel_alloc;
>>> + for (i = 0; i < num_ca; i++, cap++) {
>>> + int chs_bytes = chs * 4;
>>> + unsigned int tlv_chmap[HDMI_MAX_SPEAKERS];
>>> +
>>> + if (cap->channels != chs)
>>> + continue;
>>> +
>>> + if (!(cap->spk_mask == (spk_mask & cap->spk_mask)))
>>> + continue;
>>> +
> Seems missing check here, related question below, in answer to your comment
> if (size < 8)
> return -ENOMEM;
>>> + /*
>>> + * Channel mapping is fixed as hdmi codec capability
>>> + * is not know.
>>> + */
>>> + if (put_user(SNDRV_CTL_TLVT_CHMAP_FIXED, dst) ||
>>> + put_user(chs_bytes, dst + 1))
>>> + return -EFAULT;
>>> +
>>> + dst += 2;
>>> + size -= 8;
>>> + count += 8;
>>> +
>>> + if (size < chs_bytes)
>>> + return -ENOMEM;
>>> +
>>> + size -= chs_bytes;
>>> + count += chs_bytes;
>>> + hdmi_cea_alloc_to_tlv_chmap(cap, tlv_chmap);
>>> +
>>> + if (copy_to_user(dst, tlv_chmap, chs_bytes))
>>> + return -EFAULT;
>>> + dst += chs;
>>> + }
>>> + }
>>> +
>>> + if (put_user(count, tlv + 1))
>>> + return -EFAULT;
>>> +
>>> + return 0;
>>> +}
>>> +
>>
>> This function has a bug to cause buffer-over-run in user space because
>> applications can request with a small buffer.
> "size" is used for this, the bug is that a check is missing (the one i
> added in comment above), or i missed something else?
You already realize my indication correctly, good.
I note that we can find similar codes in 'sound/core/control.c'.
>>> static const struct snd_kcontrol_new hdmi_controls[] = {
>>> {
>>> .access = SNDRV_CTL_ELEM_ACCESS_READ |
>>> @@ -79,6 +400,17 @@ static const struct snd_kcontrol_new hdmi_controls[] = {
>>> .info = hdmi_eld_ctl_info,
>>> .get = hdmi_eld_ctl_get,
>>> },
>>> + {
>>> + .access = SNDRV_CTL_ELEM_ACCESS_READ |
>>> + SNDRV_CTL_ELEM_ACCESS_TLV_READ |
>>> + SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK |
>>> + SNDRV_CTL_ELEM_ACCESS_VOLATILE,
>>> + .iface = SNDRV_CTL_ELEM_IFACE_PCM,
>>> + .name = "Playback Channel Map",
>>> + .info = hdmi_codec_chmap_ctl_info,
>>> + .get = hdmi_codec_chmap_ctl_get,
>>> + .tlv.c = hdmi_codec_chmap_ctl_tlv,
>>> + },
>>> };
>>
>> If you can keep the same interface for applications as
>> 'snd_pcm_add_chmap_ctls()' have, it's better to integrate the function
>> to have different tables/callbacks depending on drivers.
>>
> I had a look before define it. Unfortunately i cannot use
> snd_pcm_add_chmap_ctls. snd_pcm_add_chmap_ctls requests "snd_pcm"
> structure as input param. ASoC codec not aware of it.
> i could add an helper for ASoC, but hdmi-codec should be used for HDMI
> drivers and i'm not sure that there is another need in ASoC.
For example, splitting the function to two parts; one gets the parameter
for pcm instance, another deal with adding control element sets. Symbols
of both functions are going to be exported and your code can reuse the
latter.
My motivation for this idea is to use the same code for control element
sets with the same name of 'Playback Channel Map'. In this case, usage
of the same code brings us an merit. We can produce the consist way for
applications to handle the control element sets, even if long term
development brings much code changes. It has an advantage to reduce
maintaining effort.
This is merely my initial idea. If it's impossible, your idea is preferable.
Regards
Takashi Sakamoto
More information about the Alsa-devel
mailing list