Re: [RFC] Add VIDIOC_DESTROY_BUFS

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

On Mon, Nov 18, 2019 at 02:59:50PM +0100, Hans Verkuil wrote:
> On 11/18/19 2:52 PM, Boris Brezillon wrote:
> > On Mon, 18 Nov 2019 14:06:40 +0100 Hans Verkuil wrote:
> > 
> >> Here is a proposal for a new VIDIOC_DESTROY_BUFS ioctl:
> > 
> > Thanks for sending this RFC.
> > 
> >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> >> index c7c1179eea65..1a80d1119768 100644
> >> --- a/include/uapi/linux/videodev2.h
> >> +++ b/include/uapi/linux/videodev2.h
> >> @@ -2423,6 +2423,19 @@ struct v4l2_create_buffers {
> >>  	__u32			reserved[7];
> >>  };
> >>
> >> +/**
> >> + * struct v4l2_destroy_buffers - VIDIOC_DESTROY_BUFS argument
> >> + * @type:	stream type
> >> + * @index:	index of the first buffer to destroy
> >> + * @count:	number of consecutive buffers starting from @index to destroy
> >> + */
> >> +struct v4l2_destroy_buffers {
> >> +	__u32			type;
> >> +	__u32			index;
> >> +	__u32			count;
> >> +};

Another option, to make this more flexible, is to replace index by a
pointer to an array of count elements, each containing an index of a
buffer to destroy.

> >> +
> >> +
> >>  /*
> >>   *	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
> >>   *
> >> @@ -2522,6 +2535,7 @@ struct v4l2_create_buffers {
> >>  #define VIDIOC_DBG_G_CHIP_INFO  _IOWR('V', 102, struct v4l2_dbg_chip_info)
> >>
> >>  #define VIDIOC_QUERY_EXT_CTRL	_IOWR('V', 103, struct v4l2_query_ext_ctrl)
> >> +#define VIDIOC_DESTROY_BUFS	_IOW ('V', 104, struct v4l2_destroy_buffers)
> >>
> >>  /* Reminder: when adding new ioctls please add support for them to
> >>     drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
> >>
> >>
> >>
> >> So this basically just destroys buffers [index..index+count-1]. Does nothing if
> >> count == 0. All buffers in the sequence must be dequeued or it will return
> >> -EBUSY and do nothing.
> >>
> >> If some of the buffers in that range are already destroyed, or in fact were
> >> never created, then they will be ignored. I.e., DESTROY_BUFS won't return
> >> an error in that case.
> > 
> > Sounds good to me.
> > 
> >> VIDIOC_CREATE_BUFS will need a few changes:
> >>
> >> CREATE_BUFS will try to find a range of <count> free consecutive buffers.
> >> If that's not available, then it will reduce <count> to the count of the
> >> maximum freely available consecutive buffers. If <count> is 0, then it
> >> will set <index> to the maximum index of an existing buffer + 1.
> >>
> >> As long as DESTROY_BUFS isn't used, then CREATE_BUFS acts exactly the same
> >> as it does today.
> > 
> > Sounds good too.
> > 
> >> I would also like to extend struct v4l2_create_buffers with a new field:
> >> __u32 max_index. This is a maximum index possible, typically VIDEO_MAX_FRAME-1.
> > 
> > Shouldn't max_buffers be a property of the queue, set through a separate
> > ioctl()? BTW, how would you decrease the queue depth?
> > CREATE_BUFS.{count=0,max_index=<new-depth>}?
> 
> I think the name might be confusing: cap_max_index might be better: this is just
> a read-only capability: i.e. how many buffers can userspace create? Currently
> this is 32, but in the future drivers should be able to allow for more buffers.
> It should be something they tell vb2.

Why should we set a limit though ? And how would driver decide ?

-- 
Regards,

Laurent Pinchart