[mtl-005] pipeline: Ignore CPC value and use always the safe maximum

[ujfalusi]

Based on observations, the clock scaling can result underclocking when all modules in the pipeline have valid CPC value provided. On streams where at least one module's CPC is 0 the issue is not observed.

Ignore the CPC value for now to see if this is the rootcause of a failure observed.

Fixes: #7990

[ranj063]

@ujfalusi doesnt this defeat the purpose of having the cpc set in the manifest?

[btian1]

I would suggest keep max for now, even defeat the cpc purpose, this is because there always have abnormal component mcps, it is much larger than avg(cpc setting), take below perf log as example：

[ 1.246608] component: comp_copy: comp:1 0x30000 perf comp_copy samples 48 period 1000 cpu avg 240 peak 67 1539

avg cycle only 240, however, at first 1s, and the 67th ms, mixout cycles is almost 7 times compared with average.

Until we fixed or root-caused, I would suggest keep cpc as max.
@ranj063 @mengdonglin @lgirdwood

[ujfalusi]

@ujfalusi doesnt this defeat the purpose of having the cpc set in the manifest?

I prefer to use max for now to not lock the DSP but make it easier to test corrected CPC values in manifest (the kernel is sending them, only the firmware ignores it.
This patch eventually needs to be reverted/dropped, but if we do it with a Kconfig way (CONSTANT_CPC_AS_HZ_DIVIDED_BY ?) then three patches would needs to be dropped/reverted as the CPC value ignoring should not be done.

[ujfalusi]

@ranj063, the code change would be something like this:

commit 9068f7142ed799fa188d85992de3e0939bd571f8
Author: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Date:   Fri Aug 4 11:30:50 2023 +0300

    pipeline: Handle CONFIG_CONSTANT_CPC_AS_HZ_DIVIDED_BY for CPC value
    
    If CONFIG_CONSTANT_CPC_AS_HZ_DIVIDED_BY is not 0 then use it in place of
    any CPC value provided for the component.
    
    Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

diff --git a/src/audio/pipeline/pipeline-stream.c b/src/audio/pipeline/pipeline-stream.c
index 3203d53df675..1d17e99a4e38 100644
--- a/src/audio/pipeline/pipeline-stream.c
+++ b/src/audio/pipeline/pipeline-stream.c
@@ -297,7 +297,20 @@ static int add_pipeline_cps_consumption(struct comp_dev *current,
 		cd = &md->cfg.base_cfg;
 	}
 
-	if (cd->cpc == 0) {
+	if (CONFIG_CONSTANT_CPC_AS_HZ_DIVIDED_BY) {
+		uint32_t new_cpc = CLK_MAX_CPU_HZ / CONFIG_CONSTANT_CPC_AS_HZ_DIVIDED_BY;
+
+		if (!cd->cpc)
+			tr_warn(pipe,
+				"CPC for module: %#x, core: %d: 0, overriding to %u",
+				current->ipc_config.id, ppl_data->p->core, new_cpc);
+		else
+			tr_info(pipe,
+				"CPC for module: %#x, core: %d: %u, overriding to %u",
+				current->ipc_config.id, ppl_data->p->core, cd->cpc, new_cpc);
+
+		cd->cpc = new_cpc;
+	} else if (cd->cpc == 0) {
 		/* Use maximum clock budget, assume 1ms chunk size */
 		cd->cpc = CLK_MAX_CPU_HZ / 1000;
 		tr_warn(pipe,

Plus one patch for the Kconfig file and one for app/boards/intel_adsp_ace15_mtpm.conf (plus I would do that for tgl, but it is not even compiling)

[ujfalusi]

I have prepared a series for the Kconfig way, if it is preferred, I can push that as a replacement of the single patch.

[ranj063]

@ujfalusi using the kconfig seems like a better option for now so that it will be easy to change when we need to remove the restriction and as for the name can we simply call it CONFIG_OVERRIDE_MANIFEST_CPC or something like that?

[btian1]

I would suggest change back to DRAFT due to so much CI errors, after fix those errors change to open again.

[ujfalusi]

I would suggest change back to DRAFT due to so much CI errors, after fix those errors change to open again.

This is for mtl005 branch, it is like this, but I will push a new version with Kconfig (CONFIG_OVERRIDE_CPC_AS_HZ_DIVIDED_BY)

[ujfalusi]

Changes since v1:

• Use Kconfig (CONFIG_OVERRIDE_CPC_AS_HZ_DIVIDED_BY) to enable/disable the CPC override.

[abonislawski]

We will not support this on 005 because of power impact.
On main we can disable whole feature with CONFIG_KCPS_DYNAMIC_CLOCK_CONTROL=n (PR 8019)

[ujfalusi]

OK

[ujfalusi]

@abonislawski , do we have numbers on the power impact?
I mean: running "full speed + clock gating" VS "running slow and thus no clock gating" ?

[abonislawski]

It was tested on 004 to fix power results with deep buffering so we should have it somewhere (cannot share here)
On 005 when we accidentally ran into the fastest clock it resulted in unacceptable clock residency stats for deep buffering case (fixed in your PR 4508)

[ujfalusi]

@abonislawski, sure I understand if we run always at highest clock then the clock residency is going to reflect that.
That was not my question.

[ujfalusi]

@abonislawski, but sure, let's discuss internally