Am Sonntag, den 06.03.2011, 12:47 +0100 schrieb Laurent Pinchart:
> 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 ?
Becouse we do uvc_set_video_ctrl with one bandwidth and uvc_video_init
with other. I do not know if it is good in case we recalculate the
value.
>
> > }
> > }
> >
> > @@ -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);
And here we can just fail, or try the luck (and may be fail). So this
part independet of all other changes.
> > altsetting = i;
> > best_psize = psize;
> > best_ep = ep;
> > - }
> > + break;
> > + }
> > }
here we make too match loops, so it is nice to have here a break
> > if (best_ep == NULL) {
>
_______________________________________________
Linux-uvc-devel mailing list
Linux-uvc-devel@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/linux-uvc-devel
