pipeline 2.0: Introducing Data Processing Queue

[marcinszkudlinski]

re-inroducing DP queue. This PR contains DpQueue code only

dependencies:

• github.com/DP: changes in sink/source #8002
• Remove sparse cache annotations from buffer/audio_stream step2 #8167
• PR Remove sparse step1 #8166

some sparse warning are reported now till RFC changes are implemented

DP queue is a lockless circular buffer
providing safe consumer/producer cached operations cross cores

prerequisites:

1. incoming and outgoing data rate MUST be the same
2. Both data consumer and data producer declare
   max chunk sizes they want to use (IBS/OBS)

required Buffer size:
- 2MAX(IBS,OBS) if the larger of IBS/OBS
is multiplication of smaller
- 3MAX(IBS,OBS) otherwise

The queue may work in 2 modes

1. local mode
   in case both receiver and sender are located on
   the same core and cache coherency
   does not matter. dp_queue structure is located in cached memory
   In this case DP Queue is a simple ring buffer

2. shared mode
   In this case we need to writeback cache when new data
   arrive and invalidate cache on secondary core.
   dp_queue structure is located in shared memory

dpQueue is a lockless consumer/producer safe buffer.
It is achieved by having only 2 shared variables:

write_offset - can be modified by data producer only
read_offset - can be modified by data consumer only

as 32 bit operations are atomic, it is multi-thread and multi-core save

There some explanation needed how free_space and
available_data are calculated

number of avail data in circular buffer may be calculated as:
data_avail = write_offset - read_offset
and check for wrap around
if (data_avail < 0) data_avail = buffer_size + data_avail

The problem is when write_offset == read_offset,
!!! it may mean either that the buffer is empty
or the buffer is completely filled !!!

To solve the above issue having only 2 variables mentioned before:

• allow both offsets to point from 0 to DOUBLE buffer_size
• when calculating pointers to data, use:
  data_bufer[offset % buffer_size]
• use double buffer size in wrap around check when calculating
  available data

And now:

• write_offset == read_offset
  always means "buffer empty"

• write_offset == read_offset + buffer_size
  always means "buffer full"

• data_avail = write_offset - read_offset
  if (data_avail < 0) data_avail = 2 * buffer_size + data_avail

[lgirdwood]

Overall I think this looks good, but we do need to prioritze MCPS over partitioning of code between private and public API calls.

[kv2019i]

Looks good to me. The issue with spinlock with non-shared use needs to fixed, but otherwise seems good to go.

[marcinszkudlinski]

@lgirdwood
Probably a lock like below will need to be used in the system several times. Maybe it is a good idea to export it to a library?

union lock_key {
	k_spinlock_key_t spinlock;
	int irq_lock;
};

static inline union lock_key dp_queue_lock(struct dp_queue *dp_queue)
{
	/* use cross-core spinlock in case of shared queue
	 * When shared, dp_queue structure is located in not cached memory
	 * as required by spinlock
	 */
	union lock_key key;

	if (dp_queue_is_shared(dp_queue))
		key.spinlock = k_spin_lock(&dp_queue->lock);
	else
		/* use faster irq_lock in case of not shared queue (located in cached mem) */
		key.irq_lock = irq_lock();

	return key;
}

static inline void dp_queue_unlock(struct dp_queue *dp_queue, union lock_key key)
{
	if (dp_queue_is_shared(dp_queue))
		k_spin_unlock(&dp_queue->lock, key.spinlock);
	else
		irq_unlock(key.irq_lock);
}

[kv2019i]

A comment w.r.t. locks inline, but not blocking this PR. My immediate concerns addressed, so +1 to this.

[kv2019i]

This goes a bit out of the scope of this PR, but given we have very little use of locking in sof/src/audio (mostly in the DAI code, chain-dma and kpb), this is worth a discussion.

Given we might want to run some of the audio modules in unprivileged code, having code that uses spinlocks or irqlocks is not really future proof. So I wonder if we should just move to mutexes in places like this, which would be available both for user-space and kernel threads.

This does incur a perf penalty, and might cause problems as we still have non-Zephyr SOF users who run audio components in IRQ context, so this is probably not something we can do now, but I'd say this should be the longterm solution for audio code.

[lgirdwood]

@marcinszkudlinski I'm seeing a CI build error with Zephyr

ccache /home/cwd_user/zephyr-sdk-0.16.1/xtensa-intel_ace15_mtpm_zephyr-elf/bin/xtensa-intel_ace15_mtpm_zephyr-elf-gcc -DCC_OPTIMIZE_FLAGS=\"-O2\" -DKERNEL -DXCC_TOOLS_VERSION=\"zephyr\" -D__ZEPHYR__=1 -DRELATIVE_FILE=\"../sof/src/audio/dp_queue.c\" -I/zep_workspace/zephyr/include -I/zep_workspace/build-mtl/zephyr/include/generated -I/zep_workspace/zephyr/soc/xtensa/intel_adsp/ace -I/zep_workspace/zephyr/soc/xtensa/intel_adsp/ace/include -I/zep_workspace/zephyr/soc/xtensa/intel_adsp/ace/include/intel_ace15_mtpm -I/zep_workspace/zephyr/soc/xtensa/intel_adsp/common/include -I/zep_workspace/zephyr/drivers -I/zep_workspace/sof/zephyr/include -I/zep_workspace/sof/src/platform/intel/ace/include -I/zep_workspace/sof/src/platform/meteorlake/include -I/zep_workspace/sof/src/include/sof/audio/module_adapter/iadk -I/zep_workspace/sof/src/include/sof/audio/module_adapter/library -I/zep_workspace/modules/hal/xtensa/include -I/zep_workspace/modules/hal/xtensa/XTENSA_HAL -I/zep_workspace/modules/hal/xtensa/zephyr/soc/intel_ace15_mtpm -I/zep_workspace/sof/rimage/src/include -I/zep_workspace/sof/src/include -I/zep_workspace/sof/src/arch/xtensa/include -I/zep_workspace/sof/third_party/include -I/zep_workspace/sof/xtos/include -isystem /zep_workspace/zephyr/lib/libc/minimal/include -isystem /opt/toolchains/zephyr-sdk-0.16.1/xtensa-intel_ace15_mtpm_zephyr-elf/bin/../lib/gcc/xtensa-intel_ace15_mtpm_zephyr-elf/12.2.0/include -isystem /opt/toolchains/zephyr-sdk-0.16.1/xtensa-intel_ace15_mtpm_zephyr-elf/bin/../lib/gcc/xtensa-intel_ace15_mtpm_zephyr-elf/12.2.0/include-fixed -fno-strict-aliasing -O2 -imacros /zep_workspace/build-mtl/zephyr/include/generated/autoconf.h -ffreestanding -fno-common -g -gdwarf-4 -fdiagnostics-color=always --sysroot=/home/cwd_user/zephyr-sdk-0.16.1/xtensa-intel_ace15_mtpm_zephyr-elf/xtensa-intel_ace15_mtpm_zephyr-elf -imacros /zep_workspace/zephyr/include/zephyr/toolchain/zephyr_stdint.h -Wall -Wformat -Wformat-security -Wno-format-zero-length -Wno-pointer-sign -Wpointer-arith -Wexpansion-to-defined -Wno-unused-but-set-variable -Werror=implicit-int -fno-pic -fno-pie -Werror -fno-asynchronous-unwind-tables -fstrict-overflow -fno-reorder-functions --param=min-pagesize=0 -fno-defer-pop -fmacro-prefix-map=/zep_workspace/sof/app=CMAKE_SOURCE_DIR -fmacro-prefix-map=/zep_workspace/zephyr=ZEPHYR_BASE -fmacro-prefix-map=/zep_workspace=WEST_TOPDIR -ffunction-sections -fdata-sections -mlongcalls -DXTENSA_TOOLCHAIN_VARIANT=1 -std=c99 -nostdinc -fno-inline-functions -std=gnu99 -MD -MT modules/sof/CMakeFiles/modules_sof.dir/zep_workspace/sof/src/audio/dp_queue.c.obj -MF modules/sof/CMakeFiles/modules_sof.dir/zep_workspace/sof/src/audio/dp_queue.c.obj.d -o modules/sof/CMakeFiles/modules_sof.dir/zep_workspace/sof/src/audio/dp_queue.c.obj -c /zep_workspace/sof/src/audio/dp_queue.c
/zep_workspace/sof/src/audio/dp_queue.c: In function 'dp_queue_create':
/zep_workspace/sof/src/audio/dp_queue.c:344:9: error: implicit declaration of function 'sink_set_obs'; did you mean 'sink_set_rate'? [-Werror=implicit-function-declaration]
  344 |         sink_set_obs(&dp_queue->sink_api, ibs);
      |         ^~~~~~~~~~~~
      |         sink_set_rate
/zep_workspace/sof/src/audio/dp_queue.c:345:9: error: implicit declaration of function 'source_set_ibs'; did you mean 'source_set_rate'? [-Werror=implicit-function-declaration]
  345 |         source_set_ibs(&dp_queue->source_api, obs);
      |         ^~~~~~~~~~~~~~
      |         source_set_rate
cc1: all warnings being treated as errors
[227/378] Building C object modules/sof/CMakeFiles/modules_sof.dir/lib/alloc.c.obj
[228/378] Building C object modules/sof/CMakeFiles/modules_sof.dir/lib/cpu.c.obj

[marcinszkudlinski]

github.com/#8002
dependency

[marcinszkudlinski]

rebased after #8002 merge

[kv2019i]

@marcinszkudlinski Is #8006 still a hard dependency (like in description), or is this ok to merge once reviews-and-testing is ok?

[marcinszkudlinski]

All requested changes addressed

1. inline functions for performance
2. rebased on [DNM] Direct use of cached / not-cached shared memory alias instead of coherent.h in case of buffers #8106 to avoid rebase problems

PLEASE SKIP 1st COMMIT IN REVIEW

[RanderWang]

LGTM

[marcinszkudlinski]

all deps resolved, rebased on newest main

[lgirdwood]

all deps resolved, rebased on newest main

@marcinszkudlinski Can you check in the CI results. Thanks !

[marcinszkudlinski]

all deps resolved, rebased on newest main

@marcinszkudlinski Can you check in the CI results. Thanks !

looks like there's no results:
https://sof-ci.01.org/sof-pr-viewer/#/build/PR8020/build12764971

pushing an empty commit (rebase to newest main) to retrigger

[kv2019i]

One extra merge-conflict bit in the commit message, can you clean up @marcinszkudlinski . Otherwise I can re-confirm my +1 for this.

[marcinszkudlinski]

all green

[lyakh]

I still don't understand this calculation...

[marcinszkudlinski]

In fact I should reconsider 3x as it was estimated in first implementation when data needed to be passed in chunks
of cacheline size.
Now I went trough the same example I was analyzing before and probably 3* is not needed anymore

Will double check it and compare with reference FW and probably remove in next commit

remainint comments can be addressed in a follow-up

[kv2019i]

CI is good. Zephyr mainline CIs fail due to zephyrproject-rtos/zephyr#62464

@marcinszkudlinski @lyakh let's address the size_t/int variables, and if needed, clarify the rationale for 2/3x buffer sizing, in a follow-up PR. Thanks all for reviews!