Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Hans Verkuil <hansverk@cisco.com>]

On Thursday, April 07, 2011 10:53:32 Guennadi Liakhovetski wrote:
> On Thu, 7 Apr 2011, Hans Verkuil wrote:
> 
> > > On Thu, 7 Apr 2011, Hans Verkuil wrote:
> > >
> > >> On Wednesday, April 06, 2011 18:19:18 Guennadi Liakhovetski wrote:
> > >> > On Tue, 5 Apr 2011, Hans Verkuil wrote:
> > >> >
> > >> > > On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
> > >> > > > On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
> > >> >
> > >> > [snip]
> > >> >
> > >> > > > >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> > >> > > > >   *
> > >> > > > > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> > >> > > > >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> > >> > > > > v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT
> > >> _IOW('V', 91,
> > >> > > > > struct v4l2_event_subscription)
> > >> > > > >
> > >> > > > > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct
> > >> v4l2_create_buffers)
> > >> > > > > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct
> > >> v4l2_buffer_span)
> > >> > > > > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> > >> > > > > +
> > >> > > >
> > >> > > > In case we later need to pass other information (such as flags) 
to
> > >> > > > VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
> > >> > >
> > >> > > I would just pass struct v4l2_buffer to this ioctl, just like
> > >> QBUF/DQBUF do.
> > >> >
> > >> > As I said, I didn't like this very much, because it involves 
redundant
> > >> > data, but if we want to call .buf_prepare() from it, then we need
> > >> > v4l2_buffer...
> > >>
> > >> I don't see a problem with this. Applications already *have* the
> > >> v4l2_buffer
> > >> after all. It's not as if they have to fill that structure just for 
this
> > >> call.
> > >>
> > >> Furthermore, you need all that data anyway because you need to do the
> > >> same
> > >> checks that vb2_qbuf does.
> > >>
> > >> Regarding DESTROY_BUFS: perhaps we should just skip this for now and
> > >> wait for
> > >> the first use-case. That way we don't need to care about holes. I don't
> > >> like
> > >> artificial restrictions like 'no holes'. If someone has a good use-case
> > >> for
> > >> selectively destroying buffers, then we need to look at this again.
> > >
> > > Sorry, skip what? skip the ioctl completely and rely on REQBUFS(0) /
> > > close()?
> > 
> > Yes.
> 
> Ok, how about this: I remove the ioctl definition and struct 
> v4l2_ioctl_ops callback for it, but I keep the span struct and the vb2 
> implementation - in case we need it later? The vb2_destroy_bufs() will be 
> used to implement freeing the queue as a particular case of freeing some 
> buffers.

I wouldn't do that. Just keep the code clean and the patch as small as 
possible. Having unused/unnecessary code is bad practice.

Regards,

        Hans