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
Re: [PATCH 0/2] [media] gspca_kinect: enable both video and depth streams
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. 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 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
