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

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

 


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

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