Re: [RFC PATCH] ALSA: compress_offload: introduce passthrough operation mode

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

On 5/27/2024 9:11 AM, Jaroslav Kysela wrote:
> There is a requirement to expose the audio hardware that accelerates various
> tasks for user space such as sample rate converters, compressed
> stream decoders, etc.
> 
> This is description for the API extension for the compress ALSA API which
> is able to handle "tasks" that are not bound to real-time operations
> and allows for the serialization of operations.
> 
> For details, refer to "compress-passthrough.rst" document.
> 
> Note: This code is RFC (not tested, just to clearify the API requirements).
> My goal is to add a test (loopback) driver and add a support to tinycompress
> library in the next step.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Shengjiu Wang <shengjiu.wang@gmail.com>
> Cc: Nicolas Dufresne <nicolas@ndufresne.ca>
> Cc: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> ---

Seems mostly ok to me, some nitpicks below.

>   .../sound/designs/compress-passthrough.rst    | 125 +++++++
>   include/sound/compress_driver.h               |  32 ++
>   include/uapi/sound/compress_offload.h         |  44 ++-
>   sound/core/Kconfig                            |   4 +
>   sound/core/compress_offload.c                 | 314 +++++++++++++++++-
>   5 files changed, 511 insertions(+), 8 deletions(-)
>   create mode 100644 Documentation/sound/designs/compress-passthrough.rst
> 
> diff --git a/Documentation/sound/designs/compress-passthrough.rst b/Documentation/sound/designs/compress-passthrough.rst
> new file mode 100644
> index 000000000000..bb5a0ae5c496
> --- /dev/null
> +++ b/Documentation/sound/designs/compress-passthrough.rst
> @@ -0,0 +1,125 @@
> +=================================
> +ALSA Co-processor Passthrough API
> +=================================
> +
> +Jaroslav Kysela <perex@perex.cz>
> +
> +
> +Overview
> +========
> +
> +There is a requirement to expose the audio hardware that accelerates various
> +tasks for user space such as sample rate converters, compressed
> +stream decoders, etc.
> +
> +This is description for the API extension for the compress ALSA API which
> +is able to handle "tasks" that are not bound to real-time operations
> +and allows for the serialization of operations.
> +
> +Requirements
> +============
> +
> +The main requirements are:
> +
> +- serialization of multiple tasks for user space to allow multiple
> +  operations without user space intervention
> +
> +- separate buffers (input + output) for each operation
> +
> +- expose buffers using mmap to user space
> +
> +- signal user space when the task is finished (standard poll mechanism)
> +
> +Design
> +======
> +
> +A new direction SND_COMPRESS_PASSTHROUGH is introduced to identify
> +the passthrough API.
> +
> +The API extension shares device enumeration and parameters handling from
> +the main compressed API. All other realtime streaming ioctls are deactivated
> +and a new set of task related ioctls are introduced. The standard
> +read/write/mmap I/O operations are not supported in the passtrough device.

passthrough

> +
> +Device ("stream") state handling is reduced to OPEN/SETUP. All other
> +states are not available for the passthrough mode.
> +
> +Data I/O mechanism is using standard dma-buf interface with all advantages
> +like mmap, standard I/O, buffer sharing etc. One buffer is used for the
> +input data and second (separate) buffer is used for the ouput data. Each task

output

> +have separate I/O buffers.
> +
> +For the buffering parameters, the fragments means a limit of allocated tasks
> +for given device. The fragment_size limits the input buffer size for the given
> +device. The output buffer size is determined by the driver (may be different
> +from the input buffer size).
> +
> +State Machine
> +=============
> +
> +The passtrough audio stream state machine is described below :

passthrough

> +
> +                                       +----------+
> +                                       |          |
> +                                       |   OPEN   |
> +                                       |          |
> +                                       +----------+
> +                                             |
> +                                             |
> +                                             | compr_set_params()
> +                                             |
> +                                             v
> +         all passthrough task ops      +----------+
> +  +------------------------------------|          |
> +  |                                    |   SETUP  |
> +  |                                    |
> +  |                                    +----------+
> +  |                                          |
> +  +------------------------------------------+
> +
> +
> +Passthrough operations (ioctls)
> +===============================
> +
> +CREATE
> +------
> +Creates a set of input/output buffers. The input buffer size is
> +fragment_size. Allocates unique seqno.
> +
> +The hardware drivers allocate internal 'struct dma_buf' for both input and
> +output buffers (using 'dma_buf_export()' function). The anonymous
> +file descriptors for those buffers are passed to user space.
> +
> +FREE
> +----
> +Free a set of input/output buffers. If an task is active, the stop

_a_ task?


> +
> +static int snd_compr_task_start(struct snd_compr_stream *stream, struct snd_compr_task *utask)
> +{
> +	struct snd_compr_task_runtime *task;
> +	int retval;
> +
> +	if (utask->origin_seqno > 0) {
> +		task = snd_compr_find_task(stream, utask->seqno);
> +		retval = snd_compr_task_start_prepare(task, utask);
> +		if (retval < 0)
> +			return retval;
> +		task->seqno = utask->seqno = snd_compr_seqno_next(stream);
> +		utask->origin_seqno = 0;
> +		list_move_tail(&task->list, &stream->runtime->tasks);
> +	} else {
> +		task = snd_compr_find_task(stream, utask->seqno);
> +		if (task && task->finished)
> +			return -EBUSY;
> +		retval = snd_compr_task_start_prepare(task, utask);
> +		if (retval < 0)
> +			return retval;
> +	}
> +	retval = stream->ops->task_start(stream, task);

Should probably check somewhere that ops are set properly, or is it this 
way because they all considered mandatory?


> +
> +static int snd_compr_task_status(struct snd_compr_stream *stream,
> +					struct snd_compr_task_status *status)
> +{
> +	struct snd_compr_task_runtime *task;
> +
> +	task = snd_compr_find_task(stream, status->seqno);
> +	if (task == NULL)
> +		return -EINVAL;
> +	status->output_size = task->output_size;
> +	status->active = task->active ? 1 : 0;
> +	status->finished = task->finished ? 1 : 0;

Not sure, but access to ->active and ->finished, feels to me like 
something that may require locking.