[PATCH 0/2] [media] gspca_kinect: enable both video and depth streams

[PATCH 0/2] [media] gspca_kinect: enable both video and depth streams

[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.

* 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(-)

-- 
2.7.0

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

[Ulrik de Muelenaere]

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 <ulrikdem@gmail.com>
---
 drivers/media/usb/gspca/gspca.c | 129 +++++++++++++++++++++++-----------------
 drivers/media/usb/gspca/gspca.h |   1 +
 2 files changed, 76 insertions(+), 54 deletions(-)

diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
index af5cd82..d002b8b 100644
--- a/drivers/media/usb/gspca/gspca.c
+++ b/drivers/media/usb/gspca/gspca.c
@@ -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;
 }
diff --git a/drivers/media/usb/gspca/gspca.h b/drivers/media/usb/gspca/gspca.h
index d39adf9..dcafc89 100644
--- a/drivers/media/usb/gspca/gspca.h
+++ b/drivers/media/usb/gspca/gspca.h
@@ -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
-- 
2.7.0

[PATCH 2/2] [media] gspca_kinect: enable both video and depth streams

[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 <ulrikdem@gmail.com>
---
 drivers/media/usb/gspca/kinect.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/media/usb/gspca/kinect.c b/drivers/media/usb/gspca/kinect.c
index 3cb30a3..4bc5b7d 100644
--- a/drivers/media/usb/gspca/kinect.c
+++ b/drivers/media/usb/gspca/kinect.c
@@ -36,8 +36,6 @@ MODULE_AUTHOR("Antonio Ospite <ospite@studenti.unina.it>");
 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");
-- 
2.7.0

Re: [PATCH 0/2] [media] gspca_kinect: enable both video and depth streams

[Hans de Goede]

Hi Ulrik,

On 06-03-16 14:50, Ulrik de Muelenaere wrote:
> 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.

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


>
> 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(-)
>

Re: [PATCH 0/2] [media] gspca_kinect: enable both video and depth streams

[Ulrik de Muelenaere]

Hi Hans,

On Mon, Mar 07, 2016 at 08:00:46PM +0100, Hans de Goede wrote:
> Hi Ulrik,
> 
> On 06-03-16 14:50, Ulrik de Muelenaere wrote:
> >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.
> 
> 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.

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

Regards,
Ulrik

> 
> Regards,
> 
> Hans
> 
> 
> >
> >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(-)
> >