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

[Jaroslav Kysela <perex@xxxxxxxx>]

On 27. 05. 24 13:23, Takashi Iwai wrote:
> On Mon, 27 May 2024 09:11:33 +0200,
> 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@xxxxxxxxxx>
> > Cc: Shengjiu Wang <shengjiu.wang@xxxxxxxxx>
> > Cc: Nicolas Dufresne <nicolas@xxxxxxxxxxxx>
> > Cc: Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>
> > Cc: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> > Cc: Vinod Koul <vkoul@xxxxxxxxxx>
> > Signed-off-by: Jaroslav Kysela <perex@xxxxxxxx>
> > ---
> (snip)
>
> Through a quick glance, the general idea looks simple enough and
> straightforward.  If this satisfies the needs, we can go in this way.
>
> A few subtle things:
>
> - We might want to have a safe limit for the task creations.
>    An evil user-space can fill up too easily.

The limit was proposed in snd_compress_check_input() but not used later 
(TASK_CREATE ioctl). I will send v2 shortly.

> - The error checks of memdup_user() are missing.
>    A caveat is that memdup_user() returns ERR_PTR() instead of NULL.
>    So with the automatic cleanup, you'd need to fiddle with
>    PTR_ERR(no_free_ptr(p)).

Thanks.

> - The use of __u64 is rather for uapi headers.  The kernel internals
>    can be with straight u64.

Yes, thanks.

> - The task list add/removal might need some locks.  It looks racy.
>    Also the task runtime returned from snd_compr_find_task() might be
>    freed.  Need for some locking or refcounting?

All task operations are covered using stream->driver->lock unless I overlooked 
something.

> - A linear lookup is OK from performance POV?  Maybe enough for small
>    number of tasks.

We can improve this later, but I would not expect too many queued tasks. I set 
the limit to 64 for the first implementation.

> - Better to have a protocol version check and accept the R/W mode only
>    with the newer version.

There is no R/W mode for passthrough direction. An error should be returned. 
The direction is returned from the driver using SNDRV_COMPRESS_GET_CAPS and 
also open mode must be O_RDWR so things seem to be separated enough.

					Jaroslav

--
Jaroslav Kysela <perex@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.