| Message ID | 20240206080219.11951-1-benjamin.gaignard@collabora.com |
|---|---|
| Headers | show |
| Series | Add DELETE_BUF ioctl | expand |
On 06/02/2024 09:58, Hans Verkuil wrote: > On 06/02/2024 09:02, Benjamin Gaignard wrote: >> Unlike when resolution change on keyframes, dynamic resolution change >> on inter frames doesn't allow to do a stream off/on sequence because >> it is need to keep all previous references alive to decode inter frames. >> This constraint have two main problems: >> - more memory consumption. >> - more buffers in use. >> To solve these issue this series introduce DELETE_BUFS ioctl and remove >> the 32 buffers limit per queue. > > This v19 looks good. There are three outstanding issues that I need to take a > look at: > > 1) Can we still signal support for DELETE_BUFS in the V4L2_BUF_CAP_ caps? > It would be nice to have, but I'm not sure if and how that can be done. > > 2) I want to take another look at providing a next version of the VIDIOC_CREATE_BUFS > ioctl and how that would possibly influence the design of DELETE_BUFS. > Just to make sure we're not blocking anything or making life harder. So I just tried creating a new version of the VIDIOC_CREATE_BUFS ioctl that is greatly simplified. This just updates the header, no real code yet: Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index 03443833aaaa..a7398e4c85e7 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -2624,6 +2624,39 @@ struct v4l2_create_buffers { __u32 reserved[5]; }; +/** + * struct v4l2_create_buffers_v2 - VIDIOC_CREATE_BUFFERS argument + * @type: enum v4l2_buf_type + * @memory: enum v4l2_memory; buffer memory type + * @count: entry: number of requested buffers, + * return: number of created buffers + * @num_planes: requested number of planes for each buffer + * @sizes: requested plane sizes for each buffer + * @start_index:on return, index of the first created buffer + * @total_count:on return, the total number of allocated buffers + * @capabilities: capabilities of this buffer type. + * @flags: additional buffer management attributes (ignored unless the + * queue has V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS capability + * and configured for MMAP streaming I/O). + * @max_num_buffers: if V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS capability flag is set + * this field indicate the maximum possible number of buffers + * for this queue. + * @reserved: future extensions + */ +struct v4l2_create_buffers_v2 { + __u32 type; + __u32 memory; + __u32 count; + __u32 num_planes; + __u32 size[VIDEO_MAX_PLANES]; + __u32 start_index; + __u32 total_count; + __u32 capabilities; + __u32 flags; + __u32 max_num_buffers; + __u32 reserved[7]; +}; + /** * struct v4l2_delete_buffers - VIDIOC_DELETE_BUFS argument * @index: the first buffer to be deleted @@ -2738,6 +2771,7 @@ struct v4l2_delete_buffers { #define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl) #define VIDIOC_DELETE_BUFS _IOWR('V', 104, struct v4l2_delete_buffers) +#define VIDIOC_CREATE_BUFFERS _IOWR('V', 105, struct v4l2_create_buffers_v2) /* Reminder: when adding new ioctls please add support for them to Sadly, struct v4l2_create_buffers was already used for the existing VIDIOC_CREATE_BUFS (I wish it was called v4l2_create_bufs...), so I did have to add a _v2 suffix. Compared to the old struct it just moves the type, num_planes and sizes from v4l2_format into the new struct and drops the format field. Note that those fields are the only v4l2_format fields that VIDIOC_CREATE_BUFS used, so getting rid of the format makes live much easier. I also renamed 'index' to 'start_index' and added a new 'total_count' field to report the total number of buffers. The reason for adding 'total_count' is that VIDIOC_CREATE_BUFS with count == 0 would report the total number of buffers in the 'index' field, which was rather odd. Adding a specific field for this purpose is nicer. One thing that might impact your series is the name of the ioctls. We have: VIDIOC_CREATE_BUFS/v4l2_create_buffers VIDIOC_DELETE_BUFS/v4l2_delete_buffers VIDIOC_CREATE_BUFFERS/v4l2_create_buffers_v2 (TBD) I'm wondering if VIDIOC_DELETE_BUFS should be renamed to VIDIOC_DELETE_BUFFERS, that would make it more consistent with the proposed VIDIOC_CREATE_BUFFERS. Alternatively, instead of calling it VIDIOC_CREATE_BUFFERS we might call it VIDIOC_CREATE_BUFS_V2. Or perhaps some other naming convention? Ideas are welcome. Regards, Hans
On 07/02/2024 12:15, Benjamin Gaignard wrote: > > Le 07/02/2024 à 10:12, Hans Verkuil a écrit : >> Hi Benjamin, >> >> On 06/02/2024 09:58, Hans Verkuil wrote: >>> On 06/02/2024 09:02, Benjamin Gaignard wrote: >>>> Unlike when resolution change on keyframes, dynamic resolution change >>>> on inter frames doesn't allow to do a stream off/on sequence because >>>> it is need to keep all previous references alive to decode inter frames. >>>> This constraint have two main problems: >>>> - more memory consumption. >>>> - more buffers in use. >>>> To solve these issue this series introduce DELETE_BUFS ioctl and remove >>>> the 32 buffers limit per queue. >>> This v19 looks good. There are three outstanding issues that I need to take a >>> look at: >>> >>> 1) Can we still signal support for DELETE_BUFS in the V4L2_BUF_CAP_ caps? >>> It would be nice to have, but I'm not sure if and how that can be done. >> So, I came up with the following patch to add back the V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS >> capability. If the DELETE_BUFS ioctl is valid, then it sets this capability >> before calling vidioc_reqbufs or vidioc_create_bufs. So right now it will set >> this for any queue. If we ever want to disable this for a specific queue, then >> either the driver has to override these two ops and clear the flag, or a new >> vb2_queue flag (e.g. disable_delete_bufs) is added and vb2_set_flags_and_caps() >> will clear that capability based on that flag. >> >> In any case, for now just set it for both queues by default. >> >> If you agree that this is a good way to proceed, then can you incorporate this >> into a v20? You can add the documentation for this cap from the v17 version. > > Do you want it to be a separate patch or included in the patch introducing DELETE_BUFS ioctl ? Up to you, whatever makes the most sense. Regards, Hans > >> >> Regards, >> >> Hans >> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c >> index 8e437104f9c1..64f2d662d068 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c >> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c >> @@ -685,7 +685,7 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory, >> *flags &= V4L2_MEMORY_FLAG_NON_COHERENT; >> } >> >> - *caps = V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS; >> + *caps |= V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS; >> if (q->io_modes & VB2_MMAP) >> *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP; >> if (q->io_modes & VB2_USERPTR) >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >> index a172d33edd19..45bc705e171e 100644 >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >> @@ -2100,6 +2100,7 @@ static int v4l_overlay(const struct v4l2_ioctl_ops *ops, >> static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops, >> struct file *file, void *fh, void *arg) >> { >> + struct video_device *vfd = video_devdata(file); >> struct v4l2_requestbuffers *p = arg; >> int ret = check_fmt(file, p->type); >> >> @@ -2108,6 +2109,9 @@ static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops, >> >> memset_after(p, 0, flags); >> >> + if (is_valid_ioctl(vfd, VIDIOC_DELETE_BUFS)) >> + p->capabilities = V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS; >> + >> return ops->vidioc_reqbufs(file, fh, p); >> } >> >> @@ -2141,6 +2145,7 @@ static int v4l_dqbuf(const struct v4l2_ioctl_ops *ops, >> static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops, >> struct file *file, void *fh, void *arg) >> { >> + struct video_device *vfd = video_devdata(file); >> struct v4l2_create_buffers *create = arg; >> int ret = check_fmt(file, create->format.type); >> >> @@ -2151,6 +2156,9 @@ static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops, >> >> v4l_sanitize_format(&create->format); >> >> + if (is_valid_ioctl(vfd, VIDIOC_DELETE_BUFS)) >> + create->capabilities = V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS; >> + >> ret = ops->vidioc_create_bufs(file, fh, create); >> >> if (create->format.type == V4L2_BUF_TYPE_VIDEO_CAPTURE || >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >> index 03443833aaaa..da307f46f903 100644 >> --- a/include/uapi/linux/videodev2.h >> +++ b/include/uapi/linux/videodev2.h >> @@ -1036,6 +1036,7 @@ struct v4l2_requestbuffers { >> #define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 5) >> #define V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS (1 << 6) >> #define V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS (1 << 7) >> +#define V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS (1 << 8) >> >> /** >> * struct v4l2_plane - plane info for multi-planar buffers >> >> >>
Le 07/02/2024 à 11:28, Hans Verkuil a écrit : > On 06/02/2024 09:58, Hans Verkuil wrote: >> On 06/02/2024 09:02, Benjamin Gaignard wrote: >>> Unlike when resolution change on keyframes, dynamic resolution change >>> on inter frames doesn't allow to do a stream off/on sequence because >>> it is need to keep all previous references alive to decode inter frames. >>> This constraint have two main problems: >>> - more memory consumption. >>> - more buffers in use. >>> To solve these issue this series introduce DELETE_BUFS ioctl and remove >>> the 32 buffers limit per queue. >> This v19 looks good. There are three outstanding issues that I need to take a >> look at: >> >> 1) Can we still signal support for DELETE_BUFS in the V4L2_BUF_CAP_ caps? >> It would be nice to have, but I'm not sure if and how that can be done. >> >> 2) I want to take another look at providing a next version of the VIDIOC_CREATE_BUFS >> ioctl and how that would possibly influence the design of DELETE_BUFS. >> Just to make sure we're not blocking anything or making life harder. > So I just tried creating a new version of the VIDIOC_CREATE_BUFS ioctl > that is greatly simplified. This just updates the header, no real code yet: > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 03443833aaaa..a7398e4c85e7 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -2624,6 +2624,39 @@ struct v4l2_create_buffers { > __u32 reserved[5]; > }; > > +/** > + * struct v4l2_create_buffers_v2 - VIDIOC_CREATE_BUFFERS argument > + * @type: enum v4l2_buf_type > + * @memory: enum v4l2_memory; buffer memory type > + * @count: entry: number of requested buffers, > + * return: number of created buffers > + * @num_planes: requested number of planes for each buffer > + * @sizes: requested plane sizes for each buffer > + * @start_index:on return, index of the first created buffer > + * @total_count:on return, the total number of allocated buffers > + * @capabilities: capabilities of this buffer type. > + * @flags: additional buffer management attributes (ignored unless the > + * queue has V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS capability > + * and configured for MMAP streaming I/O). > + * @max_num_buffers: if V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS capability flag is set > + * this field indicate the maximum possible number of buffers > + * for this queue. > + * @reserved: future extensions > + */ > +struct v4l2_create_buffers_v2 { > + __u32 type; > + __u32 memory; > + __u32 count; > + __u32 num_planes; > + __u32 size[VIDEO_MAX_PLANES]; > + __u32 start_index; > + __u32 total_count; > + __u32 capabilities; > + __u32 flags; > + __u32 max_num_buffers; > + __u32 reserved[7]; > +}; > + > /** > * struct v4l2_delete_buffers - VIDIOC_DELETE_BUFS argument > * @index: the first buffer to be deleted > @@ -2738,6 +2771,7 @@ struct v4l2_delete_buffers { > > #define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl) > #define VIDIOC_DELETE_BUFS _IOWR('V', 104, struct v4l2_delete_buffers) > +#define VIDIOC_CREATE_BUFFERS _IOWR('V', 105, struct v4l2_create_buffers_v2) > > > /* Reminder: when adding new ioctls please add support for them to > > > Sadly, struct v4l2_create_buffers was already used for the existing > VIDIOC_CREATE_BUFS (I wish it was called v4l2_create_bufs...), so I did > have to add a _v2 suffix. Compared to the old struct it just moves the > type, num_planes and sizes from v4l2_format into the new struct and drops > the format field. Note that those fields are the only v4l2_format fields > that VIDIOC_CREATE_BUFS used, so getting rid of the format makes live > much easier. I also renamed 'index' to 'start_index' and added a new 'total_count' > field to report the total number of buffers. > > The reason for adding 'total_count' is that VIDIOC_CREATE_BUFS with > count == 0 would report the total number of buffers in the 'index' field, > which was rather odd. Adding a specific field for this purpose is nicer. > > One thing that might impact your series is the name of the ioctls. > > We have: > > VIDIOC_CREATE_BUFS/v4l2_create_buffers > VIDIOC_DELETE_BUFS/v4l2_delete_buffers > VIDIOC_CREATE_BUFFERS/v4l2_create_buffers_v2 (TBD) > > I'm wondering if VIDIOC_DELETE_BUFS should be renamed to > VIDIOC_DELETE_BUFFERS, that would make it more consistent with > the proposed VIDIOC_CREATE_BUFFERS. > > Alternatively, instead of calling it VIDIOC_CREATE_BUFFERS we > might call it VIDIOC_CREATE_BUFS_V2. > > Or perhaps some other naming convention? Maybe VIDIOC_NEW_BUFS/v4l2_new_buffers ? I will wait until naming convention is fixed before send v20. Regards, Benjamin > > Ideas are welcome. > > Regards, > > Hans >