Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-11-09 Thread Frank Schäfer
Am 09.11.2012 17:02, schrieb Mauro Carvalho Chehab:
> Em Thu, 08 Nov 2012 20:03:47 +0200
> Frank Schäfer  escreveu:
>
>> Am 30.10.2012 19:18, schrieb Frank Schäfer:
>>> Am 30.10.2012 06:06, schrieb Mauro Carvalho Chehab:
>>>
>>> 
 Did a git bisect. The last patch where the bug doesn't occur is this 
 changeset:
em28xx: add module parameter for selection of the preferred USB 
 transfer type

 That means that this changeset broke it:

em28xx: use common urb data copying function for vbi and non-vbi devices
>>> Ok, thanks.
>>> That means we are VERY close...
>>>
>>> I think this is the only change that could cause the trouble:
 @@ -599,6 +491,7 @@ static inline int em28xx_urb_data_copy_vbi(struct 
 em28xx *dev, struct urb *urb)
len = actual_length - 4;
} else if (p[0] == 0x22 && p[1] == 0x5a) {
/* start video */
 +  dev->capture_type = 1;
p += 4;
len = actual_length - 4;
} else {
>>> Could you try again with this line commented out ? (em28xx-video.c, line
>>> 494 in the patched file).
>>> usb_debug=1 would be usefull, too.
>>>
 I didn't test them with my Silvercrest webcam yet.
>>> I re-tested 5 minutes ago with this device and it works fine.
>>> Btw, which frame rates do you get  ? ;)
>>>
>>> Regards,
>>> Frank
>> Today I had the chance to test these patches with a Hauppauge HVR-930c.
>> Couldn't test analog TV (not supported yet), but DVB works fine, too.
> While I would love to have it, analog support for HVR-930C would likely
> not happen. I don't know anyone working on it. There are two issues there:
> 1) it uses an unsupported micronas analog demod chipset;
> 2) drx-k requrires some changes to tune on analog  mode.

Yeah, I heard about that. :(

> As usual, patches for it are of course very welcome.

I don't own this device, just borrowed it for some minutes for testing.
Apart from that, it seems I will be busy with the Laplace webcam support
for the next years... :D

Regards,
Frank

>
>> So patches 1 to 21 have been tested now and do at least not cause any
>> regressions.
>>
>> I would like to drop the last two patches (22+23) of this series, because
>> - they are actually not related to USB bulk transfers
>> - patch 22 needs to be fixed for analog+vbi (will get an analog device
>> for testing next week)
>> - I'm working on further improvements/changes in this area (including
>> em25xx support)
>> So I will better come up with a separate patch series later.
> OK.
>
>> Will send a v2 of this patch series soon.
>>
>> Regards,
>> Frank
>>
>>
> Cheers,
> Mauro

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-11-09 Thread Mauro Carvalho Chehab
Em Thu, 08 Nov 2012 20:03:47 +0200
Frank Schäfer  escreveu:

> Am 30.10.2012 19:18, schrieb Frank Schäfer:
> > Am 30.10.2012 06:06, schrieb Mauro Carvalho Chehab:
> >
> > 
> >> Did a git bisect. The last patch where the bug doesn't occur is this 
> >> changeset:
> >>em28xx: add module parameter for selection of the preferred USB 
> >> transfer type
> >>
> >> That means that this changeset broke it:
> >>
> >>em28xx: use common urb data copying function for vbi and non-vbi devices
> > Ok, thanks.
> > That means we are VERY close...
> >
> > I think this is the only change that could cause the trouble:
> >> @@ -599,6 +491,7 @@ static inline int em28xx_urb_data_copy_vbi(struct 
> >> em28xx *dev, struct urb *urb)
> >>len = actual_length - 4;
> >>} else if (p[0] == 0x22 && p[1] == 0x5a) {
> >>/* start video */
> >> +  dev->capture_type = 1;
> >>p += 4;
> >>len = actual_length - 4;
> >>} else {
> > Could you try again with this line commented out ? (em28xx-video.c, line
> > 494 in the patched file).
> > usb_debug=1 would be usefull, too.
> >
> >> I didn't test them with my Silvercrest webcam yet.
> > I re-tested 5 minutes ago with this device and it works fine.
> > Btw, which frame rates do you get  ? ;)
> >
> > Regards,
> > Frank
> 
> Today I had the chance to test these patches with a Hauppauge HVR-930c.
> Couldn't test analog TV (not supported yet), but DVB works fine, too.

While I would love to have it, analog support for HVR-930C would likely
not happen. I don't know anyone working on it. There are two issues there:
1) it uses an unsupported micronas analog demod chipset;
2) drx-k requrires some changes to tune on analog  mode.

As usual, patches for it are of course very welcome.

> 
> So patches 1 to 21 have been tested now and do at least not cause any
> regressions.
> 
> I would like to drop the last two patches (22+23) of this series, because
> - they are actually not related to USB bulk transfers
> - patch 22 needs to be fixed for analog+vbi (will get an analog device
> for testing next week)
> - I'm working on further improvements/changes in this area (including
> em25xx support)
> So I will better come up with a separate patch series later.

OK.

> Will send a v2 of this patch series soon.
> 
> Regards,
> Frank
> 
> 

Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-11-08 Thread Frank Schäfer
Am 30.10.2012 19:18, schrieb Frank Schäfer:
> Am 30.10.2012 06:06, schrieb Mauro Carvalho Chehab:
>
> 
>> Did a git bisect. The last patch where the bug doesn't occur is this 
>> changeset:
>>  em28xx: add module parameter for selection of the preferred USB 
>> transfer type
>>
>> That means that this changeset broke it:
>>
>>  em28xx: use common urb data copying function for vbi and non-vbi devices
> Ok, thanks.
> That means we are VERY close...
>
> I think this is the only change that could cause the trouble:
>> @@ -599,6 +491,7 @@ static inline int em28xx_urb_data_copy_vbi(struct em28xx 
>> *dev, struct urb *urb)
>>  len = actual_length - 4;
>>  } else if (p[0] == 0x22 && p[1] == 0x5a) {
>>  /* start video */
>> +dev->capture_type = 1;
>>  p += 4;
>>  len = actual_length - 4;
>>  } else {
> Could you try again with this line commented out ? (em28xx-video.c, line
> 494 in the patched file).
> usb_debug=1 would be usefull, too.
>
>> I didn't test them with my Silvercrest webcam yet.
> I re-tested 5 minutes ago with this device and it works fine.
> Btw, which frame rates do you get  ? ;)
>
> Regards,
> Frank

Today I had the chance to test these patches with a Hauppauge HVR-930c.
Couldn't test analog TV (not supported yet), but DVB works fine, too.

So patches 1 to 21 have been tested now and do at least not cause any
regressions.

I would like to drop the last two patches (22+23) of this series, because
- they are actually not related to USB bulk transfers
- patch 22 needs to be fixed for analog+vbi (will get an analog device
for testing next week)
- I'm working on further improvements/changes in this area (including
em25xx support)
So I will better come up with a separate patch series later.

Will send a v2 of this patch series soon.

Regards,
Frank


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-31 Thread Benny Amorsen
Ezequiel Garcia  writes:

> Very interesting. Let me see if I understand this: you say it's not a
> problem with USB bandwidth, but with isochronous transfers, in the
> sense it could achieve enough speed for streaming if bulk transfers
> were used?

It is more of a hope than a statement... I have no proof.

> Do you have any links supporting this?

Only old stuff like 
http://www.mail-archive.com/linux-usb@vger.kernel.org/msg04232.html

There are quite a few reports of problems with USB cameras in general
and Kinect in particular. That seems to point at problems with
isochronous transfers. A typical USB camera does not need particularly
much bandwidth.

The Nanostick only needs 40Mbps + overhead for me -- the size of the
largest MUX in the UK currently. That is less than 10% of the 480Mbps
theoretically available.

The other problem reports tend to be about "full speed" (11Mbps) USB
devices which are difficult for the Pi hardware to handle. Most of the
reports are getting old, some have reported that driver upgrades fixed
the problems.

I believe most people experience stable ethernet performance (and the
ethernet is USB attached as you are undoubtedly aware). That is a lot
more demanding than a USB camera or a Nanostick. However, the ethernet
chip uses bulk transfers, not isochronous ones.


/Benny

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-31 Thread Ezequiel Garcia
Benny,

On Wed, Oct 31, 2012 at 4:58 PM, Benny Amorsen  wrote:
>>
>> Is this a regression caused by patches or a general issue with the
>> Raspberry board ?
>
> It is a general issue with the Raspberry USB host controller or driver.
> Bulk transfers work, isochronous transfers have problems. I was hoping I
> could somehow convince the Nanostick to use bulk transfers instead of
> isochronous transfers. Since that seems to require a firmware change, I
> will have to give up on it.
>
>

Very interesting. Let me see if I understand this: you say it's not a
problem with USB bandwidth,
but with isochronous transfers, in the sense it could achieve enough
speed for streaming
if bulk transfers were used?

Do you have any links supporting this?

Thanks,

Ezequiel
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-31 Thread Benny Amorsen
Frank Schäfer  writes:

> For DVB, the em28xx always selects the alternate setting with the
> largest wMaxPacketSize.
> There is a module parameter 'alt' to select it manually for experiments,
> but the current code unfortunately applies it for analog capturing only. :(

What is the meaning of "alternate setting" here? Would I gain anything
if the driver was modified to apply alternate setting for DVB as well?


/Benny

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-31 Thread Benny Amorsen
Ezequiel Garcia  writes:

> Isn't this completely OT?

It may be off topic, but those issues were the reason I was testing the
patches...

> Anyway, RPI has known issues regarding USB bandwidth.

Indeed. Thank you for the links, I had not followed the latest
development and did not know about Gordon implementing EHCI in firmware.
That will be interesting.


/Benny

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-31 Thread Benny Amorsen

Frank Schäfer  writes:

> It seems like your device has no bulk endpoint for DVB.
> What does lsusb say ?

lsusb mentions 4 different end points, all isochronous. So out of luck
there. I did not know I could use lsusb to find this out.

> The module parameter is called prefer_bulk, but what it actually does is
> "force bulk" (which doesn't make much sense when the device has no bulk
> endpoints).
> I will fix this in v2 of the patch series.

Well, I was hoping to get "force_bulk", so that part is not a problem
for me.

> Am 31.10.2012 03:39, schrieb Benny Amorsen:

>> It works great with isochronous transfers on my PC and the Fedora
>> kernel, but the Raspberry USB host blows up when trying to do
>> isochronous mode.
>
> Is this a regression caused by patches or a general issue with the
> Raspberry board ?

It is a general issue with the Raspberry USB host controller or driver.
Bulk transfers work, isochronous transfers have problems. I was hoping I
could somehow convince the Nanostick to use bulk transfers instead of
isochronous transfers. Since that seems to require a firmware change, I
will have to give up on it.


/Benny

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-31 Thread Frank Schäfer
Am 31.10.2012 12:57, schrieb Ezequiel Garcia:
> On Tue, Oct 30, 2012 at 10:39 PM, Benny Amorsen  
> wrote:
>> Frank Schäfer  writes:
>>
>>> This patch series adds support for USB bulk transfers to the em28xx driver.
>> I tried these patches on my Raspberry Pi, 3.6.1 kernel, Nanostick 290e
>>
>> options em28xx prefer_bulk=1 core_debug=1 usb_debug=1
>> options em28xx_dvb debug=1
>>
>> [5.469510] em28xx: New device PCTV Systems PCTV 290e @ 480 Mbps 
>> (2013:024f, interface 0, class 0)
>> [5.890637] em28xx: DVB interface 0 found
>> [6.025292] em28xx #0: chip ID is em28174
>> [6.515383] em28xx #0: Identified as PCTV nanoStick T2 290e (card=78)
>> [6.567066] em28xx #0: v4l2 driver version 0.1.3
>> [6.614720] em28xx #0 em28xx_set_alternate :minimum isoc packet size: 
>> 2888 (alt=0)
>> [6.663064] em28xx #0 em28xx_set_alternate :setting alternate 0 with 
>> wMaxPacketSize=0
>> [6.715934] em28xx #0 em28xx_accumulator_set :em28xx Scale: 
>> (1,1)-(179,143)
>> [6.765694] em28xx #0 em28xx_capture_area_set :em28xx Area Set: (180,144)
>> [6.793060] em28xx #0: V4L2 video device registered as video0
>> [6.808200] em28xx #0 em28xx_alloc_urbs :em28xx: called em28xx_alloc_isoc 
>> in mode 2
>> [6.819456] em28xx #0: no endpoint for DVB mode and transfer type 1
>> [6.829283] em28xx: Failed to pre-allocate USB transfer buffers for DVB.
>> [6.839454] em28xx: probe of 1-1.3.1:1.0 failed with error -22
>> [6.852511] usbcore: registered new interface driver em28xx
>> [7.255738] em28xx #0 em28xx_accumulator_set :em28xx Scale: 
>> (1,1)-(179,143)
>> [7.291575] em28xx #0 em28xx_capture_area_set :em28xx Area Set: (180,144)
>> [7.326200] em28xx #0 em28xx_uninit_usb_xfer :em28xx: called 
>> em28xx_uninit_usb_xfer in mode 1
>>
>> Is the Nanostick 290e just fundamentally incompatible with bulk
>> transfers, or is there hope yet?
>>
>> It works great with isochronous transfers on my PC and the Fedora
>> kernel, but the Raspberry USB host blows up when trying to do
>> isochronous mode.
>>
>>
> Isn't this completely OT?
>
> Anyway, RPI has known issues regarding USB bandwidth.
>
> See here
>
> https://github.com/ezequielgarcia/stk1160-standalone/issues/8
> https://github.com/ezequielgarcia/stk1160-standalone/issues/2
> http://www.raspberrypi.org/phpBB3/viewtopic.php?p=196918#p196918

For DVB, the em28xx always selects the alternate setting with the
largest wMaxPacketSize.
There is a module parameter 'alt' to select it manually for experiments,
but the current code unfortunately applies it for analog capturing only. :(

Hope this helps,
Frank

> Regards,
>
> Ezequiel

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-31 Thread Frank Schäfer
Hi Benny,

Am 31.10.2012 03:39, schrieb Benny Amorsen:
> Frank Schäfer  writes:
>
>> This patch series adds support for USB bulk transfers to the em28xx driver.
> I tried these patches on my Raspberry Pi, 3.6.1 kernel, Nanostick 290e

Thank you for testing !

> options em28xx prefer_bulk=1 core_debug=1 usb_debug=1
> options em28xx_dvb debug=1
>
> [5.469510] em28xx: New device PCTV Systems PCTV 290e @ 480 Mbps 
> (2013:024f, interface 0, class 0)
> [5.890637] em28xx: DVB interface 0 found
> [6.025292] em28xx #0: chip ID is em28174
> [6.515383] em28xx #0: Identified as PCTV nanoStick T2 290e (card=78)
> [6.567066] em28xx #0: v4l2 driver version 0.1.3
> [6.614720] em28xx #0 em28xx_set_alternate :minimum isoc packet size: 2888 
> (alt=0)
> [6.663064] em28xx #0 em28xx_set_alternate :setting alternate 0 with 
> wMaxPacketSize=0
> [6.715934] em28xx #0 em28xx_accumulator_set :em28xx Scale: (1,1)-(179,143)
> [6.765694] em28xx #0 em28xx_capture_area_set :em28xx Area Set: (180,144)
> [6.793060] em28xx #0: V4L2 video device registered as video0
> [6.808200] em28xx #0 em28xx_alloc_urbs :em28xx: called em28xx_alloc_isoc 
> in mode 2
> [6.819456] em28xx #0: no endpoint for DVB mode and transfer type 1
> [6.829283] em28xx: Failed to pre-allocate USB transfer buffers for DVB.
> [6.839454] em28xx: probe of 1-1.3.1:1.0 failed with error -22
> [6.852511] usbcore: registered new interface driver em28xx
> [7.255738] em28xx #0 em28xx_accumulator_set :em28xx Scale: (1,1)-(179,143)
> [7.291575] em28xx #0 em28xx_capture_area_set :em28xx Area Set: (180,144)
> [7.326200] em28xx #0 em28xx_uninit_usb_xfer :em28xx: called 
> em28xx_uninit_usb_xfer in mode 1
>
> Is the Nanostick 290e just fundamentally incompatible with bulk
> transfers, or is there hope yet?

It seems like your device has no bulk endpoint for DVB.
What does lsusb say ?

The module parameter is called prefer_bulk, but what it actually does is
"force bulk" (which doesn't make much sense when the device has no bulk
endpoints).
I will fix this in v2 of the patch series.

> It works great with isochronous transfers on my PC and the Fedora
> kernel, but the Raspberry USB host blows up when trying to do
> isochronous mode.

Is this a regression caused by patches or a general issue with the
Raspberry board ?

Regards,
Frank

> /Benny


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-31 Thread Ezequiel Garcia
On Tue, Oct 30, 2012 at 10:39 PM, Benny Amorsen  wrote:
> Frank Schäfer  writes:
>
>> This patch series adds support for USB bulk transfers to the em28xx driver.
>
> I tried these patches on my Raspberry Pi, 3.6.1 kernel, Nanostick 290e
>
> options em28xx prefer_bulk=1 core_debug=1 usb_debug=1
> options em28xx_dvb debug=1
>
> [5.469510] em28xx: New device PCTV Systems PCTV 290e @ 480 Mbps 
> (2013:024f, interface 0, class 0)
> [5.890637] em28xx: DVB interface 0 found
> [6.025292] em28xx #0: chip ID is em28174
> [6.515383] em28xx #0: Identified as PCTV nanoStick T2 290e (card=78)
> [6.567066] em28xx #0: v4l2 driver version 0.1.3
> [6.614720] em28xx #0 em28xx_set_alternate :minimum isoc packet size: 2888 
> (alt=0)
> [6.663064] em28xx #0 em28xx_set_alternate :setting alternate 0 with 
> wMaxPacketSize=0
> [6.715934] em28xx #0 em28xx_accumulator_set :em28xx Scale: (1,1)-(179,143)
> [6.765694] em28xx #0 em28xx_capture_area_set :em28xx Area Set: (180,144)
> [6.793060] em28xx #0: V4L2 video device registered as video0
> [6.808200] em28xx #0 em28xx_alloc_urbs :em28xx: called em28xx_alloc_isoc 
> in mode 2
> [6.819456] em28xx #0: no endpoint for DVB mode and transfer type 1
> [6.829283] em28xx: Failed to pre-allocate USB transfer buffers for DVB.
> [6.839454] em28xx: probe of 1-1.3.1:1.0 failed with error -22
> [6.852511] usbcore: registered new interface driver em28xx
> [7.255738] em28xx #0 em28xx_accumulator_set :em28xx Scale: (1,1)-(179,143)
> [7.291575] em28xx #0 em28xx_capture_area_set :em28xx Area Set: (180,144)
> [7.326200] em28xx #0 em28xx_uninit_usb_xfer :em28xx: called 
> em28xx_uninit_usb_xfer in mode 1
>
> Is the Nanostick 290e just fundamentally incompatible with bulk
> transfers, or is there hope yet?
>
> It works great with isochronous transfers on my PC and the Fedora
> kernel, but the Raspberry USB host blows up when trying to do
> isochronous mode.
>
>

Isn't this completely OT?

Anyway, RPI has known issues regarding USB bandwidth.

See here

https://github.com/ezequielgarcia/stk1160-standalone/issues/8
https://github.com/ezequielgarcia/stk1160-standalone/issues/2
http://www.raspberrypi.org/phpBB3/viewtopic.php?p=196918#p196918

Regards,

Ezequiel
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-30 Thread Benny Amorsen
Frank Schäfer  writes:

> This patch series adds support for USB bulk transfers to the em28xx driver.

I tried these patches on my Raspberry Pi, 3.6.1 kernel, Nanostick 290e

options em28xx prefer_bulk=1 core_debug=1 usb_debug=1
options em28xx_dvb debug=1

[5.469510] em28xx: New device PCTV Systems PCTV 290e @ 480 Mbps (2013:024f, 
interface 0, class 0)
[5.890637] em28xx: DVB interface 0 found
[6.025292] em28xx #0: chip ID is em28174
[6.515383] em28xx #0: Identified as PCTV nanoStick T2 290e (card=78)
[6.567066] em28xx #0: v4l2 driver version 0.1.3
[6.614720] em28xx #0 em28xx_set_alternate :minimum isoc packet size: 2888 
(alt=0)
[6.663064] em28xx #0 em28xx_set_alternate :setting alternate 0 with 
wMaxPacketSize=0
[6.715934] em28xx #0 em28xx_accumulator_set :em28xx Scale: (1,1)-(179,143)
[6.765694] em28xx #0 em28xx_capture_area_set :em28xx Area Set: (180,144)
[6.793060] em28xx #0: V4L2 video device registered as video0
[6.808200] em28xx #0 em28xx_alloc_urbs :em28xx: called em28xx_alloc_isoc in 
mode 2
[6.819456] em28xx #0: no endpoint for DVB mode and transfer type 1
[6.829283] em28xx: Failed to pre-allocate USB transfer buffers for DVB.
[6.839454] em28xx: probe of 1-1.3.1:1.0 failed with error -22
[6.852511] usbcore: registered new interface driver em28xx
[7.255738] em28xx #0 em28xx_accumulator_set :em28xx Scale: (1,1)-(179,143)
[7.291575] em28xx #0 em28xx_capture_area_set :em28xx Area Set: (180,144)
[7.326200] em28xx #0 em28xx_uninit_usb_xfer :em28xx: called 
em28xx_uninit_usb_xfer in mode 1

Is the Nanostick 290e just fundamentally incompatible with bulk
transfers, or is there hope yet?

It works great with isochronous transfers on my PC and the Fedora
kernel, but the Raspberry USB host blows up when trying to do
isochronous mode.


/Benny
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-30 Thread Frank Schäfer
Am 30.10.2012 15:08, schrieb Devin Heitmueller:
> On Tue, Oct 30, 2012 at 12:06 AM, Mauro Carvalho Chehab
>  wrote:
>> Did a git bisect. The last patch where the bug doesn't occur is this
>> changeset:
>> em28xx: add module parameter for selection of the preferred USB 
>> transfer type
>>
>> That means that this changeset broke it:
>>
>> em28xx: use common urb data copying function for vbi and non-vbi 
>> devices
> Oh good, when I saw the subject line for that patch, I got worried.
> Looking at the patch, it seems like he just calls the VBI version for
> both cases assuming the VBI version is a complete superset of the
> non-VBI version, which it is clearly not.

Devin, I've read the code carefully and both functions are basically
doing the same, except that the non-VBI-version bails out when it
detects a VBI-header (see the FIXME statement ;) ).
Btw: the header checks at the end of the non-VBI-version are incomplete,
which corrupts frames from time to time when using bulk transfers.
Fixing this would make both functions even more similar.

> That whole patch should just be reverted.  If he's going to spend the
> time to refactor the code to allow the VBI version to be used for both
> then fine, but blindly calling the VBI version without making real
> code changes is *NOT* going to work.

That's all plain wrong.
a) Nothing needs to be reverted, because these patches are not yet in
the repo.
b) I'm doing nothing blindly here, I tested the patch carefully and it
works with Silvercrest webcam (em2820).
c) the patch not just calls the VBI-version of the function, it also
fixes/improves some issues. And (as mentioned above), it does a few
things better than non-VBI-version, so I don't need to fix the latter.

> Frank, good job in naming your patch - it made me scream "WAIT!" when
> I saw it.

Ehm... ??? That sounded a bit different in your previous mail...

Citation:
"This is generally good stuff. When I originally added the VBI support,
I kept the URB handlers separate initially to reduce the risk of breaking
existing devices, and always assumed that at some point the two routines
would be merged."
> Bad job for blindly submitting a code change without any
> idea whether it actually worked.  ;-)

See above, it's plain wrong.

> I know developers have the tendency to look at code and say "oh,
> that's ugly, I should change that."  However it's more important that
> it actually work than it be pretty.

I agree.
And some developers have the tendency to just add tons of very similar
new code which is basically doing the same just because it saves time. ;)
But thinking about things a few minutes longer / doing some more testing
at the beginning saves much more time in the long run...

Devin, your whole message really isn't helpfull.
Instead of making lots of wrong assertions, it would be nice to get some
constructive comments regarding the code changes or hardware behavior.

Regards,
Frank

> Devin
>


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-30 Thread Frank Schäfer
Am 30.10.2012 06:06, schrieb Mauro Carvalho Chehab:


> Did a git bisect. The last patch where the bug doesn't occur is this 
> changeset:
>   em28xx: add module parameter for selection of the preferred USB 
> transfer type
>
> That means that this changeset broke it:
>
>   em28xx: use common urb data copying function for vbi and non-vbi devices
Ok, thanks.
That means we are VERY close...

I think this is the only change that could cause the trouble:
> @@ -599,6 +491,7 @@ static inline int em28xx_urb_data_copy_vbi(struct em28xx 
> *dev, struct urb *urb)
>   len = actual_length - 4;
>   } else if (p[0] == 0x22 && p[1] == 0x5a) {
>   /* start video */
> + dev->capture_type = 1;
>   p += 4;
>   len = actual_length - 4;
>   } else {

Could you try again with this line commented out ? (em28xx-video.c, line
494 in the patched file).
usb_debug=1 would be usefull, too.

> I didn't test them with my Silvercrest webcam yet.

I re-tested 5 minutes ago with this device and it works fine.
Btw, which frame rates do you get  ? ;)

Regards,
Frank

> I hope that helps.
>
> Regards,
> Mauro
>
> PS.: Logs of the latest working driver is enclosed.
>
>
> [ 4658.112071] media: Linux media interface: v0.10
> [ 4658.118615] Linux video capture interface: v2.00
> [ 4658.123229] WARNING: You are using an experimental version of the media 
> stack.
>   As the driver is backported to an older kernel, it doesn't offer
>   enough quality for its usage in production.
>   Use it with care.
> Latest git patches (needed if you report a bug to 
> linux-media@vger.kernel.org):
>   d259aec2578fe66c43ae89ef65735fa7b70fa948 [media] em28xx: add module 
> parameter for selection of the preferred USB transfer type
>   2c930ac7e1aa0c8181d5d283e4cb5de69b8121d5 [media] em28xx: improve USB 
> endpoint logic, also use bulk transfers
>   6b43cf6a235d564487fd10d73d8c377d4d6bafff [media] em28xx: set USB 
> alternate settings for analog video bulk transfers properly
> [ 4658.192384] em28xx: New device  WinTV HVR-980 @ 480 Mbps (2040:6513, 
> interface 0, class 0)
> [ 4658.200654] em28xx: Audio Vendor Class interface 0 found
> [ 4658.205957] em28xx: Video interface 0 found
> [ 4658.210133] em28xx: DVB interface 0 found
> [ 4658.214178] em28xx #0: chip ID is em2882/em2883
> [ 4658.361126] em28xx #0: i2c eeprom 00: 1a eb 67 95 40 20 13 65 d0 12 5c 03 
> 82 1e 6a 18
> [ 4658.369192] em28xx #0: i2c eeprom 10: 00 00 24 57 66 07 01 00 00 00 00 00 
> 00 00 00 00
> [ 4658.377234] em28xx #0: i2c eeprom 20: 46 00 01 00 f0 10 02 00 b8 00 00 00 
> 5b 1c 00 00
> [ 4658.385334] em28xx #0: i2c eeprom 30: 00 00 20 40 20 80 02 20 01 01 01 01 
> 00 00 00 00
> [ 4658.393444] em28xx #0: i2c eeprom 40: 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00 00 00
> [ 4658.401638] em28xx #0: i2c eeprom 50: 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00 00 00
> [ 4658.409788] em28xx #0: i2c eeprom 60: 00 00 00 00 00 00 00 00 00 00 18 03 
> 34 00 30 00
> [ 4658.417909] em28xx #0: i2c eeprom 70: 32 00 38 00 34 00 34 00 39 00 30 00 
> 31 00 38 00
> [ 4658.425992] em28xx #0: i2c eeprom 80: 00 00 1e 03 57 00 69 00 6e 00 54 00 
> 56 00 20 00
> [ 4658.434026] em28xx #0: i2c eeprom 90: 48 00 56 00 52 00 2d 00 39 00 38 00 
> 30 00 00 00
> [ 4658.442100] em28xx #0: i2c eeprom a0: 84 12 00 00 05 50 1a 7f d4 78 23 b1 
> fe d0 18 85
> [ 4658.450227] em28xx #0: i2c eeprom b0: ff 00 00 00 04 84 0a 00 01 01 20 77 
> 00 40 fa 40
> [ 4658.458311] em28xx #0: i2c eeprom c0: 1d f0 74 02 01 00 01 79 4f 00 00 00 
> 00 00 00 00
> [ 4658.466409] em28xx #0: i2c eeprom d0: 84 12 00 00 05 50 1a 7f d4 78 23 b1 
> fe d0 18 85
> [ 4658.474500] em28xx #0: i2c eeprom e0: ff 00 00 00 04 84 0a 00 01 01 20 77 
> 00 40 fa 40
> [ 4658.482585] em28xx #0: i2c eeprom f0: 1d f0 74 02 01 00 01 79 4f 00 00 00 
> 00 00 00 00
> [ 4658.490566] em28xx #0: EEPROM ID= 0x9567eb1a, EEPROM hash = 0x994b2bdd
> [ 4658.497081] em28xx #0: EEPROM info:
> [ 4658.500564] em28xx #0: AC97 audio (5 sample rates)
> [ 4658.505346] em28xx #0: 500mA max power
> [ 4658.509087] em28xx #0: Table at 0x24, strings=0x1e82, 0x186a, 0x
> [ 4658.515433] em28xx #0: Identified as Hauppauge WinTV HVR 950 (card=16)
> [ 4658.525141] tveeprom 3-0050: Hauppauge model 65201, rev A1C0, serial# 
> 1917178
> [ 4658.532283] tveeprom 3-0050: tuner model is Xceive XC3028 (idx 120, type 
> 71)
> [ 4658.539312] tveeprom 3-0050: TV standards PAL(B/G) PAL(I) PAL(D/D1/K) 
> ATSC/DVB Digital (eeprom 0xd4)
> [ 4658.548426] tveeprom 3-0050: audio processor is None (idx 0)
> [ 4658.554069] tveeprom 3-0050: has radio
> [ 4658.606748] tvp5150 3-005c: chip found @ 0xb8 (em28xx #0)
> [ 4658.612155] tvp5150 3-005c: tvp5150am1 detected.
> [ 4658.636054] tuner 3-0061: Tuner -1 found with type(s) Radio TV.
> [ 4658.643381] xc2028: Xcv2028/3028 init called!
> [ 4658.643384] xc2028 3-0061: creating new instance
> [ 4658.647993] xc

Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-30 Thread Mauro Carvalho Chehab
Em Tue, 30 Oct 2012 09:08:15 -0400
Devin Heitmueller  escreveu:

> On Tue, Oct 30, 2012 at 12:06 AM, Mauro Carvalho Chehab
>  wrote:
> > Did a git bisect. The last patch where the bug doesn't occur is this
> > changeset:
> > em28xx: add module parameter for selection of the preferred USB 
> > transfer type
> >
> > That means that this changeset broke it:
> >
> > em28xx: use common urb data copying function for vbi and non-vbi 
> > devices
> 
> Oh good, when I saw the subject line for that patch, I got worried.
> Looking at the patch, it seems like he just calls the VBI version for
> both cases assuming the VBI version is a complete superset of the
> non-VBI version, which it is clearly not.
> 
> That whole patch should just be reverted. 

I didn't apply Frank's patch yet. So, no need to revert ;) Also, the test
is still incomplete, as I didn't check if the existing webcam support is
still working. I also didn't review each individual changes (although,
on a quick glance, except for those two final patches, nothing bad popped-up). 

I'll likely finish testing and will review it only after my return
back from ELCE.

> If he's going to spend the
> time to refactor the code to allow the VBI version to be used for both
> then fine, but blindly calling the VBI version without making real
> code changes is *NOT* going to work.

Yes.

> Frank, good job in naming your patch - it made me scream "WAIT!" when
> I saw it.  Bad job for blindly submitting a code change without any
> idea whether it actually worked.  ;-)
> 
> I know developers have the tendency to look at code and say "oh,
> that's ugly, I should change that."  However it's more important that
> it actually work than it be pretty.
> 
> Devin
> 


-- 
Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-30 Thread Devin Heitmueller
On Tue, Oct 30, 2012 at 12:06 AM, Mauro Carvalho Chehab
 wrote:
> Did a git bisect. The last patch where the bug doesn't occur is this
> changeset:
> em28xx: add module parameter for selection of the preferred USB 
> transfer type
>
> That means that this changeset broke it:
>
> em28xx: use common urb data copying function for vbi and non-vbi 
> devices

Oh good, when I saw the subject line for that patch, I got worried.
Looking at the patch, it seems like he just calls the VBI version for
both cases assuming the VBI version is a complete superset of the
non-VBI version, which it is clearly not.

That whole patch should just be reverted.  If he's going to spend the
time to refactor the code to allow the VBI version to be used for both
then fine, but blindly calling the VBI version without making real
code changes is *NOT* going to work.

Frank, good job in naming your patch - it made me scream "WAIT!" when
I saw it.  Bad job for blindly submitting a code change without any
idea whether it actually worked.  ;-)

I know developers have the tendency to look at code and say "oh,
that's ugly, I should change that."  However it's more important that
it actually work than it be pretty.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-29 Thread Mauro Carvalho Chehab
Em Tue, 30 Oct 2012 01:00:12 -0200
Mauro Carvalho Chehab  escreveu:

> Em Mon, 29 Oct 2012 23:14:55 +0200
> Frank Schäfer  escreveu:
> 
> > Am 29.10.2012 22:03, schrieb Mauro Carvalho Chehab:
> > > Em Mon, 29 Oct 2012 17:33:12 +0200
> > > Frank Schäfer  escreveu:
> > >
> > >> Am 28.10.2012 21:57, schrieb Mauro Carvalho Chehab:
> > >>> Em Sun, 21 Oct 2012 19:52:05 +0300
> > >>> Frank Schäfer  escreveu:
> > >>>
> >  This patch series adds support for USB bulk transfers to the em28xx 
> >  driver.
> > 
> >  Patch 1 is a bugfix for the image data processing with non-interlaced 
> >  devices (webcams)
> >  that should be considered for stable (see commit message).
> > 
> >  Patches 2-21 extend the driver to support USB bulk transfers.
> >  USB endpoint mapping had to be extended and is a bit tricky.
> >  It might still not be sufficient to handle ALL isoc/bulk endpoints of 
> >  ALL existing devices,
> >  but it should work with the devices we have seen so far and (most 
> >  important !) 
> >  preserves backwards compatibility to the current driver behavior.
> >  Isoc endpoints/transfers are preffered by default, patch 21 adds a 
> >  module parameter to change this behavior.
> > 
> >  The last two patches are follow-up patches not really related to USB 
> >  tranfers.
> >  Patch 22 reduces the code size in em28xx-video by merging the two URB 
> >  data processing functions 
> >  and patch 23 enables VBI-support for em2840-devices.
> > 
> >  Please note that I could test the changes with an analog 
> >  non-interlaced non-VBI device only !
> >  So further tests with DVB/interlaced/VBI devices are strongly 
> >  recommended !
> > >>> Did a quick test here with all applied, with analog TV with xawtv and 
> > >>> tvtime. 
> > >>> Didn't work.
> > >> Ok, thanks for testing.
> > >>
> > >>> I'll need to postpone it, until I have more time to double check it and 
> > >>> bisect.
> > >> I would also need further informations about the test you've made (did
> > >> you enable bulk ?) and the device you used (supports VBI ?).
> > > I used a WinTV HVR-950/980. Logs enclosed.
> > >
> > > Regards,
> > > Mauro
> > 
> > Thanks.
> > Did you load the module with prefer_bulk=1 ?
> 
> No.
> 
> > You just started xawtv/tvtime but got no picture, right ?
> 
> Yes. I tested also v4l2grab. No frames got returned there.
> 
> > 
> > There is nothing unusual in the log, except...
> > 
> > ...
> > > [ 8412.464698] xc2028 3-0061: Can't find firmware for type=BASE INIT1 
> > > F8MHZ MTS (4007), id .
> > ...
> > > [ 8412.464709] xc2028 3-0061: Can't find firmware for type=BASE INIT1 MTS 
> > > (4005), id .
> > ...
> 
> Those are normal. AFAIKT, only firmwares < version 2.0 had init broken into
> INIT0 and INIT1.
> 
> > > [ 8412.490804] xc2028 3-0061: Can't find firmware for type=MTS SCODE 
> > > (2004), id 00010007.
> 
> This is ok, as another similar SCODE firmware got loaded.
> 
> > 
> > and
> > 
> > ...
> > > [ 8454.966006] xc2028 3-0061: xc2028_get_reg 0002 called
> > > [ 8454.990113] xc2028 3-0061: i2c input error: rc = -19 (should be 2)
> > > [ 8454.996282] xc2028 3-0061: xc2028_signal called
> > > [ 8454.997656] xc2028 3-0061: xc2028_get_reg 0002 called
> > > [ 8455.021846] xc2028 3-0061: i2c input error: rc = -19 (should be 2)
> 
> Those are weird. In any case, as far as I remember, even if xc3028 is bad 
> tuned,
> em28xx should be outputing some data. I suspect, however, that those errors
> maybe because the em28xx chip has died already.
> 
> > 
> > Are these errors normal ?
> > Are you sure the device is working properly without my patches ?
> 
> Yes. See below.
> 
> > 
> > You could try to load the em28xx module with usb_debug=1.
> 
> I'll post it on a separate email.
> 

Did a git bisect. The last patch where the bug doesn't occur is this 
changeset:
em28xx: add module parameter for selection of the preferred USB 
transfer type

That means that this changeset broke it:

em28xx: use common urb data copying function for vbi and non-vbi devices

I didn't test them with my Silvercrest webcam yet.

I hope that helps.

Regards,
Mauro

PS.: Logs of the latest working driver is enclosed.


[ 4658.112071] media: Linux media interface: v0.10
[ 4658.118615] Linux video capture interface: v2.00
[ 4658.123229] WARNING: You are using an experimental version of the media 
stack.
As the driver is backported to an older kernel, it doesn't offer
enough quality for its usage in production.
Use it with care.
Latest git patches (needed if you report a bug to linux-media@vger.kernel.org):
d259aec2578fe66c43ae89ef65735fa7b70fa948 [media] em28xx: add module 
parameter for selection of the preferred USB transfer type
2c930ac7e1aa0c8181d5d283e4cb5de69b8121d5 [media] em28xx: improve USB 
endpoint logic, also use bulk transfers
6b43cf6a235d564

Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-29 Thread Mauro Carvalho Chehab
Em Mon, 29 Oct 2012 23:14:55 +0200
Frank Schäfer  escreveu:

> Am 29.10.2012 22:03, schrieb Mauro Carvalho Chehab:
> > Em Mon, 29 Oct 2012 17:33:12 +0200
> > Frank Schäfer  escreveu:
> >
> >> Am 28.10.2012 21:57, schrieb Mauro Carvalho Chehab:
> >>> Em Sun, 21 Oct 2012 19:52:05 +0300
> >>> Frank Schäfer  escreveu:
> >>>
>  This patch series adds support for USB bulk transfers to the em28xx 
>  driver.
> 
>  Patch 1 is a bugfix for the image data processing with non-interlaced 
>  devices (webcams)
>  that should be considered for stable (see commit message).
> 
>  Patches 2-21 extend the driver to support USB bulk transfers.
>  USB endpoint mapping had to be extended and is a bit tricky.
>  It might still not be sufficient to handle ALL isoc/bulk endpoints of 
>  ALL existing devices,
>  but it should work with the devices we have seen so far and (most 
>  important !) 
>  preserves backwards compatibility to the current driver behavior.
>  Isoc endpoints/transfers are preffered by default, patch 21 adds a 
>  module parameter to change this behavior.
> 
>  The last two patches are follow-up patches not really related to USB 
>  tranfers.
>  Patch 22 reduces the code size in em28xx-video by merging the two URB 
>  data processing functions 
>  and patch 23 enables VBI-support for em2840-devices.
> 
>  Please note that I could test the changes with an analog non-interlaced 
>  non-VBI device only !
>  So further tests with DVB/interlaced/VBI devices are strongly 
>  recommended !
> >>> Did a quick test here with all applied, with analog TV with xawtv and 
> >>> tvtime. 
> >>> Didn't work.
> >> Ok, thanks for testing.
> >>
> >>> I'll need to postpone it, until I have more time to double check it and 
> >>> bisect.
> >> I would also need further informations about the test you've made (did
> >> you enable bulk ?) and the device you used (supports VBI ?).
> > I used a WinTV HVR-950/980. Logs enclosed.
> >
> > Regards,
> > Mauro
> 
> Thanks.
> Did you load the module with prefer_bulk=1 ?

No.

> You just started xawtv/tvtime but got no picture, right ?

Yes. I tested also v4l2grab. No frames got returned there.

> 
> There is nothing unusual in the log, except...
> 
> ...
> > [ 8412.464698] xc2028 3-0061: Can't find firmware for type=BASE INIT1 F8MHZ 
> > MTS (4007), id .
> ...
> > [ 8412.464709] xc2028 3-0061: Can't find firmware for type=BASE INIT1 MTS 
> > (4005), id .
> ...

Those are normal. AFAIKT, only firmwares < version 2.0 had init broken into
INIT0 and INIT1.

> > [ 8412.490804] xc2028 3-0061: Can't find firmware for type=MTS SCODE 
> > (2004), id 00010007.

This is ok, as another similar SCODE firmware got loaded.

> 
> and
> 
> ...
> > [ 8454.966006] xc2028 3-0061: xc2028_get_reg 0002 called
> > [ 8454.990113] xc2028 3-0061: i2c input error: rc = -19 (should be 2)
> > [ 8454.996282] xc2028 3-0061: xc2028_signal called
> > [ 8454.997656] xc2028 3-0061: xc2028_get_reg 0002 called
> > [ 8455.021846] xc2028 3-0061: i2c input error: rc = -19 (should be 2)

Those are weird. In any case, as far as I remember, even if xc3028 is bad tuned,
em28xx should be outputing some data. I suspect, however, that those errors
maybe because the em28xx chip has died already.

> 
> Are these errors normal ?
> Are you sure the device is working properly without my patches ?

Yes. See below.

> 
> You could try to load the em28xx module with usb_debug=1.

I'll post it on a separate email.

> 
> Regards,
> Frank
> 
> 

dmesg without your patches:

[  729.964324] Linux video capture interface: v2.00
[  729.968927] WARNING: You are using an experimental version of the media 
stack.
As the driver is backported to an older kernel, it doesn't offer
enough quality for its usage in production.
Use it with care.
Latest git patches (needed if you report a bug to linux-media@vger.kernel.org):
8e216e50ddca0550ffd477ce27e843a506b3ae2e [media] it913x [BUG] Enable 
endpoint 3 on devices with HID interface
684259353666b05a148cc70dfeed8e699daedbcd [media] Add Fujitsu Siemens 
Amilo Pi 2530 to gspca upside down table
a66cd0b691c730ed751dbf66ffbd0edf18241790 [media] winbond-cir: do not 
rename input name
[  730.034451] em28xx: New device  WinTV HVR-980 @ 480 Mbps (2040:6513, 
interface 0, class 0)
[  730.042715] em28xx: Audio Vendor Class interface 0 found
[  730.048033] em28xx: Video interface 0 found
[  730.052225] em28xx: DVB interface 0 found
[  730.056320] em28xx #0: chip ID is em2882/em2883
[  730.202750] em28xx #0: i2c eeprom 00: 1a eb 67 95 40 20 13 65 d0 12 5c 03 82 
1e 6a 18
[  730.210868] em28xx #0: i2c eeprom 10: 00 00 24 57 66 07 01 00 00 00 00 00 00 
00 00 00
[  730.218848] em28xx #0: i2c eeprom 20: 46 00 01 00 f0 10 02 00 b8 00 00 00 5b 
1c 00 00
[  730.226821] em28xx #0: i2c eeprom 30: 00 00 20 40 20 80 02 20 01 01 01 01 00 
00 00

Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-29 Thread Frank Schäfer
Am 29.10.2012 22:03, schrieb Mauro Carvalho Chehab:
> Em Mon, 29 Oct 2012 17:33:12 +0200
> Frank Schäfer  escreveu:
>
>> Am 28.10.2012 21:57, schrieb Mauro Carvalho Chehab:
>>> Em Sun, 21 Oct 2012 19:52:05 +0300
>>> Frank Schäfer  escreveu:
>>>
 This patch series adds support for USB bulk transfers to the em28xx driver.

 Patch 1 is a bugfix for the image data processing with non-interlaced 
 devices (webcams)
 that should be considered for stable (see commit message).

 Patches 2-21 extend the driver to support USB bulk transfers.
 USB endpoint mapping had to be extended and is a bit tricky.
 It might still not be sufficient to handle ALL isoc/bulk endpoints of ALL 
 existing devices,
 but it should work with the devices we have seen so far and (most 
 important !) 
 preserves backwards compatibility to the current driver behavior.
 Isoc endpoints/transfers are preffered by default, patch 21 adds a module 
 parameter to change this behavior.

 The last two patches are follow-up patches not really related to USB 
 tranfers.
 Patch 22 reduces the code size in em28xx-video by merging the two URB data 
 processing functions 
 and patch 23 enables VBI-support for em2840-devices.

 Please note that I could test the changes with an analog non-interlaced 
 non-VBI device only !
 So further tests with DVB/interlaced/VBI devices are strongly recommended !
>>> Did a quick test here with all applied, with analog TV with xawtv and 
>>> tvtime. 
>>> Didn't work.
>> Ok, thanks for testing.
>>
>>> I'll need to postpone it, until I have more time to double check it and 
>>> bisect.
>> I would also need further informations about the test you've made (did
>> you enable bulk ?) and the device you used (supports VBI ?).
> I used a WinTV HVR-950/980. Logs enclosed.
>
> Regards,
> Mauro

Thanks.
Did you load the module with prefer_bulk=1 ?
You just started xawtv/tvtime but got no picture, right ?

There is nothing unusual in the log, except...

...
> [ 8412.464698] xc2028 3-0061: Can't find firmware for type=BASE INIT1 F8MHZ 
> MTS (4007), id .
...
> [ 8412.464709] xc2028 3-0061: Can't find firmware for type=BASE INIT1 MTS 
> (4005), id .
...
> [ 8412.490804] xc2028 3-0061: Can't find firmware for type=MTS SCODE 
> (2004), id 00010007.

and

...
> [ 8454.966006] xc2028 3-0061: xc2028_get_reg 0002 called
> [ 8454.990113] xc2028 3-0061: i2c input error: rc = -19 (should be 2)
> [ 8454.996282] xc2028 3-0061: xc2028_signal called
> [ 8454.997656] xc2028 3-0061: xc2028_get_reg 0002 called
> [ 8455.021846] xc2028 3-0061: i2c input error: rc = -19 (should be 2)

Are these errors normal ?
Are you sure the device is working properly without my patches ?

You could try to load the em28xx module with usb_debug=1.

Regards,
Frank


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-29 Thread Mauro Carvalho Chehab
Em Mon, 29 Oct 2012 17:33:12 +0200
Frank Schäfer  escreveu:

> Am 28.10.2012 21:57, schrieb Mauro Carvalho Chehab:
> > Em Sun, 21 Oct 2012 19:52:05 +0300
> > Frank Schäfer  escreveu:
> >
> >> This patch series adds support for USB bulk transfers to the em28xx driver.
> >>
> >> Patch 1 is a bugfix for the image data processing with non-interlaced 
> >> devices (webcams)
> >> that should be considered for stable (see commit message).
> >>
> >> Patches 2-21 extend the driver to support USB bulk transfers.
> >> USB endpoint mapping had to be extended and is a bit tricky.
> >> It might still not be sufficient to handle ALL isoc/bulk endpoints of ALL 
> >> existing devices,
> >> but it should work with the devices we have seen so far and (most 
> >> important !) 
> >> preserves backwards compatibility to the current driver behavior.
> >> Isoc endpoints/transfers are preffered by default, patch 21 adds a module 
> >> parameter to change this behavior.
> >>
> >> The last two patches are follow-up patches not really related to USB 
> >> tranfers.
> >> Patch 22 reduces the code size in em28xx-video by merging the two URB data 
> >> processing functions 
> >> and patch 23 enables VBI-support for em2840-devices.
> >>
> >> Please note that I could test the changes with an analog non-interlaced 
> >> non-VBI device only !
> >> So further tests with DVB/interlaced/VBI devices are strongly recommended !
> > Did a quick test here with all applied, with analog TV with xawtv and 
> > tvtime. 
> > Didn't work.
> 
> Ok, thanks for testing.
> 
> > I'll need to postpone it, until I have more time to double check it and 
> > bisect.
> 
> I would also need further informations about the test you've made (did
> you enable bulk ?) and the device you used (supports VBI ?).

I used a WinTV HVR-950/980. Logs enclosed.

Regards,
Mauro


[ 8410.539167] media: Linux media interface: v0.10
[ 8410.554658] Linux video capture interface: v2.00
[ 8410.559272] WARNING: You are using an experimental version of the media 
stack.
As the driver is backported to an older kernel, it doesn't offer
enough quality for its usage in production.
Use it with care.
Latest git patches (needed if you report a bug to linux-media@vger.kernel.org):
8e216e50ddca0550ffd477ce27e843a506b3ae2e [media] it913x [BUG] Enable 
endpoint 3 on devices with HID interface
684259353666b05a148cc70dfeed8e699daedbcd [media] Add Fujitsu Siemens 
Amilo Pi 2530 to gspca upside down table
a66cd0b691c730ed751dbf66ffbd0edf18241790 [media] winbond-cir: do not 
rename input name
[ 8410.669117] em28xx: New device  WinTV HVR-980 @ 480 Mbps (2040:6513, 
interface 0, class 0)
[ 8410.677360] em28xx: Audio Vendor Class interface 0 found
[ 8410.682652] em28xx: Video interface 0 found
[ 8410.686817] em28xx: DVB interface 0 found
[ 8410.690955] em28xx #0: chip ID is em2882/em2883
[ 8410.837123] em28xx #0: i2c eeprom 00: 1a eb 67 95 40 20 13 65 d0 12 5c 03 82 
1e 6a 18
[ 8410.845092] em28xx #0: i2c eeprom 10: 00 00 24 57 66 07 01 00 00 00 00 00 00 
00 00 00
[ 8410.853063] em28xx #0: i2c eeprom 20: 46 00 01 00 f0 10 02 00 b8 00 00 00 5b 
1c 00 00
[ 8410.861019] em28xx #0: i2c eeprom 30: 00 00 20 40 20 80 02 20 01 01 01 01 00 
00 00 00
[ 8410.868974] em28xx #0: i2c eeprom 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00
[ 8410.876926] em28xx #0: i2c eeprom 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00
[ 8410.884901] em28xx #0: i2c eeprom 60: 00 00 00 00 00 00 00 00 00 00 18 03 34 
00 30 00
[ 8410.892874] em28xx #0: i2c eeprom 70: 32 00 38 00 34 00 34 00 39 00 30 00 31 
00 38 00
[ 8410.900838] em28xx #0: i2c eeprom 80: 00 00 1e 03 57 00 69 00 6e 00 54 00 56 
00 20 00
[ 8410.908791] em28xx #0: i2c eeprom 90: 48 00 56 00 52 00 2d 00 39 00 38 00 30 
00 00 00
[ 8410.916743] em28xx #0: i2c eeprom a0: 84 12 00 00 05 50 1a 7f d4 78 23 b1 fe 
d0 18 85
[ 8410.924695] em28xx #0: i2c eeprom b0: ff 00 00 00 04 84 0a 00 01 01 20 77 00 
40 fa 40
[ 8410.932646] em28xx #0: i2c eeprom c0: 1d f0 74 02 01 00 01 79 4f 00 00 00 00 
00 00 00
[ 8410.940596] em28xx #0: i2c eeprom d0: 84 12 00 00 05 50 1a 7f d4 78 23 b1 fe 
d0 18 85
[ 8410.948549] em28xx #0: i2c eeprom e0: ff 00 00 00 04 84 0a 00 01 01 20 77 00 
40 fa 40
[ 8410.956502] em28xx #0: i2c eeprom f0: 1d f0 74 02 01 00 01 79 4f 00 00 00 00 
00 00 00
[ 8410.964459] em28xx #0: EEPROM ID= 0x9567eb1a, EEPROM hash = 0x994b2bdd
[ 8410.970959] em28xx #0: EEPROM info:
[ 8410.974431] em28xx #0:   AC97 audio (5 sample rates)
[ 8410.979201] em28xx #0:   500mA max power
[ 8410.982948] em28xx #0:   Table at 0x24, strings=0x1e82, 0x186a, 0x
[ 8410.989277] em28xx #0: Identified as Hauppauge WinTV HVR 950 (card=16)
[ 8410.997388] tveeprom 3-0050: Hauppauge model 65201, rev A1C0, serial# 1917178
[ 8411.004502] tveeprom 3-0050: tuner model is Xceive XC3028 (idx 120, type 71)
[ 8411.011522] tveeprom 3-0050: TV standards PAL(B/G) PAL(I) PAL(D/D1/K) 
ATSC/DVB Digital (eeprom 0xd4)
[ 8411.020646] tveeprom 3-0050: a

Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-29 Thread Frank Schäfer
Am 28.10.2012 21:57, schrieb Mauro Carvalho Chehab:
> Em Sun, 21 Oct 2012 19:52:05 +0300
> Frank Schäfer  escreveu:
>
>> This patch series adds support for USB bulk transfers to the em28xx driver.
>>
>> Patch 1 is a bugfix for the image data processing with non-interlaced 
>> devices (webcams)
>> that should be considered for stable (see commit message).
>>
>> Patches 2-21 extend the driver to support USB bulk transfers.
>> USB endpoint mapping had to be extended and is a bit tricky.
>> It might still not be sufficient to handle ALL isoc/bulk endpoints of ALL 
>> existing devices,
>> but it should work with the devices we have seen so far and (most important 
>> !) 
>> preserves backwards compatibility to the current driver behavior.
>> Isoc endpoints/transfers are preffered by default, patch 21 adds a module 
>> parameter to change this behavior.
>>
>> The last two patches are follow-up patches not really related to USB 
>> tranfers.
>> Patch 22 reduces the code size in em28xx-video by merging the two URB data 
>> processing functions 
>> and patch 23 enables VBI-support for em2840-devices.
>>
>> Please note that I could test the changes with an analog non-interlaced 
>> non-VBI device only !
>> So further tests with DVB/interlaced/VBI devices are strongly recommended !
> Did a quick test here with all applied, with analog TV with xawtv and tvtime. 
> Didn't work.

Ok, thanks for testing.

> I'll need to postpone it, until I have more time to double check it and 
> bisect.

I would also need further informations about the test you've made (did
you enable bulk ?) and the device you used (supports VBI ?).

Regards,
Frank

>
> Regards,
> Mauro

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-28 Thread Mauro Carvalho Chehab
Em Sun, 21 Oct 2012 19:52:05 +0300
Frank Schäfer  escreveu:

> This patch series adds support for USB bulk transfers to the em28xx driver.
> 
> Patch 1 is a bugfix for the image data processing with non-interlaced devices 
> (webcams)
> that should be considered for stable (see commit message).
> 
> Patches 2-21 extend the driver to support USB bulk transfers.
> USB endpoint mapping had to be extended and is a bit tricky.
> It might still not be sufficient to handle ALL isoc/bulk endpoints of ALL 
> existing devices,
> but it should work with the devices we have seen so far and (most important 
> !) 
> preserves backwards compatibility to the current driver behavior.
> Isoc endpoints/transfers are preffered by default, patch 21 adds a module 
> parameter to change this behavior.
> 
> The last two patches are follow-up patches not really related to USB tranfers.
> Patch 22 reduces the code size in em28xx-video by merging the two URB data 
> processing functions 
> and patch 23 enables VBI-support for em2840-devices.
> 
> Please note that I could test the changes with an analog non-interlaced 
> non-VBI device only !
> So further tests with DVB/interlaced/VBI devices are strongly recommended !

Did a quick test here with all applied, with analog TV with xawtv and tvtime. 
Didn't work.

I'll need to postpone it, until I have more time to double check it and bisect.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-28 Thread Frank Schäfer
Am 28.10.2012 16:08, schrieb Mauro Carvalho Chehab:
> Em 28-10-2012 11:00, Frank Schäfer escreveu:
>> Am 21.10.2012 19:52, schrieb Frank Schäfer:
>>> This patch series adds support for USB bulk transfers to the em28xx
>>> driver.
>>
>> Mauro,
>> I sent this patches to you because you are listed as the maintainer and
>> it would be nice to get at least a reply from you.
>
> Yeah, they're on my queue. I'll reply about patch specifics when I get
> there.
> There are still some other patches that arrived before yours.
>
> Whenever I touch them, patchwork will notify you.

Ok, thanks, that's all I wanted to know.

>
>> What's you plan with these patches ?
>
> As you asked for tests with VBI, it will require me to have some spare
> time
> for testing, in a timeframe where I could get some TV program with VBI.
> Only a few news programs on one or two channels here transmit VBI.

Maybe my warnings were a bit too loud ;) I just wanted to be honest
about untested paths.
The changes in frame processing are not specific to analog/DVB
interlaced/progressive or VBI/non-VBI.
As they have been tested carefully with an analog/progressive/non-VBI
device, they should work for others, too.
I'm only concerned about further existing bugs that didin't show up as
long as isoc is used only...
OTOH, we keep on using isoc by default, so noone will notice them until
explicitly enabling bulk.

IMO, the most critical change is patch 20. You should review the changes
regarding the endpoint mapping logic carefully.
Maybe you get some ideas how we could improve this in the future without
breaking backwards compatibility.


>
> So, it may take a little for them to get into. If I can't handle today,
> I will likely be able to do it only after my return back from ELCE.

No need to hurry.
For me important to know is, that a) it makes sense to continue working
on this driver and that b) I can base the next patches on this series.

Regards,
Frank

>
>> ATM I'm not sure if it makes sense to continue working (and spending a
>> good amount of my free time) on further changes/extensions.
>> The em25xx stuff you want to have in this driver requires lots of much
>> more complicating changes and without any assistance/coorporation from
>> the linux-media list, it will be impossible to get them upstream.
>>
>> Regards,
>> Frank
>>
>>
>
> Regards,
> Mauro

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-28 Thread Mauro Carvalho Chehab

Em 28-10-2012 11:00, Frank Schäfer escreveu:

Am 21.10.2012 19:52, schrieb Frank Schäfer:

This patch series adds support for USB bulk transfers to the em28xx driver.


Mauro,
I sent this patches to you because you are listed as the maintainer and
it would be nice to get at least a reply from you.


Yeah, they're on my queue. I'll reply about patch specifics when I get there.
There are still some other patches that arrived before yours.

Whenever I touch them, patchwork will notify you.


What's you plan with these patches ?


As you asked for tests with VBI, it will require me to have some spare time
for testing, in a timeframe where I could get some TV program with VBI.
Only a few news programs on one or two channels here transmit VBI.

So, it may take a little for them to get into. If I can't handle today,
I will likely be able to do it only after my return back from ELCE.


ATM I'm not sure if it makes sense to continue working (and spending a
good amount of my free time) on further changes/extensions.
The em25xx stuff you want to have in this driver requires lots of much
more complicating changes and without any assistance/coorporation from
the linux-media list, it will be impossible to get them upstream.

Regards,
Frank




Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-28 Thread Frank Schäfer
Am 21.10.2012 19:52, schrieb Frank Schäfer:
> This patch series adds support for USB bulk transfers to the em28xx driver.
>
> Patch 1 is a bugfix for the image data processing with non-interlaced devices 
> (webcams)
> that should be considered for stable (see commit message).
>
> Patches 2-21 extend the driver to support USB bulk transfers.
> USB endpoint mapping had to be extended and is a bit tricky.
> It might still not be sufficient to handle ALL isoc/bulk endpoints of ALL 
> existing devices,
> but it should work with the devices we have seen so far and (most important 
> !) 
> preserves backwards compatibility to the current driver behavior.
> Isoc endpoints/transfers are preffered by default, patch 21 adds a module 
> parameter to change this behavior.
>
> The last two patches are follow-up patches not really related to USB tranfers.
> Patch 22 reduces the code size in em28xx-video by merging the two URB data 
> processing functions 
> and patch 23 enables VBI-support for em2840-devices.
>
> Please note that I could test the changes with an analog non-interlaced 
> non-VBI device only !
> So further tests with DVB/interlaced/VBI devices are strongly recommended !
>
>
>
>
> Frank Schäfer (23):
>   em28xx: fix wrong data offset for non-interlaced mode in
> em28xx_copy_video
>   em28xx: clarify meaning of field 'progressive' in struct em28xx
>   em28xx: rename isoc packet number constants and parameters
>   em28xx: rename struct em28xx_usb_isoc_bufs to em28xx_usb_bufs
>   em28xx: rename struct em28xx_usb_isoc_ctl to em28xx_usb_ctl
>   em28xx: remove obsolete #define EM28XX_URB_TIMEOUT
>   em28xx: update description of em28xx_irq_callback
>   em28xx: rename function em28xx_uninit_isoc to em28xx_uninit_usb_xfer
>   em28xx: create a common function for isoc and bulk URB allocation and
> setup
>   em28xx: create a common function for isoc and bulk USB transfer
> initialization
>   em28xx: clear USB halt/stall condition in em28xx_init_usb_xfer when
> using bulk transfers
>   em28xx: remove double checks for urb->status == -ENOENT in
> urb_data_copy functions
>   em28xx: rename function em28xx_isoc_copy and extend for USB bulk
> transfers
>   em28xx: rename function em28xx_isoc_copy_vbi and extend for USB bulk
> transfers
>   em28xx: rename function em28xx_dvb_isoc_copy and extend for USB bulk
> transfers
>   em28xx: rename usb debugging module parameter and macro
>   em28xx: rename some USB parameter fields in struct em28xx to clarify
> their role
>   em28xx: add fields for analog and DVB USB transfer type selection to
> struct em28xx
>   em28xx: set USB alternate settings for analog video bulk transfers
> properly
>   em28xx: improve USB endpoint logic, also use bulk transfers
>   em28xx: add module parameter for selection of the preferred USB
> transfer type
>   em28xx: use common urb data copying function for vbi and non-vbi
> devices
>   em28xx: enable VBI-support for em2840 devices
>
>  drivers/media/usb/em28xx/em28xx-cards.c |  114 +---
>  drivers/media/usb/em28xx/em28xx-core.c  |  250 ---
>  drivers/media/usb/em28xx/em28xx-dvb.c   |   85 ++---
>  drivers/media/usb/em28xx/em28xx-reg.h   |4 +-
>  drivers/media/usb/em28xx/em28xx-vbi.c   |4 +-
>  drivers/media/usb/em28xx/em28xx-video.c |  288 
> +++
>  drivers/media/usb/em28xx/em28xx.h   |   78 +
>  7 Dateien geändert, 452 Zeilen hinzugefügt(+), 371 Zeilen entfernt(-)

Mauro,
I sent this patches to you because you are listed as the maintainer and
it would be nice to get at least a reply from you.

What's you plan with these patches ?

ATM I'm not sure if it makes sense to continue working (and spending a
good amount of my free time) on further changes/extensions.
The em25xx stuff you want to have in this driver requires lots of much
more complicating changes and without any assistance/coorporation from
the linux-media list, it will be impossible to get them upstream.

Regards,
Frank


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-23 Thread Frank Schäfer
Hi,

Am 21.10.2012 21:13, schrieb Devin Heitmueller:
> Hi Frank,
>
> On Sun, Oct 21, 2012 at 12:52 PM, Frank Schäfer
>  wrote:
>> This patch series adds support for USB bulk transfers to the em28xx driver.
> This is a welcome change that some users have been asking about for a while.

Yes, I know...

>
>> Patch 1 is a bugfix for the image data processing with non-interlaced 
>> devices (webcams)
>> that should be considered for stable (see commit message).
>>
>> Patches 2-21 extend the driver to support USB bulk transfers.
>> USB endpoint mapping had to be extended and is a bit tricky.
>> It might still not be sufficient to handle ALL isoc/bulk endpoints of ALL 
>> existing devices,
>> but it should work with the devices we have seen so far and (most important 
>> !)
>> preserves backwards compatibility to the current driver behavior.
>> Isoc endpoints/transfers are preffered by default, patch 21 adds a module 
>> parameter to change this behavior.
>>
>> The last two patches are follow-up patches not really related to USB 
>> tranfers.
>> Patch 22 reduces the code size in em28xx-video by merging the two URB data 
>> processing functions
> This is generally good stuff.  When I originally added the VBI
> support, I kept the URB handlers separate initially to reduce the risk
> of breaking existing devices, and always assumed that at some point
> the two routines would be merged.  You did regression test without VBI
> support enabled though, right?

Yes, but when you take a look at the code, you will see that this patch
nothing really changes for VBI devices.
The problem / regression potential is the non-VBI-devices as they are
now using the VBI-version, too, but they have been tested.
Btw, why didn't you test this function with VBI disabled when you added
it ? ;)

>
>> and patch 23 enables VBI-support for em2840-devices.
> Patch 23 shouldn't be applied unless somebody has an em2840 device to
> test with first.  Nobody has complained about this so far, and it's
> better to not support VBI than to possibly break existing support.

Btw, what about em2874 / em2884 / em28174 ?
We should really sort these kind of things out when adding new devices...

>
>> Please note that I could test the changes with an analog non-interlaced 
>> non-VBI device only !
>> So further tests with DVB/interlaced/VBI devices are strongly recommended !
> So here's the problem:  I don't have the cycles to test this, and all
> the refactoring presents a very real risk that breakage of existing
> support could occur.  You've basically got three options if you want
> to see this merged upstream:
>
> 1.  Wait for me to eventually do the testing.
> 2.  Plead for users to do testing, in particular of the VBI support
> for interlaced devices (which is 99% of devices out there)
> 3.  See if Mauro has time to do the testing.

I would say 1 + 2 + 3 ;)
And maybe it's a good chance for the people who were asking for this
feature in the past.

I know there are lots of other people on this list having such a device.

> 4.  Spend $30 and buy one of the dozens of em28xx based analog capture
> devices out there and do the validation yourself (a huge percentage of
> the "Video tape capture devices" are em28xx based.  For example, when
> I did the original VBI work, I used the following:
>
> KWorld DVD Maker USB 2.0 VS- USB2800 USB 2.0 Interface
> http://www.newegg.com/Product/Product.aspx?Item=N82E16815100112
>
> If you're in the United States, I can mail you a device for testing.
> But given how dirt-cheap they are, buying one yourself might be easier
> (and if the money is really the issue, send me your paypal details
> offline and I'll give you the $30.00).

No, thank you. I already have too many devices which I actually don't need.
I'm doing this as a hobby and at the moment, I'm focussed on getting the
devices I already have working properly (which isn't a small task).

I personally don't need this feature uptsream at the moment.
The device I used for testing supports ISOC as well and the em25xx
webcam I'm currently working on will likely use gspca at the end.

> Thanks for you hard work on this, and it will be great to get this
> stuff into the mainline.

I did what I could do. Now its up to others ;)

Regards,
Frank

> Devin
>

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-21 Thread Devin Heitmueller
Hi Frank,

On Sun, Oct 21, 2012 at 12:52 PM, Frank Schäfer
 wrote:
> This patch series adds support for USB bulk transfers to the em28xx driver.

This is a welcome change that some users have been asking about for a while.

> Patch 1 is a bugfix for the image data processing with non-interlaced devices 
> (webcams)
> that should be considered for stable (see commit message).
>
> Patches 2-21 extend the driver to support USB bulk transfers.
> USB endpoint mapping had to be extended and is a bit tricky.
> It might still not be sufficient to handle ALL isoc/bulk endpoints of ALL 
> existing devices,
> but it should work with the devices we have seen so far and (most important !)
> preserves backwards compatibility to the current driver behavior.
> Isoc endpoints/transfers are preffered by default, patch 21 adds a module 
> parameter to change this behavior.
>
> The last two patches are follow-up patches not really related to USB tranfers.
> Patch 22 reduces the code size in em28xx-video by merging the two URB data 
> processing functions

This is generally good stuff.  When I originally added the VBI
support, I kept the URB handlers separate initially to reduce the risk
of breaking existing devices, and always assumed that at some point
the two routines would be merged.  You did regression test without VBI
support enabled though, right?

> and patch 23 enables VBI-support for em2840-devices.

Patch 23 shouldn't be applied unless somebody has an em2840 device to
test with first.  Nobody has complained about this so far, and it's
better to not support VBI than to possibly break existing support.

> Please note that I could test the changes with an analog non-interlaced 
> non-VBI device only !
> So further tests with DVB/interlaced/VBI devices are strongly recommended !

So here's the problem:  I don't have the cycles to test this, and all
the refactoring presents a very real risk that breakage of existing
support could occur.  You've basically got three options if you want
to see this merged upstream:

1.  Wait for me to eventually do the testing.
2.  Plead for users to do testing, in particular of the VBI support
for interlaced devices (which is 99% of devices out there)
3.  See if Mauro has time to do the testing.
4.  Spend $30 and buy one of the dozens of em28xx based analog capture
devices out there and do the validation yourself (a huge percentage of
the "Video tape capture devices" are em28xx based.  For example, when
I did the original VBI work, I used the following:

KWorld DVD Maker USB 2.0 VS- USB2800 USB 2.0 Interface
http://www.newegg.com/Product/Product.aspx?Item=N82E16815100112

If you're in the United States, I can mail you a device for testing.
But given how dirt-cheap they are, buying one yourself might be easier
(and if the money is really the issue, send me your paypal details
offline and I'll give you the $30.00).

Thanks for you hard work on this, and it will be great to get this
stuff into the mainline.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 00/23] em28xx: add support fur USB bulk transfers

2012-10-21 Thread Frank Schäfer
This patch series adds support for USB bulk transfers to the em28xx driver.

Patch 1 is a bugfix for the image data processing with non-interlaced devices 
(webcams)
that should be considered for stable (see commit message).

Patches 2-21 extend the driver to support USB bulk transfers.
USB endpoint mapping had to be extended and is a bit tricky.
It might still not be sufficient to handle ALL isoc/bulk endpoints of ALL 
existing devices,
but it should work with the devices we have seen so far and (most important !) 
preserves backwards compatibility to the current driver behavior.
Isoc endpoints/transfers are preffered by default, patch 21 adds a module 
parameter to change this behavior.

The last two patches are follow-up patches not really related to USB tranfers.
Patch 22 reduces the code size in em28xx-video by merging the two URB data 
processing functions 
and patch 23 enables VBI-support for em2840-devices.

Please note that I could test the changes with an analog non-interlaced non-VBI 
device only !
So further tests with DVB/interlaced/VBI devices are strongly recommended !




Frank Schäfer (23):
  em28xx: fix wrong data offset for non-interlaced mode in
em28xx_copy_video
  em28xx: clarify meaning of field 'progressive' in struct em28xx
  em28xx: rename isoc packet number constants and parameters
  em28xx: rename struct em28xx_usb_isoc_bufs to em28xx_usb_bufs
  em28xx: rename struct em28xx_usb_isoc_ctl to em28xx_usb_ctl
  em28xx: remove obsolete #define EM28XX_URB_TIMEOUT
  em28xx: update description of em28xx_irq_callback
  em28xx: rename function em28xx_uninit_isoc to em28xx_uninit_usb_xfer
  em28xx: create a common function for isoc and bulk URB allocation and
setup
  em28xx: create a common function for isoc and bulk USB transfer
initialization
  em28xx: clear USB halt/stall condition in em28xx_init_usb_xfer when
using bulk transfers
  em28xx: remove double checks for urb->status == -ENOENT in
urb_data_copy functions
  em28xx: rename function em28xx_isoc_copy and extend for USB bulk
transfers
  em28xx: rename function em28xx_isoc_copy_vbi and extend for USB bulk
transfers
  em28xx: rename function em28xx_dvb_isoc_copy and extend for USB bulk
transfers
  em28xx: rename usb debugging module parameter and macro
  em28xx: rename some USB parameter fields in struct em28xx to clarify
their role
  em28xx: add fields for analog and DVB USB transfer type selection to
struct em28xx
  em28xx: set USB alternate settings for analog video bulk transfers
properly
  em28xx: improve USB endpoint logic, also use bulk transfers
  em28xx: add module parameter for selection of the preferred USB
transfer type
  em28xx: use common urb data copying function for vbi and non-vbi
devices
  em28xx: enable VBI-support for em2840 devices

 drivers/media/usb/em28xx/em28xx-cards.c |  114 +---
 drivers/media/usb/em28xx/em28xx-core.c  |  250 ---
 drivers/media/usb/em28xx/em28xx-dvb.c   |   85 ++---
 drivers/media/usb/em28xx/em28xx-reg.h   |4 +-
 drivers/media/usb/em28xx/em28xx-vbi.c   |4 +-
 drivers/media/usb/em28xx/em28xx-video.c |  288 +++
 drivers/media/usb/em28xx/em28xx.h   |   78 +
 7 Dateien geändert, 452 Zeilen hinzugefügt(+), 371 Zeilen entfernt(-)

-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html