[PATCH 0/3] v4l2 framework minor improvements

[PATCH 0/3] v4l2 framework minor improvements

[Helen Koike]

Just minor things.
Add capabilities to v4l_print_create_buffers(), clarify docs and remove
a redundant check.

Helen Koike (3):
  media: v4l2-ioctl: print capabilities in v4l_print_create_buffers()
  media: videodev2.h: clarify v4l2_pix_format_mplane.sizeimage docs when
    to set to zero
  media: videobuf2-v4l2: remove redundant error test

 drivers/media/common/videobuf2/videobuf2-v4l2.c | 5 -----
 drivers/media/v4l2-core/v4l2-ioctl.c            | 6 +++---
 include/uapi/linux/videodev2.h                  | 1 +
 3 files changed, 4 insertions(+), 8 deletions(-)

-- 
2.29.2

[PATCH 1/3] media: v4l2-ioctl: print capabilities in v4l_print_create_buffers()

[Helen Koike]

Print capabilities field from struct v4l2_create_buffers for better
debugging.

Signed-off-by: Helen Koike <helen.koike@collabora.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 3198abdd538c..848286a284f6 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -518,9 +518,9 @@ static void v4l_print_create_buffers(const void *arg, bool write_only)
 {
 	const struct v4l2_create_buffers *p = arg;
 
-	pr_cont("index=%d, count=%d, memory=%s, ",
-			p->index, p->count,
-			prt_names(p->memory, v4l2_memory_names));
+	pr_cont("index=%d, count=%d, memory=%s, capabilities=0x%08x, ",
+		p->index, p->count, prt_names(p->memory, v4l2_memory_names),
+		p->capabilities);
 	v4l_print_format(&p->format, write_only);
 }
 
-- 
2.29.2

[PATCH 2/3] media: videodev2.h: clarify v4l2_pix_format_mplane.sizeimage docs when to set to zero

[Helen Koike]

sizeimage field should be set to zero for unused planes, even when
v4l2_pix_format_mplane.num_planes is smaller then the index of planes.

Signed-off-by: Helen Koike <helen.koike@collabora.com>

---

I caught this with v4l2-compliance, which throws an error if we dirty
planes, even if invalid, so I would like to make it clear in the docs.
---
 include/uapi/linux/videodev2.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 79dbde3bcf8d..d9b7c9177605 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2227,6 +2227,7 @@ struct v4l2_mpeg_vbi_fmt_ivtv {
  * struct v4l2_plane_pix_format - additional, per-plane format definition
  * @sizeimage:		maximum size in bytes required for data, for which
  *			this plane will be used
+ *			Drivers should be set it zero for unused planes.
  * @bytesperline:	distance in bytes between the leftmost pixels in two
  *			adjacent lines
  */
-- 
2.29.2

Re: [PATCH 2/3] media: videodev2.h: clarify v4l2_pix_format_mplane.sizeimage docs when to set to zero

[Jacopo Mondi]

Hi Helen

On Thu, Jan 14, 2021 at 03:01:48PM -0300, Helen Koike wrote:
> sizeimage field should be set to zero for unused planes, even when
> v4l2_pix_format_mplane.num_planes is smaller then the index of planes.
>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>
> ---
>
> I caught this with v4l2-compliance, which throws an error if we dirty
> planes, even if invalid, so I would like to make it clear in the docs.
> ---
>  include/uapi/linux/videodev2.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 79dbde3bcf8d..d9b7c9177605 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2227,6 +2227,7 @@ struct v4l2_mpeg_vbi_fmt_ivtv {
>   * struct v4l2_plane_pix_format - additional, per-plane format definition
>   * @sizeimage:		maximum size in bytes required for data, for which
>   *			this plane will be used
> + *			Drivers should be set it zero for unused planes.

s/should be/should/ ?

And possibly a full stop at the end of the previous sentence

Thanks
  j

>   * @bytesperline:	distance in bytes between the leftmost pixels in two
>   *			adjacent lines
>   */
> --
> 2.29.2
>

Re: [PATCH 2/3] media: videodev2.h: clarify v4l2_pix_format_mplane.sizeimage docs when to set to zero

[Sakari Ailus]

Hi Helen,

Thanks for the patch.

On Thu, Jan 14, 2021 at 03:01:48PM -0300, Helen Koike wrote:
> sizeimage field should be set to zero for unused planes, even when
> v4l2_pix_format_mplane.num_planes is smaller then the index of planes.
> 
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> 
> I caught this with v4l2-compliance, which throws an error if we dirty
> planes, even if invalid, so I would like to make it clear in the docs.

Would it be possible to do this in the framework instead?

> ---
>  include/uapi/linux/videodev2.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 79dbde3bcf8d..d9b7c9177605 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2227,6 +2227,7 @@ struct v4l2_mpeg_vbi_fmt_ivtv {
>   * struct v4l2_plane_pix_format - additional, per-plane format definition
>   * @sizeimage:		maximum size in bytes required for data, for which
>   *			this plane will be used
> + *			Drivers should be set it zero for unused planes.
>   * @bytesperline:	distance in bytes between the leftmost pixels in two
>   *			adjacent lines
>   */

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/3] media: videodev2.h: clarify v4l2_pix_format_mplane.sizeimage docs when to set to zero

[Hans Verkuil]

On 14/01/2021 19:01, Helen Koike wrote:
> sizeimage field should be set to zero for unused planes, even when
> v4l2_pix_format_mplane.num_planes is smaller then the index of planes.

then -> than

> 
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> 
> I caught this with v4l2-compliance, which throws an error if we dirty
> planes, even if invalid, so I would like to make it clear in the docs.

What is the error? And with which driver?

I wonder if this isn't a v4l2-compliance bug. And if we want this to be
zeroed, then it wouldn't it be better to do that in the V4L2 core rather
than bother drivers with this?

> ---
>  include/uapi/linux/videodev2.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 79dbde3bcf8d..d9b7c9177605 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2227,6 +2227,7 @@ struct v4l2_mpeg_vbi_fmt_ivtv {
>   * struct v4l2_plane_pix_format - additional, per-plane format definition
>   * @sizeimage:		maximum size in bytes required for data, for which
>   *			this plane will be used
> + *			Drivers should be set it zero for unused planes.

This sentence is a bit garbled.

You probably meant: Drivers must set this to zero for unused planes.

But it makes no sense to just zero this field. I would zero the whole struct
contents for the unused planes.

>   * @bytesperline:	distance in bytes between the leftmost pixels in two
>   *			adjacent lines
>   */
> 

The API doesn't mention whether unused plane formats should be zeroed or not,
but it does make sense that they are. I don't think that the userspace API
should be changed (esp. since there are apparently already drivers that do
not zero these unused plane formats), but it makes sense that the compliance
test does verify this, and that the V4L2 core would zero unused plane formats.

I never like it when undefined values are allowed in an API, so it makes sense
that this is done.

Regards,

	Hans

Re: [PATCH 2/3] media: videodev2.h: clarify v4l2_pix_format_mplane.sizeimage docs when to set to zero

[Helen Koike]

On 1/25/21 6:31 AM, Hans Verkuil wrote:
> On 14/01/2021 19:01, Helen Koike wrote:
>> sizeimage field should be set to zero for unused planes, even when
>> v4l2_pix_format_mplane.num_planes is smaller then the index of planes.
> 
> then -> than

Ack.

> 
>>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>>
>> I caught this with v4l2-compliance, which throws an error if we dirty
>> planes, even if invalid, so I would like to make it clear in the docs.
> 
> What is the error? And with which driver?

I was implementing conversions to/from Ext API, and I thought v4l2-compliance
wasn't happy if I didn't zero the other entries, but I'm trying to reproduce
it now by adding a non-zero value to sizeimage and I can't reproduce it, so
it was probably my mistake.
Please ignore this patch and sorry for the noise.

> 
> I wonder if this isn't a v4l2-compliance bug. And if we want this to be
> zeroed, then it wouldn't it be better to do that in the V4L2 core rather
> than bother drivers with this?
> 
>> ---
>>  include/uapi/linux/videodev2.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 79dbde3bcf8d..d9b7c9177605 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -2227,6 +2227,7 @@ struct v4l2_mpeg_vbi_fmt_ivtv {
>>   * struct v4l2_plane_pix_format - additional, per-plane format definition
>>   * @sizeimage:		maximum size in bytes required for data, for which
>>   *			this plane will be used
>> + *			Drivers should be set it zero for unused planes.
> 
> This sentence is a bit garbled.
> 
> You probably meant: Drivers must set this to zero for unused planes.
> 
> But it makes no sense to just zero this field. I would zero the whole struct
> contents for the unused planes.
> 
>>   * @bytesperline:	distance in bytes between the leftmost pixels in two
>>   *			adjacent lines
>>   */
>>
> 
> The API doesn't mention whether unused plane formats should be zeroed or not,
> but it does make sense that they are. I don't think that the userspace API
> should be changed (esp. since there are apparently already drivers that do
> not zero these unused plane formats), but it makes sense that the compliance
> test does verify this, and that the V4L2 core would zero unused plane formats.
> 
> I never like it when undefined values are allowed in an API, so it makes sense
> that this is done.


Ack.

Thanks
Helen


> 
> Regards,
> 
> 	Hans
> 

[PATCH 3/3] media: videobuf2-v4l2: remove redundant error test

[Helen Koike]

request_fd is validated under media_request_get_by_fd() just below this
check. Thus remove it.

Suggested-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Helen Koike <helen.koike@collabora.com>
---
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 96d3b2b2aa31..bb642c0775d1 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -488,11 +488,6 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
 		    !q->ops->buf_out_validate))
 		return -EINVAL;
 
-	if (b->request_fd < 0) {
-		dprintk(q, 1, "%s: request_fd < 0\n", opname);
-		return -EINVAL;
-	}
-
 	req = media_request_get_by_fd(mdev, b->request_fd);
 	if (IS_ERR(req)) {
 		dprintk(q, 1, "%s: invalid request_fd\n", opname);
-- 
2.29.2

Re: [PATCH 3/3] media: videobuf2-v4l2: remove redundant error test

[Sakari Ailus]

On Thu, Jan 14, 2021 at 03:01:49PM -0300, Helen Koike wrote:
> request_fd is validated under media_request_get_by_fd() just below this
> check. Thus remove it.
> 
> Suggested-by: Tomasz Figa <tfiga@chromium.org>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus