[PATCH for 3.2 URGENT 0/1] Fix major regression in gspca

[PATCH for 3.2 URGENT 0/1] Fix major regression in gspca

[Hans de Goede]

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

[PATCH for 3.2 URGENT] gspca: Fix bulk mode cameras no longer working (regression fix)

[Hans de Goede]

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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/gspca/gspca.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index 271be98..5ce3557 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -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

[PATCH for 3.2 URGENT] gspca: Fix bulk mode cameras no longer working (regression fix)

[Mauro Carvalho Chehab]

From: Hans de Goede <hdegoede@redhat.com>

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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/video/gspca/gspca.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index 271be98..5ce3557 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -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

Re: [PATCH for 3.2 URGENT 0/1] Fix major regression in gspca

[Theodore Kilgore]

On Thu, 29 Dec 2011, Hans de Goede wrote:

> 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

Re: [PATCH for 3.2 URGENT] gspca: Fix bulk mode cameras no longer working (regression fix)

[Jean-Francois Moine]

On Thu, 29 Dec 2011 21:36:42 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> 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.

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/

Re: [PATCH for 3.2 URGENT] gspca: Fix bulk mode cameras no longer working (regression fix)

[Hans de Goede]

Hi,

On 12/30/2011 11:21 AM, Jean-Francois Moine wrote:
> On Thu, 29 Dec 2011 21:36:42 +0100
> Hans de Goede<hdegoede@redhat.com>  wrote:
>
>> 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.
>
> 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?

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

Re: [PATCH for 3.2 URGENT] gspca: Fix bulk mode cameras no longer working (regression fix)

[Theodore Kilgore]

On Fri, 30 Dec 2011, Hans de Goede wrote:

> Hi,
> 
> On 12/30/2011 11:21 AM, Jean-Francois Moine wrote:
> > On Thu, 29 Dec 2011 21:36:42 +0100
> > Hans de Goede<hdegoede@redhat.com>  wrote:
> > 
> > > 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.
> > 
> > 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?
> 
> 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

Re: [PATCH for 3.2 URGENT] gspca: Fix bulk mode cameras no longer working (regression fix)

[Hans de Goede]

Hi,

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

<snip>

> 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.

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

Regards,

Hans