| 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, 9 Aug 2018 12:18:13 +0300
From: "Matwey V. Kornilov" <matwey@....msu.ru>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: 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>,
Linux Media Mailing List <linux-media@...r.kernel.org>,
open list <linux-kernel@...r.kernel.org>,
Ezequiel Garcia <ezequiel@...labora.com>
Subject: Re: [PATCH v2 2/2] media: usb: pwc: Don't use coherent DMA buffers
for ISO transfer
2018-08-09 1:46 GMT+03:00 Laurent Pinchart <laurent.pinchart@...asonboard.com>:
> Hi Matwey,
>
> Thank you for the patch.
>
> On Friday, 22 June 2018 15:04:19 EEST Matwey V. Kornilov wrote:
>> DMA cocherency slows the transfer down on systems without hardware
>> coherent DMA.
>>
>> 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 = (4.4 +- 1.0) GBps
>> time = (2.4 +- 1.2) usec
>> * usb_alloc_coherent: rate = (4.1 +- 0.9) GBps
>> time = (2.5 +- 1.0) usec
>>
>> We see that the measurements agree well within error ranges in this case.
>> So no performance downgrade is introduced.
>>
>> armv7l based system (TI AM335x BeagleBone Black). 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 = (190 +- 30) MBps
>> time = (50 +- 10) usec
>> * usb_alloc_coherent: rate = (33 +- 4) MBps
>> time = (3000 +- 400) usec
>>
>> Note, that quantative difference leads (this commit leads to 5 times
>> acceleration) to qualitative behavior change in this case. As it was
>> stated before, the video stream can not 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>
>> ---
>> drivers/media/usb/pwc/pwc-if.c | 12 +++---------
>> 1 file changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
>> index 72d2897a4b9f..339a285600d1 100644
>> --- a/drivers/media/usb/pwc/pwc-if.c
>> +++ b/drivers/media/usb/pwc/pwc-if.c
>> @@ -427,11 +427,8 @@ static int pwc_isoc_init(struct pwc_device *pdev)
>> urb->interval = 1; // devik
>> 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_flags = URB_ISO_ASAP;
>> + urb->transfer_buffer = kmalloc(ISO_BUFFER_SIZE, GFP_KERNEL);
>
> ISO_BUFFER_SIZE is 970 bytes, well below a page size, so this should be fine.
> However, for other USB camera drivers, we might require larger buffers
> spanning multiple pages, and kmalloc() wouldn't be a very good choice there. I
> thus believe we should implement a helper function, possibly in the for of
> usb_alloc_noncoherent(), to allow picking the right allocation mechanism based
> on the buffer size (and possibly other parameters). Ideally the helper and the
> USB core should cooperate to avoid any overhead from DMA operations when DMA
> is coherent on the platform (on x86 and some ARM platforms).
pwc/pwc.h:
#define ISO_FRAMES_PER_DESC 10
#define ISO_MAX_FRAME_SIZE 960
#define ISO_BUFFER_SIZE (ISO_FRAMES_PER_DESC * ISO_MAX_FRAME_SIZE)
The buffer size is 9600 bytes, where did 970 come from?
>
> That being said, I don't see a reason why this patch should be blocked until
> we get such a helper function, we can also implement it when needed for
> another USB webcam driver (likely uvcvideo given the recent discussions) and
> then use it in the pwc driver.
>
> We have also determined that performances can be further improved by keeping
> mappings around and using the dma_sync_* operations at runtime. That's again
> not a reason to block this patch, as the performance improvement is already
> impressive, so
I've incorporated this changes in v3, I'll send it shortly.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
>
> But I would still like to see the above two issues addressed. If you'd like to
> give them a go, with or without getting v2 of this series merged first, please
> do so, and I'll happily review patches.
>
>> if (urb->transfer_buffer == NULL) {
>> PWC_ERROR("Failed to allocate urb buffer %d\n", i);
>> pwc_isoc_cleanup(pdev);
>> @@ -491,10 +488,7 @@ static void pwc_iso_free(struct pwc_device *pdev)
>> if (pdev->urbs[i]) {
>> 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);
>> + kfree(pdev->urbs[i]->transfer_buffer);
>> }
>> usb_free_urb(pdev->urbs[i]);
>> pdev->urbs[i] = NULL;
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
Powered by blists - more mailing lists