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

[Hans Verkuil]

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