All of lore.kernel.org
 help / color / mirror / Atom feed
* PROBLEM: Broken pixel format for Elgato Cam Link 4K
@ 2020-11-20 18:52 Benjamin Drung
  2020-11-20 21:45 ` Adam Goode
  0 siblings, 1 reply; 15+ messages in thread
From: Benjamin Drung @ 2020-11-20 18:52 UTC (permalink / raw)
  To: Laurent Pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel
  Cc: Adam Goode

Hi,

I own an Elgato Cam Link 4K which is a very popular USB HDMI capture
device (number one capture card by click rates on Geizhals [1]). The
problem is that the video feed is distorted when using the /dev/videoX
device in the browser (tested on Firefox and Chromium) for video
conferencing (tested with Jitsi Meet and Google Meet). The same
distortion is present when opening `v4l2:///dev/video0` with VLC.

The Elgato Cam Link 4K reports to have three different pixel formats:

```
$ v4l2-ctl -d /dev/video0 --list-formats-ext
ioctl: VIDIOC_ENUM_FMT
        Type: Video Capture

        [0]: 'NV12' (Y/CbCr 4:2:0)
                Size: Discrete 3840x2160
                        Interval: Discrete 0.040s (25.000 fps)
        [1]: 'NV12' (Y/CbCr 4:2:0)
                Size: Discrete 3840x2160
                        Interval: Discrete 0.040s (25.000 fps)
        [2]: 'YU12' (Planar YUV 4:2:0)
                Size: Discrete 3840x2160
                        Interval: Discrete 0.040s (25.000 fps)
```

When specifying the video format 'YU12' to VLC, the video is distorted
the same way as using the default video format. When specifying 'NV12'
to VLC, the video feed is displayed correctly:

```
vlc v4l2:///dev/video0 --v4l2-chroma=NV12
```

In OBS, the video feed is always displayed correctly. All video formats
'Y/CbCr 4:2:0', 'Planar YUV 4:2:0', 'BGR3 (Emulated)', and 'YV12
(Emulated)' combined with the color ranges 'Default', 'Partial', and
'Full' produce the same correct output.

With Linux >= 5.9 this behavior in OBS changes: The video format
'Y/CbCr 4:2:0' displays the video correctly. Switching to 'Planar YUV
4:2:0', 'BGR3 (Emulated)', or 'YV12 (Emulated)' shows the video
distorted and OBS shows this error message:

```
info: v4l2-input: Pixelformat: NV12
[...]
libv4l2: error set_fmt gave us a different result than try_fmt!
info: v4l2-input: Resolution: 3840x2160
info: v4l2-input: Pixelformat: NV12
```

Changing the video format back does not have an effect until I also
change the color range (does seem to be relevant what to select there).

Workaround
----------

You can create a v4l2loopback device and use ffmpeg to stream from the
Cam Link 4K to the loopback device:

```
ffmpeg -f v4l2 -input_format yuv420p -video_size 3840x2160 \
  -i "$camlink" -codec copy -f v4l2 "$loopdev"
```

This workaround works, but is cumbersome and burns CPU cycles.

Other reports
-------------

Searching the web for "Cam Link 4K Linux" reveals many similar reports
like this. Noteworthy is blog post [3] from Mike Walters who patched
the Cam Link 4K firmware to report the correct video format. I am
willing to debug this issue and do test, but I don't want to flash the
firmware to not break the warrenty (bisides I lack the hardware for
flashing).

Environment
-----------

This problem is present in Ubuntu 20.04 with linux 5.4.0-54.60 and
Ubuntu 20.10 with linux 5.8.0-29.31. I also tested the mainline kernels
builds 5.9.8-050908.202011101634 and 5.10.0-051000rc4.202011152030 from
Ubuntu [2].

The Cam Link 4K shows follow entries in dmesg:

```
[    1.575753] usb 2-3: new SuperSpeed Gen 1 USB device number 2 using xhci_hcd
[    1.596664] usb 2-3: LPM exit latency is zeroed, disabling LPM.
[    1.598557] usb 2-3: New USB device found, idVendor=0fd9, idProduct=0066, bcdDevice= 0.00
[    1.598558] usb 2-3: New USB device strings: Mfr=1, Product=2, SerialNumber=4
[    1.598559] usb 2-3: Product: Cam Link 4K
[    1.598560] usb 2-3: Manufacturer: Elgato
```

I have another problems with 5.9.8-050908.202011101634 and 5.10.0-
051000rc4.202011152030: Chromium fail to access the video device of Cam
Link 4K and the notebook integrated webcam has a too low brightness.

[1] https://geizhals.de/?cat=vidext
[2] https://kernel.ubuntu.com/~kernel-ppa/mainline/
[3] https://assortedhackery.com/patching-cam-link-to-play-nicer-on-linux/

--
Benjamin Drung
Debian & Ubuntu Developer


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: PROBLEM: Broken pixel format for Elgato Cam Link 4K
  2020-11-20 18:52 PROBLEM: Broken pixel format for Elgato Cam Link 4K Benjamin Drung
@ 2020-11-20 21:45 ` Adam Goode
  2020-11-22 19:34   ` Benjamin Drung
  0 siblings, 1 reply; 15+ messages in thread
From: Adam Goode @ 2020-11-20 21:45 UTC (permalink / raw)
  To: Benjamin Drung
  Cc: Laurent Pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel

On Fri, Nov 20, 2020 at 1:52 PM Benjamin Drung
<benjamin.drung@cloud.ionos.com> wrote:
>
> Hi,
>
> I own an Elgato Cam Link 4K which is a very popular USB HDMI capture
> device (number one capture card by click rates on Geizhals [1]). The
> problem is that the video feed is distorted when using the /dev/videoX
> device in the browser (tested on Firefox and Chromium) for video
> conferencing (tested with Jitsi Meet and Google Meet). The same
> distortion is present when opening `v4l2:///dev/video0` with VLC.
>
> The Elgato Cam Link 4K reports to have three different pixel formats:
>
> ```
> $ v4l2-ctl -d /dev/video0 --list-formats-ext
> ioctl: VIDIOC_ENUM_FMT
>         Type: Video Capture
>
>         [0]: 'NV12' (Y/CbCr 4:2:0)
>                 Size: Discrete 3840x2160
>                         Interval: Discrete 0.040s (25.000 fps)
>         [1]: 'NV12' (Y/CbCr 4:2:0)
>                 Size: Discrete 3840x2160
>                         Interval: Discrete 0.040s (25.000 fps)
>         [2]: 'YU12' (Planar YUV 4:2:0)
>                 Size: Discrete 3840x2160
>                         Interval: Discrete 0.040s (25.000 fps)
> ```
>
> When specifying the video format 'YU12' to VLC, the video is distorted
> the same way as using the default video format. When specifying 'NV12'
> to VLC, the video feed is displayed correctly:
>
> ```
> vlc v4l2:///dev/video0 --v4l2-chroma=NV12
> ```
>
> In OBS, the video feed is always displayed correctly. All video formats
> 'Y/CbCr 4:2:0', 'Planar YUV 4:2:0', 'BGR3 (Emulated)', and 'YV12
> (Emulated)' combined with the color ranges 'Default', 'Partial', and
> 'Full' produce the same correct output.
>
> With Linux >= 5.9 this behavior in OBS changes: The video format
> 'Y/CbCr 4:2:0' displays the video correctly. Switching to 'Planar YUV
> 4:2:0', 'BGR3 (Emulated)', or 'YV12 (Emulated)' shows the video
> distorted and OBS shows this error message:
>
> ```
> info: v4l2-input: Pixelformat: NV12
> [...]
> libv4l2: error set_fmt gave us a different result than try_fmt!
> info: v4l2-input: Resolution: 3840x2160
> info: v4l2-input: Pixelformat: NV12
> ```
>
> Changing the video format back does not have an effect until I also
> change the color range (does seem to be relevant what to select there).
>
> Workaround
> ----------
>
> You can create a v4l2loopback device and use ffmpeg to stream from the
> Cam Link 4K to the loopback device:
>
> ```
> ffmpeg -f v4l2 -input_format yuv420p -video_size 3840x2160 \
>   -i "$camlink" -codec copy -f v4l2 "$loopdev"
> ```
>
> This workaround works, but is cumbersome and burns CPU cycles.
>
> Other reports
> -------------
>
> Searching the web for "Cam Link 4K Linux" reveals many similar reports
> like this. Noteworthy is blog post [3] from Mike Walters who patched
> the Cam Link 4K firmware to report the correct video format. I am
> willing to debug this issue and do test, but I don't want to flash the
> firmware to not break the warrenty (bisides I lack the hardware for
> flashing).
>
> Environment
> -----------
>
> This problem is present in Ubuntu 20.04 with linux 5.4.0-54.60 and
> Ubuntu 20.10 with linux 5.8.0-29.31. I also tested the mainline kernels
> builds 5.9.8-050908.202011101634 and 5.10.0-051000rc4.202011152030 from
> Ubuntu [2].
>
> The Cam Link 4K shows follow entries in dmesg:
>
> ```
> [    1.575753] usb 2-3: new SuperSpeed Gen 1 USB device number 2 using xhci_hcd
> [    1.596664] usb 2-3: LPM exit latency is zeroed, disabling LPM.
> [    1.598557] usb 2-3: New USB device found, idVendor=0fd9, idProduct=0066, bcdDevice= 0.00
> [    1.598558] usb 2-3: New USB device strings: Mfr=1, Product=2, SerialNumber=4
> [    1.598559] usb 2-3: Product: Cam Link 4K
> [    1.598560] usb 2-3: Manufacturer: Elgato
> ```
>
> I have another problems with 5.9.8-050908.202011101634 and 5.10.0-
> 051000rc4.202011152030: Chromium fail to access the video device of Cam
> Link 4K and the notebook integrated webcam has a too low brightness.
>
> [1] https://geizhals.de/?cat=vidext
> [2] https://kernel.ubuntu.com/~kernel-ppa/mainline/
> [3] https://assortedhackery.com/patching-cam-link-to-play-nicer-on-linux/
>
> --
> Benjamin Drung
> Debian & Ubuntu Developer
>


Hi,

I am running on Fedora 32 which has the fix I wrote for the buggy
elgato firmware. The bug in the firmware makes it impossible to
properly select a non-0 pixel format when following the UVC
negotiation protocol. This is because the firmware returns the pixel
format in the wrong byte of the packet. The driver was following the
UVC protocol but did not send the pixel format back to the v4l2
subsystem. It does that now.

I'm not surprised that other bugs are emerging now. Ultimately the
firmware is buggy and announces pixel formats that it then rejects. If
I flip through the settings in OBS, I do manage to wedge the
interface. But most of the programs I've seen that use v4l2 are buggy
in this way. A reliable one is the qv4l2 test program. I've also had
no problems with Chromium.

That reverse engineering is interesting! But I think it hides the real
problem, where the pixel format negotiation on the firmware writes
into the wrong byte of the packet.


Adam

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: PROBLEM: Broken pixel format for Elgato Cam Link 4K
  2020-11-20 21:45 ` Adam Goode
@ 2020-11-22 19:34   ` Benjamin Drung
  2020-11-23 15:02     ` Adam Goode
  0 siblings, 1 reply; 15+ messages in thread
From: Benjamin Drung @ 2020-11-22 19:34 UTC (permalink / raw)
  To: Adam Goode
  Cc: Laurent Pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel

Am Freitag, den 20.11.2020, 16:45 -0500 schrieb Adam Goode:
> On Fri, Nov 20, 2020 at 1:52 PM Benjamin Drung
> <benjamin.drung@cloud.ionos.com> wrote:
> > 
> > Hi,
> > 
> > I own an Elgato Cam Link 4K which is a very popular USB HDMI
> > capture
> > device (number one capture card by click rates on Geizhals [1]).
> > The
> > problem is that the video feed is distorted when using the
> > /dev/videoX
> > device in the browser (tested on Firefox and Chromium) for video
> > conferencing (tested with Jitsi Meet and Google Meet). The same
> > distortion is present when opening `v4l2:///dev/video0` with VLC.
> > 
> > The Elgato Cam Link 4K reports to have three different pixel
> > formats:
> > 
> > ```
> > $ v4l2-ctl -d /dev/video0 --list-formats-ext
> > ioctl: VIDIOC_ENUM_FMT
> >         Type: Video Capture
> > 
> >         [0]: 'NV12' (Y/CbCr 4:2:0)
> >                 Size: Discrete 3840x2160
> >                         Interval: Discrete 0.040s (25.000 fps)
> >         [1]: 'NV12' (Y/CbCr 4:2:0)
> >                 Size: Discrete 3840x2160
> >                         Interval: Discrete 0.040s (25.000 fps)
> >         [2]: 'YU12' (Planar YUV 4:2:0)
> >                 Size: Discrete 3840x2160
> >                         Interval: Discrete 0.040s (25.000 fps)
> > ```
> > 
> > When specifying the video format 'YU12' to VLC, the video is
> > distorted
> > the same way as using the default video format. When specifying
> > 'NV12'
> > to VLC, the video feed is displayed correctly:
> > 
> > ```
> > vlc v4l2:///dev/video0 --v4l2-chroma=NV12
> > ```
> > 
> > In OBS, the video feed is always displayed correctly. All video
> > formats
> > 'Y/CbCr 4:2:0', 'Planar YUV 4:2:0', 'BGR3 (Emulated)', and 'YV12
> > (Emulated)' combined with the color ranges 'Default', 'Partial',
> > and
> > 'Full' produce the same correct output.
> > 
> > With Linux >= 5.9 this behavior in OBS changes: The video format
> > 'Y/CbCr 4:2:0' displays the video correctly. Switching to 'Planar
> > YUV
> > 4:2:0', 'BGR3 (Emulated)', or 'YV12 (Emulated)' shows the video
> > distorted and OBS shows this error message:
> > 
> > ```
> > info: v4l2-input: Pixelformat: NV12
> > [...]
> > libv4l2: error set_fmt gave us a different result than try_fmt!
> > info: v4l2-input: Resolution: 3840x2160
> > info: v4l2-input: Pixelformat: NV12
> > ```
> > 
> > Changing the video format back does not have an effect until I also
> > change the color range (does seem to be relevant what to select
> > there).
> > 
> > Workaround
> > ----------
> > 
> > You can create a v4l2loopback device and use ffmpeg to stream from
> > the
> > Cam Link 4K to the loopback device:
> > 
> > ```
> > ffmpeg -f v4l2 -input_format yuv420p -video_size 3840x2160 \
> >   -i "$camlink" -codec copy -f v4l2 "$loopdev"
> > ```
> > 
> > This workaround works, but is cumbersome and burns CPU cycles.
> > 
> > Other reports
> > -------------
> > 
> > Searching the web for "Cam Link 4K Linux" reveals many similar
> > reports
> > like this. Noteworthy is blog post [3] from Mike Walters who
> > patched
> > the Cam Link 4K firmware to report the correct video format. I am
> > willing to debug this issue and do test, but I don't want to flash
> > the
> > firmware to not break the warrenty (bisides I lack the hardware for
> > flashing).
> > 
> > Environment
> > -----------
> > 
> > This problem is present in Ubuntu 20.04 with linux 5.4.0-54.60 and
> > Ubuntu 20.10 with linux 5.8.0-29.31. I also tested the mainline
> > kernels
> > builds 5.9.8-050908.202011101634 and 5.10.0-051000rc4.202011152030
> > from
> > Ubuntu [2].
> > 
> > The Cam Link 4K shows follow entries in dmesg:
> > 
> > ```
> > [    1.575753] usb 2-3: new SuperSpeed Gen 1 USB device number 2
> > using xhci_hcd
> > [    1.596664] usb 2-3: LPM exit latency is zeroed, disabling LPM.
> > [    1.598557] usb 2-3: New USB device found, idVendor=0fd9,
> > idProduct=0066, bcdDevice= 0.00
> > [    1.598558] usb 2-3: New USB device strings: Mfr=1, Product=2,
> > SerialNumber=4
> > [    1.598559] usb 2-3: Product: Cam Link 4K
> > [    1.598560] usb 2-3: Manufacturer: Elgato
> > ```
> > 
> > I have another problems with 5.9.8-050908.202011101634 and 5.10.0-
> > 051000rc4.202011152030: Chromium fail to access the video device of
> > Cam
> > Link 4K and the notebook integrated webcam has a too low
> > brightness.
> > 
> > [1] https://geizhals.de/?cat=vidext
> > [2] https://kernel.ubuntu.com/~kernel-ppa/mainline/
> > [3] 
> > https://assortedhackery.com/patching-cam-link-to-play-nicer-on-linux/
> > 
> > --
> > Benjamin Drung
> > Debian & Ubuntu Developer
> > 
> 
> 
> Hi,
> 
> I am running on Fedora 32 which has the fix I wrote for the buggy
> elgato firmware. The bug in the firmware makes it impossible to
> properly select a non-0 pixel format when following the UVC
> negotiation protocol. This is because the firmware returns the pixel
> format in the wrong byte of the packet. The driver was following the
> UVC protocol but did not send the pixel format back to the v4l2
> subsystem. It does that now.
> 
> I'm not surprised that other bugs are emerging now. Ultimately the
> firmware is buggy and announces pixel formats that it then rejects.
> If
> I flip through the settings in OBS, I do manage to wedge the
> interface. But most of the programs I've seen that use v4l2 are buggy
> in this way. A reliable one is the qv4l2 test program. I've also had
> no problems with Chromium.
> 
> That reverse engineering is interesting! But I think it hides the
> real
> problem, where the pixel format negotiation on the firmware writes
> into the wrong byte of the packet.

Hi Adam,

you are talking about commit ec2c23f628802317f73fab5255cc62a776bc7930
and 8a652a17e3c005dcdae31b6c8fdf14382a29cbbe that are part of
v5.10-rc1?

What do you suggest to fix the issue? Should I contact Elgato
requesting a firmware update? Can the kernel ship a quirk for the Cam
Link 4K to workaround the firmware bug? I would be happy to try out
patches.

I tested the qv4l2 with Linux 5.10.0-051000rc4.202011152030. When using
the default settings, it displays the video correctly, but with a
vertical green one-pixel-wide line. The terminal prints "error" four
times. Sadly there is no debug mode to figure out where that error
comes from.

Selecting "YU12 (Planar YUV 4:2:0)" print the following error message
and switches back to the previous format:

libv4l2: error set_fmt gave us a different result than try_fmt!

--
Benjamin Drung
Debian & Ubuntu Developer


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: PROBLEM: Broken pixel format for Elgato Cam Link 4K
  2020-11-22 19:34   ` Benjamin Drung
@ 2020-11-23 15:02     ` Adam Goode
  2021-04-06 18:52       ` [PATCH v2] media: uvcvideo: Fix pixel format change " Benjamin Drung
  2021-06-04 17:19       ` Benjamin Drung
  0 siblings, 2 replies; 15+ messages in thread
From: Adam Goode @ 2020-11-23 15:02 UTC (permalink / raw)
  To: Benjamin Drung
  Cc: Laurent Pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel

On Sun, Nov 22, 2020 at 2:34 PM Benjamin Drung
<benjamin.drung@cloud.ionos.com> wrote:
>
> Am Freitag, den 20.11.2020, 16:45 -0500 schrieb Adam Goode:
> > On Fri, Nov 20, 2020 at 1:52 PM Benjamin Drung
> > <benjamin.drung@cloud.ionos.com> wrote:
> > >
> > > Hi,
> > >
> > > I own an Elgato Cam Link 4K which is a very popular USB HDMI
> > > capture
> > > device (number one capture card by click rates on Geizhals [1]).
> > > The
> > > problem is that the video feed is distorted when using the
> > > /dev/videoX
> > > device in the browser (tested on Firefox and Chromium) for video
> > > conferencing (tested with Jitsi Meet and Google Meet). The same
> > > distortion is present when opening `v4l2:///dev/video0` with VLC.
> > >
> > > The Elgato Cam Link 4K reports to have three different pixel
> > > formats:
> > >
> > > ```
> > > $ v4l2-ctl -d /dev/video0 --list-formats-ext
> > > ioctl: VIDIOC_ENUM_FMT
> > >         Type: Video Capture
> > >
> > >         [0]: 'NV12' (Y/CbCr 4:2:0)
> > >                 Size: Discrete 3840x2160
> > >                         Interval: Discrete 0.040s (25.000 fps)
> > >         [1]: 'NV12' (Y/CbCr 4:2:0)
> > >                 Size: Discrete 3840x2160
> > >                         Interval: Discrete 0.040s (25.000 fps)
> > >         [2]: 'YU12' (Planar YUV 4:2:0)
> > >                 Size: Discrete 3840x2160
> > >                         Interval: Discrete 0.040s (25.000 fps)
> > > ```
> > >
> > > When specifying the video format 'YU12' to VLC, the video is
> > > distorted
> > > the same way as using the default video format. When specifying
> > > 'NV12'
> > > to VLC, the video feed is displayed correctly:
> > >
> > > ```
> > > vlc v4l2:///dev/video0 --v4l2-chroma=NV12
> > > ```
> > >
> > > In OBS, the video feed is always displayed correctly. All video
> > > formats
> > > 'Y/CbCr 4:2:0', 'Planar YUV 4:2:0', 'BGR3 (Emulated)', and 'YV12
> > > (Emulated)' combined with the color ranges 'Default', 'Partial',
> > > and
> > > 'Full' produce the same correct output.
> > >
> > > With Linux >= 5.9 this behavior in OBS changes: The video format
> > > 'Y/CbCr 4:2:0' displays the video correctly. Switching to 'Planar
> > > YUV
> > > 4:2:0', 'BGR3 (Emulated)', or 'YV12 (Emulated)' shows the video
> > > distorted and OBS shows this error message:
> > >
> > > ```
> > > info: v4l2-input: Pixelformat: NV12
> > > [...]
> > > libv4l2: error set_fmt gave us a different result than try_fmt!
> > > info: v4l2-input: Resolution: 3840x2160
> > > info: v4l2-input: Pixelformat: NV12
> > > ```
> > >
> > > Changing the video format back does not have an effect until I also
> > > change the color range (does seem to be relevant what to select
> > > there).
> > >
> > > Workaround
> > > ----------
> > >
> > > You can create a v4l2loopback device and use ffmpeg to stream from
> > > the
> > > Cam Link 4K to the loopback device:
> > >
> > > ```
> > > ffmpeg -f v4l2 -input_format yuv420p -video_size 3840x2160 \
> > >   -i "$camlink" -codec copy -f v4l2 "$loopdev"
> > > ```
> > >
> > > This workaround works, but is cumbersome and burns CPU cycles.
> > >
> > > Other reports
> > > -------------
> > >
> > > Searching the web for "Cam Link 4K Linux" reveals many similar
> > > reports
> > > like this. Noteworthy is blog post [3] from Mike Walters who
> > > patched
> > > the Cam Link 4K firmware to report the correct video format. I am
> > > willing to debug this issue and do test, but I don't want to flash
> > > the
> > > firmware to not break the warrenty (bisides I lack the hardware for
> > > flashing).
> > >
> > > Environment
> > > -----------
> > >
> > > This problem is present in Ubuntu 20.04 with linux 5.4.0-54.60 and
> > > Ubuntu 20.10 with linux 5.8.0-29.31. I also tested the mainline
> > > kernels
> > > builds 5.9.8-050908.202011101634 and 5.10.0-051000rc4.202011152030
> > > from
> > > Ubuntu [2].
> > >
> > > The Cam Link 4K shows follow entries in dmesg:
> > >
> > > ```
> > > [    1.575753] usb 2-3: new SuperSpeed Gen 1 USB device number 2
> > > using xhci_hcd
> > > [    1.596664] usb 2-3: LPM exit latency is zeroed, disabling LPM.
> > > [    1.598557] usb 2-3: New USB device found, idVendor=0fd9,
> > > idProduct=0066, bcdDevice= 0.00
> > > [    1.598558] usb 2-3: New USB device strings: Mfr=1, Product=2,
> > > SerialNumber=4
> > > [    1.598559] usb 2-3: Product: Cam Link 4K
> > > [    1.598560] usb 2-3: Manufacturer: Elgato
> > > ```
> > >
> > > I have another problems with 5.9.8-050908.202011101634 and 5.10.0-
> > > 051000rc4.202011152030: Chromium fail to access the video device of
> > > Cam
> > > Link 4K and the notebook integrated webcam has a too low
> > > brightness.
> > >
> > > [1] https://geizhals.de/?cat=vidext
> > > [2] https://kernel.ubuntu.com/~kernel-ppa/mainline/
> > > [3]
> > > https://assortedhackery.com/patching-cam-link-to-play-nicer-on-linux/
> > >
> > > --
> > > Benjamin Drung
> > > Debian & Ubuntu Developer
> > >
> >
> >
> > Hi,
> >
> > I am running on Fedora 32 which has the fix I wrote for the buggy
> > elgato firmware. The bug in the firmware makes it impossible to
> > properly select a non-0 pixel format when following the UVC
> > negotiation protocol. This is because the firmware returns the pixel
> > format in the wrong byte of the packet. The driver was following the
> > UVC protocol but did not send the pixel format back to the v4l2
> > subsystem. It does that now.
> >
> > I'm not surprised that other bugs are emerging now. Ultimately the
> > firmware is buggy and announces pixel formats that it then rejects.
> > If
> > I flip through the settings in OBS, I do manage to wedge the
> > interface. But most of the programs I've seen that use v4l2 are buggy
> > in this way. A reliable one is the qv4l2 test program. I've also had
> > no problems with Chromium.
> >
> > That reverse engineering is interesting! But I think it hides the
> > real
> > problem, where the pixel format negotiation on the firmware writes
> > into the wrong byte of the packet.
>
> Hi Adam,
>
> you are talking about commit ec2c23f628802317f73fab5255cc62a776bc7930
> and 8a652a17e3c005dcdae31b6c8fdf14382a29cbbe that are part of
> v5.10-rc1?
>
> What do you suggest to fix the issue? Should I contact Elgato
> requesting a firmware update? Can the kernel ship a quirk for the Cam
> Link 4K to workaround the firmware bug? I would be happy to try out
> patches.
>

I would file a ticket. I did so some time ago, but there have been no
firmware updates for many months.

> I tested the qv4l2 with Linux 5.10.0-051000rc4.202011152030. When using
> the default settings, it displays the video correctly, but with a
> vertical green one-pixel-wide line. The terminal prints "error" four
> times. Sadly there is no debug mode to figure out where that error
> comes from.

I'm not sure where this line is coming from. Can you post a link to a
screenshot or capture? I have never encountered this issue. Can you
reproduce on a Windows or Mac computer?

>
> Selecting "YU12 (Planar YUV 4:2:0)" print the following error message
> and switches back to the previous format:
>
> libv4l2: error set_fmt gave us a different result than try_fmt!
>

Yes, this is the kernel coping with the buggy firmware. These errors
are expected. It is also expected that YU12 does not work (the device
advertises it, then rejects it when selected). I believe Windows and
Mac (you can use OBS to try) do the same thing.

A quirk is possible (I hacked something up with libuvc when I was
debugging this) but it's not something I have time to look at right
now.


Adam

> --
> Benjamin Drung
> Debian & Ubuntu Developer
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2] media: uvcvideo: Fix pixel format change for Elgato Cam Link 4K
  2020-11-23 15:02     ` Adam Goode
@ 2021-04-06 18:52       ` Benjamin Drung
  2021-06-04 17:19       ` Benjamin Drung
  1 sibling, 0 replies; 15+ messages in thread
From: Benjamin Drung @ 2021-04-06 18:52 UTC (permalink / raw)
  To: Adam Goode, Laurent Pinchart, Mauro Carvalho Chehab
  Cc: linux-media, linux-kernel, Benjamin Drung, stable

The Elgato Cam Link 4K HDMI video capture card reports to support three
different pixel formats, where the first format depends on the connected
HDMI device.

```
$ v4l2-ctl -d /dev/video0 --list-formats-ext
ioctl: VIDIOC_ENUM_FMT
	Type: Video Capture

	[0]: 'NV12' (Y/CbCr 4:2:0)
		Size: Discrete 3840x2160
			Interval: Discrete 0.033s (29.970 fps)
	[1]: 'NV12' (Y/CbCr 4:2:0)
		Size: Discrete 3840x2160
			Interval: Discrete 0.033s (29.970 fps)
	[2]: 'YU12' (Planar YUV 4:2:0)
		Size: Discrete 3840x2160
			Interval: Discrete 0.033s (29.970 fps)
```

Changing the pixel format to anything besides the first pixel format
does not work:

```
v4l2-ctl -d /dev/video0 --try-fmt-video pixelformat=YU12
Format Video Capture:
	Width/Height      : 3840/2160
	Pixel Format      : 'NV12' (Y/CbCr 4:2:0)
	Field             : None
	Bytes per Line    : 3840
	Size Image        : 12441600
	Colorspace        : sRGB
	Transfer Function : Rec. 709
	YCbCr/HSV Encoding: Rec. 709
	Quantization      : Default (maps to Limited Range)
	Flags             :
```

User space applications like VLC might show an error message on the
terminal in that case:

```
libv4l2: error set_fmt gave us a different result than try_fmt!
```

Depending on the error handling of the user space applications, they
might display a distorted video, because they use the wrong pixel format
for decoding the stream.

The Elgato Cam Link 4K responds to the USB video probe
VS_PROBE_CONTROL/VS_COMMIT_CONTROL with a malformed data structure: The
second byte contains bFormatIndex (instead of being the second byte of
bmHint). The first byte is always zero. The third byte is always 1.

The firmware bug was reported to Elgato on 2020-12-01 and it was
forwarded by the support team to the developers as feature request.
There is no firmware update available since then. The latest firmware
for Elgato Cam Link 4K as of 2021-03-23 has MCU 20.02.19 and FPGA 67.

Therefore add a quirk to correct the malformed data structure.

The quirk was successfully tested with VLC, OBS, and Chromium using
different pixel formats (YUYV, NV12, YU12), resolutions (3840x2160,
1920x1080), and frame rates (29.970 and 59.940 fps).

Cc: stable@vger.kernel.org
Signed-off-by: Benjamin Drung <bdrung@posteo.de>
---

In version 2, I only enhanced the comment in the code to document that
the quirk is only applied to broken responses (guarded by
ctrl->bmHint > 255). So in case Elgato fixes the firmware, the quirk
will be skipped.

Adam Goode suggested to also apply the quirk to Elgato Game Capture HD
60 S+ (0fd9:006a). Can someone owning this device test this patch/quirk
(or can someone borrow me one for testing)?

Feel free to propose a better name for the quirk than
UVC_QUIRK_FIX_FORMAT_INDEX.

To backport to version 5.11 and earlier, the line

```
uvc_dbg(stream->dev, CONTROL,
```

needs to be changed back to

```
uvc_trace(UVC_TRACE_CONTROL,
```

 drivers/media/usb/uvc/uvc_driver.c | 13 +++++++++++++
 drivers/media/usb/uvc/uvc_video.c  | 21 +++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 3 files changed, 35 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 30ef2a3110f7..4f245b3f8bd9 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -3132,6 +3132,19 @@ static const struct usb_device_id uvc_ids[] = {
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0,
 	  .driver_info		= UVC_INFO_META(V4L2_META_FMT_D4XX) },
+	/*
+	 * Elgato Cam Link 4K
+	 * Latest firmware as of 2021-03-23 needs this quirk.
+	 * MCU: 20.02.19, FPGA: 67
+	 */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x0fd9,
+	  .idProduct		= 0x0066,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_FIX_FORMAT_INDEX) },
 	/* Generic USB Video Class */
 	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
 	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index f2f565281e63..06a538d1008b 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -128,6 +128,27 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
 	struct uvc_frame *frame = NULL;
 	unsigned int i;
 
+	/*
+	 * The response of the Elgato Cam Link 4K is incorrect: The second byte
+	 * contains bFormatIndex (instead of being the second byte of bmHint).
+	 * The first byte is always zero. The third byte is always 1.
+	 *
+	 * The UVC 1.5 class specification defines the first five bits in the
+	 * bmHint bitfield. The remaining bits are reserved and should be zero.
+	 * Therefore a valid bmHint will be less than 32.
+	 */
+	if (stream->dev->quirks & UVC_QUIRK_FIX_FORMAT_INDEX && ctrl->bmHint > 255) {
+		__u8 corrected_format_index;
+
+		corrected_format_index = ctrl->bmHint >> 8;
+		uvc_dbg(stream->dev, CONTROL,
+			"Correct USB video probe response from {bmHint: 0x%04x, bFormatIndex: 0x%02x} to {bmHint: 0x%04x, bFormatIndex: 0x%02x}.\n",
+			ctrl->bmHint, ctrl->bFormatIndex,
+			ctrl->bFormatIndex, corrected_format_index);
+		ctrl->bmHint = ctrl->bFormatIndex;
+		ctrl->bFormatIndex = corrected_format_index;
+	}
+
 	for (i = 0; i < stream->nformats; ++i) {
 		if (stream->format[i].index == ctrl->bFormatIndex) {
 			format = &stream->format[i];
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 97df5ecd66c9..bf401d5ba27d 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -209,6 +209,7 @@
 #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
 #define UVC_QUIRK_FORCE_Y8		0x00000800
 #define UVC_QUIRK_FORCE_BPP		0x00001000
+#define UVC_QUIRK_FIX_FORMAT_INDEX	0x00002000
 
 /* Format flags */
 #define UVC_FMT_FLAG_COMPRESSED		0x00000001
-- 
2.27.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2] media: uvcvideo: Fix pixel format change for Elgato Cam Link 4K
  2020-11-23 15:02     ` Adam Goode
  2021-04-06 18:52       ` [PATCH v2] media: uvcvideo: Fix pixel format change " Benjamin Drung
@ 2021-06-04 17:19       ` Benjamin Drung
  2021-06-04 22:21         ` Laurent Pinchart
  1 sibling, 1 reply; 15+ messages in thread
From: Benjamin Drung @ 2021-06-04 17:19 UTC (permalink / raw)
  To: Linus Torvalds, Adam Goode, Laurent Pinchart, Mauro Carvalho Chehab
  Cc: linux-media, linux-kernel, Benjamin Drung, stable

The Elgato Cam Link 4K HDMI video capture card reports to support three
different pixel formats, where the first format depends on the connected
HDMI device.

```
$ v4l2-ctl -d /dev/video0 --list-formats-ext
ioctl: VIDIOC_ENUM_FMT
	Type: Video Capture

	[0]: 'NV12' (Y/CbCr 4:2:0)
		Size: Discrete 3840x2160
			Interval: Discrete 0.033s (29.970 fps)
	[1]: 'NV12' (Y/CbCr 4:2:0)
		Size: Discrete 3840x2160
			Interval: Discrete 0.033s (29.970 fps)
	[2]: 'YU12' (Planar YUV 4:2:0)
		Size: Discrete 3840x2160
			Interval: Discrete 0.033s (29.970 fps)
```

Changing the pixel format to anything besides the first pixel format
does not work:

```
$ v4l2-ctl -d /dev/video0 --try-fmt-video pixelformat=YU12
Format Video Capture:
	Width/Height      : 3840/2160
	Pixel Format      : 'NV12' (Y/CbCr 4:2:0)
	Field             : None
	Bytes per Line    : 3840
	Size Image        : 12441600
	Colorspace        : sRGB
	Transfer Function : Rec. 709
	YCbCr/HSV Encoding: Rec. 709
	Quantization      : Default (maps to Limited Range)
	Flags             :
```

User space applications like VLC might show an error message on the
terminal in that case:

```
libv4l2: error set_fmt gave us a different result than try_fmt!
```

Depending on the error handling of the user space applications, they
might display a distorted video, because they use the wrong pixel format
for decoding the stream.

The Elgato Cam Link 4K responds to the USB video probe
VS_PROBE_CONTROL/VS_COMMIT_CONTROL with a malformed data structure: The
second byte contains bFormatIndex (instead of being the second byte of
bmHint). The first byte is always zero. The third byte is always 1.

The firmware bug was reported to Elgato on 2020-12-01 and it was
forwarded by the support team to the developers as feature request.
There is no firmware update available since then. The latest firmware
for Elgato Cam Link 4K as of 2021-03-23 has MCU 20.02.19 and FPGA 67.

Therefore add a quirk to correct the malformed data structure.

The quirk was successfully tested with VLC, OBS, and Chromium using
different pixel formats (YUYV, NV12, YU12), resolutions (3840x2160,
1920x1080), and frame rates (29.970 and 59.940 fps).

Cc: stable@vger.kernel.org
Signed-off-by: Benjamin Drung <bdrung@posteo.de>
---

I am sending this patch a fourth time since I got no response and the
last resend is over a month ago. This time I am including Linus Torvalds
in the hope to get it reviewed.

 drivers/media/usb/uvc/uvc_driver.c | 13 +++++++++++++
 drivers/media/usb/uvc/uvc_video.c  | 21 +++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 3 files changed, 35 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 9a791d8ef200..6ce58950d78b 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -3164,6 +3164,19 @@ static const struct usb_device_id uvc_ids[] = {
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0,
 	  .driver_info		= UVC_INFO_META(V4L2_META_FMT_D4XX) },
+	/*
+	 * Elgato Cam Link 4K
+	 * Latest firmware as of 2021-03-23 needs this quirk.
+	 * MCU: 20.02.19, FPGA: 67
+	 */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x0fd9,
+	  .idProduct		= 0x0066,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_FIX_FORMAT_INDEX) },
 	/* Generic USB Video Class */
 	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
 	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index a777b389a66e..910d22233d74 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -131,6 +131,27 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
 	struct uvc_frame *frame = NULL;
 	unsigned int i;
 
+	/*
+	 * The response of the Elgato Cam Link 4K is incorrect: The second byte
+	 * contains bFormatIndex (instead of being the second byte of bmHint).
+	 * The first byte is always zero. The third byte is always 1.
+	 *
+	 * The UVC 1.5 class specification defines the first five bits in the
+	 * bmHint bitfield. The remaining bits are reserved and should be zero.
+	 * Therefore a valid bmHint will be less than 32.
+	 */
+	if (stream->dev->quirks & UVC_QUIRK_FIX_FORMAT_INDEX && ctrl->bmHint > 255) {
+		__u8 corrected_format_index;
+
+		corrected_format_index = ctrl->bmHint >> 8;
+		uvc_dbg(stream->dev, CONTROL,
+			"Correct USB video probe response from {bmHint: 0x%04x, bFormatIndex: 0x%02x} to {bmHint: 0x%04x, bFormatIndex: 0x%02x}.\n",
+			ctrl->bmHint, ctrl->bFormatIndex,
+			ctrl->bFormatIndex, corrected_format_index);
+		ctrl->bmHint = ctrl->bFormatIndex;
+		ctrl->bFormatIndex = corrected_format_index;
+	}
+
 	for (i = 0; i < stream->nformats; ++i) {
 		if (stream->format[i].index == ctrl->bFormatIndex) {
 			format = &stream->format[i];
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index cce5e38133cd..cbb4ef61a64d 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -209,6 +209,7 @@
 #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
 #define UVC_QUIRK_FORCE_Y8		0x00000800
 #define UVC_QUIRK_FORCE_BPP		0x00001000
+#define UVC_QUIRK_FIX_FORMAT_INDEX	0x00002000
 
 /* Format flags */
 #define UVC_FMT_FLAG_COMPRESSED		0x00000001
-- 
2.27.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] media: uvcvideo: Fix pixel format change for Elgato Cam Link 4K
  2021-06-04 17:19       ` Benjamin Drung
@ 2021-06-04 22:21         ` Laurent Pinchart
  2021-06-05  8:19           ` Benjamin Drung
                             ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Laurent Pinchart @ 2021-06-04 22:21 UTC (permalink / raw)
  To: Benjamin Drung
  Cc: Linus Torvalds, Adam Goode, Mauro Carvalho Chehab, linux-media,
	linux-kernel, stable

Hi Benjamin,

Thank you for the patch.

On Fri, Jun 04, 2021 at 05:19:42PM +0000, Benjamin Drung wrote:
> The Elgato Cam Link 4K HDMI video capture card reports to support three
> different pixel formats, where the first format depends on the connected
> HDMI device.
> 
> ```
> $ v4l2-ctl -d /dev/video0 --list-formats-ext
> ioctl: VIDIOC_ENUM_FMT
> 	Type: Video Capture
> 
> 	[0]: 'NV12' (Y/CbCr 4:2:0)
> 		Size: Discrete 3840x2160
> 			Interval: Discrete 0.033s (29.970 fps)
> 	[1]: 'NV12' (Y/CbCr 4:2:0)
> 		Size: Discrete 3840x2160
> 			Interval: Discrete 0.033s (29.970 fps)
> 	[2]: 'YU12' (Planar YUV 4:2:0)
> 		Size: Discrete 3840x2160
> 			Interval: Discrete 0.033s (29.970 fps)
> ```
> 
> Changing the pixel format to anything besides the first pixel format
> does not work:
> 
> ```
> $ v4l2-ctl -d /dev/video0 --try-fmt-video pixelformat=YU12
> Format Video Capture:
> 	Width/Height      : 3840/2160
> 	Pixel Format      : 'NV12' (Y/CbCr 4:2:0)
> 	Field             : None
> 	Bytes per Line    : 3840
> 	Size Image        : 12441600
> 	Colorspace        : sRGB
> 	Transfer Function : Rec. 709
> 	YCbCr/HSV Encoding: Rec. 709
> 	Quantization      : Default (maps to Limited Range)
> 	Flags             :
> ```
> 
> User space applications like VLC might show an error message on the
> terminal in that case:
> 
> ```
> libv4l2: error set_fmt gave us a different result than try_fmt!
> ```
> 
> Depending on the error handling of the user space applications, they
> might display a distorted video, because they use the wrong pixel format
> for decoding the stream.
> 
> The Elgato Cam Link 4K responds to the USB video probe
> VS_PROBE_CONTROL/VS_COMMIT_CONTROL with a malformed data structure: The
> second byte contains bFormatIndex (instead of being the second byte of
> bmHint). The first byte is always zero. The third byte is always 1.
> 
> The firmware bug was reported to Elgato on 2020-12-01 and it was
> forwarded by the support team to the developers as feature request.
> There is no firmware update available since then. The latest firmware
> for Elgato Cam Link 4K as of 2021-03-23 has MCU 20.02.19 and FPGA 67.

*sigh* :-( Same vendors are depressingly unable to perform even the most
basic conformance testing.

Thanks for all this analysis and bug reports.

> Therefore add a quirk to correct the malformed data structure.
> 
> The quirk was successfully tested with VLC, OBS, and Chromium using
> different pixel formats (YUYV, NV12, YU12), resolutions (3840x2160,
> 1920x1080), and frame rates (29.970 and 59.940 fps).
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Benjamin Drung <bdrung@posteo.de>
> ---
> 
> I am sending this patch a fourth time since I got no response and the
> last resend is over a month ago. This time I am including Linus Torvalds
> in the hope to get it reviewed.

The resend got to the top of my mailbox and I had time to review it
before it got burried again. Thanks for not giving up.

>  drivers/media/usb/uvc/uvc_driver.c | 13 +++++++++++++
>  drivers/media/usb/uvc/uvc_video.c  | 21 +++++++++++++++++++++
>  drivers/media/usb/uvc/uvcvideo.h   |  1 +
>  3 files changed, 35 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 9a791d8ef200..6ce58950d78b 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -3164,6 +3164,19 @@ static const struct usb_device_id uvc_ids[] = {
>  	  .bInterfaceSubClass	= 1,
>  	  .bInterfaceProtocol	= 0,
>  	  .driver_info		= UVC_INFO_META(V4L2_META_FMT_D4XX) },
> +	/*
> +	 * Elgato Cam Link 4K
> +	 * Latest firmware as of 2021-03-23 needs this quirk.
> +	 * MCU: 20.02.19, FPGA: 67
> +	 */
> +	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> +				| USB_DEVICE_ID_MATCH_INT_INFO,
> +	  .idVendor		= 0x0fd9,
> +	  .idProduct		= 0x0066,
> +	  .bInterfaceClass	= USB_CLASS_VIDEO,
> +	  .bInterfaceSubClass	= 1,
> +	  .bInterfaceProtocol	= 0,
> +	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_FIX_FORMAT_INDEX) },
>  	/* Generic USB Video Class */
>  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
>  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index a777b389a66e..910d22233d74 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -131,6 +131,27 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
>  	struct uvc_frame *frame = NULL;
>  	unsigned int i;
>  
> +	/*
> +	 * The response of the Elgato Cam Link 4K is incorrect: The second byte
> +	 * contains bFormatIndex (instead of being the second byte of bmHint).
> +	 * The first byte is always zero. The third byte is always 1.
> +	 *
> +	 * The UVC 1.5 class specification defines the first five bits in the
> +	 * bmHint bitfield. The remaining bits are reserved and should be zero.
> +	 * Therefore a valid bmHint will be less than 32.
> +	 */
> +	if (stream->dev->quirks & UVC_QUIRK_FIX_FORMAT_INDEX && ctrl->bmHint > 255) {

Given that this is likely not going to affect other devices (at least in
the same way), I'd rather test the USB VID:PID that add a quirk.
Something along the lines of

	if (usb_match_one_id(stream->dev->intf, USB_DEVICE(0x0fd9, 0x0066)) {

> +		__u8 corrected_format_index;
> +
> +		corrected_format_index = ctrl->bmHint >> 8;
> +		uvc_dbg(stream->dev, CONTROL,
> +			"Correct USB video probe response from {bmHint: 0x%04x, bFormatIndex: 0x%02x} to {bmHint: 0x%04x, bFormatIndex: 0x%02x}.\n",
> +			ctrl->bmHint, ctrl->bFormatIndex,
> +			ctrl->bFormatIndex, corrected_format_index);
> +		ctrl->bmHint = ctrl->bFormatIndex;

According to your description above, this will always be 1. Is the third
byte always 1 because the driver always sets bmHint to 1, or would it
have a different value if we set bmHint to something else ? In the first
case I'd hardcode ctrl->bmHint to 1 here.

> +		ctrl->bFormatIndex = corrected_format_index;
> +	}
> +
>  	for (i = 0; i < stream->nformats; ++i) {
>  		if (stream->format[i].index == ctrl->bFormatIndex) {
>  			format = &stream->format[i];
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index cce5e38133cd..cbb4ef61a64d 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -209,6 +209,7 @@
>  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
>  #define UVC_QUIRK_FORCE_Y8		0x00000800
>  #define UVC_QUIRK_FORCE_BPP		0x00001000
> +#define UVC_QUIRK_FIX_FORMAT_INDEX	0x00002000
>  
>  /* Format flags */
>  #define UVC_FMT_FLAG_COMPRESSED		0x00000001

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] media: uvcvideo: Fix pixel format change for Elgato Cam Link 4K
  2021-06-04 22:21         ` Laurent Pinchart
@ 2021-06-05  8:19           ` Benjamin Drung
  2021-06-05 21:51             ` Laurent Pinchart
  2021-06-05 20:05           ` [PATCH v3] " Benjamin Drung
                             ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: Benjamin Drung @ 2021-06-05  8:19 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Linus Torvalds, Adam Goode, Mauro Carvalho Chehab, linux-media,
	linux-kernel, stable

Hi Laurent,

Am Samstag, den 05.06.2021, 01:21 +0300 schrieb Laurent Pinchart:
> Hi Benjamin,
> 
> Thank you for the patch.
> 
> On Fri, Jun 04, 2021 at 05:19:42PM +0000, Benjamin Drung wrote:
> > The Elgato Cam Link 4K HDMI video capture card reports to support three
> > different pixel formats, where the first format depends on the connected
> > HDMI device.
> > 
> > ```
> > $ v4l2-ctl -d /dev/video0 --list-formats-ext
> > ioctl: VIDIOC_ENUM_FMT
> > 	Type: Video Capture
> > 
> > 	[0]: 'NV12' (Y/CbCr 4:2:0)
> > 		Size: Discrete 3840x2160
> > 			Interval: Discrete 0.033s (29.970 fps)
> > 	[1]: 'NV12' (Y/CbCr 4:2:0)
> > 		Size: Discrete 3840x2160
> > 			Interval: Discrete 0.033s (29.970 fps)
> > 	[2]: 'YU12' (Planar YUV 4:2:0)
> > 		Size: Discrete 3840x2160
> > 			Interval: Discrete 0.033s (29.970 fps)
> > ```
> > 
> > Changing the pixel format to anything besides the first pixel format
> > does not work:
> > 
> > ```
> > $ v4l2-ctl -d /dev/video0 --try-fmt-video pixelformat=YU12
> > Format Video Capture:
> > 	Width/Height      : 3840/2160
> > 	Pixel Format      : 'NV12' (Y/CbCr 4:2:0)
> > 	Field             : None
> > 	Bytes per Line    : 3840
> > 	Size Image        : 12441600
> > 	Colorspace        : sRGB
> > 	Transfer Function : Rec. 709
> > 	YCbCr/HSV Encoding: Rec. 709
> > 	Quantization      : Default (maps to Limited Range)
> > 	Flags             :
> > ```
> > 
> > User space applications like VLC might show an error message on the
> > terminal in that case:
> > 
> > ```
> > libv4l2: error set_fmt gave us a different result than try_fmt!
> > ```
> > 
> > Depending on the error handling of the user space applications, they
> > might display a distorted video, because they use the wrong pixel format
> > for decoding the stream.
> > 
> > The Elgato Cam Link 4K responds to the USB video probe
> > VS_PROBE_CONTROL/VS_COMMIT_CONTROL with a malformed data structure: The
> > second byte contains bFormatIndex (instead of being the second byte of
> > bmHint). The first byte is always zero. The third byte is always 1.
> > 
> > The firmware bug was reported to Elgato on 2020-12-01 and it was
> > forwarded by the support team to the developers as feature request.
> > There is no firmware update available since then. The latest firmware
> > for Elgato Cam Link 4K as of 2021-03-23 has MCU 20.02.19 and FPGA 67.
> 
> *sigh* :-( Same vendors are depressingly unable to perform even the most
> basic conformance testing.
> 
> Thanks for all this analysis and bug reports.
> 
> > Therefore add a quirk to correct the malformed data structure.
> > 
> > The quirk was successfully tested with VLC, OBS, and Chromium using
> > different pixel formats (YUYV, NV12, YU12), resolutions (3840x2160,
> > 1920x1080), and frame rates (29.970 and 59.940 fps).
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Benjamin Drung <bdrung@posteo.de>
> > ---
> > 
> > I am sending this patch a fourth time since I got no response and the
> > last resend is over a month ago. This time I am including Linus Torvalds
> > in the hope to get it reviewed.
> 
> The resend got to the top of my mailbox and I had time to review it
> before it got burried again. Thanks for not giving up.

Thanks for reviewing the patch.

> >  drivers/media/usb/uvc/uvc_driver.c | 13 +++++++++++++
> >  drivers/media/usb/uvc/uvc_video.c  | 21 +++++++++++++++++++++
> >  drivers/media/usb/uvc/uvcvideo.h   |  1 +
> >  3 files changed, 35 insertions(+)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 9a791d8ef200..6ce58950d78b 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -3164,6 +3164,19 @@ static const struct usb_device_id uvc_ids[] = {
> >  	  .bInterfaceSubClass	= 1,
> >  	  .bInterfaceProtocol	= 0,
> >  	  .driver_info		= UVC_INFO_META(V4L2_META_FMT_D4XX) },
> > +	/*
> > +	 * Elgato Cam Link 4K
> > +	 * Latest firmware as of 2021-03-23 needs this quirk.
> > +	 * MCU: 20.02.19, FPGA: 67
> > +	 */
> > +	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> > +				| USB_DEVICE_ID_MATCH_INT_INFO,
> > +	  .idVendor		= 0x0fd9,
> > +	  .idProduct		= 0x0066,
> > +	  .bInterfaceClass	= USB_CLASS_VIDEO,
> > +	  .bInterfaceSubClass	= 1,
> > +	  .bInterfaceProtocol	= 0,
> > +	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_FIX_FORMAT_INDEX) },
> >  	/* Generic USB Video Class */
> >  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
> >  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index a777b389a66e..910d22233d74 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -131,6 +131,27 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
> >  	struct uvc_frame *frame = NULL;
> >  	unsigned int i;
> >  
> > 
> > +	/*
> > +	 * The response of the Elgato Cam Link 4K is incorrect: The second byte
> > +	 * contains bFormatIndex (instead of being the second byte of bmHint).
> > +	 * The first byte is always zero. The third byte is always 1.
> > +	 *
> > +	 * The UVC 1.5 class specification defines the first five bits in the
> > +	 * bmHint bitfield. The remaining bits are reserved and should be zero.
> > +	 * Therefore a valid bmHint will be less than 32.
> > +	 */
> > +	if (stream->dev->quirks & UVC_QUIRK_FIX_FORMAT_INDEX && ctrl->bmHint > 255) {
> 
> Given that this is likely not going to affect other devices (at least in
> the same way), I'd rather test the USB VID:PID that add a quirk.
> Something along the lines of
> 
> 	if (usb_match_one_id(stream->dev->intf, USB_DEVICE(0x0fd9, 0x0066)) {

Adam Goode suggested that the Game Capture HD 60 S+ (0fd9:006a) from the
same vendor is probably affected by the same bug. I cannot test this
assumption since I don't have this device (I am open for a loaner
device). An Internet search did not reveal bug reports in this regard.
Most search results referred to older versions (e.g. without + or
without S+) Do you still prefer to test the USB VID:PID?

> > +		__u8 corrected_format_index;
> > +
> > +		corrected_format_index = ctrl->bmHint >> 8;
> > +		uvc_dbg(stream->dev, CONTROL,
> > +			"Correct USB video probe response from {bmHint: 0x%04x, bFormatIndex: 0x%02x} to {bmHint: 0x%04x, bFormatIndex: 0x%02x}.\n",
> > +			ctrl->bmHint, ctrl->bFormatIndex,
> > +			ctrl->bFormatIndex, corrected_format_index);
> > +		ctrl->bmHint = ctrl->bFormatIndex;
> 
> According to your description above, this will always be 1. Is the third
> byte always 1 because the driver always sets bmHint to 1, or would it
> have a different value if we set bmHint to something else ? In the first
> case I'd hardcode ctrl->bmHint to 1 here.

I will test setting bmHint to something else than 1 to check. I will
report back then. Either follow your sugstion or update the comment.

> > +		ctrl->bFormatIndex = corrected_format_index;
> > +	}
> > +
> >  	for (i = 0; i < stream->nformats; ++i) {
> >  		if (stream->format[i].index == ctrl->bFormatIndex) {
> >  			format = &stream->format[i];
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index cce5e38133cd..cbb4ef61a64d 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -209,6 +209,7 @@
> >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
> >  #define UVC_QUIRK_FORCE_Y8		0x00000800
> >  #define UVC_QUIRK_FORCE_BPP		0x00001000
> > +#define UVC_QUIRK_FIX_FORMAT_INDEX	0x00002000
> >  
> > 
> >  /* Format flags */
> >  #define UVC_FMT_FLAG_COMPRESSED		0x00000001
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] media: uvcvideo: Fix pixel format change for Elgato Cam Link 4K
  2021-06-04 22:21         ` Laurent Pinchart
  2021-06-05  8:19           ` Benjamin Drung
@ 2021-06-05 20:05           ` Benjamin Drung
  2021-06-05 20:13           ` Benjamin Drung
  2021-06-05 20:15           ` [PATCH v4] " Benjamin Drung
  3 siblings, 0 replies; 15+ messages in thread
From: Benjamin Drung @ 2021-06-05 20:05 UTC (permalink / raw)
  To: Linus Torvalds, Adam Goode, Laurent Pinchart, Mauro Carvalho Chehab
  Cc: linux-media, linux-kernel, Benjamin Drung, stable

The Elgato Cam Link 4K HDMI video capture card reports to support three
different pixel formats, where the first format depends on the connected
HDMI device.

```
$ v4l2-ctl -d /dev/video0 --list-formats-ext
ioctl: VIDIOC_ENUM_FMT
	Type: Video Capture

	[0]: 'NV12' (Y/CbCr 4:2:0)
		Size: Discrete 3840x2160
			Interval: Discrete 0.033s (29.970 fps)
	[1]: 'NV12' (Y/CbCr 4:2:0)
		Size: Discrete 3840x2160
			Interval: Discrete 0.033s (29.970 fps)
	[2]: 'YU12' (Planar YUV 4:2:0)
		Size: Discrete 3840x2160
			Interval: Discrete 0.033s (29.970 fps)
```

Changing the pixel format to anything besides the first pixel format
does not work:

```
$ v4l2-ctl -d /dev/video0 --try-fmt-video pixelformat=YU12
Format Video Capture:
	Width/Height      : 3840/2160
	Pixel Format      : 'NV12' (Y/CbCr 4:2:0)
	Field             : None
	Bytes per Line    : 3840
	Size Image        : 12441600
	Colorspace        : sRGB
	Transfer Function : Rec. 709
	YCbCr/HSV Encoding: Rec. 709
	Quantization      : Default (maps to Limited Range)
	Flags             :
```

User space applications like VLC might show an error message on the
terminal in that case:

```
libv4l2: error set_fmt gave us a different result than try_fmt!
```

Depending on the error handling of the user space applications, they
might display a distorted video, because they use the wrong pixel format
for decoding the stream.

The Elgato Cam Link 4K responds to the USB video probe
VS_PROBE_CONTROL/VS_COMMIT_CONTROL with a malformed data structure: The
second byte contains bFormatIndex (instead of being the second byte of
bmHint). The first byte is always zero. The third byte is always 1.

The firmware bug was reported to Elgato on 2020-12-01 and it was
forwarded by the support team to the developers as feature request.
There is no firmware update available since then. The latest firmware
for Elgato Cam Link 4K as of 2021-03-23 has MCU 20.02.19 and FPGA 67.

Therefore add a quirk to correct the malformed data structure.

The quirk was successfully tested with VLC, OBS, and Chromium using
different pixel formats (YUYV, NV12, YU12), resolutions (3840x2160,
1920x1080), and frame rates (29.970 and 59.940 fps).

Cc: stable@vger.kernel.org
Signed-off-by: Benjamin Drung <bdrung@posteo.de>
---
 drivers/media/usb/uvc/uvc_driver.c | 13 +++++++++++++
 drivers/media/usb/uvc/uvc_video.c  | 21 +++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 3 files changed, 35 insertions(+)

v2: enhanced the comment describing the quirk

v3:
* hardcode ctrl->bmHint to 1
* Use UVC_DBG_VIDEO instead of UVC_DBG_CONTROL (to match the rest of the
  file)

I tried setting different values for bmHint, but the response from the
Cam Link was always 1. So this patch hardcodes ctrl->bmHint to 1 as
suggested.

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 9a791d8ef200..6ce58950d78b 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -3164,6 +3164,19 @@ static const struct usb_device_id uvc_ids[] = {
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0,
 	  .driver_info		= UVC_INFO_META(V4L2_META_FMT_D4XX) },
+	/*
+	 * Elgato Cam Link 4K
+	 * Latest firmware as of 2021-03-23 needs this quirk.
+	 * MCU: 20.02.19, FPGA: 67
+	 */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x0fd9,
+	  .idProduct		= 0x0066,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_FIX_FORMAT_INDEX) },
 	/* Generic USB Video Class */
 	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
 	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index a777b389a66e..3f61cb2c9103 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -131,6 +131,27 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
 	struct uvc_frame *frame = NULL;
 	unsigned int i;
 
+	/*
+	 * The response of the Elgato Cam Link 4K is incorrect: The second byte
+	 * contains bFormatIndex (instead of being the second byte of bmHint).
+	 * The first byte is always zero. The third byte is always 1.
+	 *
+	 * The UVC 1.5 class specification defines the first five bits in the
+	 * bmHint bitfield. The remaining bits are reserved and should be zero.
+	 * Therefore a valid bmHint will be less than 32.
+	 */
+	if (stream->dev->quirks & UVC_QUIRK_FIX_FORMAT_INDEX && ctrl->bmHint > 255) {
+		__u8 corrected_format_index;
+
+		corrected_format_index = ctrl->bmHint >> 8;
+		uvc_dbg(stream->dev, VIDEO,
+			"Correct USB video probe response from {bmHint: 0x%04x, bFormatIndex: 0x%02x} to {bmHint: 0x%04x, bFormatIndex: 0x%02x}.\n",
+			ctrl->bmHint, ctrl->bFormatIndex,
+			1, corrected_format_index);
+		ctrl->bmHint = 1;
+		ctrl->bFormatIndex = corrected_format_index;
+	}
+
 	for (i = 0; i < stream->nformats; ++i) {
 		if (stream->format[i].index == ctrl->bFormatIndex) {
 			format = &stream->format[i];
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index cce5e38133cd..cbb4ef61a64d 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -209,6 +209,7 @@
 #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
 #define UVC_QUIRK_FORCE_Y8		0x00000800
 #define UVC_QUIRK_FORCE_BPP		0x00001000
+#define UVC_QUIRK_FIX_FORMAT_INDEX	0x00002000
 
 /* Format flags */
 #define UVC_FMT_FLAG_COMPRESSED		0x00000001
-- 
2.27.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] media: uvcvideo: Fix pixel format change for Elgato Cam Link 4K
  2021-06-04 22:21         ` Laurent Pinchart
  2021-06-05  8:19           ` Benjamin Drung
  2021-06-05 20:05           ` [PATCH v3] " Benjamin Drung
@ 2021-06-05 20:13           ` Benjamin Drung
  2021-06-05 20:15           ` [PATCH v4] " Benjamin Drung
  3 siblings, 0 replies; 15+ messages in thread
From: Benjamin Drung @ 2021-06-05 20:13 UTC (permalink / raw)
  To: Linus Torvalds, Adam Goode, Laurent Pinchart, Mauro Carvalho Chehab
  Cc: linux-media, linux-kernel, Benjamin Drung, stable

The Elgato Cam Link 4K HDMI video capture card reports to support three
different pixel formats, where the first format depends on the connected
HDMI device.

```
$ v4l2-ctl -d /dev/video0 --list-formats-ext
ioctl: VIDIOC_ENUM_FMT
	Type: Video Capture

	[0]: 'NV12' (Y/CbCr 4:2:0)
		Size: Discrete 3840x2160
			Interval: Discrete 0.033s (29.970 fps)
	[1]: 'NV12' (Y/CbCr 4:2:0)
		Size: Discrete 3840x2160
			Interval: Discrete 0.033s (29.970 fps)
	[2]: 'YU12' (Planar YUV 4:2:0)
		Size: Discrete 3840x2160
			Interval: Discrete 0.033s (29.970 fps)
```

Changing the pixel format to anything besides the first pixel format
does not work:

```
$ v4l2-ctl -d /dev/video0 --try-fmt-video pixelformat=YU12
Format Video Capture:
	Width/Height      : 3840/2160
	Pixel Format      : 'NV12' (Y/CbCr 4:2:0)
	Field             : None
	Bytes per Line    : 3840
	Size Image        : 12441600
	Colorspace        : sRGB
	Transfer Function : Rec. 709
	YCbCr/HSV Encoding: Rec. 709
	Quantization      : Default (maps to Limited Range)
	Flags             :
```

User space applications like VLC might show an error message on the
terminal in that case:

```
libv4l2: error set_fmt gave us a different result than try_fmt!
```

Depending on the error handling of the user space applications, they
might display a distorted video, because they use the wrong pixel format
for decoding the stream.

The Elgato Cam Link 4K responds to the USB video probe
VS_PROBE_CONTROL/VS_COMMIT_CONTROL with a malformed data structure: The
second byte contains bFormatIndex (instead of being the second byte of
bmHint). The first byte is always zero. The third byte is always 1.

The firmware bug was reported to Elgato on 2020-12-01 and it was
forwarded by the support team to the developers as feature request.
There is no firmware update available since then. The latest firmware
for Elgato Cam Link 4K as of 2021-03-23 has MCU 20.02.19 and FPGA 67.

Therefore add a quirk to correct the malformed data structure.

The quirk was successfully tested with VLC, OBS, and Chromium using
different pixel formats (YUYV, NV12, YU12), resolutions (3840x2160,
1920x1080), and frame rates (29.970 and 59.940 fps).

Cc: stable@vger.kernel.org
Signed-off-by: Benjamin Drung <bdrung@posteo.de>
---
 drivers/media/usb/uvc/uvc_video.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

v2: enhanced the comment describing the quirk

v3:
* hardcode ctrl->bmHint to 1
* Use UVC_DBG_VIDEO instead of UVC_DBG_CONTROL (to match the rest of the
  file)

v4:
* Replace quirk bit by specific check for USB VID:PID test

I tried setting different values for bmHint, but the response from the
Cam Link was always 1. So this patch hardcodes ctrl->bmHint to 1 as
suggested.

Patch version 4 implements the recommendation of Laurent Pinchart. It
requires defining the device ID as variable since usb_match_one_id takes
an pointer to it. In case more Elgato products like Game Capture
HD 60 S+ (0fd9:006a) are affected, this version is harder to extent.

Take patch version 3 or 4 depending on which version you prefer. Both
work and are tested.

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index a777b389a66e..35c3ce0e0716 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -130,6 +130,31 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
 	struct uvc_format *format = NULL;
 	struct uvc_frame *frame = NULL;
 	unsigned int i;
+	static const struct usb_device_id elgato_cam_link_4k = { USB_DEVICE(0x0fd9, 0x0066) };
+
+	/*
+	 * The response of the Elgato Cam Link 4K is incorrect: The second byte
+	 * contains bFormatIndex (instead of being the second byte of bmHint).
+	 * The first byte is always zero. The third byte is always 1.
+	 *
+	 * The UVC 1.5 class specification defines the first five bits in the
+	 * bmHint bitfield. The remaining bits are reserved and should be zero.
+	 * Therefore a valid bmHint will be less than 32.
+	 *
+	 * Latest Elgato Cam Link 4K firmware as of 2021-03-23 needs this quirk.
+	 * MCU: 20.02.19, FPGA: 67
+	 */
+	if (usb_match_one_id(stream->dev->intf, &elgato_cam_link_4k) && ctrl->bmHint > 255) {
+		__u8 corrected_format_index;
+
+		corrected_format_index = ctrl->bmHint >> 8;
+		uvc_dbg(stream->dev, VIDEO,
+			"Correct USB video probe response from {bmHint: 0x%04x, bFormatIndex: 0x%02x} to {bmHint: 0x%04x, bFormatIndex: 0x%02x}.\n",
+			ctrl->bmHint, ctrl->bFormatIndex,
+			1, corrected_format_index);
+		ctrl->bmHint = 1;
+		ctrl->bFormatIndex = corrected_format_index;
+	}
 
 	for (i = 0; i < stream->nformats; ++i) {
 		if (stream->format[i].index == ctrl->bFormatIndex) {
-- 
2.27.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v4] media: uvcvideo: Fix pixel format change for Elgato Cam Link 4K
  2021-06-04 22:21         ` Laurent Pinchart
                             ` (2 preceding siblings ...)
  2021-06-05 20:13           ` Benjamin Drung
@ 2021-06-05 20:15           ` Benjamin Drung
  2021-06-05 21:56             ` Laurent Pinchart
  3 siblings, 1 reply; 15+ messages in thread
From: Benjamin Drung @ 2021-06-05 20:15 UTC (permalink / raw)
  To: Linus Torvalds, Adam Goode, Laurent Pinchart, Mauro Carvalho Chehab
  Cc: linux-media, linux-kernel, Benjamin Drung, stable

The Elgato Cam Link 4K HDMI video capture card reports to support three
different pixel formats, where the first format depends on the connected
HDMI device.

```
$ v4l2-ctl -d /dev/video0 --list-formats-ext
ioctl: VIDIOC_ENUM_FMT
	Type: Video Capture

	[0]: 'NV12' (Y/CbCr 4:2:0)
		Size: Discrete 3840x2160
			Interval: Discrete 0.033s (29.970 fps)
	[1]: 'NV12' (Y/CbCr 4:2:0)
		Size: Discrete 3840x2160
			Interval: Discrete 0.033s (29.970 fps)
	[2]: 'YU12' (Planar YUV 4:2:0)
		Size: Discrete 3840x2160
			Interval: Discrete 0.033s (29.970 fps)
```

Changing the pixel format to anything besides the first pixel format
does not work:

```
$ v4l2-ctl -d /dev/video0 --try-fmt-video pixelformat=YU12
Format Video Capture:
	Width/Height      : 3840/2160
	Pixel Format      : 'NV12' (Y/CbCr 4:2:0)
	Field             : None
	Bytes per Line    : 3840
	Size Image        : 12441600
	Colorspace        : sRGB
	Transfer Function : Rec. 709
	YCbCr/HSV Encoding: Rec. 709
	Quantization      : Default (maps to Limited Range)
	Flags             :
```

User space applications like VLC might show an error message on the
terminal in that case:

```
libv4l2: error set_fmt gave us a different result than try_fmt!
```

Depending on the error handling of the user space applications, they
might display a distorted video, because they use the wrong pixel format
for decoding the stream.

The Elgato Cam Link 4K responds to the USB video probe
VS_PROBE_CONTROL/VS_COMMIT_CONTROL with a malformed data structure: The
second byte contains bFormatIndex (instead of being the second byte of
bmHint). The first byte is always zero. The third byte is always 1.

The firmware bug was reported to Elgato on 2020-12-01 and it was
forwarded by the support team to the developers as feature request.
There is no firmware update available since then. The latest firmware
for Elgato Cam Link 4K as of 2021-03-23 has MCU 20.02.19 and FPGA 67.

Therefore add a quirk to correct the malformed data structure.

The quirk was successfully tested with VLC, OBS, and Chromium using
different pixel formats (YUYV, NV12, YU12), resolutions (3840x2160,
1920x1080), and frame rates (29.970 and 59.940 fps).

Cc: stable@vger.kernel.org
Signed-off-by: Benjamin Drung <bdrung@posteo.de>
---
 drivers/media/usb/uvc/uvc_video.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

(now sending this patch with v4 in the subject instead of falsely v3)

v2: enhanced the comment describing the quirk

v3:
* hardcode ctrl->bmHint to 1
* Use UVC_DBG_VIDEO instead of UVC_DBG_CONTROL (to match the rest of the
  file)

v4:
* Replace quirk bit by specific check for USB VID:PID test

I tried setting different values for bmHint, but the response from the
Cam Link was always 1. So this patch hardcodes ctrl->bmHint to 1 as
suggested.

Patch version 4 implements the recommendation of Laurent Pinchart. It
requires defining the device ID as variable since usb_match_one_id takes
an pointer to it. In case more Elgato products like Game Capture
HD 60 S+ (0fd9:006a) are affected, this version is harder to extent.

Take patch version 3 or 4 depending on which version you prefer. Both
work and are tested.

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index a777b389a66e..35c3ce0e0716 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -130,6 +130,31 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
 	struct uvc_format *format = NULL;
 	struct uvc_frame *frame = NULL;
 	unsigned int i;
+	static const struct usb_device_id elgato_cam_link_4k = { USB_DEVICE(0x0fd9, 0x0066) };
+
+	/*
+	 * The response of the Elgato Cam Link 4K is incorrect: The second byte
+	 * contains bFormatIndex (instead of being the second byte of bmHint).
+	 * The first byte is always zero. The third byte is always 1.
+	 *
+	 * The UVC 1.5 class specification defines the first five bits in the
+	 * bmHint bitfield. The remaining bits are reserved and should be zero.
+	 * Therefore a valid bmHint will be less than 32.
+	 *
+	 * Latest Elgato Cam Link 4K firmware as of 2021-03-23 needs this quirk.
+	 * MCU: 20.02.19, FPGA: 67
+	 */
+	if (usb_match_one_id(stream->dev->intf, &elgato_cam_link_4k) && ctrl->bmHint > 255) {
+		__u8 corrected_format_index;
+
+		corrected_format_index = ctrl->bmHint >> 8;
+		uvc_dbg(stream->dev, VIDEO,
+			"Correct USB video probe response from {bmHint: 0x%04x, bFormatIndex: 0x%02x} to {bmHint: 0x%04x, bFormatIndex: 0x%02x}.\n",
+			ctrl->bmHint, ctrl->bFormatIndex,
+			1, corrected_format_index);
+		ctrl->bmHint = 1;
+		ctrl->bFormatIndex = corrected_format_index;
+	}
 
 	for (i = 0; i < stream->nformats; ++i) {
 		if (stream->format[i].index == ctrl->bFormatIndex) {
-- 
2.27.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] media: uvcvideo: Fix pixel format change for Elgato Cam Link 4K
  2021-06-05  8:19           ` Benjamin Drung
@ 2021-06-05 21:51             ` Laurent Pinchart
  2021-06-05 23:10               ` Benjamin Drung
  0 siblings, 1 reply; 15+ messages in thread
From: Laurent Pinchart @ 2021-06-05 21:51 UTC (permalink / raw)
  To: Benjamin Drung
  Cc: Linus Torvalds, Adam Goode, Mauro Carvalho Chehab, linux-media,
	linux-kernel, stable

Hi Benjamin,

On Sat, Jun 05, 2021 at 08:19:56AM +0000, Benjamin Drung wrote:
> Am Samstag, den 05.06.2021, 01:21 +0300 schrieb Laurent Pinchart:
> > On Fri, Jun 04, 2021 at 05:19:42PM +0000, Benjamin Drung wrote:
> > > The Elgato Cam Link 4K HDMI video capture card reports to support three
> > > different pixel formats, where the first format depends on the connected
> > > HDMI device.
> > > 
> > > ```
> > > $ v4l2-ctl -d /dev/video0 --list-formats-ext
> > > ioctl: VIDIOC_ENUM_FMT
> > > 	Type: Video Capture
> > > 
> > > 	[0]: 'NV12' (Y/CbCr 4:2:0)
> > > 		Size: Discrete 3840x2160
> > > 			Interval: Discrete 0.033s (29.970 fps)
> > > 	[1]: 'NV12' (Y/CbCr 4:2:0)
> > > 		Size: Discrete 3840x2160
> > > 			Interval: Discrete 0.033s (29.970 fps)
> > > 	[2]: 'YU12' (Planar YUV 4:2:0)
> > > 		Size: Discrete 3840x2160
> > > 			Interval: Discrete 0.033s (29.970 fps)
> > > ```
> > > 
> > > Changing the pixel format to anything besides the first pixel format
> > > does not work:
> > > 
> > > ```
> > > $ v4l2-ctl -d /dev/video0 --try-fmt-video pixelformat=YU12
> > > Format Video Capture:
> > > 	Width/Height      : 3840/2160
> > > 	Pixel Format      : 'NV12' (Y/CbCr 4:2:0)
> > > 	Field             : None
> > > 	Bytes per Line    : 3840
> > > 	Size Image        : 12441600
> > > 	Colorspace        : sRGB
> > > 	Transfer Function : Rec. 709
> > > 	YCbCr/HSV Encoding: Rec. 709
> > > 	Quantization      : Default (maps to Limited Range)
> > > 	Flags             :
> > > ```
> > > 
> > > User space applications like VLC might show an error message on the
> > > terminal in that case:
> > > 
> > > ```
> > > libv4l2: error set_fmt gave us a different result than try_fmt!
> > > ```
> > > 
> > > Depending on the error handling of the user space applications, they
> > > might display a distorted video, because they use the wrong pixel format
> > > for decoding the stream.
> > > 
> > > The Elgato Cam Link 4K responds to the USB video probe
> > > VS_PROBE_CONTROL/VS_COMMIT_CONTROL with a malformed data structure: The
> > > second byte contains bFormatIndex (instead of being the second byte of
> > > bmHint). The first byte is always zero. The third byte is always 1.
> > > 
> > > The firmware bug was reported to Elgato on 2020-12-01 and it was
> > > forwarded by the support team to the developers as feature request.
> > > There is no firmware update available since then. The latest firmware
> > > for Elgato Cam Link 4K as of 2021-03-23 has MCU 20.02.19 and FPGA 67.
> > 
> > *sigh* :-( Same vendors are depressingly unable to perform even the most
> > basic conformance testing.
> > 
> > Thanks for all this analysis and bug reports.
> > 
> > > Therefore add a quirk to correct the malformed data structure.
> > > 
> > > The quirk was successfully tested with VLC, OBS, and Chromium using
> > > different pixel formats (YUYV, NV12, YU12), resolutions (3840x2160,
> > > 1920x1080), and frame rates (29.970 and 59.940 fps).
> > > 
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Benjamin Drung <bdrung@posteo.de>
> > > ---
> > > 
> > > I am sending this patch a fourth time since I got no response and the
> > > last resend is over a month ago. This time I am including Linus Torvalds
> > > in the hope to get it reviewed.
> > 
> > The resend got to the top of my mailbox and I had time to review it
> > before it got burried again. Thanks for not giving up.
> 
> Thanks for reviewing the patch.

I'll try not to be that late for v3/v4 :-)

> > >  drivers/media/usb/uvc/uvc_driver.c | 13 +++++++++++++
> > >  drivers/media/usb/uvc/uvc_video.c  | 21 +++++++++++++++++++++
> > >  drivers/media/usb/uvc/uvcvideo.h   |  1 +
> > >  3 files changed, 35 insertions(+)
> > > 
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index 9a791d8ef200..6ce58950d78b 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -3164,6 +3164,19 @@ static const struct usb_device_id uvc_ids[] = {
> > >  	  .bInterfaceSubClass	= 1,
> > >  	  .bInterfaceProtocol	= 0,
> > >  	  .driver_info		= UVC_INFO_META(V4L2_META_FMT_D4XX) },
> > > +	/*
> > > +	 * Elgato Cam Link 4K
> > > +	 * Latest firmware as of 2021-03-23 needs this quirk.
> > > +	 * MCU: 20.02.19, FPGA: 67
> > > +	 */
> > > +	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> > > +				| USB_DEVICE_ID_MATCH_INT_INFO,
> > > +	  .idVendor		= 0x0fd9,
> > > +	  .idProduct		= 0x0066,
> > > +	  .bInterfaceClass	= USB_CLASS_VIDEO,
> > > +	  .bInterfaceSubClass	= 1,
> > > +	  .bInterfaceProtocol	= 0,
> > > +	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_FIX_FORMAT_INDEX) },
> > >  	/* Generic USB Video Class */
> > >  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
> > >  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
> > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > index a777b389a66e..910d22233d74 100644
> > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > @@ -131,6 +131,27 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
> > >  	struct uvc_frame *frame = NULL;
> > >  	unsigned int i;
> > >  
> > > 
> > > +	/*
> > > +	 * The response of the Elgato Cam Link 4K is incorrect: The second byte
> > > +	 * contains bFormatIndex (instead of being the second byte of bmHint).
> > > +	 * The first byte is always zero. The third byte is always 1.
> > > +	 *
> > > +	 * The UVC 1.5 class specification defines the first five bits in the
> > > +	 * bmHint bitfield. The remaining bits are reserved and should be zero.
> > > +	 * Therefore a valid bmHint will be less than 32.
> > > +	 */
> > > +	if (stream->dev->quirks & UVC_QUIRK_FIX_FORMAT_INDEX && ctrl->bmHint > 255) {
> > 
> > Given that this is likely not going to affect other devices (at least in
> > the same way), I'd rather test the USB VID:PID that add a quirk.
> > Something along the lines of
> > 
> > 	if (usb_match_one_id(stream->dev->intf, USB_DEVICE(0x0fd9, 0x0066)) {
> 
> Adam Goode suggested that the Game Capture HD 60 S+ (0fd9:006a) from the
> same vendor is probably affected by the same bug. I cannot test this
> assumption since I don't have this device (I am open for a loaner
> device). An Internet search did not reveal bug reports in this regard.
> Most search results referred to older versions (e.g. without + or
> without S+) Do you still prefer to test the USB VID:PID?

What bothers me a bit with a quirk is that it's supposed to be a generic
mechanism for bugs that affect a wide variety of devices. We could have
a small array of device match entries as in uvc_ctrl_prune_entity() if
you don't expect more than a handful of devices to be affected.
Otherwise, if you think a quirk is better, let's go for that, but let's
then name it with the vendor name (UVC_QUIRK_ELGATE_VIDEO_CONTROL or
similar).

> > > +		__u8 corrected_format_index;
> > > +
> > > +		corrected_format_index = ctrl->bmHint >> 8;
> > > +		uvc_dbg(stream->dev, CONTROL,
> > > +			"Correct USB video probe response from {bmHint: 0x%04x, bFormatIndex: 0x%02x} to {bmHint: 0x%04x, bFormatIndex: 0x%02x}.\n",
> > > +			ctrl->bmHint, ctrl->bFormatIndex,
> > > +			ctrl->bFormatIndex, corrected_format_index);
> > > +		ctrl->bmHint = ctrl->bFormatIndex;
> > 
> > According to your description above, this will always be 1. Is the third
> > byte always 1 because the driver always sets bmHint to 1, or would it
> > have a different value if we set bmHint to something else ? In the first
> > case I'd hardcode ctrl->bmHint to 1 here.
> 
> I will test setting bmHint to something else than 1 to check. I will
> report back then. Either follow your sugstion or update the comment.

Thanks.

> > > +		ctrl->bFormatIndex = corrected_format_index;
> > > +	}
> > > +
> > >  	for (i = 0; i < stream->nformats; ++i) {
> > >  		if (stream->format[i].index == ctrl->bFormatIndex) {
> > >  			format = &stream->format[i];
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index cce5e38133cd..cbb4ef61a64d 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -209,6 +209,7 @@
> > >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
> > >  #define UVC_QUIRK_FORCE_Y8		0x00000800
> > >  #define UVC_QUIRK_FORCE_BPP		0x00001000
> > > +#define UVC_QUIRK_FIX_FORMAT_INDEX	0x00002000
> > >  
> > >  /* Format flags */
> > >  #define UVC_FMT_FLAG_COMPRESSED		0x00000001

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] media: uvcvideo: Fix pixel format change for Elgato Cam Link 4K
  2021-06-05 20:15           ` [PATCH v4] " Benjamin Drung
@ 2021-06-05 21:56             ` Laurent Pinchart
  2021-06-05 22:58               ` Benjamin Drung
  0 siblings, 1 reply; 15+ messages in thread
From: Laurent Pinchart @ 2021-06-05 21:56 UTC (permalink / raw)
  To: Benjamin Drung
  Cc: Linus Torvalds, Adam Goode, Mauro Carvalho Chehab, linux-media,
	linux-kernel, stable

Hi Benjamin,

Thank you for the patch.

On Sat, Jun 05, 2021 at 08:15:36PM +0000, Benjamin Drung wrote:
> The Elgato Cam Link 4K HDMI video capture card reports to support three
> different pixel formats, where the first format depends on the connected
> HDMI device.
> 
> ```
> $ v4l2-ctl -d /dev/video0 --list-formats-ext
> ioctl: VIDIOC_ENUM_FMT
> 	Type: Video Capture
> 
> 	[0]: 'NV12' (Y/CbCr 4:2:0)
> 		Size: Discrete 3840x2160
> 			Interval: Discrete 0.033s (29.970 fps)
> 	[1]: 'NV12' (Y/CbCr 4:2:0)
> 		Size: Discrete 3840x2160
> 			Interval: Discrete 0.033s (29.970 fps)
> 	[2]: 'YU12' (Planar YUV 4:2:0)
> 		Size: Discrete 3840x2160
> 			Interval: Discrete 0.033s (29.970 fps)
> ```
> 
> Changing the pixel format to anything besides the first pixel format
> does not work:
> 
> ```
> $ v4l2-ctl -d /dev/video0 --try-fmt-video pixelformat=YU12
> Format Video Capture:
> 	Width/Height      : 3840/2160
> 	Pixel Format      : 'NV12' (Y/CbCr 4:2:0)
> 	Field             : None
> 	Bytes per Line    : 3840
> 	Size Image        : 12441600
> 	Colorspace        : sRGB
> 	Transfer Function : Rec. 709
> 	YCbCr/HSV Encoding: Rec. 709
> 	Quantization      : Default (maps to Limited Range)
> 	Flags             :
> ```
> 
> User space applications like VLC might show an error message on the
> terminal in that case:
> 
> ```
> libv4l2: error set_fmt gave us a different result than try_fmt!
> ```
> 
> Depending on the error handling of the user space applications, they
> might display a distorted video, because they use the wrong pixel format
> for decoding the stream.
> 
> The Elgato Cam Link 4K responds to the USB video probe
> VS_PROBE_CONTROL/VS_COMMIT_CONTROL with a malformed data structure: The
> second byte contains bFormatIndex (instead of being the second byte of
> bmHint). The first byte is always zero. The third byte is always 1.
> 
> The firmware bug was reported to Elgato on 2020-12-01 and it was
> forwarded by the support team to the developers as feature request.
> There is no firmware update available since then. The latest firmware
> for Elgato Cam Link 4K as of 2021-03-23 has MCU 20.02.19 and FPGA 67.
> 
> Therefore add a quirk to correct the malformed data structure.
> 
> The quirk was successfully tested with VLC, OBS, and Chromium using
> different pixel formats (YUYV, NV12, YU12), resolutions (3840x2160,
> 1920x1080), and frame rates (29.970 and 59.940 fps).
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Benjamin Drung <bdrung@posteo.de>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> (now sending this patch with v4 in the subject instead of falsely v3)
> 
> v2: enhanced the comment describing the quirk
> 
> v3:
> * hardcode ctrl->bmHint to 1
> * Use UVC_DBG_VIDEO instead of UVC_DBG_CONTROL (to match the rest of the
>   file)
> 
> v4:
> * Replace quirk bit by specific check for USB VID:PID test
> 
> I tried setting different values for bmHint, but the response from the
> Cam Link was always 1. So this patch hardcodes ctrl->bmHint to 1 as
> suggested.
> 
> Patch version 4 implements the recommendation of Laurent Pinchart. It
> requires defining the device ID as variable since usb_match_one_id takes
> an pointer to it. In case more Elgato products like Game Capture
> HD 60 S+ (0fd9:006a) are affected, this version is harder to extent.
> 
> Take patch version 3 or 4 depending on which version you prefer. Both
> work and are tested.
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index a777b389a66e..35c3ce0e0716 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -130,6 +130,31 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
>  	struct uvc_format *format = NULL;
>  	struct uvc_frame *frame = NULL;
>  	unsigned int i;
> +	static const struct usb_device_id elgato_cam_link_4k = { USB_DEVICE(0x0fd9, 0x0066) };

Let's avoid long line

	static const struct usb_device_id elgato_cam_link_4k = {
		USB_DEVICE(0x0fd9, 0x0066)
	};

> +
> +	/*
> +	 * The response of the Elgato Cam Link 4K is incorrect: The second byte
> +	 * contains bFormatIndex (instead of being the second byte of bmHint).
> +	 * The first byte is always zero. The third byte is always 1.
> +	 *
> +	 * The UVC 1.5 class specification defines the first five bits in the
> +	 * bmHint bitfield. The remaining bits are reserved and should be zero.
> +	 * Therefore a valid bmHint will be less than 32.
> +	 *
> +	 * Latest Elgato Cam Link 4K firmware as of 2021-03-23 needs this quirk.
> +	 * MCU: 20.02.19, FPGA: 67
> +	 */
> +	if (usb_match_one_id(stream->dev->intf, &elgato_cam_link_4k) && ctrl->bmHint > 255) {

Similarly, I'd break this as

	if (usb_match_one_id(stream->dev->intf, &elgato_cam_link_4k) &&
	    ctrl->bmHint > 255) {

> +		__u8 corrected_format_index;

You can use u8 within the kernel.

> +
> +		corrected_format_index = ctrl->bmHint >> 8;
> +		uvc_dbg(stream->dev, VIDEO,
> +			"Correct USB video probe response from {bmHint: 0x%04x, bFormatIndex: 0x%02x} to {bmHint: 0x%04x, bFormatIndex: 0x%02x}.\n",

I'd print bFormatIndex with %u as it's an index and thus more readable
as a decimal integer.

If you agree with those small changes, there's no need to resubmit, I
can fold them in when applying the patch.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +			ctrl->bmHint, ctrl->bFormatIndex,
> +			1, corrected_format_index);
> +		ctrl->bmHint = 1;
> +		ctrl->bFormatIndex = corrected_format_index;
> +	}
>  
>  	for (i = 0; i < stream->nformats; ++i) {
>  		if (stream->format[i].index == ctrl->bFormatIndex) {

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] media: uvcvideo: Fix pixel format change for Elgato Cam Link 4K
  2021-06-05 21:56             ` Laurent Pinchart
@ 2021-06-05 22:58               ` Benjamin Drung
  0 siblings, 0 replies; 15+ messages in thread
From: Benjamin Drung @ 2021-06-05 22:58 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Linus Torvalds, Adam Goode, Mauro Carvalho Chehab, linux-media,
	linux-kernel, stable

Am Sonntag, den 06.06.2021, 00:56 +0300 schrieb Laurent Pinchart:
> Hi Benjamin,
> 
> Thank you for the patch.
> 
> On Sat, Jun 05, 2021 at 08:15:36PM +0000, Benjamin Drung wrote:
> > The Elgato Cam Link 4K HDMI video capture card reports to support three
> > different pixel formats, where the first format depends on the connected
> > HDMI device.
> > 
> > ```
> > $ v4l2-ctl -d /dev/video0 --list-formats-ext
> > ioctl: VIDIOC_ENUM_FMT
> > 	Type: Video Capture
> > 
> > 	[0]: 'NV12' (Y/CbCr 4:2:0)
> > 		Size: Discrete 3840x2160
> > 			Interval: Discrete 0.033s (29.970 fps)
> > 	[1]: 'NV12' (Y/CbCr 4:2:0)
> > 		Size: Discrete 3840x2160
> > 			Interval: Discrete 0.033s (29.970 fps)
> > 	[2]: 'YU12' (Planar YUV 4:2:0)
> > 		Size: Discrete 3840x2160
> > 			Interval: Discrete 0.033s (29.970 fps)
> > ```
> > 
> > Changing the pixel format to anything besides the first pixel format
> > does not work:
> > 
> > ```
> > $ v4l2-ctl -d /dev/video0 --try-fmt-video pixelformat=YU12
> > Format Video Capture:
> > 	Width/Height      : 3840/2160
> > 	Pixel Format      : 'NV12' (Y/CbCr 4:2:0)
> > 	Field             : None
> > 	Bytes per Line    : 3840
> > 	Size Image        : 12441600
> > 	Colorspace        : sRGB
> > 	Transfer Function : Rec. 709
> > 	YCbCr/HSV Encoding: Rec. 709
> > 	Quantization      : Default (maps to Limited Range)
> > 	Flags             :
> > ```
> > 
> > User space applications like VLC might show an error message on the
> > terminal in that case:
> > 
> > ```
> > libv4l2: error set_fmt gave us a different result than try_fmt!
> > ```
> > 
> > Depending on the error handling of the user space applications, they
> > might display a distorted video, because they use the wrong pixel format
> > for decoding the stream.
> > 
> > The Elgato Cam Link 4K responds to the USB video probe
> > VS_PROBE_CONTROL/VS_COMMIT_CONTROL with a malformed data structure: The
> > second byte contains bFormatIndex (instead of being the second byte of
> > bmHint). The first byte is always zero. The third byte is always 1.
> > 
> > The firmware bug was reported to Elgato on 2020-12-01 and it was
> > forwarded by the support team to the developers as feature request.
> > There is no firmware update available since then. The latest firmware
> > for Elgato Cam Link 4K as of 2021-03-23 has MCU 20.02.19 and FPGA 67.
> > 
> > Therefore add a quirk to correct the malformed data structure.
> > 
> > The quirk was successfully tested with VLC, OBS, and Chromium using
> > different pixel formats (YUYV, NV12, YU12), resolutions (3840x2160,
> > 1920x1080), and frame rates (29.970 and 59.940 fps).
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Benjamin Drung <bdrung@posteo.de>
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > (now sending this patch with v4 in the subject instead of falsely v3)
> > 
> > v2: enhanced the comment describing the quirk
> > 
> > v3:
> > * hardcode ctrl->bmHint to 1
> > * Use UVC_DBG_VIDEO instead of UVC_DBG_CONTROL (to match the rest of the
> >   file)
> > 
> > v4:
> > * Replace quirk bit by specific check for USB VID:PID test
> > 
> > I tried setting different values for bmHint, but the response from the
> > Cam Link was always 1. So this patch hardcodes ctrl->bmHint to 1 as
> > suggested.
> > 
> > Patch version 4 implements the recommendation of Laurent Pinchart. It
> > requires defining the device ID as variable since usb_match_one_id takes
> > an pointer to it. In case more Elgato products like Game Capture
> > HD 60 S+ (0fd9:006a) are affected, this version is harder to extent.
> > 
> > Take patch version 3 or 4 depending on which version you prefer. Both
> > work and are tested.
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index a777b389a66e..35c3ce0e0716 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -130,6 +130,31 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
> >  	struct uvc_format *format = NULL;
> >  	struct uvc_frame *frame = NULL;
> >  	unsigned int i;
> > +	static const struct usb_device_id elgato_cam_link_4k = { USB_DEVICE(0x0fd9, 0x0066) };
> 
> Let's avoid long line
> 
> 	static const struct usb_device_id elgato_cam_link_4k = {
> 		USB_DEVICE(0x0fd9, 0x0066)
> 	};

Yes. That's better readable.

> > +
> > +	/*
> > +	 * The response of the Elgato Cam Link 4K is incorrect: The second byte
> > +	 * contains bFormatIndex (instead of being the second byte of bmHint).
> > +	 * The first byte is always zero. The third byte is always 1.
> > +	 *
> > +	 * The UVC 1.5 class specification defines the first five bits in the
> > +	 * bmHint bitfield. The remaining bits are reserved and should be zero.
> > +	 * Therefore a valid bmHint will be less than 32.
> > +	 *
> > +	 * Latest Elgato Cam Link 4K firmware as of 2021-03-23 needs this quirk.
> > +	 * MCU: 20.02.19, FPGA: 67
> > +	 */
> > +	if (usb_match_one_id(stream->dev->intf, &elgato_cam_link_4k) && ctrl->bmHint > 255) {
> 
> Similarly, I'd break this as
> 
> 	if (usb_match_one_id(stream->dev->intf, &elgato_cam_link_4k) &&
> 	    ctrl->bmHint > 255) {

Agreed.

> > +		__u8 corrected_format_index;
> 
> You can use u8 within the kernel.

Good to know. I can't remember why I used __u8 instead.

> > +
> > +		corrected_format_index = ctrl->bmHint >> 8;
> > +		uvc_dbg(stream->dev, VIDEO,
> > +			"Correct USB video probe response from {bmHint: 0x%04x, bFormatIndex: 0x%02x} to {bmHint: 0x%04x, bFormatIndex: 0x%02x}.\n",
> 
> I'd print bFormatIndex with %u as it's an index and thus more readable
> as a decimal integer.

So print bmHint as hex and bFormatIndex as integer? That would change
the log from e.g.

uvcvideo: Correct USB video probe response from {bmHint: 0x0300, bFormatIndex: 0x01} to {bmHint: 0x0001, bFormatIndex: 0x03}.

to

uvcvideo: Correct USB video probe response from {bmHint: 0x0300, bFormatIndex: 1} to {bmHint: 0x0001, bFormatIndex: 3}.

That would be fine for me as well (since bFormatIndex is only one byte).

> If you agree with those small changes, there's no need to resubmit, I
> can fold them in when applying the patch.

I agree. Thanks for reviewing it.

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +			ctrl->bmHint, ctrl->bFormatIndex,
> > +			1, corrected_format_index);
> > +		ctrl->bmHint = 1;
> > +		ctrl->bFormatIndex = corrected_format_index;
> > +	}
> >  
> > 
> >  	for (i = 0; i < stream->nformats; ++i) {
> >  		if (stream->format[i].index == ctrl->bFormatIndex) {
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] media: uvcvideo: Fix pixel format change for Elgato Cam Link 4K
  2021-06-05 21:51             ` Laurent Pinchart
@ 2021-06-05 23:10               ` Benjamin Drung
  0 siblings, 0 replies; 15+ messages in thread
From: Benjamin Drung @ 2021-06-05 23:10 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Linus Torvalds, Adam Goode, Mauro Carvalho Chehab, linux-media,
	linux-kernel, stable

Am Sonntag, den 06.06.2021, 00:51 +0300 schrieb Laurent Pinchart:
> Hi Benjamin,
> 
> On Sat, Jun 05, 2021 at 08:19:56AM +0000, Benjamin Drung wrote:
> > Am Samstag, den 05.06.2021, 01:21 +0300 schrieb Laurent Pinchart:
> > > On Fri, Jun 04, 2021 at 05:19:42PM +0000, Benjamin Drung wrote:
> > > > The Elgato Cam Link 4K HDMI video capture card reports to support three
> > > > different pixel formats, where the first format depends on the connected
> > > > HDMI device.
> > > > 
> > > > ```
> > > > $ v4l2-ctl -d /dev/video0 --list-formats-ext
> > > > ioctl: VIDIOC_ENUM_FMT
> > > > 	Type: Video Capture
> > > > 
> > > > 	[0]: 'NV12' (Y/CbCr 4:2:0)
> > > > 		Size: Discrete 3840x2160
> > > > 			Interval: Discrete 0.033s (29.970 fps)
> > > > 	[1]: 'NV12' (Y/CbCr 4:2:0)
> > > > 		Size: Discrete 3840x2160
> > > > 			Interval: Discrete 0.033s (29.970 fps)
> > > > 	[2]: 'YU12' (Planar YUV 4:2:0)
> > > > 		Size: Discrete 3840x2160
> > > > 			Interval: Discrete 0.033s (29.970 fps)
> > > > ```
> > > > 
> > > > Changing the pixel format to anything besides the first pixel format
> > > > does not work:
> > > > 
> > > > ```
> > > > $ v4l2-ctl -d /dev/video0 --try-fmt-video pixelformat=YU12
> > > > Format Video Capture:
> > > > 	Width/Height      : 3840/2160
> > > > 	Pixel Format      : 'NV12' (Y/CbCr 4:2:0)
> > > > 	Field             : None
> > > > 	Bytes per Line    : 3840
> > > > 	Size Image        : 12441600
> > > > 	Colorspace        : sRGB
> > > > 	Transfer Function : Rec. 709
> > > > 	YCbCr/HSV Encoding: Rec. 709
> > > > 	Quantization      : Default (maps to Limited Range)
> > > > 	Flags             :
> > > > ```
> > > > 
> > > > User space applications like VLC might show an error message on the
> > > > terminal in that case:
> > > > 
> > > > ```
> > > > libv4l2: error set_fmt gave us a different result than try_fmt!
> > > > ```
> > > > 
> > > > Depending on the error handling of the user space applications, they
> > > > might display a distorted video, because they use the wrong pixel format
> > > > for decoding the stream.
> > > > 
> > > > The Elgato Cam Link 4K responds to the USB video probe
> > > > VS_PROBE_CONTROL/VS_COMMIT_CONTROL with a malformed data structure: The
> > > > second byte contains bFormatIndex (instead of being the second byte of
> > > > bmHint). The first byte is always zero. The third byte is always 1.
> > > > 
> > > > The firmware bug was reported to Elgato on 2020-12-01 and it was
> > > > forwarded by the support team to the developers as feature request.
> > > > There is no firmware update available since then. The latest firmware
> > > > for Elgato Cam Link 4K as of 2021-03-23 has MCU 20.02.19 and FPGA 67.
> > > 
> > > *sigh* :-( Same vendors are depressingly unable to perform even the most
> > > basic conformance testing.
> > > 
> > > Thanks for all this analysis and bug reports.
> > > 
> > > > Therefore add a quirk to correct the malformed data structure.
> > > > 
> > > > The quirk was successfully tested with VLC, OBS, and Chromium using
> > > > different pixel formats (YUYV, NV12, YU12), resolutions (3840x2160,
> > > > 1920x1080), and frame rates (29.970 and 59.940 fps).
> > > > 
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Benjamin Drung <bdrung@posteo.de>
> > > > ---
> > > > 
> > > > I am sending this patch a fourth time since I got no response and the
> > > > last resend is over a month ago. This time I am including Linus Torvalds
> > > > in the hope to get it reviewed.
> > > 
> > > The resend got to the top of my mailbox and I had time to review it
> > > before it got burried again. Thanks for not giving up.
> > 
> > Thanks for reviewing the patch.
> 
> I'll try not to be that late for v3/v4 :-)

Thanks. :)

> > > >  drivers/media/usb/uvc/uvc_driver.c | 13 +++++++++++++
> > > >  drivers/media/usb/uvc/uvc_video.c  | 21 +++++++++++++++++++++
> > > >  drivers/media/usb/uvc/uvcvideo.h   |  1 +
> > > >  3 files changed, 35 insertions(+)
> > > > 
> > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > index 9a791d8ef200..6ce58950d78b 100644
> > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > @@ -3164,6 +3164,19 @@ static const struct usb_device_id uvc_ids[] = {
> > > >  	  .bInterfaceSubClass	= 1,
> > > >  	  .bInterfaceProtocol	= 0,
> > > >  	  .driver_info		= UVC_INFO_META(V4L2_META_FMT_D4XX) },
> > > > +	/*
> > > > +	 * Elgato Cam Link 4K
> > > > +	 * Latest firmware as of 2021-03-23 needs this quirk.
> > > > +	 * MCU: 20.02.19, FPGA: 67
> > > > +	 */
> > > > +	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> > > > +				| USB_DEVICE_ID_MATCH_INT_INFO,
> > > > +	  .idVendor		= 0x0fd9,
> > > > +	  .idProduct		= 0x0066,
> > > > +	  .bInterfaceClass	= USB_CLASS_VIDEO,
> > > > +	  .bInterfaceSubClass	= 1,
> > > > +	  .bInterfaceProtocol	= 0,
> > > > +	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_FIX_FORMAT_INDEX) },
> > > >  	/* Generic USB Video Class */
> > > >  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
> > > >  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
> > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > index a777b389a66e..910d22233d74 100644
> > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > @@ -131,6 +131,27 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
> > > >  	struct uvc_frame *frame = NULL;
> > > >  	unsigned int i;
> > > >  
> > > > 
> > > > 
> > > > +	/*
> > > > +	 * The response of the Elgato Cam Link 4K is incorrect: The second byte
> > > > +	 * contains bFormatIndex (instead of being the second byte of bmHint).
> > > > +	 * The first byte is always zero. The third byte is always 1.
> > > > +	 *
> > > > +	 * The UVC 1.5 class specification defines the first five bits in the
> > > > +	 * bmHint bitfield. The remaining bits are reserved and should be zero.
> > > > +	 * Therefore a valid bmHint will be less than 32.
> > > > +	 */
> > > > +	if (stream->dev->quirks & UVC_QUIRK_FIX_FORMAT_INDEX && ctrl->bmHint > 255) {
> > > 
> > > Given that this is likely not going to affect other devices (at least in
> > > the same way), I'd rather test the USB VID:PID that add a quirk.
> > > Something along the lines of
> > > 
> > > 	if (usb_match_one_id(stream->dev->intf, USB_DEVICE(0x0fd9, 0x0066)) {
> > 
> > Adam Goode suggested that the Game Capture HD 60 S+ (0fd9:006a) from the
> > same vendor is probably affected by the same bug. I cannot test this
> > assumption since I don't have this device (I am open for a loaner
> > device). An Internet search did not reveal bug reports in this regard.
> > Most search results referred to older versions (e.g. without + or
> > without S+) Do you still prefer to test the USB VID:PID?
> 
> What bothers me a bit with a quirk is that it's supposed to be a generic
> mechanism for bugs that affect a wide variety of devices. We could have
> a small array of device match entries as in uvc_ctrl_prune_entity() if
> you don't expect more than a handful of devices to be affected.
> Otherwise, if you think a quirk is better, let's go for that, but let's
> then name it with the vendor name (UVC_QUIRK_ELGATE_VIDEO_CONTROL or
> similar).

I expect that at most a handful devices might need that special
handling. Only a few companies produce video grabbers that support 4K.
All the cheap ones only support FullHD. So I don't expect that the chip
and firmware in the Cam Link is used in many other devices.

I had a look at the device array in uvc_ctrl_prune_entity(). Since that
mechanism is used there and in other places, I am fine with using it.
You have probably more experience what do implement as quirk and what to
cover with if conditions. So let's follow your suggestion and in case we
find more affected devices, we can still change it to a quirk.

> > > > +		__u8 corrected_format_index;
> > > > +
> > > > +		corrected_format_index = ctrl->bmHint >> 8;
> > > > +		uvc_dbg(stream->dev, CONTROL,
> > > > +			"Correct USB video probe response from {bmHint: 0x%04x, bFormatIndex: 0x%02x} to {bmHint: 0x%04x, bFormatIndex: 0x%02x}.\n",
> > > > +			ctrl->bmHint, ctrl->bFormatIndex,
> > > > +			ctrl->bFormatIndex, corrected_format_index);
> > > > +		ctrl->bmHint = ctrl->bFormatIndex;
> > > 
> > > According to your description above, this will always be 1. Is the third
> > > byte always 1 because the driver always sets bmHint to 1, or would it
> > > have a different value if we set bmHint to something else ? In the first
> > > case I'd hardcode ctrl->bmHint to 1 here.
> > 
> > I will test setting bmHint to something else than 1 to check. I will
> > report back then. Either follow your sugstion or update the comment.
> 
> Thanks.
> 
> > > > +		ctrl->bFormatIndex = corrected_format_index;
> > > > +	}
> > > > +
> > > >  	for (i = 0; i < stream->nformats; ++i) {
> > > >  		if (stream->format[i].index == ctrl->bFormatIndex) {
> > > >  			format = &stream->format[i];
> > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > index cce5e38133cd..cbb4ef61a64d 100644
> > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > @@ -209,6 +209,7 @@
> > > >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
> > > >  #define UVC_QUIRK_FORCE_Y8		0x00000800
> > > >  #define UVC_QUIRK_FORCE_BPP		0x00001000
> > > > +#define UVC_QUIRK_FIX_FORMAT_INDEX	0x00002000
> > > >  
> > > > 
> > > >  /* Format flags */
> > > >  #define UVC_FMT_FLAG_COMPRESSED		0x00000001
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-06-05 23:10 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20 18:52 PROBLEM: Broken pixel format for Elgato Cam Link 4K Benjamin Drung
2020-11-20 21:45 ` Adam Goode
2020-11-22 19:34   ` Benjamin Drung
2020-11-23 15:02     ` Adam Goode
2021-04-06 18:52       ` [PATCH v2] media: uvcvideo: Fix pixel format change " Benjamin Drung
2021-06-04 17:19       ` Benjamin Drung
2021-06-04 22:21         ` Laurent Pinchart
2021-06-05  8:19           ` Benjamin Drung
2021-06-05 21:51             ` Laurent Pinchart
2021-06-05 23:10               ` Benjamin Drung
2021-06-05 20:05           ` [PATCH v3] " Benjamin Drung
2021-06-05 20:13           ` Benjamin Drung
2021-06-05 20:15           ` [PATCH v4] " Benjamin Drung
2021-06-05 21:56             ` Laurent Pinchart
2021-06-05 22:58               ` Benjamin Drung

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.