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;
> +}
[Index of Archives]
[Pulseaudio]
[Linux Audio Users]
[ALSA Devel]
[Fedora Desktop]
[Fedora SELinux]
[Big List of Linux Books]
[Yosemite News]
[KDE Users]
