| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YLvy9iE2Z+DpMvmg@pendragon.ideasonboard.com>
Date: Sun, 6 Jun 2021 00:56:06 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Benjamin Drung <bdrung@...teo.de>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
Adam Goode <agoode@...gle.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
stable@...r.kernel.org
Subject: Re: [PATCH v4] media: uvcvideo: Fix pixel format change for Elgato
Cam Link 4K
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@...r.kernel.org
> Signed-off-by: Benjamin Drung <bdrung@...teo.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@...asonboard.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
Powered by blists - more mailing lists