[alsa-devel] [PATCH 0/6] topology: complete PCM parsing and code refactoring
From: Mengdong Lin mengdong.lin@linux.intel.com
There is no ABI change in this series.
This series fixes bugs and missing info of PCM (Front-end DAI & DAI link) defined by the text conf file.
There is also some code refactoring, as a preparation for Back-end DAI support and ABI update which is under internal test and review atm.
Mengdong Lin (6): topology: Set manifest size for ABI topology: Refactor functions to parse and build streams topology: Use generic pointer to realloc buffer for private data topology: Fix pcm ID & name parsing topology: Parse front-end DAI name and ID for the PCM topology: Update PCM configurations in Broadwell text conf file
include/sound/asoc.h | 2 +- include/topology.h | 4 + src/conf/topology/broadwell/broadwell.conf | 34 ++++++--- src/topology/data.c | 24 ++---- src/topology/parser.c | 4 +- src/topology/pcm.c | 119 ++++++++++++++++++++++------- src/topology/tplg_local.h | 2 +- 7 files changed, 131 insertions(+), 58 deletions(-)
From: Mengdong Lin mengdong.lin@linux.intel.com
The topology kernel driver will check the size of manifest struct, and will stop loading topology info if size mismatch is detected.
Signed-off-by: Mengdong Lin mengdong.lin@linux.intel.com
diff --git a/src/topology/parser.c b/src/topology/parser.c index 30d91f9..84117c3 100644 --- a/src/topology/parser.c +++ b/src/topology/parser.c @@ -414,6 +414,8 @@ snd_tplg_t *snd_tplg_new(void) if (!tplg) return NULL;
+ tplg->manifest.size = sizeof(struct snd_soc_tplg_manifest); + INIT_LIST_HEAD(&tplg->tlv_list); INIT_LIST_HEAD(&tplg->widget_list); INIT_LIST_HEAD(&tplg->pcm_list);
From: Mengdong Lin mengdong.lin@linux.intel.com
Previously these functions are only used by pcm elements (front-end DAI & DAI link) to parse stream capablities. Now refactor them to be reused by back-end DAI elements later.
Signed-off-by: Mengdong Lin mengdong.lin@linux.intel.com
diff --git a/src/topology/parser.c b/src/topology/parser.c index 84117c3..f6fc944 100644 --- a/src/topology/parser.c +++ b/src/topology/parser.c @@ -119,7 +119,7 @@ static int tplg_parse_config(snd_tplg_t *tplg, snd_config_t *cfg)
if (strcmp(id, "SectionPCMCapabilities") == 0) { err = tplg_parse_compound(tplg, n, - tplg_parse_pcm_caps, NULL); + tplg_parse_stream_caps, NULL); if (err < 0) return err; continue; diff --git a/src/topology/pcm.c b/src/topology/pcm.c index d75aad8..1df4f54 100644 --- a/src/topology/pcm.c +++ b/src/topology/pcm.c @@ -40,9 +40,9 @@ struct tplg_elem *lookup_pcm_dai_stream(struct list_head *base, const char* id) return NULL; }
-/* copy referenced caps to the pcm */ -static void copy_pcm_caps(const char *id, struct snd_soc_tplg_stream_caps *caps, - struct tplg_elem *ref_elem) +/* copy referenced caps to the parent (pcm or be dai) */ +static void copy_stream_caps(const char *id, + struct snd_soc_tplg_stream_caps *caps, struct tplg_elem *ref_elem) { struct snd_soc_tplg_stream_caps *ref_caps = ref_elem->stream_caps;
@@ -52,24 +52,19 @@ static void copy_pcm_caps(const char *id, struct snd_soc_tplg_stream_caps *caps, *caps = *ref_caps; }
-/* check referenced config and caps for a pcm */ -static int tplg_build_pcm_caps(snd_tplg_t *tplg, struct tplg_elem *elem) +/* find and copy the referenced stream caps */ +static int tplg_build_stream_caps(snd_tplg_t *tplg, + const char *id, struct snd_soc_tplg_stream_caps *caps) { struct tplg_elem *ref_elem = NULL; - struct snd_soc_tplg_pcm *pcm; - struct snd_soc_tplg_stream_caps *caps; unsigned int i;
- pcm = elem->pcm; - for (i = 0; i < 2; i++) { - caps = &pcm->caps[i]; - ref_elem = tplg_elem_lookup(&tplg->pcm_caps_list, - caps->name, SND_TPLG_TYPE_STREAM_CAPS); + caps[i].name, SND_TPLG_TYPE_STREAM_CAPS);
if (ref_elem != NULL) - copy_pcm_caps(elem->id, caps, ref_elem); + copy_stream_caps(id, &caps[i], ref_elem); }
return 0; @@ -91,7 +86,7 @@ int tplg_build_pcm(snd_tplg_t *tplg, unsigned int type) return -EINVAL; }
- err = tplg_build_pcm_caps(tplg, elem); + err = tplg_build_stream_caps(tplg, elem->id, elem->pcm->caps); if (err < 0) return err;
@@ -184,8 +179,8 @@ static int split_format(struct snd_soc_tplg_stream_caps *caps, char *str) return 0; }
-/* Parse pcm Capabilities */ -int tplg_parse_pcm_caps(snd_tplg_t *tplg, +/* Parse pcm stream capabilities */ +int tplg_parse_stream_caps(snd_tplg_t *tplg, snd_config_t *cfg, void *private ATTRIBUTE_UNUSED) { struct snd_soc_tplg_stream_caps *sc; @@ -263,29 +258,40 @@ int tplg_parse_pcm_caps(snd_tplg_t *tplg, return 0; }
-/* Parse the caps of a pcm stream */ -int tplg_parse_stream_caps(snd_tplg_t *tplg, snd_config_t *cfg, +/* Parse the caps and config of a pcm stream */ +static int tplg_parse_streams(snd_tplg_t *tplg, snd_config_t *cfg, void *private) { snd_config_iterator_t i, next; snd_config_t *n; struct tplg_elem *elem = private; struct snd_soc_tplg_pcm *pcm; + unsigned int *playback, *capture; + struct snd_soc_tplg_stream_caps *caps; const char *id, *value; int stream;
- pcm = elem->pcm; - snd_config_get_id(cfg, &id);
tplg_dbg("\t%s:\n", id);
+ switch (elem->type) { + case SND_TPLG_TYPE_PCM: + pcm = elem->pcm; + playback = &pcm->playback; + capture = &pcm->capture; + caps = pcm->caps; + break; + default: + return -EINVAL; + } + if (strcmp(id, "playback") == 0) { stream = SND_SOC_TPLG_STREAM_PLAYBACK; - pcm->playback = 1; + *playback = 1; } else if (strcmp(id, "capture") == 0) { stream = SND_SOC_TPLG_STREAM_CAPTURE; - pcm->capture = 1; + *capture = 1; } else return -EINVAL;
@@ -300,8 +306,10 @@ int tplg_parse_stream_caps(snd_tplg_t *tplg, snd_config_t *cfg, if (strcmp(id, "capabilities") == 0) { if (snd_config_get_string(n, &value) < 0) continue; - - elem_copy_text(pcm->caps[stream].name, value, + /* store stream caps name, to find and merge + * the caps in building phase. + */ + elem_copy_text(caps[stream].name, value, SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
tplg_dbg("\t\t%s\n\t\t\t%s\n", id, value); @@ -312,7 +320,7 @@ int tplg_parse_stream_caps(snd_tplg_t *tplg, snd_config_t *cfg, return 0; }
-/* Parse pcm */ +/* Parse pcm (for front end DAI & DAI link) */ int tplg_parse_pcm(snd_tplg_t *tplg, snd_config_t *cfg, void *private ATTRIBUTE_UNUSED) { @@ -365,7 +373,7 @@ int tplg_parse_pcm(snd_tplg_t *tplg,
if (strcmp(id, "pcm") == 0) { err = tplg_parse_compound(tplg, n, - tplg_parse_stream_caps, elem); + tplg_parse_streams, elem); if (err < 0) return err; continue; diff --git a/src/topology/tplg_local.h b/src/topology/tplg_local.h index 4c601d4..9239aef 100644 --- a/src/topology/tplg_local.h +++ b/src/topology/tplg_local.h @@ -210,7 +210,7 @@ int tplg_parse_dapm_graph(snd_tplg_t *tplg, snd_config_t *cfg, int tplg_parse_dapm_widget(snd_tplg_t *tplg, snd_config_t *cfg, void *private ATTRIBUTE_UNUSED);
-int tplg_parse_pcm_caps(snd_tplg_t *tplg, +int tplg_parse_stream_caps(snd_tplg_t *tplg, snd_config_t *cfg, void *private ATTRIBUTE_UNUSED);
int tplg_parse_pcm(snd_tplg_t *tplg,
From: Mengdong Lin mengdong.lin@linux.intel.com
Many element types have private data. So use the generic obj pointer instead of the type-specific pointer when reallocating the object to accommodate the private data.
Empty private data will be overlooked.
Signed-off-by: Mengdong Lin mengdong.lin@linux.intel.com
diff --git a/src/topology/data.c b/src/topology/data.c index 19c31bf..6606ca9 100644 --- a/src/topology/data.c +++ b/src/topology/data.c @@ -827,39 +827,29 @@ int tplg_copy_data(struct tplg_elem *elem, struct tplg_elem *ref) return -EINVAL;
tplg_dbg("Data '%s' used by '%s'\n", ref->id, elem->id); + if (!ref->data || !ref->data->size) /* overlook empty private data */ + return 0; + priv_data_size = ref->data->size; + elem->obj = realloc(elem->obj, + elem->size + priv_data_size); + if (!elem->obj) + return -ENOMEM;
switch (elem->type) { case SND_TPLG_TYPE_MIXER: - elem->mixer_ctrl = realloc(elem->mixer_ctrl, - elem->size + priv_data_size); - if (!elem->mixer_ctrl) - return -ENOMEM; priv = &elem->mixer_ctrl->priv; break;
case SND_TPLG_TYPE_ENUM: - elem->enum_ctrl = realloc(elem->enum_ctrl, - elem->size + priv_data_size); - if (!elem->enum_ctrl) - return -ENOMEM; priv = &elem->enum_ctrl->priv; break;
case SND_TPLG_TYPE_BYTES: - elem->bytes_ext = realloc(elem->bytes_ext, - elem->size + priv_data_size); - if (!elem->bytes_ext) - return -ENOMEM; priv = &elem->bytes_ext->priv; break;
- case SND_TPLG_TYPE_DAPM_WIDGET: - elem->widget = realloc(elem->widget, - elem->size + priv_data_size); - if (!elem->widget) - return -ENOMEM; priv = &elem->widget->priv; break;
Dne 28.4.2016 v 10:41 mengdong.lin@linux.intel.com napsal(a):
priv_data_size = ref->data->size;
- elem->obj = realloc(elem->obj,
elem->size + priv_data_size);- if (!elem->obj)
return -ENOMEM;
This causes a memory leak when realloc fails. You should free the original pointer when realloc() fails.
Jaroslav
On Thu, 28 Apr 2016 10:48:36 +0200, Jaroslav Kysela wrote:
Dne 28.4.2016 v 10:41 mengdong.lin@linux.intel.com napsal(a):
priv_data_size = ref->data->size;
- elem->obj = realloc(elem->obj,
elem->size + priv_data_size);- if (!elem->obj)
return -ENOMEM;This causes a memory leak when realloc fails. You should free the original pointer when realloc() fails.
Right, and the bug (the leak) has been already present before the patch...
thanks,
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, April 28, 2016 9:55 PM To: Jaroslav Kysela Cc: mengdong.lin@linux.intel.com; alsa-devel@alsa-project.org; broonie@kernel.org; Girdwood, Liam R; Lin, Mengdong; Shah, Hardik T; Singh, Guneshwor O Subject: Re: [PATCH 3/6] topology: Use generic pointer to realloc buffer for private data
On Thu, 28 Apr 2016 10:48:36 +0200, Jaroslav Kysela wrote:
Dne 28.4.2016 v 10:41 mengdong.lin@linux.intel.com napsal(a):
priv_data_size = ref->data->size;
- elem->obj = realloc(elem->obj,
elem->size + priv_data_size);- if (!elem->obj)
return -ENOMEM;This causes a memory leak when realloc fails. You should free the original pointer when realloc() fails.
Right, and the bug (the leak) has been already present before the patch...
Okay, we'll fix this. Thanks for pointing out this.
Regards Mengdong
On 04/28/2016 10:30 PM, Lin, Mengdong wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, April 28, 2016 9:55 PM To: Jaroslav Kysela Cc: mengdong.lin@linux.intel.com; alsa-devel@alsa-project.org; broonie@kernel.org; Girdwood, Liam R; Lin, Mengdong; Shah, Hardik T; Singh, Guneshwor O Subject: Re: [PATCH 3/6] topology: Use generic pointer to realloc buffer for private data
On Thu, 28 Apr 2016 10:48:36 +0200, Jaroslav Kysela wrote:
Dne 28.4.2016 v 10:41 mengdong.lin@linux.intel.com napsal(a):
priv_data_size = ref->data->size;
- elem->obj = realloc(elem->obj,
elem->size + priv_data_size);- if (!elem->obj)
return -ENOMEM;This causes a memory leak when realloc fails. You should free the original pointer when realloc() fails.
Right, and the bug (the leak) has been already present before the patch...
Okay, we'll fix this. Thanks for pointing out this.
I fixed this issue in v2 series. Would you please have a review?
Thanks again Mengdong
On Wed, 04 May 2016 03:29:33 +0200, Mengdong Lin wrote:
On 04/28/2016 10:30 PM, Lin, Mengdong wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, April 28, 2016 9:55 PM To: Jaroslav Kysela Cc: mengdong.lin@linux.intel.com; alsa-devel@alsa-project.org; broonie@kernel.org; Girdwood, Liam R; Lin, Mengdong; Shah, Hardik T; Singh, Guneshwor O Subject: Re: [PATCH 3/6] topology: Use generic pointer to realloc buffer for private data
On Thu, 28 Apr 2016 10:48:36 +0200, Jaroslav Kysela wrote:
Dne 28.4.2016 v 10:41 mengdong.lin@linux.intel.com napsal(a):
priv_data_size = ref->data->size;
- elem->obj = realloc(elem->obj,
elem->size + priv_data_size);- if (!elem->obj)
return -ENOMEM;This causes a memory leak when realloc fails. You should free the original pointer when realloc() fails.
Right, and the bug (the leak) has been already present before the patch...
Okay, we'll fix this. Thanks for pointing out this.
I fixed this issue in v2 series. Would you please have a review?
Now I'm back to online. Will start reviewing the series in tomorrow or so.
thanks,
Takashi
From: Mengdong Lin mengdong.lin@linux.intel.com
The name and ID of SectionPCM should be set to pcm_name and pcm_id, for a front-end DAI link in the kernel, not for the front-end DAI of the link.
Signed-off-by: Mengdong Lin mengdong.lin@linux.intel.com
diff --git a/include/sound/asoc.h b/include/sound/asoc.h index 920c9e0..abe49c5 100644 --- a/include/sound/asoc.h +++ b/include/sound/asoc.h @@ -414,7 +414,7 @@ struct snd_soc_tplg_pcm { __le32 size; /* in bytes of this structure */ char pcm_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN]; char dai_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN]; - __le32 pcm_id; /* unique ID - used to match */ + __le32 pcm_id; /* unique ID - used to match with DAI link */ __le32 dai_id; /* unique ID - used to match */ __le32 playback; /* supports playback mode */ __le32 capture; /* supports capture mode */ diff --git a/src/topology/pcm.c b/src/topology/pcm.c index 1df4f54..1661821 100644 --- a/src/topology/pcm.c +++ b/src/topology/pcm.c @@ -337,7 +337,7 @@ int tplg_parse_pcm(snd_tplg_t *tplg,
pcm = elem->pcm; pcm->size = elem->size; - elem_copy_text(pcm->dai_name, elem->id, SNDRV_CTL_ELEM_ID_NAME_MAXLEN); + elem_copy_text(pcm->pcm_name, elem->id, SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
tplg_dbg(" PCM: %s\n", elem->id);
@@ -366,8 +366,8 @@ int tplg_parse_pcm(snd_tplg_t *tplg, if (snd_config_get_string(n, &val) < 0) return -EINVAL;
- pcm->dai_id = atoi(val); - tplg_dbg("\t%s: %d\n", id, pcm->dai_id); + pcm->pcm_id = atoi(val); + tplg_dbg("\t%s: %d\n", id, pcm->pcm_id); continue; }
From: Mengdong Lin mengdong.lin@linux.intel.com
These two fields are necessary to create the front-end DAIs in kernel but the support is missing in text conf previously.
Signed-off-by: Mengdong Lin mengdong.lin@linux.intel.com
diff --git a/include/topology.h b/include/topology.h index b47f422..9d57ce3 100644 --- a/include/topology.h +++ b/include/topology.h @@ -533,6 +533,10 @@ extern "C" { * * id "0" # used for binding to the PCM * + * dai."name of front-end DAI" { + * id "0" # used for binding to the front-end DAI + * } + * * pcm."playback" { * capabilities "capabilities1" # capabilities for playback * diff --git a/src/topology/pcm.c b/src/topology/pcm.c index 1661821..efee58b 100644 --- a/src/topology/pcm.c +++ b/src/topology/pcm.c @@ -320,6 +320,51 @@ static int tplg_parse_streams(snd_tplg_t *tplg, snd_config_t *cfg, return 0; }
+/* Parse name and id of a front-end DAI (ie. cpu dai of a FE DAI link) */ +static int tplg_parse_fe_dai(snd_tplg_t *tplg, snd_config_t *cfg, + void *private) +{ + struct tplg_elem *elem = private; + struct snd_soc_tplg_pcm *pcm = elem->pcm; + snd_config_iterator_t i, next; + snd_config_t *n; + const char *id, *value = NULL; + unsigned long int id_val; + int err; + + snd_config_get_id(cfg, &id); + tplg_dbg("\t\tFE DAI %s:\n", id); + elem_copy_text(pcm->dai_name, id, SNDRV_CTL_ELEM_ID_NAME_MAXLEN); + + snd_config_for_each(i, next, cfg) { + + n = snd_config_iterator_entry(i); + + /* get id */ + if (snd_config_get_id(n, &id) < 0) + continue; + + if (strcmp(id, "id") == 0) { + if (snd_config_get_string(n, &value) < 0) + continue; + errno = 0; + /* no support for negative value */ + id_val = strtoul(value, NULL, 0); + if ((errno == ERANGE && id_val == ULONG_MAX) + || (errno != 0 && id_val == 0) + || id_val > UINT_MAX) { + SNDERR("error: invalid fe dai ID\n"); + return -EINVAL; + } + + pcm->dai_id = (int) id_val; + tplg_dbg("\t\t\tindex: %d\n", pcm->dai_id); + } + } + + return 0; +} + /* Parse pcm (for front end DAI & DAI link) */ int tplg_parse_pcm(snd_tplg_t *tplg, snd_config_t *cfg, void *private ATTRIBUTE_UNUSED) @@ -378,6 +423,14 @@ int tplg_parse_pcm(snd_tplg_t *tplg, return err; continue; } + + if (strcmp(id, "dai") == 0) { + err = tplg_parse_compound(tplg, n, + tplg_parse_fe_dai, elem); + if (err < 0) + return err; + continue; + } }
return 0;
From: Mengdong Lin mengdong.lin@linux.intel.com
To make this conf file a better example, update the name & ID of PCMs (front-end DAI link) and their cpu DAI (front-end DAI), same as those defined by Broadwell upstream driver.
Signed-off-by: Mengdong Lin mengdong.lin@linux.intel.com
diff --git a/src/conf/topology/broadwell/broadwell.conf b/src/conf/topology/broadwell/broadwell.conf index 05b3889..eb89377 100644 --- a/src/conf/topology/broadwell/broadwell.conf +++ b/src/conf/topology/broadwell/broadwell.conf @@ -272,18 +272,22 @@ SectionPCMCapabilities."Offload0 Playback" { SectionPCMCapabilities."Offload1 Playback" { formats "S24_LE,S16_LE" rate_min "8000" - rate_max "48000" + rate_max "192000" channels_min "2" channels_max "2" }
# PCM devices exported by Firmware -SectionPCM."System Pin" { +SectionPCM."System Playback/Capture" {
index "1"
# used for binding to the PCM - ID "0" + id "0" + + dai."System Pin" { + id "0" + }
pcm."playback" {
@@ -307,12 +311,16 @@ SectionPCM."System Pin" { } }
-SectionPCM."Offload0 Pin" { +SectionPCM."Offload0 Playback" {
index "1"
# used for binding to the PCM - ID "1" + id "1" + + dai."Offload0 Pin" { + id "1" + }
pcm."playback" {
@@ -325,12 +333,16 @@ SectionPCM."Offload0 Pin" { } }
-SectionPCM."Offload1 Pin" { +SectionPCM."Offload1 Playback" {
index "1"
# used for binding to the PCM - ID "2" + id "2" + + dai."Offload1 Pin" { + id "2" + }
pcm."playback" {
@@ -343,12 +355,16 @@ SectionPCM."Offload1 Pin" { } }
-SectionPCM."Loopback Pin" { +SectionPCM."Loopback PCM" {
index "1"
# used for binding to the PCM - ID "3" + id "3" + + dai."Loopback Pin" { + id "3" + }
pcm."capture" {
participants (5)
-
Jaroslav Kysela -
Lin, Mengdong -
Mengdong Lin -
mengdong.lin＠linux.intel.com
-
Takashi Iwai