Re: [PATCH] videodev2.h: add V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF/CREATE_BUFS
Hi Hans, I love your patch! Perhaps something to improve: [auto build test WARNING on linuxtv-media/master] [also build test WARNING on v4.20-rc3 next-20181120] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Hans-Verkuil/videodev2-h-add-V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF-CREATE_BUFS/20181120-190153 base: git://linuxtv.org/media_tree.git master config: i386-randconfig-x070-201846 (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All warnings (new ones prefixed by >>): In file included from include/linux/err.h:5:0, from drivers/media/common/videobuf2/videobuf2-v4l2.c:17: drivers/media/common/videobuf2/videobuf2-v4l2.c: In function 'fill_buf_caps_vdev': drivers/media/common/videobuf2/videobuf2-v4l2.c:878:21: error: dereferencing pointer to incomplete type 'const struct v4l2_ioctl_ops' if (vdev->ioctl_ops->vidioc_prepare_buf) ^ include/linux/compiler.h:58:30: note: in definition of macro '__trace_if' if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ ^~~~ >> drivers/media/common/videobuf2/videobuf2-v4l2.c:878:2: note: in expansion of >> macro 'if' if (vdev->ioctl_ops->vidioc_prepare_buf) ^~ vim +/if +878 drivers/media/common/videobuf2/videobuf2-v4l2.c 873 874 static void fill_buf_caps_vdev(struct video_device *vdev, u32 *caps) 875 { 876 *caps = 0; 877 fill_buf_caps(vdev->queue, caps); > 878 if (vdev->ioctl_ops->vidioc_prepare_buf) 879 *caps |= V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF; 880 if (vdev->ioctl_ops->vidioc_create_bufs) 881 *caps |= V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS; 882 } 883 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH] videodev2.h: add V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF/CREATE_BUFS
Hi Hans, I love your patch! Yet something to improve: [auto build test ERROR on linuxtv-media/master] [also build test ERROR on v4.20-rc3 next-20181120] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Hans-Verkuil/videodev2-h-add-V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF-CREATE_BUFS/20181120-190153 base: git://linuxtv.org/media_tree.git master config: i386-randconfig-x077-201846 (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): drivers/media/common/videobuf2/videobuf2-v4l2.c: In function 'fill_buf_caps_vdev': >> drivers/media/common/videobuf2/videobuf2-v4l2.c:878:21: error: dereferencing >> pointer to incomplete type 'const struct v4l2_ioctl_ops' if (vdev->ioctl_ops->vidioc_prepare_buf) ^~ vim +878 drivers/media/common/videobuf2/videobuf2-v4l2.c 873 874 static void fill_buf_caps_vdev(struct video_device *vdev, u32 *caps) 875 { 876 *caps = 0; 877 fill_buf_caps(vdev->queue, caps); > 878 if (vdev->ioctl_ops->vidioc_prepare_buf) 879 *caps |= V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF; 880 if (vdev->ioctl_ops->vidioc_create_bufs) 881 *caps |= V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS; 882 } 883 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH] videodev2.h: add V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF/CREATE_BUFS
On Tue, Nov 20, 2018 at 10:41:42AM +0100, Hans Verkuil wrote: > On 11/20/2018 10:27 AM, Sakari Ailus wrote: > > Hi Hans, > > > > On Tue, Nov 20, 2018 at 09:58:43AM +0100, Hans Verkuil wrote: > >> Add new buffer capability flags to indicate if the VIDIOC_PREPARE_BUF or > >> VIDIOC_CREATE_BUFS ioctls are supported. > > > > Are there practical benefits from the change for the user space? > > The more important ioctl to know about is PREPARE_BUF. I noticed this when > working > on v4l2-compliance: the only way to know for an application if PREPARE_BUF > exists > is by trying it, but then you already have prepared a buffer. That's not what > you > want in the application, you need a way to know up front if prepare_buf is > present > or not without having to actually execute it. > > CREATE_BUFS was added because not all drivers support it. It can be dropped > since > it is possible to test for the existence of CREATE_BUFS without actually > allocating > anything, but if I'm adding V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF anyway, then it > is > trivial to add V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS as well to avoid an > additional > ioctl call. > > Hmm, I should have explained this in the commit log. Please add: Acked-by: Sakari Ailus -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH] videodev2.h: add V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF/CREATE_BUFS
On 11/20/2018 10:27 AM, Sakari Ailus wrote: > Hi Hans, > > On Tue, Nov 20, 2018 at 09:58:43AM +0100, Hans Verkuil wrote: >> Add new buffer capability flags to indicate if the VIDIOC_PREPARE_BUF or >> VIDIOC_CREATE_BUFS ioctls are supported. > > Are there practical benefits from the change for the user space? The more important ioctl to know about is PREPARE_BUF. I noticed this when working on v4l2-compliance: the only way to know for an application if PREPARE_BUF exists is by trying it, but then you already have prepared a buffer. That's not what you want in the application, you need a way to know up front if prepare_buf is present or not without having to actually execute it. CREATE_BUFS was added because not all drivers support it. It can be dropped since it is possible to test for the existence of CREATE_BUFS without actually allocating anything, but if I'm adding V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF anyway, then it is trivial to add V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS as well to avoid an additional ioctl call. Hmm, I should have explained this in the commit log. Regards, Hans > >> >> Signed-off-by: Hans Verkuil >> --- >> Note: the flag bits will change since there are two other patches that add >> flags, so the numbering will change. >> --- >> diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst >> b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst >> index d40c60e8..abf925484aff 100644 >> --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst >> +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst >> @@ -112,6 +112,8 @@ any DMA in progress, an implicit >> .. _V4L2-BUF-CAP-SUPPORTS-USERPTR: >> .. _V4L2-BUF-CAP-SUPPORTS-DMABUF: >> .. _V4L2-BUF-CAP-SUPPORTS-REQUESTS: >> +.. _V4L2-BUF-CAP-SUPPORTS-PREPARE-BUF: >> +.. _V4L2-BUF-CAP-SUPPORTS-CREATE-BUFS: >> >> .. cssclass:: longtable >> >> @@ -132,6 +134,12 @@ any DMA in progress, an implicit >> * - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS`` >>- 0x0008 >>- This buffer type supports :ref:`requests `. >> +* - ``V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF`` >> + - 0x0010 >> + - This buffer type supports :ref:`VIDIOC_PREPARE_BUF`. >> +* - ``V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS`` >> + - 0x0020 >> + - This buffer type supports :ref:`VIDIOC_CREATE_BUFS`. >> >> Return Value >> >> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c >> b/drivers/media/common/videobuf2/videobuf2-v4l2.c >> index a17033ab2c22..27c0fafca0bf 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c >> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c >> @@ -871,6 +871,16 @@ static inline bool vb2_queue_is_busy(struct >> video_device *vdev, struct file *fil >> return vdev->queue->owner && vdev->queue->owner != >> file->private_data;D_PACK >> } >> >> +static void fill_buf_caps_vdev(struct video_device *vdev, u32 *caps) >> +{ >> +*caps = 0; >> +fill_buf_caps(vdev->queue, caps); >> +if (vdev->ioctl_ops->vidioc_prepare_buf) >> +*caps |= V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF; >> +if (vdev->ioctl_ops->vidioc_create_bufs) >> +*caps |= V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS; >> +} >> + >> /* vb2 ioctl helpers */ >> >> int vb2_ioctl_reqbufs(struct file *file, void *priv, >> @@ -879,7 +889,7 @@ int vb2_ioctl_reqbufs(struct file *file, void *priv, >> struct video_device *vdev = video_devdata(file); >> int res = vb2_verify_memory_type(vdev->queue, p->memory, p->type); >> >> -fill_buf_caps(vdev->queue, &p->capabilities); >> +fill_buf_caps_vdev(vdev, &p->capabilities); >> if (res) >> return res; >> if (vb2_queue_is_busy(vdev, file)) >> @@ -901,7 +911,7 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv, >> p->format.type); >> >> p->index = vdev->queue->num_buffers; >> -fill_buf_caps(vdev->queue, &p->capabilities); >> +fill_buf_caps_vdev(vdev, &p->capabilities); >> /* >> * If count == 0, then just check if memory and type are valid. >> * Any -EBUSY result from vb2_verify_memory_type can be mapped to 0. >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >> index c8e8ff810190..6648f8ba2277 100644 >> --- a/include/uapi/linux/videodev2.h >> +++ b/include/uapi/linux/videodev2.h >> @@ -879,6 +879,8 @@ struct v4l2_requestbuffers { >> #define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1) >> #define V4L2_BUF_CAP_SUPPORTS_DMABUF(1 << 2) >> #define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3) > > Could you align the previous lines to match the ones below? > >> +#define V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF (1 << 4) >> +#define V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS (1 << 5) >> >> /** >> * struct v4l2_plane - plane info for multi-planar buffers >
Re: [PATCH] videodev2.h: add V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF/CREATE_BUFS
Hi Hans, On Tue, Nov 20, 2018 at 09:58:43AM +0100, Hans Verkuil wrote: > Add new buffer capability flags to indicate if the VIDIOC_PREPARE_BUF or > VIDIOC_CREATE_BUFS ioctls are supported. Are there practical benefits from the change for the user space? > > Signed-off-by: Hans Verkuil > --- > Note: the flag bits will change since there are two other patches that add > flags, so the numbering will change. > --- > diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst > b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst > index d40c60e8..abf925484aff 100644 > --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst > +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst > @@ -112,6 +112,8 @@ any DMA in progress, an implicit > .. _V4L2-BUF-CAP-SUPPORTS-USERPTR: > .. _V4L2-BUF-CAP-SUPPORTS-DMABUF: > .. _V4L2-BUF-CAP-SUPPORTS-REQUESTS: > +.. _V4L2-BUF-CAP-SUPPORTS-PREPARE-BUF: > +.. _V4L2-BUF-CAP-SUPPORTS-CREATE-BUFS: > > .. cssclass:: longtable > > @@ -132,6 +134,12 @@ any DMA in progress, an implicit > * - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS`` >- 0x0008 >- This buffer type supports :ref:`requests `. > +* - ``V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF`` > + - 0x0010 > + - This buffer type supports :ref:`VIDIOC_PREPARE_BUF`. > +* - ``V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS`` > + - 0x0020 > + - This buffer type supports :ref:`VIDIOC_CREATE_BUFS`. > > Return Value > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c > b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index a17033ab2c22..27c0fafca0bf 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -871,6 +871,16 @@ static inline bool vb2_queue_is_busy(struct video_device > *vdev, struct file *fil > return vdev->queue->owner && vdev->queue->owner != file->private_data; > } > > +static void fill_buf_caps_vdev(struct video_device *vdev, u32 *caps) > +{ > + *caps = 0; > + fill_buf_caps(vdev->queue, caps); > + if (vdev->ioctl_ops->vidioc_prepare_buf) > + *caps |= V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF; > + if (vdev->ioctl_ops->vidioc_create_bufs) > + *caps |= V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS; > +} > + > /* vb2 ioctl helpers */ > > int vb2_ioctl_reqbufs(struct file *file, void *priv, > @@ -879,7 +889,7 @@ int vb2_ioctl_reqbufs(struct file *file, void *priv, > struct video_device *vdev = video_devdata(file); > int res = vb2_verify_memory_type(vdev->queue, p->memory, p->type); > > - fill_buf_caps(vdev->queue, &p->capabilities); > + fill_buf_caps_vdev(vdev, &p->capabilities); > if (res) > return res; > if (vb2_queue_is_busy(vdev, file)) > @@ -901,7 +911,7 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv, > p->format.type); > > p->index = vdev->queue->num_buffers; > - fill_buf_caps(vdev->queue, &p->capabilities); > + fill_buf_caps_vdev(vdev, &p->capabilities); > /* >* If count == 0, then just check if memory and type are valid. >* Any -EBUSY result from vb2_verify_memory_type can be mapped to 0. > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index c8e8ff810190..6648f8ba2277 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -879,6 +879,8 @@ struct v4l2_requestbuffers { > #define V4L2_BUF_CAP_SUPPORTS_USERPTR(1 << 1) > #define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2) > #define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3) Could you align the previous lines to match the ones below? > +#define V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF(1 << 4) > +#define V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS(1 << 5) > > /** > * struct v4l2_plane - plane info for multi-planar buffers -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com
[PATCH] videodev2.h: add V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF/CREATE_BUFS
Add new buffer capability flags to indicate if the VIDIOC_PREPARE_BUF or VIDIOC_CREATE_BUFS ioctls are supported. Signed-off-by: Hans Verkuil --- Note: the flag bits will change since there are two other patches that add flags, so the numbering will change. --- diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst index d40c60e8..abf925484aff 100644 --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst @@ -112,6 +112,8 @@ any DMA in progress, an implicit .. _V4L2-BUF-CAP-SUPPORTS-USERPTR: .. _V4L2-BUF-CAP-SUPPORTS-DMABUF: .. _V4L2-BUF-CAP-SUPPORTS-REQUESTS: +.. _V4L2-BUF-CAP-SUPPORTS-PREPARE-BUF: +.. _V4L2-BUF-CAP-SUPPORTS-CREATE-BUFS: .. cssclass:: longtable @@ -132,6 +134,12 @@ any DMA in progress, an implicit * - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS`` - 0x0008 - This buffer type supports :ref:`requests `. +* - ``V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF`` + - 0x0010 + - This buffer type supports :ref:`VIDIOC_PREPARE_BUF`. +* - ``V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS`` + - 0x0020 + - This buffer type supports :ref:`VIDIOC_CREATE_BUFS`. Return Value diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c index a17033ab2c22..27c0fafca0bf 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -871,6 +871,16 @@ static inline bool vb2_queue_is_busy(struct video_device *vdev, struct file *fil return vdev->queue->owner && vdev->queue->owner != file->private_data; } +static void fill_buf_caps_vdev(struct video_device *vdev, u32 *caps) +{ + *caps = 0; + fill_buf_caps(vdev->queue, caps); + if (vdev->ioctl_ops->vidioc_prepare_buf) + *caps |= V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF; + if (vdev->ioctl_ops->vidioc_create_bufs) + *caps |= V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS; +} + /* vb2 ioctl helpers */ int vb2_ioctl_reqbufs(struct file *file, void *priv, @@ -879,7 +889,7 @@ int vb2_ioctl_reqbufs(struct file *file, void *priv, struct video_device *vdev = video_devdata(file); int res = vb2_verify_memory_type(vdev->queue, p->memory, p->type); - fill_buf_caps(vdev->queue, &p->capabilities); + fill_buf_caps_vdev(vdev, &p->capabilities); if (res) return res; if (vb2_queue_is_busy(vdev, file)) @@ -901,7 +911,7 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv, p->format.type); p->index = vdev->queue->num_buffers; - fill_buf_caps(vdev->queue, &p->capabilities); + fill_buf_caps_vdev(vdev, &p->capabilities); /* * If count == 0, then just check if memory and type are valid. * Any -EBUSY result from vb2_verify_memory_type can be mapped to 0. diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index c8e8ff810190..6648f8ba2277 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -879,6 +879,8 @@ struct v4l2_requestbuffers { #define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1) #define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2) #define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3) +#define V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF (1 << 4) +#define V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS (1 << 5) /** * struct v4l2_plane - plane info for multi-planar buffers