Re: [PATCH RFC] media: em28xx: don't use coherent buffer for DMA transfers

[Mauro Carvalho Chehab]

Em Wed, 28 Feb 2018 09:49:12 -0500
Devin Heitmueller  escreveu:

> On Tue, Feb 27, 2018 at 12:42 PM, Mauro Carvalho Chehab
>  wrote:
> > While coherent memory is cheap on x86, it has problems on
> > arm. So, stop using it.
> >
> > Signed-off-by: Mauro Carvalho Chehab 
> > ---
> >
> > I wrote this patch in order to check if this would make things better
> > for ISOCH transfers on Raspberry Pi3. It didn't. Yet, keep using
> > coherent memory at USB drivers seem an overkill.
> >
> > So, I'm actually not sure if we should either go ahead and merge it
> > or not.
> >
> > Comments? Tests?
> 
> For what it's worth, while I haven't tested this patch you're
> proposing, I've been running what is essentially the same change in a
> private tree for several years in order for the device to work better
> with several TI Davinci SOC platforms.

Good to know! I guess then it is worth applying it. Btw, while here,
I'm wandering if it should keep using URB_ISO_ASAP flag or not. On
my tests, for DVB, it seems to be working both ways. Didn't test
analog TV yet.

Thanks,
Mauro

Re: [PATCH RFC] media: em28xx: don't use coherent buffer for DMA transfers

[Devin Heitmueller]

On Tue, Feb 27, 2018 at 12:42 PM, Mauro Carvalho Chehab
 wrote:
> While coherent memory is cheap on x86, it has problems on
> arm. So, stop using it.
>
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>
> I wrote this patch in order to check if this would make things better
> for ISOCH transfers on Raspberry Pi3. It didn't. Yet, keep using
> coherent memory at USB drivers seem an overkill.
>
> So, I'm actually not sure if we should either go ahead and merge it
> or not.
>
> Comments? Tests?

For what it's worth, while I haven't tested this patch you're
proposing, I've been running what is essentially the same change in a
private tree for several years in order for the device to work better
with several TI Davinci SOC platforms.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

[PATCH RFC] media: em28xx: don't use coherent buffer for DMA transfers

[Mauro Carvalho Chehab]

While coherent memory is cheap on x86, it has problems on
arm. So, stop using it.

Signed-off-by: Mauro Carvalho Chehab 
---

I wrote this patch in order to check if this would make things better
for ISOCH transfers on Raspberry Pi3. It didn't. Yet, keep using
coherent memory at USB drivers seem an overkill.

So, I'm actually not sure if we should either go ahead and merge it
or not.

Comments? Tests?

 drivers/media/usb/em28xx/em28xx-core.c | 49 +++---
 drivers/media/usb/em28xx/em28xx.h  |  2 +-
 2 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-core.c 
b/drivers/media/usb/em28xx/em28xx-core.c
index 1d0d8cc06103..6fadcb03093f 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -800,7 +800,6 @@ void em28xx_uninit_usb_xfer(struct em28xx *dev, enum 
em28xx_mode mode)
 {
struct urb *urb;
struct em28xx_usb_bufs *usb_bufs;
-   struct usb_device *udev = interface_to_usbdev(dev->intf);
int i;
 
em28xx_isocdbg("em28xx: called em28xx_uninit_usb_xfer in mode %d\n",
@@ -819,23 +818,16 @@ void em28xx_uninit_usb_xfer(struct em28xx *dev, enum 
em28xx_mode mode)
else
usb_unlink_urb(urb);
 
-   if (usb_bufs->transfer_buffer[i]) {
-   usb_free_coherent(udev,
- urb->transfer_buffer_length,
- usb_bufs->transfer_buffer[i],
- urb->transfer_dma);
-   }
usb_free_urb(urb);
usb_bufs->urb[i] = NULL;
}
-   usb_bufs->transfer_buffer[i] = NULL;
}
 
kfree(usb_bufs->urb);
-   kfree(usb_bufs->transfer_buffer);
+   kfree(usb_bufs->buf);
 
usb_bufs->urb = NULL;
-   usb_bufs->transfer_buffer = NULL;
+   usb_bufs->buf = NULL;
usb_bufs->num_bufs = 0;
 
em28xx_capture_start(dev, 0);
@@ -912,14 +904,13 @@ int em28xx_alloc_urbs(struct em28xx *dev, enum 
em28xx_mode mode, int xfer_bulk,
 
usb_bufs->num_bufs = num_bufs;
 
-   usb_bufs->urb = kzalloc(sizeof(void *)*num_bufs,  GFP_KERNEL);
+   usb_bufs->urb = kcalloc(sizeof(void *), num_bufs,  GFP_KERNEL);
if (!usb_bufs->urb)
return -ENOMEM;
 
-   usb_bufs->transfer_buffer = kzalloc(sizeof(void *)*num_bufs,
-GFP_KERNEL);
-   if (!usb_bufs->transfer_buffer) {
-   kfree(usb_bufs->urb);
+   usb_bufs->buf = kcalloc(sizeof(void *), num_bufs, GFP_KERNEL);
+   if (!usb_bufs->buf) {
+   kfree(usb_bufs->buf);
return -ENOMEM;
}
 
@@ -942,37 +933,39 @@ int em28xx_alloc_urbs(struct em28xx *dev, enum 
em28xx_mode mode, int xfer_bulk,
}
usb_bufs->urb[i] = urb;
 
-   usb_bufs->transfer_buffer[i] = usb_alloc_coherent(udev,
-   sb_size, GFP_KERNEL, >transfer_dma);
-   if (!usb_bufs->transfer_buffer[i]) {
+   usb_bufs->buf[i] = kzalloc(sb_size, GFP_KERNEL);
+   if (!usb_bufs->buf[i]) {
dev_err(>intf->dev,
"unable to allocate %i bytes for transfer 
buffer %i%s\n",
   sb_size, i,
   in_interrupt() ? " while in int" : "");
em28xx_uninit_usb_xfer(dev, mode);
+
+   while (i) {
+   kfree(usb_bufs->buf[i]);
+   i--;
+   }
+   kfree(usb_bufs->buf);
+
return -ENOMEM;
}
-   memset(usb_bufs->transfer_buffer[i], 0, sb_size);
 
if (xfer_bulk) { /* bulk */
pipe = usb_rcvbulkpipe(udev,
   mode == EM28XX_ANALOG_MODE ?
   dev->analog_ep_bulk :
   dev->dvb_ep_bulk);
-   usb_fill_bulk_urb(urb, udev, pipe,
- usb_bufs->transfer_buffer[i], sb_size,
- em28xx_irq_callback, dev);
-   urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
+   usb_fill_bulk_urb(urb, udev, pipe, usb_bufs->buf[i],
+ sb_size, em28xx_irq_callback, dev);
+   urb->transfer_flags = URB_FREE_BUFFER;
} else { /* isoc */
pipe = usb_rcvisocpipe(udev,
   mode == EM28XX_ANALOG_MODE ?