[PATCH 0/2] Generic HDMI codec: Add channel mapping control

[PATCH 0/2] Generic HDMI codec: Add channel mapping control

[Arnaud Pouliquen]

Aim of this patch is to add  'Playback Channel Map' control to export 
audio capabilities in term of HDMI sink speakers allocation.
This patch follow discussion initiates here: 
[RFC] ASOC: HDMI audio info frame speaker allocation
http://www.spinics.net/lists/alsa-devel/msg57363.html

The code is fully inspired from HDA driver.
On hw_param, HDMI sink speaker capabilities are exported via TLV ops
and  a CEA allocation is choson, based on ELD information and the number of
channels requested by user.

Mains differences with HDA implementation are:
 - Control is read only
 - Channel swap is not supported. Consequence is that unused channel in
   the mid of CEA audio infoframe channel mapping are considered as
   active.
   example for channel allocation 0x02: FL, FR, 0, FC)
	This configuration is only available for a 4 channels stream.
  - Channel allocation table has been reordered and HDMI 2.0 is not
    supported.

Arnaud Pouliquen (2):
  DRM: add help to get ELD speaker allocation
  ASoC: hdmi-codec: add channel mapping control

 include/drm/drm_edid.h        |  13 ++
 sound/soc/codecs/hdmi-codec.c | 346 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 358 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH 1/2] DRM: add help to get ELD speaker allocation

[Arnaud Pouliquen]

Add helper to allow users to retrieve the speaker allocations without
knowledge of the ELD structure.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 include/drm/drm_edid.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 919933d..0706cc6 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -245,6 +245,7 @@ struct detailed_timing {
 # define DRM_ELD_AUD_SYNCH_DELAY_MAX	0xfa	/* 500 ms */
 
 #define DRM_ELD_SPEAKER			7
+# define DRM_ELD_SPEAKER_MASK		0x7f
 # define DRM_ELD_SPEAKER_RLRC		(1 << 6)
 # define DRM_ELD_SPEAKER_FLRC		(1 << 5)
 # define DRM_ELD_SPEAKER_RC		(1 << 4)
@@ -412,6 +413,18 @@ static inline int drm_eld_size(const uint8_t *eld)
 }
 
 /**
+ * drm_eld_get_spk_alloc - Get speaker allocation
+ * @eld: pointer to an ELD memory structure
+ *
+ * The returned value is the speakers mask. User has to use %DRM_ELD_SPEAKER
+ * field definitions to identify speakers.
+ */
+static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld)
+{
+	return eld[DRM_ELD_SPEAKER] & DRM_ELD_SPEAKER_MASK;
+}
+
+/**
  * drm_eld_get_conn_type - Get device type hdmi/dp connected
  * @eld: pointer to an ELD memory structure
  *
-- 
1.9.1

[PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

[Arnaud Pouliquen]

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@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 */
+};
+
+/*
+ * 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,
+};
+
+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 */
+};
+
+/*
+ * 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().
+ */
+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 } },
+};
+
 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;
+}
+
+/**
+ * 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;
+
+			/*
+			 * 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;
+}
+
 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,
+	},
 };
 
 static int hdmi_codec_new_stream(struct snd_pcm_substream *substream,
@@ -164,7 +496,7 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
 			.dig_subframe = { 0 },
 		}
 	};
-	int ret;
+	int ret, idx;
 
 	dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
 		params_width(params), params_rate(params),
@@ -191,6 +523,16 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
 	hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
 	hp.cea.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
 
+	/* Select a channel allocation that matches with ELD and pcm channels */
+	idx = hdmi_codec_get_ch_alloc_table_idx(hcp, hp.cea.channels);
+	if (idx < 0) {
+		dev_err(dai->dev, "Not able to map channels to speakers (%d)\n",
+			ret);
+		return idx;
+	}
+	hp.cea.channel_allocation = hdmi_codec_channel_alloc[idx].ca_index;
+	hdmi_cea_alloc_to_tlv_chmap(&hdmi_codec_channel_alloc[idx], hcp->chmap);
+
 	hp.sample_width = params_width(params);
 	hp.sample_rate = params_rate(params);
 	hp.channels = params_channels(params);
@@ -407,6 +749,8 @@ static int hdmi_codec_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	hdmi_codec_cea_init_channel_alloc();
+
 	dev_set_drvdata(dev, hcp);
 	return 0;
 }
-- 
1.9.1

Re: [PATCH 0/2] Generic HDMI codec: Add channel mapping control

[Takashi Sakamoto]

On Dec 9 2016 01:37, Arnaud Pouliquen wrote:
> Aim of this patch is to add  'Playback Channel Map' control to export
> audio capabilities in term of HDMI sink speakers allocation.
> This patch follow discussion initiates here:
> [RFC] ASOC: HDMI audio info frame speaker allocation
> http://www.spinics.net/lists/alsa-devel/msg57363.html
>
> The code is fully inspired from HDA driver.
> On hw_param, HDMI sink speaker capabilities are exported via TLV ops
> and  a CEA allocation is choson, based on ELD information and the number of
> channels requested by user.
>
> Mains differences with HDA implementation are:
>  - Control is read only
>  - Channel swap is not supported. Consequence is that unused channel in
>    the mid of CEA audio infoframe channel mapping are considered as
>    active.
>    example for channel allocation 0x02: FL, FR, 0, FC)
> 	This configuration is only available for a 4 channels stream.
>   - Channel allocation table has been reordered and HDMI 2.0 is not
>     supported.
>
> Arnaud Pouliquen (2):
>   DRM: add help to get ELD speaker allocation
>   ASoC: hdmi-codec: add channel mapping control
>
>  include/drm/drm_edid.h        |  13 ++
>  sound/soc/codecs/hdmi-codec.c | 346 +++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 358 insertions(+), 1 deletion(-)

Please pay enough attention to development cycle for Linux kernel.

We're mostly on the end of development for 4.9 cycle, and review process 
for new feature might be delay for next cycle, till 4.9 release, two 
weeks later.


Regards

Takashi Sakamoto

Re: [PATCH 0/2] Generic HDMI codec: Add channel mapping control

[Takashi Sakamoto]

On Dec 9 2016 05:52, Takashi Sakamoto wrote:
> On Dec 9 2016 01:37, Arnaud Pouliquen wrote:
>> Aim of this patch is to add  'Playback Channel Map' control to export
>> audio capabilities in term of HDMI sink speakers allocation.
>> This patch follow discussion initiates here:
>> [RFC] ASOC: HDMI audio info frame speaker allocation
>> http://www.spinics.net/lists/alsa-devel/msg57363.html
>>
>> The code is fully inspired from HDA driver.
>> On hw_param, HDMI sink speaker capabilities are exported via TLV ops
>> and  a CEA allocation is choson, based on ELD information and the
>> number of
>> channels requested by user.
>>
>> Mains differences with HDA implementation are:
>>  - Control is read only
>>  - Channel swap is not supported. Consequence is that unused channel in
>>    the mid of CEA audio infoframe channel mapping are considered as
>>    active.
>>    example for channel allocation 0x02: FL, FR, 0, FC)
>>     This configuration is only available for a 4 channels stream.
>>   - Channel allocation table has been reordered and HDMI 2.0 is not
>>     supported.
>>
>> Arnaud Pouliquen (2):
>>   DRM: add help to get ELD speaker allocation
>>   ASoC: hdmi-codec: add channel mapping control
>>
>>  include/drm/drm_edid.h        |  13 ++
>>  sound/soc/codecs/hdmi-codec.c | 346
>> +++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 358 insertions(+), 1 deletion(-)
>
> Please pay enough attention to development cycle for Linux kernel.
>
> We're mostly on the end of development for 4.9 cycle, and review process
> for new feature might be delay for next cycle, till 4.9 release, two
> weeks later.

Oops. I was stupid just after awakening...

Anyway, we're mostly on the end of development for 4.10 cycle, and the 
review process for new feature might be delay till 4.10-rc1 release, two 
weeks later. During the weeks. bug fixes are preferable to be applied.


Regards

Takashi Sakamoto
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/2] Generic HDMI codec: Add channel mapping control

[Arnaud Pouliquen]

Hi,

On 12/08/2016 10:13 PM, Takashi Sakamoto wrote:
> On Dec 9 2016 05:52, Takashi Sakamoto wrote:
>> On Dec 9 2016 01:37, Arnaud Pouliquen wrote:
>>> Aim of this patch is to add  'Playback Channel Map' control to export
>>> audio capabilities in term of HDMI sink speakers allocation.
>>> This patch follow discussion initiates here:
>>> [RFC] ASOC: HDMI audio info frame speaker allocation
>>> http://www.spinics.net/lists/alsa-devel/msg57363.html
>>>
>>> The code is fully inspired from HDA driver.
>>> On hw_param, HDMI sink speaker capabilities are exported via TLV ops
>>> and  a CEA allocation is choson, based on ELD information and the
>>> number of
>>> channels requested by user.
>>>
>>> Mains differences with HDA implementation are:
>>>  - Control is read only
>>>  - Channel swap is not supported. Consequence is that unused channel in
>>>    the mid of CEA audio infoframe channel mapping are considered as
>>>    active.
>>>    example for channel allocation 0x02: FL, FR, 0, FC)
>>>     This configuration is only available for a 4 channels stream.
>>>   - Channel allocation table has been reordered and HDMI 2.0 is not
>>>     supported.
>>>
>>> Arnaud Pouliquen (2):
>>>   DRM: add help to get ELD speaker allocation
>>>   ASoC: hdmi-codec: add channel mapping control
>>>
>>>  include/drm/drm_edid.h        |  13 ++
>>>  sound/soc/codecs/hdmi-codec.c | 346
>>> +++++++++++++++++++++++++++++++++++++++++-
>>>  2 files changed, 358 insertions(+), 1 deletion(-)
>>
>> Please pay enough attention to development cycle for Linux kernel.
>>
>> We're mostly on the end of development for 4.9 cycle, and review process
>> for new feature might be delay for next cycle, till 4.9 release, two
>> weeks later.
> 
> Oops. I was stupid just after awakening...
> 
> Anyway, we're mostly on the end of development for 4.10 cycle, and the
> review process for new feature might be delay till 4.10-rc1 release, two
> weeks later. During the weeks. bug fixes are preferable to be applied.

Thank you for warning me. No problem for the delay, i fully understand
that focus is on bug fixes.
Notice that it is quite difficult for developers to find the best timing
to address this kind of patch, as integration process is not always
simple to follow... this probably come with experience.

So if i well understand your remark the best windows to integrate a
feature for the version N+1, is to propose patch from N-rc1 tag.
(expecting that patch-set is enough mature to be integrated in N+1...)

Regards
Arnaud

Re: [PATCH 0/2] Generic HDMI codec: Add channel mapping control

[Takashi Sakamoto]

On Dec 9 2016 23:06, Arnaud Pouliquen wrote:
>> Anyway, we're mostly on the end of development for 4.10 cycle, and the
>> review process for new feature might be delay till 4.10-rc1 release, two
>> weeks later. During the weeks. bug fixes are preferable to be applied.
>
> Thank you for warning me. No problem for the delay, i fully understand
> that focus is on bug fixes.
> Notice that it is quite difficult for developers to find the best timing
> to address this kind of patch, as integration process is not always
> simple to follow... this probably come with experience.

ALSA is a development project for sound subsystem of Linux operating 
system, and our work is a part of Linux kernel. Therefore, it's better 
to follow generic behavior in Linux kernel development.

> So if i well understand your remark the best windows to integrate a
> feature for the version N+1, is to propose patch from N-rc1 tag.
> (expecting that patch-set is enough mature to be integrated in N+1...)

It's a better way if you're aware of maintainers' work and your task 
allows you to do things steadily.


Regards

Takashi Sakamoto

Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

[Takashi Sakamoto]

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

> +
> +/*
> + * 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.

> +
> +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().

> +
> +/*
> + * 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.

> + */
> +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.

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

> +/**
> + * 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;
> +
> +			/*
> +			 * 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.

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

>  static int hdmi_codec_new_stream(struct snd_pcm_substream *substream,
> @@ -164,7 +496,7 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
>  			.dig_subframe = { 0 },
>  		}
>  	};
> -	int ret;
> +	int ret, idx;
>
>  	dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
>  		params_width(params), params_rate(params),
> @@ -191,6 +523,16 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
>  	hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
>  	hp.cea.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
>
> +	/* Select a channel allocation that matches with ELD and pcm channels */
> +	idx = hdmi_codec_get_ch_alloc_table_idx(hcp, hp.cea.channels);
> +	if (idx < 0) {
> +		dev_err(dai->dev, "Not able to map channels to speakers (%d)\n",
> +			ret);
> +		return idx;
> +	}
> +	hp.cea.channel_allocation = hdmi_codec_channel_alloc[idx].ca_index;
> +	hdmi_cea_alloc_to_tlv_chmap(&hdmi_codec_channel_alloc[idx], hcp->chmap);
> +
>  	hp.sample_width = params_width(params);
>  	hp.sample_rate = params_rate(params);
>  	hp.channels = params_channels(params);
> @@ -407,6 +749,8 @@ static int hdmi_codec_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>
> +	hdmi_codec_cea_init_channel_alloc();
> +
>  	dev_set_drvdata(dev, hcp);
>  	return 0;
>  }


Regards

Takashi Sakamoto

Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

[Arnaud Pouliquen]

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

Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

[Takashi Iwai]

On Mon, 12 Dec 2016 10:38:45 +0100,
Arnaud Pouliquen wrote:
> 
> 
> 
> 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@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

One slight caution: BIT() expands to an unsigned long type.


Takashi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

[Takashi Sakamoto]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

[Takashi Sakamoto]

On Dec 12 2016 18:54, Takashi Iwai wrote:
>>>> +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
>
> One slight caution: BIT() expands to an unsigned long type.

Mmm, indeed. This is my wrong indication, sorry.
Thanks for your correction.


Regards

Takashi Sakamoto

Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

[Takashi Iwai]

On Mon, 12 Dec 2016 13:12:16 +0100,
Takashi Sakamoto wrote:
> 
> On Dec 12 2016 18:54, Takashi Iwai wrote:
> >>>> +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
> >
> > One slight caution: BIT() expands to an unsigned long type.
> 
> Mmm, indeed. This is my wrong indication, sorry.
> Thanks for your correction.

Well, it's not necessarily wrong.  My point is that it requires
caution sometimes, as it's not blindly convertible.
In short: it depends on the code.


Takashi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

[Arnaud Pouliquen]

On 12/12/2016 01:03 PM, Takashi Sakamoto wrote:
> 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.
>
This table is updated in hdmi_codec_cea_init_channel_alloc so can not be
constant. In theory i could declare all field instead of computing some.
But for lisibility, i would prefer to just declare ca_index  and
speakers allocation field in this table (i will declared both as const)

Regards,

Arnaud

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

[Takashi Sakamoto]

On Dec 12 2016 21:55, Takashi Iwai wrote:
> On Mon, 12 Dec 2016 13:12:16 +0100,
> Takashi Sakamoto wrote:
>>
>> On Dec 12 2016 18:54, Takashi Iwai wrote:
>>>>>> +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
>>>
>>> One slight caution: BIT() expands to an unsigned long type.
>>
>> Mmm, indeed. This is my wrong indication, sorry.
>> Thanks for your correction.
>
> Well, it's not necessarily wrong.  My point is that it requires
> caution sometimes, as it's not blindly convertible.
> In short: it depends on the code.

Hm. Here, I prefer to avoiding needless type-coversions, especially 
between 'signed' and 'unsigned'. In C semantics of enumerator 
specifiers, these values are handled as 'int' type. On the other hand, 
the BIT() macro has 'UL' suffix.

In short: carefulness.


Regards

Takashi Sakamoto

Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

[Arnaud Pouliquen]

On 12/12/2016 03:05 PM, Takashi Sakamoto wrote:
> On Dec 12 2016 21:55, Takashi Iwai wrote:
>> On Mon, 12 Dec 2016 13:12:16 +0100,
>> Takashi Sakamoto wrote:
>>>
>>> On Dec 12 2016 18:54, Takashi Iwai wrote:
>>>>>>> +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
>>>>
>>>> One slight caution: BIT() expands to an unsigned long type.
>>>
>>> Mmm, indeed. This is my wrong indication, sorry.
>>> Thanks for your correction.
>>
>> Well, it's not necessarily wrong.  My point is that it requires
>> caution sometimes, as it's not blindly convertible.
>> In short: it depends on the code.
> 
> Hm. Here, I prefer to avoiding needless type-coversions, especially 
> between 'signed' and 'unsigned'. In C semantics of enumerator 
> specifiers, these values are handled as 'int' type. On the other hand, 
> the BIT() macro has 'UL' suffix.
> 
> In short: carefulness.
I will have a look if reasonable or not to integrate use of BIT ...

Arnaud

Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

[Arnaud Pouliquen]

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

I checked you proposal. I see 2 blocking points:
- ASoC core set the control private_data field to snd_soc_component so
it is not possible to reuse pcm_chmap_ctl_get/set/tlv that request
snd_pcm_chmap type for the private_data

- The control is a PCM control, so for multi instances it could have to
migrate to the pcm_controls field if following patch is accepted:
[PATCH v5 0/2] ALSA controls management using index/device/sub-devices
fields (http://www.spinics.net/lists/alsa-devel/msg57639.html)
in this case it must be created by the ASoC core and not the driver itself

Alternative could be to treat the control in soc-core:
In soc_link_dai_pcm_controls that is created in patch-set mentioned above.

I have to improve and test it(dirty code for concept...) but this would
be something like that :

static int soc_link_dai_pcm_controls(struct snd_soc_dai **dais, int
num_dais,
				     struct snd_soc_pcm_runtime *rtd)
{
	struct snd_kcontrol_new kctl;
	int i, j, ret;

	for (i = 0; i < num_dais; ++i) {
		for (j = 0; j < dais[i]->driver->num_pcm_controls; j++) {
			kctl = dais[i]->driver->pcm_controls[j];

+			/* TODO test also capture */
+			if(!strcmp(kctl->name, "Playback Channel Map") {
+				ret = snd_pcm_add_chmap_ctls(rtd->pcm,
+				  SNDRV_PCM_STREAM_PLAYBACK,
+				  /* Not very clean: chmap pointer is stored in private_value */
+				  (const struct snd_pcm_chmap_elem *) kctl.private_value,
+			   	  dai->driver->playback.max_channels,
+				  0, NULL);
+				continue;
+			}
			if (kctl.iface != SNDRV_CTL_ELEM_IFACE_PCM ||
			    rtd->dai_link->no_pcm) {
				dev_err(dais[i]->dev,
					"ASoC: Failed to bind %s control: %s\n",
					dais[i]->name, kctl.name);
				return -EINVAL;
			}
Regards
Arnaud

Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

[Takashi Sakamoto]

Hi Arnaud,

On Dec 12 2016 22:46, Arnaud Pouliquen wrote:
> On 12/12/2016 01:03 PM, Takashi Sakamoto wrote:
>> 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.
>>
> This table is updated in hdmi_codec_cea_init_channel_alloc so can not be
> constant. In theory i could declare all field instead of computing some.
> But for lisibility, i would prefer to just declare ca_index  and
> speakers allocation field in this table (i will declared both as const)

You should pay enough attention to a case that one system has several 
GPUs to which relevant GPU drivers register HDMI_CODEC_DRV_NAME platform 
device. The 'static' modifier has an effect to keep just one storage 
object, thus your code causes bugs in the case.


Regards

Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

[Takashi Sakamoto]

On 2016年12月13日 22:23, Takashi Sakamoto wrote:
> Hi Arnaud,
>>> This table is invariant in lifetime of the storage object, as well.
>>> Let's put into .rodata section, too.
>>>
>> This table is updated in hdmi_codec_cea_init_channel_alloc so can not be
>> constant. In theory i could declare all field instead of computing some.
>> But for lisibility, i would prefer to just declare ca_index  and
>> speakers allocation field in this table (i will declared both as const)
>
> You should pay enough attention to a case that one system has several
> GPUs to which relevant GPU drivers register HDMI_CODEC_DRV_NAME platform
> device. The 'static' modifier has an effect to keep just one storage
> object, thus your code causes bugs in the case.

Oops, the bug is unrelated to the static modifier. The modifier is for 
reference scope. I'll correct as the file local symbol has just one 
storage object.

(I might be tired tonight...)


Regards

Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

[Arnaud Pouliquen]

On 12/13/2016 02:58 PM, Takashi Sakamoto wrote:
> On 2016年12月13日 22:23, Takashi Sakamoto wrote:
>> Hi Arnaud,
>>>> This table is invariant in lifetime of the storage object, as well.
>>>> Let's put into .rodata section, too.
>>>>
>>> This table is updated in hdmi_codec_cea_init_channel_alloc so can not be
>>> constant. In theory i could declare all field instead of computing some.
>>> But for lisibility, i would prefer to just declare ca_index  and
>>> speakers allocation field in this table (i will declared both as const)
>>
>> You should pay enough attention to a case that one system has several
>> GPUs to which relevant GPU drivers register HDMI_CODEC_DRV_NAME platform
>> device. The 'static' modifier has an effect to keep just one storage
>> object, thus your code causes bugs in the case.
> 
> Oops, the bug is unrelated to the static modifier. The modifier is for 
> reference scope. I'll correct as the file local symbol has just one 
> storage object.
> 
> (I might be tired tonight...)
> 
i understood in this way :-)
As 'ca_index' and "speakers" fiel are constants
the fields computed in hdmi_codec_cea_init_channel_alloc will
not change if the functions is called several time ( multi-instances).
but agree that it is not very clean, but this should work.
To avoid to compute several time, i can also add a test in
hdmi_codec_cea_init_channel_alloc to do it only one time (by testing
hdmi_codec_channel_alloc[0].channels), or perhaps better an atomic local
variable to avoid side effect for Multiprocessor archs.

Now the second approach is to define all the field. This indeeds a big
table (around 256 lines instead of 32 lines):

static const struct hdmi_codec_cea_spk_alloc hdmi_codec_channel_alloc[] = {
	{
		.ca_index = 0x00,
		.speakers = {   0,    0,   0,   0,   0,    0,  FR,  FL } },
		.channels = 2,
		.spk_mask = FR | FL,
		.spk_na_mask = 0x00,
	},
	{ /* 2.1 */
		.ca_index = 0x01,
		.speakers = {   0,    0,   0,   0,   0,  LFE,  FR,  FL } },
		.channels = 3,
		.spk_mask = FR | FL | LFE,
		.spk_na_mask = 0x00,
	},
	{ /* Dolby Surround */
		.ca_index = 0x02,
		.speakers = {   0,    0,   0,   0,  FC,    0,  FR,  FL } },
		.channels = 4,
		.spk_mask = FR | FL | FC,
		.spk_na_mask = BIT(2),
	},
[...]

	{
		.ca_index = 0x1f,
		.speakers = { FRC,  FLC,  RR,  RL,  FC,  LFE,  FR,  FL } },
		.channels = 8,
		.spk_mask = FRC | FLC | RR | RL | FC | LFE | FR | FL,
		.spk_na_mask = 0x0,
	},

I would prefer the first one to save lines, but i can implement the
second one (or another one if you have an alternative to propose).

Regards
Arnaud
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Applied "DRM: add help to get ELD speaker allocation" to the asoc tree

[Mark Brown]

The patch

   DRM: add help to get ELD speaker allocation

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From c82dbe5c055e4d246bd07c4d7b24801c9445c241 Mon Sep 17 00:00:00 2001
From: Arnaud Pouliquen <arnaud.pouliquen@st.com>
Date: Tue, 3 Jan 2017 16:52:50 +0100
Subject: [PATCH] DRM: add help to get ELD speaker allocation

Add helper to allow users to retrieve the speaker allocations without
knowledge of the ELD structure.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/drm/drm_edid.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index c3a7d440bc11..de93543d1218 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -248,6 +248,7 @@ struct detailed_timing {
 # define DRM_ELD_AUD_SYNCH_DELAY_MAX	0xfa	/* 500 ms */
 
 #define DRM_ELD_SPEAKER			7
+# define DRM_ELD_SPEAKER_MASK		0x7f
 # define DRM_ELD_SPEAKER_RLRC		(1 << 6)
 # define DRM_ELD_SPEAKER_FLRC		(1 << 5)
 # define DRM_ELD_SPEAKER_RC		(1 << 4)
@@ -415,6 +416,18 @@ static inline int drm_eld_size(const uint8_t *eld)
 }
 
 /**
+ * drm_eld_get_spk_alloc - Get speaker allocation
+ * @eld: pointer to an ELD memory structure
+ *
+ * The returned value is the speakers mask. User has to use %DRM_ELD_SPEAKER
+ * field definitions to identify speakers.
+ */
+static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld)
+{
+	return eld[DRM_ELD_SPEAKER] & DRM_ELD_SPEAKER_MASK;
+}
+
+/**
  * drm_eld_get_conn_type - Get device type hdmi/dp connected
  * @eld: pointer to an ELD memory structure
  *
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel