volume: merge volume (ipc3) and peakvol (ipc4) component

[abonislawski]

Merge peakvol component into already existing volume component.
The main differences:

• curve duration format is in hundred of ns (ipc4) and ms (ip3);
• volume format sent by driver: Q1.31 (ipc4) and Q8.16 (ipc3);
  In ipc4 case volume format is converted into Q1.23 inside firmware.

[ranj063]

bit-pick: can we use kernel stype comments for struct fields like this:

/** struct ipc4_peak_volume_config
 * @channel_id: ID of channel. If set to ALL_CHANNELS_MASK then
 *		configuration is identical and will be set for all all channels
 * @target_volume: Target channel volume. Takes values from 0 to 0x7fffffff.
 * @curve_type : Fade curve type
 * @curve_duration: Curve duration in number of hundreds nanoseconds for format specified during
 *		    initialization.
 */
struct ipc4_peak_volume_config {
	uint32_t channel_id;
	uint32_t target_volume;
	uint32_t curve_type;
	uint32_t reserved;
	uint64_t curve_duration;
} __attribute__((packed, aligned(8)));

[abonislawski]

Looks like it is not used for other files in include/ipc4?

[lgirdwood]

A good feature to add, just minor stuff.

[lgirdwood]

@singalsu @marcinszkudlinski can we pass the shift value as a non constant variable ? I'm guessing we cant as this would simplify. If not we could define this value as a macro and set it via the CONFIG.

[lgirdwood]

Sorry I missed the context in the above question.

out_sample = AE_ROUND32F48SSYM(AE_SRAI64(mult, 1));
#else /* CONFIG_IPC_MAJOR_4 */
			out_sample = AE_ROUND32F48SSYM(AE_SRAI64(mult, 8));

I assume the 1 and 8 above must be const.

[singalsu]

The shift parameter is legacy from time when volume did format conversion. The function should be renamed to vol_s24() and hard code and optimize all shifts to minimum. Shifts by immediate instructions may be better for parallel execute than parametric shift instructions.

[singalsu]

I think this could be improved, some proposals added:

[lgirdwood]

@abonislawski can you close any resolved comments. I think most are resolved now right ?

[ranj063]

it somehow feels like all of the above should be simple macros instead of config options.

[abonislawski]

The best would be to add select in COMP_VOLUME kconfig
select A if IPC4
select B if IPC3
but it didnt work so I ended with a standard config, just hidden

@singalsu @ranj063 where do you want to put this?

[ranj063]

@abonislawski how about hiding this as ops or fields in the volume specific data. struct vol_data? You can have fields such as q_notation that are set to Q1.23 and Q1.31 depending on the value and ops for doing IPC specific operations that will be set only where relevant

[abonislawski]

Not sure if this will be optimal solution to use AE_SRAI64 with shifts stored in variables.
@singalsu already mentioned to hard code and optimize all shifts to minimum.

[ranj063]

@abonislawski not following what you mean by not optimal? You can still hardcode whatever you want based on the IPC version. Just wont be part of the config and also it will reduce al the #if/#else;s

[lgirdwood]

@abonislawski fwiw, we had a recent bug with a similar Kconfig dependency. If the volume type is only used in 1 module then we could just do a

#if CONFIG_IPC3
#define COMP_VOLUME_Q8_16 
#endif

and so on. This keeps some simplicity in the Kconfig and the format localized to volume.

[abonislawski]

Ok, moved to volume.h

[lgirdwood]

Cant add any other comments so it looks like you have pushed whilst I was reviewing.

[lgirdwood]

@singalsu can you followup with a fix for the frag API usage here. Thanks !

[lgirdwood]

CI unrelated on Jenkins.

[lgirdwood]

@abonislawski I think we are good to go, can you rebase. Thanks !

[lyakh]

is this needed because the config can be used on different cores?

[lgirdwood]

oh, I think this has been updated at the IPC layer (so that IPC layer does all the cache mgmt) instead of here so the cache inv is not needed here.

[abonislawski]

yeap, it was updated for creating comp but this case is still affected.

Anyway removed dcache_() and ipc4 handler needs to be updated in another PR.
For now it doesnt matter because looks like volume_cmd() is never used for IPC4 because commit 6a297b4 added set large config but also removed cmd translation:
module set_large_config <-------> component cmd

All of this should be fixed in another PR

[lgirdwood]

Need to check cache handling is done by IPC layer.

[lgirdwood]

@abonislawski fyi - here is the efficient copy() PR update that canbe copied here. #5300

[abonislawski]

@lgirdwood rebase ready
Big thanks to @singalsu for hifi3 support :)

[lgirdwood]

Only open on the Kconfig, otherwise good.