Skip to content

Bunch of fixes to DP processing #8467

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.

Sign up for GitHub

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

Closed
marcinszkudlinski wants to merge 3 commits into from
Closed

Bunch of fixes to DP processing #8467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b30d200
mod: too quick draining buffer fix
marcinszkudlinski Oct 20, 2023
0405692
DP: bugix - lists initialization was done too late
marcinszkudlinski Oct 31, 2023
b84b9e8
DP: provide data to next LL module no earlier than DP deadline
marcinszkudlinski Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions src/audio/module_adapter/module_adapter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ static int module_adapter_dp_queue_prepare(struct comp_dev *dev)
* first, set all parameters by calling "module prepare" with pointers to
* "main" audio_stream buffers
*/
list_init(&mod->dp_queue_ll_to_dp_list);
Copy link
Member

@lgirdwood lgirdwood Nov 13, 2023

Choose a reason for hiding this comment

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

We can merge this one straight away if WA patch alignment takes long time.

list_init(&mod->dp_queue_dp_to_ll_list);

ret = module_adapter_sink_src_prepare(dev);
if (ret)
return ret;
Expand All @@ -177,7 +180,6 @@ static int module_adapter_dp_queue_prepare(struct comp_dev *dev)
* and copy stream parameters to shadow buffers
*/
i = 0;
list_init(&mod->dp_queue_ll_to_dp_list);
list_for_item(blist, &dev->bsource_list) {
struct comp_buffer *source_buffer =
container_of(blist, struct comp_buffer, sink_list);
Expand Down Expand Up @@ -211,7 +213,6 @@ static int module_adapter_dp_queue_prepare(struct comp_dev *dev)
unsigned int period = UINT32_MAX;

i = 0;
list_init(&mod->dp_queue_dp_to_ll_list);
list_for_item(blist, &dev->bsink_list) {
struct comp_buffer *sink_buffer =
container_of(blist, struct comp_buffer, source_list);
Expand Down Expand Up @@ -1068,18 +1069,28 @@ static int module_adapter_copy_dp_queues(struct comp_dev *dev)
dp_queue = dp_queue_get_next_item(dp_queue);
}

if (mod->dp_startup_delay)
return 0;

dp_queue = dp_queue_get_first_item(&mod->dp_queue_dp_to_ll_list);
list_for_item(blist, &dev->bsink_list) {
/* output - we need to copy data from dp_queue (as source)
* to audio_stream (as sink)
*/
/* output - we need to copy data from dp_queue (as source)
* to audio_stream (as sink)
*/
Copy link
Collaborator

@kv2019i kv2019i Nov 14, 2023

Choose a reason for hiding this comment

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

I don't think we should drop this comment (from first patch in the series). The logic to not feed more than on LL is still in place, right, and should be explained.

assert(dp_queue);
struct comp_buffer *buffer =
container_of(blist, struct comp_buffer, source_list);
struct sof_sink *data_sink = audio_stream_get_sink(&buffer->stream);
struct sof_source *following_mod_data_source =
audio_stream_get_source(&buffer->stream);
struct sof_source *data_src = dp_queue_get_source(dp_queue);
uint32_t to_copy = MIN(sink_get_free_size(data_sink),
source_get_data_available(data_src));
size_t dp_data_available = source_get_data_available(data_src);

if (!dp_data_available)
comp_err(dev, "!!!! no data available from DP");

uint32_t to_copy = MIN(source_get_min_available(following_mod_data_source),
Copy link
Collaborator

@lyakh lyakh Nov 10, 2023

Choose a reason for hiding this comment

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

shouldn't we fix offending LL modules one by one - as soon as you generate respective topologies?

Copy link
Contributor Author

@marcinszkudlinski marcinszkudlinski Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

absolutely we do
But at the moment we need a working WA for google AEC topology - and we need it ASAP
EDIT: my idea is to verify/fix this when converting module to sink/src interface. All other will be protected by this WA

Copy link
Member

@lgirdwood lgirdwood Nov 14, 2023

Choose a reason for hiding this comment

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

ok, since this is a temp WA, we probably want to create an issue to track and link it to the comments here. We can then check all the other modules as we build topologies and remove the WA.
I think it also make sense to put the WA in an ifdef MODULE_WA_FOR_BLAH so that its easy for anyone to fix all modules and cleanly remove (and test module updates)..

Copy link
Collaborator

@kv2019i kv2019i Nov 14, 2023

Choose a reason for hiding this comment

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

This is at least in dp_copy_queues() function, so specific to DP-LL interface and the comment explains what is done here, so could be worse.

Copy link
Contributor Author

@marcinszkudlinski marcinszkudlinski Nov 15, 2023

Choose a reason for hiding this comment

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

@lgirdwood I don't think it is neccessary
dp_copy_queues in fact is a kind of WA itself. It is a double-buffer copying from LL buffers to pipeline 2.0 sink/src interface. It makes a DP module to look from the other components point of view (almost) exactly as a "normal" LL

Once all modules use sink/src and communication between them is in pipeline 2.0 manner (I'm preparng an RFC for this), dp_copy_queues will be removed in whole (well, hopefully together with module adapter)

Copy link
Member

@lgirdwood lgirdwood Nov 17, 2023

Choose a reason for hiding this comment

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

@marcinszkudlinski ok, and we should be able to track this in RFC GH issue. i.e. we can mention all the PRs - sound like we target v2.9 for this too.

dp_data_available);

err = source_to_sink_copy(data_src, data_sink, true, to_copy);
if (err)
Expand Down
29 changes: 29 additions & 0 deletions src/include/sof/audio/module_adapter/module/generic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,35 @@ struct processing_module {
/* module-specific flags for comp_verify_params() */
uint32_t verify_params_flags;

/* indicates that this DP module did not yet reach its first deadline and
* no data should be passed yet to next LL module
*
* why: lets assume DP with 10ms period (a.k.a a deadline). It starts and finishes
* Earlier, i.e. in 2ms providing 10ms of data. LL starts consuming data in 1ms chunks and
* will drain 10ms buffer in 10ms, expecting a new portion of data on 11th ms
* BUT - the DP module deadline is still 10ms, regardless if it had finished earlier
* and it is completely fine that processing in next cycle takes full 10ms - as long as it
* fits into the deadline.
* It may lead to underruns:
*
* LL1 (1ms) ---> DP (10ms) -->LL2 (1ms)
*
* ticks 0..9 -> LL1 is producing 1ms data portions, DP is waiting, LL2 is waiting
* tick 10 - DP has enough data to run, it starts processing
* tick 12 - DP finishes earlier, LL2 starts consuming, LL1 is producing data
* ticks 13-19 LL1 is producing data, LL2 is consuming data (both in 1ms chunks)
* tick 20 - DP starts processing a new portion of 10ms data, having 10ms to finish
* !!!! but LL2 has already consumed 8ms !!!!
* tick 22 - LL2 is consuming the last 1ms data chunk
* tick 23 - DP is still processing, LL2 has no data to process
* !!! UNDERRUN !!!!
* tick 19 - DP finishes properly in a deadline time
*
* Solution: even if DP finishes before its deadline, the data must be held till
* deadline time, so LL2 may start processing no earlier than tick 20
*/
bool dp_startup_delay;
Copy link
Member

@lgirdwood lgirdwood Nov 17, 2023

Choose a reason for hiding this comment

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

thinking aloud - would this be better as a int i.e. a counter and could be set to any delay value needed and decremented on each LL tick ?


/* flag to indicate module does not pause */
bool no_pause;

Expand Down
38 changes: 27 additions & 11 deletions src/schedule/zephyr_dp_schedule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,12 @@ struct scheduler_dp_data {

struct task_dp_pdata {
k_tid_t thread_id; /* zephyr thread ID */
uint32_t period_clock_ticks; /* period the task should be scheduled in Zephyr ticks */
uint32_t deadline_clock_ticks; /* dp module deadline in Zephyr ticks */
uint32_t deadline_ll_cycles; /* dp module deadline in LL cycles */
k_thread_stack_t __sparse_cache *p_stack; /* pointer to thread stack */
struct k_sem sem; /* semaphore for task scheduling */
struct processing_module *mod; /* the module to be scheduled */
uint32_t ll_cycles_to_deadline; /* current number of LL cycles till deadline */
};

/* Single CPU-wide lock
Expand Down Expand Up @@ -227,11 +229,20 @@ void scheduler_dp_ll_tick(void *receiver_data, enum notify_id event_type, void *
lock_key = scheduler_dp_lock();
list_for_item(tlist, &dp_sch->tasks) {
curr_task = container_of(tlist, struct task, list);
pdata = curr_task->priv_data;
struct processing_module *mod = pdata->mod;

/* decrease number of LL ticks/cycles left till the module reaches its deadline */
if (pdata->ll_cycles_to_deadline) {
pdata->ll_cycles_to_deadline--;
if (!pdata->ll_cycles_to_deadline)
/* deadline reached, clear startup delay flag.
* see dp_startup_delay comment for details
*/
mod->dp_startup_delay = false;
}

/* step 1 - check if the module is ready for processing */
if (curr_task->state == SOF_TASK_STATE_QUEUED) {
pdata = curr_task->priv_data;
struct processing_module *mod = pdata->mod;
bool mod_ready;

mod_ready = module_is_ready_to_process(mod, mod->sources,
Expand All @@ -240,11 +251,13 @@ void scheduler_dp_ll_tick(void *receiver_data, enum notify_id event_type, void *
mod->num_of_sinks);
if (mod_ready) {
/* set a deadline for given num of ticks, starting now */
k_thread_deadline_set(pdata->thread_id, pdata->period_clock_ticks);
k_thread_deadline_set(pdata->thread_id,
pdata->deadline_clock_ticks);

/* trigger the task */
curr_task->state = SOF_TASK_STATE_RUNNING;
k_sem_give(&pdata->sem);
pdata->ll_cycles_to_deadline = pdata->deadline_ll_cycles;
}
}
}
Expand Down Expand Up @@ -352,7 +365,7 @@ static int scheduler_dp_task_shedule(void *data, struct task *task, uint64_t sta
struct scheduler_dp_data *dp_sch = (struct scheduler_dp_data *)data;
struct task_dp_pdata *pdata = task->priv_data;
unsigned int lock_key;
uint64_t period_clock_ticks;
uint64_t deadline_clock_ticks;

lock_key = scheduler_dp_lock();

Expand All @@ -371,13 +384,16 @@ static int scheduler_dp_task_shedule(void *data, struct task *task, uint64_t sta
task->state = SOF_TASK_STATE_QUEUED;
list_item_prepend(&task->list, &dp_sch->tasks);

period_clock_ticks = period * CONFIG_SYS_CLOCK_TICKS_PER_SEC;
/* period is in us - convert to seconds in next step
* or it always will be zero because of fixed point calculation
deadline_clock_ticks = period * CONFIG_SYS_CLOCK_TICKS_PER_SEC;
/* period/deadline is in us - convert to seconds in next step
* or it always will be zero because of integer calculation
*/
period_clock_ticks /= 1000000;
deadline_clock_ticks /= 1000000;

pdata->period_clock_ticks = period_clock_ticks;
pdata->deadline_clock_ticks = deadline_clock_ticks;
pdata->deadline_ll_cycles = period / LL_TIMER_PERIOD_US;
pdata->ll_cycles_to_deadline = 0;
pdata->mod->dp_startup_delay = true;
scheduler_dp_unlock(lock_key);

tr_dbg(&dp_tr, "DP task scheduled with period %u [us]", (uint32_t)period);
Expand Down