diff mbox

[2/9] em28xx: Bulk transfer implementation fix

Message ID 1515110659-20145-3-git-send-email-brad@nextdimension.cc (mailing list archive)
State Superseded, archived
Headers

Commit Message

Brad Love Jan. 5, 2018, 12:04 a.m. UTC 
  Set appropriate bulk/ISOC transfer multiplier on capture start.
This sets ISOC transfer to 940 bytes (188 * 5)
This sets bulk transfer to 48128 bytes (188 * 256)

The above values are maximum allowed according to Empia.

Signed-off-by: Brad Love <brad@nextdimension.cc>
---
 drivers/media/usb/em28xx/em28xx-core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Michael Ira Krufky Jan. 5, 2018, 12:22 a.m. UTC | #1 
On Thu, Jan 4, 2018 at 7:04 PM, Brad Love <brad@nextdimension.cc> wrote:
> Set appropriate bulk/ISOC transfer multiplier on capture start.
> This sets ISOC transfer to 940 bytes (188 * 5)
> This sets bulk transfer to 48128 bytes (188 * 256)
>
> The above values are maximum allowed according to Empia.
>
> Signed-off-by: Brad Love <brad@nextdimension.cc>

:+1

Reviewed-by: Michael Ira Krufky <mkrufky@linuxtv.org>

> ---
>  drivers/media/usb/em28xx/em28xx-core.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
> index ef38e56..67ed6a3 100644
> --- a/drivers/media/usb/em28xx/em28xx-core.c
> +++ b/drivers/media/usb/em28xx/em28xx-core.c
> @@ -638,6 +638,18 @@ int em28xx_capture_start(struct em28xx *dev, int start)
>             dev->chip_id == CHIP_ID_EM28174 ||
>             dev->chip_id == CHIP_ID_EM28178) {
>                 /* The Transport Stream Enable Register moved in em2874 */
> +               if (dev->dvb_xfer_bulk) {
> +                       /* Max Tx Size = 188 * 256 = 48128 - LCM(188,512) * 2 */
> +                       em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
> +                                       EM2874_R5D_TS1_PKT_SIZE :
> +                                       EM2874_R5E_TS2_PKT_SIZE,
> +                                       0xFF);
> +               } else {
> +                       /* TS2 Maximum Transfer Size = 188 * 5 */
> +                       em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
> +                                       EM2874_R5D_TS1_PKT_SIZE :
> +                                       EM2874_R5E_TS2_PKT_SIZE, 0x05);
> +               }
>                 if (dev->ts == PRIMARY_TS)
>                         rc = em28xx_write_reg_bits(dev,
>                                 EM2874_R5F_TS_ENABLE,
> --
> 2.7.4
>
Devin Heitmueller Jan. 5, 2018, 2:20 p.m. UTC | #2 
Hi Brad,

My documents indicate that Register 0x5D and 0x5E are read-only, and
populated based on the eeprom programming.

On your device, what is the value of those registers prior to you changing them?

If you write to those registers, do they reflect the new values if you
read them back?

Does changing these values result in any change to the device's
endpoint configuration (which is typically statically defined when the
device is probed)?

What precisely is the behavior you were seeing prior to this patch?

Devin

On Thu, Jan 4, 2018 at 7:22 PM, Michael Ira Krufky <mkrufky@linuxtv.org> wrote:
> On Thu, Jan 4, 2018 at 7:04 PM, Brad Love <brad@nextdimension.cc> wrote:
>> Set appropriate bulk/ISOC transfer multiplier on capture start.
>> This sets ISOC transfer to 940 bytes (188 * 5)
>> This sets bulk transfer to 48128 bytes (188 * 256)
>>
>> The above values are maximum allowed according to Empia.
>>
>> Signed-off-by: Brad Love <brad@nextdimension.cc>
>
> :+1
>
> Reviewed-by: Michael Ira Krufky <mkrufky@linuxtv.org>
>
>> ---
>>  drivers/media/usb/em28xx/em28xx-core.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
>> index ef38e56..67ed6a3 100644
>> --- a/drivers/media/usb/em28xx/em28xx-core.c
>> +++ b/drivers/media/usb/em28xx/em28xx-core.c
>> @@ -638,6 +638,18 @@ int em28xx_capture_start(struct em28xx *dev, int start)
>>             dev->chip_id == CHIP_ID_EM28174 ||
>>             dev->chip_id == CHIP_ID_EM28178) {
>>                 /* The Transport Stream Enable Register moved in em2874 */
>> +               if (dev->dvb_xfer_bulk) {
>> +                       /* Max Tx Size = 188 * 256 = 48128 - LCM(188,512) * 2 */
>> +                       em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
>> +                                       EM2874_R5D_TS1_PKT_SIZE :
>> +                                       EM2874_R5E_TS2_PKT_SIZE,
>> +                                       0xFF);
>> +               } else {
>> +                       /* TS2 Maximum Transfer Size = 188 * 5 */
>> +                       em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
>> +                                       EM2874_R5D_TS1_PKT_SIZE :
>> +                                       EM2874_R5E_TS2_PKT_SIZE, 0x05);
>> +               }
>>                 if (dev->ts == PRIMARY_TS)
>>                         rc = em28xx_write_reg_bits(dev,
>>                                 EM2874_R5F_TS_ENABLE,
>> --
>> 2.7.4
>>
Brad Love Jan. 5, 2018, 3:21 p.m. UTC | #3 
On 2018-01-05 08:20, Devin Heitmueller wrote:
> Hi Brad,
>
> My documents indicate that Register 0x5D and 0x5E are read-only, and
> populated based on the eeprom programming.
>
> On your device, what is the value of those registers prior to you changing them?
>
> If you write to those registers, do they reflect the new values if you
> read them back?
>
> Does changing these values result in any change to the device's
> endpoint configuration (which is typically statically defined when the
> device is probed)?
>
> What precisely is the behavior you were seeing prior to this patch?
>
> Devin

Hey Devin,

We have devices programmed ISOC and bulk in eeprom, but we were seeing
before that bulk transfers were not happening as expected. This included
continuity errors and corrupted packets. After speaking with Empia they
supplied this patch to configure the multiplier explicitly. They also
suggested changing the usb configuration to match this multiplier. This
is done in patch 3/9. I will add some instrumentation to check out the
data you're looking for though. I can say offhand that modifying those
values does have tangible effects. On 'native' machines, there is little
difference, but the multiplier values are make or break in VMWare.

Will reply with the data later.

Cheers,

Brad



> On Thu, Jan 4, 2018 at 7:22 PM, Michael Ira Krufky <mkrufky@linuxtv.org> wrote:
>> On Thu, Jan 4, 2018 at 7:04 PM, Brad Love <brad@nextdimension.cc> wrote:
>>> Set appropriate bulk/ISOC transfer multiplier on capture start.
>>> This sets ISOC transfer to 940 bytes (188 * 5)
>>> This sets bulk transfer to 48128 bytes (188 * 256)
>>>
>>> The above values are maximum allowed according to Empia.
>>>
>>> Signed-off-by: Brad Love <brad@nextdimension.cc>
>> :+1
>>
>> Reviewed-by: Michael Ira Krufky <mkrufky@linuxtv.org>
>>
>>> ---
>>>  drivers/media/usb/em28xx/em28xx-core.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
>>> index ef38e56..67ed6a3 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-core.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-core.c
>>> @@ -638,6 +638,18 @@ int em28xx_capture_start(struct em28xx *dev, int start)
>>>             dev->chip_id == CHIP_ID_EM28174 ||
>>>             dev->chip_id == CHIP_ID_EM28178) {
>>>                 /* The Transport Stream Enable Register moved in em2874 */
>>> +               if (dev->dvb_xfer_bulk) {
>>> +                       /* Max Tx Size = 188 * 256 = 48128 - LCM(188,512) * 2 */
>>> +                       em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
>>> +                                       EM2874_R5D_TS1_PKT_SIZE :
>>> +                                       EM2874_R5E_TS2_PKT_SIZE,
>>> +                                       0xFF);
>>> +               } else {
>>> +                       /* TS2 Maximum Transfer Size = 188 * 5 */
>>> +                       em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
>>> +                                       EM2874_R5D_TS1_PKT_SIZE :
>>> +                                       EM2874_R5E_TS2_PKT_SIZE, 0x05);
>>> +               }
>>>                 if (dev->ts == PRIMARY_TS)
>>>                         rc = em28xx_write_reg_bits(dev,
>>>                                 EM2874_R5F_TS_ENABLE,
>>> --
>>> 2.7.4
>>>
>
>
Mauro Carvalho Chehab Jan. 30, 2018, 12:07 p.m. UTC | #4 
Em Thu,  4 Jan 2018 18:04:12 -0600
Brad Love <brad@nextdimension.cc> escreveu:

> Set appropriate bulk/ISOC transfer multiplier on capture start.
> This sets ISOC transfer to 940 bytes (188 * 5)
> This sets bulk transfer to 48128 bytes (188 * 256)
> 
> The above values are maximum allowed according to Empia.
> 
> Signed-off-by: Brad Love <brad@nextdimension.cc>
> ---
>  drivers/media/usb/em28xx/em28xx-core.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
> index ef38e56..67ed6a3 100644
> --- a/drivers/media/usb/em28xx/em28xx-core.c
> +++ b/drivers/media/usb/em28xx/em28xx-core.c
> @@ -638,6 +638,18 @@ int em28xx_capture_start(struct em28xx *dev, int start)
>  	    dev->chip_id == CHIP_ID_EM28174 ||
>  	    dev->chip_id == CHIP_ID_EM28178) {
>  		/* The Transport Stream Enable Register moved in em2874 */
> +		if (dev->dvb_xfer_bulk) {
> +			/* Max Tx Size = 188 * 256 = 48128 - LCM(188,512) * 2 */
> +			em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
> +					EM2874_R5D_TS1_PKT_SIZE :
> +					EM2874_R5E_TS2_PKT_SIZE,
> +					0xFF);
> +		} else {
> +			/* TS2 Maximum Transfer Size = 188 * 5 */
> +			em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
> +					EM2874_R5D_TS1_PKT_SIZE :
> +					EM2874_R5E_TS2_PKT_SIZE, 0x05);
> +		}

Hmm... for ISOC, the USB descriptors inform the max transfer size, with
are detected at probe time, on this part of em28xx_usb_probe:

	if (size > dev->dvb_max_pkt_size_isoc) {
		has_dvb = true; /* see NOTE (~) */
		dev->dvb_ep_isoc = e->bEndpointAddress;
		dev->dvb_max_pkt_size_isoc = size;
		dev->dvb_alt_isoc = i;
	}

If we're touching TS PKT size register, it should somehow be
aligned what's there. I mean, we should either do:

			em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
					EM2874_R5D_TS1_PKT_SIZE :
					EM2874_R5E_TS2_PKT_SIZE, dev->dvb_max_pkt_size_isoc / 188);

Or the other way around, setting dev->dvb_max_pkt_size_isoc after
writing to EM2874_R5D_TS1_PKT_SIZE or EM2874_R5E_TS2_PKT_SIZE.

Not sure what's more accurate here: the USB descriptors or the
contents of the TS size register. I doubt, I would stick with
the USB descriptor info.

Btw, I wander what happens if we write a bigger value than 5 to those
registers. Would it support a bigger transfer size than 940 for ISOCH?



Cheers,
Mauro
Brad Love Jan. 30, 2018, 7:33 p.m. UTC | #5 
On 2018-01-30 06:07, Mauro Carvalho Chehab wrote:
> Em Thu,  4 Jan 2018 18:04:12 -0600
> Brad Love <brad@nextdimension.cc> escreveu:
>
>> Set appropriate bulk/ISOC transfer multiplier on capture start.
>> This sets ISOC transfer to 940 bytes (188 * 5)
>> This sets bulk transfer to 48128 bytes (188 * 256)
>>
>> The above values are maximum allowed according to Empia.
>>
>> Signed-off-by: Brad Love <brad@nextdimension.cc>
>> ---
>>  drivers/media/usb/em28xx/em28xx-core.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
>> index ef38e56..67ed6a3 100644
>> --- a/drivers/media/usb/em28xx/em28xx-core.c
>> +++ b/drivers/media/usb/em28xx/em28xx-core.c
>> @@ -638,6 +638,18 @@ int em28xx_capture_start(struct em28xx *dev, int start)
>>  	    dev->chip_id == CHIP_ID_EM28174 ||
>>  	    dev->chip_id == CHIP_ID_EM28178) {
>>  		/* The Transport Stream Enable Register moved in em2874 */
>> +		if (dev->dvb_xfer_bulk) {
>> +			/* Max Tx Size = 188 * 256 = 48128 - LCM(188,512) * 2 */
>> +			em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
>> +					EM2874_R5D_TS1_PKT_SIZE :
>> +					EM2874_R5E_TS2_PKT_SIZE,
>> +					0xFF);
>> +		} else {
>> +			/* TS2 Maximum Transfer Size = 188 * 5 */
>> +			em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
>> +					EM2874_R5D_TS1_PKT_SIZE :
>> +					EM2874_R5E_TS2_PKT_SIZE, 0x05);
>> +		}
> Hmm... for ISOC, the USB descriptors inform the max transfer size, with
> are detected at probe time, on this part of em28xx_usb_probe:
>
> 	if (size > dev->dvb_max_pkt_size_isoc) {
> 		has_dvb = true; /* see NOTE (~) */
> 		dev->dvb_ep_isoc = e->bEndpointAddress;
> 		dev->dvb_max_pkt_size_isoc = size;
> 		dev->dvb_alt_isoc = i;
> 	}
>
> If we're touching TS PKT size register, it should somehow be
> aligned what's there. I mean, we should either do:
>
> 			em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
> 					EM2874_R5D_TS1_PKT_SIZE :
> 					EM2874_R5E_TS2_PKT_SIZE, dev->dvb_max_pkt_size_isoc / 188);
>
> Or the other way around, setting dev->dvb_max_pkt_size_isoc after
> writing to EM2874_R5D_TS1_PKT_SIZE or EM2874_R5E_TS2_PKT_SIZE.
>
> Not sure what's more accurate here: the USB descriptors or the
> contents of the TS size register. I doubt, I would stick with
> the USB descriptor info.
>
> Btw, I wander what happens if we write a bigger value than 5 to those
> registers. Would it support a bigger transfer size than 940 for ISOCH?
>
>
>
> Cheers,
> Mauro

Hi Mauro,

On the one ISOC device I have here, the usb endpoint
dvb_max_pkt_size_isoc is 940 during usb_probe and
EM2874_R5D_TS1_PKT_SIZE returns 5 when queried in start_streaming. I
just did a little checking and EM2874_R5D_TS1_PKT_SIZE accepted, and
returned the value I wrote all the way up to 32. The device is DVB
however, so I cannot test actual operation to see if it increases ISOC
packet size at all. We're just going on what Empia said here for the
maximum of 5.

I agree that this is probably more correct to use (dvb_max_pkt_size_isoc
/ 188) instead of a hardcoded 5. This at least would keep devices with
other multipliers happy. Your second method of querying the multiplier
before setting up the endpoint would require a re-organization of
usb_probe to move em28xx_init_dev higher.

I'll submit a v2 of this briefly.

Cheers,

Brad
Brad Love Jan. 30, 2018, 7:56 p.m. UTC | #6 
On 2018-01-05 08:20, Devin Heitmueller wrote:
> Hi Brad,
>
> My documents indicate that Register 0x5D and 0x5E are read-only, and
> populated based on the eeprom programming.
>
> On your device, what is the value of those registers prior to you changing them?
>
> If you write to those registers, do they reflect the new values if you
> read them back?
>
> Does changing these values result in any change to the device's
> endpoint configuration (which is typically statically defined when the
> device is probed)?
>
> What precisely is the behavior you were seeing prior to this patch?
>
> Devin

Hi Devin,

I answered ISOC half of the question dealing with endpoint configuration
in Mauro's email, just now, apologies for not cc'ing you. Here I'll
answer about the bulk half.

These registers according to Empia and my own research are writeable. I
instrumented the driver to get before and after values. The registers
are indeed updated with the 0xff value written.

The behaviour encountered is as follows. The DualHD DVB bulk model I
have with me, after device init, has TS1_PKT_SIZE:0x05 and
TS2_PKT_SIZE:0x10. Since the devices are bulk models, this leads to TS1
producing bulk packets of 940B and TS2 producing bulk packets of 3008B.
With this patch both PKT_SIZE registers are set to 0xff, and verified
returning 0xff. After that the bulk packet size produced by the em28xx
is 48128B, or 256*188. Patch 3/9 sets the bulk dvb multiplier to 94, so
then (512*94==48128) is used when starting the USB transfer. For bulk
transfers, everything should be in sync.

Such small bulk multipliers, as currently used, completely destroys
performance on embedded devices and in VMWare. This patch makes both of
those use cases very happy, and, usable at all.

Cheers,

Brad



>
> On Thu, Jan 4, 2018 at 7:22 PM, Michael Ira Krufky <mkrufky@linuxtv.org> wrote:
>> On Thu, Jan 4, 2018 at 7:04 PM, Brad Love <brad@nextdimension.cc> wrote:
>>> Set appropriate bulk/ISOC transfer multiplier on capture start.
>>> This sets ISOC transfer to 940 bytes (188 * 5)
>>> This sets bulk transfer to 48128 bytes (188 * 256)
>>>
>>> The above values are maximum allowed according to Empia.
>>>
>>> Signed-off-by: Brad Love <brad@nextdimension.cc>
>> :+1
>>
>> Reviewed-by: Michael Ira Krufky <mkrufky@linuxtv.org>
>>
>>> ---
>>>  drivers/media/usb/em28xx/em28xx-core.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
>>> index ef38e56..67ed6a3 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-core.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-core.c
>>> @@ -638,6 +638,18 @@ int em28xx_capture_start(struct em28xx *dev, int start)
>>>             dev->chip_id == CHIP_ID_EM28174 ||
>>>             dev->chip_id == CHIP_ID_EM28178) {
>>>                 /* The Transport Stream Enable Register moved in em2874 */
>>> +               if (dev->dvb_xfer_bulk) {
>>> +                       /* Max Tx Size = 188 * 256 = 48128 - LCM(188,512) * 2 */
>>> +                       em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
>>> +                                       EM2874_R5D_TS1_PKT_SIZE :
>>> +                                       EM2874_R5E_TS2_PKT_SIZE,
>>> +                                       0xFF);
>>> +               } else {
>>> +                       /* TS2 Maximum Transfer Size = 188 * 5 */
>>> +                       em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
>>> +                                       EM2874_R5D_TS1_PKT_SIZE :
>>> +                                       EM2874_R5E_TS2_PKT_SIZE, 0x05);
>>> +               }
>>>                 if (dev->ts == PRIMARY_TS)
>>>                         rc = em28xx_write_reg_bits(dev,
>>>                                 EM2874_R5F_TS_ENABLE,
>>> --
>>> 2.7.4
>>>
>
>
diff mbox

Patch

diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index ef38e56..67ed6a3 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -638,6 +638,18 @@  int em28xx_capture_start(struct em28xx *dev, int start)
 	    dev->chip_id == CHIP_ID_EM28174 ||
 	    dev->chip_id == CHIP_ID_EM28178) {
 		/* The Transport Stream Enable Register moved in em2874 */
+		if (dev->dvb_xfer_bulk) {
+			/* Max Tx Size = 188 * 256 = 48128 - LCM(188,512) * 2 */
+			em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
+					EM2874_R5D_TS1_PKT_SIZE :
+					EM2874_R5E_TS2_PKT_SIZE,
+					0xFF);
+		} else {
+			/* TS2 Maximum Transfer Size = 188 * 5 */
+			em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
+					EM2874_R5D_TS1_PKT_SIZE :
+					EM2874_R5E_TS2_PKT_SIZE, 0x05);
+		}
 		if (dev->ts == PRIMARY_TS)
 			rc = em28xx_write_reg_bits(dev,
 				EM2874_R5F_TS_ENABLE,