(no subject)

[Thomas Devoogdt]

Hi all,


I have two questions:

1.
When running v4l2-compliance on a proprietary driver, I get this error:

```
Input/Output configuration ioctls:
fail: v4l2-test-io-config.cpp(227): fmt.fmt.pix.width >=
enumtimings.timings.bt.width * 1.5
fail: v4l2-test-io-config.cpp(386): Timings check failed for input 0.
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: FAIL
```

Which brings me here:
https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-io-config.cpp#n227

```
fail_on_test(fmt.fmt.pix.width < enumtimings.timings.bt.width);
fail_on_test(fmt.fmt.pix.width >= enumtimings.timings.bt.width * 1.5);
fail_on_test(fmt.fmt.pix.height * factor < enumtimings.timings.bt.height);
fail_on_test(fmt.fmt.pix.height * factor >=
enumtimings.timings.bt.height * 1.5);
```

The problem is that the driver supports VIDIOC_S_DV_TIMINGS but is not
able to apply the specific format just returns the actual timings.
This is from what I know, not a violation, so I'm not sure what the
driver should return if it is not able to set a custom timing. Or is
this check just a bit too strict?


2.
For another driver, I get this error:

```
Input ioctls:
fail: v4l2-test-input-output.cpp(443): use of deprecated digital video status
fail: v4l2-test-input-output.cpp(496): invalid attributes for input 0
test VIDIOC_G/S/ENUMINPUT: FAIL
```

It might be true that setting V4L2_IN_ST_NO_CARRIER is deprecated, but
is that really an error, not better to just have it as a warning?
After all, how can userspace know specific details if needed? Perhaps
checking that V4L2_IN_ST_NO_SIGNAL has also been set is enough, and if
V4L2_IN_ST_NO_CARRIER is set, but not V4L2_IN_ST_NO_SIGNAL, then it
might be an error.

Thx in advance!

Kr,

Thomas Devoogdt

Re:

[Hans Verkuil]

On 22/11/2023 10:26, Thomas Devoogdt wrote:
> Hi all,
> 
> 
> I have two questions:
> 
> 1.
> When running v4l2-compliance on a proprietary driver, I get this error:
> 
> ```
> Input/Output configuration ioctls:
> fail: v4l2-test-io-config.cpp(227): fmt.fmt.pix.width >=
> enumtimings.timings.bt.width * 1.5
> fail: v4l2-test-io-config.cpp(386): Timings check failed for input 0.
> test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: FAIL
> ```
> 
> Which brings me here:
> https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-io-config.cpp#n227
> 
> ```
> fail_on_test(fmt.fmt.pix.width < enumtimings.timings.bt.width);
> fail_on_test(fmt.fmt.pix.width >= enumtimings.timings.bt.width * 1.5);
> fail_on_test(fmt.fmt.pix.height * factor < enumtimings.timings.bt.height);
> fail_on_test(fmt.fmt.pix.height * factor >=
> enumtimings.timings.bt.height * 1.5);
> ```
> 
> The problem is that the driver supports VIDIOC_S_DV_TIMINGS but is not
> able to apply the specific format just returns the actual timings.
> This is from what I know, not a violation, so I'm not sure what the
> driver should return if it is not able to set a custom timing. Or is
> this check just a bit too strict?

This test enumerates all timings returned by VIDIOC_ENUM_DV_TIMINGS.
All these timings should be valid supported timings. For each returned
timing it calls S_DV_TIMINGS to set it. Then it call G_DV_TIMINGS to
verify that the width and height of the new timings match what was
requested. Apparently that worked, otherwise the test would have
failed there.

Next it gets the new format (G_FMT). The driver must update the format
when the timings are updated. In this case the width of the returned
format is more than 1.5 times the width of the requested timings.
That doesn't sound right.

Your claim that S_DV_TIMINGS can't set the new timings is dubious
since G_DV_TIMINGS apparently returns the new timings correctly.
Also, ENUM_DV_TIMINGS shouldn't return unsupported timings either.

> 
> 
> 2.
> For another driver, I get this error:
> 
> ```
> Input ioctls:
> fail: v4l2-test-input-output.cpp(443): use of deprecated digital video status
> fail: v4l2-test-input-output.cpp(496): invalid attributes for input 0
> test VIDIOC_G/S/ENUMINPUT: FAIL
> ```
> 
> It might be true that setting V4L2_IN_ST_NO_CARRIER is deprecated, but
> is that really an error, not better to just have it as a warning?
> After all, how can userspace know specific details if needed? Perhaps
> checking that V4L2_IN_ST_NO_SIGNAL has also been set is enough, and if
> V4L2_IN_ST_NO_CARRIER is set, but not V4L2_IN_ST_NO_SIGNAL, then it
> might be an error.

The NO_CARRIER, NO_EQU and NO_ACCESS status flags are DVB specific, but this
ioctl isn't used by DVB anymore. I think very old drivers in the past (long
since removed or changed) used this, but new drivers must not use this since
it makes no sense. Note that v4l2-compliance is often more strict than the
V4L2 spec itself: it is meant to ensure that drivers follow best practices.

Note that the error message is not very good: it should say "digital TV"
rather than "digital video". I'll fix that since that's confusing.

Regards,

	Hans

> 
> Thx in advance!
> 
> Kr,
> 
> Thomas Devoogdt
> 

Re:

[Thomas Devoogdt]

Hi Hans,

Thx for the fast response!
See inline below.

Kind regards,

Thomas Devoogdt


Op wo 22 nov 2023 om 10:48 schreef Hans Verkuil <hverkuil@xs4all.nl>:
>
> On 22/11/2023 10:26, Thomas Devoogdt wrote:
> > Hi all,
> >
> >
> > I have two questions:
> >
> > 1.
> > When running v4l2-compliance on a proprietary driver, I get this error:
> >
> > ```
> > Input/Output configuration ioctls:
> > fail: v4l2-test-io-config.cpp(227): fmt.fmt.pix.width >=
> > enumtimings.timings.bt.width * 1.5
> > fail: v4l2-test-io-config.cpp(386): Timings check failed for input 0.
> > test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: FAIL
> > ```
> >
> > Which brings me here:
> > https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-io-config.cpp#n227
> >
> > ```
> > fail_on_test(fmt.fmt.pix.width < enumtimings.timings.bt.width);
> > fail_on_test(fmt.fmt.pix.width >= enumtimings.timings.bt.width * 1.5);
> > fail_on_test(fmt.fmt.pix.height * factor < enumtimings.timings.bt.height);
> > fail_on_test(fmt.fmt.pix.height * factor >=
> > enumtimings.timings.bt.height * 1.5);
> > ```
> >
> > The problem is that the driver supports VIDIOC_S_DV_TIMINGS but is not
> > able to apply the specific format just returns the actual timings.
> > This is from what I know, not a violation, so I'm not sure what the
> > driver should return if it is not able to set a custom timing. Or is
> > this check just a bit too strict?
>
> This test enumerates all timings returned by VIDIOC_ENUM_DV_TIMINGS.
> All these timings should be valid supported timings. For each returned
> timing it calls S_DV_TIMINGS to set it. Then it call G_DV_TIMINGS to
> verify that the width and height of the new timings match what was
> requested. Apparently that worked, otherwise the test would have
> failed there.

Yes, indeed that part is ok.

> Next it gets the new format (G_FMT). The driver must update the format
> when the timings are updated. In this case the width of the returned
> format is more than 1.5 times the width of the requested timings.
> That doesn't sound right.

I found that G_FMT returns 16x16 (not sure where that is done), but
DV_TIMINGS was 0x0.
So fail_on_test(fmt.fmt.pix.width >= enumtimings.timings.bt.width *
1.5) triggered.

> Your claim that S_DV_TIMINGS can't set the new timings is dubious
> since G_DV_TIMINGS apparently returns the new timings correctly.
> Also, ENUM_DV_TIMINGS shouldn't return unsupported timings either.
>

I meant hardware-wise with not supported, but S_DV_TIMINGS itself is
indeed supported.

> >
> >
> > 2.
> > For another driver, I get this error:
> >
> > ```
> > Input ioctls:
> > fail: v4l2-test-input-output.cpp(443): use of deprecated digital video status
> > fail: v4l2-test-input-output.cpp(496): invalid attributes for input 0
> > test VIDIOC_G/S/ENUMINPUT: FAIL
> > ```
> >
> > It might be true that setting V4L2_IN_ST_NO_CARRIER is deprecated, but
> > is that really an error, not better to just have it as a warning?
> > After all, how can userspace know specific details if needed? Perhaps
> > checking that V4L2_IN_ST_NO_SIGNAL has also been set is enough, and if
> > V4L2_IN_ST_NO_CARRIER is set, but not V4L2_IN_ST_NO_SIGNAL, then it
> > might be an error.
>
> The NO_CARRIER, NO_EQU and NO_ACCESS status flags are DVB specific, but this
> ioctl isn't used by DVB anymore. I think very old drivers in the past (long
> since removed or changed) used this, but new drivers must not use this since
> it makes no sense. Note that v4l2-compliance is often more strict than the
> V4L2 spec itself: it is meant to ensure that drivers follow best practices.

I changed V4L2_IN_ST_NO_CARRIER to V4L2_IN_ST_NO_POWER which is fine enough.

> Note that the error message is not very good: it should say "digital TV"
> rather than "digital video". I'll fix that since that's confusing.
>
> Regards,
>
>         Hans
>
> >
> > Thx in advance!
> >
> > Kr,
> >
> > Thomas Devoogdt
> >
>
>