| Message ID | 20240731093526.29137-1-benjamin.gaignard@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] v4l2-compliance: Add test for V4L2_FMTDESC_FLAG_ENUM_ALL flag | expand |
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@collabora.com> > --- > 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. > + 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; > }
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@collabora.com> >> --- >> 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. >> + 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; >> } >
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@collabora.com> >>> --- >>> 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; >>> } >>
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" }, \ { 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; + 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); + 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()) { + 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); return 0; }
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@collabora.com> --- 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(+)