[RFC] Add VIDIOC_DESTROY_BUFS

[RFC] Add VIDIOC_DESTROY_BUFS

[Hans Verkuil]

Here is a proposal for a new VIDIOC_DESTROY_BUFS ioctl:

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;
+};
+
+
 /*
  *	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.


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.

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.

This is important when we want to support more than 32 buffers. But we can
postpone this as well. But this is all we need to have the API support this
feature, even though more work is needed internally to actually break the
32 buffer limit.

Regards,

	Hans

Re: [RFC] Add VIDIOC_DESTROY_BUFS

[Boris Brezillon]

Hello Hans,

On Mon, 18 Nov 2019 14:06:40 +0100
Hans Verkuil <hverkuil@xs4all.nl> 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;
> +};
> +
> +
>  /*
>   *	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>}?

Thanks,

Boris

Re: [RFC] Add VIDIOC_DESTROY_BUFS

[Hans Verkuil]

On 11/18/19 2:52 PM, Boris Brezillon wrote:
> Hello Hans,
> 
> On Mon, 18 Nov 2019 14:06:40 +0100
> Hans Verkuil <hverkuil@xs4all.nl> 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;
>> +};
>> +
>> +
>>  /*
>>   *	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.

Regards,

	Hans

Re: [RFC] Add VIDIOC_DESTROY_BUFS

[Laurent Pinchart]

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