Re: [PATCH 1/6 v4] V4L: add two new ioctl()s for multi-size videobuffer management

[Hans Verkuil <hansverk@cisco.com>]

On Monday, August 22, 2011 12:40:25 Guennadi Liakhovetski wrote:
> Hi Hans
> 
> On Mon, 22 Aug 2011, Hans Verkuil wrote:
> 
> > Sorry for starting this discussion and then disappearing. I've been very
> > busy lately, so my apologies for that.
> > 
> > On Tuesday, August 16, 2011 18:14:33 Pawel Osciak wrote:
> > > Hi Guennadi,
> > > 
> > > On Tue, Aug 16, 2011 at 06:13, Guennadi Liakhovetski
> > > <g.liakhovetski@gmx.de> wrote:
> > > > On Mon, 15 Aug 2011, Guennadi Liakhovetski wrote:
> > > >
> > > >> On Mon, 15 Aug 2011, Hans Verkuil wrote:
> > > >>
> > > >> > On Monday, August 15, 2011 13:28:23 Guennadi Liakhovetski wrote:
> > > >> > > Hi Hans
> > > >> > >
> > > >> > > On Mon, 8 Aug 2011, Hans Verkuil wrote:
> > > >
> > > > [snip]
> > > >
> > > >> > > > but I've changed my mind: I think
> > > >> > > > this should use a struct v4l2_format after all.
> > > >>
> > > >> While switching back, I have to change the struct 
vb2_ops::queue_setup()
> > > >> operation to take a struct v4l2_create_buffers pointer. An earlier 
> > version
> > > >> of this patch just added one more parameter to .queue_setup(), which 
is
> > > >> easier - changes to videobuf2-core.c are smaller, but it is then
> > > >> redundant. We could use the create pointer for both input and output. 
The
> > > >> video plane configuration in frame format is the same as what is
> > > >> calculated in .queue_setup(), IIUC. So, we could just let the driver 
fill
> > > >> that one in. This would require then the videobuf2-core.c to parse 
struct
> > > >> v4l2_format to decide which union member we need, depending on the 
buffer
> > > >> type. Do we want this or shall drivers duplicate plane sizes in 
separate
> > > >> .queue_setup() parameters?
> > > >
> > > > Let me explain my question a bit. The current .queue_setup() method is
> > > >
> > > >        int (*queue_setup)(struct vb2_queue *q, unsigned int 
*num_buffers,
> > > >                           unsigned int *num_planes, unsigned int 
sizes[],
> > > >                           void *alloc_ctxs[]);
> > > >
> > > > To support multiple-size buffers we also have to pass a pointer to 
struct
> > > > v4l2_create_buffers to this function now. We can either do it like 
this:
> > > >
> > > >        int (*queue_setup)(struct vb2_queue *q,
> > > >                           struct v4l2_create_buffers *create,
> > > >                           unsigned int *num_buffers,
> > > >                           unsigned int *num_planes, unsigned int 
sizes[],
> > > >                           void *alloc_ctxs[]);
> > > >
> > > > and let all drivers fill in respective fields in *create, e.g., either 
do
> > > >
> > > >        create->format.fmt.pix_mp.plane_fmt[i].sizeimage = ...;
> > > >        create->format.fmt.pix_mp.num_planes = ...;
> > > >
> > > > and also duplicate it in method parameters
> > > >
> > > >        *num_planes = create->format.fmt.pix_mp.num_planes;
> > > >        sizes[i] = create->format.fmt.pix_mp.plane_fmt[i].sizeimage;
> > > >
> > > > or with
> > > >
> > > >        create->format.fmt.pix.sizeimage = ...;
> > > >
> > > > for single-plane. Alternatively we make the prototype
> > > >
> > > >        int (*queue_setup)(struct vb2_queue *q,
> > > >                           struct v4l2_create_buffers *create,
> > > >                           unsigned int *num_buffers,
> > > >                           void *alloc_ctxs[]);
> > > >
> > > > then drivers only fill in *create, and the videobuf2-core will have to
> > > > check create->format.type to decide, which of create->format.fmt.* is
> > > > relevant and extract plane sizes from there.
> > > 
> > > 
> > > Could we try exploring an alternative idea?
> > > The queue_setup callback was added to decouple formats from vb2 (and
> > > add some asynchronousness). But now we are doing the opposite, adding
> > > format awareness to vb2. Does vb2 really need to know about formats? I
> > > really believe it doesn't. It only needs sizes and counts. Also, we
> > > are actually complicating things I think. The proposal, IIUC, would
> > > look like this:
> > > 
> > > driver_queue_setup(..., create, num_buffers, [num_planes], ...)
> > > {
> > >     if (create != NULL && create->format != NULL) {
> > >         /* use create->fmt to fill sizes */
> > 
> > Right.
> > 
> > >     } else if (create != NULL) { /* this assumes we have both format or 
> > sizes */
> > >         /* use create->sizes to fill sizes */
> > 
> > No, create->format should always be set. If the application can fill in 
the
> > sizeimage field(s), then there is no need for create->sizes.
> > 
> > >     } else {
> > >         /* use currently selected format to fill sizes */
> > 
> > Right.
> > 
> > >     }
> > > }
> > > 
> > > driver_s_fmt(format)
> > > {
> > >     /* ... */
> > >     driver_fill_format(&create->fmt);
> > >     /* ... */
> > > }
> > 
> > ???
> > 
> > > 
> > > driver_create_bufs(create)
> > > {
> > >     vb2_create_bufs(create);
> > > }
> > > 
> > > vb2_create_bufs(create)
> > > {
> > >     driver_queue_setup(..., create, ...);
> > >     vb2_fill_format(&create->fmt); /* note different from
> > > driver_fill_format(), but both needed */
> > 
> > Huh? Why call vb2_fill_format? vb2 should have no knowledge whatsoever 
about
> > formats. The driver needs that information in order to be able to allocate
> > buffers correctly since that depends on the required format. But vb2 
doesn't
> > need that knowledge.
> 
> 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.

Regards,

	Hans

> However, from your reply I couldn't understand your attitude to removing 
> .queue_setup() altogether. Do you see any disadvantages in doing so? Is it 
> serving any special role, that we are overseeing?
> 
> Thanks
> Guennadi
> 
> > 
> > > }
> > > 
> > > vb2_reqbufs(reqbufs)
> > > {
> > >    driver_queue_setup(..., NULL, ...);
> > > }
> > > 
> > > The queue_setup not only becomes unnecessarily complicated, but I'm
> > > starting to question the convenience of it. And we are teaching vb2
> > > how to interpret format structs, even though vb2 only needs sizes, and
> > > even though the driver has to do it anyway and knows better how.
> > 
> > No, vb2 just needs to pass the format information from the user to the
> > driver.
> > 
> > There seems to be some misunderstanding here.
> > 
> > The point of my original suggestion that create_bufs should use 
v4l2_format
> > is that the driver needs the format information in order to decide how and
> > where the buffers have to be allocated. Having the format available is the
> > only reliable way to do that.
> > 
> > This is already done for REQBUFS since the driver will use the current 
format
> > to make these decisions.
> > 
> > One way of simplifying queue_setup is actually to always supply the 
format.
> > In the case of REQBUFS the driver might do something like this:
> > 
> > driver_reqbufs(requestbuffers)
> > {
> > 	struct v4l2_format fmt;
> > 	struct v4l2_create_buffers create;
> > 
> > 	vb2_free_bufs(); // reqbufs should free any existing bufs
> > 	if (requestbuffers->count == 0)
> > 		return 0;
> > 	driver_g_fmt(&fmt);	// call the g_fmt ioctl op
> > 	// fill in create
> > 	vb2_create_bufs(create);
> > }
> > 
> > So vb2 just sees a call requesting to create so many buffers for a 
particular
> > format, and it just hands that information over to the driver *without*
> > parsing it.
> > 
> > And the driver gets the request from vb2 to create X buffers for format F, 
and
> > will figure out how to do that and returns the buffer/plane/allocator 
context
> > information back to vb2.
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > > As for the idea to fill fmt in vb2, even if vb2 was to do it in
> > > create_bufs, some code to parse and fill the format fields would need
> > > to be in the driver anyway, because it still has to support s_fmt and
> > > friends. So adding that code to vb2 would duplicate it, and if the
> > > driver wanted to be non-standard in a way it filled the format fields,
> > > we'd not be allowing that.
> > > 
> > > My suggestion would be to remove queue_setup callback and instead
> > > modify vb2_reqbufs and vb2_create_bufs to accept sizes and number of
> > > buffers. I think it should simplify things both for drivers and vb2,
> > > would keep vb2 format-unaware and save us some round trips between vb2
> > > and driver:
> > > 
> > > driver_create_bufs(...) /* optional */
> > > {
> > >     /* use create->fmt (or sizes) */
> > >     ret = vb2_create_bufs(num_buffers, num_planes, buf_sizes,
> > > plane_sizes, alloc_ctxs);
> > >     fill_format(&create->fmt) /* because s_fmt has to do it anyway, so
> > > have a common function for that */
> > >     return ret;
> > > }
> > > 
> > > driver_reqbufs(...)
> > > {
> > >     /* use current format */
> > >     return vb2_reqbufs(num_buffers, num_planes, buf_sizes,
> > > plane_sizes, alloc_ctxs);
> > > }
> > > 
> > > And the call to both could easily converge into one in vb2, as the
> > > only difference is that vb2_reqbufs would need to free first, if any
> > > allocated buffers were present:
> > > 
> > > vb2_reqbufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs)
> > > {
> > >     if (buffers_allocated(num_buffers, num_planes, buf_sizes,
> > > plane_sizes, alloc_ctxs)) {
> > >         free_buffers(...);
> > >     }
> > > 
> > >     return vb2_create_bufs(num_buffers, num_planes, buf_sizes,
> > > plane_sizes, alloc_ctxs);
> > > }
> > > 
> > > If the driver didn't want create_bufs, it'd just not implement it.
> > > What do you think?
> > > 
> > > -- 
> > > Best regards,
> > > Pawel Osciak
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-media" 
in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > 
> 
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>