On 31/05/2024 09:03, Nas Chung wrote: > Hi, Hans. > >> -----Original Message----- >> From: Hans Verkuil <hverkuil@xxxxxxxxx> >> Sent: Thursday, May 30, 2024 7:32 PM >> To: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>; mchehab@xxxxxxxxxx; linux- >> media@xxxxxxxxxxxxxxx; sebastian.fricke@xxxxxxxxxxxxx; >> m.tretter@xxxxxxxxxxxxxx >> Cc: linux-kernel@xxxxxxxxxxxxxxx >> 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@xxxxxxxxxxxxxxx> >>> --- >>> 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 >