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

[Philipp Zabel <pza@pengutronix.de>]

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 ?

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)
+			return -EINVAL;
+
 		return uvc_create_buffers(&stream->queue, cb);
 	}
 

regards
Philipp