| 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: <YNDY4iesZGF+7Cr0@pendragon.ideasonboard.com>
Date: Mon, 21 Jun 2021 21:22:26 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc: Linux Media Mailing List <linux-media@...r.kernel.org>,
linuxarm@...wei.com, mauro.chehab@...wei.com,
Mauro Carvalho Chehab <mchehab@...nel.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: uvc: limit max bandwidth for HDMI capture
Hi Mauro,
Thank you for the patch.
On Mon, Feb 01, 2021 at 08:08:59PM +0100, Mauro Carvalho Chehab wrote:
> 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. For such resolution, the maximum interval
> is, instead 333333 (30 fps).
What happens if you try to select it ?
> 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@...nel.org>
> ---
> drivers/media/usb/uvc/uvc_driver.c | 15 +++++++++++++++
> drivers/media/usb/uvc/uvc_video.c | 26 +++++++++++++++++++++++---
> drivers/media/usb/uvc/uvcvideo.h | 2 ++
> 3 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 1abc122a0977..c83a329f6527 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2339,6 +2339,7 @@ static int uvc_probe(struct usb_interface *intf,
> dev->info = info ? info : &uvc_quirk_none;
> dev->quirks = uvc_quirks_param == -1
> ? dev->info->quirks : uvc_quirks_param;
> + dev->max_bandwidth = dev->info->max_bandwidth;
>
> if (id->idVendor && id->idProduct)
> uvc_dbg(dev, PROBE, "Probing known UVC device %s (%04x:%04x)\n",
> @@ -2615,6 +2616,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,
> };
> @@ -2830,6 +2836,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 f2f565281e63..4afc1fbe0801 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -162,9 +162,29 @@ 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 */
> +
> + if (format->flags & UVC_FMT_FLAG_COMPRESSED &&
> + stream->dev->max_bandwidth && frame->bFrameIntervalType) {
> + u32 bandwidth;
> +
> + for (i = 0; i < frame->bFrameIntervalType - 1; ++i) {
Why - 1 ?
> + bandwidth = frame->wWidth * frame->wHeight;
> + bandwidth *= 10000000 / frame->dwFrameInterval[i];
> +
> + if (bandwidth <= stream->dev->max_bandwidth)
> + break;
> + }
> +
> + ctrl->dwFrameInterval = frame->dwFrameInterval[i];
This doesn't seem correct, you're selecting the first frame internal
below the bandwidth limit, even if the user explicitly requests a lower
frame rate.
> + return;
> + }
> +
> + if (stream->intf->num_altsetting > 1) {
There's an incorrect change in logic here. Before the patch this code
would run only for !UVC_FMT_FLAG_COMPRESSED, while with the patch, it will
run if UVC_FMT_FLAG_COMPRESSED && !(stream->dev->max_bandwidth &&
frame->bFrameIntervalType).
> u32 interval;
> u32 bandwidth;
>
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 97df5ecd66c9..b44e0cd4c826 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -658,6 +658,7 @@ static inline u32 uvc_urb_index(const struct uvc_urb *uvc_urb)
>
> struct uvc_device_info {
> u32 quirks;
> + u32 max_bandwidth;
> u32 meta_format;
> u16 uvc_version;
> };
> @@ -667,6 +668,7 @@ struct uvc_device {
> struct usb_interface *intf;
> unsigned long warnings;
> u32 quirks;
> + u32 max_bandwidth;
uvc_device has a uvc_device_info pointer, there's no need to copy the
field here.
> int intfnum;
> char name[32];
>
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists