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
