Hi Hans, On Monday 22 August 2011 17:52:12 Hans Verkuil wrote: > On Monday, August 22, 2011 17:42:36 Laurent Pinchart wrote: > > On Monday 22 August 2011 15:54:03 Guennadi Liakhovetski wrote: > > > We discussed a bit more with Hans on IRC, and below is my attempt of a > > > summary. Hans, please, correct me, if I misunderstood anything. Pawel, > > > Sakari, Laurent: please, reply, whether you're ok with this. > > > > Sakari is on holidays this week. > > > > > On Mon, 22 Aug 2011, Hans Verkuil wrote: > > > > On Monday, August 22, 2011 12:40:25 Guennadi Liakhovetski wrote: > > > [snip] > > > > > > > > It would be good if you also could have a look at my reply to this > > > > > Pawel's mail: > > > > > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input- > > > > > > > > infrastructure/36905 > > > > > > > > > and, specifically, at the vb2_parse_planes() function in it. That's > > > > > my understanding of what would be needed, if we preserve > > > > > .queue_setup() and use your last suggestion to include struct > > > > > v4l2_format in struct v4l2_create_buffers. > > > > > > > > vb2_parse_planes can be useful as a utility function that 'normal' > > > > drivers can call from the queue_setup. But vb2 should not parse the > > > > format directly, it should just pass it on to the driver through the > > > > queue_setup function. > > > > > > > > You also mention: "All frame-format fields like fourcc code, width, > > > > height, colorspace are only input from the user. If the user didn't > > > > fill them in, they should not be used." > > > > > > > > I disagree with that. The user should fill in a full format > > > > description, just as with S/TRY_FMT. That's the information that the > > > > driver will use to set up the buffers. It could have weird rules > > > > like: if the fourcc is this, and the size is less than that, then we > > > > can allocate in this memory bank. > > > > > > > > It is also consistent with REQBUFS: there too the driver uses a full > > > > format (i.e. the last set format). > > > > > > > > I would modify queue_setup to something like this: > > > > > > > > int (*queue_setup)(struct vb2_queue *q, struct v4l2_format *fmt, > > > > > > > > unsigned int *num_buffers, > > > > unsigned int *num_planes, unsigned int sizes[], > > > > void *alloc_ctxs[]); > > > > > > > > Whether fmt is left to NULL in the reqbufs case, or whether the > > > > driver has to call g_fmt first before calling vb2 is something that > > > > could be decided by what is easiest to implement. > > > > > > 1. VIDIOC_CREATE_BUFS passes struct v4l2_create_buffers from the user > > > to > > > > > > the kernel, in which struct v4l2_format is embedded. The user _must_ > > > fill in .type member of struct v4l2_format. For .type == > > > V4L2_BUF_TYPE_VIDEO_CAPTURE or V4L2_BUF_TYPE_VIDEO_OUTPUT .fmt.pix > > > is used, for .type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE or > > > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE .fmt.pix_mp is used. In both these > > > cases the user _must_ fill in .width, .height, .pixelformat, .field, > > > .colorspace by possibly calling VIDIOC_G_FMT or VIDIOC_TRY_FMT. The > > > user also _may_ optionally fill in any further buffer-size related > > > fields, if it believes to have any special requirements to them. On > > > a successful return from the ioctl() .count and .index fields are > > > filled in by the kernel, .format stays unchanged. The user has to > > > call VIDIOC_QUERYBUF to retrieve specific buffer information. > > > > > > 2. Videobuf2 drivers, that implement .vidioc_create_bufs() operation, > > > call > > > > > > vb2_create_bufs() with a pointer to struct v4l2_create_buffers as a > > > second argument. vb2_create_bufs() in turn calls the .queue_setup() > > > > > > driver callback, whose prototype is modified as follows: > > > int (*queue_setup)(struct vb2_queue *q, const struct v4l2_format *fmt, > > > > > > unsigned int *num_buffers, > > > unsigned int *num_planes, unsigned int sizes[], > > > void *alloc_ctxs[]); > > > > > > with &create->format as a second argument. As pointed out above, > > > this struct is not modified by V4L, instead, the usual arguments > > > 3-6 are filled in by the driver, which are then used by > > > vb2_create_bufs() to call __vb2_queue_alloc(). > > > > > > 3. vb2_reqbufs() shall call .queue_setup() with fmt == NULL, which will > > > be > > > > > > a signal to the driver to use the current format. > > > > > > 4. We keep .queue_setup(), because its removal would inevitably push a > > > > > > part of the common code from vb2_reqbufs() and vb2_create_bufs() > > > down into drivers, thus creating code redundancy and increasing its > > > complexity. > > > > How much common code would be pushed down to drivers ? I don't think this > > is a real issue. I like Pawel's proposal of removing .queue_setup() > > better. > > I still don't see what removing queue_setup will solve or improve. It will remove handling of the format in vb2 (even if it's a pass-through operation). I think it would be cleaner that way. It will also avoid going back and forth between drivers and vb2, which would improve code readability. > I'd say leave it as it is to keep the diff as small as possible and someone > can always attempt to remove it later. Removing queue_setup is independent > from multi-size videobuffer management and we should not mix the two. Guennadi's patch will (at least in my opinion) be cleaner if built on top of queue_setup() removal. -- Regards, Laurent Pinchart -- 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
