From: Aidan Thornton Date: Sun, 13 Apr 2008 18:09:36 +0000 (-0300) Subject: V4L/DVB (7565): em28xx: fix buffer underrun handling X-Git-Tag: v2.6.26-rc1~1084^2~149 X-Git-Url: https://err.no/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3b5fa928a6b2971ec65571745defc5d9758b4bc1;p=linux-2.6 V4L/DVB (7565): em28xx: fix buffer underrun handling This patch fixes three related issues and a fourth trivial one: - Use buffers even if no-one's currently waiting for them (fixes underrun issues); - Don't return incomplete/mangled frames at the start of streaming and in the case of buffer underruns; - Fix an issue which could cause the driver to write to a buffer that's been freed after videobuf_queue_cancel is called (exposed by the previous two fixes - for some reason, ignoring buffers that weren't being waited on worked around the issue); - Fix a bug which could cause only one field to be filled in the first buffer (or first few buffers) after streaming is started. Signed-off-by: Aidan Thornton Signed-off-by: Mauro Carvalho Chehab --- diff --git a/drivers/media/video/em28xx/em28xx-video.c b/drivers/media/video/em28xx/em28xx-video.c index d5728c2cd3..f8bbf397d3 100644 --- a/drivers/media/video/em28xx/em28xx-video.c +++ b/drivers/media/video/em28xx/em28xx-video.c @@ -266,7 +266,7 @@ static inline void print_err_status(struct em28xx *dev, /* * video-buf generic routine to get the next available buffer */ -static inline int get_next_buf(struct em28xx_dmaqueue *dma_q, +static inline void get_next_buf(struct em28xx_dmaqueue *dma_q, struct em28xx_buffer **buf) { struct em28xx *dev = container_of(dma_q, struct em28xx, vidq); @@ -274,39 +274,21 @@ static inline int get_next_buf(struct em28xx_dmaqueue *dma_q, if (list_empty(&dma_q->active)) { em28xx_isocdbg("No active queue to serve\n"); - dev->isoc_ctl.buf = NULL; - return 0; - } - - /* Check if the last buffer were fully filled */ - *buf = dev->isoc_ctl.buf; - - /* Nobody is waiting on this buffer - discards */ - if (*buf && !waitqueue_active(&(*buf)->vb.done)) { dev->isoc_ctl.buf = NULL; *buf = NULL; + return; } - /* Returns the last buffer, to be filled with remaining data */ - if (*buf) - return 1; - /* Get the next buffer */ *buf = list_entry(dma_q->active.next, struct em28xx_buffer, vb.queue); - /* Nobody is waiting on the next buffer. returns */ - if (!*buf || !waitqueue_active(&(*buf)->vb.done)) { - em28xx_isocdbg("No active queue to serve\n"); - return 0; - } - /* Cleans up buffer - Usefull for testing for frame/URB loss */ outp = videobuf_to_vmalloc(&(*buf)->vb); memset(outp, 0, (*buf)->vb.size); dev->isoc_ctl.buf = *buf; - return 1; + return; } /* @@ -317,7 +299,7 @@ static inline int em28xx_isoc_copy(struct urb *urb) struct em28xx_buffer *buf; struct em28xx_dmaqueue *dma_q = urb->context; struct em28xx *dev = container_of(dma_q, struct em28xx, vidq); - unsigned char *outp; + unsigned char *outp = NULL; int i, len = 0, rc = 1; unsigned char *p; @@ -333,11 +315,9 @@ static inline int em28xx_isoc_copy(struct urb *urb) return 0; } - rc = get_next_buf(dma_q, &buf); - if (rc <= 0) - return rc; - - outp = videobuf_to_vmalloc(&buf->vb); + buf = dev->isoc_ctl.buf; + if (buf != NULL) + outp = videobuf_to_vmalloc(&buf->vb); for (i = 0; i < urb->number_of_packets; i++) { int status = urb->iso_frame_desc[i].status; @@ -374,24 +354,27 @@ static inline int em28xx_isoc_copy(struct urb *urb) em28xx_isocdbg("Video frame %d, length=%i, %s\n", p[2], len, (p[2] & 1)? "odd" : "even"); - if (p[2] & 1) - buf->top_field = 0; - else - buf->top_field = 1; - -// if (dev->isoc_ctl.last_field && !buf->top_field) { - if (dev->isoc_ctl.last_field != buf->top_field) { - buffer_filled(dev, dma_q, buf); - rc = get_next_buf(dma_q, &buf); - if (rc <= 0) - return rc; - outp = videobuf_to_vmalloc(&buf->vb); + if (!(p[2] & 1)) { + if (buf != NULL) + buffer_filled(dev, dma_q, buf); + get_next_buf(dma_q, &buf); + if (buf == NULL) + outp = NULL; + else + outp = videobuf_to_vmalloc(&buf->vb); + } + + if (buf != NULL) { + if (p[2] & 1) + buf->top_field = 0; + else + buf->top_field = 1; } - dev->isoc_ctl.last_field = buf->top_field; dma_q->pos = 0; } - em28xx_copy_video(dev, dma_q, buf, p, outp, len); + if (buf != NULL) + em28xx_copy_video(dev, dma_q, buf, p, outp, len); } return rc; } @@ -597,12 +580,29 @@ buffer_setup(struct videobuf_queue *vq, unsigned int *count, unsigned int *size) return 0; } +/* This is called *without* dev->slock held; please keep it that way */ static void free_buffer(struct videobuf_queue *vq, struct em28xx_buffer *buf) { + struct em28xx_fh *fh = vq->priv_data; + struct em28xx *dev = fh->dev; + unsigned long flags = 0; if (in_interrupt()) BUG(); - videobuf_waiton(&buf->vb, 0, 0); + /* We used to wait for the buffer to finish here, but this didn't work + because, as we were keeping the state as VIDEOBUF_QUEUED, + videobuf_queue_cancel marked it as finished for us. + (Also, it could wedge forever if the hardware was misconfigured.) + + This should be safe; by the time we get here, the buffer isn't + queued anymore. If we ever start marking the buffers as + VIDEOBUF_ACTIVE, it won't be, though. + */ + spin_lock_irqsave(&dev->slock, flags); + if (dev->isoc_ctl.buf == buf) + dev->isoc_ctl.buf = NULL; + spin_unlock_irqrestore(&dev->slock, flags); + videobuf_vmalloc_free(&buf->vb); buf->vb.state = VIDEOBUF_NEEDS_INIT; } diff --git a/drivers/media/video/em28xx/em28xx.h b/drivers/media/video/em28xx/em28xx.h index 993c1ed05c..6d62357a03 100644 --- a/drivers/media/video/em28xx/em28xx.h +++ b/drivers/media/video/em28xx/em28xx.h @@ -114,9 +114,6 @@ struct em28xx_usb_isoc_ctl { /* Stores already requested buffers */ struct em28xx_buffer *buf; - /* Store last filled frame */ - int last_field; - /* Stores the number of received fields */ int nfields; };