| Message ID | 1515110659-20145-3-git-send-email-brad@nextdimension.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 >>
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 >>> > >
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
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
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 --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,
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(+)