From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rob Clark Subject: Re: [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers Date: Thu, 22 Feb 2018 08:30:35 -0500 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: 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, Linux PM , David Airlie , Will Deacon , Joerg Roedel , "Rafael J. Wysocki" , "list-Y9sIeH5OGRo@public.gmane.org:IOMMU DRIVERS" , dri-devel , Linux Kernel Mailing List , Jordan Crouse , Rob Herring , Vivek Gautam , Greg KH , freedreno , Robin Murphy , Stephen Boyd , linux-arm-msm List-Id: linux-arm-msm@vger.kernel.org T24gVGh1LCBGZWIgMjIsIDIwMTggYXQgMzoxMyBBTSwgVG9tYXN6IEZpZ2EgPHRmaWdhQGNocm9t aXVtLm9yZz4gd3JvdGU6Cj4gT24gRnJpLCBGZWIgMTYsIDIwMTggYXQgOToxMyBBTSwgVG9tYXN6 IEZpZ2EgPHRmaWdhQGNocm9taXVtLm9yZz4gd3JvdGU6Cj4+IE9uIEZyaSwgRmViIDE2LCAyMDE4 IGF0IDI6MTQgQU0sIFJvYmluIE11cnBoeSA8cm9iaW4ubXVycGh5QGFybS5jb20+IHdyb3RlOgo+ Pj4gT24gMTUvMDIvMTggMDQ6MTcsIFRvbWFzeiBGaWdhIHdyb3RlOgo+Pj4gWy4uLl0KPj4+Pj4K Pj4+Pj4gQ291bGQgeW91IGVsYWJvcmF0ZSBvbiB3aGF0IGtpbmQgb2YgbG9ja2luZyB5b3UgYXJl IGNvbmNlcm5lZCBhYm91dD8KPj4+Pj4gQXMgSSBleHBsYWluZWQgYmVmb3JlLCB0aGUgbm9ybWFs bHkgaGFwcGVuaW5nIGZhc3QgcGF0aCB3b3VsZCBsb2NrCj4+Pj4+IGRldi0+cG93ZXJfbG9jayBv bmx5IGZvciB0aGUgYnJpZWYgbW9tZW50IG9mIGluY3JlbWVudGluZyB0aGUgcnVudGltZQo+Pj4+ PiBQTSB1c2FnZSBjb3VudGVyLgo+Pj4+Cj4+Pj4KPj4+PiBNeSBiYWQsIHRoYXQncyBub3QgZXZl biBpdC4KPj4+Pgo+Pj4+IFRoZSBhdG9taWMgdXNhZ2UgY291bnRlciBpcyBpbmNyZW1lbnRlZCBi ZWZvcmVoYW5kcywgd2l0aG91dCBhbnkKPj4+PiBsb2NraW5nIFsxXSBhbmQgdGhlIHNwaW5sb2Nr IGlzIGFjcXVpcmVkIG9ubHkgZm9yIHRoZSBzYWtlIG9mCj4+Pj4gdmFsaWRhdGluZyB0aGF0IGRl dmljZSdzIHJ1bnRpbWUgUE0gc3RhdGUgcmVtYWluZWQgdmFsaWQgaW5kZWVkIFsyXSwKPj4+PiB3 aGljaCB3b3VsZCBiZSB0aGUgY2FzZSBpbiB0aGUgZmFzdCBwYXRoIG9mIHRoZSBzYW1lIGRyaXZl ciBkb2luZyB0d28KPj4+PiBtYXBwaW5ncyBpbiBwYXJhbGxlbCwgd2l0aCB0aGUgbWFzdGVyIHBv d2VyZWQgb24gKGFuZCBzbyB0aGUgU01NVSwKPj4+PiB0aHJvdWdoIGRldmljZSBsaW5rczsgaWYg bWFzdGVyIHdhcyBub3QgcG93ZXJlZCBvbiBhbHJlYWR5LCBwb3dlcmluZwo+Pj4+IG9uIHRoZSBT TU1VIGlzIHVuYXZvaWRhYmxlIGFueXdheSBhbmQgaXQgd291bGQgYWRkIG11Y2ggbW9yZSBsYXRl bmN5Cj4+Pj4gdGhhbiB0aGUgc3BpbmxvY2sgaXRzZWxmKS4KPj4+Cj4+Pgo+Pj4gV2Ugbm93IGhh dmUgbm8gbG9ja2luZyBhdCBhbGwgaW4gdGhlIG1hcCBwYXRoLCBhbmQgb25seSBhIHBlci1kb21h aW4gbG9jawo+Pj4gYXJvdW5kIFRMQiBzeW5jIGluIHVubWFwIHdoaWNoIGlzIHVuZm9ydHVuYXRl bHkgbmVjZXNzYXJ5IGZvciBjb3JyZWN0bmVzczsKPj4+IHRoZSBsYXR0ZXIgaXNuJ3QgdG9vIHRl cnJpYmxlLCBzaW5jZSBpbiAic2VyaW91cyIgaGFyZHdhcmUgaXQgc2hvdWxkIG9ubHkgYmUKPj4+ IHNlcmlhbGlzaW5nIGEgZmV3IGNwdXMgc2VydmluZyB0aGUgc2FtZSBkZXZpY2UgYWdhaW5zdCBl YWNoIG90aGVyIChlLmcuIGZvcgo+Pj4gbXVsdGlwbGUgcXVldWVzIG9uIGEgc2luZ2xlIE5JQyku Cj4+Pgo+Pj4gUHV0dGluZyBpbiBhIGdsb2JhbCBsb2NrIHdoaWNoIHNlcmlhbGlzZXMgKmFsbCog Y29uY3VycmVudCBtYXAgYW5kIHVubWFwCj4+PiBjYWxscyBmb3IgKmFsbCogdW5yZWxhdGVkIGRl dmljZXMgbWFrZXMgdGhpbmdzIHdvcnNlLiBQZXJpb2QuIEV2ZW4gaWYgdGhlCj4+PiBsb2NrIGl0 c2VsZiB3ZXJlIGhlbGQgZm9yIHRoZSBtaW5pbXVtIHBvc3NpYmxlIHRpbWUsIGkuZS4gdHJpdmlh bGx5Cj4+PiAic3Bpbl9sb2NrKCZsb2NrKTsgc3Bpbl91bmxvY2soJmxvY2spIiwgdGhlIGNvc3Qg b2YgcmVwZWF0ZWRseSBib3VuY2luZyB0aGF0Cj4+PiBvbmUgY2FjaGUgbGluZSBhcm91bmQgYmV0 d2VlbiA5NiBDUFVzIGFjcm9zcyB0d28gc29ja2V0cyBpcyBub3QgbmVnbGlnaWJsZS4KPj4KPj4g RmFpciBlbm91Z2guIE5vdGUgdGhhdCB3ZSdyZSBpbiBhIHF1aXRlIGludGVyZXN0aW5nIHNpdHVh dGlvbiBub3c6Cj4+ICBhKSBXZSBuZWVkIHRvIGhhdmUgcnVudGltZSBQTSBlbmFibGVkIG9uIFF1 YWxjb21tIFNvQyB0byBoYXZlIHBvd2VyCj4+IHByb3Blcmx5IG1hbmFnZWQsCj4+ICBiKSBXZSBu ZWVkIHRvIGhhdmUgbG9jay1mcmVlIG1hcC91bm1hcCBvbiBzdWNoIGRpc3RyaWJ1dGVkIHN5c3Rl bXMsCj4+ICBjKSBJZiBydW50aW1lIFBNIGlzIGVuYWJsZWQsIHdlIG5lZWQgdG8gY2FsbCBpbnRv IHJ1bnRpbWUgUE0gZnJvbSBhbnkKPj4gY29kZSB0aGF0IGRvZXMgaGFyZHdhcmUgYWNjZXNzZXMs IG90aGVyd2lzZSB0aGUgSU9NTVUgQVBJIChhbmQgc28gRE1BCj4+IEFQSSBhbmQgdGhlbiBhbnkg VjRMMiBkcml2ZXIpIGJlY29tZXMgdW51c2FibGUuCj4+Cj4+IEkgY2FuIHNlZSBvbmUgbW9yZSB3 YXkgdGhhdCBjb3VsZCBwb3RlbnRpYWxseSBsZXQgdXMgaGF2ZSBhbGwgdGhlCj4+IHRocmVlLiBI b3cgYWJvdXQgZW5hYmxpbmcgcnVudGltZSBQTSBvbmx5IG9uIHNlbGVjdGVkIGltcGxlbWVudGF0 aW9ucwo+PiAoZS5nLiBxY29tLHNtbXUpIGFuZCB0aGVuIGhhdmluZyBhbGwgdGhlIHJ1bnRpbWUg UE0gY2FsbHMgc3Vycm91bmRlZAo+PiB3aXRoIGlmIChwbV9ydW50aW1lX2VuYWJsZWQoKSksIHdo aWNoIGlzIGxvY2tsZXNzPwo+Pgo+Cj4gU29ycnkgZm9yIHBpbmdpbmcsIGJ1dCBhbnkgb3Bpbmlv biBvbiB0aGlzIGtpbmQgb2YgYXBwcm9hY2g/Cj4KCkl0IGlzIG9rIGJ5IG1lLCBmb3Igd2hhdGV2 ZXIgdGhhdCBpcyB3b3J0aAoKQlIsCi1SCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fCkZyZWVkcmVubyBtYWlsaW5nIGxpc3QKRnJlZWRyZW5vQGxpc3RzLmZy ZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3Rp bmZvL2ZyZWVkcmVubwo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: ARC-Seal: i=1; a=rsa-sha256; t=1519306237; cv=none; d=google.com; s=arc-20160816; b=AxfzeavprO0I8u8PPy+wjuQl646HCFgCG/Mhto/kIIsCReDsJwoUzzWWbEltArQqIb qixZYRi8bKLuSlOk6aF5IRyct6m5/WMDPaEQeGm8nToWryfvo3ZhksdyIJlhaL6rbdBN hSY7b0J5eykiN97iGwGXbG7gDfr99hLpXQVsbfujrWgFHVPJiF4xihQstUEutKWWuXZQ QJmPUNjc3ozTvGnVJ/QiNZdz6e+5zNp2BnoN5au3dZa8kB27fWPJY25xTjp0SUSgBclE R8wIwZ0zBrmCocI2aliLm4kfbjbubeH7HR/pA6CkZzn1vpKaJIib6pQCa+jYL33Fo1M4 OUcw== 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=nQ0pzz3DscSiLHgEor+6L44bgNqEqtnGsHdNflsCcws=; b=C12N1lc4GjLyP57v3y5iN0xea8umB5Eh716EuX8cD9F+VVZt/Q5O25lH2UST8MMmGS seMw4xPJiYcwcYybvT3Qc56GXeVCWLo/W2cxcF91LQQ+m4kh+M2sEkxUFTErEFf7e4bM uO3yyFmUol8iET/I8VtCNicjnwhL0iEo8CCaaDYEDt5hI3deht8Co9vAVQhVT633jQLH CbeLpS9H4BdR4pCnfzngH5a2XzFWhvV2xtixVZU/In/rxydIFHXAgoj450T5ZM0dn7vy bU7qqM2X9my5CXyhOsIobwwLlhE1bclUE8h46m1IQHUAENP2L0A17nBEYwa3skxLiVrN xu2Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=P8TriEc7; spf=pass (google.com: domain of robdclark@gmail.com designates 209.85.220.65 as permitted sender) smtp.mailfrom=robdclark@gmail.com; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=P8TriEc7; spf=pass (google.com: domain of robdclark@gmail.com designates 209.85.220.65 as permitted sender) smtp.mailfrom=robdclark@gmail.com; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com X-Google-Smtp-Source: AH8x224U93SJEKZU1hvK0j0QS8SgrRc27icKqCD7gKDGkVKCoJdEP33Mr/R8YwKr1Eoxj/fRUdpMJscRqKTlXKN3lVI= 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> From: Rob Clark Date: Thu, 22 Feb 2018 08:30:35 -0500 Message-ID: Subject: Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers To: Tomasz Figa Cc: Robin Murphy , Mark Rutland , devicetree@vger.kernel.org, Jordan Crouse , Linux PM , David Airlie , "Rafael J. Wysocki" , Joerg Roedel , Will Deacon , "list@263.net:IOMMU DRIVERS" , dri-devel , Linux Kernel Mailing List , Rob Herring , Vivek Gautam , Greg KH , freedreno , Stephen Boyd , linux-arm-msm 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?1593108057154142888?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: On Thu, Feb 22, 2018 at 3:13 AM, Tomasz Figa wrote: > On Fri, Feb 16, 2018 at 9:13 AM, 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? >> > > Sorry for pinging, but any opinion on this kind of approach? > It is ok by me, for whatever that is worth BR, -R