Skip to content

Correctness and style fixes #4856

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

Merged
lgirdwood merged 3 commits into from
Oct 15, 2021
Merged

Correctness and style fixes #4856

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
dc02b59
timer_domain: don't unregister interrupt handler from itself
lyakh Sep 23, 2021
94f7b20
ll_schedule: don't re-enable the domain if no tasks are active
lyakh Sep 23, 2021
87d9c5e
ll_schedule: (cosmetic) remove a redundant type-cast
lyakh Sep 24, 2021
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
5 changes: 3 additions & 2 deletions src/schedule/ll_schedule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,8 @@ static void schedule_ll_tasks_run(void *data)
}

/* tasks on current core finished, re-enable domain on it */
domain_enable(domain, core);
if (atomic_read(&sch->num_tasks))
domain_enable(domain, core);

spin_unlock(&domain->lock);

Expand Down Expand Up @@ -329,7 +330,7 @@ static void schedule_ll_domain_clear(struct ll_schedule_data *sch,
domain_disable(domain, cpu_get_id());

/* unregister the task */
domain_unregister(domain, task, (uint32_t)atomic_read(&sch->num_tasks));
domain_unregister(domain, task, atomic_read(&sch->num_tasks));

tr_info(&ll_tr, "num_tasks %d total_num_tasks %d",
atomic_read(&sch->num_tasks),
Expand Down
28 changes: 8 additions & 20 deletions src/schedule/timer_domain.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,42 +49,30 @@ static int timer_domain_register(struct ll_schedule_domain *domain,
{
struct timer_domain *timer_domain = ll_sch_domain_get_pdata(domain);
int core = cpu_get_id();
int ret = 0;

tr_dbg(&ll_tr, "timer_domain_register()");

/* tasks already registered on this core */
if (timer_domain->arg[core])
goto out;
return 0;

timer_domain->arg[core] = arg;
ret = timer_register(timer_domain->timer, handler, arg);

tr_info(&ll_tr, "timer_domain_register domain->type %d domain->clk %d domain->ticks_per_ms %d",
domain->type, domain->clk, domain->ticks_per_ms);
out:

return ret;
return timer_register(timer_domain->timer, handler, arg);
}

static int timer_domain_unregister(struct ll_schedule_domain *domain,
struct task *task, uint32_t num_tasks)
{
struct timer_domain *timer_domain = ll_sch_domain_get_pdata(domain);
int core = cpu_get_id();

tr_dbg(&ll_tr, "timer_domain_unregister()");

/* tasks still registered on this core */
if (!timer_domain->arg[core] || num_tasks)
return 0;

tr_info(&ll_tr, "timer_domain_unregister domain->type %d domain->clk %d",
domain->type, domain->clk);

timer_unregister(timer_domain->timer, timer_domain->arg[core]);

timer_domain->arg[core] = NULL;
/*
* domain_unregister() can be called from the domain handler itself,
* and we cannot unregister the timer interrupt handler from the handler
* itself. Keep the interrupt registered but disabled. This assumes that
* the handler is at least not unloaded.
*/

@keyonjie keyonjie Oct 18, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@lgirdwood @lyakh the finding here is good, but this is behavior change for the timer_domain_unregister().

Better just remove the unregister call from the interrupt handler itself, while keep this timer_domain_unregister() unchanged:

diff --git a/src/schedule/ll_schedule.c b/src/schedule/ll_schedule.c
index c63d34a20a70..59309af3b8cd 100644
--- a/src/schedule/ll_schedule.c
+++ b/src/schedule/ll_schedule.c
@@ -107,11 +107,6 @@ static void schedule_ll_task_update_start(struct ll_schedule_data *sch,
 static void schedule_ll_task_done(struct ll_schedule_data *sch,
                                  struct task *task)
 {
-       /* Remove from the task list, schedule_task_cancel() won't handle it again */
-       list_item_del(&task->list);
-
-       domain_unregister(sch->domain, task, atomic_sub(&sch->num_tasks, 1) - 1);
-
        tr_info(&ll_tr, "task complete %p %pU", task, task->uid);
        tr_info(&ll_tr, "num_tasks %d total_num_tasks %d",
                atomic_read(&sch->num_tasks),

@lgirdwood lgirdwood Oct 18, 2021

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.

@lyakh a followup PR needed ?

@lyakh lyakh Oct 19, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@lgirdwood I don't know. I'd prefer to know exactly what that would fix. Also I'm not 100% certain we always cancel all tasks. I actually think we don't. I think it's valid to just return COMPLETED from a task and not cancel it at all.

@keyonjie keyonjie Oct 19, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@lgirdwood I don't know. I'd prefer to know exactly what that would fix. Also I'm not 100% certain we always cancel all tasks. I actually think we don't. I think it's valid to just return COMPLETED from a task and not cancel it at all.

@lyakh I think yes, as the task_done itself can't unregister the handler from itself, we need to call .schedule_task_cancel() for all tasks added via .schedule_task(), if any exception found, we should fix it.

The .schedule_task_cancel() is something like finishing a task formally, it changes the task state to _CANCEL, delete it from the scheduler task list and decrease scheduler task count, and eventually unregister task from domain. No matter if the task is already done or not, from running POV, we need this formal symmetric ops.

At the moment, we follow that for all pipelines, for SA task, for DMIC ramping task, for IDC comp copy task, for DMA trace task and probe task.


return 0;
}
Expand Down