On 31/07/2024 09:03, Benjamin Gaignard wrote: > > Le 30/07/2024 à 09:30, Hans Verkuil a écrit : >> Hi Benjamin, >> >> On 22/07/2024 17:06, Benjamin Gaignard wrote: >>> Add a test to check if V4L2_FMT_FLAG_ENUM_ALL is supported >>> or not by drivers. >>> >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx> >>> --- >>> include/linux/videodev2.h | 3 ++ >>> utils/common/v4l2-info.cpp | 1 + >>> utils/v4l2-compliance/v4l2-compliance.cpp | 1 + >>> utils/v4l2-compliance/v4l2-compliance.h | 1 + >>> utils/v4l2-compliance/v4l2-test-formats.cpp | 60 +++++++++++++++++++-- >>> 5 files changed, 63 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h >>> index f18a40d4..8e2a8b36 100644 >>> --- a/include/linux/videodev2.h >>> +++ b/include/linux/videodev2.h >>> @@ -864,6 +864,9 @@ struct v4l2_fmtdesc { >>> #define V4L2_FMT_FLAG_CSC_QUANTIZATION 0x0100 >>> #define V4L2_FMT_FLAG_META_LINE_BASED 0x0200 >>> +/* Format description flag, to be ORed with the index */ >>> +#define V4L2_FMT_FLAG_ENUM_ALL 0x80000000 >>> + >>> /* Frame Size and frame rate enumeration */ >>> /* >>> * F R A M E S I Z E E N U M E R A T I O N >>> diff --git a/utils/common/v4l2-info.cpp b/utils/common/v4l2-info.cpp >>> index aaf7b0b5..c2c49ad0 100644 >>> --- a/utils/common/v4l2-info.cpp >>> +++ b/utils/common/v4l2-info.cpp >>> @@ -383,6 +383,7 @@ static constexpr flag_def fmtdesc_ ## enc_type ## _def[] = { \ >>> { V4L2_FMT_FLAG_CSC_QUANTIZATION, "csc-quantization" }, \ >>> { V4L2_FMT_FLAG_CSC_XFER_FUNC, "csc-xfer-func" }, \ >>> { V4L2_FMT_FLAG_META_LINE_BASED, "meta-line-based" }, \ >>> + { V4L2_FMT_FLAG_ENUM_ALL, "enum-all-formats" }, \ >>> { 0, NULL } \ >>> }; >>> diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp >>> index 144f9618..3446f66f 100644 >>> --- a/utils/v4l2-compliance/v4l2-compliance.cpp >>> +++ b/utils/v4l2-compliance/v4l2-compliance.cpp >>> @@ -1444,6 +1444,7 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ >>> printf("Format ioctls%s:\n", suffix); >>> printf("\ttest VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: %s\n", ok(testEnumFormats(&node))); >>> + printf("\ttest VIDIOC_ENUM_FMT ALL FORMATS: %s\n", ok(testEnumAllFormats(&node))); >> This shouldn't be a new high-level test, instead it should be part of >> testEnumFormats(). >> >>> printf("\ttest VIDIOC_G/S_PARM: %s\n", ok(testParm(&node))); >>> printf("\ttest VIDIOC_G_FBUF: %s\n", ok(testFBuf(&node))); >>> printf("\ttest VIDIOC_G_FMT: %s\n", ok(testGetFormats(&node))); >>> diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h >>> index 948b62fd..be72590f 100644 >>> --- a/utils/v4l2-compliance/v4l2-compliance.h >>> +++ b/utils/v4l2-compliance/v4l2-compliance.h >>> @@ -366,6 +366,7 @@ int testEdid(struct node *node); >>> // Format ioctl tests >>> int testEnumFormats(struct node *node); >>> +int testEnumAllFormats(struct node *node); >>> int testParm(struct node *node); >>> int testFBuf(struct node *node); >>> int testGetFormats(struct node *node); >>> diff --git a/utils/v4l2-compliance/v4l2-test-formats.cpp b/utils/v4l2-compliance/v4l2-test-formats.cpp >>> index fc16ad39..2b9b00ae 100644 >>> --- a/utils/v4l2-compliance/v4l2-test-formats.cpp >>> +++ b/utils/v4l2-compliance/v4l2-test-formats.cpp >>> @@ -221,7 +221,7 @@ static int testEnumFrameSizes(struct node *node, __u32 pixfmt) >>> return 0; >>> } >>> -static int testEnumFormatsType(struct node *node, unsigned type) >>> +static int _testEnumFormatsType(struct node *node, unsigned type, bool enum_all_formats) >>> { >>> pixfmt_map &map = node->buftype_pixfmts[type]; >>> struct v4l2_fmtdesc fmtdesc; >>> @@ -234,6 +234,9 @@ static int testEnumFormatsType(struct node *node, unsigned type) >>> fmtdesc.index = f; >>> fmtdesc.mbus_code = 0; >>> + if (enum_all_formats) >>> + fmtdesc.index |= V4L2_FMT_FLAG_ENUM_ALL; >>> + >>> ret = doioctl(node, VIDIOC_ENUM_FMT, &fmtdesc); >>> if (ret == ENOTTY) >>> return ret; >>> @@ -246,7 +249,7 @@ static int testEnumFormatsType(struct node *node, unsigned type) >>> ret = check_0(fmtdesc.reserved, sizeof(fmtdesc.reserved)); >>> if (ret) >>> return fail("fmtdesc.reserved not zeroed\n"); >>> - if (fmtdesc.index != f) >>> + if ((fmtdesc.index & ~V4L2_FMT_FLAG_ENUM_ALL) != f) >>> return fail("fmtdesc.index was modified\n"); >>> if (fmtdesc.type != type) >>> return fail("fmtdesc.type was modified\n"); >>> @@ -312,7 +315,7 @@ static int testEnumFormatsType(struct node *node, unsigned type) >>> continue; >>> // Update define in v4l2-compliance.h if new buffer types are added >>> assert(type <= V4L2_BUF_TYPE_LAST); >>> - if (map.find(fmtdesc.pixelformat) != map.end()) >>> + if (map.find(fmtdesc.pixelformat) != map.end() && !enum_all_formats) >>> return fail("duplicate format %08x (%s)\n", >>> fmtdesc.pixelformat, fcc2s(fmtdesc.pixelformat).c_str()); >>> map[fmtdesc.pixelformat] = fmtdesc.flags; >>> @@ -321,6 +324,16 @@ static int testEnumFormatsType(struct node *node, unsigned type) >>> return 0; >>> } >>> +static int testEnumFormatsType(struct node *node, unsigned type) >>> +{ >>> + return _testEnumFormatsType(node, type, false); >>> +} >>> + >>> +static int testEnumAllFormatsType(struct node *node, unsigned type) >>> +{ >>> + return _testEnumFormatsType(node, type, true); >>> +} >>> + >>> int testEnumFormats(struct node *node) >>> { >>> bool supported = false; >>> @@ -372,6 +385,47 @@ int testEnumFormats(struct node *node) >>> return supported ? 0 : ENOTTY; >>> } >>> +int testEnumAllFormats(struct node *node) >>> +{ >>> + bool supported = false; >>> + unsigned type; >>> + int ret; >>> + >>> + for (type = 0; type <= V4L2_BUF_TYPE_LAST; type++) { >>> + ret = testEnumAllFormatsType(node, type); >>> + if (ret && ret != ENOTTY) >>> + return ret; >>> + if (!ret) >>> + supported = true; >>> + switch (type) { >>> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: >>> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: >>> + case V4L2_BUF_TYPE_VIDEO_OVERLAY: >>> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: >>> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: >>> + case V4L2_BUF_TYPE_SDR_CAPTURE: >>> + case V4L2_BUF_TYPE_SDR_OUTPUT: >>> + if (supported && ret && (node->g_caps() & buftype2cap[type])) >>> + return fail("%s cap set, but no %s formats defined\n", >>> + buftype2s(type).c_str(), buftype2s(type).c_str()); >>> + if (supported && !ret && !(node->g_caps() & buftype2cap[type])) >>> + return fail("%s cap not set, but %s formats defined\n", >>> + buftype2s(type).c_str(), buftype2s(type).c_str()); >>> + break; >>> + case V4L2_BUF_TYPE_META_CAPTURE: >>> + case V4L2_BUF_TYPE_META_OUTPUT: >>> + /* Metadata formats need not be present for the current input/output */ >>> + break; >>> + default: >>> + if (!ret) >>> + return fail("Buffer type %s not allowed!\n", buftype2s(type).c_str()); >>> + break; >>> + } >>> + } >>> + >>> + return supported ? 0 : ENOTTY; >> This does not test that the set of formats returned with this flag must contain all formats >> returned when ENUM_FMT is called without this flag. I.e., it must be a superset of that. >> >> Also note that testEnumFormatsType() calls testEnumFrameSizes which in turn will call >> testEnumFrameIntervals. So the question is: if ENUM_FMT called with the new flag returns a >> format that would normally be suppressed, and you pass that to VIDIOC_ENUM_FRAMESIZES/INTERVALS, >> what should those ioctls do? Return -EINVAL? Or should it just work? Or leave it up to the >> driver? Shouldn't this be documented? > > I will rework the test. > I think formats enumerated with the flag shouldn't be use for VIDIOC_ENUM_FRAMESIZES/INTERVALS > but the drivers can't know that they have been discovered by using the flag... > I will add in the documentation a word saying that these formats shouldn't be used for > VIDIOC_ENUM_FRAMESIZES/INTERVALS And perhaps add that it will be driver-dependent whether it will return -EINVAL or return valid information. v4l2-compliance should also allow an EINVAL return for pixelformats found when called with the flags, that were not in the set found without the flag. I see that visl supports VIDIOC_ENUM_FRAMESIZES, so I would recommend that visl returns -EINVAL for such pixelformats. That helps develop the v4l2-compliance test code. Regards, Hans > > Regards, > Benjamin > >> >> Regards, >> >> Hans >> >>> +} >>> + >>> static int testColorspace(bool non_zero_colorspace, >>> __u32 pixelformat, __u32 colorspace, __u32 ycbcr_enc, __u32 quantization) >>> { >>