v4l2-compliance tests for cache flags

v4l2-compliance tests for cache flags

[Dave Stevenson]

Hi Hans, Sergey, & the list.

I'm running the latest v4l2-compliance (85ec2917 "v4l2-compliance: fix
stateful encoder tests") against the Raspberry Pi vendor 5.4 kernel
tree. That means the kernel isn't supporting
V4L2_BUF_FLAG_NO_CACHE_[INVALIDATE|CLEAN] flags on the buffers, nor is
anything advertising V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS. I'm
getting failures in setupMmap.

With patch "784be6a v4l2-utils: test cache_hints for MMAP queues" the
test deliberately sets V4L2_BUF_FLAG_NO_CACHE_INVALIDATE and
V4L2_BUF_FLAG_NO_CACHE_CLEAN on the buffers that it queues.
Nothing in the kernel is expecting to manipulate those flags as the
behaviour wasn't really defined before, but the test is failed if they
aren't cleared. The v4l2 core would only clear them if the kernel
includes the patch "f5f5fa7 media: videobuf2: handle V4L2 buffer cache
flags", which currently means only linuxtv/master as that patch isn't
even in 5.8-rc3.

Is it really valid to look at the V4L2_BUF_FLAG_NO_CACHE_* flags at
all if V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS isn't set? If the
capability isn't advertised, surely the state of those bits is
undefined.

Thanks
  Dave

Re: v4l2-compliance tests for cache flags

[Hans Verkuil]

On 07/07/2020 15:33, Dave Stevenson wrote:
> Hi Hans, Sergey, & the list.
> 
> I'm running the latest v4l2-compliance (85ec2917 "v4l2-compliance: fix
> stateful encoder tests") against the Raspberry Pi vendor 5.4 kernel
> tree. That means the kernel isn't supporting
> V4L2_BUF_FLAG_NO_CACHE_[INVALIDATE|CLEAN] flags on the buffers, nor is
> anything advertising V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS. I'm
> getting failures in setupMmap.
> 
> With patch "784be6a v4l2-utils: test cache_hints for MMAP queues" the
> test deliberately sets V4L2_BUF_FLAG_NO_CACHE_INVALIDATE and
> V4L2_BUF_FLAG_NO_CACHE_CLEAN on the buffers that it queues.
> Nothing in the kernel is expecting to manipulate those flags as the
> behaviour wasn't really defined before, but the test is failed if they
> aren't cleared. The v4l2 core would only clear them if the kernel
> includes the patch "f5f5fa7 media: videobuf2: handle V4L2 buffer cache
> flags", which currently means only linuxtv/master as that patch isn't
> even in 5.8-rc3.
> 
> Is it really valid to look at the V4L2_BUF_FLAG_NO_CACHE_* flags at
> all if V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS isn't set? If the
> capability isn't advertised, surely the state of those bits is
> undefined.

v4l2-compliance tests are in sync with our master and it expects that
that kernel is used.

It is possible in rare cases to add a version check (use the version
field returned by QUERYCAP) to determine if a feature is in the latest
kernel. It's not done anywhere in the current compliance test, but I'd
accept a patch that disables this test for kernels < 5.9 (since this feature
will land in 5.9).

Regards,

	Hans

Re: v4l2-compliance tests for cache flags

[Dave Stevenson]

Hi Hans

On Tue, 7 Jul 2020 at 14:49, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 07/07/2020 15:33, Dave Stevenson wrote:
> > Hi Hans, Sergey, & the list.
> >
> > I'm running the latest v4l2-compliance (85ec2917 "v4l2-compliance: fix
> > stateful encoder tests") against the Raspberry Pi vendor 5.4 kernel
> > tree. That means the kernel isn't supporting
> > V4L2_BUF_FLAG_NO_CACHE_[INVALIDATE|CLEAN] flags on the buffers, nor is
> > anything advertising V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS. I'm
> > getting failures in setupMmap.
> >
> > With patch "784be6a v4l2-utils: test cache_hints for MMAP queues" the
> > test deliberately sets V4L2_BUF_FLAG_NO_CACHE_INVALIDATE and
> > V4L2_BUF_FLAG_NO_CACHE_CLEAN on the buffers that it queues.
> > Nothing in the kernel is expecting to manipulate those flags as the
> > behaviour wasn't really defined before, but the test is failed if they
> > aren't cleared. The v4l2 core would only clear them if the kernel
> > includes the patch "f5f5fa7 media: videobuf2: handle V4L2 buffer cache
> > flags", which currently means only linuxtv/master as that patch isn't
> > even in 5.8-rc3.
> >
> > Is it really valid to look at the V4L2_BUF_FLAG_NO_CACHE_* flags at
> > all if V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS isn't set? If the
> > capability isn't advertised, surely the state of those bits is
> > undefined.
>
> v4l2-compliance tests are in sync with our master and it expects that
> that kernel is used.

Thanks, I'd never noted that restriction. All previous times I'd used
v4l2-compliance against any kernel it had performed as expected. This
is the first change that causes a major failure of tests due to an
older kernel.

> It is possible in rare cases to add a version check (use the version
> field returned by QUERYCAP) to determine if a feature is in the latest
> kernel. It's not done anywhere in the current compliance test, but I'd
> accept a patch that disables this test for kernels < 5.9 (since this feature
> will land in 5.9).

OK, I'll try to sort out a patch for you.

  Dave

> Regards,
>
>         Hans

Re: v4l2-compliance tests for cache flags

[Sergey Senozhatsky]

On (20/07/07 15:41), Dave Stevenson wrote:
> > v4l2-compliance tests are in sync with our master and it expects that
> > that kernel is used.
> 
> Thanks, I'd never noted that restriction. All previous times I'd used
> v4l2-compliance against any kernel it had performed as expected. This
> is the first change that causes a major failure of tests due to an
> older kernel.

It depends on Linux UAPI headers, so the restriction is sort of mandated,
but probably not widely recognized the by the distributions, looking at
arch, for instance:
https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/v4l-utils

Adding a Linux version check code can be a bit intrusive. There has been
a whole bunch of changes all over the place, for instance:

---

+++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
@@ -381,8 +381,6 @@ int buffer::check(unsigned type, unsigned memory, unsigned index,
        if (g_flags() & V4L2_BUF_FLAG_BFRAME)
                frame_types++;
        fail_on_test(frame_types > 1);
-       fail_on_test(g_flags() & (V4L2_BUF_FLAG_NO_CACHE_INVALIDATE |
-                                 V4L2_BUF_FLAG_NO_CACHE_CLEAN));
---

So running newer v4l-compliance against the older kernel or older
v4l-compliance against the newer kernel may trigger various failures.

	-ss

Re: v4l2-compliance tests for cache flags

[Hans Verkuil]

On 08/07/2020 05:59, Sergey Senozhatsky wrote:
> On (20/07/07 15:41), Dave Stevenson wrote:
>>> v4l2-compliance tests are in sync with our master and it expects that
>>> that kernel is used.
>>
>> Thanks, I'd never noted that restriction. All previous times I'd used
>> v4l2-compliance against any kernel it had performed as expected. This
>> is the first change that causes a major failure of tests due to an
>> older kernel.
> 
> It depends on Linux UAPI headers, so the restriction is sort of mandated,
> but probably not widely recognized the by the distributions, looking at
> arch, for instance:
> https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/v4l-utils
> 
> Adding a Linux version check code can be a bit intrusive. There has been
> a whole bunch of changes all over the place, for instance:
> 
> ---
> 
> +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
> @@ -381,8 +381,6 @@ int buffer::check(unsigned type, unsigned memory, unsigned index,
>         if (g_flags() & V4L2_BUF_FLAG_BFRAME)
>                 frame_types++;
>         fail_on_test(frame_types > 1);
> -       fail_on_test(g_flags() & (V4L2_BUF_FLAG_NO_CACHE_INVALIDATE |
> -                                 V4L2_BUF_FLAG_NO_CACHE_CLEAN));
> ---
> 
> So running newer v4l-compliance against the older kernel or older
> v4l-compliance against the newer kernel may trigger various failures.

It shouldn't. It's (I believe) just the check that those two flags
are cleared if cache hints are not supported that should be under a
version check.

Regards,

	Hans

Re: v4l2-compliance tests for cache flags

[Dave Stevenson]

Hi Sergey & Hans

On Wed, 8 Jul 2020 at 09:22, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 08/07/2020 05:59, Sergey Senozhatsky wrote:
> > On (20/07/07 15:41), Dave Stevenson wrote:
> >>> v4l2-compliance tests are in sync with our master and it expects that
> >>> that kernel is used.
> >>
> >> Thanks, I'd never noted that restriction. All previous times I'd used
> >> v4l2-compliance against any kernel it had performed as expected. This
> >> is the first change that causes a major failure of tests due to an
> >> older kernel.
> >
> > It depends on Linux UAPI headers, so the restriction is sort of mandated,
> > but probably not widely recognized the by the distributions, looking at
> > arch, for instance:
> > https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/v4l-utils

Except that v4l-utils has a private copy of all the headers in
https://git.linuxtv.org/v4l-utils.git/tree/include/linux, so it builds
independently of the distribution's kernel headers.

> > Adding a Linux version check code can be a bit intrusive. There has been
> > a whole bunch of changes all over the place, for instance:
> >
> > ---
> >
> > +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
> > @@ -381,8 +381,6 @@ int buffer::check(unsigned type, unsigned memory, unsigned index,
> >         if (g_flags() & V4L2_BUF_FLAG_BFRAME)
> >                 frame_types++;
> >         fail_on_test(frame_types > 1);
> > -       fail_on_test(g_flags() & (V4L2_BUF_FLAG_NO_CACHE_INVALIDATE |
> > -                                 V4L2_BUF_FLAG_NO_CACHE_CLEAN));
> > ---
> >
> > So running newer v4l-compliance against the older kernel or older
> > v4l-compliance against the newer kernel may trigger various failures.
>
> It shouldn't. It's (I believe) just the check that those two flags
> are cleared if cache hints are not supported that should be under a
> version check.

Indeed, the only change I need to make for the tests to work is to
disable the two lines in the else clause:

--- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
+++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
@@ -1272,7 +1272,7 @@ static int setupMmap(struct node *node, cv4l_queue &q)
                if (cache_hints) {
                        fail_on_test(!(flags &
V4L2_BUF_FLAG_NO_CACHE_INVALIDATE));
                        fail_on_test(!(flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN));
-               } else {
+               } else if (0) {
                        fail_on_test(flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE);
                        fail_on_test(flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN);
                }

I started looking at kernel_version, but currently it only gets set
for 2.6.x kernels, and is set to the x.

Hans, would you be happy with taking the kernel's KERNEL_VERSION macro
and using it to encode the whole version, eg

diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp
b/utils/v4l2-compliance/v4l2-compliance.cpp
index d0441651..c372a15c 100644
--- a/utils/v4l2-compliance/v4l2-compliance.cpp
+++ b/utils/v4l2-compliance/v4l2-compliance.cpp
@@ -1497,10 +1497,9 @@ int main(int argc, char **argv)
        printf(", %zd bits, %zd-bit time_t\n", sizeof(void *) * 8,
sizeof(time_t) * 8);
        uname(&uts);
        sscanf(uts.release, "%d.%d.%d", &v1, &v2, &v3);
-       if (v1 == 2 && v2 == 6)
-               kernel_version = v3;
+       kernel_version = KERNEL_VERSION(v1, v2, v3);
        if (kernel_version)
-               printf("Running on 2.6.%d\n", kernel_version);
+               printf("Running on %d.%d.%d\n", v1, v2, v3);
        printf("\n");

        if (!env_media_apps_color || !strcmp(env_media_apps_color, "auto"))
diff --git a/utils/v4l2-compliance/v4l2-compliance.h
b/utils/v4l2-compliance/v4l2-compliance.h
index ae10bdf9..657f5a2a 100644
--- a/utils/v4l2-compliance/v4l2-compliance.h
+++ b/utils/v4l2-compliance/v4l2-compliance.h
@@ -61,6 +61,8 @@ extern int kernel_version;
 extern int media_fd;
 extern unsigned warnings;

+#define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))
+
 enum poll_mode {
        POLL_MODE_NONE,
        POLL_MODE_SELECT,
diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp
b/utils/v4l2-compliance/v4l2-test-buffers.cpp
index b0b878c1..7e6fb30e 100644
--- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
+++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
@@ -1272,7 +1272,7 @@ static int setupMmap(struct node *node, cv4l_queue &q)
                if (cache_hints) {
                        fail_on_test(!(flags &
V4L2_BUF_FLAG_NO_CACHE_INVALIDATE));
                        fail_on_test(!(flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN));
-               } else {
+               } else if (kernel_version >= KERNEL_VERSION(5, 9, 0)) {
                        fail_on_test(flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE);
                        fail_on_test(flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN);
                }
diff --git a/utils/v4l2-compliance/v4l2-test-controls.cpp
b/utils/v4l2-compliance/v4l2-test-controls.cpp
index d81dddb2..dde661ed 100644
--- a/utils/v4l2-compliance/v4l2-test-controls.cpp
+++ b/utils/v4l2-compliance/v4l2-test-controls.cpp
@@ -411,7 +411,8 @@ int testSimpleControls(struct node *node)
                        ctrl.value = 0;
                        // This call will crash on kernels <= 2.6.37
for control classes due to
                        // a bug in v4l2-ctrls.c. So skip this on those kernels.
-                       if (kernel_version < 38 && qctrl.type ==
V4L2_CTRL_TYPE_CTRL_CLASS)
+                       if (kernel_version < KERNEL_VERSION(2, 6, 38) &&
+                           qctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS)
                                ret = EACCES;
                        else
                                ret = doioctl(node, VIDIOC_S_CTRL, &ctrl);

Actually isn't the current test of kernel_version in
v4l2-test-controls.cpp wrong?
If running on a 3.x, 4.x, or 5.x kernel then kernel_version will be
left at 0. 0 < 38, so we fall into the ret = EACESS; clause and skip
the test.
Is that what was desired? Presumably the bug in v4l2-ctls.c referenced
is fixed in all >= 3.x kernels. I obviously don't want to alter the
behaviour in an incorrect manner here.

Cheers
  Dave

Re: v4l2-compliance tests for cache flags

[Hans Verkuil]

On 08/07/2020 11:30, Dave Stevenson wrote:
> Hi Sergey & Hans
> 
> On Wed, 8 Jul 2020 at 09:22, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 08/07/2020 05:59, Sergey Senozhatsky wrote:
>>> On (20/07/07 15:41), Dave Stevenson wrote:
>>>>> v4l2-compliance tests are in sync with our master and it expects that
>>>>> that kernel is used.
>>>>
>>>> Thanks, I'd never noted that restriction. All previous times I'd used
>>>> v4l2-compliance against any kernel it had performed as expected. This
>>>> is the first change that causes a major failure of tests due to an
>>>> older kernel.
>>>
>>> It depends on Linux UAPI headers, so the restriction is sort of mandated,
>>> but probably not widely recognized the by the distributions, looking at
>>> arch, for instance:
>>> https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/v4l-utils
> 
> Except that v4l-utils has a private copy of all the headers in
> https://git.linuxtv.org/v4l-utils.git/tree/include/linux, so it builds
> independently of the distribution's kernel headers.
> 
>>> Adding a Linux version check code can be a bit intrusive. There has been
>>> a whole bunch of changes all over the place, for instance:
>>>
>>> ---
>>>
>>> +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
>>> @@ -381,8 +381,6 @@ int buffer::check(unsigned type, unsigned memory, unsigned index,
>>>         if (g_flags() & V4L2_BUF_FLAG_BFRAME)
>>>                 frame_types++;
>>>         fail_on_test(frame_types > 1);
>>> -       fail_on_test(g_flags() & (V4L2_BUF_FLAG_NO_CACHE_INVALIDATE |
>>> -                                 V4L2_BUF_FLAG_NO_CACHE_CLEAN));
>>> ---
>>>
>>> So running newer v4l-compliance against the older kernel or older
>>> v4l-compliance against the newer kernel may trigger various failures.
>>
>> It shouldn't. It's (I believe) just the check that those two flags
>> are cleared if cache hints are not supported that should be under a
>> version check.
> 
> Indeed, the only change I need to make for the tests to work is to
> disable the two lines in the else clause:
> 
> --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
> +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
> @@ -1272,7 +1272,7 @@ static int setupMmap(struct node *node, cv4l_queue &q)
>                 if (cache_hints) {
>                         fail_on_test(!(flags &
> V4L2_BUF_FLAG_NO_CACHE_INVALIDATE));
>                         fail_on_test(!(flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN));
> -               } else {
> +               } else if (0) {
>                         fail_on_test(flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE);
>                         fail_on_test(flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN);
>                 }
> 
> I started looking at kernel_version, but currently it only gets set
> for 2.6.x kernels, and is set to the x.

Try this patch:

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp
index d0441651..e32b57e3 100644
--- a/utils/v4l2-compliance/v4l2-compliance.cpp
+++ b/utils/v4l2-compliance/v4l2-compliance.cpp
@@ -637,6 +637,8 @@ static int testCap(struct node *node)
 		warn("media bus_info '%s' differs from V4L2 bus_info '%s'\n",
 		     node->media_bus_info.c_str(), vcap.bus_info);
 	fail_on_test((vcap.version >> 16) < 3);
+	if (vcap.version >= 0x050900)  // Present from 5.9.0 onwards
+		node->might_support_cache_hints = true;
 	fail_on_test(check_0(vcap.reserved, sizeof(vcap.reserved)));
 	caps = vcap.capabilities;
 	dcaps = vcap.device_caps;
diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h
index ae10bdf9..38a4ded3 100644
--- a/utils/v4l2-compliance/v4l2-compliance.h
+++ b/utils/v4l2-compliance/v4l2-compliance.h
@@ -146,6 +146,8 @@ struct base_node {
 	__u32 valid_buftype;
 	__u32 valid_memorytype;
 	bool supports_orphaned_bufs;
+	// support for this was introduced in 5.9
+	bool might_support_cache_hints;
 };

 struct node : public base_node, public cv4l_fd {
diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp
index b0b878c1..cdfbbd34 100644
--- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
+++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
@@ -1272,7 +1272,7 @@ static int setupMmap(struct node *node, cv4l_queue &q)
 		if (cache_hints) {
 			fail_on_test(!(flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE));
 			fail_on_test(!(flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN));
-		} else {
+		} else if (node->might_support_cache_hints) {
 			fail_on_test(flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE);
 			fail_on_test(flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN);
 		}

This will work as well for people who use the media_build system to backport
the media subsystem from our tree to an older kernel.

> 
> Hans, would you be happy with taking the kernel's KERNEL_VERSION macro
> and using it to encode the whole version, eg

Not worth the effort, but if we need more of such checks, then I might change
my mind :-)

<snip>

> diff --git a/utils/v4l2-compliance/v4l2-test-controls.cpp
> b/utils/v4l2-compliance/v4l2-test-controls.cpp
> index d81dddb2..dde661ed 100644
> --- a/utils/v4l2-compliance/v4l2-test-controls.cpp
> +++ b/utils/v4l2-compliance/v4l2-test-controls.cpp
> @@ -411,7 +411,8 @@ int testSimpleControls(struct node *node)
>                         ctrl.value = 0;
>                         // This call will crash on kernels <= 2.6.37
> for control classes due to
>                         // a bug in v4l2-ctrls.c. So skip this on those kernels.
> -                       if (kernel_version < 38 && qctrl.type ==
> V4L2_CTRL_TYPE_CTRL_CLASS)
> +                       if (kernel_version < KERNEL_VERSION(2, 6, 38) &&
> +                           qctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS)
>                                 ret = EACCES;
>                         else
>                                 ret = doioctl(node, VIDIOC_S_CTRL, &ctrl);
> 
> Actually isn't the current test of kernel_version in
> v4l2-test-controls.cpp wrong?
> If running on a 3.x, 4.x, or 5.x kernel then kernel_version will be
> left at 0. 0 < 38, so we fall into the ret = EACESS; clause and skip
> the test.
> Is that what was desired? Presumably the bug in v4l2-ctls.c referenced
> is fixed in all >= 3.x kernels. I obviously don't want to alter the
> behaviour in an incorrect manner here.

Yup, I'm about to commit a patch that removes kernel_version and kills
this workaround in testSimpleControls(). I really don't care about such
old kernels.

Thanks for reporting this, good catch!

Regards,

	Hans

> 
> Cheers
>   Dave
> 

Re: v4l2-compliance tests for cache flags

[Dave Stevenson]

On Wed, 8 Jul 2020 at 12:40, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 08/07/2020 11:30, Dave Stevenson wrote:
> > Hi Sergey & Hans
> >
> > On Wed, 8 Jul 2020 at 09:22, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 08/07/2020 05:59, Sergey Senozhatsky wrote:
> >>> On (20/07/07 15:41), Dave Stevenson wrote:
> >>>>> v4l2-compliance tests are in sync with our master and it expects that
> >>>>> that kernel is used.
> >>>>
> >>>> Thanks, I'd never noted that restriction. All previous times I'd used
> >>>> v4l2-compliance against any kernel it had performed as expected. This
> >>>> is the first change that causes a major failure of tests due to an
> >>>> older kernel.
> >>>
> >>> It depends on Linux UAPI headers, so the restriction is sort of mandated,
> >>> but probably not widely recognized the by the distributions, looking at
> >>> arch, for instance:
> >>> https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/v4l-utils
> >
> > Except that v4l-utils has a private copy of all the headers in
> > https://git.linuxtv.org/v4l-utils.git/tree/include/linux, so it builds
> > independently of the distribution's kernel headers.
> >
> >>> Adding a Linux version check code can be a bit intrusive. There has been
> >>> a whole bunch of changes all over the place, for instance:
> >>>
> >>> ---
> >>>
> >>> +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
> >>> @@ -381,8 +381,6 @@ int buffer::check(unsigned type, unsigned memory, unsigned index,
> >>>         if (g_flags() & V4L2_BUF_FLAG_BFRAME)
> >>>                 frame_types++;
> >>>         fail_on_test(frame_types > 1);
> >>> -       fail_on_test(g_flags() & (V4L2_BUF_FLAG_NO_CACHE_INVALIDATE |
> >>> -                                 V4L2_BUF_FLAG_NO_CACHE_CLEAN));
> >>> ---
> >>>
> >>> So running newer v4l-compliance against the older kernel or older
> >>> v4l-compliance against the newer kernel may trigger various failures.
> >>
> >> It shouldn't. It's (I believe) just the check that those two flags
> >> are cleared if cache hints are not supported that should be under a
> >> version check.
> >
> > Indeed, the only change I need to make for the tests to work is to
> > disable the two lines in the else clause:
> >
> > --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
> > +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
> > @@ -1272,7 +1272,7 @@ static int setupMmap(struct node *node, cv4l_queue &q)
> >                 if (cache_hints) {
> >                         fail_on_test(!(flags &
> > V4L2_BUF_FLAG_NO_CACHE_INVALIDATE));
> >                         fail_on_test(!(flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN));
> > -               } else {
> > +               } else if (0) {
> >                         fail_on_test(flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE);
> >                         fail_on_test(flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN);
> >                 }
> >
> > I started looking at kernel_version, but currently it only gets set
> > for 2.6.x kernels, and is set to the x.
>
> Try this patch:

Tried and works perfectly for me - thanks.

Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp
> index d0441651..e32b57e3 100644
> --- a/utils/v4l2-compliance/v4l2-compliance.cpp
> +++ b/utils/v4l2-compliance/v4l2-compliance.cpp
> @@ -637,6 +637,8 @@ static int testCap(struct node *node)
>                 warn("media bus_info '%s' differs from V4L2 bus_info '%s'\n",
>                      node->media_bus_info.c_str(), vcap.bus_info);
>         fail_on_test((vcap.version >> 16) < 3);
> +       if (vcap.version >= 0x050900)  // Present from 5.9.0 onwards
> +               node->might_support_cache_hints = true;
>         fail_on_test(check_0(vcap.reserved, sizeof(vcap.reserved)));
>         caps = vcap.capabilities;
>         dcaps = vcap.device_caps;
> diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h
> index ae10bdf9..38a4ded3 100644
> --- a/utils/v4l2-compliance/v4l2-compliance.h
> +++ b/utils/v4l2-compliance/v4l2-compliance.h
> @@ -146,6 +146,8 @@ struct base_node {
>         __u32 valid_buftype;
>         __u32 valid_memorytype;
>         bool supports_orphaned_bufs;
> +       // support for this was introduced in 5.9
> +       bool might_support_cache_hints;
>  };
>
>  struct node : public base_node, public cv4l_fd {
> diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp
> index b0b878c1..cdfbbd34 100644
> --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
> +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
> @@ -1272,7 +1272,7 @@ static int setupMmap(struct node *node, cv4l_queue &q)
>                 if (cache_hints) {
>                         fail_on_test(!(flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE));
>                         fail_on_test(!(flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN));
> -               } else {
> +               } else if (node->might_support_cache_hints) {
>                         fail_on_test(flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE);
>                         fail_on_test(flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN);
>                 }
>
> This will work as well for people who use the media_build system to backport
> the media subsystem from our tree to an older kernel.
>
> >
> > Hans, would you be happy with taking the kernel's KERNEL_VERSION macro
> > and using it to encode the whole version, eg
>
> Not worth the effort, but if we need more of such checks, then I might change
> my mind :-)

That's why I wanted to ask for your preference on implementation!

> <snip>
>
> > diff --git a/utils/v4l2-compliance/v4l2-test-controls.cpp
> > b/utils/v4l2-compliance/v4l2-test-controls.cpp
> > index d81dddb2..dde661ed 100644
> > --- a/utils/v4l2-compliance/v4l2-test-controls.cpp
> > +++ b/utils/v4l2-compliance/v4l2-test-controls.cpp
> > @@ -411,7 +411,8 @@ int testSimpleControls(struct node *node)
> >                         ctrl.value = 0;
> >                         // This call will crash on kernels <= 2.6.37
> > for control classes due to
> >                         // a bug in v4l2-ctrls.c. So skip this on those kernels.
> > -                       if (kernel_version < 38 && qctrl.type ==
> > V4L2_CTRL_TYPE_CTRL_CLASS)
> > +                       if (kernel_version < KERNEL_VERSION(2, 6, 38) &&
> > +                           qctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS)
> >                                 ret = EACCES;
> >                         else
> >                                 ret = doioctl(node, VIDIOC_S_CTRL, &ctrl);
> >
> > Actually isn't the current test of kernel_version in
> > v4l2-test-controls.cpp wrong?
> > If running on a 3.x, 4.x, or 5.x kernel then kernel_version will be
> > left at 0. 0 < 38, so we fall into the ret = EACESS; clause and skip
> > the test.
> > Is that what was desired? Presumably the bug in v4l2-ctls.c referenced
> > is fixed in all >= 3.x kernels. I obviously don't want to alter the
> > behaviour in an incorrect manner here.
>
> Yup, I'm about to commit a patch that removes kernel_version and kills
> this workaround in testSimpleControls(). I really don't care about such
> old kernels.
>
> Thanks for reporting this, good catch!

No problem.

  Dave