| Message ID | 20240528020425.4994-1-nas.chung@chipsnmedia.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] media: uapi: v4l: Change V4L2_TYPE_IS_CAPTURE condition | expand |
On 28/05/2024 04:04, Nas Chung wrote: > Explicitly compare a buffer type only with valid buffer types, > to avoid matching the buffer type outside of valid buffer > type set. > > Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com> > --- > include/uapi/linux/videodev2.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index fe6b67e83751..fa2b7086e480 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -157,6 +157,10 @@ enum v4l2_buf_type { > V4L2_BUF_TYPE_PRIVATE = 0x80, > }; > > +#define V4L2_TYPE_IS_VALID(type) \ > + ((type) >= V4L2_BUF_TYPE_VIDEO_CAPTURE \ > + && (type) <= V4L2_BUF_TYPE_META_OUTPUT) > + > #define V4L2_TYPE_IS_MULTIPLANAR(type) \ > ((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE \ > || (type) == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > @@ -171,7 +175,8 @@ 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) \ > + (V4L2_TYPE_IS_VALID(type) && !V4L2_TYPE_IS_OUTPUT(type)) > > enum v4l2_tuner_type { > V4L2_TUNER_RADIO = 1, This patch introduced this warning: In function 'find_format_by_index', inlined from 'vdec_enum_fmt' at drivers/media/platform/qcom/venus/vdec.c:452:8: drivers/media/platform/qcom/venus/vdec.c:172:32: warning: 'valid' may be used uninitialized [-Wmaybe-uninitialized] 172 | if (k == index && valid) | ~~~~~~~~~~~^~~~~~~~ drivers/media/platform/qcom/venus/vdec.c: In function 'vdec_enum_fmt': drivers/media/platform/qcom/venus/vdec.c:157:22: note: 'valid' was declared here 157 | bool valid; | ^~~~~ This driver does this: bool valid; if (V4L2_TYPE_IS_OUTPUT(type)) { valid = venus_helper_check_codec(inst, fmt[i].pixfmt); } else if (V4L2_TYPE_IS_CAPTURE(type)) { valid = venus_helper_check_format(inst, fmt[i].pixfmt); With your patch both V4L2_TYPE_IS_OUTPUT(type) and V4L2_TYPE_IS_CAPTURE(type) can return false, something that wasn't possible without this patch. In this driver the fix would just be to drop the second 'if' altogether, so just '} else {'. Since these defines are part of the public API, this change introduces a subtle behavior difference that can affect applications. That said, I do consider this an improvement. I would like some changes, though: 1) Just after "V4L2_BUF_TYPE_META_OUTPUT = 14," in enum v4l2_buf_type, add a comment saying that V4L2_TYPE_IS_VALID and (for output types) V4L2_TYPE_IS_OUTPUT must be updated if a new type is added. It's all too easy to forget that otherwise. 2) Add a patch fixing the venus/vdec.c code. 3) Something else I noticed, but I think this change should be in a separate patch: V4L2_TYPE_IS_OUTPUT() returns true for V4L2_BUF_TYPE_VIDEO_OVERLAY, but that definitely belongs to CAPTURE. Nobody really uses that type anymore, but still, it should be fixed. Regards, Hans
Hi, Hans. >-----Original Message----- >From: Hans Verkuil <hverkuil@xs4all.nl> >Sent: Thursday, May 30, 2024 7:32 PM >To: Nas Chung <nas.chung@chipsnmedia.com>; mchehab@kernel.org; linux- >media@vger.kernel.org; sebastian.fricke@collabora.com; >m.tretter@pengutronix.de >Cc: linux-kernel@vger.kernel.org >Subject: Re: [PATCH v2] media: uapi: v4l: Change V4L2_TYPE_IS_CAPTURE >condition > >On 28/05/2024 04:04, Nas Chung wrote: >> Explicitly compare a buffer type only with valid buffer types, >> to avoid matching the buffer type outside of valid buffer >> type set. >> >> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com> >> --- >> include/uapi/linux/videodev2.h | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/include/uapi/linux/videodev2.h >b/include/uapi/linux/videodev2.h >> index fe6b67e83751..fa2b7086e480 100644 >> --- a/include/uapi/linux/videodev2.h >> +++ b/include/uapi/linux/videodev2.h >> @@ -157,6 +157,10 @@ enum v4l2_buf_type { >> V4L2_BUF_TYPE_PRIVATE = 0x80, >> }; >> >> +#define V4L2_TYPE_IS_VALID(type) \ >> + ((type) >= V4L2_BUF_TYPE_VIDEO_CAPTURE \ >> + && (type) <= V4L2_BUF_TYPE_META_OUTPUT) >> + >> #define V4L2_TYPE_IS_MULTIPLANAR(type) \ >> ((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE \ >> || (type) == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >> @@ -171,7 +175,8 @@ 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) \ >> + (V4L2_TYPE_IS_VALID(type) && !V4L2_TYPE_IS_OUTPUT(type)) >> >> enum v4l2_tuner_type { >> V4L2_TUNER_RADIO = 1, > >This patch introduced this warning: > >In function 'find_format_by_index', > inlined from 'vdec_enum_fmt' at >drivers/media/platform/qcom/venus/vdec.c:452:8: >drivers/media/platform/qcom/venus/vdec.c:172:32: warning: 'valid' may be >used uninitialized [-Wmaybe-uninitialized] > 172 | if (k == index && valid) > | ~~~~~~~~~~~^~~~~~~~ >drivers/media/platform/qcom/venus/vdec.c: In function 'vdec_enum_fmt': >drivers/media/platform/qcom/venus/vdec.c:157:22: note: 'valid' was >declared here > 157 | bool valid; > | ^~~~~ > >This driver does this: > > bool valid; > > if (V4L2_TYPE_IS_OUTPUT(type)) { > valid = venus_helper_check_codec(inst, fmt[i].pixfmt); > } else if (V4L2_TYPE_IS_CAPTURE(type)) { > valid = venus_helper_check_format(inst, fmt[i].pixfmt); > >With your patch both V4L2_TYPE_IS_OUTPUT(type) and >V4L2_TYPE_IS_CAPTURE(type) >can return false, something that wasn't possible without this patch. > >In this driver the fix would just be to drop the second 'if' altogether, >so just >'} else {'. > >Since these defines are part of the public API, this change introduces a >subtle >behavior difference that can affect applications. Thank you for a detailed description. > >That said, I do consider this an improvement. > >I would like some changes, though: > >1) Just after "V4L2_BUF_TYPE_META_OUTPUT = 14," in enum >v4l2_buf_type, > add a comment saying that V4L2_TYPE_IS_VALID and (for output types) > V4L2_TYPE_IS_OUTPUT must be updated if a new type is added. It's all > too easy to forget that otherwise. >2) Add a patch fixing the venus/vdec.c code. 1), 2) I will address both in v3. >3) Something else I noticed, but I think this change should be in a >separate patch: > V4L2_TYPE_IS_OUTPUT() returns true for V4L2_BUF_TYPE_VIDEO_OVERLAY, but >that > definitely belongs to CAPTURE. Nobody really uses that type anymore, >but still, > it should be fixed. 3) Would you prefer this modification to be included as a separate patch in the v3 of this patch series, or should it be submitted as a new standalone patch ? Thanks. Nas. > >Regards, > > Hans
Hey Nas, just before you send out V3 ... On 28.05.2024 11:04, Nas Chung wrote: >Explicitly compare a buffer type only with valid buffer types, >to avoid matching the buffer type outside of valid buffer >type set. s/matching the buffer type outside of valid buffer type set/ matching a buffer type outside of the valid buffer type set/ Regards, Sebastian >Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com> >--- > include/uapi/linux/videodev2.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > >diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >index fe6b67e83751..fa2b7086e480 100644 >--- a/include/uapi/linux/videodev2.h >+++ b/include/uapi/linux/videodev2.h >@@ -157,6 +157,10 @@ enum v4l2_buf_type { > V4L2_BUF_TYPE_PRIVATE = 0x80, > }; > >+#define V4L2_TYPE_IS_VALID(type) \ >+ ((type) >= V4L2_BUF_TYPE_VIDEO_CAPTURE \ >+ && (type) <= V4L2_BUF_TYPE_META_OUTPUT) >+ > #define V4L2_TYPE_IS_MULTIPLANAR(type) \ > ((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE \ > || (type) == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >@@ -171,7 +175,8 @@ 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) \ >+ (V4L2_TYPE_IS_VALID(type) && !V4L2_TYPE_IS_OUTPUT(type)) > > enum v4l2_tuner_type { > V4L2_TUNER_RADIO = 1, >-- >2.25.1 >
On 31/05/2024 09:03, Nas Chung wrote: > Hi, Hans. > >> -----Original Message----- >> From: Hans Verkuil <hverkuil@xs4all.nl> >> Sent: Thursday, May 30, 2024 7:32 PM >> To: Nas Chung <nas.chung@chipsnmedia.com>; mchehab@kernel.org; linux- >> media@vger.kernel.org; sebastian.fricke@collabora.com; >> m.tretter@pengutronix.de >> Cc: linux-kernel@vger.kernel.org >> Subject: Re: [PATCH v2] media: uapi: v4l: Change V4L2_TYPE_IS_CAPTURE >> condition >> >> On 28/05/2024 04:04, Nas Chung wrote: >>> Explicitly compare a buffer type only with valid buffer types, >>> to avoid matching the buffer type outside of valid buffer >>> type set. >>> >>> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com> >>> --- >>> include/uapi/linux/videodev2.h | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/uapi/linux/videodev2.h >> b/include/uapi/linux/videodev2.h >>> index fe6b67e83751..fa2b7086e480 100644 >>> --- a/include/uapi/linux/videodev2.h >>> +++ b/include/uapi/linux/videodev2.h >>> @@ -157,6 +157,10 @@ enum v4l2_buf_type { >>> V4L2_BUF_TYPE_PRIVATE = 0x80, >>> }; >>> >>> +#define V4L2_TYPE_IS_VALID(type) \ >>> + ((type) >= V4L2_BUF_TYPE_VIDEO_CAPTURE \ >>> + && (type) <= V4L2_BUF_TYPE_META_OUTPUT) >>> + >>> #define V4L2_TYPE_IS_MULTIPLANAR(type) \ >>> ((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE \ >>> || (type) == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >>> @@ -171,7 +175,8 @@ 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) \ >>> + (V4L2_TYPE_IS_VALID(type) && !V4L2_TYPE_IS_OUTPUT(type)) >>> >>> enum v4l2_tuner_type { >>> V4L2_TUNER_RADIO = 1, >> >> This patch introduced this warning: >> >> In function 'find_format_by_index', >> inlined from 'vdec_enum_fmt' at >> drivers/media/platform/qcom/venus/vdec.c:452:8: >> drivers/media/platform/qcom/venus/vdec.c:172:32: warning: 'valid' may be >> used uninitialized [-Wmaybe-uninitialized] >> 172 | if (k == index && valid) >> | ~~~~~~~~~~~^~~~~~~~ >> drivers/media/platform/qcom/venus/vdec.c: In function 'vdec_enum_fmt': >> drivers/media/platform/qcom/venus/vdec.c:157:22: note: 'valid' was >> declared here >> 157 | bool valid; >> | ^~~~~ >> >> This driver does this: >> >> bool valid; >> >> if (V4L2_TYPE_IS_OUTPUT(type)) { >> valid = venus_helper_check_codec(inst, fmt[i].pixfmt); >> } else if (V4L2_TYPE_IS_CAPTURE(type)) { >> valid = venus_helper_check_format(inst, fmt[i].pixfmt); >> >> With your patch both V4L2_TYPE_IS_OUTPUT(type) and >> V4L2_TYPE_IS_CAPTURE(type) >> can return false, something that wasn't possible without this patch. >> >> In this driver the fix would just be to drop the second 'if' altogether, >> so just >> '} else {'. >> >> Since these defines are part of the public API, this change introduces a >> subtle >> behavior difference that can affect applications. > > Thank you for a detailed description. > >> >> That said, I do consider this an improvement. >> >> I would like some changes, though: >> >> 1) Just after "V4L2_BUF_TYPE_META_OUTPUT = 14," in enum >> v4l2_buf_type, >> add a comment saying that V4L2_TYPE_IS_VALID and (for output types) >> V4L2_TYPE_IS_OUTPUT must be updated if a new type is added. It's all >> too easy to forget that otherwise. > >> 2) Add a patch fixing the venus/vdec.c code. > > 1), 2) I will address both in v3. > >> 3) Something else I noticed, but I think this change should be in a >> separate patch: >> V4L2_TYPE_IS_OUTPUT() returns true for V4L2_BUF_TYPE_VIDEO_OVERLAY, but >> that >> definitely belongs to CAPTURE. Nobody really uses that type anymore, >> but still, >> it should be fixed. > > 3) Would you prefer this modification to be included as a separate patch > in the v3 of this patch series, or should it be submitted as a new > standalone patch ? Separate patch in this series, please. Regards, Hans > > Thanks. > Nas. > >> >> Regards, >> >> Hans >
Hi Sebastian. >-----Original Message----- >From: Sebastian Fricke <sebastian.fricke@collabora.com> >Sent: Friday, May 31, 2024 4:26 PM >To: Nas Chung <nas.chung@chipsnmedia.com> >Cc: mchehab@kernel.org; linux-media@vger.kernel.org; >m.tretter@pengutronix.de; linux-kernel@vger.kernel.org >Subject: Re: [PATCH v2] media: uapi: v4l: Change V4L2_TYPE_IS_CAPTURE >condition > >Hey Nas, > >just before you send out V3 ... > >On 28.05.2024 11:04, Nas Chung wrote: >>Explicitly compare a buffer type only with valid buffer types, >>to avoid matching the buffer type outside of valid buffer >>type set. > >s/matching the buffer type outside of valid buffer type set/ > matching a buffer type outside of the valid buffer type set/ Thank you for the review! I will fix it in v3. Thanks. Nas. > >Regards, >Sebastian > >>Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com> >>--- >> include/uapi/linux/videodev2.h | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >>diff --git a/include/uapi/linux/videodev2.h >b/include/uapi/linux/videodev2.h >>index fe6b67e83751..fa2b7086e480 100644 >>--- a/include/uapi/linux/videodev2.h >>+++ b/include/uapi/linux/videodev2.h >>@@ -157,6 +157,10 @@ enum v4l2_buf_type { >> V4L2_BUF_TYPE_PRIVATE = 0x80, >> }; >> >>+#define V4L2_TYPE_IS_VALID(type) \ >>+ ((type) >= V4L2_BUF_TYPE_VIDEO_CAPTURE \ >>+ && (type) <= V4L2_BUF_TYPE_META_OUTPUT) >>+ >> #define V4L2_TYPE_IS_MULTIPLANAR(type) \ >> ((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE \ >> || (type) == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >>@@ -171,7 +175,8 @@ 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) \ >>+ (V4L2_TYPE_IS_VALID(type) && !V4L2_TYPE_IS_OUTPUT(type)) >> >> enum v4l2_tuner_type { >> V4L2_TUNER_RADIO = 1, >>-- >>2.25.1 >>
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index fe6b67e83751..fa2b7086e480 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -157,6 +157,10 @@ enum v4l2_buf_type { V4L2_BUF_TYPE_PRIVATE = 0x80, }; +#define V4L2_TYPE_IS_VALID(type) \ + ((type) >= V4L2_BUF_TYPE_VIDEO_CAPTURE \ + && (type) <= V4L2_BUF_TYPE_META_OUTPUT) + #define V4L2_TYPE_IS_MULTIPLANAR(type) \ ((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE \ || (type) == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) @@ -171,7 +175,8 @@ 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) \ + (V4L2_TYPE_IS_VALID(type) && !V4L2_TYPE_IS_OUTPUT(type)) enum v4l2_tuner_type { V4L2_TUNER_RADIO = 1,
Explicitly compare a buffer type only with valid buffer types, to avoid matching the buffer type outside of valid buffer type set. Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com> --- include/uapi/linux/videodev2.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)