Re: [PATCH] zr36067: Debugging cleanups (updated again)

On Thu, 24 May 2007, Ronald S. Bultje wrote:
> Hi Trent,
>
> On May 23, 2007, at 11:08 PM, Trent Piepho wrote:
> > I've attached my patch to this email.
>
> That doesn't look right. You should do it in this order:
> 1) make sure we're the actively capturing process. If not, error out.

How do you check this?  fh->v4l_buffers.active != ZORAN_FREE

> Only when you've confirmed that we're the actively capturing process
> and zr-> == fh->, then can you access the zr-> values. Also, you
> appear to poll_wait() if the buf is already DONE:

That's ok.  Keep in mind that poll_wait() doesn't wait.  It just adds the
wait queue supplied to the list of queues poll may wait on.  The poll
driver function returns immediately, and then the kernel poll logic decides
if it needs to wait on any of the queues supplied.

Typically, the driver's poll fuction will get called twice when a user
process does a select().  The first time it returns 0 and the kernel will
wait on the v4l cap queue (and any queues from the other fds in the select
call).  When the v4l cap queue wakes, the poll function is called again and
this time returns POLLIN.

> The only thing that you need to change in the original code is this
> (quasi-patch):
>
> 		queue = &zr->v4l_capq;
> 		frame = zr->v4l_pend[zr->v4l_pend_tail & V4L_MASK_FRAME];
> +              if (fh->/zr->v4l_buffers.buffer[frame].state !=
> BUZ_STATE_DONE)
> -		poll_wait(file, queue, wait);
> +			poll_wait(file, queue, wait);
> 		if (fh->v4l_buffers.buffer[frame].state == BUZ_STATE_DONE)
> ?                ^^^^ change to zr->
> 			res = POLLIN | POLLRDNORM;

This still has some of the same bugs.  It's entirely possible that there is
already a frame that can be returned, this function doesn't check that.
For example when buffer[v4l_sync_tail].state == DONE and pend_head ==
pend_tail, it returns POLLNVAL, when it should return POLLIN since there is
a frame ready to call DQBUF on.

It also doesn't properly protect the accesses with the spinlock.  I moved
the code around a bit to minimize the code that needs to be inside the
spinlock protected region.

I'm not sure if returning POLLNVAL or POLLHUP is right.  Would it be
possible for one thread to call poll() on the fd and then have another
thread call QBUF on the same fd while the first thread was sleeping in the
poll?

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

_______________________________________________
Mjpeg-developer mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/mjpeg-developer

From: Trent Piepho <[email protected]>

zr36067: Fix poll() operation

From: Trent Piepho <[email protected]>

The poll() function was looking the wrong frame.  It was using the frame
the driver was going to capture into next (pend_tail), when it should
have been looking at the next frame to be de-queued with DQBUF/SYNC
(sync_tail).

It also wasn't looking in the right spot.  It was looking at the file
handle's copy of the buffer status, rather than the driver core copy.
The interrupt routine marks frames as done in the driver core copy, the
file handle copy isn't updated.  So even if poll() looked at the right
frame, it would never see it transition to done and return POLLIN.

Signed-off-by: Trent Piepho <[email protected]>

diff --git a/linux/drivers/media/video/zoran_driver.c b/linux/drivers/media/video/zoran_driver.c
--- a/linux/drivers/media/video/zoran_driver.c
+++ b/linux/drivers/media/video/zoran_driver.c
@@ -4271,6 +4271,7 @@ zoran_poll (struct file *file,
 	struct zoran *zr = fh->zr;
 	wait_queue_head_t *queue = NULL;
 	int res = 0, frame;
+	unsigned long flags;
 
 	/* we should check whether buffers are ready to be synced on
 	 * (w/o waits - O_NONBLOCK) here
@@ -4284,19 +4285,35 @@ zoran_poll (struct file *file,
 
 	switch (fh->map_mode) {
 	case ZORAN_MAP_MODE_RAW:
-		if (fh->v4l_buffers.active == ZORAN_FREE ||
-		    zr->v4l_pend_head == zr->v4l_pend_tail) {
+		frame = zr->v4l_pend[zr->v4l_sync_tail & V4L_MASK_FRAME];
+
+		spin_lock_irqsave(&zr->spinlock, flags);
+		dprintk(3,
+			KERN_DEBUG
+			"%s: %s() - active=%c, sync_tail=%lu/%c, pend_tail=%lu, pend_head=%lu\n",
+			ZR_DEVNAME(zr), __FUNCTION__,
+			"FAL"[fh->v4l_buffers.active], zr->v4l_sync_tail,
+			"UPMD"[zr->v4l_buffers.buffer[frame].state],
+			zr->v4l_pend_tail, zr->v4l_pend_head);
+		/* Process isn't the one capturing? */
+		if (fh->v4l_buffers.active == ZORAN_FREE)
+			res = POLLNVAL;
+		/* Buffer ready to DQBUF? */
+		else if (zr->v4l_buffers.buffer[frame].state == BUZ_STATE_DONE)
+			res = POLLIN | POLLRDNORM;
+		/* No?  Are there going to be any? */
+		else if (zr->v4l_pend_head == zr->v4l_pend_tail)
+			res = POLLHUP;
+		spin_unlock_irqrestore(&zr->spinlock, flags);
+
+		if (res != POLLNVAL)
+			poll_wait(file, &zr->v4l_capq, wait);
+		else {
 			dprintk(1,
+				KERN_WARNING
 				"%s: zoran_poll() - no buffers queued\n",
 				ZR_DEVNAME(zr));
-			res = POLLNVAL;
-			goto poll_unlock_and_return;
-		}
-		queue = &zr->v4l_capq;
-		frame = zr->v4l_pend[zr->v4l_pend_tail & V4L_MASK_FRAME];
-		poll_wait(file, queue, wait);
-		if (fh->v4l_buffers.buffer[frame].state == BUZ_STATE_DONE)
-			res = POLLIN | POLLRDNORM;
+		}
 		break;
 
 	case ZORAN_MAP_MODE_JPG_REC: