'Re: [Mjpeg-developer] [PATCH] zr36067: Debugging cleanups (updated'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       mjpeg-developer
Subject:    Re: [Mjpeg-developer] [PATCH] zr36067: Debugging cleanups (updated
From:       Trent Piepho <xyzzy () speakeasy ! org>
Date:       2007-05-25 16:39:34
Message-ID: Pine.LNX.4.58.0705250851230.31308 () shell4 ! speakeasy ! net
[Download RAW message or body]

>
> There's probably some bugs, I fully realize that. E.g., it indeed
> shouldn't use frame=zr->v4l_pend[pend_tail & MASK_FRAME], it should
> use v4l_pend[sync_tail & MASK_FRAME] instead. Also, it appears that
> with all this, we don't need to check pend_head/pend_tail at all, you
> can probably remove that (a few lines up from here). With those
> changes, I think the code is fine. E.g. in the line "if (zr-
>  >v4l_buffers.buffer[frame].state == BUZ_STATE_DONE)", we then do
> check whether the frame is ready to be returned already.
>
> > 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?
>
> Theoretically, although there really isn't any point. Since we use
> locks (resource_lock) in zoran_poll, this should be fine.

I've made a mutli-threaded test application, and the way poll works isn't
right.  Keep in mind I'm talking about a single process with multiple
threads sharing the same file descriptor.

My test will call select() on the zoran capture fd in one thread.  Then,
while that thread is blocked inside select(), another thread will call
REQBUFS/QBUF/STREAMON.

If you don't call poll_wait() to add the v4l capture queue, it doesn't
work.  select() blocks (poll returning POLLNVAL doesn't cause select to
return an error) and never returns.  The other thread queues and captures a
frame, but since the capq wasn't added select never wakes up and sees that
a frame is ready.

This is what the log looks like:
[these syscalls by thread 1]
DC10plus[0]: VIDIOC_S_FMT - type=1, size=192x128, fmt=0x56595559 (YUYV)
DC10plus[0]: zoran_poll() - active=F, sync_tail=0/U, pend_tail=0, pend_head=0
DC10plus[0]: zoran_poll() - no buffers queued
[thread 1 is now blocked inside select()]
[following syscalls by thread 2]
DC10plus[0]: VIDIOC_REQBUFS - type=1
DC10plus[0]: VIDIOC_QBUF - type=1, index=0
DC10plus[0]: VIDIOC_QBUF - type=1, index=1
DC10plus[0]: VIDIOC_STREAMON
DC10plus[0]: set_vfe() - width = 192, height = 128
[frame is ready, but select() did not wake, zr->v4l_capq wasn't added]
[process will just sit here until you interrupt it with a signal]

After my latest patch, attached, this is what happens:

[these syscalls by thread 1]
DC10plus[0]: VIDIOC_S_FMT - type=1, size=192x128, fmt=0x56595559 (YUYV)
DC10plus[0]: zoran_poll() - active=F, sync_tail=0/U, pend_tail=0, pend_head=0
[thread 1 is now blocked inside select()]
[following syscalls by thread 2]
DC10plus[0]: VIDIOC_REQBUFS - type=1
DC10plus[0]: VIDIOC_QBUF - type=1, index=0
DC10plus[0]: VIDIOC_QBUF - type=1, index=1
DC10plus[0]: VIDIOC_STREAMON
DC10plus[0]: set_vfe() - width = 192, height = 128
[kernel select() checks poll status again, since capture started]
DC10plus[0]: zoran_poll() - active=L, sync_tail=0/P, pend_tail=0, pend_head=2
[frame still in PEND state, continue to block on select()]
[frame captured, select() wakes and checks poll status]
DC10plus[0]: zoran_poll() - active=L, sync_tail=0/D, pend_tail=1, pend_head=2
[select returns and thread 1 calls DQBUF]
DC10plus[0]: VIDIOC_DQBUF - type=1
["zr_poll.patch" (TEXT/PLAIN)]

From: Trent Piepho <xyzzy@speakeasy.org>

zr36067: Fix poll() operation

From: Trent Piepho <xyzzy@speakeasy.org>

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.

There was some logic to detect when there was no current capture in process
nor any frames queued and try to return an error, which ends up being a bad
idea.  It's possible to call select() from one thread while no capture is in
process, and then start a capture from another thread.

Signed-off-by: Trent Piepho <xyzzy@speakeasy.org>

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,24 @@ 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) {
-			dprintk(1,
-				"%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)
+		poll_wait(file, &zr->v4l_capq, wait);
+		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 is the one capturing? */
+		if (fh->v4l_buffers.active != ZORAN_FREE &&
+		    /* Buffer ready to DQBUF? */
+		    zr->v4l_buffers.buffer[frame].state == BUZ_STATE_DONE)
 			res = POLLIN | POLLRDNORM;
+		spin_unlock_irqrestore(&zr->spinlock, flags);
+
 		break;
 
 	case ZORAN_MAP_MODE_JPG_REC:


-------------------------------------------------------------------------
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
Mjpeg-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mjpeg-developer


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic