Re: [PATCH] videodev2.h: add V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF/CREATE_BUFS

2018-11-20 Thread kbuild test robot 
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

2018-11-20 Thread kbuild test robot 
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

2018-11-20 Thread Sakari Ailus 
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

2018-11-20 Thread Hans Verkuil 
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, >capabilities);
>> +fill_buf_caps_vdev(vdev, >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, >capabilities);
>> +fill_buf_caps_vdev(vdev, >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

2018-11-20 Thread Sakari Ailus 
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, >capabilities);
> + fill_buf_caps_vdev(vdev, >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, >capabilities);
> + fill_buf_caps_vdev(vdev, >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

2018-11-20 Thread Hans Verkuil 
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, >capabilities);
+   fill_buf_caps_vdev(vdev, >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, >capabilities);
+   fill_buf_caps_vdev(vdev, >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