ASoC: SOF: topology: use set_get_data in process load

Commit Message

Pierre-Louis Bossart Aug. 7, 2019, 2:52 p.m. UTC

From: Jaska Uimonen <[e-mail]>

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 <[e-mail]>
Signed-off-by: Pierre-Louis Bossart <[e-mail]>
---
 sound/soc/sof/topology.c | 204 +++++++++++++++++++++++++--------------
 1 file changed, 130 insertions(+), 74 deletions(-)

Comments

Cezary Rojewski Aug. 7, 2019, 7:30 p.m. UTC | #1

On 2019-08-07 16:52, Pierre-Louis Bossart wrote:
> From: Jaska Uimonen <[e-mail]>

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

Sridharan, Ranjani Aug. 7, 2019, 8:09 p.m. UTC | #2

On Wed, Aug 7, 2019 at 12:32 PM Cezary Rojewski <[e-mail]>
wrote:

> On 2019-08-07 16:52, Pierre-Louis Bossart wrote:
> > From: Jaska Uimonen <[e-mail]>
>
> >       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
> [e-mail]
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Pierre-Louis Bossart Aug. 7, 2019, 8:17 p.m. UTC | #3

On 8/7/19 3:09 PM, Sridharan, Ranjani wrote:
> 
> 
> On Wed, Aug 7, 2019 at 12:32 PM Cezary Rojewski 
> <[e-mail] <mailto:[e-mail]>> wrote:
> 
>     On 2019-08-07 16:52, Pierre-Louis Bossart wrote:
>      > From: Jaska Uimonen <[e-mail]
>     <mailto:[e-mail]>>
> 
>      >       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?

Patch

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;
 }