From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.6 required=3.0 tests=DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS,T_DKIM_INVALID, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9695EECDFB8 for ; Fri, 20 Jul 2018 09:36:13 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 443EA20673 for ; Fri, 20 Jul 2018 09:36:13 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="FLOZqlLy" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 443EA20673 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=sai.msu.ru Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728273AbeGTKXe (ORCPT ); Fri, 20 Jul 2018 06:23:34 -0400 Received: from mail-oi0-f66.google.com ([209.85.218.66]:38460 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727266AbeGTKXd (ORCPT ); Fri, 20 Jul 2018 06:23:33 -0400 Received: by mail-oi0-f66.google.com with SMTP id v8-v6so20234988oie.5; Fri, 20 Jul 2018 02:36:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=0WCvZ/UwTuSTOy/hE5dovvSALzhVcvXV0ItuU1I71fY=; b=FLOZqlLyG2EheiodwW9/3/qpUErSzGln893aC9YKuj+dvXpfYyn22Z3F8OHLio1g0l 8wRS5xSenJPCdB37/WKA1VIBBjVWzxPf7dwXSVJI5lPQVEkZQId8vRlfqCasLtL+Cg/1 H1DtfZ2tql80SRojaX0M0TrOAkIHGcStjYFMA8C10ZeR/gXpsBOoFQtSJaeZS4faOz8V k0PHxuDlODrlcfT89aVR7z6LdWC6uBLR25U/XIirjdCXQLrmSskUhhx0L0GWbLG/nnUZ hN6xu+bGjMC5ogxgQtDABTRetU/0OUB+ymaw5urhVU8s7a0xuoPcuEY9pmq7z4DhBrWe 0PUA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=0WCvZ/UwTuSTOy/hE5dovvSALzhVcvXV0ItuU1I71fY=; b=o6UHOuTE06YXDfdc1L9e8ArNB6Anu+C3j35bn02aqAFHcsbfWWKXLFHkaop+nr9Mt8 PBhfnxsLot1T0N3hZfOI+TJbBjZpxFGP6Sl8UhM7sZqbILDqXFkpB3hZzB/rfvZTL9J1 GdVZG2p2o2hJH72S15A407HAPqeUXwJ1XjPqUzvGNZ0iLDOSzSf0aJt/cFu0iIfHUcKz lWwpPYS/fCjTt+HPevMvOvIKNEG1yLgCNAviLrZk36LeHKfInVcKpcG3tq4v+rvLDRz+ MF6H9ObFMfMOKN4swe+Plj+y45NPmCqCDx78ZTB55XtgEzMNif7UJU+QGAw7blSHm0f2 Q2Ng== X-Gm-Message-State: AOUpUlFWEQ04Pf2Fxn3fMv1YCS3rD9XA4Ae7Ll5xnFkMM+jiBvEYcf2R d7OGDHLvkllcxJmaWRpr0kcMfnPDcVi1EtdMJpMjvbnW X-Google-Smtp-Source: AAOMgpcOtlwTz28Az83CMyOPFS7fg7DkRQFX13ExX6+vZtswesdnZTFu86ex54Vc6KOg+yeueAaBve2cekQ7ek8ggFI= X-Received: by 2002:aca:c514:: with SMTP id v20-v6mr1386677oif.153.1532079369443; Fri, 20 Jul 2018 02:36:09 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a9d:734:0:0:0:0:0 with HTTP; Fri, 20 Jul 2018 02:35:49 -0700 (PDT) In-Reply-To: <31826cf1b99fa2372cd4cf0b6cee8ba0ba4288f1.camel@collabora.com> References: <20180617143625.32133-1-matwey@sai.msu.ru> <20180617143625.32133-2-matwey@sai.msu.ru> <31826cf1b99fa2372cd4cf0b6cee8ba0ba4288f1.camel@collabora.com> From: "Matwey V. Kornilov" Date: Fri, 20 Jul 2018 12:35:49 +0300 X-Google-Sender-Auth: HRtKaj3nVgFRNXKo-ixNR_tXo_8 Message-ID: Subject: Re: [PATCH 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer To: Ezequiel Garcia Cc: Alan Stern , Hans de Goede , hverkuil@xs4all.nl, mchehab@kernel.org, Laurent Pinchart , Steven Rostedt , mingo@redhat.com, Mike Isely , Bhumika Goyal , Colin King , linux-media@vger.kernel.org, open list Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2018-07-20 2:36 GMT+03:00 Ezequiel Garcia : > On Wed, 2018-07-18 at 15:10 +0300, Matwey V. Kornilov wrote: >> 2018-07-17 23:10 GMT+03:00 Ezequiel Garcia : >> > Hi Matwey, >> > >> > First of all, sorry for the delay. >> > >> > Adding Alan and Hans. Guys, do you have any feedback here? >> > >> > See below for some feedback on my side. >> > >> > On Mon, 2018-06-18 at 10:10 +0300, Matwey V. Kornilov wrote: >> > > Hi Ezequiel, >> > > >> > > 2018-06-18 8:11 GMT+03:00 Ezequiel Garcia : >> > > > + Laurent >> > > > >> > > > On Sun, 2018-06-17 at 17:36 +0300, 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 >> > > > > >> > > > >> > > > This is quite interesting! I have receive similar complaints >> > > > from users wanting to use stk1160 on BBB and Raspberrys, >> > > > without much luck on either, due to insufficient isoc bandwidth. >> > > > >> > > > I'm guessing other ARM platforms could be suffering >> > > > from the same issue. >> > > > >> > > > Note that stk1160 and uvcvideo drivers use kmalloc on platforms >> > > > where DMA_NONCOHERENT is defined, but this is not the case >> > > > on ARM platforms. >> > > >> > > There are some ARMv7 platforms that have coherent DMA (for instance >> > > Broadcome Horthstar Plus series), but the most of them don't have. It >> > > is defined in device tree file, and there is no way to recover this >> > > information at runtime in USB perepherial driver. >> > > >> > > > >> > > > So, what is the benefit of using consistent >> > > > for these URBs, as opposed to streaming? >> > > >> > > I don't know, I think there is no real benefit and all we see is a >> > > consequence of copy-pasta when some webcam drivers were inspired by >> > > others and development priparily was going at x86 platforms. >> > >> > You are probably right about the copy-pasta. >> > >> > > It would >> > > be great if somebody corrected me here. DMA Coherence is quite strong >> > > property and I cannot figure out how can it help when streaming video. >> > > The CPU host always reads from the buffer and never writes to. >> > > Hardware perepherial always writes to and never reads from. Moreover, >> > > buffer access is mutually exclusive and separated in time by Interrupt >> > > fireing and URB starting (when we reuse existing URB for new request). >> > > Only single one memory barrier is really required here. >> > > >> > >> > Yeah, and not setting URB_NO_TRANSFER_DMA_MAP makes the USB core >> > create DMA mappings and use the streaming API. Which makes more >> > sense in hardware without hardware coherency. >> > >> > The only thing that bothers me with this patch is that it's not >> > really something specific to this driver. If this fix is valid >> > for pwc, then it's valid for all the drivers allocating coherent >> > memory. >> > >> > And also, this path won't prevent further copy-paste spread >> > of the coherent allocation. >> > >> > Is there any chance we can introduce a helper to allocate >> > isoc URBs, and then change all drivers to use it? No need >> > to do all of them now, but it would be good to at least have >> > a plan for it. >> >> Well, basically I am agree with you. >> However, I don't have all possible hardware to test, so I can't fix >> all possible drivers. > > Sure. And keep in mind this is more about the USB host controller, > than about this specific driver. So it's the controller what we > would have to test! > >> Also I can not figure out how could the helper looked like. What do >> you think about usb_alloc() (c.f. usb_alloc_coherent()) ? >> > > I do not know that either. But it's something we can think about. > > Meanwhile, it would be a shame to loose or stall this excellent > effort (which is effectively enabling a cameras on a bunch of devices). > > How about you introduce a driver parameter (or device attribute), > to avoid changing the behavior for USB host controllers we don't know > about? It would be fine for me, if there are no objections. I know that some maintainers don't like module parameters per se. > > Something like 'alloc_coherent_urbs=y/n'. Perhaps set that > to 'yes' by default in x86, and 'no' by default in the rest? > > We can think about a generic solution later. > > Thanks, > Eze > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382