| Message ID | 1352398313-3698-22-git-send-email-fschaefer.oss@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 08.11.2012 21:19, schrieb Devin Heitmueller: > On Thu, Nov 8, 2012 at 1:11 PM, Frank Schäfer > <fschaefer.oss@googlemail.com> wrote: >> By default, isoc transfers are used if possible. >> With the new module parameter, bulk can be selected as the >> preferred USB transfer type. > Hi Frank, > > Does your device actually expose both isoc and bulk endpoints? If I > recall from the datasheet, whether isoc or bulk mode is provided is > not actually configurable from the driver. The EEPROM dictates how > the endpoint map gets defined, and hence it's either one or the other. > If that is indeed the case, then we don't need a modprobe option at > all (since would never actually be user configurable), and we should > just add a field to the board definition to indicate that bulk should > be used for that product. > > Devin > Hi Devin, at least the "Silvercrest Webcam 1.3mpix" (board 71) exposes both endpoint types (0x82=isoc and 0x84=bulk). Regards, Frank -- 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
On Thu, Nov 8, 2012 at 1:11 PM, Frank Schäfer <fschaefer.oss@googlemail.com> wrote: > By default, isoc transfers are used if possible. > With the new module parameter, bulk can be selected as the > preferred USB transfer type. Hi Frank, Does your device actually expose both isoc and bulk endpoints? If I recall from the datasheet, whether isoc or bulk mode is provided is not actually configurable from the driver. The EEPROM dictates how the endpoint map gets defined, and hence it's either one or the other. If that is indeed the case, then we don't need a modprobe option at all (since would never actually be user configurable), and we should just add a field to the board definition to indicate that bulk should be used for that product. Devin
On Thu, Nov 8, 2012 at 1:37 PM, Frank Schäfer <fschaefer.oss@googlemail.com> wrote: > at least the "Silvercrest Webcam 1.3mpix" (board 71) exposes both > endpoint types (0x82=isoc and 0x84=bulk). Ah, interesting. It might be worthwhile to log a warning in dmesg if the user sets the modprobe option but the board doesn't actually expose any bulk endpoints. This might help avoid questions from users (we already got one such question by somebody who believed enabling this would put the device into bulk mode even though his hardware didn't support it). Devin
Am 08.11.2012 21:46, schrieb Devin Heitmueller: > On Thu, Nov 8, 2012 at 1:37 PM, Frank Schäfer > <fschaefer.oss@googlemail.com> wrote: >> at least the "Silvercrest Webcam 1.3mpix" (board 71) exposes both >> endpoint types (0x82=isoc and 0x84=bulk). > Ah, interesting. It might be worthwhile to log a warning in dmesg if > the user sets the modprobe option but the board doesn't actually > expose any bulk endpoints. This might help avoid questions from users > (we already got one such question by somebody who believed enabling > this would put the device into bulk mode even though his hardware > didn't support it). > > Devin > Well, I deliberately called the module 'prefer_bulk' (and not 'use_bulk', 'force_bulk' ...) which should imply that nothing is guaranteed. And selecting bulk transfers for a device which actually doesn not provide bulk support doesn't make sense and is clearly the users fault. Anway, I'm fine with adding a warning message and maybe I could extend the module parameter description, too. I'm going to wait for further feedback from Mauro before sending an updated version of the patch (series). Regards, Frank -- 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
On Fri, Nov 9, 2012 at 11:00 AM, Frank Schäfer <fschaefer.oss@googlemail.com> wrote: > Well, I deliberately called the module 'prefer_bulk' (and not > 'use_bulk', 'force_bulk' ...) which should imply that nothing is guaranteed. > And selecting bulk transfers for a device which actually doesn not > provide bulk support doesn't make sense and is clearly the users fault. > Anway, I'm fine with adding a warning message and maybe I could extend > the module parameter description, too. > > I'm going to wait for further feedback from Mauro before sending an > updated version of the patch (series). Yeah, none of this should hold up it being merged as-is. A patch adding the warning can be submitted after the fact. Devin
Hi Frank, Em Thu, 8 Nov 2012 20:11:53 +0200 Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > By default, isoc transfers are used if possible. > With the new module parameter, bulk can be selected as the > preferred USB transfer type. I did some tests yesterday with prefer_bulk. IMHO, webcams should select bulk mode by default, as this allows more than one camera to work at the same time (I tested yesterday with 3 Silvercrest ones on my notebook). With ISOC transfers, the core won't let it to happen, as a single camera reserves 51% of the max allowed isoc traffic. > Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> > --- > drivers/media/usb/em28xx/em28xx-cards.c | 11 +++++++++-- > 1 Datei geändert, 9 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-) > > diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c > index a9344f0..7f5b303 100644 > --- a/drivers/media/usb/em28xx/em28xx-cards.c > +++ b/drivers/media/usb/em28xx/em28xx-cards.c > @@ -61,6 +61,11 @@ static unsigned int card[] = {[0 ... (EM28XX_MAXBOARDS - 1)] = UNSET }; > module_param_array(card, int, NULL, 0444); > MODULE_PARM_DESC(card, "card type"); > > +static unsigned int prefer_bulk; > +module_param(prefer_bulk, int, 0644); This needs to be changed to 0444, as prefer_bulk doesn't allow changing it dynamically, as the test is done during device probe, not at stream on. Regards, Mauro -- 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
Am 23.12.2012 14:44, schrieb Mauro Carvalho Chehab: > Hi Frank, > > Em Thu, 8 Nov 2012 20:11:53 +0200 > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > >> By default, isoc transfers are used if possible. >> With the new module parameter, bulk can be selected as the >> preferred USB transfer type. > I did some tests yesterday with prefer_bulk. IMHO, webcams should > select bulk mode by default, as this allows more than one camera to > work at the same time (I tested yesterday with 3 Silvercrest ones on > my notebook). With ISOC transfers, the core won't let it to happen, as > a single camera reserves 51% of the max allowed isoc traffic. Ok. I just didn't want to change the current behavior because of potential regressions. Why not change it for all devices ? Frame data processing with bulk transfers has a smaller overhead than with isoc (although not really measurable ;) ). I will send a patch after christmas. > >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> >> --- >> drivers/media/usb/em28xx/em28xx-cards.c | 11 +++++++++-- >> 1 Datei geändert, 9 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-) >> >> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c >> index a9344f0..7f5b303 100644 >> --- a/drivers/media/usb/em28xx/em28xx-cards.c >> +++ b/drivers/media/usb/em28xx/em28xx-cards.c >> @@ -61,6 +61,11 @@ static unsigned int card[] = {[0 ... (EM28XX_MAXBOARDS - 1)] = UNSET }; >> module_param_array(card, int, NULL, 0444); >> MODULE_PARM_DESC(card, "card type"); >> >> +static unsigned int prefer_bulk; >> +module_param(prefer_bulk, int, 0644); > This needs to be changed to 0444, as prefer_bulk doesn't allow changing > it dynamically, as the test is done during device probe, not at stream on. Good catch ! Can you fix it ? I'm a bit in hurry right now. Otherwise I will try to send a patch tomorrow. Merry Christmas ! Frank > > Regards, > Mauro -- 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
Em Sun, 23 Dec 2012 15:01:26 +0100 Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > Am 23.12.2012 14:44, schrieb Mauro Carvalho Chehab: > > Hi Frank, > > > > Em Thu, 8 Nov 2012 20:11:53 +0200 > > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > > > >> By default, isoc transfers are used if possible. > >> With the new module parameter, bulk can be selected as the > >> preferred USB transfer type. > > I did some tests yesterday with prefer_bulk. IMHO, webcams should > > select bulk mode by default, as this allows more than one camera to > > work at the same time (I tested yesterday with 3 Silvercrest ones on > > my notebook). With ISOC transfers, the core won't let it to happen, as > > a single camera reserves 51% of the max allowed isoc traffic. > > Ok. I just didn't want to change the current behavior because of > potential regressions. > Why not change it for all devices ? Frame data processing with bulk > transfers has a smaller overhead than with isoc (although not really > measurable ;) ). It is better to keep it as-is for the other devices. There are simply too much non-webcam devices for us to be sure that this will always work. As there are very few webcams supported, the risk of this change is low if applied only to webcams. > I will send a patch after christmas. > > > > >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> > >> --- > >> drivers/media/usb/em28xx/em28xx-cards.c | 11 +++++++++-- > >> 1 Datei geändert, 9 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-) > >> > >> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c > >> index a9344f0..7f5b303 100644 > >> --- a/drivers/media/usb/em28xx/em28xx-cards.c > >> +++ b/drivers/media/usb/em28xx/em28xx-cards.c > >> @@ -61,6 +61,11 @@ static unsigned int card[] = {[0 ... (EM28XX_MAXBOARDS - 1)] = UNSET }; > >> module_param_array(card, int, NULL, 0444); > >> MODULE_PARM_DESC(card, "card type"); > >> > >> +static unsigned int prefer_bulk; > >> +module_param(prefer_bulk, int, 0644); > > This needs to be changed to 0444, as prefer_bulk doesn't allow changing > > it dynamically, as the test is done during device probe, not at stream on. > > Good catch ! > Can you fix it ? I'm a bit in hurry right now. > Otherwise I will try to send a patch tomorrow. Yeah, I can do it. > Merry Christmas ! > Frank Merry Christmas!
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c index a9344f0..7f5b303 100644 --- a/drivers/media/usb/em28xx/em28xx-cards.c +++ b/drivers/media/usb/em28xx/em28xx-cards.c @@ -61,6 +61,11 @@ static unsigned int card[] = {[0 ... (EM28XX_MAXBOARDS - 1)] = UNSET }; module_param_array(card, int, NULL, 0444); MODULE_PARM_DESC(card, "card type"); +static unsigned int prefer_bulk; +module_param(prefer_bulk, int, 0644); +MODULE_PARM_DESC(prefer_bulk, "prefer USB bulk transfers"); + + /* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS - 1 */ static unsigned long em28xx_devused; @@ -3334,9 +3339,11 @@ static int em28xx_usb_probe(struct usb_interface *interface, } /* Select USB transfer types to use */ - if (has_video && !dev->analog_ep_isoc) + if (has_video && + (!dev->analog_ep_isoc || (prefer_bulk && dev->analog_ep_bulk))) dev->analog_xfer_bulk = 1; - if (has_dvb && !dev->dvb_ep_isoc) + if (has_dvb && + (!dev->dvb_ep_isoc || (prefer_bulk && dev->dvb_ep_bulk))) dev->dvb_xfer_bulk = 1; snprintf(dev->name, sizeof(dev->name), "em28xx #%d", nr);
By default, isoc transfers are used if possible. With the new module parameter, bulk can be selected as the preferred USB transfer type. Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> --- drivers/media/usb/em28xx/em28xx-cards.c | 11 +++++++++-- 1 Datei geändert, 9 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-)