Hi Chris,
On Friday 13 July 2007, Christopher HORLER wrote:
> Hi,
>
> Here's a first attempt at supporting read - I'm sure there's bits to iron
> out, but it works for me.
I already applied whitespace cleanup and other cosmetic changes. I'll comment
on the rest.
> Index: uvc_queue.c
> ===================================================================
> --- uvc_queue.c (revision 111)
> +++ uvc_queue.c (working copy)
> @@ -63,6 +63,8 @@
> * At that point, any process waiting on the buffer will be woken up.
> If a * process tries to dequeue a buffer after it has been marked ready,
> the * dequeing will succeed immediately.
> +
> + * Note: During the read operation the user / client is the driver
> itself. *
> * 2. Buffers are queued, user is waiting on a buffer and the device gets
> * disconnected.
> @@ -91,11 +93,11 @@
> *
> * Pages are reserved to make sure they will not be swaped, as they will
> be * filled in URB completion handler.
> - *
> + *
> * Buffers will be individually mapped, so they must all be page aligned.
> */
> int uvc_alloc_buffers(struct uvc_video_queue *queue, unsigned int
> nbuffers, - unsigned int buflength)
> + unsigned int buflength, int memtype)
> {
> unsigned int bufsize = PAGE_ALIGN(buflength);
> unsigned int i;
> @@ -108,7 +110,7 @@
> mutex_lock(&queue->mutex);
>
> if ((ret = uvc_free_buffers(queue)) < 0)
> - goto done;
> + goto done;
>
> /* Decrement the number of buffers until allocation succeeds. */
> for (; nbuffers > 0; --nbuffers) {
> @@ -131,7 +133,8 @@
> queue->buffer[i].buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> queue->buffer[i].buf.sequence = 0;
> queue->buffer[i].buf.field = V4L2_FIELD_NONE;
> - queue->buffer[i].buf.memory = V4L2_MEMORY_MMAP;
> + if (memtype == V4L2_MEMORY_MMAP)
> + queue->buffer[i].buf.memory = memtype;
> queue->buffer[i].buf.flags = 0;
> init_waitqueue_head(&queue->buffer[i].wait);
> }
> @@ -154,6 +157,8 @@
> {
> unsigned int i;
>
> + uvc_trace(UVC_TRACE_CAPTURE, "Freeing buffers\n");
> +
> for (i = 0; i < queue->count; ++i) {
> if (queue->buffer[i].vma_use_count != 0)
> return -EBUSY;
> @@ -171,19 +176,20 @@
> #endif
> vfree(queue->mem);
> queue->count = 0;
> + queue->mem = NULL;
This is not required, as queue alloc/free operations use queue->count to track
the free/allocated state.
> }
>
> return 0;
> }
>
> void uvc_query_buffer(struct uvc_buffer *buf,
> - struct v4l2_buffer *v4l2_buf)
> + struct v4l2_buffer *v4l2_buf)
> {
> memcpy(v4l2_buf, &buf->buf, sizeof *v4l2_buf);
>
> if(buf->vma_use_count)
> v4l2_buf->flags |= V4L2_BUF_FLAG_MAPPED;
> -
> +
> switch (buf->state) {
> case UVC_BUF_STATE_ERROR:
> case UVC_BUF_STATE_DONE:
> @@ -199,6 +205,7 @@
> }
> }
>
> +
> /*
> * Queue a video buffer. Attempting to queue a buffer that has already
> been * queued will return -EINVAL.
> @@ -211,11 +218,9 @@
>
> uvc_trace(UVC_TRACE_CAPTURE, "Queuing buffer %u.\n", v4l2_buf->index);
>
> - if (v4l2_buf->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
> - v4l2_buf->memory != V4L2_MEMORY_MMAP) {
> - uvc_trace(UVC_TRACE_CAPTURE, "[E] Invalid buffer type (%u) "
> - "and/or memory (%u).\n", v4l2_buf->type,
> - v4l2_buf->memory);
> + if (v4l2_buf->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ) {
> + uvc_trace(UVC_TRACE_CAPTURE, "[E] Invalid buffer type (%u) ",
> + v4l2_buf->type);
> return -EINVAL;
> }
>
> @@ -254,9 +259,12 @@
> ? 0 : -EAGAIN;
> }
>
> - return wait_event_interruptible(buf->wait,
> - buf->state != UVC_BUF_STATE_QUEUED &&
> - buf->state != UVC_BUF_STATE_ACTIVE);
> + if (wait_event_interruptible(buf->wait,
> + buf->state != UVC_BUF_STATE_QUEUED &&
> + buf->state != UVC_BUF_STATE_ACTIVE))
> + return -ERESTARTSYS;
> +
> + return 0;
wait_event_interruptible returns -ERESTARTSYS directly, there's no need for an
additional check.
> }
>
> /*
> @@ -269,11 +277,9 @@
> struct uvc_buffer *buf;
> int ret = 0;
>
> - if (v4l2_buf->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
> - v4l2_buf->memory != V4L2_MEMORY_MMAP) {
> - uvc_trace(UVC_TRACE_CAPTURE, "[E] Invalid buffer type (%u) "
> - "and/or memory (%u).\n", v4l2_buf->type,
> - v4l2_buf->memory);
> + if (v4l2_buf->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ) {
> + uvc_trace(UVC_TRACE_CAPTURE, "[E] Invalid buffer type (%u) ",
> + v4l2_buf->type);
> return -EINVAL;
> }
>
> @@ -286,7 +292,7 @@
>
> buf = list_entry(queue->mainqueue.next, struct uvc_buffer, stream);
> uvc_trace(UVC_TRACE_CAPTURE, "Dequeuing buffer %u.\n", buf->buf.index);
> - if ((ret = uvc_queue_waiton(buf, nonblocking)) < 0)
> + if ((ret = uvc_queue_waiton(buf, nonblocking)))
This is not needed.
> goto done;
>
> switch (buf->state) {
> Index: uvc_v4l2.c
> ===================================================================
> --- uvc_v4l2.c (revision 111)
> +++ uvc_v4l2.c (working copy)
> @@ -241,6 +241,7 @@
>
> static int uvc_v4l2_set_format(struct uvc_video_device *video, struct
> v4l2_format *fmt) {
> +
> struct uvc_streaming_control probe;
> struct uvc_format *format;
> struct uvc_frame *frame;
> @@ -401,7 +402,10 @@
> struct video_device *vdev;
> struct uvc_video_device *video;
> struct uvc_fh *handle;
> + struct v4l2_requestbuffers req;
> + unsigned int bufsize;
> int ret = 0;
> + int i;
>
> uvc_trace(UVC_TRACE_CALLS, "uvc_v4l2_open\n");
> mutex_lock(&uvc_driver.open_mutex);
> @@ -426,8 +430,43 @@
>
> kref_get(&video->dev->kref);
>
> -done:
> mutex_unlock(&uvc_driver.open_mutex);
> +
> + if ((ret = uvc_acquire_privileges(handle)) < 0) {
> + return ret;
> + }
> +
> + bufsize = video->streaming->ctrl.dwMaxVideoFrameSize;
> + memset(&req, 0, sizeof req);
> + req.count = 4;
> + req.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +
> + if ((ret = uvc_alloc_buffers(&video->queue, req.count, bufsize,
> req.memory)) < 0) { + uvc_trace(UVC_TRACE_CALLS, "failed to allocate
> buffers\n");
> + return ret;
> + }
> +
> + /* check stream is on */
> + if (!video->stream) {
> + uvc_trace(UVC_TRACE_CALLS, "enabling stream\n");
> + if ((ret = uvc_video_enable(video, 1)) < 0)
> + return ret;
> + video->stream = UVC_STREAM_ON;
> + }
> +
> + uvc_trace(UVC_TRACE_CALLS, "queueing buffers\n");
> + for (i = 0; i < video->queue.count; ++i) {
> + struct v4l2_buffer v4l2_buf;
> + memset(&v4l2_buf, 0, sizeof v4l2_buf);
> + v4l2_buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + v4l2_buf.index = i;
> + uvc_queue_buffer(&video->queue, &v4l2_buf);
> + }
> +
> +
> + return 0;
> + done:
> + mutex_unlock(&uvc_driver.open_mutex);
Start streaming the first time read() is called, not when the device is
opened. The queue should be allocated at the first read() call as well. Don't
hardcode the buffer count to 4, use the value set by VIDIOC_S_PARM (with
sensible limits).
> return ret;
> }
>
> @@ -442,6 +481,7 @@
> /* Only free resources if this is a privileged handle. */
> if (uvc_has_privileges(handle)) {
> uvc_video_enable(video, 0);
> + video->stream = UVC_STREAM_OFF;
>
> mutex_lock(&video->queue.mutex);
> if (uvc_free_buffers(&video->queue) < 0)
video->stream was a leftover of some old idea I can't really remember. I
committed a change to remove it.
> @@ -483,7 +523,7 @@
> sizeof cap->bus_info);
> cap->version = DRIVER_VERSION_NUMBER;
> cap->capabilities = V4L2_CAP_VIDEO_CAPTURE
> - | V4L2_CAP_STREAMING;
> + | V4L2_CAP_STREAMING | V4L2_CAP_READWRITE;
> }
> break;
>
> @@ -591,7 +631,7 @@
> if (index != 0)
> return -EINVAL;
> iterm = list_entry(video->iterms.next,
> - struct uvc_entity, chain);
> + struct uvc_entity, chain);
> pin = iterm->id;
> } else if (pin < selector->selector.bNrInPins) {
> pin = selector->selector.baSourceID[index];
> @@ -815,6 +855,11 @@
> struct v4l2_requestbuffers *rb = arg;
> unsigned int bufsize =
> video->streaming->ctrl.dwMaxVideoFrameSize;
>
> + /* overcome the open function allocation */
> + uvc_queue_cancel(&video->queue);
> + INIT_LIST_HEAD(&video->queue.mainqueue);
> +
> + /* TODO: User pointer */
> if (rb->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
> rb->memory != V4L2_MEMORY_MMAP)
> return -EINVAL;
> @@ -822,10 +867,13 @@
> if ((ret = uvc_acquire_privileges(handle)) < 0)
> return ret;
>
> - if ((ret = uvc_alloc_buffers(&video->queue, rb->count,
> bufsize)) < 0)
> + if ((ret = uvc_alloc_buffers(&video->queue, rb->count, bufsize,
> rb->memory)) < 0) return ret;
>
> rb->count = ret;
> +
> +
> +
> }
> break;
>
> @@ -887,13 +935,23 @@
> if (!uvc_has_privileges(handle))
> return -EBUSY;
>
> - return uvc_video_enable(video, 0);
> + if ((ret = uvc_video_enable(video, 0)) < 0)
> + return ret;
> +
> + video->stream = UVC_STREAM_OFF;
Old leftover, can be removed.
> + return ret;
> }
>
> /* Analog video standards make no sense for digital cameras. */
> + case VIDIOC_G_STD:
> + {
> + v4l2_std_id *sidp = arg;
> + *sidp = V4L2_STD_UNKNOWN;
> + }
> + break;
> +
> case VIDIOC_ENUMSTD:
> case VIDIOC_QUERYSTD:
> - case VIDIOC_G_STD:
This has nothing to do with read() support. VIDIOC_G_STD has been discussed a
lot in the v4l mailing list in the past, and I still believe it makes no
sense for webcams.
> case VIDIOC_S_STD:
>
> case VIDIOC_OVERLAY:
> @@ -922,11 +980,109 @@
> return video_usercopy(inode, file, cmd, arg, uvc_v4l2_do_ioctl);
> }
>
> +
> static ssize_t uvc_v4l2_read(struct file *file, char __user *data,
> - size_t count, loff_t *ppos)
> + size_t count, loff_t *ppos)
> {
> - uvc_trace(UVC_TRACE_CALLS, "uvc_v4l2_read: not implemented.\n");
> - return -ENODEV;
> +
> + static struct uvc_buffer *buf = NULL;
> + struct video_device *vdev = video_devdata(file);
> + struct uvc_video_device *video = video_get_drvdata(vdev);
> + struct uvc_fh *handle = (struct uvc_fh *)file->private_data;
> + int nonblock = file->f_flags & O_NONBLOCK;
> + ssize_t ret = 0;
> + struct v4l2_buffer v4l2_b;
> + ssize_t len;
> + void *mem;
> +
> + memset(&v4l2_b, 0, sizeof v4l2_b);
> + v4l2_b.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +
> + uvc_trace(UVC_TRACE_READ, "uvc_v4l2_read\n");
> + uvc_trace(UVC_TRACE_READ, "size requested: %u\n", count);
> +
> +
> + if (!uvc_has_privileges(handle)) {
> + uvc_trace(UVC_TRACE_READ, "no privileges returning EBUSY\n");
> + return -EBUSY;
> + }
As stated above, streaming shouldn't be started in uvc_v4l2_open(). You should
acquire privileges here instead of testing for them.
> +
> + if (!data) {
> + return -EINVAL;
> + }
Checking for NULL doesn't make sense. Issues will be caught by copy_to_user.
> +
> + if (!video->stream) {
> + int i;
> + uvc_trace(UVC_TRACE_READ, "enabling stream\n");
> + if ((ret = uvc_video_enable(video, 1)) < 0)
> + return ret;
> + video->stream = UVC_STREAM_ON;
> + uvc_trace(UVC_TRACE_READ, "stream on\n");
> + uvc_trace(UVC_TRACE_READ, "queueing buffers\n");
> + for (i = 0; i < video->queue.count; ++i) {
> + struct v4l2_buffer v4l2_buf;
> + memset(&v4l2_buf, 0, sizeof v4l2_buf);
> + v4l2_buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + v4l2_buf.index = i;
> + uvc_queue_buffer(&video->queue, &v4l2_buf);
> + }
> + }
> +
> + /* it's all mine, nobody else can have it:-) */
> + if(mutex_lock_interruptible(&video->queue.mutex)) {
> + return -ERESTARTSYS;
> + }
> +
> + /* valid queue? */
> + if (list_empty(&video->queue.mainqueue)) {
> + uvc_trace(UVC_TRACE_READ, "internal driver error, queue is
> empty -
> aborting\n"); + ret = -ERESTARTSYS;
> + goto done;
> + }
Why would the queue be empty ?
> +
> + buf = list_entry(video->queue.mainqueue.next, struct uvc_buffer,
> stream);
Don't access the queue directly. Use the information returned by
uvc_dequeue_buffer().
> +
> + mutex_unlock(&video->queue.mutex);
> + if ((ret = uvc_dequeue_buffer(&video->queue, &v4l2_b, nonblock)) < 0) {
> + return ret;
> + }
> +
> + len = v4l2_b.bytesused;
> + if (len > count || len == 0) {
> + uvc_trace(UVC_TRACE_READ, "invalid length - aborting\n"
> + "\tlen: %u\n\tbufsize: %u\n\tcount: %u\n",
> + len, buf->buf.length, count);
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + if (mutex_lock_interruptible(&video->queue.mutex)) {
> + uvc_trace(UVC_TRACE_READ, "failed to keep mutex - interrupted,
> requeueing buffer\n"); + uvc_queue_buffer(&video->queue,
> &v4l2_b);
> + return -ERESTARTSYS;
> + }
No need to lock the queue here.
> +
> + /* copy the frame */
> + mem = video->queue.mem + buf->buf.m.offset;
> + if (copy_to_user(data, mem, len)) {
> + uvc_trace(UVC_TRACE_READ, "copy_to_user error\n");
> + mutex_unlock(&video->queue.mutex);
> + uvc_queue_buffer(&video->queue, &v4l2_b);
> + return -EFAULT;
> + }
This won't work. read() can be called with data buffers smaller than a video
frame. You should not assume that a read() call will read a whole buffer at
once.
> +
> + mutex_unlock(&video->queue.mutex);
And no need to unlock it here.
> + if (uvc_queue_buffer(&video->queue, &v4l2_b) < 0) {
> + uvc_trace(UVC_TRACE_READ, "problem queueing?\n");
> + }
> +
> + uvc_trace(UVC_TRACE_READ, "success: %u\n", len);
> + return len;
> +
> + done:
> + uvc_trace(UVC_TRACE_READ, "probable error condition, or interrupted\n");
> + mutex_unlock(&video->queue.mutex);
> + return ret;
> }
>
> /*
> Index: uvcvideo.h
> ===================================================================
> --- uvcvideo.h (revision 111)
> +++ uvcvideo.h (working copy)
> @@ -235,7 +235,7 @@
> /* Maximum number of video buffers. */
> #define UVC_MAX_VIDEO_BUFFERS 32
>
> -#define UVC_CTRL_TIMEOUT 300
> +#define UVC_CTRL_TIMEOUT 600
This is part of another fix. I'll work on it.
>
> /* Devices quirks */
> #define UVC_QUIRK_STATUS_INTERVAL 0x00000001
> @@ -581,11 +581,11 @@
> };
>
> struct uvc_driver {
> - struct usb_driver driver;
> + struct usb_driver driver;
>
> struct mutex open_mutex; /* protects from open/disconnect race */
>
> - struct list_head devices; /* struct uvc_device list */
> + struct list_head devices; /* struct uvc_device list */
> struct list_head controls; /* struct uvc_control_info list */
> struct mutex ctrl_mutex; /* protects controls and devices lists
> */
> };
> @@ -602,6 +602,7 @@
> #define UVC_TRACE_CALLS (1 << 5)
> #define UVC_TRACE_IOCTL (1 << 6)
> #define UVC_TRACE_FRAME (1 << 7)
> +#define UVC_TRACE_READ (1 << 8)
>
> extern unsigned int uvc_trace_param;
>
> @@ -634,7 +635,7 @@
> /* Video buffers queue management. */
> extern void uvc_queue_init(struct uvc_video_queue *queue);
> extern int uvc_alloc_buffers(struct uvc_video_queue *queue,
> - unsigned int nbuffers, unsigned int buflength);
> + unsigned int nbuffers, unsigned int buflength, int
> memtype);
> extern int uvc_free_buffers(struct uvc_video_queue *queue);
> extern void uvc_query_buffer(struct uvc_buffer *buf,
> struct v4l2_buffer *v4l2_buf);
I might have forgotten a few comments, so feel free to ask if anything seems
unclear, or if you need help.
Cheers, and thanks for the work.
Laurent Pinchart
_______________________________________________
Linux-uvc-devel mailing list
Linux-uvc-devel@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/linux-uvc-devel
