Skip to content

schedule: dma_multi_chan: do not skip tasks for dma_domain scheduler#3803

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
dbaluta merged 2 commits into from
Feb 9, 2021
Merged

schedule: dma_multi_chan: do not skip tasks for dma_domain scheduler #3803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61ad267
schedule: dma_multi_chan: do not skip tasks for dma_domain scheduler
iuliana-prodan Feb 1, 2021
72c1896
platform: imx: use DMA_DOMAIN in full synchronous mode
iuliana-prodan Feb 1, 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
2 changes: 2 additions & 0 deletions src/include/sof/schedule/ll_schedule.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -30,6 +30,8 @@ extern struct tr_ctx ll_tr;

@dbalutadbalutaFeb 2, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@iuliana-prodan the patch looks good to me. I understand the reasons behind it. Is it possible at our current level of understanding to specify what is the difference between synchronous and full_sync?

@dbalutadbalutaFeb 2, 2021•
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@iuliana-prodan also can you enhance the commit message in order to better describe what is the current problem.

This

We do this because for the DMA_DOMAIN there are no guarantees
for the accuracy of the period of the registrable task
which drives the entire scheduling

is fine.

We need to include in the commit message along the lines the scenario we are using and facing issues:

e.g

In the case we are using a mixer topology there will be 3 pipelines for playback scenario:

  • 1 pipeline for pcm1.0 (task 0)
  • 1 pipeline for pcm1.1 (task 1)
  • 1 pipeline for DAI (task 2)

Each of these pipelines correspond to a task.

As far as I understood one of the task0 or task1 sometimes starves thus resulting in timeouts on Host.

@lgirdwoodlgirdwoodFeb 2, 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.

ack, I'm not fully following the difference between DMA and full sync.

@iuliana-prodaniuliana-prodanFeb 2, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@dbaluta@lgirdwood The difference is that now, the DMA synchronous is still time dependent: the task->start is used to schedule a task.
The full synchronous is not using the time anymore - is based on the period of each task.

I could have put all these changes on synchronous attribute, but I wanted it to have a small impact on the other targets that use dma_multi_chan domain.

@iuliana-prodaniuliana-prodanFeb 2, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Here’s an example of the issue we see on imx.
In the case of a mixer topology there will be 3 pipelines for playback scenario:

  • 1 pipeline for pcm1.0 (task 0);
  • 1 pipeline for pcm1.1 (task 1);
  • 1 pipeline for DAI (task 2).

Task 2 is the registrable task and this is always scheduled.
The other tasks (task 0 and task 1) are scheduled based on task->start.

The problem we see is that, at some point, the DMA transfer from DAI is finished earlier than task->start for one of the task 0 or task 1 and this is not scheduled anymore (based on the check from https://github.com/thesofproject/sof/blob/master/src/schedule/dma_multi_chan_domain.c#L308).
And, task 2 ends up depleting one of the source buffers, thus blocking the entire data flow.

So, task 0 and task 1 should be run at the frequency they were configured with.

@dbaluta I'll add a part of this in the commit message, as you suggested.

@lgirdwoodlgirdwoodFeb 3, 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.

@iuliana-prodan thanks got you now. Can we have this as scheduling type for topology too as a subsequent PR as it seems something we should be specifying at that level along side the other pipeline scheduling config.

@iuliana-prodaniuliana-prodanFeb 4, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@lgirdwood I've created #3817 for your proposal. Let's continue the discussion there. Thanks!

struct ll_task_pdata {
uint64_t period;
uint16_t ratio; /**< ratio of periods compared to the registrable task */
uint16_t skip_cnt; /**< how many times the task was skipped for execution */
};

int scheduler_init_ll(struct ll_schedule_domain *domain);
Expand Down
2 changes: 2 additions & 0 deletions src/include/sof/schedule/ll_schedule_domain.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -50,6 +50,7 @@ struct ll_schedule_domain {
int type; /**< domain type */
int clk; /**< source clock */
bool synchronous; /**< are tasks should be synchronous */
bool full_sync; /**< tasks should be full synchronous, no time dependent */
void *priv_data; /**< pointer to private data */
bool registered[CONFIG_CORE_COUNT]; /**< registered cores */
bool enabled[CONFIG_CORE_COUNT]; /**< enabled cores */
Expand DownExpand Up@@ -80,6 +81,7 @@ static inline struct ll_schedule_domain *domain_init
domain->type = type;
domain->clk = clk;
domain->synchronous = synchronous;
domain->full_sync = false;
domain->ticks_per_ms = clock_ms_to_ticks(clk, 1);
domain->ops = ops;

Expand Down
3 changes: 3 additions & 0 deletions src/include/sof/schedule/task.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -31,6 +31,9 @@ struct sof;
#define SOF_TASK_DEADLINE_ALMOST_IDLE (SOF_TASK_DEADLINE_IDLE - 1)
#define SOF_TASK_DEADLINE_NOW 0

/** \brief Task counter initial value. */
#define SOF_TASK_SKIP_COUNT 0xFFFFu

/** \brief Task states. */
enum task_state {
SOF_TASK_STATE_INIT = 0,
Expand Down
3 changes: 3 additions & 0 deletions src/platform/imx8/platform.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -176,6 +176,9 @@ int platform_init(struct sof *sof)
sof->platform_dma_domain = dma_multi_chan_domain_init
(&sof->dma_info->dma_array[0], 1,
PLATFORM_DEFAULT_CLOCK, false);

/* i.MX platform DMA domain will be full synchronous, no time dependent */
sof->platform_dma_domain->full_sync = true;

@lgirdwoodlgirdwoodFeb 2, 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.

Seems like this should be an argument or flag to dma_multi_chan_domain_init()

@iuliana-prodaniuliana-prodanFeb 2, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

If you agree to add these changes under, the current, synchronous attribute we won't need to modify the function signature(s).

@dbalutadbalutaFeb 2, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@lgirdwood so one reason for adding a new mode (full_sync) is that we don't want to force other platforms to use our optimization.
Anyhow, I do think that this might be a real problem on Intel platforms too (although I think none of them uses DMA DAI for scheduling at this moment)

@slawblauciak@keyonjie can you guys have a look as I've seen you've experimented a little bit with scheduling code.

@lgirdwoodlgirdwoodFeb 3, 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.

ok, I think this is a topology scheduler type now.

scheduler_init_ll(sof->platform_dma_domain);

/* initialize the host IPC mechanims */
Expand Down
3 changes: 3 additions & 0 deletions src/platform/imx8m/platform.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -175,6 +175,9 @@ int platform_init(struct sof *sof)
sof->platform_dma_domain =
dma_multi_chan_domain_init(&sof->dma_info->dma_array[1],
1, PLATFORM_DEFAULT_CLOCK, true);

/* i.MX platform DMA domain will be full synchronous, no time dependent */
sof->platform_dma_domain->full_sync = true;
scheduler_init_ll(sof->platform_dma_domain);

/* initialize the host IPC mechanims */
Expand Down
29 changes: 24 additions & 5 deletions src/schedule/dma_multi_chan_domain.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -280,6 +280,7 @@ static bool dma_multi_chan_domain_is_pending(struct ll_schedule_domain *domain,
struct dma_domain *dma_domain = ll_sch_domain_get_pdata(domain);
struct pipeline_task *pipe_task = pipeline_task_get(task);
struct dma *dmas = dma_domain->dma_array;
struct ll_task_pdata *pdata;
uint32_t status;
int i;
int j;
Expand All@@ -303,11 +304,29 @@ static bool dma_multi_chan_domain_is_pending(struct ll_schedule_domain *domain,
pipe_task->sched_comp)
continue;

/* it's too soon for this task */
if (!pipe_task->registrable &&
pipe_task->task.start >
platform_timer_get_atomic(timer_get()))
continue;
/* Schedule task based on the frequency they
* were configured with, not time (task.start)
*
* There are cases when a DMA transfer from a DAI
* is finished earlier than task.start and,
* without full_sync mode, this task will not
* be scheduled
*/
if (domain->full_sync) {
Comment thread
iuliana-prodan marked this conversation as resolved.
Outdated
pdata = ll_sch_get_pdata(&pipe_task->task);
pdata->skip_cnt++;
if (pdata->skip_cnt == pdata->ratio)
pdata->skip_cnt = 0;

if (pdata->skip_cnt != 0)
continue;
} else {
/* it's too soon for this task */
if (!pipe_task->registrable &&
pipe_task->task.start >
platform_timer_get_atomic(timer_get()))
continue;
}

notifier_event(&dmas[i].chan[j], NOTIFIER_ID_DMA_IRQ,
NOTIFIER_TARGET_CORE_LOCAL,
Expand Down
44 changes: 44 additions & 0 deletions src/schedule/ll_schedule.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -324,8 +324,11 @@ static int schedule_ll_task(void *data, struct task *task, uint64_t start,
{
struct ll_schedule_data *sch = data;
struct ll_task_pdata *pdata;
struct ll_task_pdata *reg_pdata;
struct list_item *tlist;
struct task *curr_task;
struct task *registrable_task = NULL;
struct pipeline_task *pipe_task;
uint32_t flags;
int ret = 0;

Expand DownExpand Up@@ -353,6 +356,47 @@ static int schedule_ll_task(void *data, struct task *task, uint64_t start,

pdata->period = period;

/* for full synchronous domain, calculate ratio and initialize skip_cnt for task */
if (sch->domain->full_sync) {
pdata->ratio = 1;
pdata->skip_cnt = (uint16_t)SOF_TASK_SKIP_COUNT;

/* get the registrable task */
list_for_item(tlist, &sch->tasks) {
curr_task = container_of(tlist, struct task, list);
pipe_task = pipeline_task_get(curr_task);

/* registrable task found */
if (pipe_task->registrable) {
registrable_task = curr_task;
break;
}
}

/* we found a registrable task */
if (registrable_task) {
Comment thread
iuliana-prodan marked this conversation as resolved.
Outdated
reg_pdata = ll_sch_get_pdata(registrable_task);

/* update ratio for all tasks */
list_for_item(tlist, &sch->tasks) {
curr_task = container_of(tlist, struct task, list);
pdata = ll_sch_get_pdata(curr_task);

/* the assumption is that the registrable
* task has the smallest period
*/
if (pdata->period >= reg_pdata->period) {
pdata->ratio = period / reg_pdata->period;
} else {
tr_err(&ll_tr,
"schedule_ll_task(): registrable task has a period longer than current task");
ret = -EINVAL;
goto out;
}
}
}
}

/* insert task into the list */
schedule_ll_task_insert(task, &sch->tasks);

Expand Down