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: Fri, 16 Feb 2018 09:13:50 +0900 Message-ID: References: <1517999482-17317-1-git-send-email-vivek.gautam@codeaurora.org> <7406f1ce-c2c9-a6bd-2886-5a34de45add6@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <7406f1ce-c2c9-a6bd-2886-5a34de45add6-5wv7dgnIgG8@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: freedreno-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "Freedreno" To: Robin Murphy 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 , "list-Y9sIeH5OGRo@public.gmane.org:IOMMU DRIVERS" , dri-devel , Linux Kernel Mailing List , Rob Clark , Rob Herring , Vivek Gautam , Greg KH , freedreno , Stephen Boyd , linux-arm-msm List-Id: linux-arm-msm@vger.kernel.org T24gRnJpLCBGZWIgMTYsIDIwMTggYXQgMjoxNCBBTSwgUm9iaW4gTXVycGh5IDxyb2Jpbi5tdXJw aHlAYXJtLmNvbT4gd3JvdGU6Cj4gT24gMTUvMDIvMTggMDQ6MTcsIFRvbWFzeiBGaWdhIHdyb3Rl Ogo+IFsuLi5dCj4+Pgo+Pj4gQ291bGQgeW91IGVsYWJvcmF0ZSBvbiB3aGF0IGtpbmQgb2YgbG9j a2luZyB5b3UgYXJlIGNvbmNlcm5lZCBhYm91dD8KPj4+IEFzIEkgZXhwbGFpbmVkIGJlZm9yZSwg dGhlIG5vcm1hbGx5IGhhcHBlbmluZyBmYXN0IHBhdGggd291bGQgbG9jawo+Pj4gZGV2LT5wb3dl cl9sb2NrIG9ubHkgZm9yIHRoZSBicmllZiBtb21lbnQgb2YgaW5jcmVtZW50aW5nIHRoZSBydW50 aW1lCj4+PiBQTSB1c2FnZSBjb3VudGVyLgo+Pgo+Pgo+PiBNeSBiYWQsIHRoYXQncyBub3QgZXZl biBpdC4KPj4KPj4gVGhlIGF0b21pYyB1c2FnZSBjb3VudGVyIGlzIGluY3JlbWVudGVkIGJlZm9y ZWhhbmRzLCB3aXRob3V0IGFueQo+PiBsb2NraW5nIFsxXSBhbmQgdGhlIHNwaW5sb2NrIGlzIGFj cXVpcmVkIG9ubHkgZm9yIHRoZSBzYWtlIG9mCj4+IHZhbGlkYXRpbmcgdGhhdCBkZXZpY2UncyBy dW50aW1lIFBNIHN0YXRlIHJlbWFpbmVkIHZhbGlkIGluZGVlZCBbMl0sCj4+IHdoaWNoIHdvdWxk IGJlIHRoZSBjYXNlIGluIHRoZSBmYXN0IHBhdGggb2YgdGhlIHNhbWUgZHJpdmVyIGRvaW5nIHR3 bwo+PiBtYXBwaW5ncyBpbiBwYXJhbGxlbCwgd2l0aCB0aGUgbWFzdGVyIHBvd2VyZWQgb24gKGFu ZCBzbyB0aGUgU01NVSwKPj4gdGhyb3VnaCBkZXZpY2UgbGlua3M7IGlmIG1hc3RlciB3YXMgbm90 IHBvd2VyZWQgb24gYWxyZWFkeSwgcG93ZXJpbmcKPj4gb24gdGhlIFNNTVUgaXMgdW5hdm9pZGFi bGUgYW55d2F5IGFuZCBpdCB3b3VsZCBhZGQgbXVjaCBtb3JlIGxhdGVuY3kKPj4gdGhhbiB0aGUg c3BpbmxvY2sgaXRzZWxmKS4KPgo+Cj4gV2Ugbm93IGhhdmUgbm8gbG9ja2luZyBhdCBhbGwgaW4g dGhlIG1hcCBwYXRoLCBhbmQgb25seSBhIHBlci1kb21haW4gbG9jawo+IGFyb3VuZCBUTEIgc3lu YyBpbiB1bm1hcCB3aGljaCBpcyB1bmZvcnR1bmF0ZWx5IG5lY2Vzc2FyeSBmb3IgY29ycmVjdG5l c3M7Cj4gdGhlIGxhdHRlciBpc24ndCB0b28gdGVycmlibGUsIHNpbmNlIGluICJzZXJpb3VzIiBo YXJkd2FyZSBpdCBzaG91bGQgb25seSBiZQo+IHNlcmlhbGlzaW5nIGEgZmV3IGNwdXMgc2Vydmlu ZyB0aGUgc2FtZSBkZXZpY2UgYWdhaW5zdCBlYWNoIG90aGVyIChlLmcuIGZvcgo+IG11bHRpcGxl IHF1ZXVlcyBvbiBhIHNpbmdsZSBOSUMpLgo+Cj4gUHV0dGluZyBpbiBhIGdsb2JhbCBsb2NrIHdo aWNoIHNlcmlhbGlzZXMgKmFsbCogY29uY3VycmVudCBtYXAgYW5kIHVubWFwCj4gY2FsbHMgZm9y ICphbGwqIHVucmVsYXRlZCBkZXZpY2VzIG1ha2VzIHRoaW5ncyB3b3JzZS4gUGVyaW9kLiBFdmVu IGlmIHRoZQo+IGxvY2sgaXRzZWxmIHdlcmUgaGVsZCBmb3IgdGhlIG1pbmltdW0gcG9zc2libGUg dGltZSwgaS5lLiB0cml2aWFsbHkKPiAic3Bpbl9sb2NrKCZsb2NrKTsgc3Bpbl91bmxvY2soJmxv Y2spIiwgdGhlIGNvc3Qgb2YgcmVwZWF0ZWRseSBib3VuY2luZyB0aGF0Cj4gb25lIGNhY2hlIGxp bmUgYXJvdW5kIGJldHdlZW4gOTYgQ1BVcyBhY3Jvc3MgdHdvIHNvY2tldHMgaXMgbm90IG5lZ2xp Z2libGUuCgpGYWlyIGVub3VnaC4gTm90ZSB0aGF0IHdlJ3JlIGluIGEgcXVpdGUgaW50ZXJlc3Rp bmcgc2l0dWF0aW9uIG5vdzoKIGEpIFdlIG5lZWQgdG8gaGF2ZSBydW50aW1lIFBNIGVuYWJsZWQg b24gUXVhbGNvbW0gU29DIHRvIGhhdmUgcG93ZXIKcHJvcGVybHkgbWFuYWdlZCwKIGIpIFdlIG5l ZWQgdG8gaGF2ZSBsb2NrLWZyZWUgbWFwL3VubWFwIG9uIHN1Y2ggZGlzdHJpYnV0ZWQgc3lzdGVt cywKIGMpIElmIHJ1bnRpbWUgUE0gaXMgZW5hYmxlZCwgd2UgbmVlZCB0byBjYWxsIGludG8gcnVu dGltZSBQTSBmcm9tIGFueQpjb2RlIHRoYXQgZG9lcyBoYXJkd2FyZSBhY2Nlc3Nlcywgb3RoZXJ3 aXNlIHRoZSBJT01NVSBBUEkgKGFuZCBzbyBETUEKQVBJIGFuZCB0aGVuIGFueSBWNEwyIGRyaXZl cikgYmVjb21lcyB1bnVzYWJsZS4KCkkgY2FuIHNlZSBvbmUgbW9yZSB3YXkgdGhhdCBjb3VsZCBw b3RlbnRpYWxseSBsZXQgdXMgaGF2ZSBhbGwgdGhlCnRocmVlLiBIb3cgYWJvdXQgZW5hYmxpbmcg cnVudGltZSBQTSBvbmx5IG9uIHNlbGVjdGVkIGltcGxlbWVudGF0aW9ucwooZS5nLiBxY29tLHNt bXUpIGFuZCB0aGVuIGhhdmluZyBhbGwgdGhlIHJ1bnRpbWUgUE0gY2FsbHMgc3Vycm91bmRlZAp3 aXRoIGlmIChwbV9ydW50aW1lX2VuYWJsZWQoKSksIHdoaWNoIGlzIGxvY2tsZXNzPwoKPgo+PiBb MV0KPj4gaHR0cDovL2VsaXhpci5mcmVlLWVsZWN0cm9ucy5jb20vbGludXgvdjQuMTYtcmMxL3Nv dXJjZS9kcml2ZXJzL2Jhc2UvcG93ZXIvcnVudGltZS5jI0wxMDI4Cj4+IFsyXQo+PiBodHRwOi8v ZWxpeGlyLmZyZWUtZWxlY3Ryb25zLmNvbS9saW51eC92NC4xNi1yYzEvc291cmNlL2RyaXZlcnMv YmFzZS9wb3dlci9ydW50aW1lLmMjTDYxMwo+Pgo+PiBJbiBhbnkgY2FzZSwgSSBjYW4ndCBpbWFn aW5lIHRoaXMgd29ya2luZyB3aXRoIFY0TDIgb3IgYW55dGhpbmcgZWxzZQo+PiByZWx5aW5nIG9u IGFueSBtZW1vcnkgbWFuYWdlbWVudCBtb3JlIGdlbmVyaWMgdGhhbiBjYWxsaW5nIElPTU1VIEFQ SQo+PiBkaXJlY3RseSBmcm9tIHRoZSBkcml2ZXIsIHdpdGggdGhlIElPTU1VIGRldmljZSBoYXZp bmcgcnVudGltZSBQTQo+PiBlbmFibGVkLCBidXQgd2l0aG91dCBtYW5hZ2luZyB0aGUgcnVudGlt ZSBQTSBmcm9tIHRoZSBJT01NVSBkcml2ZXIncwo+PiBjYWxsYmFja3MgdGhhdCBuZWVkIGFjY2Vz cyB0byB0aGUgaGFyZHdhcmUuIEFzIEkgbWVudGlvbmVkIGJlZm9yZSwKPj4gb25seSB0aGUgSU9N TVUgZHJpdmVyIGtub3dzIHdoZW4gZXhhY3RseSB0aGUgcmVhbCBoYXJkd2FyZSBhY2Nlc3MKPj4g bmVlZHMgdG8gYmUgZG9uZSAoZS5nLiBSb2NrY2hpcC9FeHlub3MgZG9uJ3QgbmVlZCB0byBkbyB0 aGF0IGZvcgo+PiBtYXAvdW5tYXAgaWYgdGhlIHBvd2VyIGlzIGRvd24sIGJ1dCBzb21lIGltcGxl bWVudGF0aW9ucyBvZiBTTU1VIHdpdGgKPj4gVExCIHBvd2VyZWQgc2VwYXJhdGVseSBtaWdodCBu ZWVkIHRvIGRvIHNvKS4KPgo+Cj4gSXQncyB3b3J0aCBub3RpbmcgdGhhdCBFeHlub3MgYW5kIFJv Y2tjaGlwIGFyZSByZWxhdGl2ZWx5IHNtYWxsCj4gc2VsZi1jb250YWluZWQgSVAgYmxvY2tzIGlu dGVncmF0ZWQgY2xvc2VseSB3aXRoIHRoZSBpbnRlcmZhY2VzIG9mIHRoZWlyCj4gcmVsZXZhbnQg bWFzdGVyIGRldmljZXM7IFNNTVUgaXMgYW4gYXJjaGl0ZWN0dXJlLCBpbXBsZW1lbnRhdGlvbnMg b2Ygd2hpY2gKPiBtYXkgYmUgbGFyZ2UsIGRpc3RyaWJ1dGVkLCBhbmQgaGF2ZSBjb21wbGV4IGFu ZCB3aWxkbHkgZGlmZmVyaW5nIGludGVybmFsCj4gdG9wb2xvZ2llcy4gQXMgc3VjaCwgaXQncyBh IGxvdCBoYXJkZXIgdG8gbWFrZSBoYXJkd2FyZS1zcGVjaWZpYyBhc3N1bXB0aW9ucwo+IGFuZC9v ciBiZSBjb3JyZWN0IGZvciBhbGwgcG9zc2libGUgY2FzZXMuCj4KPiBEb24ndCBnZXQgbWUgd3Jv bmcsIEkgZG8gdWx0aW1hdGVseSBhZ3JlZSB0aGF0IHRoZSBJT01NVSBkcml2ZXIgaXMgdGhlIG9u bHkKPiBhZ2VudCB3aG8gdWx0aW1hdGVseSBrbm93cyB3aGF0IGNhbGxzIGFyZSBnb2luZyB0byBi ZSBuZWNlc3NhcnkgZm9yIHdoYXRldmVyCj4gb3BlcmF0aW9uIGl0J3MgcGVyZm9ybWluZyBvbiBp dHMgb3duIGhhcmR3YXJlKjsgaXQncyBqdXN0IHRoYXQgZm9yIFNNTVUgaXQKPiBuZWVkcyB0byBi ZSBpbXBsZW1lbnRlZCBpbiBhIHdheSB0aGF0IGhhcyB6ZXJvIGltcGFjdCBvbiB0aGUgY2FzZXMg d2hlcmUgaXQKPiBkb2Vzbid0IG1hdHRlciwgYmVjYXVzZSBpdCdzIG5vdCB2aWFibGUgdG8gc3Bl Y2lhbGlzZSB0aGF0IGRyaXZlciBmb3IgYW55Cj4gcGFydGljdWxhciBJUCBpbXBsZW1lbnRhdGlv bi91c2UtY2FzZS4KClN0aWxsLCBleGFjdGx5IHRoZSBzYW1lIGhvbGRzIGZvciB0aGUgbG93IHBv d2VyIGVtYmVkZGVkIHVzZSBjYXNlcywKd2hlcmUgd2Ugc3RyaXZlIGZvciB0aGUgbG93ZXN0IHBv c3NpYmxlIHBvd2VyIGNvbnN1bXB0aW9uLCB3aGlsZQptYWludGFpbmluZyBwZXJmb3JtYW5jZSBs ZXZlbHMgaGlnaCBhcyB3ZWxsLiBBbmQgc28gdGhlIFNNTVUgY29kZSBpcwpleHBlY3RlZCB0byBh bHNvIHdvcmsgd2l0aCBvdXIgdXNlIGNhc2VzLCBzdWNoIGFzIFY0TDIgb3IgRFJNIGRyaXZlcnMu ClNpbmNlIHRoZXNlIHBvaW50cyBkb24ndCBob2xkIGZvciBjdXJyZW50IFNNTVUgY29kZSwgSSBj b3VsZCBzYXkgdGhhdAp0aGUgaXQgaGFzIGJlZW4gYWxyZWFkeSBzcGVjaWFsaXplZCBmb3IgbGFy Z2UsIGRpc3RyaWJ1dGVkCmltcGxlbWVudGF0aW9ucy4KCkJlc3QgcmVnYXJkcywKVG9tYXN6Cl9f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCkZyZWVkcmVubyBt YWlsaW5nIGxpc3QKRnJlZWRyZW5vQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3Rz LmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2ZyZWVkcmVubwo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: ARC-Seal: i=1; a=rsa-sha256; t=1518740055; cv=none; d=google.com; s=arc-20160816; b=SV8EYAswGd7Uogbxb6m1srE0os7XaRnTQ09V1BvReiQXRCxiqgzhN1VBJRiXelV/F4 /1Y4sWQiWc67OW1KDKjqNINcqgtFJDjEUNU1hzGJW+CFCPY9NQRh8cAda9FPGAvkG2gh Ib8nn7/3WH3sK3z305+f9IrIcYU2VHoYo59QerdxJ1nibl1BShOALqSUd/X0Da53EJqh K58MhX1W7wvzAFBMbZydJ16J8HXgAfK3Yde0CEpnqWUvANOnZalFjpdbNMpYqUmzCYLu JkM4tz00fX+p4MZXaH7BAAFH60FDdGxAuoSGSZR14suTCWc/zlGAhv1mk8hu4o6XdWZ1 xuGg== 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=3iwGQ+YCglKALHoird8t+GuQaLlnMdFI5CwSxgmgWyo=; b=0nlam5GPZr3ejXdz17M4aJ+GCVayQaLFCM3QDdn1ZyDxNuMf2kLsoy6/RKzqNodKRI 2Y0hLGhCx52dDc0aJ444HNY/t0laWY6MHv44tG8SuZFc5MLtlxN6axcoYyh1sILbPYyy sSC9EqG7YlGWinEhZwCDp7rpibXzm/gUWO3EBoeRt78Jf7/L1FuS1MB//o8qnuPO0UFQ 71p8Vu4GEcBDE/K2SfG3Gh67m19ZFaS4d16bB+tLoTy2TuczoVhpLnq8Q3mGPPR3UDyn iA8RVa0RHS2LClWglAd+ASpQh/MjGw32lgQ8slgw5Ik/QN5kIkk5aLI9I6mTKqRXtI7I +O7g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=h73dtpNY; 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=h73dtpNY; 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: AH8x224Z0d1W4Rhu99NtQXQ8C8J+9nVtMkPfFmOQ5oxKiV/0CWqm0fN9EXSkN2FeKLdx8eEKxVsH1g== MIME-Version: 1.0 In-Reply-To: <7406f1ce-c2c9-a6bd-2886-5a34de45add6@arm.com> References: <1517999482-17317-1-git-send-email-vivek.gautam@codeaurora.org> <7406f1ce-c2c9-a6bd-2886-5a34de45add6@arm.com> From: Tomasz Figa Date: Fri, 16 Feb 2018 09:13:50 +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 Cc: Vivek Gautam , Will Deacon , Rob Clark , "list@263.net:IOMMU DRIVERS" , Joerg Roedel , Rob Herring , Mark Rutland , "Rafael J. Wysocki" , 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?1592514372997667847?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: 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? > >> [1] >> http://elixir.free-electrons.com/linux/v4.16-rc1/source/drivers/base/power/runtime.c#L1028 >> [2] >> http://elixir.free-electrons.com/linux/v4.16-rc1/source/drivers/base/power/runtime.c#L613 >> >> In any case, I can't imagine this working with V4L2 or anything else >> relying on any memory management more generic than calling IOMMU API >> directly from the driver, with the IOMMU device having runtime PM >> enabled, but without managing the runtime PM from the IOMMU driver's >> callbacks that need access to the hardware. As I mentioned before, >> only the IOMMU driver knows when exactly the real hardware access >> needs to be done (e.g. Rockchip/Exynos don't need to do that for >> map/unmap if the power is down, but some implementations of SMMU with >> TLB powered separately might need to do so). > > > It's worth noting that Exynos and Rockchip are relatively small > self-contained IP blocks integrated closely with the interfaces of their > relevant master devices; SMMU is an architecture, implementations of which > may be large, distributed, and have complex and wildly differing internal > topologies. As such, it's a lot harder to make hardware-specific assumptions > and/or be correct for all possible cases. > > Don't get me wrong, I do ultimately agree that the IOMMU driver is the only > agent who ultimately knows what calls are going to be necessary for whatever > operation it's performing on its own hardware*; it's just that for SMMU it > needs to be implemented in a way that has zero impact on the cases where it > doesn't matter, because it's not viable to specialise that driver for any > particular IP implementation/use-case. Still, exactly the same holds for the low power embedded use cases, where we strive for the lowest possible power consumption, while maintaining performance levels high as well. And so the SMMU code is expected to also work with our use cases, such as V4L2 or DRM drivers. Since these points don't hold for current SMMU code, I could say that the it has been already specialized for large, distributed implementations. Best regards, Tomasz