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?
Regards,
Hans
> +}
> +
> static int testColorspace(bool non_zero_colorspace,
> __u32 pixelformat, __u32 colorspace, __u32 ycbcr_enc, __u32 quantization)
> {
