From ae98ac2a08250b99a418dcf2f6c7b6f77cc16c44 Mon Sep 17 00:00:00 2001 From: Laurentiu Mihalcea Date: Mon, 22 May 2023 15:28:27 +0300 Subject: [PATCH 1/5] schedule: zephyr_dma_domain: Add support for DMACs with shared IRQs In the case of DMACs having a single IRQ for all their channels, dma_irq_handler() is registered for each of these channels resulting in it giving the semaphore more than 1 resource at a time which is wrong. This commit fixes the issue by checking if the IRQ was triggered for the channel passed as argument to the handler before giving the semaphore resources. Signed-off-by: Laurentiu Mihalcea --- src/schedule/zephyr_dma_domain.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/schedule/zephyr_dma_domain.c b/src/schedule/zephyr_dma_domain.c index 41b01e3d6200..afd6086e243d 100644 --- a/src/schedule/zephyr_dma_domain.c +++ b/src/schedule/zephyr_dma_domain.c @@ -143,13 +143,16 @@ static void dma_irq_handler(void *data) irq = zephyr_dma_domain_data->irq; dt = zephyr_dma_domain_data->dt; - /* clear IRQ */ - dma_interrupt_legacy(channel, DMA_IRQ_CLEAR); - interrupt_clear_mask(irq, BIT(channel_index)); - - /* give resources to thread semaphore */ - if (dt->handler) - k_sem_give(sem); + /* was the IRQ triggered by this channel? */ + if (dma_interrupt_legacy(channel, DMA_IRQ_STATUS_GET)) { + /* clear IRQ */ + dma_interrupt_legacy(channel, DMA_IRQ_CLEAR); + interrupt_clear_mask(irq, BIT(channel_index)); + + /* give resources to thread semaphore */ + if (dt->handler) + k_sem_give(sem); + } } static int enable_dma_irq(struct zephyr_dma_domain_data *data) From f69dd359c1fe8e146814ea4525675e0e5a6468b7 Mon Sep 17 00:00:00 2001 From: Laurentiu Mihalcea Date: Mon, 22 May 2023 15:33:51 +0300 Subject: [PATCH 2/5] sof: audio: pipeline.h: Add field priv to struct pipeline_task This field will be used for Zepyhr DMA domain internal housekeeping. Signed-off-by: Laurentiu Mihalcea --- src/include/sof/audio/pipeline.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/include/sof/audio/pipeline.h b/src/include/sof/audio/pipeline.h index d80e4380aa82..d604e347fb7f 100644 --- a/src/include/sof/audio/pipeline.h +++ b/src/include/sof/audio/pipeline.h @@ -129,6 +129,10 @@ struct pipeline_task { struct task task; /**< parent structure */ bool registrable; /**< should task be registered on irq */ struct comp_dev *sched_comp; /**< pipeline scheduling component */ + /* private data used by Zephyr DMA domain for internal + * housekeeping - don't touch. + */ + void *priv; }; #define pipeline_task_get(t) container_of(t, struct pipeline_task, task) From cbae30075e7b0205f4626419c290919f92108df4 Mon Sep 17 00:00:00 2001 From: Laurentiu Mihalcea Date: Mon, 22 May 2023 15:44:46 +0300 Subject: [PATCH 3/5] sof: schedule: ll_schedule_domain.h: Make domain_is_pending optional Since domain_is_pending is not used by all domains (e.g: Intel's Zephyr timer domain) and we don't want to force the domains to pointlessly define their own domain_is_pending which would return true we do this inside ll_schedule_domain.h. If the domain_is_pending() operation is not defined then the function will just return true, thus not affecting the flow for domains which don't need this function. Signed-off-by: Laurentiu Mihalcea --- src/include/sof/schedule/ll_schedule_domain.h | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/include/sof/schedule/ll_schedule_domain.h b/src/include/sof/schedule/ll_schedule_domain.h index f555c256398b..126939f168fc 100644 --- a/src/include/sof/schedule/ll_schedule_domain.h +++ b/src/include/sof/schedule/ll_schedule_domain.h @@ -195,13 +195,10 @@ static inline void domain_disable(struct ll_schedule_domain *domain, int core) static inline bool domain_is_pending(struct ll_schedule_domain *domain, struct task *task, struct comp_dev **comp) { - bool ret; - - assert(domain->ops->domain_is_pending); - - ret = domain->ops->domain_is_pending(domain, task, comp); - - return ret; + if (domain->ops->domain_is_pending) + return domain->ops->domain_is_pending(domain, task, comp); + else + return true; } From 481766fa0f7ed450c4d05b334e8e9e8c60454ab2 Mon Sep 17 00:00:00 2001 From: Laurentiu Mihalcea Date: Mon, 22 May 2023 15:47:04 +0300 Subject: [PATCH 4/5] zephyr: schedule: Don't schedule tasks unless an IRQ was triggered Currently, tasks are scheduled whenever an IRQ comes. This means that, for example, if there's 2 pipeline tasks (capture and playback => 2 channels) and the DMA IRQ is triggered for the capture pipeline task, the playback task will also be scheduled resulting in warnings such as "no bytes to copy". This commit fixes this issue by making use of the domain_is_pending() function which will return true if the task can be executed. This only affects the Zephyr DMA domain. Currently, this approach will not work for the mixer. Signed-off-by: Laurentiu Mihalcea --- src/schedule/zephyr_dma_domain.c | 48 +++++++++++++++++++++++++++++++- src/schedule/zephyr_ll.c | 4 +++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/src/schedule/zephyr_dma_domain.c b/src/schedule/zephyr_dma_domain.c index afd6086e243d..a560dc569a76 100644 --- a/src/schedule/zephyr_dma_domain.c +++ b/src/schedule/zephyr_dma_domain.c @@ -52,6 +52,10 @@ struct zephyr_dma_domain_data { struct dma_chan_data *channel; /* channel data */ bool enabled_irq; /* true if DMA IRQ was already enabled */ struct zephyr_dma_domain_thread *dt; + /* set to true by the DMA IRQ handler when an IRQ for + * associated channel was triggered. + */ + bool triggered; }; struct zephyr_dma_domain_thread { @@ -80,11 +84,14 @@ static int zephyr_dma_domain_unregister(struct ll_schedule_domain *domain, uint32_t num_tasks); static void zephyr_dma_domain_task_cancel(struct ll_schedule_domain *domain, struct task *task); +static bool zephyr_dma_domain_is_pending(struct ll_schedule_domain *domain, + struct task *task, struct comp_dev **comp); static const struct ll_schedule_domain_ops zephyr_dma_domain_ops = { .domain_register = zephyr_dma_domain_register, .domain_unregister = zephyr_dma_domain_unregister, - .domain_task_cancel = zephyr_dma_domain_task_cancel + .domain_task_cancel = zephyr_dma_domain_task_cancel, + .domain_is_pending = zephyr_dma_domain_is_pending, }; struct ll_schedule_domain *zephyr_dma_domain_init(struct dma *dma_array, @@ -145,6 +152,11 @@ static void dma_irq_handler(void *data) /* was the IRQ triggered by this channel? */ if (dma_interrupt_legacy(channel, DMA_IRQ_STATUS_GET)) { + /* IRQ is for this channel. Make sure that the scheduler + * can schedule the associated pipeline task. + */ + zephyr_dma_domain_data->triggered = true; + /* clear IRQ */ dma_interrupt_legacy(channel, DMA_IRQ_CLEAR); interrupt_clear_mask(irq, BIT(channel_index)); @@ -219,6 +231,15 @@ static int register_dma_irq(struct zephyr_dma_domain *domain, crt_data->channel = crt_chan; crt_data->dt = dt; + /* attach crt_data to pipeline task. + * + * this will be used in + * zephyr_dma_domain_is_pending to decide + * whether there was a DMA IRQ for the channel + * associated with this task. + */ + pipe_task->priv = crt_data; + if (dt->started) { /* if the Zephyr thread was started, we * can safely enable the DMA IRQs @@ -494,3 +515,28 @@ static void zephyr_dma_domain_task_cancel(struct ll_schedule_domain *domain, k_sem_give(&dt->sem); } } + +static bool zephyr_dma_domain_is_pending(struct ll_schedule_domain *domain, + struct task *task, struct comp_dev **comp) +{ + struct zephyr_dma_domain_data *data; + struct pipeline_task *pipe_task; + + pipe_task = pipeline_task_get(task); + data = pipe_task->priv; + + /* note: there's no need to disable IRQs here as they already + * are inside zephyr_ll_run when this function is called. + * + * VERY IMPORTANT: if this function needs to be moved outside + * of the atomic area PLEASE make sure to disable IRQs before + * calling it or disable IRQs here. + */ + + if (data->triggered) { + data->triggered = false; + return true; + } + + return false; +} diff --git a/src/schedule/zephyr_ll.c b/src/schedule/zephyr_ll.c index 7405dc0b22a9..cc195791f347 100644 --- a/src/schedule/zephyr_ll.c +++ b/src/schedule/zephyr_ll.c @@ -195,6 +195,10 @@ static void zephyr_ll_run(void *data) list_item_del(list); list_item_append(list, &task_head); + /* check if the task can be scheduled */ + if (!domain_is_pending(sch->ll_domain, task, NULL)) + continue; + zephyr_ll_unlock(sch, &flags); /* From 2389f13c3819374e99586fcb65b80940dba491fc Mon Sep 17 00:00:00 2001 From: Laurentiu Mihalcea Date: Tue, 23 May 2023 11:29:43 +0300 Subject: [PATCH 5/5] schedule: zephyr_dma_domain: Add mixer support Currently, because of the fact only registrable pipeline tasks can be executed, the mixer no longer works since the pipeline tasks going into the mixer are non-registrable. In order to overcome this issue, non-registrable pipeline tasks will point to the same struct zephyr_dma_domain_data as the registrable pipeline task so when the DMA IRQ comes and marks the registrable pipeline task as executable these non-registrable pipeline tasks can also be executed. This is done by keeping track of how many pipeline task point to the same struct zephyr_dma_domain_data and of how many times domain_is_pending() has been called for said struct zephyr_dma_domain_data. When pending_count reaches 0, data->triggered can be set to false, meaning both the registrable and non-registrable pipeline tasks have been executed. Let's assume the following scenario with 4 pipeline tasks: 1) Host -------> Volume --------> DAI 2) Host -------> Mixer ----------> DAI ^ | Host -----------| This fix assumes that the mixer pipeline tasks will be registered as a group, meaning the first pipeline task will not be registered in between the pipeline tasks associated with the mixer. Signed-off-by: Laurentiu Mihalcea --- src/schedule/zephyr_dma_domain.c | 81 +++++++++++++++++++++++--------- 1 file changed, 60 insertions(+), 21 deletions(-) diff --git a/src/schedule/zephyr_dma_domain.c b/src/schedule/zephyr_dma_domain.c index a560dc569a76..eb4eb7345778 100644 --- a/src/schedule/zephyr_dma_domain.c +++ b/src/schedule/zephyr_dma_domain.c @@ -56,6 +56,27 @@ struct zephyr_dma_domain_data { * associated channel was triggered. */ bool triggered; + /* used to keep track of how many pipeline tasks point + * to the same data structure. + */ + int usage_count; + /* used to keep track of how many times data->triggered + * can be left set to true before resetting it to false. + * + * this is needed for mixer topologies because the + * non-registrable pipeline tasks need to be executed + * when an IRQ comes for the registrable pipeline task. + */ + int pending_count; + /* used when linking a data structure to a non-registrable + * pipeline task. + * + * in the case of mixer, the non-registrable pipeline tasks + * have the same scheduling component as the registrable + * pipeline task so this can be used as a criteria to attach + * the data structure to the non-registrable pipeline task. + */ + struct comp_dev *sched_comp; }; struct zephyr_dma_domain_thread { @@ -219,6 +240,24 @@ static int register_dma_irq(struct zephyr_dma_domain *domain, if (core != crt_chan->core) continue; + if ((crt_data->sched_comp == pipe_task->sched_comp || + !crt_data->sched_comp) && !pipe_task->registrable) { + /* attach crt_data to current pipeline + * task only if there's a match between + * the scheduling components or i + * crt_data's sched_comp field wasn't + * set yet. + */ + pipe_task->priv = crt_data; + + crt_data->usage_count++; + crt_data->pending_count++; + + return 0; + } else if (!pipe_task->registrable) { + return 0; + } + /* skip if IRQ was already registered */ if (crt_data->enabled_irq) continue; @@ -230,14 +269,10 @@ static int register_dma_irq(struct zephyr_dma_domain *domain, crt_data->irq = irq; crt_data->channel = crt_chan; crt_data->dt = dt; + crt_data->sched_comp = pipe_task->sched_comp; + crt_data->usage_count++; + crt_data->pending_count++; - /* attach crt_data to pipeline task. - * - * this will be used in - * zephyr_dma_domain_is_pending to decide - * whether there was a DMA IRQ for the channel - * associated with this task. - */ pipe_task->priv = crt_data; if (dt->started) { @@ -285,16 +320,6 @@ static int zephyr_dma_domain_register(struct ll_schedule_domain *domain, tr_info(&ll_tr, "zephyr_dma_domain_register()"); - /* don't even bother trying to register DMA IRQ for - * non-registrable tasks. - * - * this is needed because zephyr_dma_domain_register() will - * return -EINVAL for non-registrable tasks because of - * register_dma_irq() which is not right. - */ - if (!pipe_task->registrable) - return 0; - /* the DMA IRQ has to be registered before the Zephyr thread is * started. * @@ -309,12 +334,16 @@ static int zephyr_dma_domain_register(struct ll_schedule_domain *domain, if (ret < 0) { tr_err(&ll_tr, - "failed to register DMA IRQ for pipe task %p on core %d", - pipe_task, core); + "failed to register DMA IRQ for pipe task %p on core %d reg: %d", + pipe_task, core, pipe_task->registrable); return ret; } + /* don't let non-registrable tasks go further */ + if (!pipe_task->registrable) + return 0; + /* skip if Zephyr thread was already started on this core */ if (dt->handler) return 0; @@ -453,15 +482,20 @@ static int zephyr_dma_domain_unregister(struct ll_schedule_domain *domain, struct zephyr_dma_domain *zephyr_dma_domain; struct zephyr_dma_domain_thread *dt; struct pipeline_task *pipe_task; + struct zephyr_dma_domain_data *data; int ret, core; zephyr_dma_domain = ll_sch_get_pdata(domain); pipe_task = pipeline_task_get(task); core = cpu_get_id(); dt = zephyr_dma_domain->domain_thread + core; + data = pipe_task->priv; tr_info(&ll_tr, "zephyr_dma_domain_unregister()"); + data->pending_count--; + data->usage_count--; + /* unregister the DMA IRQ only for PPL tasks marked as "registrable" * * this is done because, in case of mixer topologies there's @@ -532,9 +566,14 @@ static bool zephyr_dma_domain_is_pending(struct ll_schedule_domain *domain, * of the atomic area PLEASE make sure to disable IRQs before * calling it or disable IRQs here. */ - if (data->triggered) { - data->triggered = false; + data->pending_count--; + + if (!data->pending_count) { + data->triggered = false; + data->pending_count = data->usage_count; + } + return true; }