[08:36] <hverkuil> mchehab: did you post a PR for Linus for the rkisp1 fixes? The linux-media ML appears to be still eating posts, so it is hard to tell.
[08:45] <hverkuil> (the ML seems to be back in business, at least my latest patches now go through again)
[10:06] <hverkuil> mchehab: in the past you posted useful DVB information in your Samsung blog: http://blogs.s-osg.org/media-controller-support-for-dvb-part-1/
[10:06] <hverkuil> But these have been removed, it seems. Is it possible to resurrect those posts on linuxtv.org?
[10:11] <hverkuil> mchehab: the links to the git repos on this page: https://linuxtv.org/repo/ are wrong and need to be updated.
[12:42] <mchehab> hverkuil: no, didn't send rkisp1 fixes yet. I was waiting for a linux-next message about the merge
[12:43] <mchehab> there was a next tree released on friday, but I didn't receive the e-mail about that
[12:43] <mchehab> I'm now checking the e-mail from the today's next. if OK, I'll be sending the PR today
[12:44] <mchehab> hverkuil: the entire s-osg.org server was dropped
[12:45] <mchehab> the area at the org which used to host it was completely changed. I guess I can't legally post its content somewhere else :-(
[12:45] <mchehab> I guess I don't even have anymore its original content
[15:56] <mchehab> hverkuil: just sent a PR with rkisp1 fixes.
[15:57] <hverkuil> mchehab: thank you!
[15:57] <mchehab> anytime
[15:57] <mchehab> pinchartl: I bought a new UVC device those days. It is an HDMI cheap capture board:
[15:58] <mchehab> Bus 001 Device 035: ID 534d:2109 MacroSilicon
[15:58] <mchehab> while this board works fine, it currently has an issue with UVC:
[15:59] <mchehab> it only supports the maximum fps (60Hz) up to a 720 lines. At the maximum resolution (1920x1080), the fps should be set to 30 fps, as otherwise it starts receiving incomplete URBs
[16:01] <mchehab> it would be nice to add some sort of quirk on it in order for it to reduce the fps at higher resolutions in order to fit the hardware limits
[16:04] <mchehab> should some quirk be added to the UVC driver for it to address that? maybe &uvc_quirk_fix_bandwidth?
[16:15] <mchehab> right now, when the device is set to YUYV, the frame rate is properly reduced as the frame size increases
[16:16] <mchehab> but, with MJPG, the driver says that it always support 60fps
[16:45] <pinchartl> mchehab: does the device claim it support 1920x1080 @60fps in YUV in its UVC descriptors ?
[16:47] <pinchartl> I'm not sure to understand the problem. you say that 1920x1080 @>30fps results in incomplete URBs, but then you said that right now the frame rate is properly reduced as the frame size increases
[16:49] <mchehab> pinchartl: yes. basically, the device supports 60fps at 1280x720
[16:50] <mchehab> when the resolution is higher than that, it supports a max of 30fps
[16:51] <mchehab> the device supports 2 types of formats: MJPEG and YUYV.
[16:51] <mchehab> the above limit is for MJPEG
[16:52] <mchehab> when YUYV is set, the UVC driver already imposes a limit
[16:52] <mchehab> I didn't check the UVC descriptors
[16:53] <mchehab> (how can I dump the UVC descriptors?)
[16:55] <mchehab> VideoStreaming Interface Descriptor:
[16:55] <mchehab> bLength 46
[16:55] <mchehab> bDescriptorType 36
[16:55] <mchehab> bDescriptorSubtype 7 (FRAME_MJPEG)
[16:55] <mchehab> bFrameIndex 1
[16:55] <mchehab> bmCapabilities 0x00
[16:55] <mchehab> Still image unsupported
[16:55] <mchehab> wWidth 1920
[16:55] <mchehab> wHeight 1080
[16:55] <mchehab> dwMinBitRate 768000
[16:55] <mchehab> dwMaxBitRate 196608000
[16:55] <mchehab> dwMaxVideoFrameBufferSize 4147200
[16:55] <mchehab> dwDefaultFrameInterval 166666
[16:55] <mchehab> bFrameIntervalType 5
[16:55] <mchehab> dwFrameInterval( 0) 166666
[16:55] <mchehab> dwFrameInterval( 1) 333333
[16:55] <mchehab> dwFrameInterval( 2) 400000
[16:55] <mchehab> dwFrameInterval( 3) 500000
[16:55] <mchehab> dwFrameInterval( 4) 1000000
[16:56] <mchehab> btw, I'm getting a warning when using lsusb:
[16:56] <mchehab> $ sudo lsusb -d 534d:2109 -vvvvvvvvvvv >/dev/null
[16:56] <mchehab> can't get debug descriptor: Resource temporarily unavailable
[16:57] <pinchartl> the bandwidth problem you've noticed, is it for YUV only, or also for MJPEG ?
[16:57] <mchehab> YUV works fine (it reduces to 5fps at max res)
[16:58] <mchehab> btw, when I bought this device, I knew already that it has such limit
[16:58] <mchehab> https://www.amazon.de/gp/product/B088NWWSKN/ref=ppx_yo_dt_b_asin_title_o05_s00?ie=UTF8&psc=1
[16:58] <mchehab> "The maximum output resolution 1920 × 1080 @ 30Hz. "
[16:59] <mchehab> there are several different HDMI capture vendors with the same limit... probably they all use the same chipset (or even the same hw design)
[17:01] <mchehab> like this similar model: https://www.amazon.de/dp/B08DFRS36T
[17:03] <mchehab> btw, I'm not sure if this is a USB bandwidth issue, or if it is due to some internal BW limit
[17:10] <pinchartl> I'm not sure the bandwidth fix quirk would be very effective here. it's meant for YUV, for MJPEG the driver has no way to really evaluate bandwidth requirements
[17:11] <mchehab> yeah, I did a quick test (and even checked the code)... it won't be doing any good
[17:12] <mchehab> as it is ignored if the format is compressed
[17:12] <pinchartl> there's been at least one patch proposed to extend that quirk to compressed formats, but I don't think it's possible
[17:13] <pinchartl> so maybe we need another quirk
[17:13] <mchehab> FYI, this is the highest resolution it supports 60fps:
[17:13] <mchehab> VideoStreaming Interface Descriptor:
[17:13] <mchehab> bLength 46
[17:13] <mchehab> bDescriptorType 36
[17:13] <mchehab> bDescriptorSubtype 7 (FRAME_MJPEG)
[17:13] <mchehab> bFrameIndex 6
[17:13] <mchehab> bmCapabilities 0x00
[17:13] <pinchartl> possibly extending the uvc_device_info structure with a per-format resolution limit, or something similar
[17:13] <mchehab> Still image unsupported
[17:13] <mchehab> wWidth 1280
[17:13] <mchehab> wHeight 720
[17:13] <mchehab> dwMinBitRate 768000
[17:13] <mchehab> dwMaxBitRate 196608000
[17:14] <mchehab> dwMaxVideoFrameBufferSize 1843200
[17:14] <mchehab> dwDefaultFrameInterval 166666
[17:14] <mchehab> bFrameIntervalType 5
[17:14] <mchehab> dwFrameInterval( 0) 166666
[17:14] <mchehab> dwFrameInterval( 1) 200000
[17:14] <mchehab> dwFrameInterval( 2) 333333
[17:14] <mchehab> dwFrameInterval( 3) 500000
[17:14] <mchehab> dwFrameInterval( 4) 1000000
[17:15] <pinchartl> that dwMaxVideoFrameBufferSize value wouldn't work at 60fps
[17:16] <mchehab> yep. the max one that works is 1843200
[17:17] <mchehab> I guess the driver could use that in order to limit the max fps
[17:34] <pinchartl> 1843200 * 60 * 8 > 80% * 480MB/s
[17:39] <mchehab> heh. this is probably over-estimated
[17:40] <mchehab> (1280*720*2*8)/1843200 = 8.0
[17:41] <mchehab> that's basically 1280*720*2 - e. g. 2 bits/pixel for a MJPEG compressed frame
[17:42] <mchehab> hmm... actually 16 bits/pixel
[17:42] <mchehab> that is for YUYV... it sounds the descriptor is mangled
[17:43] <mchehab> as MJPEG will compress the frame
[17:57] <pinchartl> their worst case estimate is the uncompressed size
[17:57] <pinchartl> I suppose that never happens
[17:57] <mchehab> a really dirty quick hack would be:
[17:57] <mchehab> $ git diff drivers/media/usb/uvc/uvc_v4l2.c
[17:57] <mchehab> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
[17:57] <mchehab> index 252136cc885c..1d011463f034 100644
[17:57] <mchehab> --- a/drivers/media/usb/uvc/uvc_v4l2.c
[17:57] <mchehab> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
[17:57] <mchehab> @@ -112,6 +112,13 @@ static u32 uvc_try_frame_interval(struct uvc_frame *frame, u32 interval)
[17:57] <mchehab> ? interval - frame->dwFrameInterval[i]
[17:57] <mchehab> : frame->dwFrameInterval[i] - interval;
[17:57] <mchehab>
[17:57] <mchehab> + // HACK!!!
[17:57] <mchehab> + int res = frame->wWidth * frame->wHeight;
[17:57] <mchehab> + if (res > 921600 && frame->dwFrameInterval[i] == 166666) {
[17:57] <mchehab> +pr_info("Quirk: fixing frame interval\n");
[17:57] <mchehab> + continue;
[17:57] <mchehab> + }
[17:57] <mchehab> +
[17:57] <mchehab> if (dist > best)
[17:57] <pinchartl> but the driver can't use this information to estimate bandwidth requirements, as it's obviously wrong :-/
[17:57] <mchehab> break;
[17:58] <mchehab> yes, the worst case is too worse to be useful
[17:58] <mchehab> the real problem seems to be:
[17:58] <mchehab> dwFrameInterval( 0) 166666
[17:58] <pinchartl> I try to avoid heuristics like this one, as they will fix the issue on your device, and also likely break other already supported devices. it becomes a game of whack-a-mole :-)
[17:58] <mchehab> this is not valid for resolutions > 1280x720
[17:59] <mchehab> yeah agreed
[17:59] <mchehab> finding something that would be more generic here seems a challenge
[18:00] <pinchartl> so I think extending uvc_device_info with a list of formats, sizes and frame rates limits (one way or another, we'll have to find a way to express that so it remains simple), and skipping formats/resolutions/frame rates above those limits, would be best
[18:00] <pinchartl> it would require a device-specific entry, but I don't think we can do much better
[18:01] <pinchartl> historically the driver had quirks only, but a "if > 1280x720 in MJPEG then disable 60fps" quirk flag is a bit too specific
[18:01] <pinchartl> expressing it as additional data in uvc_device_info would I believe be better
[18:01] <mchehab> yeah, it will need a device-specific entry, and probably a value or a table associated with the quirk
[18:02] <pinchartl> it could also be in the form of a single bandwidth limit
[18:02] <pinchartl> we don't know exactly how much a particular device can reduce the bandwidth by compressing in MJPEG
[18:03] <pinchartl> but we could express, in uvc_device_info, that compressed formats that have an uncompressed bandwidth higher than some value should be rejected
[18:03] <pinchartl> >>> 1280*720*60
[18:03] <pinchartl> 55296000
[18:03] <pinchartl> >>> 1920*1080*30
[18:03] <pinchartl> 62208000
[18:04] <mchehab> sounds doable
[18:04] <pinchartl> so we could set the limit to 62208000 in uvc_device_info and reject fmt == MJPEG && width * height * fps > 62208000
[18:04] <pinchartl> it's easier than expressing a complex list of formats, sizes and frame rates
[18:05] <pinchartl> as long as we record the exact limit in the commit message, I'd be fine with this, and it can always be reworked with a more complex mechanism in the future if other devices need something better
[18:05] <pinchartl> btw you could send me the output of 'sudo lsusb -d 534d:2109 -v' by e-mail ?
[18:05] <pinchartl> I try to keep a list of all the known devices
[18:05] <pinchartl> dinner time, I'll be back in a bit
[18:07] <mchehab> somethign like this, I guess:
[18:07] <mchehab> static const struct uvc_device_info uvc_quirk_fix_bw_622 = {
[18:07] <mchehab> .quirks = UVC_QUIRK_FIX_BANDWIDTH,
[18:07] <mchehab> .max_bandwidth = 62208000;
[18:07] <mchehab> };
[18:09] <pinchartl> we can even skip the quirk flag, if the max_bandwidth is set, we can take it into account unconditionally
[18:10] <mchehab> true - yet, IMHO, this would document it better. In terms of space, it wouldn't matter, as the static struct will allocate the same amount of space
[18:10] <mchehab> (I guess)
[18:11] <mchehab> gcc might do some trick with the const to avoid that
[19:25] <mchehab> patch submitted
[19:26] <pinchartl> thank you