Em 08-12-2010 08:01, Hans de Goede escreveu:
> Hi,
>
> On 12/07/2010 09:01 PM, Mauro Carvalho Chehab wrote:
>> Hi Jean-Fronçois,
>>
>> git commit 35680baa6822df98a6ed602e2380aa0a04e18b07 (see enclosed) caused not only a regression
>> at PS/3 Eye webcam (git commit f43402fa55bf5e7e190c176343015122f694857c), but also at sn9c201 driver,
>> when used on SXGA mode. What happens is that only the highest alternate mode is enough for the
>> maximum resolution.
>>
>> I suspect that other drivers broke due to that change. So, IMO, the better is to revert and work
>> on another alternative.
>>
>
> I have to agree with Mauro here, dropping back to a lower alt setting for all devices with a usb
> audio interface is the wrong thing to do.
Btw, I tested here with other hardware. It completely broke stv06xx driver for devices with audio
(046d:08f6 Logitech, Inc. QuickCam Messenger Plus), as there's just one alternate config:
gspca: no transfer endpoint found
My proposal, for now, is to just revert the logic, enabling the hack only in the specific case where
doing alt-1 work (see enclosed patch). I tested the enclosed patch and it fixed for PS/3, Quickcam
and Gigaware (the 3 cams where I noticed the breakage).
Of course, it will break the specific model(s) where the audio hack work, so we need to do:
gspca_dev->audio_hack = 1;
On the specific model(s) tested with the hack.
> For example I know multiple devices where the highest alt setting does not have a wMaxPacketSize
> of 1023, but use something lower instead, to make sure there is enough bandwidth left for usb audio
> even if the highest alt setting get used.
>
>> Btw, no matter what resolution is used, sn9c201 is setting the same alternate for all modes,
>> spending more USB bandwidth than needed. Why gspca is just getting the highest value for
>> wMaxPacketSize? It should, instead, seek for the minimum packet size that is needed for a
>> given resolution.
>
> There are 2 reasons for this:
> 1) We do not know the minimum packetsize for a given resolution, unlike with uvc where this
> is arranged in the protocol, for proprietary apps we can only guess
Ok, but, during the driver development, someone could test it.
> 2) Almost all devices supported by gspca are usb-1.1 and thus the framerate is bandwidth
> limited. We want to deliver the highest framerate possible, and thus get as high an alt
> setting as possible.
This is a bogus argument. Core shouldn't assume that all devices are usb 1.1.
In the specific case of sn9c20x, it has 8 alternates. This probably means that all those alternates
could be used, depending on the bandwidth requirements.
At the maximum packet size (alt 8) means to spend 60% of the USB 2.0 traffic for isoc, being
required for SXGA. Alt 7 spends 36% and it is enough for VGA resolution.
If someone wants to have 2 webcams at VGA resolution, wasting bandwith using alt8 would prevent it,
as core don't provide 120% bandwidth ;)
The alternate selection should be based on the desired resolution. It is a function of
the imagesize, the frame rate, and the line width. On em28xx, we've made a function that estimates
the needed alternate. This works pretty well.
> I most note that many gspca subdrivers are defective wrt how this is all handled though, in
> that they don't vary the framerate when the available bandwidth changes. So in many cases if
> the gspca core cannot get the highest alt setting things may not function at all (because the
> subdriver fails to select a lower framerate, resulting in parts of frames getting dropped
> due to bandwidth limitations).
Ok, but gspca shouldn't assume that subdrivers won't do it. I did a "grep alt" and discovered that
most drivers just force the number of alternates to something else, like:
drivers/media/video/gspca/spca561.c: gspca_dev->nbalt = 7 + 1; /* choose alternate 7 first */
Despite the comment, what the above does is to change the alternates count to 8 (0 to 7), instead of
asking the core to use alternate 7. Ah, and, due to the audio hack, the above hack won't work anymore,
if the device has audio, as the core will decrement it to 6.
The only driver that seems to have a logic to work with different alts per-resolution is xirlink_cit.c,
but it is based on a try-and-error approach.
Drivers shouldn't be allowed to change the number of alternates. Instead, the core should provide a standard
way to use an alternate number provided by the sub-driver, or, otherwise, fallback to an auto-select
mode. Something like:
if (gspca_dev->force_alt && gspca_dev->force_alt < gspca_dev->nbalt)
gspca_dev->alt = gspca_dev->force_alt;
else {
/* Current way */
}
> Fixing this requires:
> 1) Figuring out how to change the framerate for each sensor + bridge combo
> 2) Testing to see what framerate will still work at which alt setting
Yes, but core needs to provide a way for it.
I think I'll try to implement something like the above for sn9c20x driver, as I'm currently
working with two webcams here[1].
Btw, one of the cams I'm working has a mono sensor (MI1300/MT9M001) that produces image
in GREY. We'll likely need some support at libv4l for grey format.
Cheers,
Mauro
[1] side note: I'm trying to make zbar to read Brazil's bank codebars. I'm just tired
on having to type all those large payment codes... The codebars used here are large
(about 12 cm). They seem to need a webcam with a very good resolution, otherwise zbar won't
catch it. SXGA doesn't seem enough with a color sensor. I'm hoping to have better results
with a mono sensor. If this won't work, I'll likely need to have a higher resolution webcam ;)
Btw, I've made a patch for zbar to work with libv4l:
http://sourceforge.net/tracker/?func=detail&aid=3128542&group_id=189236&atid=928517
>
> Regards,
>
> Hans
diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index 0a7af73..2572706 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -652,7 +652,7 @@ static struct usb_host_endpoint *get_ep(struct gspca_dev *gspca_dev)
: USB_ENDPOINT_XFER_ISOC;
i = gspca_dev->alt; /* previous alt setting */
if (gspca_dev->cam.reverse_alts) {
- if (gspca_dev->audio && i < gspca_dev->nbalt - 2)
+ if (gspca_dev->audio_hack && gspca_dev->audio)
i++;
while (++i < gspca_dev->nbalt) {
ep = alt_xfer(&intf->altsetting[i], xfer);
@@ -660,7 +660,7 @@ static struct usb_host_endpoint *get_ep(struct gspca_dev *gspca_dev)
break;
}
} else {
- if (gspca_dev->audio && i > 1)
+ if (gspca_dev->audio_hack && gspca_dev->audio)
i--;
while (--i >= 0) {
ep = alt_xfer(&intf->altsetting[i], xfer);
diff --git a/drivers/media/video/gspca/gspca.h b/drivers/media/video/gspca/gspca.h
index d4d210b..1f35bf4 100644
--- a/drivers/media/video/gspca/gspca.h
+++ b/drivers/media/video/gspca/gspca.h
@@ -220,6 +220,7 @@ struct gspca_dev {
__u8 alt; /* USB alternate setting */
__u8 nbalt; /* number of USB alternate settings */
u8 audio; /* presence of audio device */
+ u8 audio_hack; /* audio hack to reserve bw for audio */
};
int gspca_dev_probe(struct usb_interface *intf,
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
