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