Hi Hans On Mon, 6 Jan 2025 at 16:49, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > 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 I understood it correctly the quirk skips one of the intervals. If we add the quirk to uvc_ioctl_enum_frameintervals and uvc_try_frame_interval(), the user will not enumerate frame intervals that will not work. IMO that is better than accepting an interval and then changing it to something else.
