tplg: refine topology file to support voulume with multi-channels #34

RanderWang · 2018-07-19T05:59:24Z

Using standard SND_SOC_TPLG_CTL_VOLSW to register our kcontrol function
because only it supports binding all kcontrol functions to bespoke ones
Info function can't be bound to a bespoke one with vendor ID

To support multi-channels volume control, please use these m4 files
in your topology files

Test is on APL, works
This patch works with:thesofproject/linux#39

Signed-off-by: Rander Wang rander.wang@linux.intel.com

RanderWang · 2018-07-19T06:03:32Z

This patch works with:thesofproject/linux#39

ranj063 · 2018-07-19T06:32:14Z

topology/sof/pipe-low-latency-capture.m4

 # Volume Mixer control with max value of 32
 C_CONTROLMIXER(PCM PCM_ID Capture Volume, PIPELINE_ID,
-	CONTROLMIXER_OPS(volsw, 256 binds the mixer control to volume get/put handlers, 256, 256),
+	CONTROLMIXER_OPS(volsw, 1 binds the mixer control to volume get/put handlers, 1, 1),


@RanderWang This seems incorrect. the sof volume_put/get handlers can handle any number of channels up to MAX chan allowed in topology?

yes, volume_put/get works with MAX chan. But user only see two channels in user mode. Please
refer to my thesofproject/linux#39

this also makes no sense to me. this would require all topology files to be changes to remove this special value of 256 used to bind volume with kcontrols.

@RanderWang if the issue is because the default info function used in topology (volsw) supports only 2 channels, then the right fix for this issue would be have a bespoke info function in SOF as well? as Pierre pointed out, we cannot change the binding for all the topologies that are known to work today.

@ranj063 yes, I totally agree with you. But I have pointed out that: if the id is set to 256, the info function cant be binded to a bespoke sof info function. This is the limitation of Asoc

@RanderWang, i am not sure this is the right thing to do. But can we try "volsw_xr_sw" for info instead of "volsw" to get around the stereo restriction?

@ranj063 "volsw_xr_sw" doesn't work and it makes topology loaded failed.
To get info function to be bound with vendor id like 256, a great change will be made
in Asoc. I discussed with Mengdong who is a expert in topology, and got this conclusion.

ranj063 · 2018-07-19T06:34:48Z

topology/m4/mixercontrol.m4


+dnl KCONTROL_2CHANNELS(reg)
+define(`KCONTROL_2CHANNELS',
+`KCONTROL_CHANNEL(FL, $1, 0),'


@RanderWang we dont really use the reg and shift values in our IO handler at all. Should we just remove them so that people dont get confused about why it is like this?

it is used in soc-topology.c in ASOC

@RanderWang the question is if it's used in the SOF code.

@RanderWang agreed it may be used by other kcontrol handlers. But have you verified the reg and shift values are correct? I still think we should remove them.

@ranj063 @plbossart It is not used in SOF now, but not never. I verify the the reg and shift, no problem. It is used in Asoc for two usages. The first usage is to check the volume control is for mono audio or for stereo audio. If it is removed, it is for mono audio. The second usage is to set hardware register. But for SOF, it is no use.

plbossart

I don't get what you are trying to do

the binding change looks wrong, this change will break all known topology files
the speaker mapping isn't right in all cases, e.g. capture.

And please fix typos in commit messages....

plbossart · 2018-07-19T13:34:53Z

topology/m4/mixercontrol.m4


+dnl KCONTROL_2CHANNELS(reg)
+define(`KCONTROL_2CHANNELS',
+`KCONTROL_CHANNEL(FL, $1, 0),'


@RanderWang the question is if it's used in the SOF code.

plbossart · 2018-07-19T13:37:32Z

topology/m4/mixercontrol.m4

+`KCONTROL_CHANNEL(FC, $1, 4),'
+`KCONTROL_CHANNEL(LFE, $1, 5),'
+`KCONTROL_CHANNEL(SL, $1, 6),'
+`KCONTROL_CHANNEL(SR, $1, 7)')


This is a bit controversial. It's not because there are 8 channels that they will follow the speaker arrangement that you describe here. It's probably a better idea to keep the names less precise, e.g. vol_ch0..vo_chl7, to avoid confusions.
Take the example of capture on the GP-MRB, the channels are FM, AUX and mic, nothing to do with 7.1 speaker notation

I followed the topology definition in alsa lib, else it would be built failed

The channel map reg is the register offset for the control, shift is the

bit shift within the register for the channel and the section name is the

channel name and can be one of the following :-

mono # mono stream

fl # front left

fr # front right

rl # rear left

rr # rear right

fc # front center

lfe # LFE

sl # side left

sr # side right

rc # rear center

plbossart · 2018-07-19T13:38:09Z

topology/m4/mixercontrol.m4

+dnl KCONTROL_CHANNEL_ARRAY(size, reg)
+define(`KCONTROL_CHANNEL_ARRAY',
+`ifelse(
+        $1, `2', `KCONTROL_2CHANNELS($2)',


what about odd number of channels. it's perfectly doable in TDM mode...

Thanks, I will refine it

plbossart · 2018-07-19T13:39:32Z

topology/sof/pipe-low-latency-capture.m4

 # Volume Mixer control with max value of 32
 C_CONTROLMIXER(PCM PCM_ID Capture Volume, PIPELINE_ID,
-	CONTROLMIXER_OPS(volsw, 256 binds the mixer control to volume get/put handlers, 256, 256),
+	CONTROLMIXER_OPS(volsw, 1 binds the mixer control to volume get/put handlers, 1, 1),


this also makes no sense to me. this would require all topology files to be changes to remove this special value of 256 used to bind volume with kcontrols.

plbossart · 2018-07-19T13:39:51Z

topology/sof/pipe-low-latency-playback.m4

 # Volume Mixer control with max value of 32
 C_CONTROLMIXER(PCM PCM_ID Playback Volume, PIPELINE_ID,
-	CONTROLMIXER_OPS(volsw, 256 binds the mixer control to volume get/put handlers, 256, 256),
+	CONTROLMIXER_OPS(volsw, 1 binds the mixer control to volume get/put handlers, 1, 1),


same here, not sure why you are changing this binding.

see my above comments and thesofproject/linux#39

plbossart · 2018-07-19T13:40:17Z

topology/sof/pipe-low-latency-playback.m4

 # Volume Mixer control with max value of 32
 C_CONTROLMIXER(Master Playback Volume, PIPELINE_ID,
-	CONTROLMIXER_OPS(volsw, 256 binds the mixer control to volume get/put handlers, 256, 256),
+	CONTROLMIXER_OPS(volsw, 1 binds the mixer control to volume get/put handlers, 1, 1),


here too, why change to 1?

see my above comments and thesofproject/linux#39

RanderWang · 2018-07-20T03:14:56Z

[Qurestion] the binding change looks wrong, this change will break all known topology files
[Answer] no, 256 still works because it is still kept in SOF kernel driver and registered to ASOC.
If this id is not registered to Asoc, it would stop working

RanderWang · 2018-07-20T06:27:54Z

update my patch, thanks for review.

I discussed with Meng dong, made clear something
(1) Is this patch compatible with existing topology files?
Yes, I just add another path, not rebuild one. The legacy
topology files still work

(2) Does vender id like 256 still works?
yes, it still works with two channels volume controller

(3)why use standard id to support multi-channels? not vendor id.
we need binding info function in kcontrol to return correct channel number.
It can't be done with vendor id because there is a limitation in Asoc.
At the design time of topology ABI, it is expected that info function should
be the default one, so the info id is used as a type not a id. But later, the feature
that info function can be bound to a bespoke one is added. It is a pity that
the info id is wrong. So the new feature doesn't work at all.

to change ABI is impossible. And to hacking in Asoc to fix this issue is a great
work. Now the standard id works. It is ok.

lgirdwood · 2018-07-20T09:39:15Z

@RanderWang the idea behind the venor IDs is that you can create your own versions of get(), put() and info() or reuse existing versions. I'm not following you here as to why the vendor versions can't use 8 channels, can you point me to the limitation in topology or ASoC core that is blocking this for you.

ranj063 · 2018-07-20T18:15:22Z

@RanderWang the SOF kcontrol volume_put() IO handler takes care of reading the volume table and sending the gain to the firmware. How is the standard kcontrol handler doing this?

ranj063 · 2018-07-21T04:33:00Z

@RanderWang browsing through the legacy driver code, I ran into sst_slot_enum_info which seems to be a bespoke info callback for an enum control. This might be useful for you to add the info callback to the SOF IO handler.

RanderWang · 2018-07-23T07:58:35Z

@ranj063 yes, in sst driver, it use standard id: SNDRV_CTL_ELEM_IFACE_MIXER
What I use is also standard id: SND_SOC_TPLG_CTL_VOLSW. So no difference.
Thank your for your info!

RanderWang · 2018-07-23T08:11:03Z

@lgirdwood the following lines are gotten from the alsa lib.

This mapping shows info() using the standard "volsw" info callback whilst
the get() and put() are mapped to bespoke driver callbacks.
The Standard operations names for control get(), put() and info calls
are :-
- volsw
- volsw_sx
- volsw_xr_sx
- enum

At the design time, info function can't be bound to bespoke one.

/*

Genericl Operations IDs, for binding Kcontrol or Bytes ext ops
Kcontrol ops need get/put/info.
Bytes ext ops need get/put.
*/
struct snd_soc_tplg_io_ops {
__le32 get;
__le32 put;
__le32 info;
} attribute((packed));
you can get idea from the comments. info is not used as get & put. It is set to volsw, and get & put are set to vendor defined id, for example 256.

the usage of info in soc_tplg_kcontrol_elems_load
switch (control_hdr->ops.info) {
case SND_SOC_TPLG_CTL_VOLSW:
case SND_SOC_TPLG_CTL_STROBE:
case SND_SOC_TPLG_CTL_VOLSW_SX:
....

in function:soc_tplg_kcontrol_bind_io

	if (k->put == NULL && ops[i].id == hdr->ops.put) // 256 == 256
		k->put = ops[i].put;
	if (k->get == NULL && ops[i].id == hdr->ops.get) // 256 == 256
		k->get = ops[i].get;
	if (k->info == NULL && ops[i].id == hdr->ops.info) // 256 != 1 (VOLSW)
		k->info = ops[i].info;

the get & put can be bound for id are equal, but for info, id is not equal.

I discussed with Meng Dong. She said it is a mistake, and you can't expect it at design time.

lgirdwood · 2018-07-24T09:07:00Z

topology/m4/mixercontrol.m4

+`KCONTROL_CHANNEL(FR, $1, 1),'
+`KCONTROL_CHANNEL(FC, $1, 2)')
+
+dnl KCONTROL_4CHANNELS(reg)


We also need to watch the naming here, because it should also imply any channel mapping (rather than just calling it 4channels). Can you have a look and see if ALSA has a naming scheme for common channel mappings and reuse.

Yes, I check it in alsa, but I don't find any usage of 4 channels except mono or stereo audio. I search in google and find that audio with more then 2 channels are all called surround audio.

Using standard SND_SOC_TPLG_CTL_VOLSW to register our kcontrol function because only it supports binding all kcontrol functions to bespoke ones Info function can't be bound to a bespoke one with vendor ID To support multi-channels volume control, please use these m4 files in your topology files Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

RanderWang · 2018-07-25T08:40:48Z

Refine the name of channel, thanks for review

plbossart · 2018-07-25T15:19:45Z

@RanderWang it seems to me you are trying to work-around a limitation in the ASoC topology core and/or alsa-lib, so why not fix this limitation directly? I don't get your point that the ABI cannot be changed, sure it can as long as it's backwards compatible.

RanderWang · 2018-07-26T05:38:08Z

@plbossart yes, it is a work-around method. I want to use our vendor id, but it does't work. And after I thought many times, I found standard id work. I also discussed with MengDong about ABI. If ABI is modified, asoc and alsa lib also need to be refined. This results all the released topology files can't work with modified linux kernel. She got great pain with a case like this.

RanderWang mentioned this pull request Jul 19, 2018

The volume of rear channels can not be changed on GP platform #31

Closed

ranj063 reviewed Jul 19, 2018

View reviewed changes

plbossart suggested changes Jul 19, 2018

View reviewed changes

RanderWang force-pushed the volume branch from 911c99e to e9fda80 Compare July 20, 2018 06:24

lgirdwood suggested changes Jul 24, 2018

View reviewed changes

RanderWang force-pushed the volume branch from e9fda80 to 9a9d1af Compare July 25, 2018 08:35

RanderWang mentioned this pull request Jul 25, 2018

ASoC: SOF: refine sof for multi-channels support thesofproject/linux#39

Closed

RanderWang closed this Aug 20, 2018

tplg: refine topology file to support voulume with multi-channels #34

tplg: refine topology file to support voulume with multi-channels #34

Uh oh!

Conversation

RanderWang commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Jul 19, 2018

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!

ranj063 Jul 19, 2018 • 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!

RanderWang Jul 20, 2018 • 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!

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!

RanderWang Jul 19, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang Jul 20, 2018 • 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!

plbossart 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ranj063 commented Jul 20, 2018

Uh oh!

ranj063 commented Jul 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

RanderWang commented Jul 19, 2018 •

edited

Loading

ranj063 Jul 19, 2018 •

edited

Loading

RanderWang Jul 20, 2018 •

edited

Loading

RanderWang Jul 19, 2018 •

edited

Loading

RanderWang Jul 20, 2018 •

edited

Loading

plbossart left a comment •

edited

Loading

RanderWang commented Jul 20, 2018 •

edited

Loading

RanderWang commented Jul 20, 2018 •

edited

Loading

lgirdwood commented Jul 20, 2018 •

edited

Loading

ranj063 commented Jul 21, 2018 •

edited

Loading

RanderWang commented Jul 23, 2018 •

edited

Loading

RanderWang commented Jul 23, 2018 •

edited

Loading

RanderWang Jul 25, 2018 •

edited

Loading

RanderWang commented Jul 26, 2018 •

edited

Loading