Skip to content

[RFC] generic effect type with fir and iir eq #127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lgirdwood merged 2 commits into from
Sep 24, 2018
Merged

[RFC] generic effect type with fir and iir eq #127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ac9230
ASOC: SOF: topology: introduce effect type for widgets
Sep 9, 2018
7b19676
ASOC: SOF: topology: change eq to use effect type
Sep 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions include/uapi/sound/sof-ipc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,12 +724,28 @@ struct sof_ipc_comp_tone {
struct sof_ipc_comp_eq_fir {
struct sof_ipc_comp comp;
struct sof_ipc_comp_config config;
int32_t size;
unsigned char data[0];
} __attribute__((packed));

/* IIR equalizer component */
struct sof_ipc_comp_eq_iir {
struct sof_ipc_comp comp;
struct sof_ipc_comp_config config;
int32_t size;
unsigned char data[0];
} __attribute__((packed));

/** \brief Types of EFFECT */
enum sof_ipc_effect_type {
SOF_EFFECT_INTEL_NONE = 0, /**< None */
SOF_EFFECT_INTEL_EQFIR, /**< Intel FIR */
SOF_EFFECT_INTEL_EQIIR, /**< Intel IIR */
};

/* general purpose EFFECT configuration */
struct sof_ipc_comp_effect {
enum sof_ipc_effect_type type;
} __attribute__((packed));
Copy link
Member

@plbossart plbossart Oct 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood @juimonen @singalsu
Are you sure this definition is correct? All soc_ipc_comp_xxx structures start with struct sof_ipc_comp comp;
e.g.

struct sof_ipc_comp_eq_fir {
	struct sof_ipc_comp comp;
	struct sof_ipc_comp_config config;
	uint32_t size;
	unsigned char data[0];
} __attribute__((packed));

here you are missing the comp field so all the address calculations will be off.

struct sof_ipc_comp_effect {
>>> missing comp field?
	enum sof_ipc_effect_type type;
} __attribute__((packed));

I wonder how this might work since there is no way the IPC can read the right fields, this will read uninitialized data.

While I am at it, is there really a need for this IPC? For example, for an EQ, you will have to rely on two IPC messages, one to set the effect type and one to set the actual parameters. Is this a desired feature and can the topology actually support this? I can't recall any other component where we do this.

Copy link
Author

@juimonen juimonen Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi,
long time since I checked this...

So as I understand the ipc_comp_effect is never sent to DSP it is mainly helper to used to parse the token from topology, so that we can then parse that particular effects data (which is sent to dsp). Like ipc_frame_type or ipc_dai_type. So actually I don't know why it is called "ipc". I think I copied the structure from the existing types.

And as I understood in the topology the ordering doesn't matter if you are just using tuples and not some byte fields etc?. So you can basically parse the tuples in any order you want...now I might be horribly wrong with this. I think I tested this to work with tuples and I remember that whole thing collapsed if you put bytes field to widget data. The effect type is actually after the general comp data in the topology.

As the dapm "effect" doesn't have any subcategories we need some way to differentiate between effects, like we have now already iir and fir. in the previous demo the dapm widget effect was hardcoded to be iir.


/* frees components, buffers and pipelines
Expand Down
2 changes: 2 additions & 0 deletions include/uapi/sound/sof-topology.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,4 +91,6 @@
#define SOF_TKN_INTEL_DMIC_PDM_CLK_EDGE 705
#define SOF_TKN_INTEL_DMIC_PDM_SKEW 706

#define SOF_TKN_EFFECT_TYPE 900

#endif
277 changes: 236 additions & 41 deletions sound/soc/sof/topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,28 @@ static enum sof_ipc_frame find_format(const char *name)
return SOF_IPC_FRAME_S32_LE;
}

struct sof_effect_types {
const char *name;
enum sof_ipc_effect_type type;
};

static const struct sof_effect_types sof_effects[] = {
{"EQFIR", SOF_EFFECT_INTEL_EQFIR},
{"EQIIR", SOF_EFFECT_INTEL_EQIIR},
};

static enum sof_ipc_effect_type find_effect(const char *name)
{
int i;

for (i = 0; i < ARRAY_SIZE(sof_effects); i++) {
if (strcmp(name, sof_effects[i].name) == 0)
return sof_effects[i].type;
}

return SOF_EFFECT_INTEL_NONE;
}

/*
* Standard Kcontrols.
*/
Expand Down Expand Up @@ -276,6 +298,8 @@ static int sof_control_load_bytes(struct snd_soc_component *scomp,
{
struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp);
struct sof_ipc_ctrl_data *cdata;
struct snd_soc_tplg_bytes_control *control =
(struct snd_soc_tplg_bytes_control *)hdr;

/* init the get/put bytes data */
scontrol->size = SOF_IPC_MSG_MAX_SIZE;
Expand All @@ -290,6 +314,21 @@ static int sof_control_load_bytes(struct snd_soc_component *scomp,
dev_dbg(sdev->dev, "tplg: load kcontrol index %d chans %d\n",
scontrol->comp_id, scontrol->num_channels);

if (le32_to_cpu(control->priv.size) > SOF_IPC_MSG_MAX_SIZE) {
dev_warn(sdev->dev, "bytes priv data size %d too big\n",
control->priv.size);
return -EINVAL;
}

if (le32_to_cpu(control->priv.size) > 0) {
memcpy(cdata->data->data, control->priv.data,
le32_to_cpu(control->priv.size));
cdata->data->size = control->priv.size;
cdata->data->magic = SOF_ABI_MAGIC;
cdata->data->abi = SOF_ABI_VERSION;
cdata->data->comp_abi = SOF_ABI_VERSION;
}

return 0;
}

Expand Down Expand Up @@ -342,6 +381,15 @@ static int get_token_dai_type(void *elem, void *object, u32 offset, u32 size)
return 0;
}

static int get_token_effect_type(void *elem, void *object, u32 offset, u32 size)
{
struct snd_soc_tplg_vendor_string_elem *velem = elem;
u32 *val = object + offset;

*val = find_effect(velem->string);
return 0;
}

/* Buffers */
static const struct sof_topology_token buffer_tokens[] = {
{SOF_TKN_BUF_SIZE, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32,
Expand Down Expand Up @@ -405,14 +453,11 @@ static const struct sof_topology_token src_tokens[] = {
static const struct sof_topology_token tone_tokens[] = {
};

/* EQ FIR */
/*
static const struct sof_topology_token eq_fir_tokens[] = {
};
*/

/* EQ IIR */
static const struct sof_topology_token eq_iir_tokens[] = {
/* EFFECT */
static const struct sof_topology_token effect_tokens[] = {
{SOF_TKN_EFFECT_TYPE, SND_SOC_TPLG_TUPLE_TYPE_STRING,
get_token_effect_type,
offsetof(struct sof_ipc_comp_effect, type), 0},
};

/* PCM */
Expand Down Expand Up @@ -1345,67 +1390,217 @@ static int sof_widget_load_siggen(struct snd_soc_component *scomp, int index,
kfree(tone);
return ret;
}
/*
* Effect Topology. Only IIR equalizer is supported at this moment.
* TODO: Need to add also FIR support and have a way to add other
* effects and enhancements.
*/

static int sof_widget_load_effect(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)
static int sof_effect_fir_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)

{
struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp);
struct snd_soc_tplg_private *private = &tw->priv;
struct sof_ipc_comp_eq_iir *eq;
struct snd_sof_control *scontrol = NULL;
struct snd_soc_dapm_widget *widget = swidget->widget;
const struct snd_kcontrol_new *kc = NULL;
struct soc_bytes_ext *sbe;
struct sof_abi_hdr *pdata = NULL;
struct sof_ipc_comp_eq_fir *fir;
size_t ipc_size = 0, fir_data_size = 0;
int ret;

eq = kzalloc(sizeof(*eq), GFP_KERNEL);
if (!eq)
/* get possible eq controls */
kc = &widget->kcontrol_news[0];
if (kc) {
sbe = (struct soc_bytes_ext *)kc->private_value;
scontrol = sbe->dobj.private;
}

/*
* Check if there's eq parameters in control's private member and set
* data size accordingly. If there's no parameters eq will use defaults
* in firmware (which in this case is passthrough).
*/
if (scontrol && scontrol->cmd == SOF_CTRL_CMD_BINARY) {
pdata = scontrol->control_data->data;
if (pdata->size > 0 && pdata->magic == SOF_ABI_MAGIC)
fir_data_size = pdata->size;
}

ipc_size = sizeof(struct sof_ipc_comp_eq_fir) +
le32_to_cpu(private->size) +
fir_data_size;

fir = kzalloc(ipc_size, GFP_KERNEL);
if (!fir)
return -ENOMEM;

/* configure IIR EQ IPC message */
eq->comp.hdr.size = sizeof(*eq);
eq->comp.hdr.cmd = SOF_IPC_GLB_TPLG_MSG | SOF_IPC_TPLG_COMP_NEW;
eq->comp.id = swidget->comp_id;
eq->comp.pipeline_id = index;
/* configure fir IPC message */
fir->comp.hdr.size = ipc_size;
fir->comp.hdr.cmd = SOF_IPC_GLB_TPLG_MSG | SOF_IPC_TPLG_COMP_NEW;
fir->comp.id = swidget->comp_id;
fir->comp.type = SOF_COMP_EQ_FIR;
fir->comp.pipeline_id = index;

eq->comp.type = SOF_COMP_EQ_IIR;
ret = sof_parse_tokens(scomp, eq, eq_iir_tokens,
ARRAY_SIZE(eq_iir_tokens), private->array,
ret = sof_parse_tokens(scomp, &fir->config, comp_tokens,
ARRAY_SIZE(comp_tokens), private->array,
le32_to_cpu(private->size));
if (ret) {
dev_err(sdev->dev, "error: parse EQ tokens failed %d\n",
private->size);
if (ret != 0) {
dev_err(sdev->dev, "error: parse fir.cfg tokens failed %d\n",
le32_to_cpu(private->size));
goto err;
}

ret = sof_parse_tokens(scomp, &eq->config, comp_tokens,
sof_dbg_comp_config(scomp, &fir->config);

/* we have a private data found in control, so copy it */
if (fir_data_size > 0) {
memcpy(&fir->data, pdata->data, pdata->size);
fir->size = fir_data_size;
}

swidget->private = (void *)fir;

ret = sof_ipc_tx_message(sdev->ipc, fir->comp.hdr.cmd, fir,
ipc_size, r, sizeof(*r));
if (ret >= 0)
return ret;
err:
kfree(fir);
return ret;
}

static int sof_effect_iir_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)
{
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 = NULL;
struct soc_bytes_ext *sbe;
struct snd_sof_control *scontrol = NULL;
struct sof_abi_hdr *pdata = NULL;
struct sof_ipc_comp_eq_iir *iir;
size_t ipc_size = 0, iir_data_size = 0;
int ret;

/* get possible eq controls */
kc = &widget->kcontrol_news[0];
if (kc) {
sbe = (struct soc_bytes_ext *)kc->private_value;
scontrol = sbe->dobj.private;
}

/*
* Check if there's eq parameters in control's private member and set
* data size accordingly. If there's no parameters eq will use defaults
* in firmware (which in this case is passthrough).
*/
if (scontrol && scontrol->cmd == SOF_CTRL_CMD_BINARY) {
pdata = scontrol->control_data->data;
if (pdata->size > 0 && pdata->magic == SOF_ABI_MAGIC)
iir_data_size = pdata->size;
}

ipc_size = sizeof(struct sof_ipc_comp_eq_iir) +
le32_to_cpu(private->size) +
iir_data_size;

iir = kzalloc(ipc_size, GFP_KERNEL);
if (!iir)
return -ENOMEM;

/* configure iir IPC message */
iir->comp.hdr.size = ipc_size;
iir->comp.hdr.cmd = SOF_IPC_GLB_TPLG_MSG | SOF_IPC_TPLG_COMP_NEW;
iir->comp.id = swidget->comp_id;
iir->comp.type = SOF_COMP_EQ_IIR;
iir->comp.pipeline_id = index;

ret = sof_parse_tokens(scomp, &iir->config, comp_tokens,
ARRAY_SIZE(comp_tokens), private->array,
le32_to_cpu(private->size));
if (ret) {
dev_err(sdev->dev, "error: parse EQ.cfg tokens failed %d\n",
if (ret != 0) {
dev_err(sdev->dev, "error: parse iir.cfg tokens failed %d\n",
le32_to_cpu(private->size));
goto err;
}

dev_dbg(sdev->dev, "eq iir %s created\n", swidget->widget->name);

sof_dbg_comp_config(scomp, &eq->config);
sof_dbg_comp_config(scomp, &iir->config);

swidget->private = (void *)eq;
/* we have a private data found in control, so copy it */
if (iir_data_size > 0) {
memcpy(&iir->data, pdata->data, pdata->size);
iir->size = iir_data_size;
}

ret = sof_ipc_tx_message(sdev->ipc, eq->comp.hdr.cmd, eq,
sizeof(*eq), r, sizeof(*r));
swidget->private = (void *)iir;

ret = sof_ipc_tx_message(sdev->ipc, iir->comp.hdr.cmd, iir,
ipc_size, r, sizeof(*r));
if (ret >= 0)
return ret;
err:
kfree(eq);
kfree(iir);
return ret;
}

/*
* Effect Topology
*/

static int sof_widget_load_effect(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)
{
struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp);
struct snd_soc_tplg_private *private = &tw->priv;
struct sof_ipc_comp_effect config;
int ret;

/* check we have some tokens - we need at least effect type */
if (le32_to_cpu(private->size) == 0) {
dev_err(sdev->dev, "error: effect tokens not found\n");
return -EINVAL;
}

memset(&config, 0, sizeof(config));

/* get the effect token */
ret = sof_parse_tokens(scomp, &config, effect_tokens,
ARRAY_SIZE(effect_tokens), private->array,
le32_to_cpu(private->size));
if (ret != 0) {
dev_err(sdev->dev, "error: parse effect tokens failed %d\n",
le32_to_cpu(private->size));
return ret;
}

/* now load effect specific data and send IPC */
switch (config.type) {
case SOF_EFFECT_INTEL_EQFIR:
ret = sof_effect_fir_load(scomp, index, swidget, tw, r);
break;
case SOF_EFFECT_INTEL_EQIIR:
ret = sof_effect_iir_load(scomp, index, swidget, tw, r);
break;
default:
dev_err(sdev->dev, "error: invalid effect type %d\n",
config.type);
ret = -EINVAL;
break;
}

if (ret < 0) {
dev_err(sdev->dev, "error: effect loading failed\n");
return ret;
}

return 0;
}

/*
* Generic widget loader.
*/
Expand Down