On 31/07/2024 12:21, Benjamin Gaignard wrote:
>
> Le 31/07/2024 à 12:02, Hans Verkuil a écrit :
>> On 31/07/2024 11:35, Benjamin Gaignard wrote:
>>> If V4L2_FMTDESC_FLAG_ENUM_ALL flag is supported, test if all
>>> pixel formats list with VIDIOC_ENUM_FMT without the flag been set
>>> is a subset of the list created with the flag.
>>> Also Test that the flag is cleared after calling VIDIOC_ENUM_FMT.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx>
>>> ---
>>> changes in version 2:
>>> - Completely rework how the test it done.
>>>
>>> include/linux/videodev2.h | 3 ++
>>> utils/common/v4l2-info.cpp | 1 +
>>> utils/v4l2-compliance/v4l2-test-formats.cpp | 36 +++++++++++++++++++++
>>> 3 files changed, 40 insertions(+)
>>>
>>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
>>> index f18a40d4..c166bb35 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_FMTDESC_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..248ab9c3 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_FMTDESC_FLAG_ENUM_ALL, "enum-all-format" }, \
>> Ah, no. This lists the possible flag of the flags field, V4L2_FMTDESC_FLAG_ENUM_ALL
>> doesn't belong there.
>>
>> In fact, there is no need to add it at all since it is never reported.
>>
>> But you do need to add support for it in v4l2-ctl: I think that the various
>> --list-formats(-ext) options needs to check for an optional suboption 'all',
>> which will set the new V4L2_FMTDESC_FLAG_ENUM_ALL flag. That's a separate patch
>> for v4l2-ctl.
>>
>>> { 0, NULL } \
>>> };
>>> diff --git a/utils/v4l2-compliance/v4l2-test-formats.cpp b/utils/v4l2-compliance/v4l2-test-formats.cpp
>>> index fc16ad39..2b743da5 100644
>>> --- a/utils/v4l2-compliance/v4l2-test-formats.cpp
>>> +++ b/utils/v4l2-compliance/v4l2-test-formats.cpp
>>> @@ -224,6 +224,7 @@ static int testEnumFrameSizes(struct node *node, __u32 pixfmt)
>>> static int testEnumFormatsType(struct node *node, unsigned type)
>>> {
>>> pixfmt_map &map = node->buftype_pixfmts[type];
>>> + pixfmt_map enum_all;
>>> struct v4l2_fmtdesc fmtdesc;
>>> unsigned f = 0;
>>> int ret;
>>> @@ -317,6 +318,41 @@ static int testEnumFormatsType(struct node *node, unsigned type)
>>> fmtdesc.pixelformat, fcc2s(fmtdesc.pixelformat).c_str());
>>> map[fmtdesc.pixelformat] = fmtdesc.flags;
>>> }
>>> +
>>> + /* Test V4L2_FMTDESC_FLAG_ENUM_ALL if supported */
>>> + f = 0;
>>> + for (;;) {
>>> + memset(&fmtdesc, 0xff, sizeof(fmtdesc));
>>> + fmtdesc.type = type;
>>> + fmtdesc.index = f | V4L2_FMTDESC_FLAG_ENUM_ALL;
>>> + fmtdesc.mbus_code = 0;
>>> +
>>> + ret = doioctl(node, VIDIOC_ENUM_FMT, &fmtdesc);
>>> + if (ret == ENOTTY)
>>> + return ret;
>> This would be a failure: it can't return ENOTTY since that was already tested
>> in the first test without the ALL flag. You can drop this test, since it will
>> fail later.
>>
>>> + if (f == 0 && ret == EINVAL)
>>> + return 0;
>>> + if (ret == EINVAL)
>>> + break;
>>> + if (ret)
>>> + return fail("expected EINVAL, but got %d when enumerating buftype %d\n", ret, type);
>> This will catch the ENOTTY.
>>
>>> + if (fmtdesc.index != f)
>>> + return fail("V4L2_FMTDESC_FLAG_ENUM_ALL hasn't been cleared from fmtdesc.index 0x%x f 0x%x\n", fmtdesc.index, f);
>>> + f++;
>>> + if (type == V4L2_BUF_TYPE_PRIVATE)
>>> + continue;
>>> + assert(type <= V4L2_BUF_TYPE_LAST);
>>> + if (enum_all.find(fmtdesc.pixelformat) != enum_all.end())
>>> + return fail("duplicate format %08x (%s)\n",
>>> + fmtdesc.pixelformat, fcc2s(fmtdesc.pixelformat).c_str());
>>> + enum_all[fmtdesc.pixelformat] = fmtdesc.flags;
>>> + }
>>> + /* if V4L2_FMTDESC_FLAG_ENUM_ALL is supported, verify that the list is a subset of VIDIOC_ENUM_FMT list */
>>> + if (!enum_all.empty()) {
>> You can drop this if. If enum_all is empty, and map isn't, then it will just
>> fail in the next for loop.
>
> enum_all could be empty if V4L2_FMTDESC_FLAG_ENUM_ALL isn't supported and
> it shouldn't fail in this case.
But that is caught by the "if (f == 0 && ret == EINVAL)" check above. In that case
it just returns 0.
Regards,
Hans
>
>>> + for (auto it = map.begin(); it != map.end(); it++)
>>> + if (enum_all.find(it->first) == enum_all.end())
>>> + return fail("V4L2_FMTDESC_FLAG_ENUM_ALL failed to enumerate format %08x (%s)\n", it->first, fcc2s(it->first).c_str());
>>> + }
>>> info("found %d formats for buftype %d\n", f, type);
>> Note that f was reset above, so this info message should be moved up to just before the
>> V4L2_FMTDESC_FLAG_ENUM_ALL test starts.
>>
>> But perhaps keep this info message here as well, just change it to:
>>
>> info("found %d formats for buftype %d (with V4L2_FMTDESC_FLAG_ENUM_ALL)\n", f, type);
>>
>> Regards,
>>
>> Hans
>>
>>> return 0;
>>> }
>>
