[DNM] Audio: Volume: Skip stream start fade as defined in Kconfig

[singalsu]

No description provided.

[btian1]

those changes will make volume more complicated, we may need investigate and discuss first before take action.

[singalsu]

I tested all kconfig permutations so they work, but yes, there's more complexity.

[kv2019i]

As this is not performance critical, I'd put the checks in an inline function and have just one call to volume_prepare_start_with_fade() and one to volume_prepare_start_without_fade().

static inline bool start_with_fade(direction)
{
#if CONFIG_COMP_VOLUME_PLAYBACK_START_FADE
	if (direction == SOF_IPC_STREAM_PLAYBACK)
                return true;
#endif 
#if CONFIG_COMP_VOLUME_CAPTURE_START_FADE
	if (direction == SOF_IPC_STREAM_CAPTURE)
                return true;
#endif 

        return false;
}

I think we can spare one conditional check in prepare().

[btian1]

don't add new code if it is not must, our initial problem is high ramp mcps, now, with change to linear and optimize windows fade, we already get good results, as liam just mentioned in mail, we should keep module easy to read/understand, especially for volume such simply module.

If volume is complicated, how can we do for other real complicated modules.

[singalsu]

@lgirdwood @btian1 @wszypelt Seems we can't disable the playback start ramp in Kconfig without failing the 01_10_TestGainPeakVolApi test case. What should we do?

In my tests with UPX I could not hear any start pops so this change could be done. If the test build could set CONFIG_COMP_VOLUME_PLAYBACK_START_FADE the test should pass.

[kv2019i]

One typoe and one code refactor proposal.

[kv2019i]

typoe: "Pops in playback direction"

[kv2019i]

As this is not performance critical, I'd put the checks in an inline function and have just one call to volume_prepare_start_with_fade() and one to volume_prepare_start_without_fade().

static inline bool start_with_fade(direction)
{
#if CONFIG_COMP_VOLUME_PLAYBACK_START_FADE
	if (direction == SOF_IPC_STREAM_PLAYBACK)
                return true;
#endif 
#if CONFIG_COMP_VOLUME_CAPTURE_START_FADE
	if (direction == SOF_IPC_STREAM_CAPTURE)
                return true;
#endif 

        return false;
}

I think we can spare one conditional check in prepare().

[andrula-song]

@lgirdwood @btian1 @wszypelt Seems we can't disable the playback start ramp in Kconfig without failing the 01_10_TestGainPeakVolApi test case. What should we do?

In my tests with UPX I could not hear any start pops so this change could be done. If the test build could set CONFIG_COMP_VOLUME_PLAYBACK_START_FADE the test should pass.

I recall there is a test about fade in (start ramp) curve in internal CI test, is that one?

[singalsu]

@lgirdwood @btian1 @wszypelt Seems we can't disable the playback start ramp in Kconfig without failing the 01_10_TestGainPeakVolApi test case. What should we do?
In my tests with UPX I could not hear any start pops so this change could be done. If the test build could set CONFIG_COMP_VOLUME_PLAYBACK_START_FADE the test should pass.

I recall there is a test about fade in (start ramp) curve in internal CI test, is that one?

Yep, it is failed. I marked this as do not merge since I'm not sure at all this is good way though it's efficient for MCPS. It would be better to have control of initial fade-in via topology.

[mszleszy]

@andrula-song, @singalsu Just a reminder that Gain and PeakVol have fade in length definition inside of InitInstance and LargeConfigSet messages. Part of verification is comparison of recorded curve length with the module configuration.
Here's an example of Gain init instance with fade in set to 0.1s

[singalsu]

@andrula-song, @singalsu Just a reminder that Gain and PeakVol have fade in length definition inside of InitInstance and LargeConfigSet messages. Part of verification is comparison of recorded curve length with the module configuration. Here's an example of Gain init instance with fade in set to 0.1s

@mszleszy There's curve_type in both init IPC and volume command IPC. Is it a possible scenario that curve duration is 0 in init and changed in successive volume command during streaming to e.g 0.1s? Also apperently the curve type can change on the fly between no_fade and fade.

We don't support such in Linux not at least yet but we could add such support to firmware if it's used by Windows OS driver.

[mszleszy]

@singalsu It's possible to start with 0 curve length, it can be changed before stream starts, and ipc contains (channel id, target volume, curve type and curve duration). Same IPC can be sent while pipeline is running but i think only volume will be actually changed on the fly.

We have same ipc structure for setting gain parameters before and during pipeline run so it looks like we can change curve type on the fly but i don't think that's an actual requirement or production scenario. @mwasko please correct me if I'm wrong

[singalsu]

@singalsu It's possible to start with 0 curve length, it can be changed before stream starts, and ipc contains (channel id, target volume, curve type and curve duration). Same IPC can be sent while pipeline is running but i think only volume will be actually changed on the fly.

We have same ipc structure for setting gain parameters before and during pipeline run so it looks like we can change curve type on the fly but i don't think that's an actual requirement or production scenario. @mwasko please correct me if I'm wrong

I'll close this PR since it's causing a regression. With the curve_duration change on the fly, at least Windows OS driver can avoid the high peak MCPS when several gains are starting. We can make sure the FW handles it correctly. Not sure we can utilize it in Linux though.

[singalsu]

Closing