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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

Thanks Jaroslav, couple of questions below:

> +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).

if (stream->direction == SND_COMPRESS_PASSTHROUGH)
	max_fragments = 64;			/* safe value */

Is there anything preventing us from increasing this if there was a need
for it? Wondering if this would be an ABI restriction or just an
internal safeguard userspace doesn't need to know.

> +
> +State Machine
> +=============
> +
> +The passthrough audio stream state machine is described below :
> +
> +                                       +----------+
> +                                       |          |
> +                                       |   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.

The code adds the tasks in the order in which they are created:

	list_add_tail(&task->list, &stream->runtime->tasks);

This should probably be documented, there's no explicit mechanism to
chain the tasks other than the order of creation.

> +FREE
> +----
> +Free a set of input/output buffers. If a task is active, the stop
> +operation is executed before. If seqno is zero, operation is executed for all
> +tasks.

Can a task in the middle of the list be freed?

If yes, is any locking required?

> +START
> +-----
> +Starts (queues) a task. There are two cases of the task start - right after
> +the task is created. In this case, origin_seqno must be zero.
> +The second case is for reusing of already finished task. The origin_seqno
> +must identify the task to be reused. In both cases, a new seqno value
> +is allocated and returned to user space.
> +
> +The prerequisite is that application filled input dma buffer with
> +new source data and set input_size to pass the real data size to the driver.
> +
> +The order of data processing is preserved (first started job must be
> +finished at first).
> +
> +STOP
> +----
> +Stop (dequeues) a task. If seqno is zero, operation is executed for all
> +tasks.

What happens if a STOP is sent to a task in the middle of the list?


It's similar to the question on free above, the creation needs to follow
an order but the free/stop can be individual tasks so there could be
interesting state machine transition and programming errors.

I honestly find the state machine confusing, it looks like in the SETUP
stage tasks can be added/removed dynamically, but I am not sure if it's
a real use case? Most pipeline management add a bunch of processing,
then go in the 'run' mode. Adding/removing stuff on a running pipeline
is really painful and not super useful, is it?

>  /**
>   * struct snd_compr_runtime: runtime stream description
>   * @state: stream state
> @@ -54,6 +70,11 @@ struct snd_compr_runtime {
>  	dma_addr_t dma_addr;
>  	size_t dma_bytes;
>  	struct snd_dma_buffer *dma_buffer_p;
> +
> +	u32 active_tasks;
> +	u32 total_tasks;
> +	u64 task_seqno;
> +	struct list_head tasks;
>  };

should there be some sort of identifier that says the stream in used in
passthrough mode and only then are the 4 added members relevant?


'struct snd_compr_runtime' doesn't have a notion of direction, so
there's no real way to know what set_params() requested.

> diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
> index d185957f3fe0..5fed1979522b 100644
> --- a/include/uapi/sound/compress_offload.h
> +++ b/include/uapi/sound/compress_offload.h
> @@ -1,4 +1,4 @@
> -/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +	/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */

spurious change?

>  /*
>   *  compress_offload.h - compress offload header definations
>   *
> @@ -14,7 +14,7 @@

> +/**
> + * struct snd_compr_task - task primitive for non-realtime operation
> + * @seqno: sequence number (task identifier)
> + * @origin_seqno: previous sequence number (task identifier) - for reuse
> + * @input_fd: data input file descriptor (dma-buf)
> + * @output_fd: data output file descriptor (dma-buf)
> + * @input_size: filled data in bytes (from caller, must not exceed fragment size)
> + */
> +struct snd_compr_task {
> +	__u64 seqno;
> +	__u64 origin_seqno;
> +	int input_fd;
> +	int output_fd;
> +	__u64 input_size;

Any reason why output_size is not listed here....

> +	__u8 reserved[16];
> +} __attribute__((packed, aligned(4)));
> +
> +enum snd_compr_state {
> +	SND_COMPRESS_TASK_STATE_IDLE = 0,
> +	SND_COMPRESS_TASK_STATE_ACTIVE,
> +	SND_COMPRESS_TASK_STATE_FINISHED
> +};
> +
> +/**
> + * struct snd_compr_task_status - task status
> + * @seqno: sequence number (task identifier)
> + * @output_size: filled data in bytes (from driver)
> + * @state: actual task state (SND_COMPRESS_TASK_STATE_*)
> + */
> +struct snd_compr_task_status {
> +	__u64 seqno;
> +	__u64 output_size;

... but it's listed here.

It'd be worth explaining why the input and output are in different
structures. I can understand that for the configuration only the host
can provide data in the input, but in a status it'd be good to have a
snapshot of the two variables, no?


> +	__u8 state;
> +	__u8 reserved[15];
> +} __attribute__((packed, aligned(4)));

> +static int snd_compr_task_new(struct snd_compr_stream *stream, struct snd_compr_task *utask)
> +{
> +	struct snd_compr_task_runtime *task;
> +	int retval;
> +
> +	if (stream->runtime->total_tasks >= stream->runtime->fragments)
> +		return -EBUSY;
> +	if (utask->origin_seqno != 0 || utask->input_size != 0)
> +		return -EINVAL;
> +	task = kzalloc(sizeof(*task), GFP_KERNEL);
> +	if (task == NULL)
> +		return -ENOMEM;
> +	task->seqno = utask->seqno = snd_compr_seqno_next(stream);
> +	task->input_size = utask->input_size;
> +	retval = stream->ops->task_create(stream, task);
> +	if (retval < 0)
> +		goto cleanup;
> +	utask->input_fd = dma_buf_fd(task->input, O_WRONLY|O_CLOEXEC);
> +	if (utask->input_fd < 0) {
> +		retval = utask->input_fd;
> +		goto cleanup;
> +	}
> +	utask->output_fd = dma_buf_fd(task->output, O_RDONLY|O_CLOEXEC);
> +	if (utask->output_fd < 0) {
> +		retval = utask->output_fd;
> +		goto cleanup;
> +	}
> +	/* keep dmabuf reference until freed with task free ioctl */
> +	dma_buf_get(utask->input_fd);
> +	dma_buf_get(utask->output_fd);
> +	list_add_tail(&task->list, &stream->runtime->tasks);
> +	stream->runtime->total_tasks++;

no locking for these two lines? If there's already a lock handled by the
ALSA/ASoC/compressed frameworks, it'd be worth explaining which one is
assumed to be held.

> +	return 0;
> +cleanup:
> +	snd_compr_task_free(task);
> +	return retval;
> +}