[LaurentiuM1234]

We need to make the Zephyr DMA domain thread's priorty higher than the logging thread priority. If this is not done we may have some issues such as some buzzing noises appearing after resuming a paused stream. This was the case for i.MX93. Presumably this is because the SAI FIFO is underrun (please correct me if I'm wrong).

This fixes the issue on i.MX93 but will require some more tests to make sure that it doesn't break anything on the other i.MX boards which will use Zephyr DMA domain. This is why I added the [DNM] tag.

[paulstelian97]

Is there any central place where this define might be more appropriate? So as to be able to decide on ordering some things.

I like the idea behind the patch.

[iuliana-prodan]

@LaurentiuM1234 how did you decide on value 5?
How to you know the priority is less than logging?

[LaurentiuM1234]

@LaurentiuM1234 how did you decide on value 5?

I guess it does look kinda random. I chose this value based on the following criteria: the thread's priority needs to be higher than the logging thread's (which is either the lowest or some custom value set by the user) but lower than highest priority of the threads used by SOF (which if I'm not mistaken is 1: edf scheduler's thread).

How to you know the priority is less than logging?

If not set to some other value by the user, the logging thread's priority is the lowest: CONFIG_NUM_PREEMPT_PRIORITIES - 1 so this case is covered. In the case user does decide to change the logging thread's priority then this solution will not hold if he/she sets it to a value lower than 5. In this case I thought of 2 options:

1. Make ZEPHYR_DMA_DOMAIN_PRIO dependent on the logging thread's priority. Kinda like: ZEPHYR_DMA_DOMAIN_PRIO = LOGGING_THREAD_PRIO - 1. This might be problematic if the user sets LOGGING_THREAD_PRIO to highest priority possible (a negative value) (not sure why you'd do that) as ZEPHYR_DMA_DOMAIN_PRIO would go out of bounds so an additional check may be required. Also, this might cause problems with the other threads as the zephyr dma domain thread may have a higher priority than them in which case I can't guarantee this won't affect the flow.
2. Add some sanity checks. Kinda like:

if (ZEPHYR_DMA_DOMAIN_PRIO > LOGGING_THREAD_PRIO) {
    pr_warn("Some warning message here.");
}

Personally I prefer number 2.

Is there any central place where this define might be more appropriate? So as to be able to decide on ordering some things.

I like the idea behind the patch.

I think it would be best to keep the macro in zephyr_dma_domain.c as it's specific to it. It should be grouped with SEM_LIMIT and ZEPHYR_PDOMAIN_STACK_SIZE kinda like:

#define SEM_INIT 1
#define ZEPHYR_PDOMAIN_STACK_SIZE 8192
#define ZEPHYR_DMA_DOMAIN_THREAD_PRIO 5

I'll make sure to rectify this.

[lgirdwood]

Could this be based on a Zephyr Kconfig around the logging thread priorty ? e.g. have a Kconfig to make the Zephyr logging preemptive so that DMA pipeline preempts logging ?

[teburd]

You could do that if you know that it always needs to be one greater than logging for example. The logging process thread by default is set to the lowest preemptible priority if not explicitly configured (priority K_LOWEST_APPLICATION_THREAD_PRIO)

#define ZEPHYR_DMA_DOMAIN_THREAD_PRIO CONFIG_LOG_PROCESS_THREAD_PRIORTY-1

Otherwise the way I might go about it is exposing both to Kconfig and in the build Kconfig .prj file putting all the prio's nicely next to each other with some comments around it.

A BUILD_ASSERT() can be done with this as well to ensure that DMA_PRIO > LOG_PRIO at build time if you like.

BUILD_ASSERT(CONFIG_DMA_DOMAIN_THREAD_PRIO < CONFIG_LOG_PROCESS_THREAD_PRIORITY, "Some nice messaging here about why");

[LaurentiuM1234]

@teburd Thanks for the suggestion ! Should be a bit better now.

[lgirdwood]

@kv2019i @teburd any advice on the logging thread priority ?

[lgirdwood]

Could this be based on a Zephyr Kconfig around the logging thread priorty ? e.g. have a Kconfig to make the Zephyr logging preemptive so that DMA pipeline preempts logging ?

[LaurentiuM1234]

Thanks for all the feedback. Now the DMA domain's thread priority depends on the logging thread's priority. This way we won't have to use a hardcoded value which seems random. Also, it's important to note that the Zephyr DMA domain's thread priority doesn't necessarily need to be LOGGING_THREAD_PRIO - 1 as long as the criteria described in the introduced comment is met. We'll use this definition for cases where the user might want to set a custom priority to the logging thread. This way we can make sure that the Zephyr DMA domain thread has a higher priority than the logging thread.

@lgirdwood would it be problematic if the Zephyr DMA domain thread has a lower priority than edf_scheduler's thread priority? I did a small pause/resume test with Zephyr DMA domain thread priority = 0 and it seemed to work fine. If this wouldn't pose a problem I can remove this part of the comment:

preferably the Zephyr DMA domain's thread priority should be lower than the lowest priority
of the threads used by SOF which is 1 (EDF_SCHEDULER)

[sys-pt1s]

Can one of the admins verify this patch?

[dbaluta]

I don't feel comfortable linking the DMA domain thread priority with the DMA thread log priority. I would go the other way around.

define the DMA log thread priority based on the DMA domain thread priority and issue a build warning if the link cannot be done.

[teburd]

@dbaluta Is the log process thread priority being set? If not and its not the lowest priority then.. that seems like a different issue.

[LaurentiuM1234]

define the DMA log thread priority based on the DMA domain thread priority and issue a build warning if the link cannot be done.

Seems more problematic. I'm not sure there's a reason right now to set the logging thread's priority to something != CONFIG_NUM_PREEMPT_PRIORITES - 1 and setting it to CONFIG_NUM_PREEMPT_PRIORITES - 1 seems kinda redundant as it's already supposed to be set to that value if CONFIG_LOG_PROCESS_THREAD_CUSTOM_PRIORITY=n. At this point the DMA domain is pretty flexible regarding its thread's priority as the only critieria for it is to be higher than the logging thread's.

I don't feel comfortable linking the DMA domain thread priority with the DMA thread log priority. I would go the other way around.

Is there a particular reason for this? I wouldn't mind making the logging thread's priority depend on the DMA thread's priority or setting them both to some values in the board's .config but I'm not sure I get the problem with the current solution.

@dbaluta Is the log process thread priority being set? If not and its not the lowest priority then.. that seems like a different issue.

It's not

[LaurentiuM1234]

Marked this as a draft as the same test case used for testing Zephyr DMA domain fails.

[teburd]

@LaurentiuM1234 Are you able to run one of the tracing tools (e.g. tracealyzer, tracecompass)? Those are invaluable for understanding whats running when and this sounds like a problem that could be greatly helped by that perhaps?

[LaurentiuM1234]

Re-did the test and it seems to work fine. Reached about 368 rounds before stopping it manually. The issue because of which I marked this as a draft was probably caused by some mistake I've made during testing (e.g: not updating the .ri). The environment I've used is the following:

1. Linux kernel 6.1
2. SOF main branch + this PR applied.

@dbaluta Let me know if you want me to test something else. Otherwise, I can remove the [DNM] tag and we can merge this.

[dbaluta]

@lgirdwood this ready to be merged.

[lyakh]

why are you actually using such a low priority (a high priority number) for the DMA domain? With the Zephyr domain we use the highest available priority -CONFIG_NUM_COOP_PRIORITIES

[LaurentiuM1234]

It's mostly due to the fact that its priority being in the preemptive range hasn't caused us any issues that I'm aware of. I think we can leave this in the preemptive range and switch over to cooperative range if there's ever a reason to.

[kv2019i]

@wszypelt @lgirdwood This is not touching code used in Intel configurations, so overriding the Intel checks to merge this. This has been pending for a long time.