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

[Laurent Pinchart]

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?

-- 
Regards,

Laurent Pinchart