* [PATCH 1/2] [media] gspca: allow multiple probes per USB interface
2016-03-06 13:50 [PATCH 0/2] [media] gspca_kinect: enable both video and depth streams Ulrik de Muelenaere
@ 2016-03-06 13:51 ` Ulrik de Muelenaere
2016-03-06 13:51 ` [PATCH 2/2] [media] gspca_kinect: enable both video and depth streams Ulrik de Muelenaere
2016-03-07 19:00 ` [PATCH 0/2] " Hans de Goede
2 siblings, 0 replies; 5+ messages in thread
From: Ulrik de Muelenaere @ 2016-03-06 13:51 UTC (permalink / raw)
To: linux-media; +Cc: Hans de Goede, Antonio Ospite
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/2] [media] gspca_kinect: enable both video and depth streams
2016-03-06 13:50 [PATCH 0/2] [media] gspca_kinect: enable both video and depth streams Ulrik de Muelenaere
2016-03-06 13:51 ` [PATCH 1/2] [media] gspca: allow multiple probes per USB interface Ulrik de Muelenaere
@ 2016-03-06 13:51 ` Ulrik de Muelenaere
2016-03-07 19:00 ` [PATCH 0/2] " Hans de Goede
2 siblings, 0 replies; 5+ messages in thread
From: Ulrik de Muelenaere @ 2016-03-06 13:51 UTC (permalink / raw)
To: linux-media; +Cc: Hans de Goede, Antonio Ospite
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 0/2] [media] gspca_kinect: enable both video and depth streams
2016-03-06 13:50 [PATCH 0/2] [media] gspca_kinect: enable both video and depth streams Ulrik de Muelenaere
2016-03-06 13:51 ` [PATCH 1/2] [media] gspca: allow multiple probes per USB interface Ulrik de Muelenaere
2016-03-06 13:51 ` [PATCH 2/2] [media] gspca_kinect: enable both video and depth streams Ulrik de Muelenaere
@ 2016-03-07 19:00 ` Hans de Goede
2016-03-07 19:23 ` Ulrik de Muelenaere
2 siblings, 1 reply; 5+ messages in thread
From: Hans de Goede @ 2016-03-07 19:00 UTC (permalink / raw)
To: Ulrik de Muelenaere, linux-media; +Cc: Antonio Ospite
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(-)
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 0/2] [media] gspca_kinect: enable both video and depth streams
2016-03-07 19:00 ` [PATCH 0/2] " Hans de Goede
@ 2016-03-07 19:23 ` Ulrik de Muelenaere
0 siblings, 0 replies; 5+ messages in thread
From: Ulrik de Muelenaere @ 2016-03-07 19:23 UTC (permalink / raw)
To: Hans de Goede; +Cc: Ulrik de Muelenaere, linux-media, Antonio Ospite
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(-)
> >
^ permalink raw reply [flat|nested] 5+ messages in thread