gspca_kinect: enable both video and depth streams

PATCH In review Ulrik de Muelenaere 2 participants 5 messages Mar 6, 2016 – Mar 7, 2016

Subsystems (2)

Media (V4L/DVB) Gspca USB webcam driver

lore

Share & tools

lore

Ulrik de Muelenaere

author

2016-03-06 13:50 UTC #0 lore raw

cover gspca_kinect: enable both video and depth streams

Hello,

The Kinect produces both a video and depth stream, but the current implementation of the gspca_kinect subdriver requires a depth_mode parameter at probe time to select one of the streams which will be exposed as a v4l device. This patchset allows both streams to be accessed as separate video nodes.

A possible solution which has been discussed in the past is to call gspca_dev_probe() multiple times in order to create multiple video nodes. See the following mails:

http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/26194/focus=26213
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/76715/focus=78344

In the second mail linked above, it was mentioned that gspca_dev_probe() cannot be called multiple times for the same USB interface, since gspca_dev_probe2() stores a pointer to the new gspca_dev as the interface's private data. This is required for gspca_disconnect(), gspca_suspend() and gspca_resume(). As far as I can tell, this is the only reason gspca_dev_probe() cannot be called multiple times.

The first patch fixes this by storing other devices linked to the same interface as a linked list. The second patch then calls gspca_dev_probe() twice in the gspca_kinect subdriver, to create a video node for each stream.

Some notes on error handling, which I think should be reviewed:

* I could not test the gspca_suspend() and gspca_resume() functions. From my evaluation of the code, it seems that the only effect of returning an error code from gspca_resume() is that a message is logged. Therefore I decided to attempt to resume each gspca_dev when the interface is resumed, even if one fails. Bitwise OR seems like the best way to combine potentially multiple error codes.

* In the gspca_kinect subdriver, if the second call to gspca_dev_probe() fails, the first video node will still be available. I handle this case by calling gspca_disconnect(), which worked when I was testing, but I am not sure if this is the correct way to handle it.

Regards,
Ulrik

Ulrik de Muelenaere (2):
[media] gspca: allow multiple probes per USB interface [media] gspca_kinect: enable both video and depth streams

 drivers/media/usb/gspca/gspca.c  | 129 +++++++++++++++++++++++----------------
 drivers/media/usb/gspca/gspca.h  |   1 +
 drivers/media/usb/gspca/kinect.c |  28 +++++----
 3 files changed, 91 insertions(+), 67 deletions(-)

Signature · 1 line

2.7.0

Ulrik de Muelenaere

author

2016-03-06 13:51 UTC #1 lore raw

PATCH 1/2 gspca: allow multiple probes per USB interface

Allow gspca_dev_probe() to be called multiple times per USB interface, resulting in multiple video device nodes.

A pointer to the created gspca_dev is stored as the interface's private data. Store other devices linked to the same interface as a linked list, allowing all devices to be disconnected, suspended or resumed when given the interface.

This is useful for subdrivers such as gspca_kinect, where a single USB interface produces both video and depth streams.

Signed-off-by Ulrik de Muelenaere <[email protected]>

---
 drivers/media/usb/gspca/gspca.c | 129 +++++++++++++++++++++++-----------------
 drivers/media/usb/gspca/gspca.h |   1 +
 2 files changed, 76 insertions(+), 54 deletions(-)

drivers/media/usb/gspca/gspca.c +75 ~~-54~~

index af5cd82..d002b8b 100644
@@ -2004,8 +2004,9 @@ static const struct video_device gspca_template = {
 /*
  * probe and create a new gspca device
  *
- * This function must be called by the sub-driver when it is
- * called for probing a new device.
+ * This function must be called by the sub-driver when it is called for probing
+ * a new device. It may be called multiple times per USB interface, resulting in
+ * multiple video device nodes.
  */
 int gspca_dev_probe2(struct usb_interface *intf,
 		const struct usb_device_id *id,

@@ -2037,6 +2038,7 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	gspca_dev->dev = dev;
 	gspca_dev->iface = intf->cur_altsetting->desc.bInterfaceNumber;
 	gspca_dev->xfer_ep = -1;
+	gspca_dev->next_dev = usb_get_intfdata(intf);
 
 	/* check if any audio device */
 	if (dev->actconfig->desc.bNumInterfaces != 1) {

@@ -2169,41 +2171,50 @@ EXPORT_SYMBOL(gspca_dev_probe);
  */
 void gspca_disconnect(struct usb_interface *intf)
 {
-	struct gspca_dev *gspca_dev = usb_get_intfdata(intf);
+	struct gspca_dev *gspca_dev = usb_get_intfdata(intf), *next_dev;
 #if IS_ENABLED(CONFIG_INPUT)
 	struct input_dev *input_dev;
 #endif
 
-	PDEBUG(D_PROBE, "%s disconnect",
-		video_device_node_name(&gspca_dev->vdev));
+	while (gspca_dev) {
+		PDEBUG(D_PROBE, "%s disconnect",
+			video_device_node_name(&gspca_dev->vdev));
 
-	mutex_lock(&gspca_dev->usb_lock);
+		mutex_lock(&gspca_dev->usb_lock);
 
-	gspca_dev->present = 0;
-	destroy_urbs(gspca_dev);
+		gspca_dev->present = 0;
+		destroy_urbs(gspca_dev);
 
 #if IS_ENABLED(CONFIG_INPUT)
-	gspca_input_destroy_urb(gspca_dev);
-	input_dev = gspca_dev->input_dev;
-	if (input_dev) {
-		gspca_dev->input_dev = NULL;
-		input_unregister_device(input_dev);
-	}
+		gspca_input_destroy_urb(gspca_dev);
+		input_dev = gspca_dev->input_dev;
+		if (input_dev) {
+			gspca_dev->input_dev = NULL;
+			input_unregister_device(input_dev);
+		}
 #endif
-	/* Free subdriver's streaming resources / stop sd workqueue(s) */
-	if (gspca_dev->sd_desc->stop0 && gspca_dev->streaming)
-		gspca_dev->sd_desc->stop0(gspca_dev);
-	gspca_dev->streaming = 0;
-	gspca_dev->dev = NULL;
-	wake_up_interruptible(&gspca_dev->wq);
+		/* Free subdriver's streaming resources / stop sd
+		 * workqueue(s)
+		 */
+		if (gspca_dev->sd_desc->stop0 && gspca_dev->streaming)
+			gspca_dev->sd_desc->stop0(gspca_dev);
+		gspca_dev->streaming = 0;
+		gspca_dev->dev = NULL;
+		wake_up_interruptible(&gspca_dev->wq);
 
-	v4l2_device_disconnect(&gspca_dev->v4l2_dev);
-	video_unregister_device(&gspca_dev->vdev);
+		v4l2_device_disconnect(&gspca_dev->v4l2_dev);
+		video_unregister_device(&gspca_dev->vdev);
 
-	mutex_unlock(&gspca_dev->usb_lock);
+		mutex_unlock(&gspca_dev->usb_lock);
 
-	/* (this will call gspca_release() immediately or on last close) */
-	v4l2_device_put(&gspca_dev->v4l2_dev);
+		next_dev = gspca_dev->next_dev;
+		/* (this will call gspca_release() immediately or on last
+		 * close)
+		 */
+		v4l2_device_put(&gspca_dev->v4l2_dev);
+
+		gspca_dev = next_dev;
+	}
 }
 EXPORT_SYMBOL(gspca_disconnect);

@@ -2212,21 +2223,27 @@ int gspca_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct gspca_dev *gspca_dev = usb_get_intfdata(intf);
 
-	gspca_input_destroy_urb(gspca_dev);
+	while (gspca_dev) {
+		gspca_input_destroy_urb(gspca_dev);
 
-	if (!gspca_dev->streaming)
-		return 0;
+		if (!gspca_dev->streaming) {
+			gspca_dev = gspca_dev->next_dev;
+			continue;
+		}
 
-	mutex_lock(&gspca_dev->usb_lock);
-	gspca_dev->frozen = 1;		/* avoid urb error messages */
-	gspca_dev->usb_err = 0;
-	if (gspca_dev->sd_desc->stopN)
-		gspca_dev->sd_desc->stopN(gspca_dev);
-	destroy_urbs(gspca_dev);
-	gspca_set_alt0(gspca_dev);
-	if (gspca_dev->sd_desc->stop0)
-		gspca_dev->sd_desc->stop0(gspca_dev);
-	mutex_unlock(&gspca_dev->usb_lock);
+		mutex_lock(&gspca_dev->usb_lock);
+		gspca_dev->frozen = 1;		/* avoid urb error messages */
+		gspca_dev->usb_err = 0;
+		if (gspca_dev->sd_desc->stopN)
+			gspca_dev->sd_desc->stopN(gspca_dev);
+		destroy_urbs(gspca_dev);
+		gspca_set_alt0(gspca_dev);
+		if (gspca_dev->sd_desc->stop0)
+			gspca_dev->sd_desc->stop0(gspca_dev);
+		mutex_unlock(&gspca_dev->usb_lock);
+
+		gspca_dev = gspca_dev->next_dev;
+	}
 
 	return 0;
 }

@@ -2237,22 +2254,26 @@ int gspca_resume(struct usb_interface *intf)
 	struct gspca_dev *gspca_dev = usb_get_intfdata(intf);
 	int streaming, ret = 0;
 
-	mutex_lock(&gspca_dev->usb_lock);
-	gspca_dev->frozen = 0;
-	gspca_dev->usb_err = 0;
-	gspca_dev->sd_desc->init(gspca_dev);
-	/*
-	 * Most subdrivers send all ctrl values on sd_start and thus
-	 * only write to the device registers on s_ctrl when streaming ->
-	 * Clear streaming to avoid setting all ctrls twice.
-	 */
-	streaming = gspca_dev->streaming;
-	gspca_dev->streaming = 0;
-	if (streaming)
-		ret = gspca_init_transfer(gspca_dev);
-	else
-		gspca_input_create_urb(gspca_dev);
-	mutex_unlock(&gspca_dev->usb_lock);
+	while (gspca_dev) {
+		mutex_lock(&gspca_dev->usb_lock);
+		gspca_dev->frozen = 0;
+		gspca_dev->usb_err = 0;
+		gspca_dev->sd_desc->init(gspca_dev);
+		/*
+		 * Most subdrivers send all ctrl values on sd_start and thus
+		 * only write to the device registers on s_ctrl when streaming -
+		 * Clear streaming to avoid setting all ctrls twice.
+		 */
+		streaming = gspca_dev->streaming;
+		gspca_dev->streaming = 0;
+		if (streaming)
+			ret |= gspca_init_transfer(gspca_dev);
+		else
+			gspca_input_create_urb(gspca_dev);
+		mutex_unlock(&gspca_dev->usb_lock);
+
+		gspca_dev = gspca_dev->next_dev;
+	}
 
 	return ret;
 }

drivers/media/usb/gspca/gspca.h +1 -0

index d39adf9..dcafc89 100644
@@ -207,6 +207,7 @@ struct gspca_dev {
 	__u8 alt;			/* USB alternate setting */
 	int xfer_ep;			/* USB transfer endpoint address */
 	u8 audio;			/* presence of audio device */
+	struct gspca_dev *next_dev;	/* next device sharing USB interface */
 
 	/* (*) These variables are proteced by both usb_lock and queue_lock,
 	   that is any code setting them is holding *both*, which means that

Signature · 1 line

2.7.0

Ulrik de Muelenaere

author

2016-03-06 13:51 UTC #2 lore raw

PATCH 2/2 gspca_kinect: enable both video and depth streams

In reply to Ulrik de Muelenaere

The Kinect produces both a video stream and a depth stream. Call gspca_dev_probe() twice to create a video device node for each.

Remove the depth_mode parameter which had to be set at probe time in order to select either the video or depth stream.

Signed-off-by Ulrik de Muelenaere <[email protected]>

---
 drivers/media/usb/gspca/kinect.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

drivers/media/usb/gspca/kinect.c +15 ~~-13~~

index 3cb30a3..4bc5b7d 100644
@@ -36,8 +36,6 @@ MODULE_AUTHOR("Antonio Ospite <[email protected]>");
 MODULE_DESCRIPTION("GSPCA/Kinect Sensor Device USB Camera Driver");
 MODULE_LICENSE("GPL");
 
-static bool depth_mode;
-
 struct pkt_hdr {
 	uint8_t magic[2];
 	uint8_t pad;

@@ -424,7 +422,7 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev, u8 *__data, int len)
 
 /* sub-driver description */
 static const struct sd_desc sd_desc_video = {
-	.name      = MODULE_NAME,
+	.name      = MODULE_NAME "_video",
 	.config    = sd_config_video,
 	.init      = sd_init,
 	.start     = sd_start_video,

@@ -436,7 +434,7 @@ static const struct sd_desc sd_desc_video = {
 	*/
 };
 static const struct sd_desc sd_desc_depth = {
-	.name      = MODULE_NAME,
+	.name      = MODULE_NAME "_depth",
 	.config    = sd_config_depth,
 	.init      = sd_init,
 	.start     = sd_start_depth,

@@ -460,12 +458,19 @@ MODULE_DEVICE_TABLE(usb, device_table);
 /* -- device connect -- */
 static int sd_probe(struct usb_interface *intf, const struct usb_device_id *id)
 {
-	if (depth_mode)
-		return gspca_dev_probe(intf, id, &sd_desc_depth,
-				       sizeof(struct sd), THIS_MODULE);
-	else
-		return gspca_dev_probe(intf, id, &sd_desc_video,
-				       sizeof(struct sd), THIS_MODULE);
+	int res;
+
+	res = gspca_dev_probe(intf, id, &sd_desc_video, sizeof(struct sd),
+			      THIS_MODULE);
+	if (res < 0)
+		return res;
+
+	res = gspca_dev_probe(intf, id, &sd_desc_depth, sizeof(struct sd),
+			      THIS_MODULE);
+	if (res < 0)
+		gspca_disconnect(intf);
+
+	return res;
 }
 
 static struct usb_driver sd_driver = {

@@ -481,6 +486,3 @@ static struct usb_driver sd_driver = {
 };
 
 module_usb_driver(sd_driver);
-
-module_param(depth_mode, bool, 0644);
-MODULE_PARM_DESC(depth_mode, "0=video 1=depth");

Signature · 1 line

2.7.0

Hans de Goede

2016-03-07 19:00 UTC #3 lore raw

In reply to Ulrik de Muelenaere

Hi Ulrik,

Quoted 31 lines · Ulrik de Muelenaere

Hello,
The Kinect produces both a video and depth stream, but the current implementation of the gspca_kinect subdriver requires a depth_mode parameter at probe time to select one of the streams which will be exposed as a v4l device. This patchset allows both streams to be accessed as separate video nodes.
A possible solution which has been discussed in the past is to call gspca_dev_probe() multiple times in order to create multiple video nodes. See the following mails:
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/26194/focus=26213
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/76715/focus=78344
In the second mail linked above, it was mentioned that gspca_dev_probe() cannot be called multiple times for the same USB interface, since gspca_dev_probe2() stores a pointer to the new gspca_dev as the interface's private data. This is required for gspca_disconnect(), gspca_suspend() and gspca_resume(). As far as I can tell, this is the only reason gspca_dev_probe() cannot be called multiple times.
The first patch fixes this by storing other devices linked to the same interface as a linked list. The second patch then calls gspca_dev_probe() twice in the gspca_kinect subdriver, to create a video node for each stream.
Some notes on error handling, which I think should be reviewed:
* I could not test the gspca_suspend() and gspca_resume() functions. From my evaluation of the code, it seems that the only effect of returning an error code from gspca_resume() is that a message is logged. Therefore I decided to attempt to resume each gspca_dev when the interface is resumed, even if one fails. Bitwise OR seems like the best way to combine potentially multiple error codes.

Quoted 4 lines

* In the gspca_kinect subdriver, if the second call to gspca_dev_probe() fails, the first video node will still be available. I handle this case by calling gspca_disconnect(), which worked when I was testing, but I am not sure if this is the correct way to handle it.

Thanks for the patch-set overall I like it, one thing which worries is me is that sd_start_video and sd_start_depth may race, which is esp. problematic taking into account that both start/stop the depth stream (see the comment about this in sd_start_video()) this will require some coordination, so you like need to do something a bit more advanced and create a shared data struct somewhere for coordination (and with a lock in it).

Likewise the v4l2 core is designed to have only one struct v4l2_device per struct device, so you need to modify probe2 to not call v4l2_device_register when it is called a second time for the same intf, and to make gspca_dev->vdev.v4l2_dev point to the v4l2_dev of the first gspca_dev registered.

You will also need some changes for this in gspca_disconnect, as you will need to do all the calls on &gspca_dev->v4l2_dev only for the first registered device there, and you will need to do all the calls in gspca_release except for the v4l2_device_unregister() in a loop like you're using in disconnect.

Note since you need a shared data struct anyways it might be easier to just use that (add some shared pointer to struct gspca_dev) for the array of gspca_devs rather then using the linked list, as for disconnect / release you will want to use the first registered gspca_dev to get the v4l2_dev address, and your linked approach gives you the last.

Regards,

Hans

Quoted 13 lines

Regards,
Ulrik
Ulrik de Muelenaere (2):
[media] gspca: allow multiple probes per USB interface [media] gspca_kinect: enable both video and depth streams
  drivers/media/usb/gspca/gspca.c  | 129 +++++++++++++++++++++++----------------
  drivers/media/usb/gspca/gspca.h  |   1 +
  drivers/media/usb/gspca/kinect.c |  28 +++++----
  3 files changed, 91 insertions(+), 67 deletions(-)

Ulrik de Muelenaere

author

2016-03-07 19:23 UTC #4 lore raw

Hi Hans,

Quoted 58 lines · Hans de Goede

Hi Ulrik,
Quoted 31 lines · Ulrik de Muelenaere
Hello,
The Kinect produces both a video and depth stream, but the current implementation of the gspca_kinect subdriver requires a depth_mode parameter at probe time to select one of the streams which will be exposed as a v4l device. This patchset allows both streams to be accessed as separate video nodes.
A possible solution which has been discussed in the past is to call gspca_dev_probe() multiple times in order to create multiple video nodes. See the following mails:
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/26194/focus=26213
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/76715/focus=78344
In the second mail linked above, it was mentioned that gspca_dev_probe() cannot be called multiple times for the same USB interface, since gspca_dev_probe2() stores a pointer to the new gspca_dev as the interface's private data. This is required for gspca_disconnect(), gspca_suspend() and gspca_resume(). As far as I can tell, this is the only reason gspca_dev_probe() cannot be called multiple times.
The first patch fixes this by storing other devices linked to the same interface as a linked list. The second patch then calls gspca_dev_probe() twice in the gspca_kinect subdriver, to create a video node for each stream.
Some notes on error handling, which I think should be reviewed:
* I could not test the gspca_suspend() and gspca_resume() functions. From my evaluation of the code, it seems that the only effect of returning an error code from gspca_resume() is that a message is logged. Therefore I decided to attempt to resume each gspca_dev when the interface is resumed, even if one fails. Bitwise OR seems like the best way to combine potentially multiple error codes.
Quoted 4 lines
* In the gspca_kinect subdriver, if the second call to gspca_dev_probe() fails, the first video node will still be available. I handle this case by calling gspca_disconnect(), which worked when I was testing, but I am not sure if this is the correct way to handle it.
Thanks for the patch-set overall I like it, one thing which worries is me is that sd_start_video and sd_start_depth may race, which is esp. problematic taking into account that both start/stop the depth stream (see the comment about this in sd_start_video()) this will require some coordination, so you like need to do something a bit more advanced and create a shared data struct somewhere for coordination (and with a lock in it).
Likewise the v4l2 core is designed to have only one struct v4l2_device per struct device, so you need to modify probe2 to not call v4l2_device_register when it is called a second time for the same intf, and to make gspca_dev->vdev.v4l2_dev point to the v4l2_dev of the first gspca_dev registered.
You will also need some changes for this in gspca_disconnect, as you will need to do all the calls on &gspca_dev->v4l2_dev only for the first registered device there, and you will need to do all the calls in gspca_release except for the v4l2_device_unregister() in a loop like you're using in disconnect.

Quoted 5 lines

Note since you need a shared data struct anyways it might be easier to just use that (add some shared pointer to struct gspca_dev) for the array of gspca_devs rather then using the linked list, as for disconnect / release you will want to use the first registered gspca_dev to get the v4l2_dev address, and your linked approach gives you the last.

Thanks for the input. I'll start working on your suggestions.

Regards,
Ulrik

Quoted 19 lines

Regards,
Hans
Quoted 13 lines
Regards,
Ulrik
Ulrik de Muelenaere (2):
[media] gspca: allow multiple probes per USB interface [media] gspca_kinect: enable both video and depth streams
 drivers/media/usb/gspca/gspca.c  | 129 +++++++++++++++++++++++----------------
 drivers/media/usb/gspca/gspca.h  |   1 +
 drivers/media/usb/gspca/kinect.c |  28 +++++----
 3 files changed, 91 insertions(+), 67 deletions(-)