ASoC: SOF: control: override volume info callback by juimonen · Pull Request #2469 · thesofproject/linux

juimonen · 2020-09-24T19:02:10Z

ASoC dapm controls don't support more than 2 channels. If we want to
provide more controllable channels to user space we need to override the
info callback. This is the case for example with 4 channel dmic pga.
Only controls with more than 2 channels will use sof info callback.
Mono and stereo will use the existing dapm info callback.

Signed-off-by: Jaska Uimonen jaska.uimonen@linux.intel.com

ranj063 · 2020-09-24T20:26:50Z


+	/*
+	 * if control has more than 2 channels, we need to override the info as
+	 * asoc layer dapm controls support only mono and stereo.


no its more because the pre-defined mixer control types only support upto stereo channels. As far as the control definition goes, it can have up to 8 channels

ok. i'll refine this. I think this is still correct, in a way. I know that kcontrol supports more channels, but dapm restricts it to max 2. So I mean you can define control to have bigger channel count, but most control related functions in soc/dapm layer just don't want to co-operate if you have > 2 channels.

tried to make this better, but probably still confusing to people not familiar with the issue...

singalsu

No opinion from me for doing this in ASoC vs. SOF but this approach for SOF looks good to me and I've tested it to work correctly with a APL UP2 4ch capture topology with old 2ch configured volume and updated 4ch configured topology file. 2ch case worked as before (channels 3-4 remained without vol & mute control). 4ch case applied all common and individually set volumes and mutes correctly.

kv2019i

Thanks @juimonen . This seems like the most sensible approach to fix the current issue we have.

In longer term, I think ASoC should evolve towards better support for multichannel (now there is widespread use of "is_stereo()" logic in soc-topology.c. This affects all ASoC users, so this work will have to be done incrementally.

I think the current patch is ok. If/when the "is_stereo()" limitations are removed from ASoC core, we can just drop the info override in SOF. I think this is a better approach than adding new machine to override the info method.

juimonen · 2020-09-25T12:27:30Z

related fw pr: thesofproject/sof#3470

ranj063 · 2020-09-25T18:24:57Z

@plbossart what do you think of this one?

plbossart · 2020-09-25T18:40:06Z

Errors on BYT_MB_NOCODEC? let's see if a second CI run makes this go away?

plbossart · 2020-09-25T18:40:14Z

SOFCI TEST

plbossart

I don't have enough background to review this patch without additional comments/explanations.

Also I am not sure doing an SOF-specific fix makes sense. Why not fix the real issue instead of working around it?

plbossart · 2020-09-25T18:43:11Z

+
+	if (!sm->platform_max)
+		sm->platform_max = sm->max;
+	platform_max = sm->platform_max;


It wouldn't hurt to add a comment on what max and platform_max represent? I am not familiar with all this and can't follow if the code makes sense.

@plbossart This is verbatim following soc-opc.c:snd_soc_info_volsw_range(), i.e. the function we override. There seems to be support to a range of channels that do not start from 1 and thus the code is a bit funky. But yeah, following soc-ops.c seems fine.

plbossart · 2020-09-25T18:44:14Z

+		sm->platform_max = sm->max;
+	platform_max = sm->platform_max;
+
+	if (platform_max == 1 && !strstr(kcontrol->id.name, " Volume"))


also don't get what this means and the intent of this test.

@plbossart Ditto here same as soc-ops.c

plbossart · 2020-09-25T18:46:04Z

+	 * If control has more than 2 channels we need to override the info. This is because even if
+	 * ASoC layer has defined topology's max channel count to SND_SOC_TPLG_MAX_CHAN = 8, the
+	 * pre-defined dapm control types (and related functions) creating the actual control
+	 * restrict the channels only to mono or stereo.


so the question will be: why not fix the ASoC topology core, instead of doing an SOF-specific fix?

Intel is pretty much the only user of the topology, so it'd be odd not to fix what we depend on?

@plbossart its a bit of a chicken and egg kind of situation. topolopgy expects one of the three kinds of controls (mixer, enum or bytes) and this is determined based on the info op ID. So, bespoke info IDs are not allowed. But adding a bespoke info ID is also hard because we dont know what type of control to load in this case.

What I think might be a better thing to do would be to introduce a new type of info called SND_SOC_TPLG_CTL_VOLSW_MULTI_CHANNEL or something like that and use that in tplg. This would require changes in alsalib as well. @juimonen what do you think?

is there a reason why SND_SOC_TPLG_CTL_VOLSW would be limited to 2ch?

juimonen · 2020-09-28T07:52:10Z

@plbossart , @ranj063

The asoc control mechanism's seems to be inherently mono/stereo. I don't know the reason for this, maybe historically that has just been the dominant case. Also it is heavily based on register access and shift mechanism in it. So I guess some/many hw has split hw register for stereo volume control and you use shift to access the correct channel.

Anyway, there's quite many legacy codecs using this. These are not even Intel stuff, but other vendors, and it would be really difficult for me to make sure I don't break those. So this is the main reason I would not like to touch the asoc part.

We are not using this register twiddling, as we are sending messages to dsp over ipc, thus we are overriding the get/put methods. However we are not overriding the info method, because you can't do that. So for some reason asoc has enabled the bespoke override to get/put, but not info. Again I don't know is there any good reason for this, or that just nobody has ever needed that. Somehow it would seem symmetrical that we would maybe override info also as we override get/put?

The info method in this PR is just a copy from the asoc one, except I return the "correct" channel count. So those twiddlings with "Volume " strings and others are from asoc, I just don't want to break amixer/alsamixer mechanisms if someone is relying on those. As said above I can't override the info from topology, but I have to do it "violently" in sof mixer creation. And I wanted to limit the damage to only controls > 2 channels, as I think there still will be very few of those. So this override is currently only for the 4 channel dmic, everything else works as before.

So I see 3 options here:

modify asoc to support multi channel (biggest work, risky because of legacy)
modify asoc to support bespoke info callback from topology (quite easy to do, but touches asoc, don't know if acceptable upstream)
Ranjani's idea of adding new "official" callback for multi channel (doable, but touches all possible layers)
override info callback in sof for multi channel (this PR)

My personal choices would be 2 & 4 :)

ranj063 · 2020-09-28T17:16:25Z

So I see 3 options here:

modify asoc to support multi channel (biggest work, risky because of legacy)

modify asoc to support bespoke info callback from topology (quite easy to do, but touches asoc, don't know if acceptable upstream)

Ranjani's idea of adding new "official" callback for multi channel (doable, but touches all possible layers)

override info callback in sof for multi channel (this PR)

My personal choices would be 2 & 4 :)

@juimonen 2. is not straightforward. If the info is bespoke, you cant tell which type of kcontrol to create isnt it(I mean mixer or enum or bytes or something else). What you will have to do in this case is leave itup to the ASoC driver to all the steps needed to create the kcontrol and do nothing in the core. Its doable and probably worth it too.

plbossart · 2020-09-28T19:31:44Z

@plbossart , @ranj063

The asoc control mechanism's seems to be inherently mono/stereo. I don't know the reason for this, maybe historically that has just been the dominant case. Also it is heavily based on register access and shift mechanism in it. So I guess some/many hw has split hw register for stereo volume control and you use shift to access the correct channel.

Anyway, there's quite many legacy codecs using this. These are not even Intel stuff, but other vendors, and it would be really difficult for me to make sure I don't break those. So this is the main reason I would not like to touch the asoc part.

I feel exactly the other way. If we don't roll-up our sleeves and provide multi-channel support, nobody will, so we are going to be constrained forever due to limitations from 10 years ago.

We are not using this register twiddling, as we are sending messages to dsp over ipc, thus we are overriding the get/put methods. However we are not overriding the info method, because you can't do that. So for some reason asoc has enabled the bespoke override to get/put, but not info. Again I don't know is there any good reason for this, or that just nobody has ever needed that. Somehow it would seem symmetrical that we would maybe override info also as we override get/put?

is this actually possible? If you can override the info, then how would kernel+userspace know what the control is?

The info method in this PR is just a copy from the asoc one, except I return the "correct" channel count. So those twiddlings with "Volume " strings and others are from asoc, I just don't want to break amixer/alsamixer mechanisms if someone is relying on those. As said above I can't override the info from topology, but I have to do it "violently" in sof mixer creation. And I wanted to limit the damage to only controls > 2 channels, as I think there still will be very few of those. So this override is currently only for the 4 channel dmic, everything else works as before.

So I see 3 options here:

modify asoc to support multi channel (biggest work, risky because of legacy)

modify asoc to support bespoke info callback from topology (quite easy to do, but touches asoc, don't know if acceptable upstream)

Ranjani's idea of adding new "official" callback for multi channel (doable, but touches all possible layers)

override info callback in sof for multi channel (this PR)

My personal choices would be 2 & 4 :)

I vote for 1. 4 would be an acceptable solution until 1 is implemented.

juimonen · 2020-09-29T07:30:15Z

@plbossart you're right about the info type issue. I can try to change the asoc controls to multi-channel, and we can ask upstream or some other expert what they think about it? My initial work was actually like that, but I got scared on the way...

lgirdwood · 2020-09-29T10:04:17Z

Fwiw, this is relatively simple enough to be a "fix" patch for back porting to stable kernels if needed.

kv2019i · 2020-09-30T12:55:59Z

I (still) give my vote for 4 (for 5.10 and backports). Option 1 for 5.11/5.12 sound good and I agree with Pierre that unless we do it, probably noone will, so we should have a go.

plbossart

i will approve since it's the only pragmatic solution short-term, but we'll have to change the commit message before upstreaming to state that we are going to submit patches to remove the restrictions.

plbossart · 2020-09-30T13:52:59Z

SOFCI TEST

juimonen · 2020-10-01T11:42:55Z

@plbossart @ranj063 @kv2019i

I made this now to enable info callback setup from topology:
#2483

Reason: I tried to add the multi channel support to volsw and others, but it is just really awkward and doesn't feel right. Also I'm not sure I understand all possible register/shift combinations that could rise from multi channel. Those methods are really hardcoded for mono/stereo register access and they have all kinds of quirks in them. Also I'm not sure if this kind of register fiddling even exists in any codec's (possible) multi channel control register. Also we are not using almost any methods in soc-ops so I don't see too much point of hacking the whole thing just for channel_count. info override just seems a better fit for this and with the above PR you can choose from topology if using it or not.

plbossart · 2020-10-01T16:40:54Z

@plbossart @ranj063 @kv2019i

I made this now to enable info callback setup from topology:
#2483

Reason: I tried to add the multi channel support to volsw and others, but it is just really awkward and doesn't feel right. Also I'm not sure I understand all possible register/shift combinations that could rise from multi channel. Those methods are really hardcoded for mono/stereo register access and they have all kinds of quirks in them. Also I'm not sure if this kind of register fiddling even exists in any codec's (possible) multi channel control register. Also we are not using almost any methods in soc-ops so I don't see too much point of hacking the whole thing just for channel_count. info override just seems a better fit for this and with the above PR you can choose from topology if using it or not.

Just for 'fun' I started hacking ASoC to use an array of regs and shifts, instead of reg/rreg and shift/rshift. It's not too hard to do, but indeed I don't get when one value is enough but with 2 shifts, etc. This stuff is confusing,

https://github.com/plbossart/sound/tree/fix/mixer-channels

juimonen · 2020-10-01T17:05:25Z

@plbossart I did the same array thing 2 times little bit differently and fell into the same rabbit hole. So exactly how the "1 register 2 shifts", "2 registers 1 shift" and "2registers 2 shifts" can be expanded to multi channel... And as we are not using these registers in sof at all...

plbossart · 2020-10-01T17:09:05Z

@plbossart I did the same array thing 2 times little bit differently and fell into the same rabbit hole. So exactly how the "1 register 2 shifts", "2 registers 1 shift" and "2registers 2 shifts" can be expanded to multi channel... And as we are not using these registers in sof at all...

@lgirdwood these concepts of 'reg' and 'shift' have been part of ASoC since 2005, and it's not clear to any of us why we can have 1 or 2 registers, or 1 and 2 shifts. Do you mind shedding some light on what the idea was? If we want to extend to ASoC/DAPM to multichannel controls, how should we go about this?

kv2019i · 2020-10-01T17:51:17Z

Just a note: #2483 implements option (2) we discussed here

It seems cleaner to me, but as @juimonen pointed in the PR comment, it breaks compatibility with older kernels. I think we could do it and just bump the kernel req for V1.7, but not this is worth the trouble (over (4)).

@plbossart W.r.t. your comment "we'll have to change the commit message", let's do that before merging. At least to me it's unclear what we should put in the commit message. Looking at soc-ops.c today, it's no longer clear to me how'd one go about change this. Not sure we put promises to the git commit message, unless we have some line of sight. How about we send this patch, and open a separate discussion thread w.r.t. future of soc-ops.c?

plbossart · 2020-10-01T18:21:09Z

suggested commit message:

ASoC dapm controls currently don't support more than 2 channels. This is a problem for SOF-based devices where individual volume control cannot be provided on the 4 DMIC input path.

If we want to provide controls for more that 2 channels, this patch suggests a simple solution based on an override of the info callback. For example, in the case with 4 channel DMIC PGAs, an sof_info callback would be used. Mono and stereo cases will keep using the existing dapm info callback.

A longer-term solution would be to remove the limits to 2 channels in ASoC/DAPM/topology. This is a topic Intel is currently looking into, e.g. by removing the use of 'reg' and 'rreg' fields and use arrays instead. Such changes will be rather intrusive and touch multiple codec and platform drivers. Removing restrictions is the right thing to do, but this will need to be done in steps with lots of validation.

ASoC dapm controls currently don't support more than 2 channels. This is a problem for SOF-based devices where individual volume control cannot be provided on the 4 DMIC input path. If we want to provide controls for more than 2 channels, this patch suggests a simple solution based on an override of the info callback. For example, in the case with 4 channel DMIC PGAs, a sof_info callback would be used. Mono and stereo cases will keep using the existing dapm info callback. A longer-term solution would be to remove the limits to 2 channels in ASoC/DAPM/topology. This is a topic Intel is currently looking into, e.g. by removing the use of 'reg' and 'rreg' fields and use arrays instead. Such changes will be rather intrusive and touch multiple codec and platform drivers. Removing restrictions is the right thing to do, but this will need to be done in steps with lots of validation. Signed-off-by: Jaska Uimonen <jaska.uimonen@linux.intel.com>

juimonen · 2020-10-02T10:40:56Z

@plbossart @kv2019i commit message updated.

plbossart · 2020-10-02T18:39:37Z

@marc-hb Travis CI seems broken, not sure if this is an issue with .yaml file or an infrastructure issue?

marc-hb · 2020-10-02T22:14:08Z

All the failures in https://travis-ci.org/github/thesofproject/linux/jobs/732214817 look like this:

$ sudo apt-get update -qq
W: GPG error: https://packages.erlang-solutions.com/ubuntu focal Release: The following signatures were invalid: BADSIG D208507CA14F4FCA Erlang Solutions Ltd. <packages@erlang-solutions.com>
E: The repository 'https://packages.erlang-solutions.com/ubuntu focal Release' is not signed.
The command "sudo apt-get update -qq" failed and exited with 100 during .

Some other, concurrent builds in passed at the same time on a different container!

marc-hb · 2020-10-02T22:14:25Z

SOFCI TEST

juimonen requested review from bardliao, dbaluta, kv2019i, lgirdwood, plbossart, ranj063 and singalsu as code owners September 24, 2020 19:02

ranj063 reviewed Sep 24, 2020

View reviewed changes

Comment thread sound/soc/sof/control.c Resolved

ranj063 reviewed Sep 24, 2020

View reviewed changes

Comment thread sound/soc/sof/control.c Resolved

ranj063 reviewed Sep 24, 2020

View reviewed changes

juimonen force-pushed the multi_channel_volume branch from a4fce26 to 56b495d Compare September 25, 2020 07:16

singalsu previously approved these changes Sep 25, 2020

View reviewed changes

kv2019i previously approved these changes Sep 25, 2020

View reviewed changes

juimonen mentioned this pull request Sep 25, 2020

topology: add dmic channel maps for different channel counts thesofproject/sof#3470

Merged

ranj063 previously approved these changes Sep 25, 2020

View reviewed changes

plbossart reviewed Sep 25, 2020

View reviewed changes

plbossart previously approved these changes Sep 30, 2020

View reviewed changes

plbossart mentioned this pull request Oct 1, 2020

[RFC] add possibility for bespoke info #2483

Closed

juimonen dismissed stale reviews from plbossart, ranj063, kv2019i, and singalsu via fae7a9c October 2, 2020 10:40

juimonen force-pushed the multi_channel_volume branch from 56b495d to fae7a9c Compare October 2, 2020 10:40

kv2019i approved these changes Oct 2, 2020

View reviewed changes

kv2019i requested review from plbossart, ranj063 and singalsu October 2, 2020 18:33

ranj063 approved these changes Oct 2, 2020

View reviewed changes

plbossart approved these changes Oct 2, 2020

View reviewed changes

singalsu mentioned this pull request Oct 5, 2020

[BUG]When set capture volume to 0 DMIC 4ch still can capture voice #1799

Closed

kv2019i merged commit c8849c7 into thesofproject:topic/sof-dev Oct 6, 2020

Conversation

juimonen commented Sep 24, 2020

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!

singalsu left a comment • 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!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

juimonen commented Sep 25, 2020

Uh oh!

ranj063 commented Sep 25, 2020

Uh oh!

plbossart commented Sep 25, 2020

Uh oh!

plbossart commented Sep 25, 2020

Uh oh!

plbossart left a comment

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!

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!

juimonen commented Sep 28, 2020

Uh oh!

ranj063 commented Sep 28, 2020

Uh oh!

plbossart commented Sep 28, 2020

Uh oh!

juimonen commented Sep 29, 2020

Uh oh!

lgirdwood commented Sep 29, 2020

Uh oh!

kv2019i commented Sep 30, 2020

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart commented Sep 30, 2020

Uh oh!

juimonen commented Oct 1, 2020

Uh oh!

plbossart commented Oct 1, 2020

Uh oh!

juimonen commented Oct 1, 2020

Uh oh!

plbossart commented Oct 1, 2020

Uh oh!

kv2019i commented Oct 1, 2020

Uh oh!

plbossart commented Oct 1, 2020

Uh oh!

juimonen commented Oct 2, 2020

Uh oh!

plbossart commented Oct 2, 2020

Uh oh!

marc-hb commented Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Oct 2, 2020

singalsu left a comment •

edited

Loading

marc-hb commented Oct 2, 2020 •

edited

Loading