[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-media
Subject:    RE: [PATCH] media: uapi: v4l: Change V4L2_TYPE_IS_CAPTURE condition
From:       Nas Chung <nas.chung () chipsnmedia ! com>
Date:       2024-05-17 12:33:00
Message-ID: SL2P216MB12466F44DD7777E65B4F7CD3FBEE2 () SL2P216MB1246 ! KORP216 ! PROD ! OUTLOOK ! COM
[Download RAW message or body]

Hi, Sebastian.

Thanks for the prompt reply.

>-----Original Message-----
>From: Sebastian Fricke <sebastian.fricke@collabora.com>
>Sent: Friday, May 17, 2024 7:10 PM
>To: Nas Chung <nas.chung@chipsnmedia.com>
>Cc: mchehab@kernel.org; linux-media@vger.kernel.org; linux-
>kernel@vger.kernel.org
>Subject: Re: [PATCH] media: uapi: v4l: Change V4L2_TYPE_IS_CAPTURE
>condition
>
>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?

v4l2-compliance test G_SELECTION ioctl with invalid buffer type.
testBasicSelection()
{
	// Check handling of invalid type.
	sel.type = 0xff;
	fail_on_test(doioctl(node, VIDIOC_G_SELECTION, &sel) != EINVAL);
	// Check handling of invalid target.
}

In v4l2 driver, I'm trying to replace IF clause for buffer type checking to helper macro.
But, If I use V4L2_TYPE_IS_CAPTURE in g_selection() ioctl_ops function,
It cannot pass the above test case.

And, Buffer type is set by host. We cannot ensure host always set valid buffer type.

So, I think we can prevent any potential bugs.
Or checking valid buffer type in v4l_g_selection() is another option.

>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.
>"""

Much better! Thanks.

Thanks.
Nas.

>
>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
>>
>>

[prev in list] [next in list] [prev in thread] [next in thread]