Skip to content

ipc_send_queued_msg() has a hard dependency on the EDF scheduler #4356

Description

@marc-hb

I found this IPC scheduling and dependency issue while troubleshooting DMA traces on Zephyr but I suspect it has consequences even without Zephyr. In fact I wonder whether it could maybe explain the very long standing #4333 "lag" in DMA traces too.

In Zephyr the problem is simple (once you know it!): no (legacy) EDF scheduler -> task_main_primary_core does not run -> no IPC TX flush -> no DMA traces.

enum task_state task_main_primary_core(void *data)
{
	/* main audio processing loop */
	while (1) {
		/* sleep until next IPC or DMA */
		wait_for_interrupt(0);

		if (!ipc->pm_prepare_D3)
			ipc_send_queued_msg();

	}
}

I tried to schedule task_main_primary_core() in Zephyr too but I hit the following circular dependency with the EDF scheduler where the EDF scheduler wants to initialize the main task in the middle of its own initialization and task_main_init() needs the half-initialized EDF scheduler to schedule itself.

int scheduler_init_edf(void)
{
	edf_sch = rzalloc(SOF_MEM_ZONE_SYS, 0, SOF_MEM_CAPS_RAM,
			  sizeof(*edf_sch));

	scheduler_init(SOF_SCHEDULE_EDF, &schedule_edf_ops, edf_sch);

	/* initialize AND SCHEDULE main task context before enabling interrupt */
	task_main_init();

	interrupt_enable(edf_sch->irq, edf_sch);
}

Here's one super-ugly and probably unreliable workaround. Warning: many other code changes are required to get DMA traces with Zephyr.

--- a/src/trace/dma-trace.c
+++ b/src/trace/dma-trace.c
@@ -379,7 +379,7 @@ static int dma_trace_get_avail_data(struct dma_trace_data *d,
 	 * if no new trace is filled.
 	 */
 	if (d->old_host_offset != d->posn.host_offset) {
-		ipc_msg_send(d->msg, &d->posn, false);
+		ipc_msg_send(d->msg, &d->posn, true /* try to send immediately */ );
 

Even in plain SOF, this does not "feel right" and as I wrote above I wonder whether it could explain the very regular DMA traces "lag" exposed in #4333

The circular dependency and all the code above is as old as the entire scheduler itself: Aug 2019 20acc16 in giant PR #1710
There was a small ipc_send_queued_msg() refactor in Feb 2020 commit 9e0b2e5 but I don't think it changed the logic.

Activity

  1. marc-hb commented on Jun 15, 2021

    @marc-hb
    CollaboratorAuthor

    Thanks @kv2019i and @lyakh for trying to solve this issue offline, can you please share the "best bits" here?

    cc: @lgirdwood , @andyross , @plbossart and @nashif

  2. kv2019i commented on Jun 17, 2021

    @kv2019i
    Collaborator

    @marc-hb I think we should use the Zephyr ktimer API (like used in the ll-scheduler in zephyr_domain.c) to create a repeating task to flush out the IPC send queue. This could be registered from IPC code if needed (DMA traces or position updates requested).

  3. lgirdwood commented on Jun 17, 2021

    @lgirdwood
    Member

    @marc-hb lets do as @kv2019i says, we either DMA copy the buffer to host on

    1. Buffer free space < some limit - we can then wake the DMA thread
    2. Periodic empty - lets say every second we wake the DMA thread.

    We have the logic to determine if conditions 1 or 2 are true today, but do not have the thread today.

    The thread should be similar to

    dma_trace_copy_thread() {
    
        /* sleep until we are woken */
        sleep();
    
       /* something woke us - copy the available data */
       dma_copy_data_to_host();
    }

    To really simplify the logic for 2 (and reusing the LL timer) I would attach a counter to the LL thread as the dma_trace period counter and wake the thread from here (instead of creating a separate timer).

  4. lyakh commented on Jun 17, 2021

    @lyakh
    Collaborator

    @marc-hb I think we should use the Zephyr ktimer API (like used in the ll-scheduler in zephyr_domain.c) to create a repeating task to flush out the IPC send queue. This could be registered from IPC code if needed (DMA traces or position updates requested).

    @kv2019i k_timer directly? That runs in the interrupt context...

  5. kv2019i commented on Jun 17, 2021

    @kv2019i
    Collaborator

    @lyakh a similar k_thread_create woken up by the timer IRQ as in zephyr_domain.

  6. kv2019i commented on Jun 17, 2021

    @kv2019i
    Collaborator

    And a plain k_thread which sleeps, flushes IPCs and sleeps again, would do as well.

  7. lgirdwood commented on Jun 17, 2021

    @lgirdwood
    Member

    @lyakh can you link your existing kthread code as an example for @marc-hb

  8. marc-hb commented on Jun 17, 2021

    @marc-hb
    CollaboratorAuthor

    @marc-hb lets do as @kv2019i says, we either DMA copy the buffer to host on

    Buffer free space < some limit - we can then wake the DMA thread
    Periodic empty - lets say every second we wake the DMA thread.

    I think the trace_work() DMA thread already does that and it even runs in Zephyr already. However it merely adds the posn update IPC to a (shared?) IPC queue and it's only that IPC queue that does not get flushed (yet) in Zephyr.

  9. marc-hb commented on Jul 1, 2021

    @marc-hb
    CollaboratorAuthor

    #4428 addressed this differently and temporarily

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions