From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers Date: Thu, 22 Feb 2018 23:12:19 +0900 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: <28466b36-b5d3-4f60-a45e-b75d79c2a3cb@arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Robin Murphy , Vivek Gautam Cc: Mark Rutland , devicetree@vger.kernel.org, Linux PM , David Airlie , Will Deacon , "Rafael J. Wysocki" , dri-devel , Linux Kernel Mailing List , "list@263.net:IOMMU DRIVERS" , Rob Herring , Greg KH , freedreno , Stephen Boyd , linux-arm-msm List-Id: linux-arm-msm@vger.kernel.org T24gVGh1LCBGZWIgMjIsIDIwMTggYXQgMTA6NDUgUE0sIFJvYmluIE11cnBoeSA8cm9iaW4ubXVy cGh5QGFybS5jb20+IHdyb3RlOgo+IFtzb3JyeSwgSSBoYWQgaW50ZW5kZWQgdG8gcmVwbHkgc29v bmVyIGJ1dCBjbGVhcmx5IGZvcmdvdF0KPgo+Cj4gT24gMTYvMDIvMTggMDA6MTMsIFRvbWFzeiBG aWdhIHdyb3RlOgo+Pgo+PiBPbiBGcmksIEZlYiAxNiwgMjAxOCBhdCAyOjE0IEFNLCBSb2JpbiBN dXJwaHkgPHJvYmluLm11cnBoeUBhcm0uY29tPgo+PiB3cm90ZToKPj4+Cj4+PiBPbiAxNS8wMi8x OCAwNDoxNywgVG9tYXN6IEZpZ2Egd3JvdGU6Cj4+PiBbLi4uXQo+Pj4+Pgo+Pj4+Pgo+Pj4+PiBD b3VsZCB5b3UgZWxhYm9yYXRlIG9uIHdoYXQga2luZCBvZiBsb2NraW5nIHlvdSBhcmUgY29uY2Vy bmVkIGFib3V0Pwo+Pj4+PiBBcyBJIGV4cGxhaW5lZCBiZWZvcmUsIHRoZSBub3JtYWxseSBoYXBw ZW5pbmcgZmFzdCBwYXRoIHdvdWxkIGxvY2sKPj4+Pj4gZGV2LT5wb3dlcl9sb2NrIG9ubHkgZm9y IHRoZSBicmllZiBtb21lbnQgb2YgaW5jcmVtZW50aW5nIHRoZSBydW50aW1lCj4+Pj4+IFBNIHVz YWdlIGNvdW50ZXIuCj4+Pj4KPj4+Pgo+Pj4+Cj4+Pj4gTXkgYmFkLCB0aGF0J3Mgbm90IGV2ZW4g aXQuCj4+Pj4KPj4+PiBUaGUgYXRvbWljIHVzYWdlIGNvdW50ZXIgaXMgaW5jcmVtZW50ZWQgYmVm b3JlaGFuZHMsIHdpdGhvdXQgYW55Cj4+Pj4gbG9ja2luZyBbMV0gYW5kIHRoZSBzcGlubG9jayBp cyBhY3F1aXJlZCBvbmx5IGZvciB0aGUgc2FrZSBvZgo+Pj4+IHZhbGlkYXRpbmcgdGhhdCBkZXZp Y2UncyBydW50aW1lIFBNIHN0YXRlIHJlbWFpbmVkIHZhbGlkIGluZGVlZCBbMl0sCj4+Pj4gd2hp Y2ggd291bGQgYmUgdGhlIGNhc2UgaW4gdGhlIGZhc3QgcGF0aCBvZiB0aGUgc2FtZSBkcml2ZXIg ZG9pbmcgdHdvCj4+Pj4gbWFwcGluZ3MgaW4gcGFyYWxsZWwsIHdpdGggdGhlIG1hc3RlciBwb3dl cmVkIG9uIChhbmQgc28gdGhlIFNNTVUsCj4+Pj4gdGhyb3VnaCBkZXZpY2UgbGlua3M7IGlmIG1h c3RlciB3YXMgbm90IHBvd2VyZWQgb24gYWxyZWFkeSwgcG93ZXJpbmcKPj4+PiBvbiB0aGUgU01N VSBpcyB1bmF2b2lkYWJsZSBhbnl3YXkgYW5kIGl0IHdvdWxkIGFkZCBtdWNoIG1vcmUgbGF0ZW5j eQo+Pj4+IHRoYW4gdGhlIHNwaW5sb2NrIGl0c2VsZikuCj4+Pgo+Pj4KPj4+Cj4+PiBXZSBub3cg aGF2ZSBubyBsb2NraW5nIGF0IGFsbCBpbiB0aGUgbWFwIHBhdGgsIGFuZCBvbmx5IGEgcGVyLWRv bWFpbiBsb2NrCj4+PiBhcm91bmQgVExCIHN5bmMgaW4gdW5tYXAgd2hpY2ggaXMgdW5mb3J0dW5h dGVseSBuZWNlc3NhcnkgZm9yCj4+PiBjb3JyZWN0bmVzczsKPj4+IHRoZSBsYXR0ZXIgaXNuJ3Qg dG9vIHRlcnJpYmxlLCBzaW5jZSBpbiAic2VyaW91cyIgaGFyZHdhcmUgaXQgc2hvdWxkIG9ubHkK Pj4+IGJlCj4+PiBzZXJpYWxpc2luZyBhIGZldyBjcHVzIHNlcnZpbmcgdGhlIHNhbWUgZGV2aWNl IGFnYWluc3QgZWFjaCBvdGhlciAoZS5nLgo+Pj4gZm9yCj4+PiBtdWx0aXBsZSBxdWV1ZXMgb24g YSBzaW5nbGUgTklDKS4KPj4+Cj4+PiBQdXR0aW5nIGluIGEgZ2xvYmFsIGxvY2sgd2hpY2ggc2Vy aWFsaXNlcyAqYWxsKiBjb25jdXJyZW50IG1hcCBhbmQgdW5tYXAKPj4+IGNhbGxzIGZvciAqYWxs KiB1bnJlbGF0ZWQgZGV2aWNlcyBtYWtlcyB0aGluZ3Mgd29yc2UuIFBlcmlvZC4gRXZlbiBpZiB0 aGUKPj4+IGxvY2sgaXRzZWxmIHdlcmUgaGVsZCBmb3IgdGhlIG1pbmltdW0gcG9zc2libGUgdGlt ZSwgaS5lLiB0cml2aWFsbHkKPj4+ICJzcGluX2xvY2soJmxvY2spOyBzcGluX3VubG9jaygmbG9j aykiLCB0aGUgY29zdCBvZiByZXBlYXRlZGx5IGJvdW5jaW5nCj4+PiB0aGF0Cj4+PiBvbmUgY2Fj aGUgbGluZSBhcm91bmQgYmV0d2VlbiA5NiBDUFVzIGFjcm9zcyB0d28gc29ja2V0cyBpcyBub3QK Pj4+IG5lZ2xpZ2libGUuCj4+Cj4+Cj4+IEZhaXIgZW5vdWdoLiBOb3RlIHRoYXQgd2UncmUgaW4g YSBxdWl0ZSBpbnRlcmVzdGluZyBzaXR1YXRpb24gbm93Ogo+PiAgIGEpIFdlIG5lZWQgdG8gaGF2 ZSBydW50aW1lIFBNIGVuYWJsZWQgb24gUXVhbGNvbW0gU29DIHRvIGhhdmUgcG93ZXIKPj4gcHJv cGVybHkgbWFuYWdlZCwKPj4gICBiKSBXZSBuZWVkIHRvIGhhdmUgbG9jay1mcmVlIG1hcC91bm1h cCBvbiBzdWNoIGRpc3RyaWJ1dGVkIHN5c3RlbXMsCj4+ICAgYykgSWYgcnVudGltZSBQTSBpcyBl bmFibGVkLCB3ZSBuZWVkIHRvIGNhbGwgaW50byBydW50aW1lIFBNIGZyb20gYW55Cj4+IGNvZGUg dGhhdCBkb2VzIGhhcmR3YXJlIGFjY2Vzc2VzLCBvdGhlcndpc2UgdGhlIElPTU1VIEFQSSAoYW5k IHNvIERNQQo+PiBBUEkgYW5kIHRoZW4gYW55IFY0TDIgZHJpdmVyKSBiZWNvbWVzIHVudXNhYmxl Lgo+Pgo+PiBJIGNhbiBzZWUgb25lIG1vcmUgd2F5IHRoYXQgY291bGQgcG90ZW50aWFsbHkgbGV0 IHVzIGhhdmUgYWxsIHRoZQo+PiB0aHJlZS4gSG93IGFib3V0IGVuYWJsaW5nIHJ1bnRpbWUgUE0g b25seSBvbiBzZWxlY3RlZCBpbXBsZW1lbnRhdGlvbnMKPj4gKGUuZy4gcWNvbSxzbW11KSBhbmQg dGhlbiBoYXZpbmcgYWxsIHRoZSBydW50aW1lIFBNIGNhbGxzIHN1cnJvdW5kZWQKPj4gd2l0aCBp ZiAocG1fcnVudGltZV9lbmFibGVkKCkpLCB3aGljaCBpcyBsb2NrbGVzcz8KPgo+Cj4gWWVzLCB0 aGF0J3MgdGhlIGtpbmQgb2YgdGhpbmcgSSB3YXMgZ3Jhdml0YXRpbmcgdG93YXJkcyAtIG15IHZh Z3VlIHRob3VnaHQKPiB3YXMgYWRkaW5nIHNvbWUgZmxhZyB0byB0aGUgc21tdV9kb21haW4sIGJ1 dCBwbV9ydW50aW1lX2VuYWJsZWQoKSBkb2VzIGxvb2sKPiBjb25jZXB0dWFsbHkgYSBsb3QgY2xl YW5lci4KCkdyZWF0LCB0aGFua3MuIExvb2tzIGxpa2Ugd2UncmUgaW4gYWdyZWVtZW50IG5vdy4g XG8vCgpWaXZlaywgZG9lcyB0aGlzIHNvdW5kIHJlYXNvbmFibGUgdG8geW91PwoKQmVzdCByZWdh cmRzLApUb21hc3oKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3Jn Cmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVs Cg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: ARC-Seal: i=1; a=rsa-sha256; t=1519308764; cv=none; d=google.com; s=arc-20160816; b=hn6N22u/Z9pV6PTYMP4bKIjIu4YO0GkOd+n3q7PpJlhSonrRtiWy/S/KDgJDEweOKQ 7+zeK6J7boGl9kBS5ejacSGn7T7m/idBCId5YsCQYWcUD4hqa/VrE81qwDVfcCYccqS0 /cPt/5CWT/MsXGw9RjC/dhFqccyJ4F8onM50ZxlBrrpJYe+F4EOl5JLn8TukBJVhLE5x iAbTo4FTIzjW6U/seW7PtTMFa8aVHZ8fydGJU3WztaFcRrwIF5SP9o3rUZitLRNMkL6z ZrHPb82fTAkTPTPW6+641edtwF6zycsdU1y+EI0D5RWsuK/Wy1CT1gtq6sJxSwe03W3H s2Ew== 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:dkim-signature:arc-authentication-results; bh=aKJFfG4wsMng7qV6luft7LTn0ZuMm6S+gP/CavxQMb4=; b=saEIbNL5vR4B+cj7I7Z67NyNsNa3Z0ih4yXQrL2sQep6zajIaTiWbvxoD2FC88JOAd /+UFKQ7rIIlPMepaE+8F/hskKRxVjvRQ5/gYVM93oKMjrtfr3cIC15+Ao835z5eXaQu5 tYHLOUcWAyONLCkjTjB8/BJZE5whUq9NBHmY1D1WXQIkyW3KuHCdKjJ4otjjhgMSQZQD AzvnWwqqtk+q5tXUwZvmSun4g3XO8GpLAzCbjAVbbrcRBT8C3sm4foISIXBB8wl5f5tS U9dnQ+FKhv44q1UYjIClQn5AtxLQLtnNakwZXJuZUBJ5zYl0ZWLMN1CjP0cPadzKEb8T ocgQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=PuvprHqF; spf=pass (google.com: domain of tfiga@chromium.org designates 209.85.220.41 as permitted sender) smtp.mailfrom=tfiga@chromium.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=PuvprHqF; spf=pass (google.com: domain of tfiga@chromium.org designates 209.85.220.41 as permitted sender) smtp.mailfrom=tfiga@chromium.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org X-Google-Smtp-Source: AH8x2240ZS/o5ddBLlBjZ/QYMI4UArYJuhxCLT38WEYgs7V6UWbHRfAmQAInLCF4m3W24/QgntRhEw== MIME-Version: 1.0 In-Reply-To: <28466b36-b5d3-4f60-a45e-b75d79c2a3cb@arm.com> 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: Tomasz Figa Date: Thu, 22 Feb 2018 23:12:19 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers To: Robin Murphy , Vivek Gautam Cc: 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?1593110707024384742?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: 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? Best regards, Tomasz