Hi Étienne,

On Sunday 09 December 2007, Étienne Bersac wrote:
> Hi,
>
> Follows an updated patch based on Ivan work. It's half weighted, mainly
> because it does not extract and load the AppleUSBVideoSupport OS X
> driver. This is done in userspace using isight-firmware-tools availables
> at http://bersace03.free.fr/ift/ and udev.  No SHA1 computation, No
> twice probe, No double device entry, No kernel space firmware loading,
> No is_isight(), etc. isight.h has been merged into uvcvideo.h and
> isight.c has been renamed to uvc_isight.c.

First of all, thanks for your work. Comments inlined.

> Many thanks to Laurent for helping cleaning the patch and properly
> implement the quirk workaround.
>
> Kind regards,
> Étienne.
>
> Signed-off-by: Étienne Bersac <[EMAIL PROTECTED]>
> ---
> Add built-in isight support to uvcvieo driver. Add a decode function
> member to struct uvc_video_queue. The decode function has three
> implementation : isoc, bulk and isight. uvc_isight.c implement the
> isight decode function.
>
> A BUILTIN_ISIGHT quirk is added allowing to determine wether to use
> isight decode function or not.
>
> Index: uvc_video.c
> ===================================================================
> --- uvc_video.c       (révision 149)
> +++ uvc_video.c       (copie de travail)
> @@ -452,8 +452,9 @@
>  /*
>   * Completion handler for video URBs.
>   */
> -static void uvc_video_complete_isoc(struct urb *urb,
> -     struct uvc_video_queue *queue, struct uvc_buffer *buf)
> +static void uvc_video_decode_isoc(struct urb *urb,
> +                               struct uvc_video_queue *queue,
> +                               struct uvc_buffer *buf)
>  {
>       u8 *mem;
>       int ret, i;

Please don't cleanup formatting unrelated to iSight support.

> @@ -490,8 +491,9 @@
>       }
>  }
>
> -static void uvc_video_complete_bulk(struct urb *urb,
> -     struct uvc_video_queue *queue, struct uvc_buffer *buf)
> +static void uvc_video_decode_bulk(struct urb *urb,
> +                               struct uvc_video_queue *queue,
> +                               struct uvc_buffer *buf)
>  {
>       u8 *mem;
>       int len, ret;

Ditto.

> @@ -585,10 +587,7 @@
>               buf = list_entry(queue->irqqueue.next, struct uvc_buffer, 
> queue);
>       spin_unlock_irqrestore(&queue->irqlock, flags);
>
> -     if (urb->number_of_packets)
> -             uvc_video_complete_isoc(urb, queue, buf);
> -     else
> -             uvc_video_complete_bulk(urb, queue, buf);
> +     queue->decode(urb, queue, buf);
>
>       if ((ret = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
>               uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n",
> @@ -628,7 +627,8 @@
>   * Initialize isochronous URBs and allocate transfer buffers. The packet
> size * is given by the endpoint.
>   */
> -static int uvc_init_video_isoc(struct uvc_video_device *video, struct
> usb_host_endpoint *ep) +static int uvc_init_video_isoc(struct
> uvc_video_device *video,
> +                            struct usb_host_endpoint *ep)
>  {
>       struct urb *urb;
>       unsigned int npackets, i, j;

Ditto.

> @@ -693,7 +693,8 @@
>   * Initialize bulk URBs and allocate transfer buffers. The packet size is
>   * given by the endpoint.
>   */
> -static int uvc_init_video_bulk(struct uvc_video_device *video, struct
> usb_host_endpoint *ep) +static int uvc_init_video_bulk(struct
> uvc_video_device *video,
> +                            struct usb_host_endpoint *ep)
>  {
>       struct urb *urb;
>       unsigned int pipe, i;

Ditto.

> @@ -747,7 +748,7 @@
>  {
>       struct usb_interface *intf = video->streaming->intf;
>       struct usb_host_interface *alts;
> -     struct usb_host_endpoint *ep;
> +     struct usb_host_endpoint *ep = NULL;
>       int intfnum = video->streaming->intfnum;
>       unsigned int bandwidth, psize, i;
>       int ret;

This fixes a gcc4 warning. I'll commit it as a separate patch.

> @@ -937,6 +938,16 @@
>       video->streaming->cur_frame = frame;
>       atomic_set(&video->active, 1);
>
> +
> +     /* determine the decode function */
> +     if (video->dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT)
> +             video->queue.decode = uvc_video_decode_isight;
> +     else if (video->streaming->intf->num_altsetting > 1)
> +             video->queue.decode = uvc_video_decode_isoc;
> +     else
> +             video->queue.decode = uvc_video_decode_bulk;
> +
> +

Please remove extra blank lines.

>       return 0;
>  }
>
> Index: uvc_isight.c
> ===================================================================
> --- uvc_isight.c      (révision 0)
> +++ uvc_isight.c      (révision 0)
> @@ -0,0 +1,154 @@
> +/*
> + * Copyright (C) 2006-2007 Ivan N. Zlatev <[EMAIL PROTECTED]>
> + * Copyright © 2007 Étienne Bersac <[EMAIL PROTECTED]>

Are non ascii characters allowed in Linux sources ?

> + *
> + * Based on extract.c by Ronald S. Bultje <[EMAIL PROTECTED]>
> + * Firmware loading specifics by Johannes Berg <[EMAIL PROTECTED]>
> + * at http://johannes.sipsolutions.net/MacBook/iSight

There is no firmware loading anymore.

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> + * MA 02110-1301, USA
> + */
> +
> +#include <linux/usb.h>
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +
> +#include "uvcvideo.h"
> +
> +#define isight_printk(msg...)                                \
> +     printk(KERN_DEBUG "uvcvideo: iSight: " msg)
> +

Unused, please remove.

> +/* Built-in iSight webcams are completely broken. They implement most
> + * of UVC 1.0, but the Apple engineers decided to use a completely
> + * different packet format, although the video data is in YUV. Were
> + * they on crack or just lazy ? As the hardware is 8051-based, it
> + * might be interesting to write an open-source firmware.

I think I'm the author of that message. Now that I know more about the 
hardware, I understand the 8051-based controller they used doesn't have the 
required hardware to generate UVC-compatible isochronous packets. The comment 
should be made a bit less aggressive :-)

> + *
> + * Instead of sending a header at the beginning of each isochronous
> + * transfer payload, the webcam sends a single header per image (on
> + * its own in a packet), followed by packets containing data only.
> + *
> + * Offset   Size (bytes)     Description
> + * ------------------------------------------------------------------
> + * 0x00      1       Header length
> + * 0x01      1       Flags (UVC-compliant)
> + * 0x02      4       Always equal to '11223344'
> + * 0x06      8       Always equal to 'deadbeefdeadface'
> + * 0x0e      16      Unknown
> + *
> + * The header can be prefixed by an optional, unknown-purpose byte.
> + */
> +
> +static int isight_decode (struct uvc_video_queue *queue,
> +                       struct uvc_buffer *buf,
> +                       const __u8 *data, unsigned int len)

Please use two tabs to indent function arguments, don't try to align them on 
the opening parenthesis.

> +{
> +     static const __u8 hdr[] = {
> +             0x11, 0x22, 0x33, 0x44, 0xde, 0xad, 0xbe, 0xef,
> +             0xde, 0xad, 0xfa, 0xce
> +     };
> +
> +     unsigned int maxlen, nbytes;
> +     __u8 *mem;
> +     int is_header = 0;
> +
> +     if (buf == NULL)
> +             return 0;
> +
> +     if ((len >= 14 && memcmp (&data[2], hdr, 12) == 0) ||
> +         (len >= 15 && memcmp (&data[3], hdr, 12) == 0)) {
> +             uvc_trace(UVC_TRACE_FRAME, "iSight header found\n");
> +             is_header = 1;
> +     }
> +
> +     /* Synchronize to the input stream by waiting for a header packet. */
> +     if (buf->state != UVC_BUF_STATE_ACTIVE) {
> +             if (!is_header) {
> +                     uvc_trace(UVC_TRACE_FRAME, "Dropping packet (out of "
> +                               "sync).\n");
> +                     return 0;
> +             }
> +
> +             buf->state = UVC_BUF_STATE_ACTIVE;
> +     }
> +
> +     /* Mark the buffer as done if we're at the beginning of a new frame.
> +      *
> +      * Empty buffers (bytesused == 0) don't trigger end of frame detection
> +      * as it doesn't make sense to return an empty buffer.
> +      */
> +     if (is_header && buf->buf.bytesused != 0) {
> +             buf->state = UVC_BUF_STATE_DONE;
> +             return -EAGAIN;
> +     }
> +
> +     /* Copy the video data to the buffer. Skip header packets, as they
> +      * contain no data.
> +      */
> +     if (!is_header) {
> +             maxlen = buf->buf.length - buf->buf.bytesused;
> +             mem = queue->mem + buf->buf.m.offset + buf->buf.bytesused;
> +             nbytes = min(len, maxlen);
> +             memcpy(mem, data, nbytes);
> +             buf->buf.bytesused += nbytes;
> +
> +             /* Drop the current frame if the buffer size was exceeded. */

The frame isn't dropped, the comment is incorrect.

> +             if (len > maxlen || buf->buf.bytesused == buf->buf.length) {
> +                     uvc_trace(UVC_TRACE_FRAME, "Frame complete 
> (overflow).\n");
> +                     buf->state = UVC_BUF_STATE_DONE;
> +             }
> +     }
> +
> +     return 0;
> +}
> +
> +void uvc_video_decode_isight (struct urb *urb,
> +                           struct uvc_video_queue *queue,
> +                           struct uvc_buffer *buf)

Two tabs please.

> +{
> +     int ret, i;
> +
> +     for (i = 0; i < urb->number_of_packets; ++i) {
> +             if (urb->iso_frame_desc[i].status < 0) {
> +                     uvc_trace(UVC_TRACE_FRAME, "USB isochronous frame "
> +                               "lost (%d).\n", 
> urb->iso_frame_desc[i].status);
> +                     /* Breaks 640x480
> +                        continue;
> +                     */

Please remove commented code.

> +             }
> +
> +             /* Decode the payload packet.
> +              * uvc_video_decode is entered twice when a frame transition
> +              * has been detected because the end of frame can only be
> +              * reliably detected when the first packet of the new frame
> +              * is processed. The first pass detects the transition and
> +              * closes the previous frame's buffer, the second pass
> +              * processes the data of the first payload of the new frame.
> +              */
> +             do {
> +                     ret = isight_decode(queue, buf,
> +                                         urb->transfer_buffer + 
> urb->iso_frame_desc[i].offset,
> +                                         
> urb->iso_frame_desc[i].actual_length);
> +
> +                     if (buf == NULL)
> +                             break;
> +
> +                     if (buf->state == UVC_BUF_STATE_DONE ||
> +                         buf->state == UVC_BUF_STATE_ERROR)
> +                             buf = uvc_queue_next_buffer(queue, buf);
> +             } while (ret == -EAGAIN);
> +     }
> +}
> Index: uvc_driver.c
> ===================================================================
> --- uvc_driver.c      (révision 149)
> +++ uvc_driver.c      (copie de travail)
> @@ -1667,6 +1667,13 @@
>         .bInterfaceProtocol   = 0,
>         .driver_info          = UVC_QUIRK_PROBE_MINMAX
>       },
> +     /* Apple Built-In iSight */
> +     { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE,
> +       .idVendor             = 0x05ac,
> +       .idProduct            = 0x8501,
> +       .driver_info          = UVC_QUIRK_PROBE_MINMAX
> +                             | UVC_QUIRK_BUILTIN_ISIGHT
> +     },
>       /* Microsoft Lifecam NX-6000 */
>       { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
>
>                               | USB_DEVICE_ID_MATCH_INT_INFO,
>
> Index: uvcvideo.h
> ===================================================================
> --- uvcvideo.h        (révision 149)
> +++ uvcvideo.h        (copie de travail)
> @@ -311,6 +311,7 @@
>  #define UVC_QUIRK_STATUS_INTERVAL    0x00000001
>  #define UVC_QUIRK_PROBE_MINMAX               0x00000002
>  #define UVC_QUIRK_PROBE_EXTRAFIELDS  0x00000004
> +#define UVC_QUIRK_BUILTIN_ISIGHT     0x00000008
>
>  /* Format flags */
>  #define UVC_FMT_FLAG_COMPRESSED              0x00000001
> @@ -576,6 +577,9 @@
>
>       struct list_head mainqueue;
>       struct list_head irqqueue;
> +
> +     void (*decode) (struct urb *urb, struct uvc_video_queue *queue,
> +                     struct uvc_buffer *buf);
>  };
>
>  struct uvc_video_device {
> @@ -764,6 +768,11 @@
>  extern struct usb_host_endpoint *uvc_find_endpoint(
>               struct usb_host_interface *alts, __u8 epaddr);
>
> +/* quirks functions */

Could you rename that to 'Quirks support' ?

> +void uvc_video_decode_isight (struct urb *urb, struct uvc_video_queue
> *queue, +                           struct uvc_buffer *buf);

Two tabs please.

> +
> +
>  #endif /* __KERNEL__ */
>
>  #endif
> Index: Makefile
> ===================================================================
> --- Makefile  (révision 149)
> +++ Makefile  (copie de travail)
> @@ -5,7 +5,7 @@
>  PWD          := $(shell pwd)
>
>  obj-m                := uvcvideo.o
> -uvcvideo-objs   := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o
> uvc_ctrl.o +uvcvideo-objs   := uvc_driver.o uvc_queue.o uvc_v4l2.o
> uvc_video.o uvc_ctrl.o uvc_isight.o
>
>  all: uvcvideo

Other than for those cosmetic fixes, the patch looks fine to me. Please 
resubmit it, and I'll then wait a few days to see if other people have 
comments before committing it.

Thanks a lot for your work.

Best regards,

Laurent Pinchart
_______________________________________________
Linux-uvc-devel mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/linux-uvc-devel

Reply via email to