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

[Takashi Iwai <tiwai@suse.de>]

On Fri, 21 Jun 2024 18:49:15 +0200,
Jaroslav Kysela wrote:
>  /**
>   * 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;

Those new fields deserve for some more comments (at least mentioning
that those are for passthrough operations).

> --- a/sound/core/Kconfig
> +++ b/sound/core/Kconfig
> @@ -59,6 +59,10 @@ config SND_CORE_TEST
>  config SND_COMPRESS_OFFLOAD
>  	tristate
>  
> +config SND_COMPRESS_PASSTHROUGH
> +	select DMA_BUF
> +	tristate

I suppose this is a boolean?

> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
(snip)
> @@ -85,6 +92,8 @@ static int snd_compr_open(struct inode *inode, struct file *f)
>  		dirn = SND_COMPRESS_PLAYBACK;
>  	else if ((f->f_flags & O_ACCMODE) == O_RDONLY)
>  		dirn = SND_COMPRESS_CAPTURE;
> +	else if ((f->f_flags & O_ACCMODE) == O_RDWR)
> +		dirn = SND_COMPRESS_PASSTHROUGH;
>  	else
>  		return -EINVAL;

Shouldn't it be safer to give an error for a RW access unless the
driver supports the passthrough mode?

> @@ -939,6 +988,262 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
>  	return snd_compress_wait_for_drain(stream);
>  }
>  
> +#if IS_ENABLED(CONFIG_SND_COMPRESS_PASSTHROUGH)
> +
> +static struct snd_compr_task_runtime *
> +		snd_compr_find_task(struct snd_compr_stream *stream, __u64 seqno)

Please fix indentation.

> +void snd_compr_task_finished(struct snd_compr_stream *stream,
> +			    struct snd_compr_task_runtime *task)
> +{
> +	guard(mutex)(&stream->device->lock);
> +	if (!snd_BUG_ON(stream->runtime->active_tasks == 0))
> +		stream->runtime->active_tasks--;
> +	task->state = SND_COMPRESS_TASK_STATE_FINISHED;
> +	wake_up(&stream->runtime->sleep);
> +}
> +EXPORT_SYMBOL(snd_compr_task_finished);

Let's use EXPORT_SYMBOL_GPL() for a new function.


thanks,

Takashi