[alsa-devel] [PATCH] ASoC: SOF: topology: use set_get_data in process load
From: Jaska Uimonen jaska.uimonen@intel.com
Currently when loading sof process components there's a check if binary control data is associated with it. If found the data is extracted to be part of component loading and initialization. If binary data exceeds the ipc max size, loading fails with error as large message support is only implemented in set_get_data method. So make the process loading use set_get_data to enable large parameters in component initialization.
Also refactor the process component loading function as it digs out 3 times almost identical information of related controls. This is redundant, looks ugly and makes it difficult to understand the mechanism. So make a function out of fetching the control data and use it in process loading.
Signed-off-by: Jaska Uimonen jaska.uimonen@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sof/topology.c | 204 +++++++++++++++++++++++++-------------- 1 file changed, 130 insertions(+), 74 deletions(-)
diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c index 12b7d900b9c2..30b5638622dd 100644 --- a/sound/soc/sof/topology.c +++ b/sound/soc/sof/topology.c @@ -42,6 +42,13 @@ /* size of tplg abi in byte */ #define SOF_TPLG_ABI_SIZE 3
+struct sof_widget_data { + int ctrl_type; + int ipc_cmd; + struct sof_abi_hdr *pdata; + struct snd_sof_control *control; +}; + /* send pcm params ipc */ static int ipc_pcm_params(struct snd_sof_widget *swidget, int dir) { @@ -1742,51 +1749,32 @@ static int sof_widget_load_siggen(struct snd_soc_component *scomp, int index, return ret; }
-static int sof_process_load(struct snd_soc_component *scomp, int index, - struct snd_sof_widget *swidget, - struct snd_soc_tplg_dapm_widget *tw, - struct sof_ipc_comp_reply *r, - int type) +static size_t sof_get_control_data(struct snd_sof_dev *sdev, + struct snd_soc_dapm_widget *widget, + struct sof_widget_data *wdata) { - struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp); - struct snd_soc_tplg_private *private = &tw->priv; - struct snd_soc_dapm_widget *widget = swidget->widget; const struct snd_kcontrol_new *kc; - struct soc_bytes_ext *sbe; struct soc_mixer_control *sm; + struct soc_bytes_ext *sbe; struct soc_enum *se; - struct snd_sof_control *scontrol = NULL; - struct sof_abi_hdr *pdata = NULL; - struct sof_ipc_comp_process *process; - size_t ipc_size, ipc_data_size = 0; - int ret, i, offset = 0; - - if (type == SOF_COMP_NONE) { - dev_err(sdev->dev, "error: invalid process comp type %d\n", - type); - return -EINVAL; - } + size_t size = 0; + int i;
- /* - * get possible component controls - get size of all pdata, - * then memcpy with headers - */ for (i = 0; i < widget->num_kcontrols; i++) { - kc = &widget->kcontrol_news[i];
switch (widget->dobj.widget.kcontrol_type) { case SND_SOC_TPLG_TYPE_MIXER: sm = (struct soc_mixer_control *)kc->private_value; - scontrol = sm->dobj.private; + wdata[i].control = sm->dobj.private; break; case SND_SOC_TPLG_TYPE_BYTES: sbe = (struct soc_bytes_ext *)kc->private_value; - scontrol = sbe->dobj.private; + wdata[i].control = sbe->dobj.private; break; case SND_SOC_TPLG_TYPE_ENUM: se = (struct soc_enum *)kc->private_value; - scontrol = se->dobj.private; + wdata[i].control = se->dobj.private; break; default: dev_err(sdev->dev, "error: unknown kcontrol type %d in widget %s\n", @@ -1795,31 +1783,98 @@ static int sof_process_load(struct snd_soc_component *scomp, int index, return -EINVAL; }
- if (!scontrol) { + if (!wdata[i].control) { dev_err(sdev->dev, "error: no scontrol for widget %s\n", widget->name); return -EINVAL; }
- /* don't include if no private data */ - pdata = scontrol->control_data->data; - if (!pdata) - continue; + wdata[i].pdata = wdata[i].control->control_data->data; + if (!wdata[i].pdata) + return -EINVAL;
/* make sure data is valid - data can be updated at runtime */ - if (pdata->magic != SOF_ABI_MAGIC) - continue; + if (wdata[i].pdata->magic != SOF_ABI_MAGIC) + return -EINVAL; + + size += wdata[i].pdata->size;
- ipc_data_size += pdata->size; + /* get data type */ + switch (wdata[i].control->cmd) { + case SOF_CTRL_CMD_VOLUME: + case SOF_CTRL_CMD_ENUM: + case SOF_CTRL_CMD_SWITCH: + wdata[i].ipc_cmd = SOF_IPC_COMP_SET_VALUE; + wdata[i].ctrl_type = SOF_CTRL_TYPE_VALUE_CHAN_SET; + break; + case SOF_CTRL_CMD_BINARY: + wdata[i].ipc_cmd = SOF_IPC_COMP_SET_DATA; + wdata[i].ctrl_type = SOF_CTRL_TYPE_DATA_SET; + break; + default: + break; + } + } + + return size; +} + +static int sof_process_load(struct snd_soc_component *scomp, int index, + struct snd_sof_widget *swidget, + struct snd_soc_tplg_dapm_widget *tw, + struct sof_ipc_comp_reply *r, + int type) +{ + struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp); + struct snd_soc_dapm_widget *widget = swidget->widget; + struct snd_soc_tplg_private *private = &tw->priv; + struct sof_ipc_comp_process *process = NULL; + struct sof_widget_data *wdata = NULL; + size_t ipc_data_size = 0; + size_t ipc_size; + int offset = 0; + int ret = 0; + int i; + + if (type == SOF_COMP_NONE) { + dev_err(sdev->dev, "error: invalid process comp type %d\n", + type); + return -EINVAL; + } + + /* allocate struct for widget control data sizes and types */ + if (widget->num_kcontrols) { + wdata = kcalloc(widget->num_kcontrols, + sizeof(*wdata), + GFP_KERNEL); + + if (!wdata) + return -ENOMEM; + + /* get possible component controls and get size of all pdata */ + ipc_data_size = sof_get_control_data(sdev, widget, wdata); + + if (ipc_data_size <= 0) { + kfree(wdata); + return ipc_data_size; + } }
ipc_size = sizeof(struct sof_ipc_comp_process) + le32_to_cpu(private->size) + ipc_data_size;
+ /* we are exceeding max ipc size, config needs to be sent separately */ + if (ipc_size > SOF_IPC_MSG_MAX_SIZE) { + ipc_size -= ipc_data_size; + ipc_data_size = 0; + } + process = kzalloc(ipc_size, GFP_KERNEL); - if (!process) + if (!process) { + kfree(wdata); return -ENOMEM; + }
/* configure iir IPC message */ process->comp.hdr.size = ipc_size; @@ -1835,7 +1890,9 @@ static int sof_process_load(struct snd_soc_component *scomp, int index, if (ret != 0) { dev_err(sdev->dev, "error: parse process.cfg tokens failed %d\n", le32_to_cpu(private->size)); - goto err; + kfree(wdata); + kfree(process); + return ret; }
sof_dbg_comp_config(scomp, &process->config); @@ -1845,40 +1902,13 @@ static int sof_process_load(struct snd_soc_component *scomp, int index, * get possible component controls - get size of all pdata, * then memcpy with headers */ - for (i = 0; i < widget->num_kcontrols; i++) { - kc = &widget->kcontrol_news[i]; - - switch (widget->dobj.widget.kcontrol_type) { - case SND_SOC_TPLG_TYPE_MIXER: - sm = (struct soc_mixer_control *)kc->private_value; - scontrol = sm->dobj.private; - break; - case SND_SOC_TPLG_TYPE_BYTES: - sbe = (struct soc_bytes_ext *)kc->private_value; - scontrol = sbe->dobj.private; - break; - case SND_SOC_TPLG_TYPE_ENUM: - se = (struct soc_enum *)kc->private_value; - scontrol = se->dobj.private; - break; - default: - dev_err(sdev->dev, "error: unknown kcontrol type %d in widget %s\n", - widget->dobj.widget.kcontrol_type, - widget->name); - return -EINVAL; + if (ipc_data_size) { + for (i = 0; i < widget->num_kcontrols; i++) { + memcpy(&process->data + offset, + wdata[i].pdata->data, + wdata[i].pdata->size); + offset += wdata[i].pdata->size; } - - /* don't include if no private data */ - pdata = scontrol->control_data->data; - if (!pdata) - continue; - - /* make sure data is valid - data can be updated at runtime */ - if (pdata->magic != SOF_ABI_MAGIC) - continue; - - memcpy(&process->data + offset, pdata->data, pdata->size); - offset += pdata->size; }
process->size = ipc_data_size; @@ -1886,10 +1916,36 @@ static int sof_process_load(struct snd_soc_component *scomp, int index,
ret = sof_ipc_tx_message(sdev->ipc, process->comp.hdr.cmd, process, ipc_size, r, sizeof(*r)); - if (ret >= 0) + + if (ret < 0) { + dev_err(sdev->dev, "error: create process failed\n"); + kfree(wdata); + kfree(process); return ret; -err: - kfree(process); + } + + /* we sent the data in single message so return */ + if (ipc_data_size) { + kfree(wdata); + return ret; + } + + /* send control data with large message supported method */ + for (i = 0; i < widget->num_kcontrols; i++) { + wdata[i].control->readback_offset = 0; + ret = snd_sof_ipc_set_get_comp_data(sdev->ipc, wdata[i].control, + wdata[i].ipc_cmd, + wdata[i].ctrl_type, + wdata[i].control->cmd, + true); + if (ret != 0) { + dev_err(sdev->dev, "error: send control failed\n"); + kfree(process); + break; + } + } + + kfree(wdata); return ret; }
On 2019-08-07 16:52, Pierre-Louis Bossart wrote:
From: Jaska Uimonen jaska.uimonen@intel.com
process = kzalloc(ipc_size, GFP_KERNEL);
- if (!process)
if (!process) {
kfree(wdata);return -ENOMEM;
}
/* configure iir IPC message */ process->comp.hdr.size = ipc_size;
@@ -1835,7 +1890,9 @@ static int sof_process_load(struct snd_soc_component *scomp, int index, if (ret != 0) { dev_err(sdev->dev, "error: parse process.cfg tokens failed %d\n", le32_to_cpu(private->size));
goto err;
kfree(wdata);kfree(process);return ret;
@@ -1886,10 +1916,36 @@ static int sof_process_load(struct snd_soc_component *scomp, int index,
ret = sof_ipc_tx_message(sdev->ipc, process->comp.hdr.cmd, process, ipc_size, r, sizeof(*r));
- if (ret >= 0)
- if (ret < 0) {
dev_err(sdev->dev, "error: create process failed\n");kfree(wdata);kfree(process);-err:
- kfree(process);
- }
- /* we sent the data in single message so return */
- if (ipc_data_size) {
kfree(wdata);return ret;- }
- /* send control data with large message supported method */
- for (i = 0; i < widget->num_kcontrols; i++) {
wdata[i].control->readback_offset = 0;ret = snd_sof_ipc_set_get_comp_data(sdev->ipc, wdata[i].control,wdata[i].ipc_cmd,wdata[i].ctrl_type,wdata[i].control->cmd,true);if (ret != 0) {dev_err(sdev->dev, "error: send control failed\n");kfree(process);break;}- }
- kfree(wdata); return ret; }
On several occasions you've added individual error paths instead of a unified one. Personally I don't find it easier to read and understand function's flow at all.
<ifs with goto err>
err: kfree(process); kfree(wdata); return ret;
doesn't look that bad..
On Wed, Aug 7, 2019 at 12:32 PM Cezary Rojewski cezary.rojewski@intel.com wrote:
On 2019-08-07 16:52, Pierre-Louis Bossart wrote:
From: Jaska Uimonen jaska.uimonen@intel.com
process = kzalloc(ipc_size, GFP_KERNEL);
if (!process)
if (!process) {kfree(wdata); return -ENOMEM;} /* configure iir IPC message */ process->comp.hdr.size = ipc_size;@@ -1835,7 +1890,9 @@ static int sof_process_load(struct
snd_soc_component *scomp, int index,
if (ret != 0) { dev_err(sdev->dev, "error: parse process.cfg tokens failed%d\n",
le32_to_cpu(private->size));
goto err;
kfree(wdata);kfree(process);return ret; }@@ -1886,10 +1916,36 @@ static int sof_process_load(struct
snd_soc_component *scomp, int index,
ret = sof_ipc_tx_message(sdev->ipc, process->comp.hdr.cmd, process, ipc_size, r, sizeof(*r));
if (ret >= 0)
if (ret < 0) {dev_err(sdev->dev, "error: create process failed\n");kfree(wdata);kfree(process); return ret;-err:
kfree(process);
}/* we sent the data in single message so return */if (ipc_data_size) {kfree(wdata);return ret;}/* send control data with large message supported method */for (i = 0; i < widget->num_kcontrols; i++) {wdata[i].control->readback_offset = 0;ret = snd_sof_ipc_set_get_comp_data(sdev->ipc,wdata[i].control,
wdata[i].ipc_cmd,wdata[i].ctrl_type,wdata[i].control->cmd,true);if (ret != 0) {dev_err(sdev->dev, "error: send control failed\n");kfree(process);break;}}kfree(wdata); return ret;On several occasions you've added individual error paths instead of a unified one. Personally I don't find it easier to read and understand function's flow at all.
<ifs with goto err>
err: kfree(process); kfree(wdata); return ret;
doesn't look that bad..
Thanks for pointing out. Perhaps, the error handling can be improved a little. We can fix in v2.
Thanks, Ranjani
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 8/7/19 3:09 PM, Sridharan, Ranjani wrote:
On Wed, Aug 7, 2019 at 12:32 PM Cezary Rojewski <cezary.rojewski@intel.com mailto:cezary.rojewski@intel.com> wrote:
On 2019-08-07 16:52, Pierre-Louis Bossart wrote: > From: Jaska Uimonen <jaska.uimonen@intel.com <mailto:jaska.uimonen@intel.com>> > process = kzalloc(ipc_size, GFP_KERNEL); > - if (!process) > + if (!process) { > + kfree(wdata); > return -ENOMEM; > + } > > /* configure iir IPC message */ > process->comp.hdr.size = ipc_size; > @@ -1835,7 +1890,9 @@ static int sof_process_load(struct snd_soc_component *scomp, int index, > if (ret != 0) { > dev_err(sdev->dev, "error: parse process.cfg tokens failed %d\n", > le32_to_cpu(private->size)); > - goto err; > + kfree(wdata); > + kfree(process); > + return ret; > } > > @@ -1886,10 +1916,36 @@ static int sof_process_load(struct snd_soc_component *scomp, int index, > > ret = sof_ipc_tx_message(sdev->ipc, process->comp.hdr.cmd, process, > ipc_size, r, sizeof(*r)); > - if (ret >= 0) > + > + if (ret < 0) { > + dev_err(sdev->dev, "error: create process failed\n"); > + kfree(wdata); > + kfree(process); > return ret; > -err: > - kfree(process); > + } > + > + /* we sent the data in single message so return */ > + if (ipc_data_size) { > + kfree(wdata); > + return ret; > + } > + > + /* send control data with large message supported method */ > + for (i = 0; i < widget->num_kcontrols; i++) { > + wdata[i].control->readback_offset = 0; > + ret = snd_sof_ipc_set_get_comp_data(sdev->ipc, wdata[i].control, > + wdata[i].ipc_cmd, > + wdata[i].ctrl_type, > + wdata[i].control->cmd, > + true); > + if (ret != 0) { > + dev_err(sdev->dev, "error: send control failed\n"); > + kfree(process); > + break; > + } > + } > + > + kfree(wdata); > return ret; > } On several occasions you've added individual error paths instead of a unified one. Personally I don't find it easier to read and understand function's flow at all. <ifs with goto err> err: kfree(process); kfree(wdata); return ret; doesn't look that bad..Thanks for pointing out. Perhaps, the error handling can be improved a little. We can fix in v2.
I took a look at this and there's really only 2/3 places where we could use a goto, but we'd have to use 2 labels depending on whether we free process/wdata so not sure if we'd make the code more self-explanatory in the end.
Jaska, can you take a look?
participants (3)
-
Cezary Rojewski -
Pierre-Louis Bossart -
Sridharan, Ranjani