CPC: fix 0 CPC module clock scaling #9319
Conversation
lyakh
requested review from
lgirdwood,
plbossart,
mmaka1,
lbetlej,
dbaluta and
kv2019i
as code owners
|
replaces the draft version #9318 |
| @@ -41,6 +41,7 @@ CONFIG_DMA_DW_LLI_POOL_SIZE=50 | |||
| CONFIG_INTEL_MODULES=y | |||
| CONFIG_LIBRARY_MANAGER=y | |||
| CONFIG_LIBRARY_AUTH_SUPPORT=y | |||
| CONFIG_LIBRARY_PIPELINE_FORCE_MIN_LIFETIME=100 | |||
There was a problem hiding this comment.
Sorry, not following commit message, it not clear where the CPC calculation is transitioning from/to, i.e. CPC at pipeline start/stop is start and end point of this commit, pls also say why current method is broken. i.e. what use case.
src/audio/pipeline/pipeline-graph.c
Outdated
| if (comp_is_llext(current)) | ||
| /* pipelines are allocated using rzalloc(), so initially .init_time = 0 */ | ||
| current->pipeline->init_time = sof_cycle_get_64(); | ||
|
|
There was a problem hiding this comment.
Not following why this is different for llext?
UPDATE: now saa the new Kconfig for min lifetime, but maybe a comment would be good here as well.
src/ipc/ipc4/helper.c
Outdated
| life_time = k_cyc_to_ms_near64(sof_cycle_get_64() - ipc_pipe->pipeline->init_time); | ||
| pipe_dbg(ipc_pipe->pipeline, "Extend pipeline life beyond %llu", life_time); | ||
| if (life_time < CONFIG_LIBRARY_PIPELINE_FORCE_MIN_LIFETIME) | ||
| k_msleep(CONFIG_LIBRARY_PIPELINE_FORCE_MIN_LIFETIME - life_time); |
There was a problem hiding this comment.
This still looks quite odd. I guess there is some race in start and stopping the pipeline tasks, but this seems awfully brittle way to address it.
@lgirdwood This isn't difficult to answer - it's logging from LLEXT again. You might remember, that earlier I removed log prints from converted modules in their clean-up paths. Because when LLEXT modules are unloaded, if they use logging at that time, the logger thread runs after the module is unloaded and accesses unmapped memory. Now with the speaker-test it also happens with logs on initialisation and configuration paths - when pipelines are created and very quickly destroyed again, the same thing happens. |
|
CI failures:
|
Extract code to obtain a pointer to the module base configuration, depending on its type, into a separate function. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Extract the module_is_llext() macro into a header and add a function to identify a loadable module type of a component. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
|
||
| DECLARE_TR_CTX(drc_tr, SOF_UUID(drc_uuid), LOG_LEVEL_INFO); | ||
| extern const struct sof_uuid drc_uuid; | ||
| extern struct tr_ctx drc_tr; | ||
|
|
||
| void drc_reset_state(struct drc_state *state) | ||
| { |
There was a problem hiding this comment.
This commit is optional. With it dropped log entries look like
[ 140.980950] <inf> drc:
and without it like
[ 326.927381] <inf>
So using this and similar log context relocation for LLEXT modules adds a bit of context when messages are dropped, but it isn't critical
No, we did not have any pause-release failures on MTL for a very long time. The only known pause-release failures are:
|
In fact we didn't have ANY failure whatsoever on MTL for a very long time. Same tests failed again in https://sof-ci.01.org/sofpr/PR9319/build7188/devicetest/index.html so this PR is definitely guilty. EDIT: dunno what changed but ACE https://sof-ci.01.org/sofpr/PR9319/build7210/devicetest/index.html is passing now. |
Adding or subtracting module CPC when starting and stopping pipelines is brittle. Particularly it's prone to mistakes with modules, not specifying their CPC explicitly. Instead recalculate CPC every time a pipeline is started or stopped. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
The library manager isn't supported on TGL, disable it there. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
No need to set clock frequency if the new one is equal to the current one. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
uuid.h uses uint32_t, uint16_t and uint8_t types, it must include stdint.h or inttypes.h. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Logging context can be accessed from the deferred logging thread. Therefore it must be always available to avoid access to unmapped memory when an LLEXT module is unloaded. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
west.yml
Outdated
| @@ -43,7 +43,7 @@ manifest: | |||
|
|
|||
| - name: zephyr | |||
| repo-path: zephyr | |||
| revision: 795ac34f291c2f9895e7a3a03eafc053ad1d36f2 | |||
| revision: pull/77289/head | |||
There was a problem hiding this comment.
Still needed - or has west been updated now ?
There was a problem hiding this comment.
@lgirdwood zephyrproject-rtos/zephyr#77289 still under review, waiting for the assignee in particular
lyakh
changed the title
|
converted to draft to avoid accidental merging while waiting for a Zephyr PR to be merged |
Zephyr PR 77289 fixed deferred logging with LLEXT objects, update to a Zephyr version, containing it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
lyakh
changed the title
|
LNL test results are too https://sof-ci.01.org/sofpr/PR9319/build7508/devicetest/index.html truncated, MTL results https://sof-ci.01.org/sofpr/PR9319/build7509/devicetest/index.html were green here, but I'll rerun all Jenkins tests to get a better LNL coverage |
|
SOFCI TEST |
|
LNL build https://sof-ci.01.org/sofpr/PR9319/build7512/devicetest/index.html looks better now AFAICS - only known errors. |
Currently when a 0-CPC module is used, it will raise the system clock to the maximum and keep it there. Fix this by recalculating needed KCPS every time when a pipeline is created or deleted from scratch instead of adding or subtracting module values.