[BUG] IPC4 volume windows ramp was broken with latest sof-dev kernel and sof firmware

[btian1]

Describe the bug
When volume change, volume ramp should take effect, however, from debug, never see ramp is happening, from sof side, finally find the cdata from kernel driver have random value, no the ramp time.

To Reproduce

1. build sof with ipc4 with logs inserted in volume ramp.
2. run it on test machine.
3. aplay a wav file.
4. enable mtrace and adjust volume with alsamixer, inserted log never come out.

Reproduction Rate
100%

Expected behavior
volume ramp should happen, and cdata should be right.

Impact
volume ramp missed, pop noise come when volume change.

Environment

1. Branch name and commit hash of the 2 repositories: sof (firmware/topology) and linux (kernel driver).
   • Kernel: latest
   • SOF: latest
2. Name of the topology file
   • Topology: cavs_sdw.tplg
3. Name of the platform(s) on which the bug is observed.
   • Platform: dell adlp test machine.

Screenshots or console output
n/a

Please also include the relevant sections from the firmware log and kernel log in the report (and attach the full logs for complete reference). Kernel log is taken from dmesg and firmware log from sof-logger. See https://thesofproject.github.io/latest/developer_guides/debugability/logger/index.html

[RanderWang]

good finding! we lack volume change test

[RanderWang]

@plbossart @ujfalusi @ranj063. This issue is caused by driver used an old version of gain ipc msg.

struct sof_ipc4_gain_data {
	uint32_t channels;
	uint32_t init_val;
	uint32_t curve_type;
	uint32_t reserved;
	uint32_t curve_duration;
        // compiler will allocate more u32 memory to aligned(8)
} __aligned(8);

But FW adopts a new version

struct ipc4_peak_volume_config {

	uint32_t channel_id;
	uint32_t target_volume;
	uint32_t curve_type;
	uint32_t reserved;
	/**
	 * Curve duration in number of hundreds nanoseconds for format specified during
	 * initialization.
	 */
	uint64_t curve_duration;
} __packed __aligned(8);

Change definition is simple but how to deal with token reader since the curve_duration is retrieved from topology. Current our get_token only support return value of int. And does topology support u64 token size ?

struct sof_topology_token {
	u32 token;
	u32 type;
	int (*get_token)(void *elem, void *object, u32 offset);
	u32 offset;
};

Can we define u32 curve_duration_u and curve_duration_l in driver header file and also change topology definition ? then we don't have trouble with u64 in both topology and driver side ?

Welcome to give suggestion.

[plbossart]

@RanderWang are we talking about the same functionality? the peak volume is typically to REPORT the volume level from the stream, not SET the gain.

[RanderWang]

@plbossart Peak volume has two modes. The gain mode doesn't report volume level. Sof uses gain now.

enum ipc4_vol_mode {
	IPC4_MODE_PEAKVOL		= 1,
	IPC4_MODE_GAIN			= 2,
};

[singalsu]

good finding! we lack volume change test

There is volume_check_levels.sh in sof-test for nocodec topologies. I've just set improvements PR to it to be more robust against control timing jitter. The driver changes have cause more delay to controls to happen vs. the time when I first developed the test. It would be great to get CI to run it.

@marc-hb @aiChaoSONG

[marc-hb]

There is volume_check_levels.sh in sof-test for nocodec topologies. I've just set improvements PR to it to be more robust against control timing jitter.

I'm guessing thesofproject/sof-test#985

BTW there's another volume test fix that has been in review for a (too?) long time:

• check-pause-resume: handle volume MAX output in expect sof-test#931

[plbossart]

@RanderWang so is the problem that we retrieve a curve duration from topology as a u32, but the firmware expects a u64? In terms of endianess that should work.

Or that the firmware expects different definitions for the duration? I see this in the firmware commit message

- curve duration format is in hundred of ns (ipc4) and ms (ip3);

I don't know why anyone would want a ramp duration in ns... That's completely overkill. even micro-seconds would be too fine grained given the audio sampling rates.

A simple fix would be to multiple the curve duration by 10e6, e.g.

diff --git a/sound/soc/sof/ipc4-topology.c b/sound/soc/sof/ipc4-topology.c
index 10189b6428c24..d8a4b00631191 100644
--- a/sound/soc/sof/ipc4-topology.c
+++ b/sound/soc/sof/ipc4-topology.c
@@ -680,6 +680,9 @@ static int sof_ipc4_widget_setup_comp_pga(struct snd_sof_widget *swidget)
        gain->data.channels = SOF_IPC4_GAIN_ALL_CHANNELS_MASK;
        gain->data.init_val = SOF_IPC4_VOL_ZERO_DB;
 
+       /* volume ramps are defined in 100 of nsec for IPC4 */
+       gain->data.curve_duration *= (NSEC_PER_MSEC / 100);
+
        /* The out_audio_fmt in topology is ignored as it is not required to be sent to the FW */
        ret = sof_ipc4_get_audio_fmt(scomp, swidget, &gain->available_fmt, false);
        if (ret)

The other parameters look identical to me or are overridden by the driver

	gain->data.channels = SOF_IPC4_GAIN_ALL_CHANNELS_MASK;
	gain->data.init_val = SOF_IPC4_VOL_ZERO_DB;

[RanderWang]

@plbossart thanks for your feedback. so u32 is enough to support ramp feature. The key issue is caused by the set volume data.
(1) sizeof(struct sof_ipc4_gain_data data) = 6 * 32bits, one of 32bits is padded by compiler to align 8.
(2) currently we don't clear "struct sof_ipc4_gain_data data" to zero and only set curve_duration as 32bits, so the padded 32bits is full of garbage, 0xffffffff as I printed
(3) in fw curve_duration is 64bits data, so its value is 0xffffffff0000xxxx, so huge and result to panic

since u32 can support up to 100000ms and we can just clear "struct sof_ipc4_gain_data data" to zero in the sof_ipc4_set_volume_data function and only use u32 data.

struct sof_ipc4_gain_data {
	uint32_t channels;
	uint32_t init_val;
	uint32_t curve_type;
	uint32_t reserved;
	uint32_t curve_duration;
} __aligned(8);

sof_ipc4_set_volume_data(struct snd_sof_dev *sdev, struct snd_sof_widget *swidget,
			 struct snd_sof_control *scontrol)
{
       ...
	struct sof_ipc4_gain_data data;
        ...
	for (i = 0; i < scontrol->num_channels; i++) {
		if (all_channels_equal) {
			data.channels = SOF_IPC4_GAIN_ALL_CHANNELS_MASK;
			data.init_val = cdata->chanv[0].value;
		} else {
			data.channels = cdata->chanv[i].channel;
			data.init_val = cdata->chanv[i].value;
		}

		/* set curve type and duration from topology */
		data.curve_duration = gain->data.curve_duration;
		data.curve_type = gain->data.curve_type;
               ....
	}

	return 0;
}

[aiChaoSONG]

what about windows compatibility? I think windows driver will send a u64 value, right?

[RanderWang]

what about windows compatibility? I think windows driver will send a u64 value, right?

It is a issue of Linux, not windows. FW does it in right way

[plbossart]

There's no way we can extend the topology to read 64-bit values.
I think the 'right' solution would be to re-align the definitions with the firmware, but split the curve_duration into _l and _h part, with the _h part explicitly zeroed out in the driver.

[aiChaoSONG]

There is volume_check_levels.sh in sof-test for nocodec topologies. I've just set improvements PR to it to be more robust against control timing jitter. The driver changes have cause more delay to controls to happen vs. the time when I first developed the test. It would be great to get CI to run it.

Looks the test case only works on IPC3 nocodec platforms, and by the way, it will requires sof source coded to be downloaded to $HOME, because it depends on some octave libraries, which is in sof source tree.

[marc-hb]

Good catch @aiChaoSONG , thanks. We can still merge this test but cannot run it in CI in the short term and it needs a good error message in case someone tries to run it without the sof repo. Is there one?

[ujfalusi]

There's no way we can extend the topology to read 64-bit values. I think the 'right' solution would be to re-align the definitions with the firmware, but split the curve_duration into _l and _h part, with the _h part explicitly zeroed out in the driver.

I also think this is the right solution for now.