Hi Alexey,
On Sunday 06 March 2011 11:57:15 Alexey Fisher wrote:
A description of how you're making it more robust would be nice.
> Signed-off-by: Alexey Fisher <bug-tr...@fisher-privat.net>
> ---
> drivers/media/video/uvc/uvc_video.c | 59
> ++++++++++++++++++++++++++++++----- 1 files changed, 51 insertions(+), 8
> deletions(-)
>
> diff --git a/drivers/media/video/uvc/uvc_video.c
> b/drivers/media/video/uvc/uvc_video.c index 64bd1d6..5380189 100644
> --- a/drivers/media/video/uvc/uvc_video.c
> +++ b/drivers/media/video/uvc/uvc_video.c
> @@ -92,10 +92,9 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming
> *stream, {
> struct uvc_format *format;
> struct uvc_frame *frame = NULL;
> + struct usb_host_endpoint *ep;
> unsigned int i;
>
> - pr_debug("%s", __func__);
> -
> if (ctrl->bFormatIndex <= 0 ||
> ctrl->bFormatIndex > stream->nformats)
> return;
> @@ -112,6 +111,9 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming
> *stream, if (frame == NULL)
> return;
>
> + pr_debug("%s: %s %ux%u", __func__,
> + format->name, frame->wWidth, frame->wHeight);
> +
> if (!(format->flags & UVC_FMT_FLAG_COMPRESSED) ||
> (ctrl->dwMaxVideoFrameSize == 0 &&
> stream->dev->uvc_version < 0x0110) ||
> @@ -123,11 +125,13 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming
> *stream, frame->dwMaxVideoFrameBufferSize;
> }
>
> - if (!(format->flags & UVC_FMT_FLAG_COMPRESSED) &&
> + if ((!(format->flags & UVC_FMT_FLAG_COMPRESSED) ||
> + format->fcc == V4L2_PIX_FMT_MJPEG ) &&
> stream->dev->quirks & UVC_QUIRK_FIX_BANDWIDTH &&
> stream->intf->num_altsetting > 1) {
> u32 interval;
> u32 bandwidth;
> + unsigned int best_psize = 3 * 1024;
>
> interval = (ctrl->dwFrameInterval > 100000)
> ? ctrl->dwFrameInterval
> @@ -139,13 +143,17 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming
> *stream, * high-speed devices) per second and add the UVC header size
> * (assumed to be 12 bytes long).
> */
> - bandwidth = frame->wWidth * frame->wHeight / 8 * format->bpp;
> + bandwidth = frame->dwMaxVideoFrameBufferSize;
> + //bandwidth = frame->wWidth * frame->wHeight / 8 * format->bpp;
Why ? The whole point of your patch set is to work around untrusted
dwMaxVideoFrameBufferSize values.
> bandwidth *= 10000000 / interval + 1;
> bandwidth /= 1000;
> if (stream->dev->udev->speed == USB_SPEED_HIGH)
> bandwidth /= 8;
> bandwidth += 12;
>
> + /* add 20% more, in some cases it is still not enough */
> + bandwidth *= 1.2;
> +
That's not the kind of things I can accept with very very good arguments. I
don't want to break some cameras just to lower the size of the V4L2 buffers
allocated by the driver.
> /* The bandwidth estimate is too low for many cameras. Don't use
> * maximum packet sizes lower than 1024 bytes to try and work
> * around the problem. According to measurements done on two
> @@ -153,12 +161,39 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming
> *stream, * resolutions working while not preventing two simultaneous
> * VGA streams at 15 fps.
> */
> + pr_debug("%s: calculated bandwidth: %u for %u(fps); (before 1024
> workaround).", + __func__, bandwidth,
> 10000000/interval);
> bandwidth = max_t(u32, bandwidth, 1024);
>
> + i = 0;
> + for (i = 0; i < stream->intf->num_altsetting; ++i) {
> + struct usb_host_interface *alts;
> + struct usb_host_endpoint *ep;
> + unsigned int psize;
> +
> + alts = &stream->intf->altsetting[i];
> + ep = uvc_find_endpoint(alts,
> + stream->header.bEndpointAddress);
> + if (ep == NULL)
> + continue;
> +
> + psize = le16_to_cpu(ep->desc.wMaxPacketSize);
> + psize = (psize & 0x07ff) * (1 + ((psize >> 11) & 3));
> + if ((psize >= bandwidth && psize <= best_psize) ||
> + (i == stream->intf->num_altsetting - 1)) {
> + if (bandwidth > psize)
> + pr_debug("%s: trying psize=%u even if "
> + "bandwidth=%u", __func__,
> + psize, bandwidth);
> + best_psize = psize;
> + break;
> + }
> + }
> +
> pr_debug("%s: rewrite dwMaxPayloadTransferSize=%u to %u.",
> - __func__, ctrl->dwMaxPayloadTransferSize, bandwidth);
> + __func__, ctrl->dwMaxPayloadTransferSize, best_psize);
>
> - ctrl->dwMaxPayloadTransferSize = bandwidth;
> + ctrl->dwMaxPayloadTransferSize = best_psize;
Is this needed ? uvc_init_video() already selects the best endpoint alternate
setting based on the requested bandwidth. This looks like duplicate code,
what's the point ?
> }
> }
>
> @@ -1099,11 +1134,19 @@ static int uvc_init_video(struct uvc_streaming
> *stream, gfp_t gfp_flags) /* Check if the bandwidth is high enough. */
> psize = le16_to_cpu(ep->desc.wMaxPacketSize);
> psize = (psize & 0x07ff) * (1 + ((psize >> 11) & 3));
> - if (psize >= bandwidth && psize <= best_psize) {
> + pr_debug("%s: bandwidth: %u; psize: %u; best_psize: %u",
> + __func__, bandwidth, psize, best_psize);
> + if ((psize >= bandwidth && psize <= best_psize) ||
> + (i == intf->num_altsetting - 1)) {
> + if (bandwidth > psize)
> + pr_debug("%s: trying psize=%u even if "
> + "bandwidth=%u", __func__,
> + psize, bandwidth);
> altsetting = i;
> best_psize = psize;
> best_ep = ep;
> - }
> + break;
> + }
> }
>
> if (best_ep == NULL) {
--
Regards,
Laurent Pinchart
_______________________________________________
Linux-uvc-devel mailing list
Linux-uvc-devel@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/linux-uvc-devel
