Hi, On 6-Jan-25 3:48 PM, Ricardo Ribalda wrote: > On Mon, 6 Jan 2025 at 15:09, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >> >> From: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> >> >> This device: >> 534d:2109 MacroSilicon >> >> Announces that it supports several frame intervals for >> their resolutions for MJPEG compression: >> >> VideoStreaming Interface Descriptor: >> bLength 46 >> bDescriptorType 36 >> bDescriptorSubtype 7 (FRAME_MJPEG) >> bFrameIndex 1 >> bmCapabilities 0x00 >> Still image unsupported >> wWidth 1920 >> wHeight 1080 >> dwMinBitRate 768000 >> dwMaxBitRate 196608000 >> dwMaxVideoFrameBufferSize 4147200 >> dwDefaultFrameInterval 166666 >> bFrameIntervalType 5 >> dwFrameInterval( 0) 166666 >> dwFrameInterval( 1) 333333 >> dwFrameInterval( 2) 400000 >> dwFrameInterval( 3) 500000 >> dwFrameInterval( 4) 1000000 >> >> However, the highest frame interval (166666), which means 60 fps >> is not supported. If set to it, URB packages will be dropped, >> causing JPEG decoding errors and part of the video frames will >> be missed. >> >> Also, as specified at the device's documentation, for such >> resolution, the maximum interval is 30 fps (interval == 333333). >> >> The last format that supports such frame interval is 1280x720. >> >> Add a quirk to estimate a raw bandwidth, by doing: >> width * height * framerate >> E. g.: >> 1920 * 1080 * 30 = 62208000 >> >> if the bandwidth is greater than such threshold, get >> the next value from the dwFrameInterval. >> >> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> >> Link: https://lore.kernel.org/r/bca0634691cea503c2c5df62b98ba66f0c85bf85.1624350539.git.mchehab+huawei@xxxxxxxxxx >> [hdegoede@xxxxxxxxxx add "pixels per second" comment, use UVC_FIVAL_DENOM] >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> >> --- >> drivers/media/usb/uvc/uvc_driver.c | 14 ++++++++++++++ >> drivers/media/usb/uvc/uvc_video.c | 29 ++++++++++++++++++++++++++--- >> drivers/media/usb/uvc/uvcvideo.h | 1 + >> 3 files changed, 41 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c >> index f754640fddf5..6d001d4f0902 100644 >> --- a/drivers/media/usb/uvc/uvc_driver.c >> +++ b/drivers/media/usb/uvc/uvc_driver.c >> @@ -2491,6 +2491,11 @@ static const struct uvc_device_info uvc_quirk_fix_bandwidth = { >> .quirks = UVC_QUIRK_FIX_BANDWIDTH, >> }; >> >> +static const struct uvc_device_info uvc_quirk_fix_bw_622 = { >> + .quirks = UVC_QUIRK_FIX_BANDWIDTH, >> + .max_bandwidth = 62208000, >> +}; >> + >> static const struct uvc_device_info uvc_quirk_probe_def = { >> .quirks = UVC_QUIRK_PROBE_DEF, >> }; >> @@ -2794,6 +2799,15 @@ static const struct usb_device_id uvc_ids[] = { >> .bInterfaceSubClass = 1, >> .bInterfaceProtocol = 0, >> .driver_info = (kernel_ulong_t)&uvc_quirk_fix_bandwidth }, >> + /* MacroSilicon HDMI capture */ >> + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE >> + | USB_DEVICE_ID_MATCH_INT_INFO, >> + .idVendor = 0x534d, >> + .idProduct = 0x2109, >> + .bInterfaceClass = USB_CLASS_VIDEO, >> + .bInterfaceSubClass = 1, >> + .bInterfaceProtocol = 0, >> + .driver_info = (kernel_ulong_t)&uvc_quirk_fix_bw_622 }, >> /* Genesys Logic USB 2.0 PC Camera */ >> { .match_flags = USB_DEVICE_ID_MATCH_DEVICE >> | USB_DEVICE_ID_MATCH_INT_INFO, >> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c >> index 41cb1b45fa23..af5233ab667e 100644 >> --- a/drivers/media/usb/uvc/uvc_video.c >> +++ b/drivers/media/usb/uvc/uvc_video.c >> @@ -225,9 +225,32 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream, >> if ((ctrl->dwMaxPayloadTransferSize & 0xffff0000) == 0xffff0000) >> ctrl->dwMaxPayloadTransferSize &= ~0xffff0000; >> >> - if (!(format->flags & UVC_FMT_FLAG_COMPRESSED) && >> - stream->dev->quirks & UVC_QUIRK_FIX_BANDWIDTH && >> - stream->intf->num_altsetting > 1) { >> + if (!(stream->dev->quirks & UVC_QUIRK_FIX_BANDWIDTH)) >> + return; >> + >> + /* Handle UVC_QUIRK_FIX_BANDWIDTH */ >> + > > Shouldn't this quirk be better implemented in uvc_try_frame_interval() ? This is the place where the other bandwidth checks / UVC_QUIRK_FIX_BANDWIDTH handling is done, so I guess it makes sense to keep this here. >> + if (format->flags & UVC_FMT_FLAG_COMPRESSED) { >> + u32 bandwidth; >> + >> + if (!stream->dev->info->max_bandwidth || !frame->bFrameIntervalType) >> + return; >> + >> + for (i = 0; i < frame->bFrameIntervalType; ++i) { >> + bandwidth = frame->wWidth * frame->wHeight; >> + bandwidth *= UVC_FIVAL_DENOM / frame->dwFrameInterval[i]; > > frame->dwFrameInterval[i] comes from the device, it is not sanitized. > We need to make sure it is not zero. That is a good point, in the non-compressed UVC_QUIRK_FIX_BANDWIDTH handling this is handled by doing: bandwidth *= UVC_FIVAL_DENOM / (frame->dwFrameInterval[i] + 1); I guess we should do the same here then ? Regards, Hans > >> + >> + if (bandwidth <= stream->dev->info->max_bandwidth) >> + break; >> + } >> + >> + if (ctrl->dwFrameInterval < frame->dwFrameInterval[i]) >> + ctrl->dwFrameInterval = frame->dwFrameInterval[i]; >> + >> + return; >> + } >> + >> + if (stream->intf->num_altsetting > 1) { >> u32 interval; >> u32 bandwidth; >> >> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h >> index 966ff82c2ba8..6b702219173f 100644 >> --- a/drivers/media/usb/uvc/uvcvideo.h >> +++ b/drivers/media/usb/uvc/uvcvideo.h >> @@ -535,6 +535,7 @@ static inline u32 uvc_urb_index(const struct uvc_urb *uvc_urb) >> >> struct uvc_device_info { >> u32 quirks; >> + u32 max_bandwidth; /* In pixels per second */ >> u32 meta_format; >> u16 uvc_version; >> }; >> -- >> 2.47.1 >> >> > >
