[v1] media: uapi: v4l: Change V4L2_TYPE_IS_CAPTURE condition

[PATCH] media: uapi: v4l: Change V4L2_TYPE_IS_CAPTURE condition

Posted by Nas Chung 1 year, 8 months ago

We expect V4L2_TYPE_IS_CAPTURE() macro allow only CAPTURE type.
But, Inverting OUTPUT type can allow undefined v4l2_buf_type.
Check CAPTURE type directly instead of inverting OUTPUT type.

Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
 include/uapi/linux/videodev2.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index fe6b67e83751..32b10e2b7695 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -171,7 +171,13 @@ enum v4l2_buf_type {
 	 || (type) == V4L2_BUF_TYPE_SDR_OUTPUT			\
 	 || (type) == V4L2_BUF_TYPE_META_OUTPUT)
 
-#define V4L2_TYPE_IS_CAPTURE(type) (!V4L2_TYPE_IS_OUTPUT(type))
+#define V4L2_TYPE_IS_CAPTURE(type)				\
+	((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE			\
+	 || (type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE	\
+	 || (type) == V4L2_BUF_TYPE_VBI_CAPTURE			\
+	 || (type) == V4L2_BUF_TYPE_SLICED_VBI_CAPTURE		\
+	 || (type) == V4L2_BUF_TYPE_SDR_CAPTURE			\
+	 || (type) == V4L2_BUF_TYPE_META_CAPTURE)
 
 enum v4l2_tuner_type {
 	V4L2_TUNER_RADIO	     = 1,
-- 
2.25.1

Re: [PATCH] media: uapi: v4l: Change V4L2_TYPE_IS_CAPTURE condition

Posted by Michael Tretter 1 year, 8 months ago

On Fri, 17 May 2024 18:49:40 +0900, Nas Chung wrote:
> We expect V4L2_TYPE_IS_CAPTURE() macro allow only CAPTURE type.
> But, Inverting OUTPUT type can allow undefined v4l2_buf_type.
> Check CAPTURE type directly instead of inverting OUTPUT type.
> 
> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
> ---
>  include/uapi/linux/videodev2.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index fe6b67e83751..32b10e2b7695 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -171,7 +171,13 @@ enum v4l2_buf_type {
>  	 || (type) == V4L2_BUF_TYPE_SDR_OUTPUT			\
>  	 || (type) == V4L2_BUF_TYPE_META_OUTPUT)
>  
> -#define V4L2_TYPE_IS_CAPTURE(type) (!V4L2_TYPE_IS_OUTPUT(type))
> +#define V4L2_TYPE_IS_CAPTURE(type)				\
> +	((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE			\
> +	 || (type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE	\
> +	 || (type) == V4L2_BUF_TYPE_VBI_CAPTURE			\
> +	 || (type) == V4L2_BUF_TYPE_SLICED_VBI_CAPTURE		\
> +	 || (type) == V4L2_BUF_TYPE_SDR_CAPTURE			\
> +	 || (type) == V4L2_BUF_TYPE_META_CAPTURE)

Maybe adding a V4L2_TYPE_IS_VALID(type) macro would be helpful to define
TYPE_IS_CAPTURE as all valid types that are not OUTPUT:

	#define V4L2_TYPE_IS_VALID(type) \
		((type) >= V4L2_BUF_TYPE_VIDEO_CAPTURE \
		&& (type) <= V4L2_BUF_TYPE_META_OUTPUT)

	#define V4L2_TYPE_IS_CAPTURE(type) \
		(V4L2_TYPE_IS_VALID(type) && !V4L2_TYPE_IS_OUTPUT(type))

This would avoid keeping the two explicit lists of OUTPUT and CAPTURE
types.

Michael

>  
>  enum v4l2_tuner_type {
>  	V4L2_TUNER_RADIO	     = 1,
> -- 
> 2.25.1
> 
> 
>

Re: [PATCH] media: uapi: v4l: Change V4L2_TYPE_IS_CAPTURE condition

Posted by Sebastian Fricke 1 year, 8 months ago

Hey Nas,

thanks for the patch, I think making the macro more explicit is
generally a good idea, but in this case all !OUTPUT are actually CAPTURE
types (besides the one deprecated type) and when I look at the
definitions of some of the set commands like S_FMT, I can see that they
require a type as parameter.
So, could you explain in the commit message, how it can happen that the
buf_type is undefined? And if so maybe that case should be fixed
instead?
I have improved your commit message below, but please explain why this
is needed, e.g. which case did you hit where you found an undefined
buffer.

On 17.05.2024 18:49, Nas Chung wrote:
>We expect V4L2_TYPE_IS_CAPTURE() macro allow only CAPTURE type.
>But, Inverting OUTPUT type can allow undefined v4l2_buf_type.
>Check CAPTURE type directly instead of inverting OUTPUT type.

My suggestion for this commit message:

"""
Explicitly compare the type of the buffer with the available CAPTURE
buffer types, to avoid matching a buffer type outside of the valid
buffer type set.
"""

Basically fixing the sentence structure and grammar and focusing more on
the reason of your action instead of describing what the code does
(which should hopefully be obvious in most cases)

I hope that helps :)

Regards,
Sebastian

>
>Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
>---
> include/uapi/linux/videodev2.h | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
>diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>index fe6b67e83751..32b10e2b7695 100644
>--- a/include/uapi/linux/videodev2.h
>+++ b/include/uapi/linux/videodev2.h
>@@ -171,7 +171,13 @@ enum v4l2_buf_type {
> 	 || (type) == V4L2_BUF_TYPE_SDR_OUTPUT			\
> 	 || (type) == V4L2_BUF_TYPE_META_OUTPUT)
>
>-#define V4L2_TYPE_IS_CAPTURE(type) (!V4L2_TYPE_IS_OUTPUT(type))
>+#define V4L2_TYPE_IS_CAPTURE(type)				\
>+	((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE			\
>+	 || (type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE	\
>+	 || (type) == V4L2_BUF_TYPE_VBI_CAPTURE			\
>+	 || (type) == V4L2_BUF_TYPE_SLICED_VBI_CAPTURE		\
>+	 || (type) == V4L2_BUF_TYPE_SDR_CAPTURE			\
>+	 || (type) == V4L2_BUF_TYPE_META_CAPTURE)
>
> enum v4l2_tuner_type {
> 	V4L2_TUNER_RADIO	     = 1,
>-- 
>2.25.1
>
>