gspca second isoc endpoint / kinect depth

Hi,

I took drivers/media/usb/gspca/kinect.c as skeleton to build a depth driver for the kinect camera.

I needed to implement this feature because libfreenect performs so badly on the raspberry pi that you can't get a single frame.

The kinecet has two isoc endpoints but gspca only uses the first. To get it running I made a dirty hack to drivers/media/usb/gspca/gspca.c I changed usb_host_endpoint *alt_xfer(...) so that it always returns the second endpoint, which is not really good for everyone.

My driver is not ready for upstream now, it can not coexist with the current gspca_kinect so you have to decide if you want to load the video or the depth driver. Would be better to have one driver to do it all.

But in the meantime I would like to ask for ideas about a more clean solution to get other isoc endpoints.

There was already a little discussion about this when kinect.c was written by Antonio Ospite:
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/26194

http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/26213

Has something changed?

Is there a point against making multiple endpoints available?

Better solution?

Add a xfer_ep_index field to struct gspca_dev, and change alt_xfer() so that it accepts a parameter which represents a specific endpoint to look for.

If a subdriver wants to specify a value for gspca_dev->xfer_ep_index it can do that in its sd_config() callback.

Signed-off-by Antonio Ospite <[email protected]>

---

I am not sure if it is OK to specify an endpoint _index_ or if it would be better to specify the endpoint address directly (in Kinect 0x81 is for video data and 0x82 is for depth data).

Hans, any comment on that?

Thanks,
Antonio

 drivers/media/usb/gspca/gspca.c | 20 ++++++++++++++------
 drivers/media/usb/gspca/gspca.h |  1 +
 2 files changed, 15 insertions(+), 6 deletions(-)

drivers/media/usb/gspca/gspca.c +14 -6

index f3a7ace..7e5226c 100644
@@ -603,10 +603,13 @@ static void gspca_stream_off(struct gspca_dev *gspca_dev)
 }
 
 /*
- * look for an input transfer endpoint in an alternate setting
+ * look for an input transfer endpoint in an alternate setting.
+ *
+ * If xfer_ep_index is negative, return the first valid one found, otherwise
+ * look for exactly the one in position xfer_ep.
  */
 static struct usb_host_endpoint *alt_xfer(struct usb_host_interface *alt,
-					  int xfer)
+					  int xfer, int xfer_ep_index)
 {
 	struct usb_host_endpoint *ep;
 	int i, attr;

@@ -616,8 +619,10 @@ static struct usb_host_endpoint *alt_xfer(struct usb_host_interface *alt,
 		attr = ep->desc.bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
 		if (attr == xfer
 		    && ep->desc.wMaxPacketSize != 0
-		    && usb_endpoint_dir_in(&ep->desc))
+		    && usb_endpoint_dir_in(&ep->desc)
+		    && (xfer_ep_index < 0 || i == xfer_ep_index)) {
 			return ep;
+		}
 	}
 	return NULL;
 }

@@ -689,7 +694,7 @@ static int build_isoc_ep_tb(struct gspca_dev *gspca_dev,
 		found = 0;
 		for (j = 0; j < nbalt; j++) {
 			ep = alt_xfer(&intf->altsetting[j],
-				      USB_ENDPOINT_XFER_ISOC);
+				      USB_ENDPOINT_XFER_ISOC, gspca_dev->xfer_ep_index);
 			if (ep == NULL)
 				continue;
 			if (ep->desc.bInterval == 0) {

@@ -862,7 +867,8 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 	/* if bulk or the subdriver forced an altsetting, get the endpoint */
 	if (gspca_dev->alt != 0) {
 		gspca_dev->alt--;	/* (previous version compatibility) */
-		ep = alt_xfer(&intf->altsetting[gspca_dev->alt], xfer);
+		ep = alt_xfer(&intf->altsetting[gspca_dev->alt], xfer,
+			      gspca_dev->xfer_ep_index);
 		if (ep == NULL) {
 			pr_err("bad altsetting %d\n", gspca_dev->alt);
 			return -EIO;

@@ -904,7 +910,8 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 		if (!gspca_dev->cam.no_urb_create) {
 			PDEBUG(D_STREAM, "init transfer alt %d", alt);
 			ret = create_urbs(gspca_dev,
-				alt_xfer(&intf->altsetting[alt], xfer));
+				alt_xfer(&intf->altsetting[alt], xfer,
+					 gspca_dev->xfer_ep_index));
 			if (ret < 0) {
 				destroy_urbs(gspca_dev);
 				goto out;

@@ -2030,6 +2037,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_index = -1;
 
 	/* check if any audio device */
 	if (dev->actconfig->desc.bNumInterfaces != 1) {

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

index 300642d..92317af 100644
@@ -205,6 +205,7 @@ struct gspca_dev {
 	char memory;			/* memory type (V4L2_MEMORY_xxx) */
 	__u8 iface;			/* USB interface number */
 	__u8 alt;			/* USB alternate setting */
+	int xfer_ep_index;		/* index of the USB transfer endpoint */
 	u8 audio;			/* presence of audio device */
 
 	/* (*) These variables are proteced by both usb_lock and queue_lock,

Hi Antonio,

Thanks for working on this.

Quoted 14 lines · Antonio Ospite

Add a xfer_ep_index field to struct gspca_dev, and change alt_xfer() so that it accepts a parameter which represents a specific endpoint to look for.

If a subdriver wants to specify a value for gspca_dev->xfer_ep_index it can do that in its sd_config() callback.

Signed-off-by Antonio Ospite <[email protected]>

---

I am not sure if it is OK to specify an endpoint _index_ or if it would be better to specify the endpoint address directly (in Kinect 0x81 is for video data and 0x82 is for depth data).

Quoted 1 line

Hans, any comment on that?

I think it would be better to use the endpoint address directly for this, relying on the order in which the endpoints are listed in the descriptor feels wrong to me.

Other then that this patch looks good.

Regards,

Hans

Quoted 90 lines

Thanks,
Antonio

 drivers/media/usb/gspca/gspca.c | 20 ++++++++++++++------
 drivers/media/usb/gspca/gspca.h |  1 +
 2 files changed, 15 insertions(+), 6 deletions(-)

drivers/media/usb/gspca/gspca.c +14 -6

index f3a7ace..7e5226c 100644
@@ -603,10 +603,13 @@ static void gspca_stream_off(struct gspca_dev *gspca_dev)
 }
 
 /*
- * look for an input transfer endpoint in an alternate setting
+ * look for an input transfer endpoint in an alternate setting.
+ *
+ * If xfer_ep_index is negative, return the first valid one found, otherwise
+ * look for exactly the one in position xfer_ep.
  */
 static struct usb_host_endpoint *alt_xfer(struct usb_host_interface *alt,
-					  int xfer)
+					  int xfer, int xfer_ep_index)
 {
 	struct usb_host_endpoint *ep;
 	int i, attr;

@@ -616,8 +619,10 @@ static struct usb_host_endpoint *alt_xfer(struct usb_host_interface *alt,
 		attr = ep->desc.bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
 		if (attr == xfer
 		    && ep->desc.wMaxPacketSize != 0
-		    && usb_endpoint_dir_in(&ep->desc))
+		    && usb_endpoint_dir_in(&ep->desc)
+		    && (xfer_ep_index < 0 || i == xfer_ep_index)) {
 			return ep;
+		}
 	}
 	return NULL;
 }

@@ -689,7 +694,7 @@ static int build_isoc_ep_tb(struct gspca_dev *gspca_dev,
 		found = 0;
 		for (j = 0; j < nbalt; j++) {
 			ep = alt_xfer(&intf->altsetting[j],
-				      USB_ENDPOINT_XFER_ISOC);
+				      USB_ENDPOINT_XFER_ISOC, gspca_dev->xfer_ep_index);
 			if (ep == NULL)
 				continue;
 			if (ep->desc.bInterval == 0) {

@@ -862,7 +867,8 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 	/* if bulk or the subdriver forced an altsetting, get the endpoint */
 	if (gspca_dev->alt != 0) {
 		gspca_dev->alt--;	/* (previous version compatibility) */
-		ep = alt_xfer(&intf->altsetting[gspca_dev->alt], xfer);
+		ep = alt_xfer(&intf->altsetting[gspca_dev->alt], xfer,
+			      gspca_dev->xfer_ep_index);
 		if (ep == NULL) {
 			pr_err("bad altsetting %d\n", gspca_dev->alt);
 			return -EIO;

@@ -904,7 +910,8 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 		if (!gspca_dev->cam.no_urb_create) {
 			PDEBUG(D_STREAM, "init transfer alt %d", alt);
 			ret = create_urbs(gspca_dev,
-				alt_xfer(&intf->altsetting[alt], xfer));
+				alt_xfer(&intf->altsetting[alt], xfer,
+					 gspca_dev->xfer_ep_index));
 			if (ret < 0) {
 				destroy_urbs(gspca_dev);
 				goto out;

@@ -2030,6 +2037,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_index = -1;
 
 	/* check if any audio device */
 	if (dev->actconfig->desc.bNumInterfaces != 1) {

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

index 300642d..92317af 100644
@@ -205,6 +205,7 @@ struct gspca_dev {
 	char memory;			/* memory type (V4L2_MEMORY_xxx) */
 	__u8 iface;			/* USB interface number */
 	__u8 alt;			/* USB alternate setting */
+	int xfer_ep_index;		/* index of the USB transfer endpoint */
 	u8 audio;			/* presence of audio device */
 
 	/* (*) These variables are proteced by both usb_lock and queue_lock,

Quoted 19 lines · On Thu, 19 Jun 2014 16:27:59 +0200 Hans de Goede <[email protected]> wrote:

Hi Antonio,

Thanks for working on this.

Quoted 14 lines · Antonio Ospite

Add a xfer_ep_index field to struct gspca_dev, and change alt_xfer() so that it accepts a parameter which represents a specific endpoint to look for.

If a subdriver wants to specify a value for gspca_dev->xfer_ep_index it can do that in its sd_config() callback.

Signed-off-by Antonio Ospite <[email protected]>

---

I am not sure if it is OK to specify an endpoint _index_ or if it would be better to specify the endpoint address directly (in Kinect 0x81 is for video data and 0x82 is for depth data).

Quoted 6 lines

Quoted 1 line

Hans, any comment on that?

I think it would be better to use the endpoint address directly for this, relying on the order in which the endpoints are listed in the descriptor feels wrong to me.

I see.

If I declare the new field as __u8 (same type of a bEndpointAddress), I could mark an invalid ep address with ~(USB_ENDPOINT_DIR_MASK | USB_ENDPOINT_NUMBER_MASK) in gspca_dev_probe2(), instead of using an int set to -1; how does that sound?

Add support for the depth mode at 10bpp, use a command line parameter to switch mode.

NOTE: this is just a proof-of-concept, the final implementation will have to expose two v4l2 devices, one for the video stream and one for the depth stream.

Signed-off-by Alexander Sosna <[email protected]>

Signed-off-by Antonio Ospite <[email protected]>

---

For now a command line parameter called "depth_mode" is used to select which mode to activate when loading the driver, this is necessary because gspca is not quite ready to have a subdriver call gspca_dev_probe() multiple times.

The problem seems to be that gspca assumes one video device per USB _interface_, and so it stores a pointer to gspca_dev as the interface private data: see usb_set_intfdata(intf, gspca_dev) in gspca_dev_probe2().

If anyone feels brave (do a backup first, etc. etc.), hack the sd_probe() below to register both the devices: you will get the two v4l nodes and both streams will work OK, but the kernel will halt when you disconnect the device, i.e. some problem arises in gspca_disconnect() after the usb_get_intfdata(intf) call.

I am still figuring out the details of the failure sequence, and I'll try to imagine a way to support the use case "multiple v4l devices on one USB interface", but this will take some more time.

Thanks,
Antonio

 drivers/media/usb/gspca/kinect.c | 97 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 86 insertions(+), 11 deletions(-)

drivers/media/usb/gspca/kinect.c +86 -11

index 081f051..ce688d8 100644
@@ -36,6 +36,8 @@ 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;

@@ -73,6 +75,14 @@ struct sd {
 
 #define FPS_HIGH       0x0100
 
+static const struct v4l2_pix_format depth_camera_mode[] = {
+	{640, 480, V4L2_PIX_FMT_Y10BPACK, V4L2_FIELD_NONE,
+	 .bytesperline = 640 * 10 / 8,
+	 .sizeimage =  640 * 480 * 10 / 8,
+	 .colorspace = V4L2_COLORSPACE_SRGB,
+	 .priv = MODE_640x488 | FORMAT_Y10B},
+};
+
 static const struct v4l2_pix_format video_camera_mode[] = {
 	{640, 480, V4L2_PIX_FMT_SGRBG8, V4L2_FIELD_NONE,
 	 .bytesperline = 640,

@@ -219,7 +229,7 @@ static int write_register(struct gspca_dev *gspca_dev, uint16_t reg,
 }
 
 /* this function is called at probe time */
-static int sd_config(struct gspca_dev *gspca_dev,
+static int sd_config_video(struct gspca_dev *gspca_dev,
 		     const struct usb_device_id *id)
 {
 	struct sd *sd = (struct sd *) gspca_dev;

@@ -227,8 +237,7 @@ static int sd_config(struct gspca_dev *gspca_dev,
 
 	sd->cam_tag = 0;
 
-	/* Only video stream is supported for now,
-	 * which has stream flag = 0x80 */
+	/* video has stream flag = 0x80 */
 	sd->stream_flag = 0x80;
 
 	cam = &gspca_dev->cam;

@@ -245,6 +254,26 @@ static int sd_config(struct gspca_dev *gspca_dev,
 	return 0;
 }
 
+static int sd_config_depth(struct gspca_dev *gspca_dev,
+		     const struct usb_device_id *id)
+{
+	struct sd *sd = (struct sd *) gspca_dev;
+	struct cam *cam;
+
+	sd->cam_tag = 0;
+
+	cam = &gspca_dev->cam;
+
+	/* depth has stream flag = 0x70 */
+	sd->stream_flag = 0x70;
+	cam->cam_mode = depth_camera_mode;
+	cam->nmodes = ARRAY_SIZE(depth_camera_mode);
+
+	gspca_dev->xfer_ep_index = 1;
+
+	return 0;
+}
+
 /* this function is called at probe and resume time */
 static int sd_init(struct gspca_dev *gspca_dev)
 {

@@ -253,7 +282,7 @@ static int sd_init(struct gspca_dev *gspca_dev)
 	return 0;
 }
 
-static int sd_start(struct gspca_dev *gspca_dev)
+static int sd_start_video(struct gspca_dev *gspca_dev)
 {
 	int mode;
 	uint8_t fmt_reg, fmt_val;

@@ -325,12 +354,39 @@ static int sd_start(struct gspca_dev *gspca_dev)
 	return 0;
 }
 
-static void sd_stopN(struct gspca_dev *gspca_dev)
+static int sd_start_depth(struct gspca_dev *gspca_dev)
+{
+	/* turn off IR-reset function */
+	write_register(gspca_dev, 0x105, 0x00);
+
+	/* reset depth stream */
+	write_register(gspca_dev, 0x06, 0x00);
+	/* Depth Stream Format 0x03: 11 bit stream | 0x02: 10 bit */
+	write_register(gspca_dev, 0x12, 0x02);
+	/* Depth Stream Resolution 1: standard (640x480) */
+	write_register(gspca_dev, 0x13, 0x01);
+	/* Depth Framerate / 0x1e (30): 30 fps */
+	write_register(gspca_dev, 0x14, 0x1e);
+	/* Depth Stream Control  / 2: Open Depth Stream */
+	write_register(gspca_dev, 0x06, 0x02);
+	/* disable depth hflip / LSB = 0: Smoothing Disabled */
+	write_register(gspca_dev, 0x17, 0x00);
+
+	return 0;
+}
+
+static void sd_stopN_video(struct gspca_dev *gspca_dev)
 {
 	/* reset video stream */
 	write_register(gspca_dev, 0x05, 0x00);
 }
 
+static void sd_stopN_depth(struct gspca_dev *gspca_dev)
+{
+	/* reset depth stream */
+	write_register(gspca_dev, 0x06, 0x00);
+}
+
 static void sd_pkt_scan(struct gspca_dev *gspca_dev, u8 *__data, int len)
 {
 	struct sd *sd = (struct sd *) gspca_dev;

@@ -366,12 +422,24 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev, u8 *__data, int len)
 }
 
 /* sub-driver description */
-static const struct sd_desc sd_desc = {
+static const struct sd_desc sd_desc_video = {
 	.name      = MODULE_NAME,
-	.config    = sd_config,
+	.config    = sd_config_video,
 	.init      = sd_init,
-	.start     = sd_start,
-	.stopN     = sd_stopN,
+	.start     = sd_start_video,
+	.stopN     = sd_stopN_video,
+	.pkt_scan  = sd_pkt_scan,
+	/*
+	.get_streamparm = sd_get_streamparm,
+	.set_streamparm = sd_set_streamparm,
+	*/
+};
+static const struct sd_desc sd_desc_depth = {
+	.name      = MODULE_NAME,
+	.config    = sd_config_depth,
+	.init      = sd_init,
+	.start     = sd_start_depth,
+	.stopN     = sd_stopN_depth,
 	.pkt_scan  = sd_pkt_scan,
 	/*
 	.get_streamparm = sd_get_streamparm,

@@ -391,8 +459,12 @@ MODULE_DEVICE_TABLE(usb, device_table);
 /* -- device connect -- */
 static int sd_probe(struct usb_interface *intf, const struct usb_device_id *id)
 {
-	return gspca_dev_probe(intf, id, &sd_desc, sizeof(struct sd),
-				THIS_MODULE);
+	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);
 }
 
 static struct usb_driver sd_driver = {

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

Hi,

Quoted 3 lines · Antonio Ospite

Add support for the depth mode at 10bpp, use a command line parameter to switch mode.

Quoted 3 lines

NOTE: this is just a proof-of-concept, the final implementation will have to expose two v4l2 devices, one for the video stream and one for the depth stream.

Thanks for the patch. If this is useful for some people I'm willing to merge this until we've a better fix.

Quoted 19 lines

Signed-off-by Alexander Sosna <[email protected]>

Signed-off-by Antonio Ospite <[email protected]>

---

For now a command line parameter called "depth_mode" is used to select which mode to activate when loading the driver, this is necessary because gspca is not quite ready to have a subdriver call gspca_dev_probe() multiple times.

The problem seems to be that gspca assumes one video device per USB _interface_, and so it stores a pointer to gspca_dev as the interface private data: see usb_set_intfdata(intf, gspca_dev) in gspca_dev_probe2().

If anyone feels brave (do a backup first, etc. etc.), hack the sd_probe() below to register both the devices: you will get the two v4l nodes and both streams will work OK, but the kernel will halt when you disconnect the device, i.e. some problem arises in gspca_disconnect() after the usb_get_intfdata(intf) call.

Quoted 3 lines

I am still figuring out the details of the failure sequence, and I'll try to imagine a way to support the use case "multiple v4l devices on one USB interface", but this will take some more time.

I believe that support 2 devices would require separating the per video node / stream data and global data into separate structs, and then refactoring everything so that we can have 2 streams on one gspca_dev. If you do this please make it a patch-set with many small patches, rather then 1 or 2 very large patches.

And then in things like disconnect, loop over the streams and stop both, unregister both nodes, etc.

If you ever decide to add support for controls you will also need to think about what to do with those, but for now I guess you can just register all the controls on the first video-node/stream (which will be the only one for all devices except kinect devices, and the kinect code currently does not have controls.