Web lists-archives.com

[Spca50x-devs] PATCH: gspcav2: pac207 support: gspca_main fixes

Hi all,

I've finally managed to make some time to work on gspcav2. I've been working on fixing the pac207 to work and to incorporate some improvements I've added to my own pac207 v4l2 driver (better autogain support).

During this I've hit 3 issues in gspca_main.ko from gspcav2-0.0.28, for which I've attached the following patches, listed in order in which I encountered the issues during development:

Due to a bug in pac207.c (which I introduced while working on it) the cam crashed, causing the entire ISOC urb flow to stop, so no more iso_irq's. This caused gspca_frame_wait, to wait undefinitely on the waitqueue causing a softlock up of the kernel (yeah lots of reboots to debug that one).

This patch fixes this by adding a timeout, and it also adds a check for device disconnects and some other thread / user stopping the stream (yes this can happen with our current design, which imho warrant a rethink, more on that later).

ekiga, for some unknown reason does the following:
1) open the device
2) request buffers
3) open the device again
4) close the fd returned in 3)
5) queue buffers

5) fails because the close in 4) will free all buffers / frames causing any index passed in 5) to be invalid.

Now one may claim this is an ekiga bug, but it is not, the current gspca_main.ko wrongly claims to support multiple opens and multiple readers even. This clearly shows it doesn't, with multiple (possibly independent) readers each reader will want to be in charge of the frame/buffer queue, this will only work if there is a queue _per_ user and then some ugliness in the isoirq handling to feed received data to all users with available buffers. But currently we only have one queue per device, not per user. Since the v4l2 spec does not ask for support for multiple readers, the easiest (and cleanest / sanest) fix is to not allow multiple opens at all. This is ok with ekiga as its fine with its step 3) failing.

gspca_frame_add() wrongly assumes (in a PDEBUG call) that it may dereference its data argument even if the len argument is 0. Since the pac207 code does a: gspca_frame_add(gspca_dev, FIRST_PACKET, frame, NULL, 0); on a sof (and the drops back to its normal routine of adding INTERMEDIATE packets with actual data), this causes a kernel panic (null deref in irq context) when debugging is enabled. This was another fun one to debug.

With these 3 patches applied (and a heavily modified pac207.c) I now have gspcav2 working with a itworks PCW03 (093a:2460) pac207 webcam.

I still need todo some cleanups to my pac207 code and then I'll send a patch for that too.



--- gspcav2-0.0.28/gspca.c	2008-04-12 11:55:47.000000000 +0200
+++ gspcav2-0.0.28.new/gspca.c	2008-04-19 21:19:42.000000000 +0200
@@ -1346,10 +1345,13 @@
 	/* wait till a frame is ready */
 	for (;;) {
-		ret = wait_event_interruptible(gspca_dev->wq,
-					atomic_read(&gspca_dev->nevent) > 0);
-		if (ret != 0)
-			return ret;
+		ret = wait_event_interruptible_timeout(gspca_dev->wq,
+					atomic_read(&gspca_dev->nevent) > 0,
+					msecs_to_jiffies(3000));
+		if (ret <= 0)
+			return (ret < 0)? ret : -EIO;
+		if (!gspca_dev->streaming || !gspca_dev->present)
+			return -EIO;
 		i = gspca_dev->fr_o;
 		j = gspca_dev->fr_queue[i];
 		frame = &gspca_dev->frame[j];
--- gspcav2-0.0.28/gspca.c	2008-04-12 11:55:47.000000000 +0200
+++ gspcav2-0.0.28.new/gspca.c	2008-04-19 21:19:42.000000000 +0200
@@ -172,8 +172,7 @@
 	int i, j;
-	PDEBUG(D_PACK, "add t:%d l:%d %02x %02x %02x %02x...",
-		packet_type, len, data[0], data[1], data[2], data[3]);
+	PDEBUG(D_PACK, "add t:%d l:%d",	packet_type, len);
 	/* when start of a new frame, if the current frame buffer
 	 * is not queued, discard the whole frame */
--- gspcav2-0.0.28/gspca.c	2008-04-12 11:55:47.000000000 +0200
+++ gspcav2-0.0.28.new/gspca.c	2008-04-19 21:19:42.000000000 +0200
@@ -859,7 +858,7 @@
 			PDEBUG(D_ERR|D_CONF, "init device failed %d", ret);
 			goto out;
-	} else if (gspca_dev->users > 8) {	/* (arbitrary value) */
+	} else if (gspca_dev->users >= 1) { /* should never be > 1 */
 		ret = -EBUSY;
 		goto out;
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
Spca50x-devs mailing list