On 24. 06. 24 9:13, Pierre-Louis Bossart wrote:
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.
It can be increased later when required. I think that 64 is good value for
start, but it's not a limit which is set to a stone.
+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.
I agree. I will document this in next version.
+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?
Yes.
If yes, is any locking required?
There's always stream mutex (stream->device->lock) held before any callback
(operation) is called.
+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?
The driver MUST remove this job from the queue.
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.
Yes, I see your point. Probably the current code should call stop in reverse
for "STOP ALL" or "FREE ALL" ioctls or in the file descriptor release callback
to avoid "jump to next active job" in the driver for a short moment.
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?
This I/O mechanism tries to be "universal". As opposite to the standard
streaming APIs, those tasks may be individual (without any state handling
among multiple tasks). In this case, the "stop" in the middle makes sense.
Also, it may make sense for real-time operation (remove altered/old data and
feed new).
/**
* 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.
'struct snd_compr_stream' is passed to all callbacks, so direction is known.
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?
Yes. I will fix that.
/*
* 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....
The real output size is known after the operation is finished not before. The
maximal output size is determined by the driver based on the maximal input
buffer size.
+ __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?
Is this really useful? The application already knows what was passed to kernel.
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.
Called inside the stream mutex.
Jaroslav
--
Jaroslav Kysela <perex@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
[Index of Archives]
[Pulseaudio]
[Linux Audio Users]
[ALSA Devel]
[Fedora Desktop]
[Fedora SELinux]
[Big List of Linux Books]
[Yosemite News]
[KDE Users]
