[RFC] generic effect type with fir and iir eq

[juimonen]

This is a demo/test to try to separate different effect types. Needs a lot of massaging and review. For actually to work, you need corresponding PR's from firmware and tools.

[ranj063]

@juimonen could you please update the commit message for the first commit? It seems to be empty

[ranj063]

can this be moved to load_effect instead of duplicating it for load_iir and load_fir?

[juimonen]

Yes will try to do it.

[juimonen]

now thinking of this, the ipc struct (including config) is created only in the specialized effect loading. I should create separate config then in effect_load and send it as parameter to special effect loading. IMHO actually does not help too much and makes things little bit more confusing (also need to take care of extra error cases).

[ranj063]

typo in the comment?

[juimonen]

yeah will fix

[juimonen]

btw this also in tonegen...where I copied this part :)

[ranj063]

Is this the size of the data required for configuring the effect component?
Why should it be limited to 156? Should it be max ipc size - the headers?

[juimonen]

it is probably a bad name... It is the position of widget priv + comp data + effect type (where the actual eq payload starts). I understood the comp data needs to be there all the time. Now I'm adding the effect type. As the data following is binary blob, I can't rely on the vendor array mechanism to get to the proper position.

[ranj063]

I see then maybe it should be called PAYLOAD_OFFSET?

[ranj063]

also I'm thinking, should this be hardcoded? What if one of the values changes?

[juimonen]

I mean somehow we need to have an "agreement" what is in the priv member before the payload starts and what is the size of it. I thought before the situation was that for pipeline components you need to have the sof_comp_tokens there. if there's something following that (like bytes field) you somehow need to know the offset to that. I guess the vendor token parsing is updating the array pointer, but I'm not sure can that be used (is the position guaranteed to be after tuples). I'm open to suggestions :)

[ranj063]

@juimonen the commit title prefix should follow the standard prefixes we have such as ASOC: SOF: topology

[ranj063]

we need an error message here

[juimonen]

ok will add

[ranj063]

I think the last argument could just be le32_to_cpu(private->size) just like in every other case. @juimonen what do you think?

[juimonen]

if I understood correctly, sof_parse_tokens doesn't like if the size spans the whole priv struct. It assumes it's all vendor token arrays and tries to read the following bytes field as tokens (will fail). So this is limiting the amount and making this actually work.

[ranj063]

@juimonen I still think this line should be le32_to_cpu(private->size) instead of SOF_EFFECT_PAYLOAD_OFFSET

[juimonen]

I think my previous comment was lost due to pr update. So sof_parse_tokens, assumes that the private->array consist solely of vendor tokens and it tries to parse them as much as le32_to_cpu(private->size) is. Now if the private has also other stuff like bytes fields the function will try to read that as tokens (and will fail, because array size is random etc.). That's why I have to "limit" the size what is given to token parsing. Not sure then If I have understood the parsing incorrectly...?

[ranj063]

@juimonen I think sof_parse_tokens only tries to find the tokens in the token_type_array (comp_tokens in the above call) in the private->array and this precisely why I am worried that this wont work if we add things later. The comp_tokens could be anywhere in the private->array and we need to do a full sweep.

[juimonen]

hmm ok. But I guess then we assume that the binary blob mixed with vendor arrays always contains first 4 bytes as size?

[juimonen]

@lgirdwood any comments how should I parse the bytes field mixed with vendor arrays in the widget private data? Also should I use the abi_hdr for sending the bytes to firmware?

[lgirdwood]

@juimonen iirc, this has it's own private data structure and does not use tuples. i.e. you create a private data structure and append the bytes to the end of it.

[ranj063]

@juimonen sorry it took me a while to get back to it. But I understand the sticky situation here. Can we pass this as a private data for the byte control associated with the EQ instead? Then there will not be any mixing of vendor tuples array and byte data.

[ranj063]

@juimonen what happens if there is more than 1 control. Is that allowed?

[juimonen]

I mean I check that the comp_id of the control matches this components id? Can you set multiple controls from topology, isn't that quite bad already? If there are multiple controls for 1 component then this code will try to take the params from the first control...

[ranj063]

@juimonen yes you can have as many controls in topology as you want. Thats why we should check for sanity.

[ranj063]

if there's no private data for the control, will it cause a failure in the component load?

[juimonen]

no, the data is copied only if the private data exists

[juimonen]

so I tried this with "empty" data section and also removing data section completely from control in topology. eq loads fine, but there's no parameters so its passthrough

[ranj063]

OK can you please add it in the comments that if private data is empty, it will be passthrough?

[ranj063]

same here. I think we should check that we have one control or type byte control

[juimonen]

this is the following line if (scontrol->cmd == SOF_CTRL_CMD_BINARY &&
scontrol->comp_id == swidget->comp_id)

[ranj063]

@juimonen also I dont think we need to loop through the list. The widget->kcontrol_news[0] will be the control for this widget. So if we should sure there's only one control for eq and then use that one.

[juimonen]

@ranj063 updated according to your comments

[plbossart]

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

[juimonen]

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.