Repository navigation
timer_domain: bring the timer_domain_unregister() back - #4885
Conversation
Keep the task in the list and the subsequent call to schedule_ll_task_cancel() will clean it up. Remove the operation in schedule_ll_task_done() to avoid double cleaning, and unregistering from the interrupt handler itself won't happen anymore. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
This partially revert the below commit, to bring the regitser/unregister symmetry back with restore the timer_domain_unregister() implementation: commit d4fd1b3 ("timer_domain: don't unregister interrupt handler from itself") Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
only partially revert commit d4fd1b3 ("timer_domain: don't unregister interrupt handler from itself"), and provide another solution to the crash issue @lyakh mentioned in his previous PR. |
|
@lgirdwood @keyonjie see #4856 (comment) - I'm not certain |
|
@lgirdwood @lyakh the commit commit d4fd1b3 ("timer_domain: don't unregister interrupt handler from itself") breaks multi-core playback on the ADL nocodec topology for me. This PR fixes it. |
| list_item_del(&task->list); | ||
|
|
||
| domain_unregister(sch->domain, task, atomic_sub(&sch->num_tasks, 1) - 1); | ||
|
|
There was a problem hiding this comment.
this function is now only called once and only prints traces, maybe you could remove it completely and move prints directly into the caller
There was a problem hiding this comment.
this function is now only called once and only prints traces, maybe you could remove it completely and move prints directly into the caller
How about let's keep it at the moment for better bisection.
There was a problem hiding this comment.
@keyonjie I've investigated the conflict between this PR and my now reverted #4724. As you know, the latter is moving TRIGGER_STOP handling into the pipeline task. That way the pipeline task itself processes the trigger and completes and yes, in that case there is no call to schedule_task_cancel() exactly because you don't want to cancel the task from itself, potentially deregistering the interrupt handler from itself.
Let's try to understand better what this PR was trying to fix. I see a comment from @ranj063 that this fixes "multicore playback" on ADL. What exactly does that mean? Running multiple-pipeline-playback.sh on a multicore topology? The SOF upstream after d4fd1b3 was broken or upstream plus multicore coherency API? Do we know how the breakage was happening?
There was a problem hiding this comment.
@keyonjie I've investigated the conflict between this PR and my now reverted #4724. As you know, the latter is moving
TRIGGER_STOPhandling into the pipeline task. That way the pipeline task itself processes the trigger and completes and yes, in that case there is no call toschedule_task_cancel()exactly because you don't want to cancel the task from itself, potentially deregistering the interrupt handler from itself. Let's try to understand better what this PR was trying to fix. I see a comment from @ranj063 that this fixes "multicore playback" on ADL. What exactly does that mean? Running multiple-pipeline-playback.sh on a multicore topology? The SOF upstream after d4fd1b3 was broken or upstream plus multicore coherency API? Do we know how the breakage was happening?
@lyakh it broke the upstream + multi-core nocodec (multi-core + dynamic ppl actually) topology, normal playback or capture fails.
|
SOFCI TEST |
|
Alloc failure #4929 bisected to this. |
commit d4fd1b3 ("timer_domain: don't unregister interrupt handler from itself")