[PATCH] Add support for sq905 based cameras to gspca

[PATCH] Add support for sq905 based cameras to gspca

[Adam Baker]

Add initial support for cameras based on the SQ Technologies SQ-905
chipset (USB ID 2770:9120) to V4L2 using the gspca infrastructure.
Currently only supports one resolution and doesn't attempt to inform
libv4l what image flipping options are needed.

Signed-off-by: Adam Baker <linux@baker-net.org.uk>
Signed-off-by: Theodore Kilgore <kilgota@auburn.edu>

---
Following all the comments when I posted this for review Theodore and I have
produced 2 new versions. The most critical comment last time was that we
were using the system work queue inappropriately so this version creates
its own work queue. The alternative version that I will post shortly avoids a
work queue altogether by using asynchronous USB commands but in order to
do so has increased the code size.

I'll leave it to the assembled list expertise to say which option is preferable.

I think we've included all of Hans de Goede's review suggestions and most
of Alexey Klimov's, the exception being about the format of the camera ID debug 
message which I have other plans for once we can actually make use of
that info.
---
diff -r c1ebc0e03fa1 linux/drivers/media/video/gspca/Kconfig
--- a/linux/drivers/media/video/gspca/Kconfig	Sun Jan 18 18:24:52 2009 +0100
+++ b/linux/drivers/media/video/gspca/Kconfig	Mon Jan 19 22:59:24 2009 +0000
@@ -176,6 +176,15 @@ config USB_GSPCA_SPCA561
 	  To compile this driver as a module, choose M here: the
 	  module will be called gspca_spca561.
 
+config USB_GSPCA_SQ905
+	tristate "SQ Technologies SQ905 based USB Camera Driver"
+	depends on VIDEO_V4L2 && USB_GSPCA
+	help
+	  Say Y here if you want support for cameras based on the SQ905 chip.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called gspca_sq905.
+
 config USB_GSPCA_STK014
 	tristate "Syntek DV4000 (STK014) USB Camera Driver"
 	depends on VIDEO_V4L2 && USB_GSPCA
diff -r c1ebc0e03fa1 linux/drivers/media/video/gspca/Makefile
--- a/linux/drivers/media/video/gspca/Makefile	Sun Jan 18 18:24:52 2009 +0100
+++ b/linux/drivers/media/video/gspca/Makefile	Mon Jan 19 22:59:24 2009 +0000
@@ -16,6 +16,7 @@ obj-$(CONFIG_USB_GSPCA_SPCA506)  += gspc
 obj-$(CONFIG_USB_GSPCA_SPCA506)  += gspca_spca506.o
 obj-$(CONFIG_USB_GSPCA_SPCA508)  += gspca_spca508.o
 obj-$(CONFIG_USB_GSPCA_SPCA561)  += gspca_spca561.o
+obj-$(CONFIG_USB_GSPCA_SQ905)    += gspca_sq905.o
 obj-$(CONFIG_USB_GSPCA_SUNPLUS)  += gspca_sunplus.o
 obj-$(CONFIG_USB_GSPCA_STK014)   += gspca_stk014.o
 obj-$(CONFIG_USB_GSPCA_T613)     += gspca_t613.o
@@ -41,6 +42,7 @@ gspca_spca506-objs  := spca506.o
 gspca_spca506-objs  := spca506.o
 gspca_spca508-objs  := spca508.o
 gspca_spca561-objs  := spca561.o
+gspca_sq905-objs    := sq905.o
 gspca_stk014-objs   := stk014.o
 gspca_sunplus-objs  := sunplus.o
 gspca_t613-objs     := t613.o
diff -r c1ebc0e03fa1 linux/drivers/media/video/gspca/sq905.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/drivers/media/video/gspca/sq905.c	Mon Jan 19 22:59:24 2009 +0000
@@ -0,0 +1,410 @@
+/*
+ * SQ905 subdriver
+ *
+ * Copyright (C) 2008, 2009 Adam Baker and Theodore Kilgore
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+/*
+ * History and Acknowledgments
+ *
+ * The original Linux driver for SQ905 based cameras was written by
+ * Marcell Lengyel and furter developed by many other contributers
+ * and is available from http://sourceforge.net/projects/sqcam/
+ *
+ * This driver takes advantage of the reverse engineering work done for
+ * that driver and for libgphoto2 but shares no code with them.
+ *
+ * This driver has used as a base the finepix driver and other gspca
+ * based drivers and may still contain code fragments taken from those
+ * drivers.
+ */
+
+#define MODULE_NAME "sq905"
+
+#include <linux/workqueue.h>
+#include "gspca.h"
+
+MODULE_AUTHOR("Adam Baker <linux@baker-net.org.uk>, "
+		"Theodore Kilgore <kilgota@auburn.edu>");
+MODULE_DESCRIPTION("GSPCA/SQ905 USB Camera Driver");
+MODULE_LICENSE("GPL");
+
+/* Default timeouts, in ms */
+#define SQ905_CMD_TIMEOUT 500
+#define SQ905_DATA_TIMEOUT 250
+
+/* Maximum transfer size to use. */
+#define SQ905_MAX_TRANSFER 0x8000
+#define FRAME_HEADER_LEN 64
+
+/* The known modes, or registers. These go in the "value" slot. */
+
+/* 00 is "none" obviously */
+
+#define SQ905_BULK_READ	0x03	/* precedes any bulk read */
+#define SQ905_COMMAND	0x06	/* precedes the command codes below */
+#define SQ905_PING	0x07	/* when reading an "idling" command */
+#define SQ905_READ_DONE 0xc0    /* ack bulk read completed */
+
+/* Some command codes. These go in the "index" slot. */
+
+#define SQ905_ID      0xf0	/* asks for model string */
+#define SQ905_CONFIG  0x20	/* gets photo alloc. table, not used here */
+#define SQ905_DATA    0x30	/* accesses photo data, not used here */
+#define SQ905_CLEAR   0xa0	/* clear everything */
+#define SQ905_CAPTURE_LOW 0x60	/* Starts capture at 160x120 */
+#define SQ905_CAPTURE_MED 0x61	/* Starts capture at 320x240 */
+/* note that the capture command also controls the output dimensions */
+/* 0x60 -> 160x120, 0x61 -> 320x240 0x62 -> 640x480 depends on camera */
+/* 0x62 is not correct, at least for some cams. Should be 0x63 ? */
+
+
+
+/* Structure to hold all of our device specific stuff */
+struct sd {
+	struct gspca_dev gspca_dev;	/* !! must be the first item */
+
+	enum {
+		SQ905_MODEL_DEFAULT,
+		SQ905_MODEL_POCK_CAM,
+		SQ905_MODEL_MAGPIX
+	} cam_model;
+
+	/*
+	 * Driver stuff
+	 */
+	__u8 *buffer;
+	struct work_struct work_struct;
+	struct workqueue_struct *work_thread;
+	struct completion can_close;
+	int streaming;
+};
+
+/* The driver only supports 320x240 so far. */
+static struct v4l2_pix_format sq905_mode[1] = {
+	{ 320, 240, V4L2_PIX_FMT_SBGGR8, V4L2_FIELD_NONE,
+		.bytesperline = 320,
+		.sizeimage = 320 * 240,
+		.colorspace = V4L2_COLORSPACE_SRGB,
+		.priv = 0}
+};
+
+/*
+ * Send a command to the camera - this function, sq905_ack_frame and
+ * sq905_read_data must only be called from start / stop or init
+ * when not streaming or from the streaming thread when streaming to avoid
+ * a race on accessing usb_buf.
+ */
+static int sq905_command(struct gspca_dev *gspca_dev, __u16 index)
+{
+	int ret;
+	gspca_dev->usb_buf[0] = '\0';
+	ret = usb_control_msg(gspca_dev->dev,
+			      usb_sndctrlpipe(gspca_dev->dev, 0),
+			      USB_REQ_SYNCH_FRAME,                /* request */
+			      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      SQ905_COMMAND, index, gspca_dev->usb_buf, 1,
+			      SQ905_CMD_TIMEOUT);
+	if (ret != 1) {
+		PDEBUG(D_ERR, "%s: usb_control_msg failed (%d)",
+			__func__, ret);
+		return -EIO;
+	}
+
+	ret = usb_control_msg(gspca_dev->dev,
+			      usb_sndctrlpipe(gspca_dev->dev, 0),
+			      USB_REQ_SYNCH_FRAME,                /* request */
+			      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      SQ905_PING, 0, gspca_dev->usb_buf, 1,
+			      SQ905_CMD_TIMEOUT);
+	if (ret != 1) {
+		PDEBUG(D_ERR, "%s: usb_control_msg failed 2 (%d)",
+			__func__, ret);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+/*
+ * Acknowledge the end of a frame - see warning on sq905_command.
+ */
+static int sq905_ack_frame(struct gspca_dev *gspca_dev)
+{
+	int ret;
+	gspca_dev->usb_buf[0] = '\0';
+	ret = usb_control_msg(gspca_dev->dev,
+			      usb_sndctrlpipe(gspca_dev->dev, 0),
+			      USB_REQ_SYNCH_FRAME,                /* request */
+			      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      SQ905_READ_DONE, 0, gspca_dev->usb_buf, 1,
+			      SQ905_CMD_TIMEOUT);
+	if (ret != 1) {
+		PDEBUG(D_ERR, "%s: usb_control_msg failed (%d)", __func__, ret);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+/*
+ *  request and read a block of data - see warning on sq905_command.
+ */
+static int
+sq905_read_data(struct gspca_dev *gspca_dev, __u8 *data, int size)
+{
+	int ret;
+	int act_len;
+
+	if (!data) {
+		PDEBUG(D_ERR, "%s: data pointer was NULL\n", __func__);
+		return -EINVAL;
+	}
+
+	gspca_dev->usb_buf[0] = '\0';
+	ret = usb_control_msg(gspca_dev->dev,
+			      usb_sndctrlpipe(gspca_dev->dev, 0),
+			      USB_REQ_SYNCH_FRAME,                /* request */
+			      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      SQ905_BULK_READ, size, gspca_dev->usb_buf,
+			      1, SQ905_CMD_TIMEOUT);
+	if (ret != 1) {
+		PDEBUG(D_ERR, "%s: usb_control_msg failed (%d)", __func__, ret);
+		return -EIO;
+	}
+	ret = usb_bulk_msg(gspca_dev->dev,
+			   usb_rcvbulkpipe(gspca_dev->dev, 0x81),
+			   data, size, &act_len, SQ905_DATA_TIMEOUT);
+	/* successful, it returns 0, otherwise  negative */
+	if ((ret != 0) || (act_len != size)) {
+		PDEBUG(D_ERR, "%s: bulk read fail (%d) len %d/%d",
+			__func__, ret, act_len, size);
+		return -EIO;
+	}
+	return 0;
+}
+
+/* This function is called as a workqueue function and runs whenever the camera
+ * is streaming data. Because it is a workqueue function it is allowed to sleep
+ * so we can use synchronous USB calls. There appears to be no reason to
+ * terminate before we stop streaming.
+ */
+static void sq905_dostream(struct work_struct *work)
+{
+	struct sd *dev = container_of(work, struct sd, work_struct);
+	struct gspca_frame *frame;
+	int bytes_left; /* bytes remaining in current frame. */
+	int data_len;   /* size to use for the next read. */
+	int header_read; /* true if we have already read the frame header. */
+	int discarding; /* true if we failed to get space for frame. */
+	int packet_type;
+	__u8 *data;
+
+	while ((dev->gspca_dev.present) && (dev->streaming)) {
+		/* request some data and then read it until we have
+		 * a complete frame. */
+		bytes_left = sq905_mode[0].sizeimage + FRAME_HEADER_LEN;
+		header_read = 0;
+		discarding = 0;
+
+		while (bytes_left > 0) {
+			data_len = bytes_left > SQ905_MAX_TRANSFER  ?
+				SQ905_MAX_TRANSFER : bytes_left;
+			if (sq905_read_data(&dev->gspca_dev, dev->buffer,
+					 data_len) == 0) {
+				PDEBUG(D_STREAM,
+					"Got %d bytes out of %d for frame",
+					data_len, bytes_left);
+				bytes_left -= data_len;
+				data = dev->buffer;
+				if (!header_read) {
+					packet_type = FIRST_PACKET;
+					/* The first 64 bytes of each frame are
+					 * a header full of FF 00 bytes */
+					data += FRAME_HEADER_LEN;
+					data_len -= FRAME_HEADER_LEN;
+					header_read = 1;
+				} else if (bytes_left == 0) {
+					packet_type = LAST_PACKET;
+				} else {
+					packet_type = INTER_PACKET;
+				}
+				frame = gspca_get_i_frame(&dev->gspca_dev);
+				if (frame &&  !discarding)
+					gspca_frame_add(&dev->gspca_dev,
+						packet_type, frame,
+						data, data_len);
+				else
+					discarding = 1;
+			} else {
+				/* shut down if we got a USB error. */
+				dev->streaming = 0;
+				goto quit_stream;
+			}
+		}
+		/* acknowledge the frame */
+		sq905_ack_frame(&dev->gspca_dev);
+	}
+quit_stream:
+	complete(&dev->can_close);
+}
+
+/* this function is called at probe time */
+static int sd_config(struct gspca_dev *gspca_dev,
+		const struct usb_device_id *id)
+{
+	struct cam *cam = &gspca_dev->cam;
+
+	cam->cam_mode = sq905_mode;
+	cam->nmodes = 1;
+	/* We don't use the buffer gspca allocates so make it small. */
+	cam->bulk_size = 64;
+	return 0;
+}
+
+/* Stop streaming and free the resources allocated by sd_start. */
+static void sd_stopN(struct gspca_dev *gspca_dev)
+{
+	struct sd *dev = (struct sd *) gspca_dev;
+
+	dev->streaming = 0;
+
+	/* wait for the work queue to terminate */
+	wait_for_completion(&dev->can_close);
+}
+
+/* called on streamoff with alt 0 and disconnect */
+static void sd_stop0(struct gspca_dev *gspca_dev)
+{
+	struct sd *dev = (struct sd *) gspca_dev;
+	sq905_command(gspca_dev, SQ905_CLEAR);
+	kfree(dev->buffer);
+	destroy_workqueue(dev->work_thread);
+	dev->work_thread = NULL;
+}
+
+/* this function is called at probe and resume time */
+static int sd_init(struct gspca_dev *gspca_dev)
+{
+	struct sd *dev = (struct sd *) gspca_dev;
+	dev->work_thread = NULL;
+	INIT_WORK(&dev->work_struct, sq905_dostream);
+
+	/* connect to the camera and read
+	 * the model ID and process that and put it away.
+	 */
+	sq905_command(gspca_dev, SQ905_CLEAR);
+	sq905_command(gspca_dev, SQ905_ID);
+	sq905_read_data(gspca_dev, gspca_dev->usb_buf, 4);
+	sq905_command(gspca_dev, SQ905_CLEAR);
+	if (!memcmp(gspca_dev->usb_buf, "\x09\x05\x01\x19", 4)) {
+		dev->cam_model = SQ905_MODEL_POCK_CAM;
+		PDEBUG(D_CONF, "Model is SQ905_MODEL_POCK_CAM");
+	} else if (!memcmp(gspca_dev->usb_buf, "\x09\x05\x01\x32", 4)) {
+		dev->cam_model = SQ905_MODEL_MAGPIX;
+		PDEBUG(D_CONF, "Model is SQ905_MODEL_MAGPIX");
+	} else {
+		dev->cam_model = SQ905_MODEL_DEFAULT;
+		PDEBUG(D_CONF, "Model is SQ905_MODEL_DEFAULT");
+	}
+	return 0;
+}
+
+/* Set up for getting frames. */
+static int sd_start(struct gspca_dev *gspca_dev)
+{
+	struct sd *dev = (struct sd *) gspca_dev;
+
+	/* Various initializations. */
+	dev->buffer = kmalloc(SQ905_MAX_TRANSFER, GFP_KERNEL | GFP_DMA);
+	if (!dev->buffer) {
+		PDEBUG(D_ERR, "%s: Couldn't allocate USB buffer\n", __func__);
+		return -ENOMEM;
+	}
+	init_completion(&dev->can_close);
+	dev->streaming = 1;
+
+	/* "Open the shutter" and set size, to start capture */
+	if (sq905_command(gspca_dev, SQ905_CAPTURE_MED)) {
+		PDEBUG(D_ERR, "%s: start streaming command failed\n",
+			__func__);
+		dev->streaming = 0;
+		return -EIO;
+	}
+	/* Start the workqueue function to do the streaming */
+	BUG_ON(dev->work_thread);
+	dev->work_thread = create_workqueue(MODULE_NAME);
+	queue_work(dev->work_thread, &dev->work_struct);
+
+	return 0;
+}
+
+/* Table of supported USB devices */
+static const __devinitdata struct usb_device_id device_table[] = {
+	{USB_DEVICE(0x2770, 0x9120)},
+	{}
+};
+
+MODULE_DEVICE_TABLE(usb, device_table);
+
+/* sub-driver description */
+static const struct sd_desc sd_desc = {
+	.name   = MODULE_NAME,
+	.config = sd_config,
+	.init   = sd_init,
+	.start  = sd_start,
+	.stopN  = sd_stopN,
+	.stop0  = sd_stop0,
+};
+
+/* -- device connect -- */
+static int sd_probe(struct usb_interface *intf,
+		const struct usb_device_id *id)
+{
+	return gspca_dev_probe(intf, id,
+			&sd_desc,
+			sizeof(struct sd),
+			THIS_MODULE);
+}
+
+static struct usb_driver sd_driver = {
+	.name       = MODULE_NAME,
+	.id_table   = device_table,
+	.probe      = sd_probe,
+	.disconnect = gspca_disconnect,
+#ifdef CONFIG_PM
+	.suspend = gspca_suspend,
+	.resume  = gspca_resume,
+#endif
+};
+
+/* -- module insert / remove -- */
+static int __init sd_mod_init(void)
+{
+	if (usb_register(&sd_driver) < 0)
+		return -1;
+	PDEBUG(D_PROBE, "registered");
+	return 0;
+}
+static void __exit sd_mod_exit(void)
+{
+	usb_deregister(&sd_driver);
+	PDEBUG(D_PROBE, "deregistered");
+}
+
+module_init(sd_mod_init);
+module_exit(sd_mod_exit);

Re: [PATCH] Add support for sq905 based cameras to gspca

[Alexey Klimov]

Hello, Adam
May i add small note if you don't mind ?

On Mon, 2009-01-19 at 23:22 +0000, Adam Baker wrote:

<snip>

> +/* -- module insert / remove -- */
> +static int __init sd_mod_init(void)
> +{
> +	if (usb_register(&sd_driver) < 0)
> +		return -1;
> +	PDEBUG(D_PROBE, "registered");
> +	return 0;
> +}
> +static void __exit sd_mod_exit(void)
> +{
> +	usb_deregister(&sd_driver);
> +	PDEBUG(D_PROBE, "deregistered");
> +}

May be it's better for CodingStyle if sd_mod_init will look like this:

static int __init sd_mod_init(void) 
{ 
	int ret; 
	ret = usb_register(&sd_driver); 
	if (ret < 0) 
		return ret; 
	PDEBUG(D_PROBE, "registered"); 
	return 0; 
}

?


> +module_init(sd_mod_init);
> +module_exit(sd_mod_exit);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Best regards, Klimov Alexey

Re: [PATCH] Add support for sq905 based cameras to gspca

[Jean-Francois Moine]

On Mon, 19 Jan 2009 23:22:33 +0000
Adam Baker <linux@baker-net.org.uk> wrote:

> Add initial support for cameras based on the SQ Technologies SQ-905
> chipset (USB ID 2770:9120) to V4L2 using the gspca infrastructure.
	[snip]
> ---
> Following all the comments when I posted this for review Theodore and
> I have produced 2 new versions. The most critical comment last time
> was that we were using the system work queue inappropriately so this
> version creates its own work queue. The alternative version that I
> will post shortly avoids a work queue altogether by using
> asynchronous USB commands but in order to do so has increased the
> code size.
> 
> I'll leave it to the assembled list expertise to say which option is
> preferable.
	[snip]

Hello Adam and Theodore,

I looked at your two versions, and I think the first one (work queue)
is the simplest. So, I am ready to put your driver in my repository for
inclusion in a next linux kernel.

I have just a few remarks and a request.

- There are still small CodingStyle errors.

- Why do you need the function name in the debug messages?

- In sd_init, you should better convert the 4 bytes to u32 and do a
  switch.

- On disconnection, the function sd_stopN is not called, so the
  workqueue may be still running.

- At streamon time (sd_start), you allocate the buffer and send a
  command. This may be done in the workqueue function. This function may
  also do the buffer free and send the stop command on exit.

Re-thinking the streaming part gives:
. streamon (sd_start)
	. init_completion()
	. start the workqueue
	  (dev->streaming is not useful)
. workqueue function
	. allocate the transfer buffer (pointer in the stack)
	. send 'start capture'
	. read loop - don't forget:
		- to test gspca_dev->streaming: it may be streamoff,
			close or disconnect
		- to protect to usb_control_msg by the
			gspca_dev->usb_lock mutex: this will permit
			to handle future webcam controls.
	. on streamoff or USB error
		. free the transfer buffer
		. complete()
. streamoff
	. sd_stopN: non useful
	. sd_stop0:
		. wait_for_completion
		. dev->work_thread = NULL

Now, the request: some guys asked for support of their webcams based on
sq930x chips. A SANE backend driver exists, written by Gerard Klaver
(http://gkall.hobby.nl/sq930x.html).
May you have a look and say if handling these chips may be done in your
driver?

Regards.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] Add support for sq905 based cameras to gspca

[kilgota]

On Wed, 21 Jan 2009, Jean-Francois Moine wrote:

<snip>

> Hello Adam and Theodore,
>
> I looked at your two versions, and I think the first one (work queue)
> is the simplest. So, I am ready to put your driver in my repository for
> inclusion in a next linux kernel.

Thank you.

>
> I have just a few remarks and a request.

The remarks need close attention, which will be applied. Thank you again 
for attention to detail. The request I can deal with right now.

> Now, the request: some guys asked for support of their webcams based on
> sq930x chips. A SANE backend driver exists, written by Gerard Klaver
> (http://gkall.hobby.nl/sq930x.html).
> May you have a look and say if handling these chips may be done in your
> driver?

In a word, no. Sorry. The only real resemblance of either of these devices 
to the other is that they share the same Vendor number.

Now, thanks again for the comments. Adam and I will get busy on them.

Theodore Kilgore

Why not to try to combine sq905 and sq

[kilgota]

On Wed, 21 Jan 2009, Jean-Francois Moine wrote:

(previous material covered a different topic)

> Now, the request: some guys asked for support of their webcams based on
> sq930x chips. A SANE backend driver exists, written by Gerard Klaver
> (http://gkall.hobby.nl/sq930x.html).
> May you have a look and say if handling these chips may be done in your
> driver?

I am sorry to have been rather brief about this one in my previous 
message, but I am quite serious in saying "no." Clearly it is always 
possible somehow to jam-fit the support for two or more very different 
devices into one block of code. One would require some kind of branching 
at every logical step in the code, and nobody would be able to read it and 
make sense of that code, or to maintain it.

In this present case of the sq930x, I did look at Gerard's information.
Indeed, the very structure of the commands is different from the commands
for the sq905 and for that matter from the sq905c cameras, with both of
which I am intimately familiar. A mere glance at the file

creative protocol I420.txt

at Gerard's web page suffices to show that the command sequences are 
completely different. One can add to this that the frames are JPEG, 
whereas the frames for the sq905 are uncompressed bitmap data.

I would be glad to help with the sq930x if there is any help I can give 
and if the help is wanted. Anyone who wants me to do that should just ask. 
But that is a totally different matter from trying to glue together the 
support for two very unlike devices in one driver module, apparently for 
the reason that the two devices share the same Vendor number.

As a very relevant analogy, permit me to say that I wrote the driver in
libgphoto2 for the Logitech Clicksmart 310 camera. When it became clear
that this is yet another spca50x camera, I contacted the person who had
written the unified spca50x driver for libgphoto2 and asked him if,
because of this, he wanted this new spca5x camera to be included there. I
sent him my code. He looked at it, and he said to leave it as a separate
driver. In spite of using the same chipset, the camera was just too
different. He further added that he was sorry he had tried to put so many
cameras into one driver in the first place, had learned a lesson from the
experience, and if he had it to do over again he would not.

Theodore Kilgore

Re: [PATCH] Add support for sq905 based cameras to gspca

[Jean-Francois Moine]

On Mon, 2 Feb 2009 20:36:31 -0600 (CST)
kilgota@banach.math.auburn.edu wrote:
> Just now when I logged in, a fortune came up which says:
> 
> "A little experience often upsets a lot of theory."
> 
> It struck me funny after our recent experiences, so I thought I would 
> share it with both of you.

Hello,

May be this message made me to look again at the gspca code. Well, it's
my fault: I did not check the previous patch. Sorry for all trouble.

The patch is simply:

diff -r 3f4a7bc53d8e linux/drivers/media/video/gspca/gspca.c
--- a/linux/drivers/media/video/gspca/gspca.c	Mon Feb 02 20:25:38 2009 +0100
+++ b/linux/drivers/media/video/gspca/gspca.c	Tue Feb 03 10:37:51 2009 +0100
@@ -435,7 +435,7 @@
 			break;
 
 		gspca_dev->urb[i] = NULL;
-		if (!gspca_dev->present)
+		if (gspca_dev->present)
 			usb_kill_urb(urb);
 		if (urb->transfer_buffer != NULL)
 			usb_buffer_free(gspca_dev->dev,


Best regards.

-- 
Ken ar c'hentan	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] Add support for sq905 based cameras to gspca

[kilgota]

On Tue, 3 Feb 2009, Jean-Francois Moine wrote:

> On Mon, 2 Feb 2009 20:36:31 -0600 (CST)
> kilgota@banach.math.auburn.edu wrote:
>> Just now when I logged in, a fortune came up which says:
>>
>> "A little experience often upsets a lot of theory."
>>
>> It struck me funny after our recent experiences, so I thought I would
>> share it with both of you.
>
> Hello,
>
> May be this message made me to look again at the gspca code. Well, it's
> my fault: I did not check the previous patch. Sorry for all trouble.
>
> The patch is simply:
>
> diff -r 3f4a7bc53d8e linux/drivers/media/video/gspca/gspca.c
> --- a/linux/drivers/media/video/gspca/gspca.c	Mon Feb 02 20:25:38 2009 +0100
> +++ b/linux/drivers/media/video/gspca/gspca.c	Tue Feb 03 10:37:51 2009 +0100
> @@ -435,7 +435,7 @@
> 			break;
>
> 		gspca_dev->urb[i] = NULL;
> -		if (!gspca_dev->present)
> +		if (gspca_dev->present)
> 			usb_kill_urb(urb);
> 		if (urb->transfer_buffer != NULL)
> 			usb_buffer_free(gspca_dev->dev,
>

Well, OK. But this will solve the problem which comes from disconnect 
while streaming? Sure, I will try this and see if it does the job or not. 
But whatever the outcome of that might be, I do not follow the logic.

Theodore Kilgore

Re: [PATCH] Add support for sq905 based cameras to gspca

[kilgota]

On Tue, 3 Feb 2009, kilgota@banach.math.auburn.edu wrote:

>
>
> On Tue, 3 Feb 2009, Jean-Francois Moine wrote:
>
>> On Mon, 2 Feb 2009 20:36:31 -0600 (CST)
>> kilgota@banach.math.auburn.edu wrote:
>>> Just now when I logged in, a fortune came up which says:
>>> 
>>> "A little experience often upsets a lot of theory."
>>> 
>>> It struck me funny after our recent experiences, so I thought I would
>>> share it with both of you.
>> 
>> Hello,
>> 
>> May be this message made me to look again at the gspca code. Well, it's
>> my fault: I did not check the previous patch. Sorry for all trouble.
>> 
>> The patch is simply:
>> 
>> diff -r 3f4a7bc53d8e linux/drivers/media/video/gspca/gspca.c
>> --- a/linux/drivers/media/video/gspca/gspca.c	Mon Feb 02 20:25:38 2009 
>> +0100
>> +++ b/linux/drivers/media/video/gspca/gspca.c	Tue Feb 03 10:37:51 2009 
>> +0100
>> @@ -435,7 +435,7 @@
>> 			break;
>>
>> 		gspca_dev->urb[i] = NULL;
>> -		if (!gspca_dev->present)
>> +		if (gspca_dev->present)
>> 			usb_kill_urb(urb);
>> 		if (urb->transfer_buffer != NULL)
>> 			usb_buffer_free(gspca_dev->dev,
>> 
>
> Well, OK. But this will solve the problem which comes from disconnect while 
> streaming? Sure, I will try this and see if it does the job or not. But 
> whatever the outcome of that might be, I do not follow the logic.
>
> Theodore Kilgore

I should add to the above that now I have tested, and indeed this change 
does not solve the problem of kernel oops after disconnect while 
streaming. It does make one change. The xterm does not go wild with error 
messages. But it is still not possible to close the svv window. Moreover, 
ps ax reveals that [svv] is running as an unkillable process, with 
[sq905/0] and [sq905/1], equally unkillable, in supporting roles. And 
dmesg reveals an oops. The problem is after all notorious by now, so I do 
not see much need for yet another log of debug output unless specifically 
asked for such.

Perhaps we also need to do what Adam suggested yesterday, and add a call 
to destroy_urbs() in gspca_disconnect()? That really did appear to cure 
the problem. But the way I did it is

void gspca_disconnect(struct usb_interface *intf)
{
         struct gspca_dev *gspca_dev = usb_get_intfdata(intf);

         gspca_dev->present = 0;
         destroy_urbs(gspca_dev);

         usb_set_intfdata(intf, NULL);

         /* release the device */
         /* (this will call gspca_release() immediatly or on last close) */
         video_unregister_device(&gspca_dev->vdev);

         PDEBUG(D_PROBE, "disconnect complete");
}

and it would appear that with your changes above it would work better to 
do here first

         destroy_urbs(gspca_dev);

and then only after that

         gspca_dev->present = 0;

Or?

Theodore Kilgore

Re: [PATCH] Add support for sq905 based cameras to gspca

[Jean-Francois Moine]

On Tue, 3 Feb 2009 12:21:55 -0600 (CST)
kilgota@banach.math.auburn.edu wrote:

> I should add to the above that now I have tested, and indeed this
> change does not solve the problem of kernel oops after disconnect
> while streaming. It does make one change. The xterm does not go wild
> with error messages. But it is still not possible to close the svv
> window. Moreover, ps ax reveals that [svv] is running as an
> unkillable process, with [sq905/0] and [sq905/1], equally unkillable,
> in supporting roles. And dmesg reveals an oops. The problem is after
> all notorious by now, so I do not see much need for yet another log
> of debug output unless specifically asked for such.

Why is there 2 sq905 processes?

What is the oops? (Your last trace was good, because it gave the last
gspca/sq905 messages and the full oops)

> Perhaps we also need to do what Adam suggested yesterday, and add a
> call to destroy_urbs() in gspca_disconnect()?

Surely not! The destroy_urbs() must be called at the right time, i.e.
on close().

-- 
Ken ar c'hentan	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] Add support for sq905 based cameras to gspca

[kilgota]

On Tue, 3 Feb 2009, Jean-Francois Moine wrote:

> On Tue, 3 Feb 2009 12:21:55 -0600 (CST)
> kilgota@banach.math.auburn.edu wrote:
>
>> I should add to the above that now I have tested, and indeed this
>> change does not solve the problem of kernel oops after disconnect
>> while streaming. It does make one change. The xterm does not go wild
>> with error messages. But it is still not possible to close the svv
>> window. Moreover, ps ax reveals that [svv] is running as an
>> unkillable process, with [sq905/0] and [sq905/1], equally unkillable,
>> in supporting roles. And dmesg reveals an oops. The problem is after
>> all notorious by now, so I do not see much need for yet another log
>> of debug output unless specifically asked for such.
>
> Why is there 2 sq905 processes?

I of course do not fully understand why there are two such processes. 
However, I would suspect that [sq905/0] is running on processor 0 and 
[sq905/1] is running on processor 1. As I remember, there is only one 
[sq905] process which runs on a single-core machine.

>
> What is the oops? (Your last trace was good, because it gave the last
> gspca/sq905 messages and the full oops)

Well, I can do it again, I suppose. So you get that in a few minutes. But 
my private speculation is it will look just about like the last one, 
because the problem is not addressed.


>
>> Perhaps we also need to do what Adam suggested yesterday, and add a
>> call to destroy_urbs() in gspca_disconnect()?
>
> Surely not! The destroy_urbs() must be called at the right time, i.e.
> on close().

Hmmm. well, the problem is that we are down there in a workqueue. We have 
to force the workqueue to close. I do not see a way to do that, even with 
the exit routines at the end of the workqueue, if it is not possible to 
call upon the functions there which will do the job (and it appears to me 
it is not thus possible, because those gspca functions are not public). 
And if we try to close the workqueue without doing something like 
destroy_urbs() as a consequence of a violent disconnect, then the 
workqueue just does not understand it is supposed to stop and merrily goes 
on its way in spite of all. Then it can not be closed because it is "busy" 
talking to a now-nonexistent piece of hardware, and the calling program 
can not be closed, either, because it is "busy" as well.

Unless I get interrupted by something extraneous, I should have a log sent 
along in a few minutes. God knows, it is easy enough to create one.

Theodore Kilgore

Re: [PATCH] Add support for sq905 based cameras to gspca

[Jean-Francois Moine]

On Tue, 3 Feb 2009 13:15:58 -0600 (CST)
kilgota@banach.math.auburn.edu wrote:

> > Why is there 2 sq905 processes?  
> 
> I of course do not fully understand why there are two such processes. 
> However, I would suspect that [sq905/0] is running on processor 0 and 
> [sq905/1] is running on processor 1. As I remember, there is only one 
> [sq905] process which runs on a single-core machine.

Indeed, the problem is there! You must have only one process reading the
webcam! I do not see how this can work with these 2 processes...

-- 
Ken ar c'hentan	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] Add support for sq905 based cameras to gspca

[kilgota]

On Tue, 3 Feb 2009, Jean-Francois Moine wrote:

> On Tue, 3 Feb 2009 13:15:58 -0600 (CST)
> kilgota@banach.math.auburn.edu wrote:
>
>>> Why is there 2 sq905 processes?
>>
>> I of course do not fully understand why there are two such processes.
>> However, I would suspect that [sq905/0] is running on processor 0 and
>> [sq905/1] is running on processor 1. As I remember, there is only one
>> [sq905] process which runs on a single-core machine.
>
> Indeed, the problem is there! You must have only one process reading the
> webcam! I do not see how this can work with these 2 processes...

The problem, then, would seem to me to boil down to the question of 
whether that is up to us. Apparently, a decision like that is not up to 
us, but rather it is up to the compiler and to the rest of the kernel to 
decide. Which, incidentally, appears to me to be a very logical way to 
arrange things. Presumably, a dual- or multi-core machine gives certain 
advantages, or it ought to, but it also requires certain accommodations.

You know, Jean-Francois, in a way it is a lucky accident that I got this 
machine for Christmas. I would never have fired up the Pentium 4, at least 
until sometime in the unforeseeable future, because in fact I was getting 
quite adequate performance out of the old Sempron box. Thus, I would not 
have been aware of this problem, either. We would have gone right ahead, 
Adam and I, blissfully ignorant, and published a module which has a flaw 
on a dual-core machine. So, in spite of the problems I say it is better to 
face the problems now.

Theodore Kilgore

Re: [PATCH] Add support for sq905 based cameras to gspca

[Jean-Francois Moine]

On Tue, 3 Feb 2009 13:54:15 -0600 (CST)
kilgota@banach.math.auburn.edu wrote:

> > Indeed, the problem is there! You must have only one process
> > reading the webcam! I do not see how this can work with these 2
> > processes...  
> 
> The problem, then, would seem to me to boil down to the question of 
> whether that is up to us. Apparently, a decision like that is not up
> to us, but rather it is up to the compiler and to the rest of the
> kernel to decide. Which, incidentally, appears to me to be a very
> logical way to arrange things. Presumably, a dual- or multi-core
> machine gives certain advantages, or it ought to, but it also
> requires certain accommodations.

Yes, a multiprocessor machine is a plus, but, you must run only one
process to handle streaming. If you have not seen it yet, this is done
changing the line 373 of sq905.c (if I have the same source as yours)
from:

	dev->work_thread = create_workqueue(MODULE_NAME);
to
	dev->work_thread = create_singlethread_workqueue(MODULE_NAME);

-- 
Ken ar c'hentan	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] Add support for sq905 based cameras to gspca

[Alan Stern]

On Tue, 3 Feb 2009 kilgota@banach.math.auburn.edu wrote:

> 
> 
> On Tue, 3 Feb 2009, Jean-Francois Moine wrote:
> 
> > On Tue, 3 Feb 2009 13:15:58 -0600 (CST)
> > kilgota@banach.math.auburn.edu wrote:
> >
> >>> Why is there 2 sq905 processes?
> >>
> >> I of course do not fully understand why there are two such processes.
> >> However, I would suspect that [sq905/0] is running on processor 0 and
> >> [sq905/1] is running on processor 1. As I remember, there is only one
> >> [sq905] process which runs on a single-core machine.
> >
> > Indeed, the problem is there! You must have only one process reading the
> > webcam! I do not see how this can work with these 2 processes...
> 
> The problem, then, would seem to me to boil down to the question of 
> whether that is up to us. Apparently, a decision like that is not up to 
> us, but rather it is up to the compiler and to the rest of the kernel to 
> decide.

Nonsense.  It's simply a matter of how you create your workqueue.  In 
the code you sent me, you call create_workqueue().  Instead, just call 
create_singlethread_workqueue().  Or maybe even 
create_freezeable_workqueue().

Alan Stern

Re: [PATCH] Add support for sq905 based cameras to gspca

[kilgota]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2497 bytes --]



On Tue, 3 Feb 2009, Alan Stern wrote:

> On Tue, 3 Feb 2009 kilgota@banach.math.auburn.edu wrote:
>
>>
>>
>> On Tue, 3 Feb 2009, Jean-Francois Moine wrote:
>>
>>> On Tue, 3 Feb 2009 13:15:58 -0600 (CST)
>>> kilgota@banach.math.auburn.edu wrote:
>>>
>>>>> Why is there 2 sq905 processes?
>>>>
>>>> I of course do not fully understand why there are two such processes.
>>>> However, I would suspect that [sq905/0] is running on processor 0 and
>>>> [sq905/1] is running on processor 1. As I remember, there is only one
>>>> [sq905] process which runs on a single-core machine.
>>>
>>> Indeed, the problem is there! You must have only one process reading the
>>> webcam! I do not see how this can work with these 2 processes...
>>
>> The problem, then, would seem to me to boil down to the question of
>> whether that is up to us. Apparently, a decision like that is not up to
>> us, but rather it is up to the compiler and to the rest of the kernel to
>> decide.
>
> Nonsense.  It's simply a matter of how you create your workqueue.  In
> the code you sent me, you call create_workqueue().  Instead, just call
> create_singlethread_workqueue().  Or maybe even
> create_freezeable_workqueue().
>
> Alan Stern
>

OK, seems one way out, might even work. I will definitely try that.

Update. I did try it.

No it does not work, sorry. :/

I changed the line
dev->work_thread = create_workqueue(MODULE_NAME);

to read

dev->work_thread = create_singlethread_workqueue(MODULE_NAME);

As a result, I can definitely report that only _two_ processes were frozen 
when the cord was pulled, named [svv] and [sq905].

I am sure that the attached log file of the oops looks a little bit 
different from the previous ones. It must, after all.

While you have this matter on your mind, I am curious about the following:

As the code for the sq905 module evolved through various stages, the 
only occasion on which any real trouble arose was at the end, when we put 
in the mutex locks which you can see in the code now. Before they were put 
in, these problems which we are discussing now did not occur. 
Specifically, there was not any such problem about an oops caused by 
camera removal until the mutex locks were put in the code. And I strongly 
suspect -- nay, I am almost certain -- that with that the same code you 
are looking at now, the oops would go away if all those mutex locks were 
simply commented out and the code re-compiled and reinstalled. Can you 
explain this? I am just curious about why.

[-- Attachment #2: Type: APPLICATION/octet-stream, Size: 1476 bytes --]

Re: [PATCH] Add support for sq905 based cameras to gspca

[Alan Stern]

On Tue, 3 Feb 2009 kilgota@banach.math.auburn.edu wrote:

> > Nonsense.  It's simply a matter of how you create your workqueue.  In
> > the code you sent me, you call create_workqueue().  Instead, just call
> > create_singlethread_workqueue().  Or maybe even
> > create_freezeable_workqueue().
> >
> > Alan Stern
> >
> 
> OK, seems one way out, might even work. I will definitely try that.
> 
> Update. I did try it.
> 
> No it does not work, sorry. :/

Again, nonsense.  Of course it works.  It causes the kernel to create
only one workqueue thread instead of two.  That's what it's supposed to
do -- it was never intended to fix your oops.

> While you have this matter on your mind, I am curious about the following:
> 
> As the code for the sq905 module evolved through various stages, the 
> only occasion on which any real trouble arose was at the end, when we put 
> in the mutex locks which you can see in the code now. Before they were put 
> in, these problems which we are discussing now did not occur. 
> Specifically, there was not any such problem about an oops caused by 
> camera removal until the mutex locks were put in the code. And I strongly 
> suspect -- nay, I am almost certain -- that with that the same code you 
> are looking at now, the oops would go away if all those mutex locks were 
> simply commented out and the code re-compiled and reinstalled. Can you 
> explain this? I am just curious about why.

You're wrong, the oops would not go away.  It would just become a lot 
less likely to occur -- and thereby all the more insidious.

Alan Stern

Re: [PATCH] Add support for sq905 based cameras to gspca

[kilgota]

On Tue, 3 Feb 2009, Alan Stern wrote:

> On Tue, 3 Feb 2009 kilgota@banach.math.auburn.edu wrote:
>
>>> Nonsense.  It's simply a matter of how you create your workqueue.  In
>>> the code you sent me, you call create_workqueue().  Instead, just call
>>> create_singlethread_workqueue().  Or maybe even
>>> create_freezeable_workqueue().
>>>
>>> Alan Stern
>>>
>>
>> OK, seems one way out, might even work. I will definitely try that.
>>
>> Update. I did try it.
>>
>> No it does not work, sorry. :/
>
> Again, nonsense.  Of course it works.  It causes the kernel to create
> only one workqueue thread instead of two.

OK, yes, it did do that.

That's what it's supposed to
> do -- it was never intended to fix your oops.

OK.

>
>> While you have this matter on your mind, I am curious about the following:
>>
>> As the code for the sq905 module evolved through various stages, the
>> only occasion on which any real trouble arose was at the end, when we put
>> in the mutex locks which you can see in the code now. Before they were put
>> in, these problems which we are discussing now did not occur.
>> Specifically, there was not any such problem about an oops caused by
>> camera removal until the mutex locks were put in the code. And I strongly
>> suspect -- nay, I am almost certain -- that with that the same code you
>> are looking at now, the oops would go away if all those mutex locks were
>> simply commented out and the code re-compiled and reinstalled. Can you
>> explain this? I am just curious about why.
>
> You're wrong, the oops would not go away.  It would just become a lot
> less likely to occur -- and thereby all the more insidious.

How nice. Thanks for explaining.

>
> Alan Stern
>

Re: [PATCH] Add support for sq905 based cameras to gspca

[Adam Baker]

On Tuesday 03 February 2009, Jean-Francois Moine wrote:
> Indeed, the problem is there! You must have only one process reading the
> webcam! I do not see how this can work with these 2 processes...

Although 2 processes are created only one ever gets used. 
create_singlethread_workqueue would therefore be less wasteful but make no 
other difference. Google searches for create_freezeable_workqueue bring up 
suggestions to avoid it so I think I will although I guess we ought to check 
at some point how well the camera copes with suspend while streaming.

At 19:53:31 Alan Stern wrote
> If a camera is unplugged while it is in use then 
> waiting until close() is no good -- it will cause destroy_urbs() to 
> pass a stale pointer to usb_buffer_free().  That's the reason for the 
> oops.

I'd go further and suggest that gspca_disconnect should, after calling 
destroy_urbs() do gspca_dev->dev = NULL; Any use of that pointer past that 
point is a bug (the downside is that doing so would have turned the current 
bug into a hard to spot memory leak).

At 03:37:35 Alan Stern wrote
> On Mon, 2 Feb 2009, Adam Baker wrote:
>> This shouldn't be a problem for sq905 (which doesn't use these URBs) or 
>> isochronous cameras (which don't need to resubmit URBs) but the finepix 
>> driver (the other supported bulk device) will need some careful
>> consideration  to avoid a race between killing the URB and resubmitting it.
>
> You shouldn't need to take any special action.  usb_kill_urb() solves 
> these races for you; it guarantees not to return until the URB has been 
> unlinked and the completion handler has finished, and it guarantees 
> that attempts by the completion handler to resubmit the URB will fail.

The sq905 driver doesn't use the URBs provided by gspca, it uses 
usb_control_msg and usb_bulk_msg which I presume do the right thing 
internally. There would be a tiny window in between when it checks the 
dev->streaming flag and when it sends a new USB msg for the disconnect to 
occur and invalidate the dev pointer. That could be fixed by holding 
gspca_dev->usb_lock in gspca_disconnect when it sets gspca_dev->present = 0.

That would also address the race between open and disconnect.

Unfortunately the finepix driver sometimes uses calls to schedule_delayed_work 
in the completion handler which then makes the call to usb_submit_urb. Fixing 
that will require someone with access to a suitable camera to test it 
otherwise there is a significant risk of adding deadlocks. It already suffers 
from this bug so we aren't making it worse.

Calling destroy_urbs from dev_close rather than gspca_disconnect could also 
trigger the same bug on isochronous cameras. I haven't looked at them closely 
but they probably also have the race between open and disconnect which taking 
the lock in disconnect is likely to fix.

I'll do some testing and post a patch if it looks promising.

Adam

Re: [PATCH] Add support for sq905 based cameras to gspca

[kilgota]

On Tue, 3 Feb 2009, Adam Baker wrote:

> On Tuesday 03 February 2009, Jean-Francois Moine wrote:
>> Indeed, the problem is there! You must have only one process reading the
>> webcam! I do not see how this can work with these 2 processes...
>
> Although 2 processes are created only one ever gets used.

How do you know that? Just curious.

> create_singlethread_workqueue would therefore be less wasteful but make no
> other difference.

In at least one respect, you seem to be right. The oops still occurs. See 
my previous mail.

Theodore Kilgore

Re: [PATCH] Add support for sq905 based cameras to gspca

[Alan Stern]

On Tue, 3 Feb 2009, Adam Baker wrote:

> The sq905 driver doesn't use the URBs provided by gspca, it uses 
> usb_control_msg and usb_bulk_msg which I presume do the right thing 
> internally. There would be a tiny window in between when it checks the 
> dev->streaming flag and when it sends a new USB msg for the disconnect to 
> occur and invalidate the dev pointer. That could be fixed by holding 
> gspca_dev->usb_lock in gspca_disconnect when it sets gspca_dev->present = 0.
> 
> That would also address the race between open and disconnect.
> 
> Unfortunately the finepix driver sometimes uses calls to schedule_delayed_work 
> in the completion handler which then makes the call to usb_submit_urb. Fixing 
> that will require someone with access to a suitable camera to test it 
> otherwise there is a significant risk of adding deadlocks. It already suffers 
> from this bug so we aren't making it worse.

If the driver submits URBs from a work routine then usb_kill_urb's
guarantees don't apply.  You'll need to synchronize all three routines:
disconnect, the completion handler, and the work routine.  That means 
you'll have to use a spinlock, since a completion handler isn't allowed 
to acquire a mutex.

Alan Stern

Re: [PATCH] Add support for sq905 based cameras to gspca

[Andy Walls]

On Tue, 2009-02-03 at 22:09 +0000, Adam Baker wrote:
> On Tuesday 03 February 2009, Jean-Francois Moine wrote:
> > Indeed, the problem is there! You must have only one process reading the
> > webcam! I do not see how this can work with these 2 processes...
> 
> Although 2 processes are created only one ever gets used. 
> create_singlethread_workqueue would therefore be less wasteful but make no 
> other difference. 

This is generally not the case.  There is a single workqueue as far as a
driver is concerned.  Work items submitted to the queue by the driver
are set to be processed by the same CPU submitting the work item (unless
you call queue_work_on() to specify the CPU).  However, there is no
guarantee that the same CPU will be submitting work requests to the
workqueue every time.

For most drivers this is a moot point though, because they only ever
instantiate and submit one work object ever per device.  This means the
workqueue depth never exceeds 1 for most drivers.  So the correct
statement would be, I believe, "only one sq905 worker thread ever gets
used (per device per capture) at a time in sq905.c"

(For the cx18 driver, where the workqueue can have up to 70 items queued
for all ongoing capture streams per device, it makes quite a difference
on whether a single thread or multiple threads process the work queue.
To preserve ordering of the work items, as needed for work orders for
moving video buffers from one place to another, a singlethreaded
workqueue had to be used.)


I did look at the patch as submitted on Jan 19, and do have what I
intend to be constructive criticisms (sorry if they're overcome by
events by now):

Creating and destroying the worker thread(s) at the start and stop of
each capture is a bit overkill.  It's akin to registering and
unregistering a device's interrupt handler before and after every
capture, but it's a bit worse overhead-wise.  It's probably better to
just instantiate the workqueue when the device "appears" (I'm not a USB
guy so insert appropraite term there) and destroy the workqueue and
worker threads(s) when the device is going to "disappear".  Or if it
will meet your performance requirements, create and destroy the
workqueue when you init and remove the module.  The workqueue and its
thread(s) are essentially the bottom half of your interrupt handler to
handle deferrable work - no point in killing them off until you really
don't need them anymore.

Also, you've created the workqueue threads with a non-unique name: the
expansion of MODULE_NAME.  You're basically saying that you only need
one workqueue, with it's per CPU thread(s), for all instantiations of an
sq905 device - which *can* be a valid design choice.  However, you're
bringing them up and tearing them down on a per capture basis.  That's a
problem that needs to be corrected if you intend to support multiple
sq905 devices on a single machine.  What happens when you attempt to
have two sq905 devices do simultaneous captures?  I don't know myself;
I've never tried to create 2 separate instantiations of a workqueue
object with the same name. 


Regards,
Andy

Re: [PATCH] Add support for sq905 based cameras to gspca

[Adam Baker]

On Wednesday 04 February 2009, Andy Walls wrote:
> On Tue, 2009-02-03 at 22:09 +0000, Adam Baker wrote:
> > On Tuesday 03 February 2009, Jean-Francois Moine wrote:
> > > Indeed, the problem is there! You must have only one process reading
> > > the webcam! I do not see how this can work with these 2 processes...
> >
> > Although 2 processes are created only one ever gets used.
> > create_singlethread_workqueue would therefore be less wasteful but make
> > no other difference.
>
> This is generally not the case.  There is a single workqueue as far as a
> driver is concerned.  Work items submitted to the queue by the driver
> are set to be processed by the same CPU submitting the work item (unless
> you call queue_work_on() to specify the CPU).  However, there is no
> guarantee that the same CPU will be submitting work requests to the
> workqueue every time.
>
> For most drivers this is a moot point though, because they only ever
> instantiate and submit one work object ever per device.  This means the
> workqueue depth never exceeds 1 for most drivers.  So the correct
> statement would be, I believe, "only one sq905 worker thread ever gets
> used (per device per capture) at a time in sq905.c"

Yes, I did mean only one gets used in the case of sq905.c (because the queue 
is created per capture and only one item gets submitted to it).

<snip>
>
> I did look at the patch as submitted on Jan 19, and do have what I
> intend to be constructive criticisms (sorry if they're overcome by
> events by now):
>
> Creating and destroying the worker thread(s) at the start and stop of
> each capture is a bit overkill.  It's akin to registering and
> unregistering a device's interrupt handler before and after every
> capture, but it's a bit worse overhead-wise.  It's probably better to
> just instantiate the workqueue when the device "appears" (I'm not a USB
> guy so insert appropraite term there) and destroy the workqueue and
> worker threads(s) when the device is going to "disappear".  Or if it
> will meet your performance requirements, create and destroy the
> workqueue when you init and remove the module.  The workqueue and its
> thread(s) are essentially the bottom half of your interrupt handler to
> handle deferrable work - no point in killing them off until you really
> don't need them anymore.
>

My thought was that the camera was likely to remain plugged in even if it 
wasn't being used so it was best to clean up as much as possible when it 
wasn't in use. I don't really know how the overheads of creating a workqueue 
when you do need it compares to leaving an unused one around sitting on the 
not ready queue in the process table but starting a capture is going to take 
many ms just for the USB traffic so a little extra overhead doesn't seem too 
worrying.

> Also, you've created the workqueue threads with a non-unique name: the
> expansion of MODULE_NAME.  You're basically saying that you only need
> one workqueue, with it's per CPU thread(s), for all instantiations of an
> sq905 device - which *can* be a valid design choice.  However, you're
> bringing them up and tearing them down on a per capture basis.  That's a
> problem that needs to be corrected if you intend to support multiple
> sq905 devices on a single machine.  What happens when you attempt to
> have two sq905 devices do simultaneous captures?  I don't know myself;
> I've never tried to create 2 separate instantiations of a workqueue
> object with the same name.
>

I see multiple instance of [pdflush] and [nfsd] which seem to work fine so I 
believe the name doesn't need to be unique, just a guide to the user of what 
is eating CPU time. I don't have 2 sq905 cameras to test it but I have had 
left over workqueues caused by a driver bug stopping it shut down and new 
ones that started also worked fine.

>
> Regards,
> Andy

Re: [PATCH] Add support for sq905 based cameras to gspca

[kilgota]

On Wed, 4 Feb 2009, Adam Baker wrote:

> On Wednesday 04 February 2009, Andy Walls wrote:
>> On Tue, 2009-02-03 at 22:09 +0000, Adam Baker wrote:
>>> On Tuesday 03 February 2009, Jean-Francois Moine wrote:
>>>> Indeed, the problem is there! You must have only one process reading
>>>> the webcam! I do not see how this can work with these 2 processes...
>>>
>>> Although 2 processes are created only one ever gets used.
>>> create_singlethread_workqueue would therefore be less wasteful but make
>>> no other difference.
>>
>> This is generally not the case.  There is a single workqueue as far as a
>> driver is concerned.  Work items submitted to the queue by the driver
>> are set to be processed by the same CPU submitting the work item (unless
>> you call queue_work_on() to specify the CPU).  However, there is no
>> guarantee that the same CPU will be submitting work requests to the
>> workqueue every time.
>>
>> For most drivers this is a moot point though, because they only ever
>> instantiate and submit one work object ever per device.  This means the
>> workqueue depth never exceeds 1 for most drivers.  So the correct
>> statement would be, I believe, "only one sq905 worker thread ever gets
>> used (per device per capture) at a time in sq905.c"
>
> Yes, I did mean only one gets used in the case of sq905.c (because the queue
> is created per capture and only one item gets submitted to it).
>
> <snip>
>>
>> I did look at the patch as submitted on Jan 19, and do have what I
>> intend to be constructive criticisms (sorry if they're overcome by
>> events by now):
>>
>> Creating and destroying the worker thread(s) at the start and stop of
>> each capture is a bit overkill.  It's akin to registering and
>> unregistering a device's interrupt handler before and after every
>> capture, but it's a bit worse overhead-wise.  It's probably better to
>> just instantiate the workqueue when the device "appears" (I'm not a USB
>> guy so insert appropraite term there) and destroy the workqueue and
>> worker threads(s) when the device is going to "disappear".  Or if it
>> will meet your performance requirements, create and destroy the
>> workqueue when you init and remove the module.  The workqueue and its
>> thread(s) are essentially the bottom half of your interrupt handler to
>> handle deferrable work - no point in killing them off until you really
>> don't need them anymore.
>>
>
> My thought was that the camera was likely to remain plugged in even if it
> wasn't being used so it was best to clean up as much as possible when it
> wasn't in use. I don't really know how the overheads of creating a workqueue
> when you do need it compares to leaving an unused one around sitting on the
> not ready queue in the process table but starting a capture is going to take
> many ms just for the USB traffic so a little extra overhead doesn't seem too
> worrying.
>
>> Also, you've created the workqueue threads with a non-unique name: the
>> expansion of MODULE_NAME.  You're basically saying that you only need
>> one workqueue, with it's per CPU thread(s), for all instantiations of an
>> sq905 device - which *can* be a valid design choice.  However, you're
>> bringing them up and tearing them down on a per capture basis.  That's a
>> problem that needs to be corrected if you intend to support multiple
>> sq905 devices on a single machine.  What happens when you attempt to
>> have two sq905 devices do simultaneous captures?  I don't know myself;
>> I've never tried to create 2 separate instantiations of a workqueue
>> object with the same name.
>>
>
> I see multiple instance of [pdflush] and [nfsd] which seem to work fine so I
> believe the name doesn't need to be unique, just a guide to the user of what
> is eating CPU time. I don't have 2 sq905 cameras to test it ...

Well, I do. Here is what happens:

1. Plugged in one of them, started the streaming.

2. Plugged in the second one. First one continues to stream, no problem. 
Attempt to start a stream from the second camera while the first one is 
streaming results in the error message,

VIDIOC_REQBUFS error 16, Device or resource busy

and the command line reappears. No obvious interference with the stream 
from the first camera is apparent.

3. After the stream from the first camera is closed, the streaming can be 
started again. However, the stream is again from the first camera which 
was plugged in.

4. After removing the first camera which was plugged in, I tried to start 
the stream from the second one. The stream will not start. A message says 
that

Cannot identify 'dev/video0': 2. No such file or directory.

5. Second camera left attached, first camera plugged in again. The first 
camera can stream again. The second one can not.

6. When the first camera is removed and the second one is then re-plugged, 
the second one will stream.

So, we can safely conclude that, as the module is presently constituted, 
there can be only one stream at a time. The second camera plugged in 
cannot stream. Neither can it conflict with the first one. The only thing 
which puzzles me a little bit is item 5. Apparently, the question is about 
which port the camera was connected to. So let's try three cameras. I will 
hook up camera number one, get a stream, close it, hook up number two 
through a different port, then remove number one and replace it with 
number three on the same port where number one was. What I suspected would 
happen is what happened. Number two would not stream. Number three would 
stream just as number one did. In other words, this was determined by 
which port was in use for number one, which is now also in use for number 
three.

So, now we know what happens.

Theodore Kilgore

Re: [PATCH] Add support for sq905 based cameras to gspca

[Adam Baker]

On Wednesday 04 February 2009, kilgota@banach.math.auburn.edu wrote:
<snip description of attempting to stream from 2 cameras at once>
> 4. After removing the first camera which was plugged in, I tried to start
> the stream from the second one. The stream will not start. A message says
> that
>
> Cannot identify 'dev/video0': 2. No such file or directory.

This line points to an error in your test method.

You need to start the second stream with svv -d /dev/video1 to tell it to pick 
the second camera.

Adam

Re: [PATCH] Add support for sq905 based cameras to gspca

[kilgota]

On Wed, 4 Feb 2009, Adam Baker wrote:

> On Wednesday 04 February 2009, kilgota@banach.math.auburn.edu wrote:
> <snip description of attempting to stream from 2 cameras at once>
>> 4. After removing the first camera which was plugged in, I tried to start
>> the stream from the second one. The stream will not start. A message says
>> that
>>
>> Cannot identify 'dev/video0': 2. No such file or directory.
>
> This line points to an error in your test method.
>
> You need to start the second stream with svv -d /dev/video1 to tell it to pick
> the second camera.
>
> Adam
>

Oops, right.

Well, in that case I have to report that two cameras work 
simultaneously just fine. No problem at all.

Theodore Kilgore

Re: [PATCH] Add support for sq905 based cameras to gspca

[kilgota]

On Wed, 4 Feb 2009, kilgota@banach.math.auburn.edu wrote:

>
>
> On Wed, 4 Feb 2009, Adam Baker wrote:
>
>> On Wednesday 04 February 2009, kilgota@banach.math.auburn.edu wrote:
>> <snip description of attempting to stream from 2 cameras at once>
>>> 4. After removing the first camera which was plugged in, I tried to start
>>> the stream from the second one. The stream will not start. A message says
>>> that
>>> 
>>> Cannot identify 'dev/video0': 2. No such file or directory.
>> 
>> This line points to an error in your test method.
>> 
>> You need to start the second stream with svv -d /dev/video1 to tell it to 
>> pick
>> the second camera.
>> 
>> Adam
>> 
>
> Oops, right.
>
> Well, in that case I have to report that two cameras work simultaneously just 
> fine. No problem at all.

For completeness, I should add the following:

ps ax now shows two svv processes. One of them is svv -d /dev/video1 and 
is followed by [sq905] and the second one is svv followed by [sq905]. Why 
in reverse order? Well, probably it is that I now fired up the second 
camera first and the first one after that. The workqueue in the 
module is started with

dev->work_thread = create_singlethread_workqueue(MODULE_NAME);

so these are distinct instances of [sq905].

Now am I supposed to check what happens if I start jerking them off of 
their tethers without closing the capture windows? No thanks, not this 
time. Let's wait until that problem is solved with just one camera. :)


Theodore Kilgore

Re: [PATCH] Add support for sq905 based cameras to gspca

[kilgota]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1245 bytes --]



On Tue, 3 Feb 2009, Jean-Francois Moine wrote:

> On Tue, 3 Feb 2009 12:21:55 -0600 (CST)
> kilgota@banach.math.auburn.edu wrote:
>
>> I should add to the above that now I have tested, and indeed this
>> change does not solve the problem of kernel oops after disconnect
>> while streaming. It does make one change. The xterm does not go wild
>> with error messages. But it is still not possible to close the svv
>> window. Moreover, ps ax reveals that [svv] is running as an
>> unkillable process, with [sq905/0] and [sq905/1], equally unkillable,
>> in supporting roles. And dmesg reveals an oops. The problem is after
>> all notorious by now, so I do not see much need for yet another log
>> of debug output unless specifically asked for such.
>
> Why is there 2 sq905 processes?
>
> What is the oops? (Your last trace was good, because it gave the last
> gspca/sq905 messages and the full oops)

Right, here is the output. I have not searched for precise differences, 
but a glance at it leaves me with the feeling that it is the same old same 
old. This was done on the Pentium 4 Dual Core, with gspca_main loaded with 
option debug=255, and this is the dmesg output which resulted when I 
pulled the cord of the camera.


Theodore Kilgore

[-- Attachment #2: Type: APPLICATION/OCTET-STREAM, Size: 1466 bytes --]

Re: [PATCH] Add support for sq905 based cameras to gspca

[Alan Stern]

On Tue, 3 Feb 2009, Jean-Francois Moine wrote:

> > Perhaps we also need to do what Adam suggested yesterday, and add a
> > call to destroy_urbs() in gspca_disconnect()?
> 
> Surely not! The destroy_urbs() must be called at the right time, i.e.
> on close().

Theodore was pointing out that destroy_urbs() must also be called 
during disconnect.  If a camera is unplugged while it is in use then 
waiting until close() is no good -- it will cause destroy_urbs() to 
pass a stale pointer to usb_buffer_free().  That's the reason for the 
oops.

Alan Stern