volume: Set max volume value from IPC for volume module

[kakulesza]

volume: Set max volume value from sof_ipc_comp_volume IPC for volume module

Signed-off-by: Kamil Kulesza kamil.kulesza@linux.intel.com

[plbossart]

LGTM, just would like to verify that the kernel/topology actually fill this value. @ranj063 can you look at this?

[lgirdwood]

Please say why in the commit message.

[ranj063]

@kakulesza @plbossart @singalsu
I think we should leave this MAX_VOLUME in the firmware unchanged. Seppo added this as an extra level of protection to prevent arithmetic overflows. The driver is oblivious to the fixed point notation used in the firmware. So technically a user could set MAX_VOLUME in topology/kernel that is greater than the max value supported in the firmware and this will lead to overflows.

[plbossart]

In that case this parameter should not be in the IPC structure?

…

On 07/17/2018 05:52 PM, Ranjani Sridharan wrote: @kakulesza <https://github.com/kakulesza> @plbossart <https://github.com/plbossart> @singalsu <https://github.com/singalsu> I think we should leave this MAX_VOLUME in the firmware unchanged. Seppo added this as an extra level of protection to prevent arithmetic overflows. The driver is oblivious to the fixed point notation used in the firmware. So technically a user could set MAX_VOLUME in topology/kernel that is greater than the max value supported in the firmware and this will lead to overflows. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#120 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABGIazbYnbO9VZJQmHxq7ktPOPBADHUSks5uHmqpgaJpZM4VTAtX>.

[ranj063]

@plbossart does it make sense to maybe keep it there in case the ipc->max_volume is < VOL_MAX?
But today we arent even setting this value from topology. So maybe the best thing to do is just to remove it.

[kakulesza]

@ranj063 But do we really want to get rid of the possibility of set other max volume level during module is creating? Maybe better is validation for ipc->max_volume value?

[ranj063]

@kakulesza sure yes, I think that makes sense. We should validate the ipc max value and then set it accordingly. But we should keep in mind that this value might not be set in by topology/driver and might be 0.

[kakulesza]

@ranj063 I understand the problem. But whether possibility of setting max value to 0 makes sense? As a result, we get the module which mute all of pipeline. Can treat 0 value as a not set by topology/driver? Then we would not have a validation problem - for 0 FW would set the default value (VOL_MAX).

[lgirdwood]

@kakulesza , @ranj063 my feeling is that all volumes should default to 0dB gain during init().

[kakulesza]

@lgirdwood In this case I can validate and set min/max volumes in volume_prepare() and set default volumes to 0 in volume_new(). But can I make max_volume assumption that it'll be set to 0 only if topology/driver don't pass this value?

[lgirdwood]

@kakulesza I'm ok for min/max value setting in prepare(), and 0dB (not 0) in init().

[ranj063]

@lgirdwood @kakulesza the current VOL_MAX setting in the firmware corresponds to 0dB value which is what is set during init.

[lgirdwood]

@ranj063 we cannot assume VOL_MAX is 0dB, what about a volume with a max gain of 3dB ? This should be defined in the volume topology (part of TLV table) and it may be something we have to come back too after 1.2

[singalsu]

It would be better to initialize volume to 0 dB than IPC defined or DSP specific max possible gain. The max gain should be eventually much larger than the current 0 dB. Perhaps define also VOL_ZERO_DB or VOL_UNITY_GAIN macro (whatever preferred) and use it in this initialization code?

[kakulesza]

@singalsu @lgirdwood I added VOL_ZERO_DB macro as init value and set min/max volume values form IPC. Also i placed vol_set_min_max_levels() in new() instead prepare() because I guess that min/max should be a module property (not stream). Additionally, I had added commit message.