aboutsummaryrefslogtreecommitdiff
path: root/drivers
diff options
context:
space:
mode:
authorAidan Thornton <makosoft@googlemail.com>2008-04-13 15:09:36 -0300
committerMauro Carvalho Chehab <mchehab@infradead.org>2008-04-24 14:09:39 -0300
commit3b5fa928a6b2971ec65571745defc5d9758b4bc1 (patch)
tree80854c270cec4154a9b86e4ed02b4970897104b5 /drivers
parentb4916f8ca1da71bb97fb6dcf1e8da3f9c64cf80e (diff)
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 <makosoft@googlemail.com> Signed-off-by: Mauro Carvalho Chehab <mchehab@infradead.org>
Diffstat (limited to 'drivers')
-rw-r--r--drivers/media/video/em28xx/em28xx-video.c84
-rw-r--r--drivers/media/video/em28xx/em28xx.h3
2 files changed, 42 insertions, 45 deletions
diff --git a/drivers/media/video/em28xx/em28xx-video.c b/drivers/media/video/em28xx/em28xx-video.c
index d5728c2cd36..f8bbf397d3a 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);
@@ -275,38 +275,20 @@ 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 993c1ed05cc..6d62357a038 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;
};