Fix major regression in gspca

Hi all,

Unfortunately the new iso bandwidth calculation code in gspca has accidentally broken support for bulk mode cameras, breaking support for a wide range of chipsets (see the patch for a full list).

Mauro, please send this patch to Linus asap, so that 3.2 won't ship with this regression.

Thanks,

Hans

The new iso bandwidth calculation code accidentally has broken support for bulk mode cameras. This has broken the following drivers:
finepix, jeilinj, ovfx2, ov534, ov534_9, se401, sq905, sq905c, sq930x, stv0680, vicam.

Thix patch fixes this. Fix tested with: se401, sq905, sq905c, stv0680 & vicam cams.

---
 drivers/media/video/gspca/gspca.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 271be98..5ce3557 100644
@@ -838,13 +838,13 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 	gspca_dev->usb_err = 0;
 
 	/* do the specific subdriver stuff before endpoint selection */
-	gspca_dev->alt = 0;
+	intf = usb_ifnum_to_if(gspca_dev->dev, gspca_dev->iface);
+	gspca_dev->alt = gspca_dev->cam.bulk ? intf->num_altsetting : 0;
 	if (gspca_dev->sd_desc->isoc_init) {
 		ret = gspca_dev->sd_desc->isoc_init(gspca_dev);
 		if (ret < 0)
 			goto unlock;
 	}
-	intf = usb_ifnum_to_if(gspca_dev->dev, gspca_dev->iface);
 	xfer = gspca_dev->cam.bulk ? USB_ENDPOINT_XFER_BULK
 				   : USB_ENDPOINT_XFER_ISOC;
 

1.7.7.4

From: Hans de Goede <[email protected]>

The new iso bandwidth calculation code accidentally has broken support for bulk mode cameras. This has broken the following drivers:
finepix, jeilinj, ovfx2, ov534, ov534_9, se401, sq905, sq905c, sq930x, stv0680, vicam.

Thix patch fixes this. Fix tested with: se401, sq905, sq905c, stv0680 & vicam cams.

---
 drivers/media/video/gspca/gspca.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 271be98..5ce3557 100644
@@ -838,13 +838,13 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 	gspca_dev->usb_err = 0;
 
 	/* do the specific subdriver stuff before endpoint selection */
-	gspca_dev->alt = 0;
+	intf = usb_ifnum_to_if(gspca_dev->dev, gspca_dev->iface);
+	gspca_dev->alt = gspca_dev->cam.bulk ? intf->num_altsetting : 0;
 	if (gspca_dev->sd_desc->isoc_init) {
 		ret = gspca_dev->sd_desc->isoc_init(gspca_dev);
 		if (ret < 0)
 			goto unlock;
 	}
-	intf = usb_ifnum_to_if(gspca_dev->dev, gspca_dev->iface);
 	xfer = gspca_dev->cam.bulk ? USB_ENDPOINT_XFER_BULK
 				   : USB_ENDPOINT_XFER_ISOC;
 

1.7.7.4

Hi Hans,

Sorry for I should not be fully awoken yet, but I don't understand the problem from your fix.

The patch just sets the altsetting to the highest one for bulk transfer. Does this mean that, in this case, the altsetting table created by build_ep_tb is wrong and the highest altsetting cannot selected?

Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Hi,

Most bulk mode cameras have only one altsetting, altsetting 0, which is seen as invalid by build_ep_tb, since it is invalid for isoc mode, resulting in the cameras not working with a: "no transfer endpoint found" error.

I've opted to fix things by causing build_ep_tb to not be called for bulk mode cameras at all, since doing bandwidth calculations for bulk mode makes no sense. bulk transfers get whatever bandwidth is left on the bus, there is no guarantee that there are 1000 / interval packets a second like there is with isoc transfers, so the bandwidth is unknown. Also note that because of this interval is 0 for bulk endpoints, since it is unused. So calling build_ep_tb for bulk mode transfers makes no sense.

WRT just choosing the highest numbered alt setting this is because some bulk mode cameras (stv0680 based ones) report 2 alt settings (which makes no sense for bulk mode, but they do it anyways), with alt setting 0 not listing any endpoints at all, and alt setting 1 listening the bulk endpoint we want, so by picking the highest alt setting we end up with picking the one and only alt setting most cameras have and picking one which actually has the bulk endpoint listed for weird cases like the stv0680 based ones.

Note that I'm spending most of my time today on testing the new bandwidth code with various cameras, I'll send you a patchset with some more proposed patches today. We should then evaluate if we want to get those into 3.2 too. I send this one yesterday since it fixes a large bunch of cameras not working at all and it is a simple and safe fix IMHO.

Regards,

Hans

Jean-Francois, Hans,

Without addressing finer points, please let me add the following:

1. I figured out what was holding me back from getting 3.2 to work (it was a config error, apparently originating between keyboard and chair).

2. Based upon my testing today, something like this patch is clearly necessary. Namely, I tested a mass storage camera. Without this patch it would not stream. When I applied the patch, it did.

Therefore, I hope very much that the problem which occasioned this patch gets fixed.

Theodore Kilgore

Hi,

On 12/31/2011 08:08 PM, Theodore Kilgore wrote:

<snip>

No worries, this patch has already found its way into Linus' tree.

Regards,

Hans

Thanks, Hans.

BTW, 3.2 so far seems totally broken for me. I could not get nearly so far as to test any of these cameras, even though several of the drivers in question are mine. I started testing the kernel at level 3.2-rc6 and got repeated kernel oopses, happening every time I mounted a partition read-write. It did boot, finally, but it barely booted. So, no testing cameras, as I said. I never got that far.

Today I tried the latest, which calls itself 3.2-rc7. Its behavior is even worse. As I recall, it did not even complete the booting. :-/

So I sure hope that at least this patch helps out those people who _can_ boot the new kernel.

Theodore Kilgore