Skip to content

pipeline: register KCPS consumption and enable dynamic clock control - #8019

Merged
lgirdwood merged 5 commits into
thesofproject:mainfrom
abonislawski:cpc_main
Apr 15, 2024
Merged

pipeline: register KCPS consumption and enable dynamic clock control#8019
lgirdwood merged 5 commits into
thesofproject:mainfrom
abonislawski:cpc_main

Conversation

@abonislawski

@abonislawskiabonislawski commented Aug 9, 2023

Copy link
Copy Markdown
Member

This will enable dynamic clock control based on KCPS budget.

Mostly cherry-picked from mtl-005-drop-stable + kconfig

pipeline: Register and unregister pipelines CPS consumption on run/pause
+Squashed commits:
924f0d2 pipeline: print warning if 0 KCPS received

7bb6e7e pipeline: register CPS consumption for a proper core

5f0c4e4 pipeline: make CPC data "opt-in" with fallback

platform: register basefw CPS consumption on boot

+Squashed commit:
38b4cb1 platform: mtl: lowest clock as default

boards: mtl: enable KCPS dynamic clock

@kv2019ikv2019i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @abonislawski ! Code looks good, a few minor nits for the documentation.

Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated
Comment threadsrc/platform/Kconfig
Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated

@lgirdwoodlgirdwood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff - just a few opens.

Comment threadsrc/platform/intel/ace/platform.c Outdated
Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated
Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated
Comment threadsrc/platform/intel/ace/platform.c Outdated
Comment threadapp/boards/intel_adsp_ace15_mtpm.conf
Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated
Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated
Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated
Comment threadsrc/audio/pipeline/pipeline-stream.c
Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated
Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated
Comment threadsrc/audio/pipeline/pipeline-stream.c
Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated
Comment threadsrc/platform/intel/ace/platform.c Outdated
Comment threadsrc/platform/intel/ace/platform.c

@lgirdwoodlgirdwood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abonislawski ping - I think this fixes some reported issues.

@lgirdwoodlgirdwood added this to the v2.7 milestone Aug 16, 2023
@abonislawski

Copy link
Copy Markdown
MemberAuthor

I think this fixes some reported issues.

Unfortunately not, I will post more info in issue thread

Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated
Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated
Comment threadsrc/platform/Kconfig
Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated
Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated
current->ipc_config.id, ppl_data->p->core, cd->cpc);
}

kcps = cd->cpc * 1000 / ppl_data->p->period;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we do check period for non zero ? (since it comes from host)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p->trigger.delay = (data.delay_ms * 1000 + p->period - 1) / p->period;
similar code, should checked.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw this is a fixed value in ipc4_create_pipeline pipe->period = LL_TIMER_PERIOD_US;
anyway what 0 period means for pipeline? Just use module cpc value in such case?

@abonislawskiabonislawskiOct 23, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood this is only one unresolved issue after today's update. Im not sure what 0 period could mean?
and if this is error condition - can we get it so late at the trigger stage?


CONFIG_PROBE=y
CONFIG_PROBE_DMA_MAX=2
CONFIG_KCPS_DYNAMIC_CLOCK_CONTROL=y

@lgirdwoodlgirdwoodAug 28, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abonislawski I'm happy to merge this if this defaults to "n" at the moment until we can resolve all the opens. We also need to add and align all the CPCs on manifest (and that's still WIP, but > 50% done). CI tests for CPC also landing soon, so safe to make default "y" when all pieces are in place.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the latest status, do we have any known opens in mainline left? @abonislawski

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abonislawski can you confirm this is now working as expected ? I suppose CI will show us if any CPC numbers are worng, but only for tested modules today.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should work correctly, already used in a few MTL releases and we have cpc data for most (or all) of our modules.
Updated today with rebase & review fixes

@btian1

Copy link
Copy Markdown
Contributor

@abonislawski , could you list here how you get each component cpc data? by below ways?

  1. cpc = avg.
  2. cpc = avg * 110%
  3. cpc = avg * 150%?
  4. cpc = (avg+peak) / 2

@mengdonglinmengdonglin modified the milestones: v2.7, v2.8Sep 1, 2023

@btian1btian1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current clock adjust happens on pipeline trigger? how about adjust clock during runtime?

Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated
current->ipc_config.id, ppl_data->p->core, cd->cpc);
}

kcps = cd->cpc * 1000 / ppl_data->p->period;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p->trigger.delay = (data.delay_ms * 1000 + p->period - 1) / p->period;
similar code, should checked.

@abonislawski
abonislawskiforce-pushed the cpc_main branch 2 times, most recently from 08d11ef to bb8b4d6CompareJanuary 29, 2024 08:30
Comment threadsrc/audio/pipeline/pipeline-stream.c Outdated
return PPL_STATUS_PATH_STOP;
} else {
kcps = cd->cpc * 1000 / current->period;
//tr_dbg(pipe, "Registering module: %#x KCPS consumption: %d, core: %d",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style

@lgirdwood

Copy link
Copy Markdown
Member

@abonislawski re the CI test failures, are we missing anything on the kernel side. i.e. does kernel need to do any IP programming flow around DSP clock change ?
Fwiw, I know we have some loops that spin around a clock register bit change, could some be timing out or spinning forever ?

[ 407.529254] kernel: snd_sof:sof_pcm_trigger: sof-audio-pci-intel-mtl 0000:00:1f.3: pcm: trigger stream 6 dir 1 cmd 0
[ 407.529276] kernel: snd_sof:sof_ipc4_trigger_pipelines: sof-audio-pci-intel-mtl 0000:00:1f.3: trigger cmd: 0 state: 3
[ 407.529297] kernel: snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-mtl 0000:00:1f.3: ipc tx : 0x13000003|0x1: GLB_SET_PIPELINE_STATE [data size: 12]
[ 408.031809] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: ipc timed out for 0x13000003|0x1
[ 408.031838] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: Attempting to prevent DSP from entering D3 state to preserve context
[ 408.031847] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: ------------[ IPC dump start ]------------
[ 408.031879] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: Host IPC initiator: 0x93000003|0x1|0x0, target: 0x1b060000|0x0|0x0, ctl: 0x3
[ 408.031887] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: ------------[ IPC dump end ]------------
[ 408.031894] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: ------------[ DSP dump start ]------------
[ 408.031899] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: IPC timeout
[ 408.031904] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: fw_state: SOF_FW_BOOT_COMPLETE (7)
[ 408.031930] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: 0x50000005: module: ROM_EXT, state: FW_ENTERED, running
[ 408.031942] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: Firmware state: 0x0, status/error code: 0x0
[ 408.031970] kernel: snd_sof:sof_ipc4_find_debug_slot_offset_by_type: sof-audio-pci-intel-mtl 0000:00:1f.3: Slot type 0x4c455400 is not available in debug window
[ 408.031975] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: ------------[ DSP dump end ]------------
[ 408.032005] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: failed to pause all pipelines
[ 408.032017] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: ASoC: error at soc_component_trigger on 0000:00:1f.3: -110
[ 408.032032] kernel: DMIC Raw: ASoC: trigger FE cmd: 0 failed: -110

@abonislawski

abonislawski commented Jan 31, 2024

Copy link
Copy Markdown
MemberAuthor

@lgirdwood We shouldnt need anything from kernel side as this is already used on MTL branches.
It seems that everything crashes on debug build (default for PR testing in linux CI) and works on release build (default for PR testing on second internal CI), I need to investigate why

Fwiw, I know we have some loops that spin around a clock register bit change, could some be timing out or spinning forever ?

I will check that part

@kv2019i

Copy link
Copy Markdown
Collaborator

@abonislawski wrote:

@lgirdwood We shouldnt need anything from kernel side as this is already used on MTL branches. It seems that everything crashes on debug build (default for PR testing in linux CI) and works on release build (default for PR testing on second internal CI), I need to investigate why

At least in some cases, there are overruns recorded. This can be due to CPC being too low for debug build, leading to too low DSP freq used for a use-case:
https://sof-ci.01.org/sofpr/PR8019/build2392/devicetest/index.html?model=MTLP_RVP_HDA&testcase=check-capture-3times

[ 407.529335] <inf> pipe: pipeline_remove_cps_consumption: Unregistering module: 0x10004 CPC: 7116, period: 1000, core: 0
[ 407.529360] <inf> clock: clock_set_freq: clock 0 set freq 23024000Hz freq_idx 0
[ 407.529383] <inf> clock: clock_set_freq: clock 1 set freq 23024000Hz freq_idx 0
[ 407.529460] <inf> clock: clock_set_freq: clock 2 set freq 23024000Hz freq_idx 0
[ 407.587696] <inf> ll_schedule: zephyr_domain_thread_fn: ll core 0 timer avg 4893, max 37788, overruns 2
...
[ 408.045146] <err> ipc: ipc_comp_free: ipc_comp_free(): comp id: 4 state is 4 cannot be freed
[ 408.045195] <err> ipc: ipc_pipeline_free: ipc_pipeline_free(): module free () failed

@kv2019i

Copy link
Copy Markdown
Collaborator

@marc-hb

Copy link
Copy Markdown
Collaborator

Such notification currently is used only in non-zephyr code (ll sched)
so it is safe to remove for MTL because of some idc issues
Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>
KCPS budged should be initialized before any KCPS adjustments
Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>
@abonislawski
abonislawskiforce-pushed the cpc_main branch 2 times, most recently from 004335c to 96cda9aCompareApril 8, 2024 10:15
@marc-hb

Copy link
Copy Markdown
Collaborator

This compilation failure seems related:

/zep_workspace/sof/src/lib/cpu-clk-manager.c: In function 'core_kcps_adjust':
/zep_workspace/sof/src/lib/cpu-clk-manager.c:67:69: error: 'CLK_MAX_CPU_HZ' undeclared (first use in this function)
67 | ret = request_freq_change(core_id, MIN(freq * 1000, CLK_MAX_CPU_HZ));
| ^~~~~~~~~~~~~~

@abonislawski
abonislawskiforce-pushed the cpc_main branch 2 times, most recently from 8d32afb to 12bd7d1CompareApril 9, 2024 08:01

@lgirdwoodlgirdwood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abonislawski LGTM,
@ujfalusi is kernel ready for KCPS ? i.e. if some modules have missing or wrong KCPS values then kernel will handle, with no audio impact and no dmesg spam ?

@kv2019ikv2019i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me mark with a -1 so this is not accidentally merged. Code looks good, but we still have CI tests failing.
E.g. https://sof-ci.01.org/sofpr/PR8019/build3791/devicetest/index.html still in today's run

kfrydryxand others added 3 commits April 14, 2024 22:56
Register and unregister pipelines CPS consumption on run/pause
Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>
Signed-off-by: Krzysztof Frydryk <krzysztofx.frydryk@intel.com>
+Squashed commits:
924f0d2 pipeline: print warning if 0 KCPS received
Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>
7bb6e7e pipeline: register CPS consumption for a proper core
Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>
5f0c4e4 pipeline: make CPC data "opt-in" with fallback
Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
On init, register consumption of 10MCPS for base fw
if CONFIG_KCPS_DYNAMIC_CLOCK_CONTROL enabled
Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>
Signed-off-by: Krzysztof Frydryk <krzysztofx.frydryk@intel.com>
This will enable dynamic clock control based on KCPS budget
Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

@kv2019ikv2019i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests seem better now, removing my -1

@abonislawski

Copy link
Copy Markdown
MemberAuthor

Internal and Linux CI pass

I have also merged two ppl consumption functions into single pipeline_calc_cps_consumption
divided base cps for primary and secondary, by default pretty high values but we can optimize this later if required

#define PRIMARY_CORE_BASE_CPS_USAGE 20000
#define SECONDARY_CORE_BASE_CPS_USAGE 10000

@lgirdwood
lgirdwood merged commit 031619c into thesofproject:mainApr 15, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@abonislawski@btian1@lgirdwood@kv2019i@marc-hb@lyakh@ujfalusi@marcinszkudlinski@tmleman@mengdonglin@kfrydryx