v4l2-compliance: Add enum all formats test

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

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