From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vivek Gautam Subject: Re: [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers Date: Thu, 22 Feb 2018 22:54:46 +0530 Message-ID: References: <1517999482-17317-1-git-send-email-vivek.gautam@codeaurora.org> <7406f1ce-c2c9-a6bd-2886-5a34de45add6@arm.com> <28466b36-b5d3-4f60-a45e-b75d79c2a3cb@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: freedreno-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "Freedreno" To: Tomasz Figa Cc: Mark Rutland , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, jcrouse-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, Linux PM , David Airlie , "Rafael J. Wysocki" , Joerg Roedel , Will Deacon , Rob Clark , dri-devel , Linux Kernel Mailing List , "list-Y9sIeH5OGRo@public.gmane.org:IOMMU DRIVERS" , Rob Herring , Greg KH , freedreno , Robin Murphy , Stephen Boyd , linux-arm-msm List-Id: linux-arm-msm@vger.kernel.org SGksCgpPbiBUaHUsIEZlYiAyMiwgMjAxOCBhdCA3OjQyIFBNLCBUb21hc3ogRmlnYSA8dGZpZ2FA Y2hyb21pdW0ub3JnPiB3cm90ZToKPiBPbiBUaHUsIEZlYiAyMiwgMjAxOCBhdCAxMDo0NSBQTSwg Um9iaW4gTXVycGh5IDxyb2Jpbi5tdXJwaHlAYXJtLmNvbT4gd3JvdGU6Cj4+IFtzb3JyeSwgSSBo YWQgaW50ZW5kZWQgdG8gcmVwbHkgc29vbmVyIGJ1dCBjbGVhcmx5IGZvcmdvdF0KPj4KPj4KPj4g T24gMTYvMDIvMTggMDA6MTMsIFRvbWFzeiBGaWdhIHdyb3RlOgo+Pj4KPj4+IE9uIEZyaSwgRmVi IDE2LCAyMDE4IGF0IDI6MTQgQU0sIFJvYmluIE11cnBoeSA8cm9iaW4ubXVycGh5QGFybS5jb20+ Cj4+PiB3cm90ZToKPj4+Pgo+Pj4+IE9uIDE1LzAyLzE4IDA0OjE3LCBUb21hc3ogRmlnYSB3cm90 ZToKPj4+PiBbLi4uXQo+Pj4+Pj4KPj4+Pj4+Cj4+Pj4+PiBDb3VsZCB5b3UgZWxhYm9yYXRlIG9u IHdoYXQga2luZCBvZiBsb2NraW5nIHlvdSBhcmUgY29uY2VybmVkIGFib3V0Pwo+Pj4+Pj4gQXMg SSBleHBsYWluZWQgYmVmb3JlLCB0aGUgbm9ybWFsbHkgaGFwcGVuaW5nIGZhc3QgcGF0aCB3b3Vs ZCBsb2NrCj4+Pj4+PiBkZXYtPnBvd2VyX2xvY2sgb25seSBmb3IgdGhlIGJyaWVmIG1vbWVudCBv ZiBpbmNyZW1lbnRpbmcgdGhlIHJ1bnRpbWUKPj4+Pj4+IFBNIHVzYWdlIGNvdW50ZXIuCj4+Pj4+ Cj4+Pj4+Cj4+Pj4+Cj4+Pj4+IE15IGJhZCwgdGhhdCdzIG5vdCBldmVuIGl0Lgo+Pj4+Pgo+Pj4+ PiBUaGUgYXRvbWljIHVzYWdlIGNvdW50ZXIgaXMgaW5jcmVtZW50ZWQgYmVmb3JlaGFuZHMsIHdp dGhvdXQgYW55Cj4+Pj4+IGxvY2tpbmcgWzFdIGFuZCB0aGUgc3BpbmxvY2sgaXMgYWNxdWlyZWQg b25seSBmb3IgdGhlIHNha2Ugb2YKPj4+Pj4gdmFsaWRhdGluZyB0aGF0IGRldmljZSdzIHJ1bnRp bWUgUE0gc3RhdGUgcmVtYWluZWQgdmFsaWQgaW5kZWVkIFsyXSwKPj4+Pj4gd2hpY2ggd291bGQg YmUgdGhlIGNhc2UgaW4gdGhlIGZhc3QgcGF0aCBvZiB0aGUgc2FtZSBkcml2ZXIgZG9pbmcgdHdv Cj4+Pj4+IG1hcHBpbmdzIGluIHBhcmFsbGVsLCB3aXRoIHRoZSBtYXN0ZXIgcG93ZXJlZCBvbiAo YW5kIHNvIHRoZSBTTU1VLAo+Pj4+PiB0aHJvdWdoIGRldmljZSBsaW5rczsgaWYgbWFzdGVyIHdh cyBub3QgcG93ZXJlZCBvbiBhbHJlYWR5LCBwb3dlcmluZwo+Pj4+PiBvbiB0aGUgU01NVSBpcyB1 bmF2b2lkYWJsZSBhbnl3YXkgYW5kIGl0IHdvdWxkIGFkZCBtdWNoIG1vcmUgbGF0ZW5jeQo+Pj4+ PiB0aGFuIHRoZSBzcGlubG9jayBpdHNlbGYpLgo+Pj4+Cj4+Pj4KPj4+Pgo+Pj4+IFdlIG5vdyBo YXZlIG5vIGxvY2tpbmcgYXQgYWxsIGluIHRoZSBtYXAgcGF0aCwgYW5kIG9ubHkgYSBwZXItZG9t YWluIGxvY2sKPj4+PiBhcm91bmQgVExCIHN5bmMgaW4gdW5tYXAgd2hpY2ggaXMgdW5mb3J0dW5h dGVseSBuZWNlc3NhcnkgZm9yCj4+Pj4gY29ycmVjdG5lc3M7Cj4+Pj4gdGhlIGxhdHRlciBpc24n dCB0b28gdGVycmlibGUsIHNpbmNlIGluICJzZXJpb3VzIiBoYXJkd2FyZSBpdCBzaG91bGQgb25s eQo+Pj4+IGJlCj4+Pj4gc2VyaWFsaXNpbmcgYSBmZXcgY3B1cyBzZXJ2aW5nIHRoZSBzYW1lIGRl dmljZSBhZ2FpbnN0IGVhY2ggb3RoZXIgKGUuZy4KPj4+PiBmb3IKPj4+PiBtdWx0aXBsZSBxdWV1 ZXMgb24gYSBzaW5nbGUgTklDKS4KPj4+Pgo+Pj4+IFB1dHRpbmcgaW4gYSBnbG9iYWwgbG9jayB3 aGljaCBzZXJpYWxpc2VzICphbGwqIGNvbmN1cnJlbnQgbWFwIGFuZCB1bm1hcAo+Pj4+IGNhbGxz IGZvciAqYWxsKiB1bnJlbGF0ZWQgZGV2aWNlcyBtYWtlcyB0aGluZ3Mgd29yc2UuIFBlcmlvZC4g RXZlbiBpZiB0aGUKPj4+PiBsb2NrIGl0c2VsZiB3ZXJlIGhlbGQgZm9yIHRoZSBtaW5pbXVtIHBv c3NpYmxlIHRpbWUsIGkuZS4gdHJpdmlhbGx5Cj4+Pj4gInNwaW5fbG9jaygmbG9jayk7IHNwaW5f dW5sb2NrKCZsb2NrKSIsIHRoZSBjb3N0IG9mIHJlcGVhdGVkbHkgYm91bmNpbmcKPj4+PiB0aGF0 Cj4+Pj4gb25lIGNhY2hlIGxpbmUgYXJvdW5kIGJldHdlZW4gOTYgQ1BVcyBhY3Jvc3MgdHdvIHNv Y2tldHMgaXMgbm90Cj4+Pj4gbmVnbGlnaWJsZS4KPj4+Cj4+Pgo+Pj4gRmFpciBlbm91Z2guIE5v dGUgdGhhdCB3ZSdyZSBpbiBhIHF1aXRlIGludGVyZXN0aW5nIHNpdHVhdGlvbiBub3c6Cj4+PiAg IGEpIFdlIG5lZWQgdG8gaGF2ZSBydW50aW1lIFBNIGVuYWJsZWQgb24gUXVhbGNvbW0gU29DIHRv IGhhdmUgcG93ZXIKPj4+IHByb3Blcmx5IG1hbmFnZWQsCj4+PiAgIGIpIFdlIG5lZWQgdG8gaGF2 ZSBsb2NrLWZyZWUgbWFwL3VubWFwIG9uIHN1Y2ggZGlzdHJpYnV0ZWQgc3lzdGVtcywKPj4+ICAg YykgSWYgcnVudGltZSBQTSBpcyBlbmFibGVkLCB3ZSBuZWVkIHRvIGNhbGwgaW50byBydW50aW1l IFBNIGZyb20gYW55Cj4+PiBjb2RlIHRoYXQgZG9lcyBoYXJkd2FyZSBhY2Nlc3Nlcywgb3RoZXJ3 aXNlIHRoZSBJT01NVSBBUEkgKGFuZCBzbyBETUEKPj4+IEFQSSBhbmQgdGhlbiBhbnkgVjRMMiBk cml2ZXIpIGJlY29tZXMgdW51c2FibGUuCj4+Pgo+Pj4gSSBjYW4gc2VlIG9uZSBtb3JlIHdheSB0 aGF0IGNvdWxkIHBvdGVudGlhbGx5IGxldCB1cyBoYXZlIGFsbCB0aGUKPj4+IHRocmVlLiBIb3cg YWJvdXQgZW5hYmxpbmcgcnVudGltZSBQTSBvbmx5IG9uIHNlbGVjdGVkIGltcGxlbWVudGF0aW9u cwo+Pj4gKGUuZy4gcWNvbSxzbW11KSBhbmQgdGhlbiBoYXZpbmcgYWxsIHRoZSBydW50aW1lIFBN IGNhbGxzIHN1cnJvdW5kZWQKPj4+IHdpdGggaWYgKHBtX3J1bnRpbWVfZW5hYmxlZCgpKSwgd2hp Y2ggaXMgbG9ja2xlc3M/Cj4+Cj4+Cj4+IFllcywgdGhhdCdzIHRoZSBraW5kIG9mIHRoaW5nIEkg d2FzIGdyYXZpdGF0aW5nIHRvd2FyZHMgLSBteSB2YWd1ZSB0aG91Z2h0Cj4+IHdhcyBhZGRpbmcg c29tZSBmbGFnIHRvIHRoZSBzbW11X2RvbWFpbiwgYnV0IHBtX3J1bnRpbWVfZW5hYmxlZCgpIGRv ZXMgbG9vawo+PiBjb25jZXB0dWFsbHkgYSBsb3QgY2xlYW5lci4KPgo+IEdyZWF0LCB0aGFua3Mu IExvb2tzIGxpa2Ugd2UncmUgaW4gYWdyZWVtZW50IG5vdy4gXG8vCj4KPiBWaXZlaywgZG9lcyB0 aGlzIHNvdW5kIHJlYXNvbmFibGUgdG8geW91PwoKWWVhLCBzb3VuZCBnb29kIHRvIG1lLiBJIHdp bGwgcmVzcGluIHRoZSBwYXRjaGVzLgoKVGhhbmtzICYgUmVnYXJkcwpWaXZlawoKPgo+IEJlc3Qg cmVnYXJkcywKPiBUb21hc3oKCgoKLS0gClFVQUxDT01NIElORElBLCBvbiBiZWhhbGYgb2YgUXVh bGNvbW0gSW5ub3ZhdGlvbiBDZW50ZXIsIEluYy4gaXMgYSBtZW1iZXIKb2YgQ29kZSBBdXJvcmEg Rm9ydW0sIGhvc3RlZCBieSBUaGUgTGludXggRm91bmRhdGlvbgpfX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fXwpGcmVlZHJlbm8gbWFpbGluZyBsaXN0CkZyZWVk cmVub0BsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcv bWFpbG1hbi9saXN0aW5mby9mcmVlZHJlbm8K From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: AH8x224L32ST4P+DW52TFzMQyBIcbpqD+XAWhe+MiJNgFNoFDKbqS1xBMu3815gHnspbYf8Gol+c ARC-Seal: i=1; a=rsa-sha256; t=1519320289; cv=none; d=google.com; s=arc-20160816; b=W+FhkMkxgehB0eSojIE29m/yhkZZGJRAfRr0CDhFJOPS4sQVjhZfZQwieMov78Buen HhJRs8pJrsWq8he/1aRxAw8ADCoNPqUPmB8aYXVjFNhLI3+1adHv6C8pWx1VOqfcjPLk A0jsvOhJ3hpJMaBVI/7kI8O82Iiv89izFVqJQeajWNOvAUzW27KFQIl8GLGSb0WjCIMc sdPnXVwfkN0K6v1oATjHd3ntXIfImXXwO/sMs6EGOaB6Om9AZuwHaizy3b3WPOr2sG4N d+rP2wBYW50Nc0ybcSrQ7nFbapjSbnUGcssMM+ZVBiJDOxIkFxC1YvgOk02aLIHGcpo8 vCLw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=cc:to:subject:message-id:date:from:references:in-reply-to :mime-version:dmarc-filter:dkim-signature:dkim-signature :arc-authentication-results; bh=Xfz+ex09fSEvfTa9ezSmai/dbBFgPMQKx5ZGo+VcfL0=; b=GqxyGeK0yGADiqmtXpBIz7/laLb8RziE4vkyNooTBd2d8/Wh3rv1c+0CJHxo/zx08V 6/jsf3v/tDpmIAxoK0e2vLtGBGT313NMyeJw4vqw01JLVZeyeIqyN+2qdVP2AtiHAkRJ yfq5+rCZNu1F3X8cRBCoWdrcUeoM4IMdAjBuL/Xvnlc08VNwzn9DcuhEnVoaf2XQTGG1 TA/gQ5GBxRSB9W9vYJ+zN3/pmTi+D2Xg5mkB64zRSG2n0ipd7W14HOGE1wHawAyPcQpg GKWggY02YDXggM7VUDDtJmV0+gvob31sHC22JnVLT9/PUTyCBBWPQu1lyY+WUOoLk0BB edWQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@codeaurora.org header.s=default header.b=Ai42tmjq; dkim=pass header.i=@codeaurora.org header.s=default header.b=CuKBTKr1; spf=pass (google.com: domain of vivek.gautam@codeaurora.org designates 198.145.29.96 as permitted sender) smtp.mailfrom=vivek.gautam@codeaurora.org Authentication-Results: mx.google.com; dkim=pass header.i=@codeaurora.org header.s=default header.b=Ai42tmjq; dkim=pass header.i=@codeaurora.org header.s=default header.b=CuKBTKr1; spf=pass (google.com: domain of vivek.gautam@codeaurora.org designates 198.145.29.96 as permitted sender) smtp.mailfrom=vivek.gautam@codeaurora.org DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 05C70607CF Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=vivek.gautam@codeaurora.org MIME-Version: 1.0 In-Reply-To: References: <1517999482-17317-1-git-send-email-vivek.gautam@codeaurora.org> <7406f1ce-c2c9-a6bd-2886-5a34de45add6@arm.com> <28466b36-b5d3-4f60-a45e-b75d79c2a3cb@arm.com> From: Vivek Gautam Date: Thu, 22 Feb 2018 22:54:46 +0530 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers To: Tomasz Figa Cc: Robin Murphy , Will Deacon , "list@263.net:IOMMU DRIVERS" , Joerg Roedel , Rob Herring , Mark Rutland , "Rafael J. Wysocki" , Rob Clark , devicetree@vger.kernel.org, Linux Kernel Mailing List , Linux PM , dri-devel , freedreno , David Airlie , Greg KH , Stephen Boyd , linux-arm-msm , jcrouse@codeaurora.org Content-Type: text/plain; charset="UTF-8" X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1591737886832187485?= X-GMAIL-MSGID: =?utf-8?q?1593122792148770160?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: Hi, On Thu, Feb 22, 2018 at 7:42 PM, Tomasz Figa wrote: > On Thu, Feb 22, 2018 at 10:45 PM, Robin Murphy wrote: >> [sorry, I had intended to reply sooner but clearly forgot] >> >> >> On 16/02/18 00:13, Tomasz Figa wrote: >>> >>> On Fri, Feb 16, 2018 at 2:14 AM, Robin Murphy >>> wrote: >>>> >>>> On 15/02/18 04:17, Tomasz Figa wrote: >>>> [...] >>>>>> >>>>>> >>>>>> Could you elaborate on what kind of locking you are concerned about? >>>>>> As I explained before, the normally happening fast path would lock >>>>>> dev->power_lock only for the brief moment of incrementing the runtime >>>>>> PM usage counter. >>>>> >>>>> >>>>> >>>>> My bad, that's not even it. >>>>> >>>>> The atomic usage counter is incremented beforehands, without any >>>>> locking [1] and the spinlock is acquired only for the sake of >>>>> validating that device's runtime PM state remained valid indeed [2], >>>>> which would be the case in the fast path of the same driver doing two >>>>> mappings in parallel, with the master powered on (and so the SMMU, >>>>> through device links; if master was not powered on already, powering >>>>> on the SMMU is unavoidable anyway and it would add much more latency >>>>> than the spinlock itself). >>>> >>>> >>>> >>>> We now have no locking at all in the map path, and only a per-domain lock >>>> around TLB sync in unmap which is unfortunately necessary for >>>> correctness; >>>> the latter isn't too terrible, since in "serious" hardware it should only >>>> be >>>> serialising a few cpus serving the same device against each other (e.g. >>>> for >>>> multiple queues on a single NIC). >>>> >>>> Putting in a global lock which serialises *all* concurrent map and unmap >>>> calls for *all* unrelated devices makes things worse. Period. Even if the >>>> lock itself were held for the minimum possible time, i.e. trivially >>>> "spin_lock(&lock); spin_unlock(&lock)", the cost of repeatedly bouncing >>>> that >>>> one cache line around between 96 CPUs across two sockets is not >>>> negligible. >>> >>> >>> Fair enough. Note that we're in a quite interesting situation now: >>> a) We need to have runtime PM enabled on Qualcomm SoC to have power >>> properly managed, >>> b) We need to have lock-free map/unmap on such distributed systems, >>> c) If runtime PM is enabled, we need to call into runtime PM from any >>> code that does hardware accesses, otherwise the IOMMU API (and so DMA >>> API and then any V4L2 driver) becomes unusable. >>> >>> I can see one more way that could potentially let us have all the >>> three. How about enabling runtime PM only on selected implementations >>> (e.g. qcom,smmu) and then having all the runtime PM calls surrounded >>> with if (pm_runtime_enabled()), which is lockless? >> >> >> Yes, that's the kind of thing I was gravitating towards - my vague thought >> was adding some flag to the smmu_domain, but pm_runtime_enabled() does look >> conceptually a lot cleaner. > > Great, thanks. Looks like we're in agreement now. \o/ > > Vivek, does this sound reasonable to you? Yea, sound good to me. I will respin the patches. Thanks & Regards Vivek > > Best regards, > Tomasz -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation