| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 11 Oct 2018 00:13:28 +0300
From: "Matwey V. Kornilov" <matwey.kornilov@...il.com>
To: Linux Media Mailing List <linux-media@...r.kernel.org>,
open list <linux-kernel@...r.kernel.org>
Cc: Tomasz Figa <tfiga@...omium.org>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Alan Stern <stern@...land.harvard.edu>,
Ezequiel Garcia <ezequiel@...labora.com>,
Hans de Goede <hdegoede@...hat.com>,
Hans Verkuil <hverkuil@...all.nl>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Steven Rostedt <rostedt@...dmis.org>, mingo@...hat.com,
Mike Isely <isely@...ox.com>,
Bhumika Goyal <bhumirks@...il.com>,
Colin King <colin.king@...onical.com>,
Kieran Bingham <kieran.bingham@...asonboard.com>,
keiichiw@...omium.org
Subject: Re: [PATCH v5 2/2] media: usb: pwc: Don't use coherent DMA buffers
for ISO transfer
вт, 11 сент. 2018 г. в 21:58, Matwey V. Kornilov <matwey.kornilov@...il.com>:
>
> вт, 28 авг. 2018 г. в 10:17, Matwey V. Kornilov <matwey.kornilov@...il.com>:
> >
> > вт, 21 авг. 2018 г. в 20:06, Matwey V. Kornilov <matwey@....msu.ru>:
> > >
> > > DMA cocherency slows the transfer down on systems without hardware
> > > coherent DMA.
> > > Instead we use noncocherent DMA memory and explicit sync at data receive
> > > handler.
> > >
> > > Based on previous commit the following performance benchmarks have been
> > > carried out. Average memcpy() data transfer rate (rate) and handler
> > > completion time (time) have been measured when running video stream at
> > > 640x480 resolution at 10fps.
> > >
> > > x86_64 based system (Intel Core i5-3470). This platform has hardware
> > > coherent DMA support and proposed change doesn't make big difference here.
> > >
> > > * kmalloc: rate = (2.0 +- 0.4) GBps
> > > time = (5.0 +- 3.0) usec
> > > * usb_alloc_coherent: rate = (3.4 +- 1.2) GBps
> > > time = (3.5 +- 3.0) usec
> > >
> > > We see that the measurements agree within error ranges in this case.
> > > So theoretically predicted performance downgrade cannot be reliably
> > > measured here.
> > >
> > > armv7l based system (TI AM335x BeagleBone Black @ 300MHz). This platform
> > > has no hardware coherent DMA support. DMA coherence is implemented via
> > > disabled page caching that slows down memcpy() due to memory controller
> > > behaviour.
> > >
> > > * kmalloc: rate = (114 +- 5) MBps
> > > time = (84 +- 4) usec
> > > * usb_alloc_coherent: rate = (28.1 +- 0.1) MBps
> > > time = (341 +- 2) usec
> > >
> > > Note, that quantative difference leads (this commit leads to 4 times
> > > acceleration) to qualitative behavior change in this case. As it was
> > > stated before, the video stream cannot be successfully received at AM335x
> > > platforms with MUSB based USB host controller due to performance issues
> > > [1].
> > >
> > > [1] https://www.spinics.net/lists/linux-usb/msg165735.html
> > >
> > > Signed-off-by: Matwey V. Kornilov <matwey@....msu.ru>
> >
> > Ping
> >
>
> Ping
>
Ping
> > > ---
> > > drivers/media/usb/pwc/pwc-if.c | 57 ++++++++++++++++++++++++++++++++----------
> > > 1 file changed, 44 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
> > > index 72d2897a4b9f..1360722ab423 100644
> > > --- a/drivers/media/usb/pwc/pwc-if.c
> > > +++ b/drivers/media/usb/pwc/pwc-if.c
> > > @@ -159,6 +159,32 @@ static const struct video_device pwc_template = {
> > > /***************************************************************************/
> > > /* Private functions */
> > >
> > > +static void *pwc_alloc_urb_buffer(struct device *dev,
> > > + size_t size, dma_addr_t *dma_handle)
> > > +{
> > > + void *buffer = kmalloc(size, GFP_KERNEL);
> > > +
> > > + if (!buffer)
> > > + return NULL;
> > > +
> > > + *dma_handle = dma_map_single(dev, buffer, size, DMA_FROM_DEVICE);
> > > + if (dma_mapping_error(dev, *dma_handle)) {
> > > + kfree(buffer);
> > > + return NULL;
> > > + }
> > > +
> > > + return buffer;
> > > +}
> > > +
> > > +static void pwc_free_urb_buffer(struct device *dev,
> > > + size_t size,
> > > + void *buffer,
> > > + dma_addr_t dma_handle)
> > > +{
> > > + dma_unmap_single(dev, dma_handle, size, DMA_FROM_DEVICE);
> > > + kfree(buffer);
> > > +}
> > > +
> > > static struct pwc_frame_buf *pwc_get_next_fill_buf(struct pwc_device *pdev)
> > > {
> > > unsigned long flags = 0;
> > > @@ -306,6 +332,11 @@ static void pwc_isoc_handler(struct urb *urb)
> > > /* Reset ISOC error counter. We did get here, after all. */
> > > pdev->visoc_errors = 0;
> > >
> > > + dma_sync_single_for_cpu(&urb->dev->dev,
> > > + urb->transfer_dma,
> > > + urb->transfer_buffer_length,
> > > + DMA_FROM_DEVICE);
> > > +
> > > /* vsync: 0 = don't copy data
> > > 1 = sync-hunt
> > > 2 = synched
> > > @@ -428,16 +459,15 @@ static int pwc_isoc_init(struct pwc_device *pdev)
> > > urb->dev = udev;
> > > urb->pipe = usb_rcvisocpipe(udev, pdev->vendpoint);
> > > urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
> > > - urb->transfer_buffer = usb_alloc_coherent(udev,
> > > - ISO_BUFFER_SIZE,
> > > - GFP_KERNEL,
> > > - &urb->transfer_dma);
> > > + urb->transfer_buffer_length = ISO_BUFFER_SIZE;
> > > + urb->transfer_buffer = pwc_alloc_urb_buffer(&udev->dev,
> > > + urb->transfer_buffer_length,
> > > + &urb->transfer_dma);
> > > if (urb->transfer_buffer == NULL) {
> > > PWC_ERROR("Failed to allocate urb buffer %d\n", i);
> > > pwc_isoc_cleanup(pdev);
> > > return -ENOMEM;
> > > }
> > > - urb->transfer_buffer_length = ISO_BUFFER_SIZE;
> > > urb->complete = pwc_isoc_handler;
> > > urb->context = pdev;
> > > urb->start_frame = 0;
> > > @@ -488,15 +518,16 @@ static void pwc_iso_free(struct pwc_device *pdev)
> > >
> > > /* Freeing ISOC buffers one by one */
> > > for (i = 0; i < MAX_ISO_BUFS; i++) {
> > > - if (pdev->urbs[i]) {
> > > + struct urb *urb = pdev->urbs[i];
> > > +
> > > + if (urb) {
> > > PWC_DEBUG_MEMORY("Freeing URB\n");
> > > - if (pdev->urbs[i]->transfer_buffer) {
> > > - usb_free_coherent(pdev->udev,
> > > - pdev->urbs[i]->transfer_buffer_length,
> > > - pdev->urbs[i]->transfer_buffer,
> > > - pdev->urbs[i]->transfer_dma);
> > > - }
> > > - usb_free_urb(pdev->urbs[i]);
> > > + if (urb->transfer_buffer)
> > > + pwc_free_urb_buffer(&urb->dev->dev,
> > > + urb->transfer_buffer_length,
> > > + urb->transfer_buffer,
> > > + urb->transfer_dma);
> > > + usb_free_urb(urb);
> > > pdev->urbs[i] = NULL;
> > > }
> > > }
> > > --
> > > 2.16.4
> > >
> >
> >
> > --
> > With best regards,
> > Matwey V. Kornilov
>
>
>
> --
> With best regards,
> Matwey V. Kornilov
--
With best regards,
Matwey V. Kornilov
Powered by blists - more mailing lists