enable multiple type controls for single widget #2423

juimonen · 2020-09-08T17:38:20Z

I was experimenting with beamformer with 2 controls (byte and enum) and
noticed this restriction: It seems that widget can have multiple controls but
they are inherently assumed to be of the same type. The alsa topology
documentation is a little bit hazy on this part...

The diff looks quite nasty and invasive, but it is only refactoring the kcontrol
creation/deletion to be based on type instead of just the topology chunk size.
Also it seems that these struct members I'm touching are not used too much
by anyone else than sof...

I tried this with eq by adding bytes and enum and it seemed to work.

juimonen · 2020-09-08T17:44:01Z

related fw PR: thesofproject/sof#3396

plbossart · 2020-09-08T22:21:57Z

Before I even review the code, may I ask what the idea for having a enum+a byte control might be?

Can't we fold the enum value into the byte control?
Can we actually set the enum independently from the byte control?

bardliao · 2020-09-09T02:49:40Z

Will it be simpler to create a new widget associated to the beamformer widget? In other words split the widget to two widgets and we can have different controls.

juimonen · 2020-09-09T05:52:12Z

@plbossart we we're seeing at least 2 use cases where we would/could use this. However, I admit there are several different strategies how you could achieve the same:

eq with enum control for setting presets and byte control to set the presets. There are many ways what the relation between these 2 control is i.e. what happens actually in the dsp component when using bytes (is it updating all presets, current preset etc)
beamformer using byte controls to set configuration and enum used to convey the sound direction to user space. Enum could also be used for beamformer preset (for e.g. beam on/off) control

@bardliao yes I guess that would be possible, however I don't personally like creating "fake" widgets to enable several controls. We are using this in some dai configuration and it is really messy, nobody understands the topologies etc. Also I don't like creating extra components to dsp, that are not doing actual processing, just receiving commands.

juimonen · 2020-09-09T06:05:33Z

@plbossart probably it would be possible to embed the enum somehow to bytes, but I would say the usage would be really confusing.

I don't see why we could not set the enum independently... I mean we have dual controls for 1 component in topologies today, like dual mixer controls with slider & switch for pga. The restriction is that they need to be of the same type. So I could also "legally" create processing component with 2 byte controls. But I don't really see, what is the purpose of this restriction.

So I guess we could use 2 byte controls for processing component, 1 to set the configuration and 1 as "fake" enum.

plbossart · 2020-09-09T18:59:10Z

Sorry @juimonen I don't get the idea and your wording "eq with enum control for setting presets and byte control to set the presets" confuses me. Why do you need presets in the first place? That's extra memory storage requirements in the firmware and you could pass the binary values when needed.

juimonen · 2020-09-10T04:45:00Z

@plbossart all fair points about the eq...but: yep maybe the eq preset's could be done by just changing the parameters with byte controls i.e. adding the preset functionality to user space. We could also remove all mute switches, because you can just use volume to mute your pga? Or we could remove volume ramps, because you can interpolate it with volume control? maybe these are not equal examples, but just pointing out it is kind of different/parallel/easier method of using the component. Maybe we could use the enum to "standardize" the eq preset across platforms and use the control to setup the platform independent params in boot (?).

btw we already support sending indexed multiple transfer function "block" to eq with byte control i.e. defining the presets.

Anyway the main point here is:

alsa already supports multiple controls for 1 widget
however, they need to be of the same type
is there some inherent reason these types need to be the same, and can't we just remove that restriction?

plbossart · 2020-09-10T13:33:11Z

@plbossart all fair points about the eq...but: yep maybe the eq preset's could be done by just changing the parameters with byte controls i.e. adding the preset functionality to user space. We could also remove all mute switches, because you can just use volume to mute your pga? Or we could remove volume ramps, because you can interpolate it with volume control? maybe these are not equal examples, but just pointing out it is kind of different/parallel/easier method of using the component. Maybe we could use the enum to "standardize" the eq preset across platforms and use the control to setup the platform independent params in boot (?).

btw we already support sending indexed multiple transfer function "block" to eq with byte control i.e. defining the presets.

Anyway the main point here is:

alsa already supports multiple controls for 1 widget

however, they need to be of the same type

is there some inherent reason these types need to be the same, and can't we just remove that restriction?

Volume and mute are typically separate. See how HDAudio, USB and SoundWire expose volume/mute controls separately.

Yes you got my point that presets could be handled in userspace without any limitation of firmware storage size.

I don't have a fundamental objection to the idea of having two separate controls, but if there's no critical need for it it means we can take more time in the reviews. If I get your explanation this is really an enablement patch, not a product feature.

plbossart

Seems to me that we need to change the widget definitions rather than the control definitions?

sound/core/control.c

sound/soc/soc-topology.c

plbossart · 2020-09-10T21:50:37Z

sound/soc/sof/topology.c

well something is really odd here. If we want to 'enable multiple type controls for single widgets', then this field should be removed completely.

Of why not instead make kcontrol_type an array. e.g.

switch (widget->dobj.widget.kcontrol_type[i]) {

and don't change the kcontrols themselves.

hmm I've changed this already, I think I've not updated the correct version, sorry about this, will push new version

oh sorry will take it back. was confused by the diff...

So yes was thinking about the array, but what is would be the size of the array? anyway we are having the individual kcontrol structs, so why not have types in those? Don't have to worry/check array bounds or dynamic memory allocation..

Now thinking it, maybe the use of kcontrol_new struct is not the best then. I think we have "somewhere" present 3 different structs: 1) kcontrol_new, 2) actual alsa kcontrol (created from 1), and 3) sof_control. So some of those should/could be used here.

And I still insist that it would be good to have the type in the control struct, not in widget :)

@ranj063 I think you are atm the best kcontrol expert on this planet, what do you think?

sorry im not getting the question here. or has the fix already been pushed and I'm missing context?

singalsu · 2020-11-18T18:41:12Z

@juimonen This PR needs a rebase to fix conflicts.

sound/soc/soc-topology.c

singalsu

I think function snd_sof_enum_get() in control.c should do a snd_sof_ipc_set_get_comp_data() to update the value from firmware and not just return the previous set command data from cache.

juimonen · 2020-11-27T13:27:20Z

@singalsuo rebased on top of sof-dev. I added control counters, so we can now get indexing of controls.

sound/soc/soc-topology.c

lyakh · 2020-12-03T08:55:25Z

sound/soc/soc-topology.c

so now this field will remain uninitialised, i.e. set to 0 by memset(&template, 0,...) and 0 is an invalid kcontrol type, is that ok?

this is related to that previously there was assumption, that only 1 type of control type is associated with the widget... so what this whole PR is trying to change :) So it should not matter at all what the value is. I tried to search for it and I think the only place it was ever used was in sof (and I'm changing it here).

juimonen · 2020-12-03T12:53:11Z

@lyakh updated per your comments. Sorry about that, I had to do "semi-manual rebase" and missed couple of changes. I tried to clean the obviously clear/responded comments, but left open some which might need ack.

lyakh

thanks for fixing! Let me override my "request for changes" before leaving for a vacation

juimonen · 2021-02-04T09:06:48Z

@plbossart @kv2019i @lgirdwood ping for this. Situation:

asked upstream comments: "don't touch alsa core" -> done -> now touching only asoc. Kind of skeptical I'll get more comments (V2 sent to upstream -> silence).
Pierre's request not to break bisect -> done -> asoc and sof changes combined to 1 patch -> could not find any elegant other solution
I'm aware this might cause discussion if sent to upstream within our sof patches, but AFAIK no one is using this feature or related parts of code -> I should not break anything existing in upstream

plbossart · 2021-02-04T15:26:57Z

I'll be honest @juimonen, it's been close to 6 months since the first submission and I am having a hard-time figuring out how urgent/important this PR is? To put things differently, what happens if we don't do anything, what are the negative consequences and limitations that would impact us?

juimonen · 2021-02-04T15:40:36Z

@plbossart nothing happens, this is a new feature. I've had couple of use cases where this would be good to have: for e.g. beamformer is one use case where we want to set the parameters with byte control and then use enum for receiving direction of arrival. Other is demux where Liam wants enum control, but I just don't know any other means to set the default params than byte control (hence 2 different controls), maybe it is then UCM etc. We could use all kinds of virtual components etc for this, but frankly to me those sound much worse kludge than this.

Anyway, if you don't like this, let's leave it. We can revisit this later if desperate need emerges.

plbossart · 2021-02-04T16:19:55Z

This feels like a nice-to-have feature but if SOF is the only user, and only tentative, it's going to be hard to get any traction.
If we are going to change the controls and topology, maybe to make things more palatable to upstream maintainers we need to have a standalone test based on what Amadeusz contributed. That way it'd feel like a full-blown capability that's tested outside of SOF. my 2 cents.

lyakh · 2021-02-05T07:14:31Z

@plbossart nothing happens, this is a new feature. I've had couple of use cases where this would be good to have: for e.g. beamformer is one use case where we want to set the parameters with byte control and then use enum for receiving direction of arrival. Other is demux where Liam wants enum control, but I just don't know any other means to set the default params than byte control (hence 2 different controls), maybe it is then UCM etc. We could use all kinds of virtual components etc for this, but frankly to me those sound much worse kludge than this.

@juimonen I thought this would allow us to get rid of binary controls, is it not the case?

juimonen · 2021-02-05T07:29:09Z

@lyakh not as such. Currently asoc allows you to create multiple controls for single widget, so you can have for example 2 volume controls or 2 byte controls etc. However, for some reason you can't have different type of controls (for a single widget). I have no idea why this is so, also upstream didn't comment too much...

OTH I think you can create this kind of widget currently with alsatplg (so with multiple different control types), and god knows what happens in the asoc in that case... it might try to create several instances of the first type it encounters.

I stumbled into this when trying to create enums for mux and left the existing byte control there (for setting default parameters). I agree that this is maybe not the most common use case, but we already could use this in beamformer (set parameters with bytes, get direction of arrival with enum).

So I can't categorize is this "enhancement" or "bug fix" etc. as even upstream hasn't been able to tell me "yes it is intended to be so that you can only create similar type controls -> don't change it" or that "oh you can do that in topology, but not parse that in asoc -> that's a bug".

OTH this should not cause any change to existing topologies/usage in upstream.

lyakh · 2021-02-05T07:49:19Z

@lyakh not as such. Currently asoc allows you to create multiple controls for single widget, so you can have for example 2 volume controls or 2 byte controls etc. However, for some reason you can't have different type of controls (for a single widget). I have no idea why this is so, also upstream didn't comment too much...

@juimonen ok, thanks! Would be pity if we drop this now without pushing upstream IMHO.

singalsu · 2021-02-08T18:37:46Z

@plbossart I just changed for my own tests the beamformer PR code from using binary + enum + enum to binary + binary + binary. The control IPC does not pass an index to controls so the binary applies everything to index = 0 control and the functionality is messed up totally. There's at least need to change the kernel to provide the index to controls to be able to handle the multiple controls.

While it's arguable that the direction controls could be binary (lose the alsamixer control but get better 32 bit precision) the beam on/off fits naturally to enum control type.

Also with binary direction control I'd need to populate git repo with 360 different pre-generated direction binary blobs for beamformer. Well maybe not 360 but quite many at least to cover the angles with fair precision (every 10 degrees?). And binary blobs for beam on / off. The same needs to be multiplied for every algorithm.

plbossart · 2021-02-08T20:15:18Z

@singalsu @juimonen maybe you misunderstood my feedback.

I am not opposed to change, I am only suggesting that our chances of upstreaming changes are higher if we show that the new capability applies beyond SOF, and that it can be tested independently of SOF.

There is one example that I just thought of where this capability could prove useful. In SDCA codecs, we have a concept of 'Entities' which expose N controls each (bypass, bassboost, volume, AGC, etc). These entities form a graph and it'd be natural to map them to 'DAPM widgets', and have different controls for each widget. We may actually be able to simplify SDCA codec drivers.
@shumingfan @jack-cy-yu @bardliao @ranj063 FYI.

lgirdwood · 2021-02-09T15:57:44Z

I am not opposed to change, I am only suggesting that our chances of upstreaming changes are higher if we show that the new capability applies beyond SOF, and that it can be tested independently of SOF.

Ack - this needs to be like a "ASoC: DAPM: Add support for multiple kcontrol types to a widget" i.e. it's generic in nature and useful for everybody.

kv2019i · 2021-02-16T17:15:14Z

@plbossart @lgirdwood Hmm, why wait with the upstreaming though? Jaroslav wanted to keep this within ASoC, but that can be addressed and otherwise there weren't any objections. It would seem SOF beamformer would be the perfect example use-case how to use this capability. Having this capability in, would allow for incremental move to non-binary controls for DSP processing.

lgirdwood · 2021-02-18T13:27:20Z

@plbossart @lgirdwood Hmm, why wait with the upstreaming though? Jaroslav wanted to keep this within ASoC, but that can be addressed and otherwise there weren't any objections. It would seem SOF beamformer would be the perfect example use-case how to use this capability. Having this capability in, would allow for incremental move to non-binary controls for DSP processing.

Ack, see my comment above, making this ASoC specific is good for me. Additionally having a use case is even better as part of the upstreaming. No objection from me to continue with this work and integrate with beamformer.

juimonen · 2021-04-29T06:19:39Z

@plbossart can we revisit this again? my apologies for not pushing this very actively, but seems we now would have some use for this in thesofproject/sof#4017.

I tried to read the longish comment chain above, but could not find any critical issues why this would not go in (or what I should still do to make it better). It is very possible that someone in upstream doesn't like this, but at least the upstream comments have been asked&addressed and sof seems to be the only driver using the parts related to this PR.

fuyuntsuo · 2021-04-29T07:29:07Z

@plbossart can we revisit this again? my apologies for not pushing this very actively, but seems we now would have some use for this in thesofproject/sof#4017.

I tried to read the longish comment chain above, but could not find any critical issues why this would not go in (or what I should still do to make it better). It is very possible that someone in upstream doesn't like this, but at least the upstream comments have been asked&addressed and sof seems to be the only driver using the parts related to this PR.

My misunderstanding of kcontrol usage origins from the layout of component topology

SectionWidget.XXX {
    index
    type
    no_pm
    data
    bytes
}

This kind of layout implicitly suggests that more than one type of kcontrol can be added to the widget.

Below is an example explicitly indicating only one type of kcontrol can be added to the widget.

SectionData.XXX_kcontrol_type {
}

SectionData.XXX_kcontrol_data {
}

SectionWidget.XXX {
    index
    type
    no_pm
    data
    control [
        XXX_kcontrol_type
        XXX_kconrol_data1
        XXX_kconrol_data2
    ]
}

However I am positive seeing ASoC to support multiple types of kcontrol because igo_nr needs a (one-shot) preset parameter set and an (run-time) on/off switch simultaneously.

juimonen · 2021-04-29T10:08:05Z

@fuyuntsuo it is not your fault... :) the documentation for this is non-existing. So basically multiple different controls are fine in topology, the alsa_tplg parser accepts these just fine. However the asoc code in alsa driver assumes all the controls are of the same type. Probably because all use cases of multiple controls have been volume+switch in pga's...

outdated review

plbossart · 2021-04-29T14:04:58Z

@plbossart can we revisit this again? my apologies for not pushing this very actively, but seems we now would have some use for this in thesofproject/sof#4017.

I tried to read the longish comment chain above, but could not find any critical issues why this would not go in (or what I should still do to make it better). It is very possible that someone in upstream doesn't like this, but at least the upstream comments have been asked&addressed and sof seems to be the only driver using the parts related to this PR.

I have no fundamental opposition to this change but it's a really big change to review, and I just don't have time to allocate on this. I removed by 'change requested' review and will others chime in, hopefully someone has more availability than me.

ranj063 · 2021-04-29T16:43:09Z

sound/soc/soc-topology.c

is this really necessary?

no, will remove

ranj063 · 2021-04-29T16:46:47Z

sound/soc/soc-topology.c

@juimonen is this correct? Isnt index supposed to be passed from topology?

I didn't see it coming from topology. That would be nice because you could assign whatever index from topology (and trust in your fw component for that index to identify to correct control). @singalsuo and I tested this and as far as I remember, there's no any integer identifier you could set from topology, or that alsa_tplg parser would assign automatically.

@juimonen index in topology is always interpreted as the pipeline_id. So assigning the contro l count would be incorrect. The index is usually in the section header and if we need the index thats what we should use.

@ranj063 we are already giving separately the tplg->index as argument to control_load ops from soc->sof (and not then actually using it for anything). the "count" here is not count, it is a running index. There is no way otherwise firmware could otherwise separate controls from each other...

so index in kcontrol_new is not used by us for anything. And if I understand correctly, it is used in core/control.c to verify the size of the kcontrol array together with count. I would say assigning pipeline id into it would also be quite wrong...? I mean pipeline_id can be whatever and it has nothing to do with size of the control array.

@juimonen ok it makes sense now.

ranj063 · 2021-04-29T16:51:15Z

@juimonen just a couple of comments besides: can you please split the patch into 2. The first one could simply move the for loop out of the individual control create functions into the soc_tplg_dapm_widget_create() function. The second patch would then address the fact that the control types could be different.

Current dapm widget has a single variable to describe its kcontrol's type. As there can be many kcontrols in one widget it is inherently presumed that the types are the same. Lately there has been use cases where different types of kcontrols would be needed for a single widget. Thus add pointer to dapm widget to hold an array for different kcontrol types and modify the kcontrol creation to operate in a loop based on individual kcontrol type. Change control creation and deletion to use individual kcontrol types in SOF driver. This is done in the same patch for not breaking bisect. SOF driver is also currently the only one using the dapm widget kcontrol_type. Signed-off-by: Jaska Uimonen <jaska.uimonen@linux.intel.com>

lyakh · 2021-04-30T07:58:29Z

apart from the comments from @ranj063 I didn't find any suspicious changes from the previous version, and all tests pass... So, let's just remove that NULL assignment, as for the index field - I don't think it's currently used, is it?

juimonen requested review from bardliao, dbaluta, kv2019i, lgirdwood, plbossart, ranj063 and singalsu as code owners September 8, 2020 17:38

juimonen mentioned this pull request Sep 8, 2020

[WIP][RFC] add enum control to topology and test multiple controls with iir thesofproject/sof#3396

Closed

plbossart previously requested changes Sep 10, 2020

View reviewed changes

juimonen force-pushed the multi_control branch from 8622012 to dcc0f7a Compare September 11, 2020 06:17

lyakh requested changes Nov 19, 2020

View reviewed changes

sound/soc/soc-topology.c Outdated Show resolved Hide resolved

sound/soc/soc-topology.c Outdated Show resolved Hide resolved

sound/soc/soc-topology.c Outdated Show resolved Hide resolved

singalsu reviewed Nov 23, 2020

View reviewed changes

juimonen force-pushed the multi_control branch from dcc0f7a to ad2dd25 Compare November 27, 2020 13:24

juimonen force-pushed the multi_control branch from ad2dd25 to dcf84fb Compare November 27, 2020 13:35

lyakh requested changes Dec 3, 2020

View reviewed changes

juimonen force-pushed the multi_control branch 2 times, most recently from 424bb37 to 2864a49 Compare December 3, 2020 12:50

lyakh previously approved these changes Dec 4, 2020

View reviewed changes

juimonen mentioned this pull request Dec 16, 2020

[RFC]Mux enum thesofproject/sof#3695

Closed

plbossart dismissed lyakh’s stale review via ac7f2cf February 10, 2021 16:56

juimonen force-pushed the multi_control branch from ac7f2cf to e973371 Compare February 17, 2021 12:00

juimonen mentioned this pull request Mar 3, 2021

src: codec_adapter: Add process passthrough path thesofproject/sof#3766

Closed

juimonen mentioned this pull request Apr 28, 2021

audio: igo_nr: added igo_nr component thesofproject/sof#4017

Merged

ranj063 reviewed Apr 29, 2021

View reviewed changes

juimonen force-pushed the multi_control branch from e973371 to 405f024 Compare April 30, 2021 08:08

lyakh approved these changes May 3, 2021

View reviewed changes

ranj063 approved these changes May 3, 2021

View reviewed changes

ranj063 merged commit 434aeb3 into thesofproject:topic/sof-dev May 3, 2021

enable multiple type controls for single widget #2423

enable multiple type controls for single widget #2423

Uh oh!

Conversation

juimonen commented Sep 8, 2020

Uh oh!

juimonen commented Sep 8, 2020

Uh oh!

plbossart commented Sep 8, 2020

Uh oh!

bardliao commented Sep 9, 2020

Uh oh!

juimonen commented Sep 9, 2020

Uh oh!

juimonen commented Sep 9, 2020

Uh oh!

plbossart commented Sep 9, 2020

Uh oh!

juimonen commented Sep 10, 2020

Uh oh!

plbossart commented Sep 10, 2020

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singalsu commented Nov 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

singalsu left a comment

Choose a reason for hiding this comment

Uh oh!

juimonen commented Nov 27, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juimonen Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juimonen commented Dec 3, 2020

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

juimonen commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Feb 4, 2021

Uh oh!

juimonen commented Feb 4, 2021

Uh oh!

plbossart commented Feb 4, 2021

Uh oh!

lyakh commented Feb 5, 2021

Uh oh!

juimonen commented Feb 5, 2021

Uh oh!

lyakh commented Feb 5, 2021

Uh oh!

singalsu commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

juimonen Dec 3, 2020 •

edited

Loading

juimonen commented Feb 4, 2021 •

edited

Loading

singalsu commented Feb 8, 2021 •

edited

Loading

fuyuntsuo commented Apr 29, 2021 •

edited

Loading