-
Notifications
You must be signed in to change notification settings - Fork 341
DP modules deadline calculations #10177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
DP modules deadline calculations #10177
Conversation
bf56dec
to
4416c7c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces proper deadline calculations for DP (Data Processing) modules using Earliest Deadline First (EDF) scheduling, allowing DP to DP connections within pipelines. The implementation calculates deadlines based on current pipeline state and buffer conditions rather than simple timing assumptions.
- Implements comprehensive deadline calculation algorithm based on buffer Latest Feeding Time (LFT) and module Longest Processing Time (LPT)
- Adds pre-run notifier events to capture timestamps for deadline calculations
- Introduces new sink API for calculating Latest Feeding Time (LFT) for buffers
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
west.yml | Updates Zephyr dependency to use custom fork with thread deadline improvements |
src/schedule/zephyr_ll.c | Adds LL_PRE_RUN notifier event before task execution |
src/schedule/ll_schedule.c | Adds LL_PRE_RUN notifier event before task execution |
src/schedule/zephyr_dp_schedule.c | Major refactoring to implement EDF scheduling with proper deadline calculations |
src/include/sof/lib/notifier.h | Adds NOTIFIER_ID_LL_PRE_RUN event type |
src/include/sof/audio/module_adapter/module/generic.h | Adds module deadline and LPT calculation APIs |
src/include/sof/audio/audio_buffer.h | Adds LFT calculation function declaration |
src/include/module/audio/sink_api.h | Adds get_lft operation to sink API |
src/audio/module_adapter/module/generic.c | Implements module deadline calculation logic |
src/audio/buffers/ring_buffer.c | Adds LFT operation to ring buffer sink ops |
src/audio/buffers/comp_buffer.c | Adds LFT operation to comp buffer sink ops |
src/audio/buffers/audio_buffer.c | Implements comprehensive LFT calculation with DP-to-DP support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/schedule/zephyr_dp_schedule.c
Outdated
* a DEADLINE is the latest moment >>a module<< must finish processing to feed all target | ||
* buffers before their LFTs | ||
* Calculation is simple | ||
* - module's deadline is the nearest LFT of all target buffers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"output buffers" / "sink buffers"?
src/schedule/zephyr_dp_schedule.c
Outdated
* extra care for 32bit overflows or use slow 64bit operations). Also all modules have the same | ||
* timestamp as "NOW", regardless of moment in the cycle the deadlines are calculated | ||
* | ||
* EXAMPLE1 (data source period is longer or equal to data source) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this correct? "data source period is longer or equal to data source?"
src/schedule/zephyr_dp_schedule.c
Outdated
* | ||
* | ||
* | ||
* EXAMPLE2 (data source period is shorter than data receiver) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be good to use consistent names "source," "sink," "input," "output"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change it to convention
- sink/source as API exposed by a buffer or by a module
- data producer/data consumer as module that is processing data using sink/source API
sink/source of data may be misleading as it has opposite meaning, depending if we're talking about a module (receiving data from source API / sending to sink API) or a buffer (receiving data from sink API / sending to source API)
* - a worst case - module period | ||
* note that module period may be calculated reasonably late, i.e. in prepare() method | ||
*/ | ||
static inline uint32_t module_get_LPT(struct processing_module *mod) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lower case please. You spell-out "last_feeding_time" in other function names, but use an abbreviation for "lpt." Let's do this consistently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
module_get_longest_processing_time is really long, but ok
uint32_t us_in_buffer = | ||
1000 * source_get_data_available(&buffer->_source_api) / bytes_per_ms; | ||
|
||
return us_in_buffer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the code below, we can add it when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to leave it, it has been tested, just cannot be enabled because of cross core and comp_buffer
I'm afraid when I remove it it will be lost - at least very hard to find
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcinszkudlinski I'd then put it under some #ifdef GOOD_CODE_JUST_NOT_ENABLED_YET
or similar macro with a comment. As is it's confusing both people and compilers IMHO
@@ -313,6 +313,9 @@ static void schedule_ll_tasks_run(void *data) | |||
|
|||
perf_cnt_init(&sch->pcd); | |||
|
|||
notifier_event(sch, NOTIFIER_ID_LL_PRE_RUN, | |||
NOTIFIER_TARGET_CORE_LOCAL, NULL, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for cmocka / tb simulations?
@@ -176,6 +176,9 @@ static void zephyr_ll_run(void *data) | |||
struct list_item *list, *tmp, task_head = LIST_INIT(task_head); | |||
uint32_t flags; | |||
|
|||
notifier_event(sch, NOTIFIER_ID_LL_PRE_RUN, | |||
NOTIFIER_TARGET_CORE_LOCAL, NULL, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this notifier is only directed to the same core as the one this LL scheduler is running on. So the notification handler will be called inline. Is this really what we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as for now - yes.
Later we need to unify all DP schedulers for all cores - as one DP may affect other regardless of a core they are located on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcinszkudlinski but that also means then that those notification handlers on this core will run in the LL scheduler context, thus delaying LL tasks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, - and what's wrong about it? LL has one principle - it must FINISH within 1ms. It does not matter how
@@ -808,6 +851,7 @@ int scheduler_dp_init(void) | |||
if (ret) | |||
return ret; | |||
|
|||
notifier_register(NULL, NULL, NOTIFIER_ID_LL_PRE_RUN, scheduler_dp_ll_tick, 0); | |||
notifier_register(NULL, NULL, NOTIFIER_ID_LL_POST_RUN, scheduler_dp_ll_tick, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in fact I'm just noticing this - the DP scheduler registers an LL task, but its callback is a NOP, while the actual processing is done in a notifier, which also is running inline in the LL scheduler thread. Can we not just use the LL task callback and remove notifiers? Just use the lowest priority to run it last?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we can do it in several ways, but notifications are really for such things like DP recalculations. It is not an "LL task", but a schedule decision operation deep in SOF internals
Notifier is a right place in my opinion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcinszkudlinski I actually think that running notifiers inline isn't a good idea. Notifiers should be asynchronous - something like a work queue or a semaphore. Triggering a notifier should be atomic and have a well bounded duration, while processing takes place later and can take a long time. In particular this isn't good because it's asymmetric whether the handler is on the same or a different core, while in fact it should be symmetric. We've discussed it before, and I still find this wrong now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introducing extra LL task(s) for DP calculations won't change a bit. DP calculations will be executed in exactly same way - in LL context, before first LL processing task and after last LL processing task. Timings will be the same. Just more complicated and less clear how it goes - because it will be hidden under priorities
I would even think about hard-coded calls to DP, not by notifications. @softwarecki - you're the owner now ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcinszkudlinski I'd prefer a hard-coded call to DP over notifiers, I don't think this is a good use-case for them. But an ordinary LL task still looks like a good solution for this too to me, potentially a better one.
the last commit shouldn't be merged obviously, adding a label. |
bf5074a
to
3e00247
Compare
changes - all "resolved" issues. Still todo: documentation |
3e00247
to
f597a87
Compare
rebase only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not clear how the IBS/OBS are configured with the source/sink buffers around headroom. Also questions on DP task priorities with EDF.
src/schedule/zephyr_dp_schedule.c
Outdated
* would never provide data in time (if LPT = period that means a module required 100% of | ||
* CPU for processing, so it is really the worst possible case) | ||
* | ||
* Example: if a data rate is 48samples/msec and OBS = 480samples, the "worst case" period |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we say frames instead of samples ?
src/schedule/zephyr_dp_schedule.c
Outdated
* Delayed start means that: | ||
* - when a DP module becomes ready for a first time, its deadline set to NOW | ||
* - even if DP module provides data early, the data will be hold in the buffer | ||
* till first LPT passes since DP become ready for the first time | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this EDF work when the DP tasks have different priorities ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no such thing as DP priorities. The rule is simple - DP (or DP chain) must provide data before the LL at the end of chain drain its buffer.
Note that priority in pipeline does not mean "one module or pipeline is more important than another", just "this must go first or the next one won't be able to proceed". Anyway - all LLs must finish in 1ms
In case of DP "a deadline" is in fact "priority" - all DP threads are on the same zephyr priority and the one with earliest deadline gets CPU (for Zephyr a deadline is just a number - the lowest one goes first)
In case of LL - see here
https://marcinszkudlinski.github.io/sof-docs/PAGES/architectures/firmware/sof-common/pipeline_2_0/pipeline2_0_discussion.html#module-creation-and-pipeline-iteration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think we need to spell that out in the comment and probably also the IPC header i.e. IPC priority is ignored for DP EDF modules.
/** | ||
* Get a Longest Processing Time estimation for the module, in us | ||
* It is either | ||
* - a value taken from the manifest or estimated from module period (TODO) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should also be able to take from topology.
src/audio/buffers/audio_buffer.c
Outdated
deadline_correction = | ||
module_get_LPT(data_producer_mod) * deadline_correction; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcinszkudlinski I think it might be worth adding some LOG_DBG() here for debugging/integration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reccomend some tracing - when enabled (kconfig?) report every deadline calculation, every DP thread state change. Logs may be not fast enough for this...
this commit adds a stubbed "get_LFT" operation to sink_api Stub of the function is needed because, of circular dependency. As described, calculation of LFT requires calculation of (next) module deadline, which requires calculation of (next) buffer LFT Signed-off-by: Marcin Szkudlinski <[email protected]>
as described in zephyr_dp_scheduler.c TODO: get estimation of LPT from module manifest currently only a "worst case" is calculated Signed-off-by: Marcin Szkudlinski <[email protected]>
This commit introduces calculating of LFT for a module following description from zephyr_dp_scheduler.c The implementation is ready for DP to DP deadline calculations, however, the rest of the code is not. Therefore the DP to DP part has been deactivated with proper comment Signed-off-by: Marcin Szkudlinski <[email protected]>
Recalculation of DP tasks states - readiness and deadlines - need to be called every time situation may change, that includes: - start of LL processing - end of LL processing - end of processing of any DP module this commit adds NOTIFIER_ID_LL_PRE_RUN to allow pre-ll call to DP scheduler Signed-off-by: Marcin Szkudlinski <[email protected]>
f597a87
to
82d9dfe
Compare
this commit enables (re) calculation of all DP modules deadlines at every DP scheduler run. It calculates all deadlines based on current buffers status and modules needs, as described Signed-off-by: Marcin Szkudlinski <[email protected]>
for testing and compilation Signed-off-by: Marcin Szkudlinski <[email protected]>
82d9dfe
to
f78f2bc
Compare
@marcinszkudlinski zephyr update just merged, pls check its good for you. |
I see zephyrproject-rtos/zephyr#94731 is still open |
This PR introduces proper Deadlines calculations, based on current pipeline state.
Verbose description provided in first commit
it allows to calculate deadlines even in case of DP to DP connections
Thinks todo
also a tiny change in Zephyr is required - thread deadline set to absolute timestamp, not always to "now",
zephyrproject-rtos/zephyr#94731
EDIT: documentation from .c file moved here:
thesofproject/sof-docs#515