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@xxxxxxxxx> > --- > With this patch the mediatek vcodec patch would no longer be needed: > https://patchwork.linuxtv.org/project/linux-media/patch/20230810082333.972165-1-harperchen1110@xxxxxxxxx/ > --- > 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
