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@xxxxxxxxxxxxxxx/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. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
