Re: [PATCH] v4l2-compliance: Add enum all formats test

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux