[PATCH 2/2] ASoC: hdmi-codec: add channel mapping control
Arnaud Pouliquen
arnaud.pouliquen at st.com
Mon Dec 12 09:38:45 UTC 2016
On 12/11/2016 07:09 AM, Takashi Sakamoto wrote:
> On Dec 9 2016 01:37, Arnaud Pouliquen wrote:
>> Add user interface to provide channel mapping.
>> In a first step this control is read only.
>>
>> As TLV type, the control provides all configurations available for
>> HDMI sink(ELD), and provides current channel mapping selected by codec
>> based on ELD and number of channels specified by user on open.
>> When control is called before the number of the channel is specified
>> (i.e. hw_params is set), it returns all channels set to UNKNOWN.
>>
>> Notice that SNDRV_CTL_TLVT_CHMAP_FIXED is used for all mappings,
>> as no information is available from HDMI driver to allow channel swapping.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen at st.com>
>> ---
>> sound/soc/codecs/hdmi-codec.c | 346 +++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 345 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
>> index f27d115..0cb83a3 100644
>> --- a/sound/soc/codecs/hdmi-codec.c
>> +++ b/sound/soc/codecs/hdmi-codec.c
>> @@ -18,12 +18,137 @@
>> #include <sound/pcm.h>
>> #include <sound/pcm_params.h>
>> #include <sound/soc.h>
>> +#include <sound/tlv.h>
>> #include <sound/pcm_drm_eld.h>
>> #include <sound/hdmi-codec.h>
>> #include <sound/pcm_iec958.h>
>>
>> #include <drm/drm_crtc.h> /* This is only to get MAX_ELD_BYTES */
>>
>> +#define HDMI_MAX_SPEAKERS 8
>> +
>> +/*
>> + * CEA speaker placement for HDMI 1.4:
>> + *
>> + * FL FLC FC FRC FR FRW
>> + *
>> + * LFE
>> + *
>> + * RL RLC RC RRC RR
>> + *
>> + * Speaker placement has to be extended to support HDMI 2.0
>> + */
>> +enum hdmi_codec_cea_spk_placement {
>> + FL = (1 << 0), /* Front Left */
>> + FC = (1 << 1), /* Front Center */
>> + FR = (1 << 2), /* Front Right */
>> + FLC = (1 << 3), /* Front Left Center */
>> + FRC = (1 << 4), /* Front Right Center */
>> + RL = (1 << 5), /* Rear Left */
>> + RC = (1 << 6), /* Rear Center */
>> + RR = (1 << 7), /* Rear Right */
>> + RLC = (1 << 8), /* Rear Left Center */
>> + RRC = (1 << 9), /* Rear Right Center */
>> + LFE = (1 << 10), /* Low Frequency Effect */
>> +};
>
> BIT() macro in "linux/bitops.h" is available.
will be corrected in a v2
>
>> +
>> +/*
>> + * ELD Speaker allocation bits in the CEA Speaker Allocation data block
>> + */
>> +static int hdmi_codec_eld_spk_alloc_bits[] = {
>> + [0] = FL | FR,
>> + [1] = LFE,
>> + [2] = FC,
>> + [3] = RL | RR,
>> + [4] = RC,
>> + [5] = FLC | FRC,
>> + [6] = RLC | RRC,
>> +};
>
> Please put this kind of invariant table into .rodata section with
> 'const' modifier.
will be corrected in a v2
>
>> +
>> +struct hdmi_codec_channel_map_table {
>> + unsigned char map; /* ALSA API channel map position */
>> + int spk_mask; /* speaker position bit mask */
>> +};
>> +
>> +static struct hdmi_codec_channel_map_table hdmi_codec_map_table[] = {
>> + { SNDRV_CHMAP_FL, FL },
>> + { SNDRV_CHMAP_FR, FR },
>> + { SNDRV_CHMAP_RL, RL },
>> + { SNDRV_CHMAP_RR, RR },
>> + { SNDRV_CHMAP_LFE, LFE },
>> + { SNDRV_CHMAP_FC, FC },
>> + { SNDRV_CHMAP_RLC, RLC },
>> + { SNDRV_CHMAP_RRC, RRC },
>> + { SNDRV_CHMAP_RC, RC },
>> + { SNDRV_CHMAP_FLC, FLC },
>> + { SNDRV_CHMAP_FRC, FRC },
>> + {} /* terminator */
>> +};
>
> In this case, the table can be put into snd_hdac_spk_to_chmap().
will be corrected in a v2
>
>> +
>> +/*
>> + * cea Speaker allocation structure
>> + */
>> +struct hdmi_codec_cea_spk_alloc {
>> + int ca_index;
>> + int speakers[HDMI_MAX_SPEAKERS];
>> +
>> + /* Derived values, computed during init */
>> + int channels;
>> + int spk_mask;
>> + int spk_na_mask;
>> +};
>> +
>> +/*
>> + * This is an ordered list!
>> + *
>> + * The preceding ones have better chances to be selected by
>> + * hdmi_channel_allocation().
>
> The function is not defined and implemented. It takes developers to be
> puzzled.
will be corrected in a v2
>
>> + */
>> +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?
>
>> +
>> struct hdmi_codec_priv {
>> struct hdmi_codec_pdata hcd;
>> struct snd_soc_dai_driver *daidrv;
>> @@ -32,6 +157,7 @@ struct hdmi_codec_priv {
>> struct snd_pcm_substream *current_stream;
>> struct snd_pcm_hw_constraint_list ratec;
>> uint8_t eld[MAX_ELD_BYTES];
>> + unsigned int chmap[HDMI_MAX_SPEAKERS];
>> };
>>
>> static const struct snd_soc_dapm_widget hdmi_widgets[] = {
>> @@ -70,6 +196,201 @@ static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol,
>> return 0;
>> }
>>
>> +static int hdmi_codec_chmap_ctl_info(struct snd_kcontrol *kcontrol,
>> + struct snd_ctl_elem_info *uinfo)
>> +{
>> + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
>> + uinfo->count = HDMI_MAX_SPEAKERS;
>> + uinfo->value.integer.min = 0;
>> + uinfo->value.integer.max = SNDRV_CHMAP_LAST;
>> +
>> + return 0;
>> +}
>> +
>> +static int hdmi_codec_spk_mask_from_alloc(int spk_alloc)
>> +{
>> + int i;
>> + int spk_mask = hdmi_codec_eld_spk_alloc_bits[0];
>> +
>> + for (i = 0; i < ARRAY_SIZE(hdmi_codec_eld_spk_alloc_bits); i++) {
>> + if (spk_alloc & (1 << i))
>> + spk_mask |= hdmi_codec_eld_spk_alloc_bits[i];
>> + }
>> +
>> + return spk_mask;
>> +}
>> +
>> +static int hdmi_codec_chmap_ctl_get(struct snd_kcontrol *kcontrol,
>> + struct snd_ctl_elem_value *ucontrol)
>> +{
>> + struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
>> + struct hdmi_codec_priv *hcp = snd_soc_component_get_drvdata(component);
>> + int i;
>> +
>> + memset(ucontrol->value.integer.value, 0,
>> + sizeof(ucontrol->value.integer.value));
>> +
>> + mutex_lock(&hcp->current_stream_lock);
>> + if (hcp->current_stream)
>> + for (i = 0; i < HDMI_MAX_SPEAKERS; i++)
>> + ucontrol->value.integer.value[i] = hcp->chmap[i];
>> +
>> + mutex_unlock(&hcp->current_stream_lock);
>> +
>> + return 0;
>> +}
>> +
>> +/* From speaker bit mask to ALSA API channel position */
>> +static int snd_hdac_spk_to_chmap(int spk)
>> +{
>> + struct hdmi_codec_channel_map_table *t = hdmi_codec_map_table;
>> +
>> + for (; t->map; t++) {
>> + if (t->spk_mask == spk)
>> + return t->map;
>> + }
>> +
>> + return 0;
>> +}
>
> Why hdac? Are there some relationship between HDA controller and table
> you added?
No relation ship just a stupid guy who does a copy/past from hda without
renaming...
>
>> +/**
>> + * hdmi_codec_cea_init_channel_alloc:
>> + * Compute derived values in hdmi_codec_channel_alloc[].
>> + * spk_na_mask is used to store unused channels in mid of the channel
>> + * allocations. These particular channels are then considered as active channels
>> + * For instance:
>> + * CA_ID 0x02: CA = (FL, FR, 0, FC) => spk_na_mask = 0x04, channels = 4
>> + * CA_ID 0x04: CA = (FL, FR, 0, 0, RC) => spk_na_mask = 0x03C, channels = 5
>> + */
>> +static void hdmi_codec_cea_init_channel_alloc(void)
>> +{
>> + int i, j, k, last;
>> + struct hdmi_codec_cea_spk_alloc *p;
>> +
>> + for (i = 0; i < ARRAY_SIZE(hdmi_codec_channel_alloc); i++) {
>> + p = hdmi_codec_channel_alloc + i;
>> + p->spk_mask = 0;
>> + p->spk_na_mask = 0;
>> + last = HDMI_MAX_SPEAKERS;
>> + for (j = 0, k = 7; j < HDMI_MAX_SPEAKERS; j++, k--) {
>> + if (p->speakers[j]) {
>> + p->spk_mask |= p->speakers[j];
>> + if (last == HDMI_MAX_SPEAKERS)
>> + last = j;
>> + } else if (last != HDMI_MAX_SPEAKERS) {
>> + p->spk_na_mask |= 1 << k;
>> + }
>> + }
>> + p->channels = 8 - last;
>> + }
>> +}
>> +
>> +static int hdmi_codec_get_ch_alloc_table_idx(struct hdmi_codec_priv *hcp,
>> + unsigned char channels)
>> +{
>> + int i, spk_alloc, spk_mask;
>> + struct hdmi_codec_cea_spk_alloc *cap = hdmi_codec_channel_alloc;
>> +
>> + spk_alloc = drm_eld_get_spk_alloc(hcp->eld);
>> + spk_mask = hdmi_codec_spk_mask_from_alloc(spk_alloc);
>> +
>> + for (i = 0; i < ARRAY_SIZE(hdmi_codec_channel_alloc); i++, cap++) {
>> + if (cap->channels != channels)
>> + continue;
>> + if (!(cap->spk_mask == (spk_mask & cap->spk_mask)))
>> + continue;
>> + return i;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static void hdmi_cea_alloc_to_tlv_chmap(struct hdmi_codec_cea_spk_alloc *cap,
>> + unsigned int *chmap)
>> +{
>> + int count = 0;
>> + int c, spk;
>> +
>> + /* Detect unused channels in cea caps, tag them as N/A channel in TLV */
>> + for (c = 0; c < HDMI_MAX_SPEAKERS; c++) {
>> + spk = cap->speakers[7 - c];
>> + if (cap->spk_na_mask & BIT(c))
>> + chmap[count++] = SNDRV_CHMAP_NA;
>> + else
>> + chmap[count++] = snd_hdac_spk_to_chmap(spk);
>> + }
>> +}
>> +
>> +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?
>
>> 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.
Thanks and Regards
Arnaud
More information about the dri-devel
mailing list