| Message ID | 20210604171941.66136-1-bdrung@posteo.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] media: uvcvideo: Fix pixel format change for Elgato Cam Link 4K | expand |
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
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 >
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) {
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 >
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
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(+)