topology: Enhance support for private data

PATCH Applied [email protected] 5 participants 14 messages Jul 15, 2016 – Jul 19, 2016

lore

Share & tools

lore

[email protected]

author

2016-07-15 12:12 UTC #0 lore raw

v2 cover topology: Enhance support for private data

From: Mengdong Lin <[email protected]>

There is no ABI change in this series.

We finished verification on latest Intel platforms. There will be 4 or 5 series. Later series will come with some ABI update for topology. The size check on ABI objects in current ASoC kernel will detect possible ABI version mismatch between user space and kernel

History:
v2: fix compiler warnings. There are 2 warnings left in topology/pcm.c
that are not caused by this series and will be fixed in later patches.

Mengdong Lin (5):
topology: An element can refer to multipe data sections in text conf
file
topology: Merge lookup for data reference into tplg_copy_data()
topology: Change uuid value to 16 separate characters in text conf
file
topology: Parse vendor private data for manifest
topology: Tuple type can have an extenstion

 include/topology.h        |  69 ++++++++++-
 src/topology/ctl.c        |  52 +++-----
 src/topology/dapm.c       |  17 ++-
 src/topology/data.c       | 307 ++++++++++++++++++++++++++++++++++++++--------
 src/topology/elem.c       |   4 +
 src/topology/parser.c     |  28 ++++-
 src/topology/tplg_local.h |  15 ++-
 7 files changed, 395 insertions(+), 97 deletions(-)

Signature · 1 line

2.5.0

[email protected]

author

2016-07-15 12:12 UTC #1 lore raw

PATCH v2 1/5 topology: An element can refer to multipe data sections in text conf file

From: Mengdong Lin <[email protected]>

Previously in text conf file, an element can only refer to one data section (SectionData). Now it can also refer to a list of data sections. Thus users can split groups of firmware parameters to multiple data sections, and put them all in the reference list.

Finally, data of these data sections will be merged, in the same order as they are in the reference list, as the element's private data for kernel.

We still support the original syntax of reference to a single data section. The doc is updated for the syntax extension.

Signed-off-by Mengdong Lin <[email protected]>

include/topology.h +28 -0

index 9d57ce3..d666505 100644
@@ -213,6 +213,34 @@ extern "C" {
  * The keyword tuples is to define vendor specific tuples. Please refer to
  * section Vendor Tokens and Vendor tuples.
  *
+ * <h5>How to define an element with private data</h5>
+ * An element can refer to a single data section or multiple data
+ * sections.
+ *
+ * <h6>To refer to a single data section:</h6>
+ * <pre>
+ * Sectionxxx."element name" {
+ *    ...
+ *	data "name of data section"		# optional private data
+ * }
+ * </pre>
+ *
+ * <h6>To refer to multiple data sections:</h6>
+ * <pre>
+ * Sectionxxx."element name" {
+ *	...
+ *	data [						# optional private data
+ *		"name of 1st data section"
+ *		"name of 2nd data section"
+ *		...
+ *	]
+ * }
+ * </pre>
+ * And data of these sections will be merged in the same order as they are
+ * in the list, as the element's private data for kernel.
+ *
+ * </pre>
+ *
  *  <h6>Vendor Tokens</h6>
  * A vendor token list is defined as a new section. Each token element is
  * a pair of string ID and integer value. And both the ID and value are

src/topology/ctl.c +9 ~~-15~~

index b948ac0..7ded0a4 100644
@@ -447,11 +447,9 @@ int tplg_parse_control_bytes(snd_tplg_t *tplg,
 		}
 
 		if (strcmp(id, "data") == 0) {
-			if (snd_config_get_string(n, &val) < 0)
-				return -EINVAL;
-
-			tplg_ref_add(elem, SND_TPLG_TYPE_DATA, val);
-			tplg_dbg("\t%s: %s\n", id, val);
+			err = tplg_parse_data_refs(n, elem);
+			if (err < 0)
+				return err;
 			continue;
 		}

@@ -587,11 +585,9 @@ int tplg_parse_control_enum(snd_tplg_t *tplg, snd_config_t *cfg,
 		}
 
 		if (strcmp(id, "data") == 0) {
-			if (snd_config_get_string(n, &val) < 0)
-				return -EINVAL;
-
-			tplg_ref_add(elem, SND_TPLG_TYPE_DATA, val);
-			tplg_dbg("\t%s: %s\n", id, val);
+			err = tplg_parse_data_refs(n, elem);
+			if (err < 0)
+				return err;
 			continue;
 		}

@@ -725,11 +721,9 @@ int tplg_parse_control_mixer(snd_tplg_t *tplg,
 		}
 
 		if (strcmp(id, "data") == 0) {
-			if (snd_config_get_string(n, &val) < 0)
-				return -EINVAL;
-
-			tplg_ref_add(elem, SND_TPLG_TYPE_DATA, val);
-			tplg_dbg("\t%s: %s\n", id, val);
+			err = tplg_parse_data_refs(n, elem);
+			if (err < 0)
+				return err;
 			continue;
 		}

src/topology/dapm.c +3 -5

index 278d605..d8eb10c 100644
@@ -598,11 +598,9 @@ int tplg_parse_dapm_widget(snd_tplg_t *tplg,
 		}
 
 		if (strcmp(id, "data") == 0) {
-			if (snd_config_get_string(n, &val) < 0)
-				return -EINVAL;
-
-			tplg_ref_add(elem, SND_TPLG_TYPE_DATA, val);
-			tplg_dbg("\t%s: %s\n", id, val);
+			err = tplg_parse_data_refs(n, elem);
+			if (err < 0)
+				return err;
 			continue;
 		}
 	}

src/topology/data.c +90 ~~-26~~

152 lines not on this page · raw message

src/topology/tplg_local.h +1 -0

index 9239aef..4daa540 100644
@@ -229,6 +229,7 @@ int tplg_build_routes(snd_tplg_t *tplg);
 int tplg_build_pcm_dai(snd_tplg_t *tplg, unsigned int type);
 
 int tplg_copy_data(struct tplg_elem *elem, struct tplg_elem *ref);
+int tplg_parse_data_refs(snd_config_t *cfg, struct tplg_elem *elem);
 
 int tplg_ref_add(struct tplg_elem *elem, int type, const char* id);
 int tplg_ref_add_elem(struct tplg_elem *elem, struct tplg_elem *elem_ref);

Signature · 1 line

2.5.0

[email protected]

author

2016-07-15 12:13 UTC #2 lore raw

PATCH v2 2/5 topology: Merge lookup for data reference into tplg_copy_data()

In reply to [email protected]

From: Mengdong Lin <[email protected]>

Code refactor to reduce function calls. Now tplg_copy_data() can look up a referenced data element and merge its data.

Signed-off-by Mengdong Lin <[email protected]>

src/topology/ctl.c +11 ~~-17~~

index 7ded0a4..592dded 100644
@@ -140,9 +140,9 @@ static int tplg_build_mixer_control(snd_tplg_t *tplg,
 				 err = copy_tlv(elem, ref->elem);
 
 		} else if (ref->type == SND_TPLG_TYPE_DATA) {
-			ref->elem = tplg_elem_lookup(&tplg->pdata_list,
-						ref->id, SND_TPLG_TYPE_DATA);
-			 err = tplg_copy_data(elem, ref->elem);
+			err = tplg_copy_data(tplg, elem, ref);
+			if (err < 0)
+				return err;
 		}
 
 		if (!ref->elem) {

@@ -188,9 +188,9 @@ static int tplg_build_enum_control(snd_tplg_t *tplg,
 				copy_enum_texts(elem, ref->elem);
 
 		} else if (ref->type == SND_TPLG_TYPE_DATA) {
-			ref->elem = tplg_elem_lookup(&tplg->pdata_list,
-						ref->id, SND_TPLG_TYPE_DATA);
-			err = tplg_copy_data(elem, ref->elem);
+			err = tplg_copy_data(tplg, elem, ref);
+			if (err < 0)
+				return err;
 		}
 		if (!ref->elem) {
 			SNDERR("error: cannot find '%s' referenced by"

@@ -208,6 +208,7 @@ static int tplg_build_bytes_control(snd_tplg_t *tplg, struct tplg_elem *elem)
 {
 	struct tplg_ref *ref;
 	struct list_head *base, *pos;
+	int err;
 
 	base = &elem->ref_list;

@@ -217,18 +218,11 @@ static int tplg_build_bytes_control(snd_tplg_t *tplg, struct tplg_elem *elem)
 		if (ref->id == NULL || ref->elem)
 			continue;
 
-		/* bytes control only reference one private data section */
-		ref->elem = tplg_elem_lookup(&tplg->pdata_list,
-			ref->id, SND_TPLG_TYPE_DATA);
-		if (!ref->elem) {
-			SNDERR("error: cannot find data '%s'"
-				" referenced by control '%s'\n",
-				ref->id, elem->id);
-			return -EINVAL;
+		 if (ref->type == SND_TPLG_TYPE_DATA) {
+			err = tplg_copy_data(tplg, elem, ref);
+			if (err < 0)
+				return err;
 		}
-
-		/* copy texts to enum elem */
-		return tplg_copy_data(elem, ref->elem);
 	}
 
 	return 0;

src/topology/dapm.c +4 -5

index d8eb10c..4d343b2 100644
@@ -191,12 +191,11 @@ static int tplg_build_widget(snd_tplg_t *tplg,
 			break;
 
 		case SND_TPLG_TYPE_DATA:
-			if (!ref->elem)
-				ref->elem = tplg_elem_lookup(&tplg->pdata_list,
-						ref->id, SND_TPLG_TYPE_DATA);
-			if (ref->elem)
-				err = tplg_copy_data(elem, ref->elem);
+			err = tplg_copy_data(tplg, elem, ref);
+			if (err < 0)
+				return err;
 			break;
+
 		default:
 			break;
 		}

src/topology/data.c +16 -8

index 0c5469a..e60114e 100644
@@ -889,22 +889,30 @@ int tplg_parse_data(snd_tplg_t *tplg, snd_config_t *cfg,
 	return err;
 }
 
-/* Merge data from a referenced data element to the parent element's
- * private data buffer.
+/* Find a referenced data element and copy its data to the parent
+ * element's private data buffer.
  * An element can refer to multiple data sections. Data of these sections
  * will be merged in the their reference order.
  */
-int tplg_copy_data(struct tplg_elem *elem, struct tplg_elem *ref)
+int tplg_copy_data(snd_tplg_t *tplg, struct tplg_elem *elem,
+		   struct tplg_ref *ref)
 {
+	struct tplg_elem *ref_elem;
 	struct snd_soc_tplg_private *priv, *old_priv;
 	int priv_data_size, old_priv_data_size;
 	void *obj;
 
-	if (!ref)
+	ref_elem = tplg_elem_lookup(&tplg->pdata_list,
+				     ref->id, SND_TPLG_TYPE_DATA);
+	if (!ref_elem) {
+		SNDERR("error: cannot find data '%s' referenced by"
+		" element '%s'\n", ref->id, elem->id);
 		return -EINVAL;
+	}
 
 	tplg_dbg("Data '%s' used by '%s'\n", ref->id, elem->id);
-	if (!ref->data || !ref->data->size) /* overlook empty private data */
+	/* overlook empty private data */
+	if (!ref_elem->data || !ref_elem->data->size)
 		return 0;
 
 	old_priv = get_priv_data(elem);

@@ -912,7 +920,7 @@ int tplg_copy_data(struct tplg_elem *elem, struct tplg_elem *ref)
 		return -EINVAL;
 	old_priv_data_size = old_priv->size;
 
-	priv_data_size = ref->data->size;
+	priv_data_size = ref_elem->data->size;
 	obj = realloc(elem->obj,
 			elem->size + priv_data_size);
 	if (!obj)

@@ -926,9 +934,9 @@ int tplg_copy_data(struct tplg_elem *elem, struct tplg_elem *ref)
 	/* merge the new data block */
 	elem->size += priv_data_size;
 	priv->size = priv_data_size + old_priv_data_size;
-	ref->compound_elem = 1;
+	ref_elem->compound_elem = 1;
 	memcpy(priv->data + old_priv_data_size,
-	       ref->data->data, priv_data_size);
+	       ref_elem->data->data, priv_data_size);
 	return 0;
 }

src/topology/tplg_local.h +3 -1

index 4daa540..518b81e 100644
@@ -228,7 +228,9 @@ int tplg_build_widgets(snd_tplg_t *tplg);
 int tplg_build_routes(snd_tplg_t *tplg);
 int tplg_build_pcm_dai(snd_tplg_t *tplg, unsigned int type);
 
-int tplg_copy_data(struct tplg_elem *elem, struct tplg_elem *ref);
+int tplg_copy_data(snd_tplg_t *tplg, struct tplg_elem *elem,
+		   struct tplg_ref *ref);
+
 int tplg_parse_data_refs(snd_config_t *cfg, struct tplg_elem *elem);
 
 int tplg_ref_add(struct tplg_elem *elem, int type, const char* id);

Signature · 1 line

2.5.0

[email protected]

author

2016-07-15 12:14 UTC #3 lore raw

PATCH v2 3/5 topology: Change uuid value to 16 separate characters in text conf file

In reply to [email protected]

From: Mengdong Lin <[email protected]>

Previously in text conf file, the uuid value of vendor tuples is a 16-characer string. Now change it to 16 characters separated by commas, easier for users to edit it manually.

Signed-off-by Mengdong Lin <[email protected]>

include/topology.h +2 -2

index d666505..89bed6c 100644
@@ -273,8 +273,8 @@ extern "C" {
  *		...
  *	}
  *
- *	tuples."uuid" {
- *		VENDOR_TOKEN_ID2 "16 character uuid"
+ *	tuples."uuid" {			# 16 characters separated by commas
+ *		VENDOR_TOKEN_ID2 "0x01,0x02,...,0x0f"
  *		...
  *	}
  *

src/topology/data.c +48 ~~-12~~

103 lines not on this page · raw message

src/topology/tplg_local.h +1 -1

index 518b81e..48db813 100644
@@ -104,7 +104,7 @@ struct tplg_tuple {
 	char token[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
 	union {
 		char string[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
-		char uuid[16];
+		unsigned char uuid[16];
 		unsigned int value;
 	};
 };

Signature · 1 line

2.5.0

[email protected]

author

2016-07-15 12:14 UTC #4 lore raw

PATCH v2 4/5 topology: Parse vendor private data for manifest

In reply to [email protected]

From: Mengdong Lin <[email protected]>

In text conf file, user can define a manifest section and let it refer to private data sections, in the same syntax as other element types.

The text conf file can have at most 1 manifest section.

Signed-off-by Mengdong Lin <[email protected]>

include/topology.h +14 -0

index 89bed6c..644e548 100644
@@ -586,6 +586,20 @@ extern "C" {
  * }
  * </pre>
  *
+ * <h4>Manifest Private Data</h4>
+ * Manfiest may have private data. Users need to define a manifest section
+ * and add the references to 1 or multiple data sections. Please refer to
+ * section 'How to define an element with private data'. <br>
+ * And the text conf file can have at most 1 manifest section. <br><br>
+ *
+ * Manifest section is defined as follows :-
+ *
+ * <pre>
+ * SectionManifest"name" {
+ *
+ *	data "name"			# optional private data
+ * }
+ * </pre>
  */
 
 /** Maximum number of channels supported in one control */

src/topology/data.c +101 -0

113 lines not on this page · raw message

src/topology/elem.c +4 -0

index 50414f0..029c9ab 100644
@@ -150,6 +150,10 @@ struct tplg_elem* tplg_elem_new_common(snd_tplg_t *tplg,
 	case SND_TPLG_TYPE_DATA:
 		list_add_tail(&elem->list, &tplg->pdata_list);
 		break;
+	case SND_TPLG_TYPE_MANIFEST:
+		list_add_tail(&elem->list, &tplg->manifest_list);
+		obj_size = sizeof(struct snd_soc_tplg_manifest);
+		break;
 	case SND_TPLG_TYPE_TEXT:
 		list_add_tail(&elem->list, &tplg->text_list);
 		break;

src/topology/parser.c +27 -1

index f6fc944..3ab64f4 100644
@@ -189,6 +189,15 @@ static int tplg_parse_config(snd_tplg_t *tplg, snd_config_t *cfg)
 			continue;
 		}
 
+		if (strcmp(id, "SectionManifest") == 0) {
+			err = tplg_parse_compound(tplg, n,
+						  tplg_parse_manifest_data,
+				NULL);
+			if (err < 0)
+				return err;
+			continue;
+		}
+
 		SNDERR("error: unknown section %s\n", id);
 	}
 	return 0;

@@ -246,6 +255,10 @@ static int tplg_build_integ(snd_tplg_t *tplg)
 	if (err <  0)
 		return err;
 
+	err = tplg_build_manifest_data(tplg);
+	if (err <  0)
+		return err;
+
 	err = tplg_build_controls(tplg);
 	if (err <  0)
 		return err;

@@ -374,8 +387,16 @@ out:
 
 int snd_tplg_set_manifest_data(snd_tplg_t *tplg, const void *data, int len)
 {
+	if (len <= 0)
+		return 0;
+
 	tplg->manifest.priv.size = len;
-	tplg->manifest_pdata = data;
+
+	tplg->manifest_pdata = malloc(len);
+	if (!tplg->manifest_pdata)
+		return -ENOMEM;
+
+	memcpy(tplg->manifest_pdata, data, len);
 	return 0;
 }

@@ -423,6 +444,7 @@ snd_tplg_t *snd_tplg_new(void)
 	INIT_LIST_HEAD(&tplg->cc_list);
 	INIT_LIST_HEAD(&tplg->route_list);
 	INIT_LIST_HEAD(&tplg->pdata_list);
+	INIT_LIST_HEAD(&tplg->manifest_list);
 	INIT_LIST_HEAD(&tplg->text_list);
 	INIT_LIST_HEAD(&tplg->pcm_config_list);
 	INIT_LIST_HEAD(&tplg->pcm_caps_list);

@@ -437,6 +459,9 @@ snd_tplg_t *snd_tplg_new(void)
 
 void snd_tplg_free(snd_tplg_t *tplg)
 {
+	if (tplg->manifest_pdata)
+		free(tplg->manifest_pdata);
+
 	tplg_elem_free_list(&tplg->tlv_list);
 	tplg_elem_free_list(&tplg->widget_list);
 	tplg_elem_free_list(&tplg->pcm_list);

@@ -444,6 +469,7 @@ void snd_tplg_free(snd_tplg_t *tplg)
 	tplg_elem_free_list(&tplg->cc_list);
 	tplg_elem_free_list(&tplg->route_list);
 	tplg_elem_free_list(&tplg->pdata_list);
+	tplg_elem_free_list(&tplg->manifest_list);
 	tplg_elem_free_list(&tplg->text_list);
 	tplg_elem_free_list(&tplg->pcm_config_list);
 	tplg_elem_free_list(&tplg->pcm_caps_list);

src/topology/tplg_local.h +7 -1

index 48db813..4d79aa7 100644
@@ -58,7 +58,7 @@ struct snd_tplg {
 
 	/* manifest */
 	struct snd_soc_tplg_manifest manifest;
-	const void *manifest_pdata;	/* copied by builder at file write */
+	void *manifest_pdata;	/* copied by builder at file write */
 
 	/* list of each element type */
 	struct list_head tlv_list;

@@ -71,6 +71,7 @@ struct snd_tplg {
 	struct list_head pdata_list;
 	struct list_head token_list;
 	struct list_head tuple_list;
+	struct list_head manifest_list;
 	struct list_head pcm_config_list;
 	struct list_head pcm_caps_list;

@@ -154,6 +155,7 @@ struct tplg_elem {
 		struct snd_soc_tplg_private *data;
 		struct tplg_vendor_tokens *tokens;
 		struct tplg_vendor_tuples *tuples;
+		struct snd_soc_tplg_manifest *manifest;
 	};
 
 	/* an element may refer to other elements:

@@ -195,6 +197,9 @@ int tplg_parse_tuples(snd_tplg_t *tplg, snd_config_t *cfg,
 
 void tplg_free_tuples(void *obj);
 
+int tplg_parse_manifest_data(snd_tplg_t *tplg, snd_config_t *cfg,
+	void *private ATTRIBUTE_UNUSED);
+
 int tplg_parse_control_bytes(snd_tplg_t *tplg,
 	snd_config_t *cfg, void *private ATTRIBUTE_UNUSED);

@@ -223,6 +228,7 @@ int tplg_parse_cc(snd_tplg_t *tplg,
 	snd_config_t *cfg, void *private ATTRIBUTE_UNUSED);
 
 int tplg_build_data(snd_tplg_t *tplg);
+int tplg_build_manifest_data(snd_tplg_t *tplg);
 int tplg_build_controls(snd_tplg_t *tplg);
 int tplg_build_widgets(snd_tplg_t *tplg);
 int tplg_build_routes(snd_tplg_t *tplg);

Signature · 1 line

2.5.0

[email protected]

author

2016-07-15 12:15 UTC #5 lore raw

PATCH v2 5/5 topology: Tuple type can have an extenstion

In reply to [email protected]

From: Mengdong Lin <[email protected]>

After the type specific string ("uuid", "string", "byte", "short" and "word"), users may append a string, like "uuidxxx". The topology parser will check the first few characters to get the tuple type.

This can allow users to put multiple tuples of the same type into one vendor tuple section (SectionVendorTuples), e.g. parameters of multiple firmware modules.

Signed-off-by Mengdong Lin <[email protected]>

include/topology.h +23 -0

index 644e548..0675b52 100644
@@ -302,6 +302,29 @@ extern "C" {
  *	}
  * }
  * </pre>
+ * To define multiple vendor tuples of same type, please append some
+ * characters after the type string ("string", "uuid", "bool", "byte", "short"
+ * or "word"), to avoid ID duplication in the SectionVendorTuples.<br>
+ * The parser will check the first few characters in ID to get the tuple type.
+ * Here is an example:
+ * <pre>
+ * SectionVendorTuples."id of the vendor tuples" {
+ *    ...
+ *	tuples."word.module0" {
+ *		VENDOR_TOKEN_PARAM_ID1 "0x00112233"
+ *		VENDOR_TOKEN_PARAM_ID2 "0x44556677"
+ *		...
+ *	}
+ *
+ *	tuples."word.module2" {
+ *		VENDOR_TOKEN_PARAM_ID1 "0x11223344"
+ *		VENDOR_TOKEN_PARAM_ID2 "0x55667788"
+ *		...
+ *	}
+ *	...
+ * }
+ *
+ * </pre>
  *
  * <h5>Mixer Controls</h5>
  * A mixer control is defined as a new section that can include channel mapping,

src/topology/data.c +6 -6

index 245a834..e81b7f1 100644
@@ -534,17 +534,17 @@ static int parse_tuple_set(snd_config_t *cfg,
 
 	snd_config_get_id(cfg, &id);
 
-	if (strcmp(id, "uuid") == 0)
+	if (strncmp(id, "uuid", 4) == 0)
 		type = SND_SOC_TPLG_TUPLE_TYPE_UUID;
-	else if (strcmp(id, "string") == 0)
+	else if (strncmp(id, "string", 5) == 0)
 		type = SND_SOC_TPLG_TUPLE_TYPE_STRING;
-	else if (strcmp(id, "bool") == 0)
+	else if (strncmp(id, "bool", 4) == 0)
 		type = SND_SOC_TPLG_TUPLE_TYPE_BOOL;
-	else if (strcmp(id, "byte") == 0)
+	else if (strncmp(id, "byte", 4) == 0)
 		type = SND_SOC_TPLG_TUPLE_TYPE_BYTE;
-	else if (strcmp(id, "short") == 0)
+	else if (strncmp(id, "short", 5) == 0)
 		type = SND_SOC_TPLG_TUPLE_TYPE_SHORT;
-	else if (strcmp(id, "word") == 0)
+	else if (strncmp(id, "word", 4) == 0)
 		type = SND_SOC_TPLG_TUPLE_TYPE_WORD;
 	else {
 		SNDERR("error: invalid tuple type '%s'\n", id);

Signature · 1 line

2.5.0

Takashi Sakamoto

Reviewed-by on the v2 series

2016-07-16 12:58 UTC #6 lore raw

In reply to [email protected]

Quoted 22 lines · On Jul 15 2016 21:17, [email protected] wrote:

From: Mengdong Lin <[email protected]>
There is no ABI change in this series.
We finished verification on latest Intel platforms. There will be 4 or 5 series. Later series will come with some ABI update for topology. The size check on ABI objects in current ASoC kernel will detect possible ABI version mismatch between user space and kernel
History:
v2: fix compiler warnings. There are 2 warnings left in topology/pcm.c
that are not caused by this series and will be fixed in later patches.
Mengdong Lin (5):
topology: An element can refer to multipe data sections in text conf
file
topology: Merge lookup for data reference into tplg_copy_data()
topology: Change uuid value to 16 separate characters in text conf
file
topology: Parse vendor private data for manifest
topology: Tuple type can have an extenstion

Quoted 8 lines

 include/topology.h        |  69 ++++++++++-
 src/topology/ctl.c        |  52 +++-----
 src/topology/dapm.c       |  17 ++-
 src/topology/data.c       | 307 ++++++++++++++++++++++++++++++++++++++--------
 src/topology/elem.c       |   4 +
 src/topology/parser.c     |  28 ++++-
 src/topology/tplg_local.h |  15 ++-
 7 files changed, 395 insertions(+), 97 deletions(-)

Reviewed-by Takashi Sakamoto <[email protected]>

Current implementation of ALSA SoC part in kernel has no call corresponding to sys_ioctl(SNDRV_CTL_IOCTL_ELEM_ADD) or something like that, thus I recommend you to drop this line by an additional patch:
http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/topology/ctl.c;h=b948ac021ceecc116bc1087075743243e4b14055;hb=HEAD#l44

Regards

Takashi Sakamoto

Mengdong Lin

author

2016-07-18 02:42 UTC #7 lore raw

Quoted 6 lines · Takashi Sakamoto

Current implementation of ALSA SoC part in kernel has no call corresponding to sys_ioctl(SNDRV_CTL_IOCTL_ELEM_ADD) or something like that, thus I recommend you to drop this line by an additional patch:
http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/topology/ctl.c;h=b948ac021ceecc116bc1087075743243e4b14055;hb=HEAD#l44

Thanks for your suggestions. But this tool is not for directly adding controls from the user space.

It's for ADSP firmware vendors to describe the controls/widgets/pcms for a specific version of firmware. Previously, these components are hard coded in ASoC platforms drivers and not so convenient to share a common driver for different firmwares. The topology binary generated by this tool will be loaded by ASoC platform driver with a specific firmware binary, and so ASoC will create the controls/widgets/pcms that match the firmware automatically.

Thanks
Mengdong

Takashi Sakamoto

2016-07-18 03:43 UTC #8 lore raw

Quoted 10 lines · Mengdong Lin

Thanks for your suggestions. But this tool is not for directly adding controls from the user space.
It's for ADSP firmware vendors to describe the controls/widgets/pcms for a specific version of firmware. Previously, these components are hard coded in ASoC platforms drivers and not so convenient to share a common driver for different firmwares. The topology binary generated by this tool will be loaded by ASoC platform driver with a specific firmware binary, and so ASoC will create the controls/widgets/pcms that match the firmware automatically.

I know all of what you explained. Therefore, You misunderstand about my suggestion.

What I suggested is to drop a support of SNDRV_CTL_ELEM_ACCESS_USER flag from current topology implementation in userspace library, because current implementation of ALSA SoC part doesn't support it.

In ALSA SoC part, snd_ctl_new1() is called to add a control element set as a result of parsing binary blob generated by topology implementation.

In snd_ctl_new1(), SNDRV_CTL_ELEM_ACCESS_USER flag is dropped, in these lines:
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/core/control.c#n263

Totally, the flag cannot be supported by current implementation of topology in kernel. Thus, in userspace, we should drop this line to prevent users from confusions:
http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/topology/ctl.c;h=b948ac021ceecc116bc1087075743243e4b14055;hb=HEAD#l44

That's all what I mentioned. If something unclear, please tell it to me.

Regards

Takashi Sakamoto

Lin, Mengdong

2016-07-18 15:06 UTC #9 lore raw

Quoted 20 lines

-----Original Message-----
From: Takashi Sakamoto [mailto:[email protected]]
Sent: Monday, July 18, 2016 11:43 AM
I know all of what you explained. Therefore, You misunderstand about my suggestion.
What I suggested is to drop a support of SNDRV_CTL_ELEM_ACCESS_USER flag from current topology implementation in userspace library, because current implementation of ALSA SoC part doesn't support it.
In ALSA SoC part, snd_ctl_new1() is called to add a control element set as a result of parsing binary blob generated by topology implementation.
In snd_ctl_new1(), SNDRV_CTL_ELEM_ACCESS_USER flag is dropped, in these lines:
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/core/c
ontrol.c#n263

Quoted 7 lines

Totally, the flag cannot be supported by current implementation of topology in kernel. Thus, in userspace, we should drop this line to prevent users from confusions:
http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/topology/ctl.c;h=b94
8ac021ceecc116bc1087075743243e4b14055;hb=HEAD#l44
That's all what I mentioned. If something unclear, please tell it to me.

Yes, you're right. snd_ctl_new1() dropped SNDRV_CTL_ELEM_ACCESS_USER flag. I'll remove this from the topology user space tool to avoid confusion.

Thanks again
Mengdong

Vinod Koul

2016-07-19 03:52 UTC #10 lore raw

Quoted 26 lines · Mengdong

Quoted 26 lines
-----Original Message-----
From: Takashi Sakamoto [mailto:[email protected]]
Sent: Monday, July 18, 2016 11:43 AM
I know all of what you explained. Therefore, You misunderstand about my suggestion.
What I suggested is to drop a support of SNDRV_CTL_ELEM_ACCESS_USER flag from current topology implementation in userspace library, because current implementation of ALSA SoC part doesn't support it.
In ALSA SoC part, snd_ctl_new1() is called to add a control element set as a result of parsing binary blob generated by topology implementation.
In snd_ctl_new1(), SNDRV_CTL_ELEM_ACCESS_USER flag is dropped, in these lines:
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/core/c
ontrol.c#n263
Totally, the flag cannot be supported by current implementation of topology in kernel. Thus, in userspace, we should drop this line to prevent users from confusions:
http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/topology/ctl.c;h=b94
8ac021ceecc116bc1087075743243e4b14055;hb=HEAD#l44

Quoted 4 lines

Quoted 1 line
That's all what I mentioned. If something unclear, please tell it to me.
Yes, you're right. snd_ctl_new1() dropped SNDRV_CTL_ELEM_ACCESS_USER flag. I'll remove this from the topology user space tool to avoid confusion.

Oh no, that won't be a good idea.

We would like to specify the access for controls from topology. Some controls can be read only and some write only :)

For example, any VU-meter controls should be read-only. Similarly if we have some user data being sent to some modules which can do do all fancy audio detection then these controls should be write-only.

Yes it seems to be broken by this, but we should fix it rather than remove.

Signature · 1 line

~Vinod

Takashi Sakamoto

2016-07-19 05:14 UTC #11 lore raw

Quoted 11 lines · Vinod Koul

Quoted 2 lines
Yes, you're right. snd_ctl_new1() dropped SNDRV_CTL_ELEM_ACCESS_USER flag. I'll remove this from the topology user space tool to avoid confusion.
Oh no, that won't be a good idea.
We would like to specify the access for controls from topology. Some controls can be read only and some write only :)
For example, any VU-meter controls should be read-only. Similarly if we have some user data being sent to some modules which can do do all fancy audio detection then these controls should be write-only.

I just suggested to remove 'user' from topology implementation in userspace library. You can still control access rights of control elements by adding enough flags; 'read' or 'write' (SNDRV_CTL_ELEM_ACCESS_READ or SNDRV_CTL_ELEM_ACCESS_WRITE). SNDRV_CTL_ELEM_ACCESS_USER has no relationship to your purposes.

When a control element has SNDRV_CTL_ELEM_ACCESS_USER flag, a control element set including the element has below operations;
- struct snd_kcontrol.info = snd_ctl_elem_user_enum_info or
snd_ctl_elem_user_info
- struct snd_kcontrol.get = snd_ctl_elem_user_get
- struct snd_kcontrol.put = snd_ctl_elem_user_put
- struct snd_kcontrol.tlv.c = snd_ctl_elem_user_tlv

When userland applications call ioctl(2) with SNDRV_CTL_IOCTL_ELEM_ADD, then ALSA ctl core adds new control element set. The 'USER' flag is used only for this case.

I think it better for you to read 'sound/core/control.c' or my test program in alsa-lib source.
http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=test/user-ctl-element-set.c

Regards

Takashi Sakamoto

Lin, Mengdong

2016-07-19 08:44 UTC #12 lore raw

In reply to Vinod Koul

Quoted 43 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Vinod Koul
Sent: Tuesday, July 19, 2016 12:00 PM
To: Lin, Mengdong
Cc: [email protected]; Mengdong Lin; [email protected]; Takashi Sakamoto; [email protected]; Girdwood, Liam R; Nc, Shreyas
Subject: Re: [alsa-devel] [PATCH v2 0/5] topology: Enhance support for private data
Quoted 9 lines · Mengdong
Quoted 9 lines
-----Original Message-----
From: Takashi Sakamoto [mailto:[email protected]]
Sent: Monday, July 18, 2016 11:43 AM
I know all of what you explained. Therefore, You misunderstand about my suggestion.
What I suggested is to drop a support of
SNDRV_CTL_ELEM_ACCESS_USER
Quoted 5 lines
Quoted 5 lines
flag from current topology implementation in userspace library, because current implementation of ALSA SoC part doesn't support it.
In ALSA SoC part, snd_ctl_new1() is called to add a control element set as a result of parsing binary blob generated by topology
implementation.
Quoted 16 lines
Quoted 16 lines
In snd_ctl_new1(), SNDRV_CTL_ELEM_ACCESS_USER flag is dropped, in these
lines:
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sou
nd/core/c
ontrol.c#n263
Totally, the flag cannot be supported by current implementation of topology in kernel. Thus, in userspace, we should drop this line to prevent users from
confusions:
http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/topology/ct
l.c;h=b94 8ac021ceecc116bc1087075743243e4b14055;hb=HEAD#l44

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
That's all what I mentioned. If something unclear, please tell it to me.
Yes, you're right. snd_ctl_new1() dropped
SNDRV_CTL_ELEM_ACCESS_USER flag.
Quoted 1 line
I'll remove this from the topology user space tool to avoid confusion.
Oh no, that won't be a good idea.

Hi Vinod,

I think topology will drop support for only one flag "SNDRV_CTL_ELEM_ACCESS_USER". All other access flags are still supported.

Thanks
Mengdong

Quoted 16 lines

We would like to specify the access for controls from topology. Some controls can be read only and some write only :)
For example, any VU-meter controls should be read-only. Similarly if we have some user data being sent to some modules which can do do all fancy audio detection then these controls should be write-only.
Yes it seems to be broken by this, but we should fix it rather than remove.
--
~Vinod
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Takashi Iwai

Applied

2016-07-17 08:03 UTC #13 lore raw

In reply to [email protected]

Quoted 14 lines · On Fri, 15 Jul 2016 14:17:21 +0200, [email protected] wrote:

From: Mengdong Lin <[email protected]>
There is no ABI change in this series.
We finished verification on latest Intel platforms. There will be 4 or 5 series. Later series will come with some ABI update for topology. The size check on ABI objects in current ASoC kernel will detect possible ABI version mismatch between user space and kernel
History:
v2: fix compiler warnings. There are 2 warnings left in topology/pcm.c
that are not caused by this series and will be fixed in later patches.

Quoted 8 lines

Mengdong Lin (5):
topology: An element can refer to multipe data sections in text conf
file
topology: Merge lookup for data reference into tplg_copy_data()
topology: Change uuid value to 16 separate characters in text conf
file
topology: Parse vendor private data for manifest
topology: Tuple type can have an extenstion

Applied, thanks.

Takashi