Re: [PATCH] [media] uvcvideo: Enable VIDIOC_CREATE_BUFS

[Hans Verkuil <hverkuil@xs4all.nl>]

On 02/05/14 08:57, Hans Verkuil wrote:
> On 02/05/2014 12:04 AM, Sylwester Nawrocki wrote:
>> Hi,
>>
>> On 02/03/2014 10:03 AM, Hans Verkuil wrote:
>>> Hi Philipp, Laurent,
>>>
>>> On 02/02/2014 02:04 PM, Philipp Zabel wrote:
>>>> On Sun, Feb 02, 2014 at 11:21:13AM +0100, Laurent Pinchart wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On Friday 31 January 2014 09:43:00 Hans Verkuil wrote:
>>>>>> I think you might want to add a check in uvc_queue_setup to verify the
>>>>>> fmt that create_bufs passes. The spec says that: "Unsupported formats
>>>>>> will result in an error". In this case I guess that the format basically
>>>>>> should match the current selected format.
>>>>>>
>>>>>> I'm unhappy with the current implementations of create_bufs (see also this
>>>>>> patch:
>>>>>> http://www.mail-archive.com/linux-media@vger.kernel.org/msg70796.html).
>>>>>>
>>>>>> Nobody is actually checking the format today, which isn't good.
>>>>>>
>>>>>> The fact that the spec says that the fmt field isn't changed by the driver
>>>>>> isn't helping as it invalidated my patch from above, although that can be
>>>>>> fixed.
>>>>>>
>>>>>> I need to think about this some more, but for this particular case you can
>>>>>> just do a memcmp of the v4l2_pix_format against the currently selected
>>>>>> format and return an error if they differ. Unless you want to support
>>>>>> different buffer sizes as well?
>>>>>
>>>>> Isn't the whole point of VIDIOC_CREATE_BUFS being able to create buffers of
>>>>> different resolutions than the current active resolution ?
>>>
>>> Or just additional buffers with the same resolution (or really, the same size).
>>>
>>>> For that to work the driver in question would need to keep track of per-buffer
>>>> format and resolution, and not only of per-queue format and resolution.
>>>>
>>>> For now, would something like the following be enough? Unfortunately the uvc
>>>> driver doesn't keep a v4l2_format around that we could just memcmp against:
>>>>
>>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>>>> index fa58131..7fa469b 100644
>>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>>> @@ -1003,10 +1003,26 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>>>   	case VIDIOC_CREATE_BUFS:
>>>>   	{
>>>>   		struct v4l2_create_buffers *cb = arg;
>>>> +		struct v4l2_pix_format *pix;
>>>> +		struct uvc_format *format;
>>>> +		struct uvc_frame *frame;
>>>>
>>>>   		if (!uvc_has_privileges(handle))
>>>>   			return -EBUSY;
>>>>
>>>> +		format = stream->cur_format;
>>>> +		frame = stream->cur_frame;
>>>> +		pix =&cb->format.fmt.pix;
>>>> +
>>>> +		if (pix->pixelformat != format->fcc ||
>>>> +		    pix->width != frame->wWidth ||
>>>> +		    pix->height != frame->wHeight ||
>>>> +		    pix->field != V4L2_FIELD_NONE ||
>>>> +		    pix->bytesperline != format->bpp * frame->wWidth / 8 ||
>>>> +		    pix->sizeimage != stream->ctrl.dwMaxVideoFrameSize ||
>>>> +		    pix->colorspace != format->colorspace)
>>>
>>> I would drop the field and colorspace checks (those do not really affect
>>> any size calculations), other than that it looks good.
>>
>> That seems completely wrong to me, AFAICT the VIDIOC_CREATE_BUFS was 
>> designed
>> so that the driver is supposed to allow any format which is supported by the
>> hardware.
>> What has currently selected format to do with the format passed to
>> VIDIOC_CREATE_BUFS ? It should be allowed to create buffers of any size
>> (implied by the passed v4l2_pix_format). It is supposed to be checked if
>> a buffer meets constraints of current configuration of
>> the hardware at QBUF, not at VIDIOC_CREATE_BUFS time. User space may well
>> allocate buffers when one image format is set, keep them aside and then
>> just before queueing them to the driver may set the format to a different
>> one, so the hardware set up matches buffers allocated with 
>> VIDIOC_CREATE_BUFS.
>>
>> What's the point of having VIDIOC_CREATE_BUFS when you are doing checks
>> like above ? Unless I'm missing something that is completely wrong. :)
>> Adjusting cb->format.fmt.pix as in VIDIOC_TRY_FORMAT seems more appropriate
>> thing to do.
> 
> OK, I agree that the code above is wrong. So ignore that.
> 
> What should CREATE_BUFS do when it is called?
> 
> Should I go back to this patch: http://www.spinics.net/lists/linux-media/msg72171.html
> 
> It will at least ensure that the fmt is consistent. It is however not quite
> according to the spec since invalid formats are generally 'reformatted' by
> TRY_FMT to something valid, and the spec says invalid formats should return
> an error. It is possible to do something more advanced here, though: you
> could make a copy of v4l2_format, call TRY_FMT on it, and check if there
> are any differences with what was passed in. If there are, return an
> error.
> 
> It's a bit of work, but probably better to do it in the core rather than
> depend on drivers to do it (since they won't :-) ).
> 
> If queue_setup can rely on fmt to be a valid format, then sizeimage can
> just be used as the buffer size.
> 
> With regards to checking constraints on QBUF: I see a problem there. For
> a regular buffer it can be checked in buf_prepare, but what if a buffer
> is already prepared using VIDIOC_PREPARE_BUF, then the format is changed
> and you call VIDIOC_QBUF with that prepared buffer? Then there is no
> callback where you can check this since the buf_prepare call has already
> happened.

A follow-up: are there any (Samsung?) drivers that use this? CREATE_BUFS and/or
PREPARE_BUF?

I'm adding streaming tests to v4l2-compliance and I also want to add tests
for CREATE_BUFS there. Nasty things like zeroing the fmt and just set the
pixelformat to something valid, or setting imagesize to half the current
format size and then queuing it. I fear the worst :-)

Regards,

	Hans