Re: [RFC] Add VIDIOC_DESTROY_BUFS

[Boris Brezillon <boris.brezillon@collabora.com>]

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