[PATCH] media: videobuf2-core.c: check if all buffers have the same number of planes

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Wed, Aug 16, 2023 at 02:47:33PM +0200, Hans Verkuil wrote:
> Currently if VIDIOC_CREATE_BUFS is called to add new buffers, then the requested
> number of planes per buffer might be different from the already allocated buffers.
> 
> However, this does not make a lot of sense and there are no drivers that allow
> for variable number of planes in the allocated buffers.
> 
> While it is possible do this today, when such a buffer is queued it will fail
> in the buf_prepare() callback where the plane sizes are checked if those are
> large enough. If fewer planes were allocated, then the size for the missing
> planes are 0 and the check will return -EINVAL.
> 
> But it is much better to do this check in VIDIOC_CREATE_BUFS.

I don't think this is a good idea. One important use case for
VIDIOC_CREATE_BUFS is to allocate buffers for a different format than
the one currently configured for the device, to prepare for a future
capture (or output) session with a different configuration. This patch
would prevent that.

> This patch adds the check to videobuf2-core.c
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco at xs4all.nl>
> ---
> With this patch the mediatek vcodec patch would no longer be needed:
> https://patchwork.linuxtv.org/project/linux-media/patch/20230810082333.972165-1-harperchen1110@gmail.com/
> ---
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index cf6727d9c81f..b88c08319bbe 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -938,6 +938,10 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  			dprintk(q, 1, "memory model mismatch\n");
>  			return -EINVAL;
>  		}
> +		if (requested_planes != q->num_planes) {
> +			dprintk(q, 1, "num_planes mismatch\n");
> +			return -EINVAL;
> +		}
>  		if (!verify_coherency_flags(q, non_coherent_mem))
>  			return -EINVAL;
>  	}
> @@ -1002,6 +1006,8 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  		mutex_unlock(&q->mmap_lock);
>  		return -ENOMEM;
>  	}
> +	if (no_previous_buffers)
> +		q->num_planes = num_planes;
>  	mutex_unlock(&q->mmap_lock);
> 
>  	/*
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 4b6a9d2ea372..799a285447b7 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -558,6 +558,7 @@ struct vb2_buf_ops {
>   * @dma_dir:	DMA mapping direction.
>   * @bufs:	videobuf2 buffer structures
>   * @num_buffers: number of allocated/used buffers
> + * @num_planes: number of planes in each buffer
>   * @queued_list: list of buffers currently queued from userspace
>   * @queued_count: number of buffers queued and ready for streaming.
>   * @owned_by_drv_count: number of buffers owned by the driver
> @@ -621,6 +622,7 @@ struct vb2_queue {
>  	enum dma_data_direction		dma_dir;
>  	struct vb2_buffer		*bufs[VB2_MAX_FRAME];
>  	unsigned int			num_buffers;
> +	unsigned int			num_planes;
> 
>  	struct list_head		queued_list;
>  	unsigned int			queued_count;

-- 
Regards,

Laurent Pinchart