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.
Regards,
Hans
> + return -EINVAL;
> +
> return uvc_create_buffers(&stream->queue, cb);
> }
>
>
> regards
> Philipp
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
