DP: Fix dp period calculation for DP modules #8462

jxstelter · 2023-11-08T15:14:18Z

The OBS parameter for module could be set to higher value
than the one defined by stream output format. This could result
in wrong calculation of period parameter.
Due to this issue we observe SRC tests regression on LNL platform.

This patch adds output stream format to period calculation in addition
to the one calculated from OBS. Lower value is selected.
Also period is round down to represent integer multiple of LL ticks.

Signed-off-by: Jaroslaw Stelter [email protected]

src/audio/module_adapter/module_adapter.c

lgirdwood · 2023-11-13T13:21:14Z

src/audio/src/src_ipc4.c

@@ -107,9 +108,17 @@ int src_set_params(struct processing_module *mod, struct sof_sink *sink)
 	 * as SRC uses period value to calculate its internal buffers,
 	 * it must be done here, right after setting sink parameters
 	 */
-	if (dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP)
-		dev->period = 1000000 * sink_get_min_free_space(sink) /
+	if (dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP) {


@jxstelter @singalsu this looks the same as the previous patch in this PR. i.e. we could make this a helper API call to avoid duplication (but can be done as an improvement in another PR).

the calcuation for sink period is more complex than this.

src/audio/module_adapter/module_adapter.c

lyakh · 2023-11-15T08:48:31Z

src/audio/module_adapter/module_adapter.c

+			(sink_get_rate(mod->sinks[i]) / (USEC_PER_SEC / LL_TIMER_PERIOD_US));
+		min_free_size = MIN(min_free_size, sink_get_min_free_space(mod->sinks[i]));
+
+		unsigned int sink_period = 1000000 * min_free_size /


can this overflow? If min_free_size is a bit more than 4K, it will overflow already?

I think there is no case where the minimal size will be close to 4K value.

take 192khz, 32 bit 8 ch with 10ms period data as example:
192 * 4 * 8 * 10 = 61440, even 1ms data size is 6144.

btw, I assume this is a one time division or do we divide on every process() ?

this is in prepare, so it is one time, no worry on performance.

src/audio/src/src_ipc4.c

btian1

we may need implement with formula like below:

sink_period = sink_produced_samples/sample_rate * 1000000

take attached 48 --> 82 as example, when produce 98, sink_period = 98/88200 * 1000000 = 1111

btian1 · 2023-11-20T08:34:37Z

src/audio/module_adapter/module_adapter.c

@@ -240,9 +241,17 @@ static int module_adapter_dp_queue_prepare(struct comp_dev *dev)
 			 &sink_buffer->stream.runtime_stream_params,
 			 sizeof(sink_buffer->stream.runtime_stream_params));
 		/* calculate time required the module to provide OBS data portion - a period */
-		unsigned int sink_period = 1000000 * sink_get_min_free_space(mod->sinks[i]) /
+		uint32_t min_free_size =


@jxstelter , please specify which converter have problem.
This is quite tricky for set sink period, @singalsu have a table to show different produce.
it may not easy to set here, check below 48 -->88.2 example:

max is 98, we may need more discuss for SRC period setting, it is more complex than we imagine.

@jxstelter , please specify which converter have problem. This is quite tricky for set sink period, @singalsu have a table to show different produce. it may not easy to set here, check below 48 -->88.2 example:

max is 98, we may need more discuss for SRC period setting, it is more complex than we imagine.

Currently the issue is observed only on LNL FPGA setup. This is because on FPGA there is much lower MCPS margin available. In that case simply Copier->SRC->Copier pipeline is affected. When sink_period is set for too high value, DP module get processing time later than it should and on FPGA it can not finish processing on time, causing glitches.
The problem is not observed on RVP yet, however it could be observed when core utilization will be much higher.

btian1 · 2023-11-20T08:35:30Z

src/audio/src/src_ipc4.c

@@ -107,9 +108,17 @@ int src_set_params(struct processing_module *mod, struct sof_sink *sink)
 	 * as SRC uses period value to calculate its internal buffers,
 	 * it must be done here, right after setting sink parameters
 	 */
-	if (dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP)
-		dev->period = 1000000 * sink_get_min_free_space(sink) /
+	if (dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP) {


the calcuation for sink period is more complex than this.

The OBS parameter for module could be set to higher value than the one defined by stream output format. This could result in wrong calculation of period parameter. Due to this issue we observe SRC tests regression on LNL platform. This patch adds output stream format to period calculation in addition to the one calculated from OBS. Lower value is selected. Also period is round down to represent integer multiple of LL ticks. Signed-off-by: Jaroslaw Stelter <[email protected]>

The OBS parameter for SRC module could be set to higher value than the one defined by stream output format. This could result in wrong calculation of period parameter. Due to this issue we observe SRC tests regression on LNL platform. This patch adds output stream format to period calculation in addition to the one calculated from OBS. Lower value is selected. Also period is round down to represent integer multiple of LL ticks. Signed-off-by: Jaroslaw Stelter <[email protected]>

lgirdwood

@jxstelter is this pending more testing before non draft ?

lgirdwood · 2023-11-24T11:16:21Z

src/audio/module_adapter/module_adapter.c

+			(sink_get_rate(mod->sinks[i]) / (USEC_PER_SEC / LL_TIMER_PERIOD_US));
+		min_free_size = MIN(min_free_size, sink_get_min_free_space(mod->sinks[i]));
+
+		unsigned int sink_period = 1000000 * min_free_size /


btw, I assume this is a one time division or do we divide on every process() ?

jxstelter · 2023-11-24T11:44:39Z

@jxstelter is this pending more testing before non draft ?

Yes. Actually all tests passed with this change, but we agreed with @marcinszkudlinski, that the resolution should be different. This is WIP.

jxstelter · 2023-11-29T10:44:43Z

Closing. Issue will be fixed different way.

jxstelter requested a review from marcinszkudlinski November 8, 2023 15:14

jxstelter force-pushed the src-fix-dp-period-calculation branch from 26c3bce to f45eba4 Compare November 8, 2023 15:23

btian1 reviewed Nov 9, 2023

View reviewed changes

src/audio/module_adapter/module_adapter.c Show resolved Hide resolved

lyakh requested changes Nov 9, 2023

View reviewed changes

src/audio/module_adapter/module_adapter.c Outdated Show resolved Hide resolved

jxstelter force-pushed the src-fix-dp-period-calculation branch from f45eba4 to 576575f Compare November 9, 2023 11:59

jxstelter requested review from btian1 and lyakh November 9, 2023 12:15

lyakh approved these changes Nov 10, 2023

View reviewed changes

lgirdwood approved these changes Nov 13, 2023

View reviewed changes

tmleman approved these changes Nov 14, 2023

View reviewed changes

lyakh reviewed Nov 15, 2023

View reviewed changes

btian1 reviewed Nov 20, 2023

View reviewed changes

jxstelter added 2 commits November 20, 2023 15:45

jxstelter force-pushed the src-fix-dp-period-calculation branch from 576575f to 2fcfba5 Compare November 20, 2023 14:45

lgirdwood reviewed Nov 24, 2023

View reviewed changes

jxstelter closed this Nov 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DP: Fix dp period calculation for DP modules #8462

DP: Fix dp period calculation for DP modules #8462

jxstelter commented Nov 8, 2023

lgirdwood Nov 13, 2023

btian1 Nov 20, 2023

lyakh Nov 15, 2023

jxstelter Nov 20, 2023

btian1 Nov 21, 2023

lgirdwood Nov 24, 2023

btian1 Nov 27, 2023

btian1 left a comment

btian1 Nov 20, 2023

jxstelter Nov 20, 2023

btian1 Nov 20, 2023

lgirdwood left a comment

lgirdwood Nov 24, 2023

jxstelter commented Nov 24, 2023

jxstelter commented Nov 29, 2023

DP: Fix dp period calculation for DP modules #8462

DP: Fix dp period calculation for DP modules #8462

Conversation

jxstelter commented Nov 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btian1 left a 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

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxstelter commented Nov 24, 2023

jxstelter commented Nov 29, 2023