Audio: Volume: Handle all volume ramp types #8207

singalsu · 2023-09-15T12:04:43Z

The first patch adds a conversion from IPC4 to SOF volume ramp types.

The second patch changes further volume to be able to handle the supported ramp types.

The third patch adds to gain.conf the other supported ramp types. The default is kept as windows fade.

singalsu · 2023-09-15T12:12:21Z

@ranj063 @kv2019i Does this change prevent topology to work with IPC4 reference firmware - and does it matter? If that is the case, then we need to also re-order for IPC4 the table

sof/src/include/ipc/topology.h

Line 151 in 036da2a

enum sof_volume_ramp {

or create new ipc4 specific table with windows_no_fade and windows_fade as first entries.

For performance reasons we want to enable topology to select also other (lighter) ramp types.

btian1 · 2023-09-15T13:24:15Z

tools/topology/topology2/include/components/gain.conf

@@ -151,7 +159,7 @@ Class.Widget."gain" {
 	uuid 			"A8:A9:BC:61:D0:18:18:4A:8E:7B:26:39:21:98:04:B7"
 	no_pm			"true"
 	cpc 			10183
-	curve_type		"fade"
+	curve_type		"windows_fade"


is it still default as fade, right? how to make it configurable?

Yep, but it should be another PR (change default to "linear" and have e.g. in nocodec topologies this set into "windows_fade" to keep it tested).

The intention with this patch is to fix wrong IPC sent to firmware.

I just heard that windows does not use tplg conf definiton, so I fully agree with Seppo's comments: default set to linear
and left windows fade to nocodec tplg.

This PR keeps operation unchanged with windows fade as default, just re-arranged the enum values.

I'd like to make a separate PR to change the default type to linear and change nocodec topology to use the windows fade curve.

ranj063 · 2023-09-15T15:14:22Z

@ranj063 @kv2019i Does this change prevent topology to work with IPC4 reference firmware - and does it matter? If that is the case, then we need to also re-order for IPC4 the table

sof/src/include/ipc/topology.h

Line 151 in 036da2a

enum sof_volume_ramp {

or create new ipc4 specific table with windows_no_fade and windows_fade as first entries.
For performance reasons we want to enable topology to select also other (lighter) ramp types.

@singalsu im not sure if this will affect the ref fw. But AFAIK this wouldnt be the first thing that breaks the ref fw. Maybe we should limit to testing just the passthrough pipelines with the ref fw to avoid any issues.

singalsu · 2023-09-18T08:29:21Z

@ranj063 @kv2019i Does this change prevent topology to work with IPC4 reference firmware - and does it matter? If that is the case, then we need to also re-order for IPC4 the table

sof/src/include/ipc/topology.h

Line 151 in 036da2a

enum sof_volume_ramp {

or create new ipc4 specific table with windows_no_fade and windows_fade as first entries.
For performance reasons we want to enable topology to select also other (lighter) ramp types.

@singalsu im not sure if this will affect the ref fw. But AFAIK this wouldnt be the first thing that breaks the ref fw. Maybe we should limit to testing just the passthrough pipelines with the ref fw to avoid any issues.

We could

Decide to change the ramp enum types for IPC4 so that SOF volume would remain compatible with Windows driver. In this case the binary would be compatible.
Or make the enums reference FW compatible with a kconfig build option
Ignore the difference and test with pass-through topologies only the reference FW

What should we do?

kv2019i

Hmm, the condition enum def is a problem. Can we have a separate enum for IPC4?

kv2019i · 2023-09-18T16:12:07Z

src/include/ipc/topology.h

@@ -156,6 +166,7 @@ enum sof_volume_ramp {
 	SOF_VOLUME_WINDOWS_FADE,
 	SOF_VOLUME_WINDOWS_NO_FADE,
 };
+#endif


Ouch, this is a problem. We have these definitions also in Linux kernel and we can't have #ifdef for IPC version. So if we need separate definitions, we need to use a different name.

@kv2019i ，seems sof driver does not have this definition.
@singalsu , if remove ipc3 sof_volume_ramp, then we can change:

then all ipc3 and ipc4 are aligned.

@kv2019i The kernel treats this as generic u32

https://github.com/thesofproject/linux/blob/cffcac365eac0d60dc38959cadaa5078bc924b8e/sound/soc/sof/ipc3-topology.c#L119

https://github.com/thesofproject/linux/blob/cffcac365eac0d60dc38959cadaa5078bc924b8e/sound/soc/sof/ipc4-topology.c#L130

There is this header but it's not used. Could we remove it instead?

https://github.com/thesofproject/linux/blob/cffcac365eac0d60dc38959cadaa5078bc924b8e/include/sound/sof/topology.h#L138

@kv2019i As other fix approach could duplicate all ramp types to

sof/src/audio/volume/peak_volume.h

Line 44 in b608ec8

enum ipc4_curve_type {

and add a conversion function from enum ipc4_peak_volume_param to enum sof_volume_ramp. What do you prefer?

@singalsu Can we have separate enums for IPC3 and IPC4 and also remove the unused enum in kernel? I think that would be the cleanest (no ifdefs in headers, no mismatch between definitions between kernel and FW).

tools/topology/topology2/include/components/gain.conf

singalsu · 2023-09-20T12:52:45Z

Re-request review from everyone since the patch is now very different. Hopefully like @kv2019i suggested. Since the code now touches a part with known issue I included also fix for code that prevented other than windows fade to work in ipc4 build.

btian1 · 2023-09-21T01:19:44Z

src/audio/volume/peak_volume.h

+	IPC4_AUDIO_CURVE_TYPE_LINEAR,
+	IPC4_AUDIO_CURVE_TYPE_LOG,
+	IPC4_AUDIO_CURVE_TYPE_LINEAR_ZC,
+	IPC4_AUDIO_CURVE_TYPE_LOG_ZC,


I like this sequence, 0, with windows fade, and then linear.
it brings:

compatible with windows.

easy to remove later curve type.

btian1 · 2023-09-21T01:27:04Z

src/audio/volume/volume.c

+#endif
+#if CONFIG_COMP_VOLUME_LINEAR_RAMP
+		case SOF_VOLUME_LINEAR:
+		case SOF_VOLUME_LINEAR_ZC:


does LINEAR_ZC also use same ramp?

Yes both use the linear equation.

btian1 · 2023-09-21T01:38:21Z

src/audio/volume/volume_ipc4.c

-	cd->initial_ramp = Q_MULTSR_32X32((int64_t)curve_duration,
-					  Q_CONVERT_FLOAT(1.0 / 10000, 31), 0, 31, 0);
+	if (cd->ramp_type == SOF_VOLUME_WINDOWS_NO_FADE)
+		cd->initial_ramp = 0;


I'd like to add:
cd->ramp_finished = true;
to fast skip the ramp in process.

Yes, good catch, I'll add it and test it.

RanderWang · 2023-09-21T08:22:04Z

@ranj063 @kv2019i Does this change prevent topology to work with IPC4 reference firmware - and does it matter? If that is the case, then we need to also re-order for IPC4 the table

sof/src/include/ipc/topology.h

Line 151 in 036da2a

enum sof_volume_ramp {

or create new ipc4 specific table with windows_no_fade and windows_fade as first entries.

For performance reasons we want to enable topology to select also other (lighter) ramp types.

Actually the old SOF definition is conflicted with windows usage. This PR makes it aligned with windows

enum CurveType
{
    AUDIO_CURVE_TYPE_NONE = 0,
    AUDIO_CURVE_TYPE_WINDOWS_FADE = 1
};

lgirdwood · 2023-09-26T13:18:35Z

@kv2019i did enum get fixed to your approval ?

singalsu · 2023-10-03T15:08:43Z

"46 successful and 1 failing checks" , rebase and retry with same patch.

This patch adds to enum ipc4_curve_type in peak_volume.h ramp types for linear and logarithmic with and without zero crossings detect. The conversion function ipc4_curve_type_convert() converts the type into enum sof_volume_ramp for use in volume.c. The set_volume_ipc4() is changed to use the convert function and the restriction to select other than windows fade type also is removed. Signed-off-by: Seppo Ingalsuo <[email protected]>

The windows fade, windows no fade, and linear and handled the switch-case statement change into volume.c. In IPC4 the ramp can be disabled with zero curve duration or with no fade type. The ramp duration convert multiplication can be passed with the no fade type. Signed-off-by: Seppo Ingalsuo <[email protected]>

This patch adds the other SOF volume ramp types linear and logarithmic with and without zero crossings detect mode to gain widget class. The names of fade values are changed to be similar as in enum sof_volume_ramp, since there is a specific curve shape required for Windows OS. Signed-off-by: Seppo Ingalsuo <[email protected]>

lgirdwood · 2023-10-04T11:11:58Z

"46 successful and 1 failing checks" , rebase and retry with same patch.

Unrelated Zephyr build issue.

kv2019i

Thanks @singalsu , looks good now!

singalsu marked this pull request as ready for review September 15, 2023 12:07

singalsu requested review from ranj063 and jsarha as code owners September 15, 2023 12:07

btian1 reviewed Sep 15, 2023

View reviewed changes

btian1 approved these changes Sep 18, 2023

View reviewed changes

singalsu force-pushed the tplg_fix_gain_curve_types branch from 23fb338 to fb4db2b Compare September 18, 2023 10:04

singalsu requested review from RanderWang, marcinszkudlinski and pblaszko as code owners September 18, 2023 10:04

singalsu changed the title ~~Tools: Topology2: Fix the gain.conf curve_type values~~ IPC: topology.h: Fix enum sof_volume_ramp compatibility in IPC4 Sep 18, 2023

marcinszkudlinski approved these changes Sep 18, 2023

View reviewed changes

kv2019i requested changes Sep 18, 2023

View reviewed changes

RanderWang approved these changes Sep 19, 2023

View reviewed changes

andrula-song approved these changes Sep 20, 2023

View reviewed changes

singalsu force-pushed the tplg_fix_gain_curve_types branch from fb4db2b to 655f5c7 Compare September 20, 2023 12:50

singalsu requested review from lgirdwood, plbossart, mmaka1, lbetlej and dbaluta as code owners September 20, 2023 12:50

singalsu requested review from btian1, kv2019i, RanderWang, marcinszkudlinski and andrula-song September 20, 2023 12:52

singalsu changed the title ~~IPC: topology.h: Fix enum sof_volume_ramp compatibility in IPC4~~ IAudio: Volume: Handle all volume ramp types Sep 20, 2023

btian1 reviewed Sep 21, 2023

View reviewed changes

btian1 mentioned this pull request Sep 21, 2023

audio: volume: enable linear ramp for ipc4 volume #8213

Merged

andrula-song approved these changes Sep 21, 2023

View reviewed changes

singalsu changed the title ~~IAudio: Volume: Handle all volume ramp types~~ Audio: Volume: Handle all volume ramp types Sep 21, 2023

singalsu force-pushed the tplg_fix_gain_curve_types branch from 655f5c7 to 8947ed1 Compare September 21, 2023 13:11

lgirdwood approved these changes Sep 22, 2023

View reviewed changes

singalsu force-pushed the tplg_fix_gain_curve_types branch from 8947ed1 to 92aa144 Compare September 25, 2023 10:54

singalsu added 3 commits October 3, 2023 18:09

singalsu force-pushed the tplg_fix_gain_curve_types branch from 92aa144 to ff098ce Compare October 3, 2023 15:10

lgirdwood approved these changes Oct 4, 2023

View reviewed changes

kv2019i approved these changes Oct 4, 2023

View reviewed changes

kv2019i merged commit 51159d8 into thesofproject:main Oct 4, 2023
37 of 38 checks passed

btian1 mentioned this pull request Oct 20, 2023

[BUG][MTL] High Peak/Average MCPS ratio for gain components #8162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audio: Volume: Handle all volume ramp types #8207

Audio: Volume: Handle all volume ramp types #8207

singalsu commented Sep 15, 2023 •

edited

Loading

singalsu commented Sep 15, 2023 •

edited

Loading

btian1 Sep 15, 2023

singalsu Sep 15, 2023

btian1 Sep 18, 2023 •

edited

Loading

singalsu Sep 18, 2023

ranj063 commented Sep 15, 2023

singalsu commented Sep 18, 2023

kv2019i left a comment

kv2019i Sep 18, 2023

btian1 Sep 19, 2023 •

edited

Loading

singalsu Sep 19, 2023

singalsu Sep 19, 2023

kv2019i Sep 20, 2023

singalsu commented Sep 20, 2023

btian1 Sep 21, 2023

btian1 Sep 21, 2023

singalsu Sep 21, 2023

btian1 Sep 21, 2023

singalsu Sep 21, 2023

RanderWang commented Sep 21, 2023

lgirdwood commented Sep 26, 2023

singalsu commented Oct 3, 2023

lgirdwood commented Oct 4, 2023

kv2019i left a comment

Audio: Volume: Handle all volume ramp types #8207

Audio: Volume: Handle all volume ramp types #8207

Conversation

singalsu commented Sep 15, 2023 • edited Loading

singalsu commented Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btian1 Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranj063 commented Sep 15, 2023

singalsu commented Sep 18, 2023

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btian1 Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singalsu commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanderWang commented Sep 21, 2023

lgirdwood commented Sep 26, 2023

singalsu commented Oct 3, 2023

lgirdwood commented Oct 4, 2023

kv2019i left a comment

Choose a reason for hiding this comment

singalsu commented Sep 15, 2023 •

edited

Loading

singalsu commented Sep 15, 2023 •

edited

Loading

btian1 Sep 18, 2023 •

edited

Loading

btian1 Sep 19, 2023 •

edited

Loading