| Message ID | e75ff985-2499-9a16-21fe-ee2e81547e6f@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: videobuf2-core.c: check if all buffers have the same number of planes | expand |
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@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;
Hi Hans, Laurent, On Wed, Aug 16, 2023 at 05:34:32PM +0300, Laurent Pinchart wrote: > 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. I'd prefer to keep this capability in videobuf2, too. Although... one way achieve that could be to add a flag (or an integer field) in struct vb2_queue to tell vb2 core that the driver wants to do the num_planes checks by itself. It'd be also nicer, considering the end result, to configure this when setting up the queue, rather than based on the first buffer created. This would involve changing a large number of drivers though.
On 16/08/2023 19:48, Sakari Ailus wrote: > Hi Hans, Laurent, > > On Wed, Aug 16, 2023 at 05:34:32PM +0300, Laurent Pinchart wrote: >> 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. > > I'd prefer to keep this capability in videobuf2, too. Although... one way > achieve that could be to add a flag (or an integer field) in struct > vb2_queue to tell vb2 core that the driver wants to do the num_planes > checks by itself. Having a flag for this was something I intended to do once we have a driver that actually supports this feature. I'm not aware of any driver that needs this today. But I'll make a v2 adding this flag, it is simple to do and doesn't hurt. > > It'd be also nicer, considering the end result, to configure this when > setting up the queue, rather than based on the first buffer created. This > would involve changing a large number of drivers though. > "to configure this": what is "this"? The new flag? Or the number of planes? And with "setting up the queue" do you mean the queue_setup callback or when the vb2_queue is initialized? Regards, Hans
Hello, On Thu, Aug 17, 2023 at 09:11:36AM +0200, Hans Verkuil wrote: > On 16/08/2023 19:48, Sakari Ailus wrote: > > On Wed, Aug 16, 2023 at 05:34:32PM +0300, Laurent Pinchart wrote: > >> 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. > > > > I'd prefer to keep this capability in videobuf2, too. Although... one way > > achieve that could be to add a flag (or an integer field) in struct > > vb2_queue to tell vb2 core that the driver wants to do the num_planes > > checks by itself. > > Having a flag for this was something I intended to do once we have a > driver that actually supports this feature. I'm not aware of any driver > that needs this today. But I'll make a v2 adding this flag, it is simple > to do and doesn't hurt. I don't think it should be a driver decision, as it's a question of userspace usage. We shouldn't couple buffer allocation and buffer usage, VIDIOC_CREATE_BUFS needs to allow allocation of any buffer suitable for the hardware *in any configuration*, not just the active configuration. It's only at buffer prepare time that the fitness of a particular buffer for the active configuration of the device can be checked. > > It'd be also nicer, considering the end result, to configure this when > > setting up the queue, rather than based on the first buffer created. This > > would involve changing a large number of drivers though. > > "to configure this": what is "this"? The new flag? Or the number of planes? > And with "setting up the queue" do you mean the queue_setup callback or > when the vb2_queue is initialized?
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;
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. This patch adds the check to videobuf2-core.c Signed-off-by: Hans Verkuil <hverkuil-cisco@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/ ---