dp: nocodec: switch playback SRC to DP by default#10469
dp: nocodec: switch playback SRC to DP by default#10469lyakh wants to merge 6 commits intothesofproject:mainfrom
Conversation
|
@lyakh Some nocodec failings. |
@lgirdwood yes, I've traced it back to an update in Zephyr, which had disabled double mapping. I'm working to fix it. |
There was a problem hiding this comment.
Pull request overview
This PR switches the playback SRC (Sample Rate Converter) to use the DP (Data Processing) domain by default in the nocodec topology, while keeping the capture SRC in the default domain. Additionally, it adds cached memory partition support for DP scheduler tasks.
Key changes:
- Splits the SRC domain configuration into separate playback and capture domains
- Adds cached memory partition support (HEAP_CACHE and CFG_CACHE) for improved memory access patterns
- Updates test topology parameters to maintain capture path DP testing
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tools/topology/topology2/cavs-nocodec.conf | Splits SRC_DOMAIN into SRC_DOMAIN_PLAYBACK (DP) and SRC_DOMAIN_CAPTURE (default) and updates widget configurations |
| tools/topology/topology2/development/tplg-targets.cmake | Updates test topology parameters from SRC_DOMAIN to SRC_DOMAIN_CAPTURE for MTL, LNL, and PTL platforms |
| src/schedule/zephyr_dp_schedule_application.c | Adds cached memory partition initialization and cleanup for both heap and mailbox partitions |
| src/schedule/zephyr_dp_schedule.h | Extends the sof_dp_part_type enum with HEAP_CACHE and CFG_CACHE partition types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
SOFCI TEST |
|
retest with #10474 merged |
|
SOFCI TEST |
|
SOFCI TEST |
b653aa5 to
1f3c6ed
Compare
|
SOFCI TEST |
|
SOFCI TEST |
softwarecki
left a comment
There was a problem hiding this comment.
I think we have crossed the line in terms of the amount of additional changes being bundled into a single PR, and it would be better to split it. This PR started as a change limited to SRC configuration, but it now also:
- updates Zephyr (disable FLIX)
- removes
fast_get/putsyscalls - modifies the objpool allocator
- introduces some changes in DP
Regarding the Zephyr update:
Commit message / description is misleading. The fix for the double exception is already in use (Zephyr update done by #10494). The Zephyr update in this PR disabling FLIX! It also switching some configuration options, which are not described.
I recommend splitting this PR into smaller, logically scoped changes. At a minimum, I would expect commit messages to be corrected so that they accurately reflect the actual changes and do not mislead reviewers.
@softwarecki no, I don't think this is correct: |
There is no gain in resending patches again. Important thing is its all separate patches, all been reviewed, passing CI (with is constrained today). Lets spend our time on the technical aspects. |
@lyakh This one disable FLIX for userspace builds:
|
@lgirdwood: Agree, this is a recommendation, not an expectation. For the future, we should avoid starting with a small PR and then adding several unrelated changes after approvals are already given. Changes added later often do not get the same level of review, because reviewers typically do not re-review a PR they have already approved. That creates a risk of changes effectively bypassing proper review. |
@softwarecki I know. But the goal of that Zephyr update was to include TLB fixes. So your statement "The fix for the double exception is already in use" was wrong. That's what I pointed out to. |
I agree that keeping approvals can backfire. But reviewers absolutely should re-review PRs after each update and update their approvals accordingly. |
|
Let's wait for #10511 and we will be fine with flix mess and zephyr update |
Now merged. |
@lyakh Indeed. I had no idea that zephyrproject-rtos/zephyr#103104 exist and I thought the double exception referred to my PR (zephyrproject-rtos/zephyr#102341). Zephyr was updated in another PR. FLIX remains enabled, so I do not see any blockers. I am not sure whether we should wait for zephyrproject-rtos/zephyr#103465 instead of disabling |
| CONFIG_MM_DRV_INTEL_VIRTUAL_REGION_COUNT=2 | ||
| CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC=38400000 | ||
| CONFIG_SYS_CLOCK_TICKS_PER_SEC=12000 | ||
| CONFIG_ACE_V1X_RTC_COUNTER=n |
There was a problem hiding this comment.
I assume this is disabled only for test.
zephyrproject-rtos/zephyr#103465 should be merged very soon and this will not be needed
There was a problem hiding this comment.
I've asked for this to be prioritized since its tainting CI results.
There was a problem hiding this comment.
@lyakh lets remove this conf change as we can rebase with the west fix for above Zephyr fix.
There was a problem hiding this comment.
but do we need that driver? If we don't need it anyway, why include it?
There was a problem hiding this comment.
Not critical feature but it is actually used in intel base_fw.
Someone made a regression in zephyr and it just needs to be fixed, luckily it's easy
|
https://github.com/thesofproject/sof/actions/runs/21742080825/job/62719480359?pr=10469 is a regression introduced by #10523 #10523 (comment) |
Since Zephyr has removed double mapping per Kconfig switch we need to restore it in SOF. Next we should try to optimize mappings to only use the ones we really need. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
DP tasks don't need to be rescheduled when pause is released. Default handling works correctly in that case too. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
We compare flags on repeated allocations from an existing pool, but initialisation got forgotten in the process. Restore it. Fixes: d6e6ac5 ("Add a object pool allocator") Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
When an objpool allocation fails an error should be returned. Fix the missing error code assignment. Fixes: fc73f9d ("sp: application: switch memory domains to object pools") Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
d263649 reverted because of a regression in SRC DP: |
A recent module-adapter feature addition broke SRC DP support. Fix it by adding a test for incomplete initialization data. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Switch both SRC instances in the nocodec topology on PTL to DP mode by default. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Switch one of the two SRC instances in the nocodec topology to DP mode by default.