| Message ID | 20240722150604.149707-1-benjamin.gaignard@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | v4l2-compliance: Add enum all formats test | expand |
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@collabora.com> > --- > 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) > {
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@collabora.com> >> --- >> 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 Regards, Benjamin > > Regards, > > Hans > >> +} >> + >> static int testColorspace(bool non_zero_colorspace, >> __u32 pixelformat, __u32 colorspace, __u32 ycbcr_enc, __u32 quantization) >> { >
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@collabora.com> >>> --- >>> 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) >>> { >>
Le 31/07/2024 à 09:10, Hans Verkuil a écrit : > 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@collabora.com> >>>> --- >>>> 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. Only visl capture queue will support the flag and add one pixel format when enumeration occurs. That will not change the driver behavior so it will returns -EINVAL when using this format. Regards, Benjamin > > Regards, > > Hans > >> Regards, >> Benjamin >> >>> Regards, >>> >>> Hans >>> >>>> +} >>>> + >>>> static int testColorspace(bool non_zero_colorspace, >>>> __u32 pixelformat, __u32 colorspace, __u32 ycbcr_enc, __u32 quantization) >>>> { >
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))); 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; +} + static int testColorspace(bool non_zero_colorspace, __u32 pixelformat, __u32 colorspace, __u32 ycbcr_enc, __u32 quantization) {
Add a test to check if V4L2_FMT_FLAG_ENUM_ALL is supported or not by drivers. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> --- 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(-)