Skip to content

cavs: budgeting for EDF tasks #3268

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
jajanusz wants to merge 3 commits into from
Closed

cavs: budgeting for EDF tasks #3268

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
229f1cc
schedule: edf: cycles budget
jajanusz Aug 4, 2020
20a6d02
drivers: cavs: set budget for IPC task
jajanusz Aug 4, 2020
980163e
idc: set budget for IDC task
jajanusz Aug 4, 2020
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
7 changes: 7 additions & 0 deletions Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,13 @@ config INTERRUPT_LEVEL_5

rsource "src/Kconfig"

config IDC_TASK_BUDGET
int "Cycles budget for IDC task"
depends on MULTICORE
default 8000
help
Cycles budget for IDC task per systick (see config SYSTICK).

choice
prompt "Optimization"
default OPTIMIZE_FOR_PERFORMANCE
Expand Down
7 changes: 7 additions & 0 deletions src/drivers/intel/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ config INTEL_DMIC
Select this to enable Intel DMIC driver. The DMIC driver provides
as DAI the SoC direct attach digital microphones interface.

config INTEL_CAVS_IPC_TASK_BUDGET
int "Intel cycles budget for IPC task"
depends on CAVS
default 8000
help
Cycles budget for IPC task per systick (see config SYSTICK).

if INTEL_DMIC

choice
Expand Down
6 changes: 4 additions & 2 deletions src/drivers/intel/cavs/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,8 +296,10 @@ int platform_ipc_init(struct ipc *ipc)
ipc_set_drvdata(ipc, NULL);

/* schedule */
schedule_task_init_edf(&ipc->ipc_task, SOF_UUID(ipc_task_uuid),
&ipc_task_ops, ipc, 0, 0);
schedule_task_init_edf_with_budget(&ipc->ipc_task,
SOF_UUID(ipc_task_uuid),
&ipc_task_ops, ipc, 0, 0,
CONFIG_INTEL_CAVS_IPC_TASK_BUDGET);

/* configure interrupt */
irq = interrupt_get_irq(PLATFORM_IPC_INTERRUPT,
Expand Down
6 changes: 4 additions & 2 deletions src/idc/idc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,8 +325,10 @@ int idc_init(void)
(*idc)->payload = cache_to_uncache((struct idc_payload *)payload);

/* process task */
schedule_task_init_edf(&(*idc)->idc_task, SOF_UUID(idc_cmd_task_uuid),
&ops, *idc, cpu_get_id(), 0);
schedule_task_init_edf_with_budget(&(*idc)->idc_task,
SOF_UUID(idc_cmd_task_uuid),
&ops, *idc, cpu_get_id(), 0,
CONFIG_IDC_TASK_BUDGET);

return platform_idc_init();
}
10 changes: 10 additions & 0 deletions src/include/sof/schedule/edf_schedule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <sof/trace/trace.h>
#include <user/trace.h>
#include <stdint.h>
#include <stdbool.h>

#define edf_sch_set_pdata(task, data) \
(task->priv_data = data)
Expand All @@ -20,6 +21,10 @@

struct edf_task_pdata {
void *ctx;
uint32_t budget; /**< cycles per systick */

@plbossart plbossart Sep 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am pretty sure in the past we defined variable ticks for EDF. Would this scale here if the tick duration is changed dynamically. And I read in a comment below that we use '120k cycles in systick instead of 400k', so shouldn't there be multiple budget definitions, one per systick value?

@plbossart plbossart Sep 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

also explain that bugdet can be zero, and the difference in behavior between zero and non-zero values.

uint64_t last_systick; /**< last systick in which budget
* was granted to task
*/
};

int scheduler_init_edf(void);
Expand All @@ -28,4 +33,9 @@ int schedule_task_init_edf(struct task *task, uint32_t uid,
const struct task_ops *ops,
void *data, uint16_t core, uint32_t flags);

int schedule_task_init_edf_with_budget(struct task *task, uint32_t uid,
const struct task_ops *ops,
void *data, uint16_t core,
uint32_t flags, uint32_t cycles_budget);

#endif /* __SOF_SCHEDULE_EDF_SCHEDULE_H__ */
54 changes: 53 additions & 1 deletion src/schedule/edf_schedule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,26 @@ static void schedule_edf_task_run(struct task *task, void *data)
}
}

static inline uint64_t edf_eval_task_deadline(struct task *task,
uint64_t systick)
{
struct edf_task_pdata *edf_pdata = edf_sch_get_pdata(task);

if (!edf_pdata->budget)

@plbossart plbossart Sep 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe clarify with a comment in the structure definition that a budget of zero will result in a specific behavior. It's also not clear to me if this is a feature or an error condition.

return task_get_deadline(task);

/* check if task was already executed in current systick */
if (edf_pdata->last_systick != systick) {
edf_pdata->last_systick = systick;
return task_get_deadline(task);
}

/* when there are no other tasks to run,
* task can be executed even if it used systick's budget,
*/
return SOF_TASK_DEADLINE_ALMOST_IDLE;
}

static void edf_scheduler_run(void *data)
{
struct edf_schedule_data *edf_sch = data;
Expand All @@ -64,9 +84,20 @@ static void edf_scheduler_run(void *data)
struct task *task;
uint64_t deadline;
uint32_t flags;
uint64_t systick;

tr_dbg(&edf_tr, "edf_scheduler_run()");

timer_disable(cpu_timer_get(), edf_sch, cpu_get_id());

/* budget is given per systick, for example if systick is configured
* as 1ms and budget is 8000 cycles, then task will be able to use
* up to 8MCPS (or more if there are no other tasks)
*/
systick = platform_timer_get(timer_get()) /

@lgirdwood lgirdwood Aug 27, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are we basing the budget on 8000 timer_ticks ? If so, I think this would be better as a per platform CONFIG_ variable (given that non CAVS platforms will have a different timer).

@jajanusz jajanusz Sep 3, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's just an example. This is defined as CONFIG and defaults to 8k.

@lgirdwood lgirdwood Sep 7, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok, it's probably good to explain this in the Kconfig "help" for the option. I guess 8k is based on a % of total budget (based on max clock rate?). Just looking for detail so that the option can be easily tuned based on teh help.

@jajanusz jajanusz Sep 10, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well it's already in help in kconfig - that it's within 1 systick IDK if we need to add more there.
It's not really % because when you switch from HPRO to LPRO you have 120k cycles in systick instead of 400k and budget for IPCs stays the same. It's just value enough to handle IPCs without stalling other tasks.

(clock_ms_to_ticks(PLATFORM_DEFAULT_CLOCK, 1)
* (CONFIG_SYSTICK_PERIOD / 1000));

irq_local_disable(flags);

/* find next task to run */
Expand All @@ -77,7 +108,7 @@ static void edf_scheduler_run(void *data)
task->state != SOF_TASK_STATE_RUNNING)
continue;

deadline = task_get_deadline(task);
deadline = edf_eval_task_deadline(task, systick);

if (deadline == SOF_TASK_DEADLINE_NOW) {
/* task needs to be scheduled ASAP */
Expand Down Expand Up @@ -134,6 +165,15 @@ static int schedule_edf_task(void *data, struct task *task, uint64_t start,
int schedule_task_init_edf(struct task *task, uint32_t uid,
const struct task_ops *ops,
void *data, uint16_t core, uint32_t flags)
{
return schedule_task_init_edf_with_budget(task, uid, ops, data,
core, flags, 0);
}

int schedule_task_init_edf_with_budget(struct task *task, uint32_t uid,
const struct task_ops *ops,
void *data, uint16_t core,
uint32_t flags, uint32_t cycles_budget)
{
struct edf_task_pdata *edf_pdata = NULL;
int ret = 0;
Expand Down Expand Up @@ -182,6 +222,7 @@ int schedule_task_init_edf(struct task *task, uint32_t uid,

static int schedule_edf_task_running(void *data, struct task *task)
{
struct edf_schedule_data *edf_sch = data;
struct edf_task_pdata *edf_pdata = edf_sch_get_pdata(task);
uint32_t flags;

Expand All @@ -192,6 +233,13 @@ static int schedule_edf_task_running(void *data, struct task *task)
task_context_set(edf_pdata->ctx);
task->state = SOF_TASK_STATE_RUNNING;

/* if budget is not set then it's not budgeted task */
if (edf_pdata->budget) {
timer_set(cpu_timer_get(), timer_get_system(cpu_timer_get())
+ edf_pdata->budget);
timer_enable(cpu_timer_get(), edf_sch, cpu_get_id());
}

irq_local_enable(flags);

return 0;
Expand Down Expand Up @@ -278,6 +326,8 @@ int scheduler_init_edf(void)
interrupt_register(edf_sch->irq, edf_scheduler_run, edf_sch);
interrupt_enable(edf_sch->irq, edf_sch);

timer_register(cpu_timer_get(), edf_scheduler_run, edf_sch);

return 0;
}

Expand All @@ -288,6 +338,8 @@ static void scheduler_free_edf(void *data)

irq_local_disable(flags);

timer_unregister(cpu_timer_get(), edf_sch);

/* disable and unregister EDF scheduler interrupt */
interrupt_disable(edf_sch->irq, edf_sch);
interrupt_unregister(edf_sch->irq, edf_sch);
Expand Down