Re: [PATCH 0/2] [media] gspca_kinect: enable both video and depth streams
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 _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(-) > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] [media] gspca_kinect: enable both video and depth streams
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 <ulrik...@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 <osp...@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, _desc_depth,
- sizeof(struct sd), THIS_MODULE);
- else
- return gspca_dev_probe(intf, id, _desc_video,
- sizeof(struct sd), THIS_MODULE);
+ int res;
+
+ res = gspca_dev_probe(intf, id, _desc_video, sizeof(struct sd),
+ THIS_MODULE);
+ if (res < 0)
+ return res;
+
+ res = gspca_dev_probe(intf, id, _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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] [media] 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 <ulrik...@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(_dev->vdev));
+ while (gspca_dev) {
+ PDEBUG(D_PROBE, "%s disconnect",
+ video_device_node_name(_dev->vdev));
- mutex_lock(_dev->usb_lock);
+ mutex_lock(_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(_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(_dev->wq);
- v4l2_device_disconnect(_dev->v4l2_dev);
- video_unregister_device(_dev->vdev);
+ v4l2_device_disconnect(_dev->v4l2_dev);
+ video_unregister_device(_dev->vdev);
- mutex_unlock(_dev->usb_lock);
+ mutex_unlock(_dev->usb_lock);
- /* (this will call gspca_release() immediately or on last close) */
- v4l2_device_put(_dev->v4l2_dev);
+ next_dev = gspca_dev->next_dev;
+ /* (this will call gspca_release() immediately or on last
+* close)
+*/
+ v4l2_device_put(_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;
[PATCH 0/2] [media] 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(-) -- 2.7.0 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
