From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?B?Q2hyaXN0aWFuIEvDtm5pZw==?= Subject: Re: [PATCH 09/17] drm/radeon: use common fence implementation for fences Date: Wed, 23 Jul 2014 08:52:15 +0200 Message-ID: <53CF5B9F.1050800@amd.com> References: <20140709093124.11354.3774.stgit@patser> <20140709122953.11354.46381.stgit@patser> <53CE2421.5040906@amd.com> <20140722114607.GL15237@phenom.ffwll.local> <20140722115737.GN15237@phenom.ffwll.local> <53CE56ED.4040109@vodafone.de> <20140722132652.GO15237@phenom.ffwll.local> <53CE6AFA.1060807@vodafone.de> <53CE84AA.9030703@amd.com> <53CE8A57.2000803@vodafone.de> <53CF58FB.8070609@canonical.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <53CF58FB.8070609-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: nouveau-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "Nouveau" To: Maarten Lankhorst , =?UTF-8?B?Q2hyaXM=?= =?UTF-8?B?dGlhbiBLw7ZuaWc=?= , Daniel Vetter Cc: Thomas Hellstrom , nouveau , LKML , dri-devel , Ben Skeggs , "Deucher, Alexander" List-Id: nouveau.vger.kernel.org QW0gMjMuMDcuMjAxNCAwODo0MCwgc2NocmllYiBNYWFydGVuIExhbmtob3JzdDoKPiBvcCAyMi0w Ny0xNCAxNzo1OSwgQ2hyaXN0aWFuIEvDtm5pZyBzY2hyZWVmOgo+PiBBbSAyMi4wNy4yMDE0IDE3 OjQyLCBzY2hyaWViIERhbmllbCBWZXR0ZXI6Cj4+PiBPbiBUdWUsIEp1bCAyMiwgMjAxNCBhdCA1 OjM1IFBNLCBDaHJpc3RpYW4gS8O2bmlnCj4+PiA8Y2hyaXN0aWFuLmtvZW5pZ0BhbWQuY29tPiB3 cm90ZToKPj4+PiBEcml2ZXJzIGV4cG9ydGluZyBmZW5jZXMgbmVlZCB0byBwcm92aWRlIGEgZmVu Y2UtPnNpZ25hbGVkIGFuZCBhIGZlbmNlLT53YWl0Cj4+Pj4gZnVuY3Rpb24sIGV2ZXJ5dGhpbmcg ZWxzZSBsaWtlIGZlbmNlLT5lbmFibGVfc2lnbmFsaW5nIG9yIGNhbGxpbmcKPj4+PiBmZW5jZV9z aWduYWxlZCgpIGZyb20gdGhlIGRyaXZlciBpcyBvcHRpb25hbC4KPj4+Pgo+Pj4+IERyaXZlcnMg d2FudGluZyB0byB1c2UgZXhwb3J0ZWQgZmVuY2VzIGRvbid0IGNhbGwgZmVuY2UtPnNpZ25hbGVk IG9yCj4+Pj4gZmVuY2UtPndhaXQgaW4gYXRvbWljIG9yIGludGVycnVwdCBjb250ZXh0LCBhbmQg bm90IHdpdGggaG9sZGluZyBhbnkgZ2xvYmFsCj4+Pj4gbG9ja2luZyBwcmltaXRpdmVzIChsaWtl IG1tYXBfc2VtIGV0Yy4uLikuIEhvbGRpbmcgbG9ja2luZyBwcmltaXRpdmVzIGxvY2FsCj4+Pj4g dG8gdGhlIGRyaXZlciBpcyBvaywgYXMgbG9uZyBhcyB0aGV5IGRvbid0IGNvbmZsaWN0IHdpdGgg YW55dGhpbmcgcG9zc2libGUKPj4+PiB1c2VkIGJ5IHRoZWlyIG93biBmZW5jZSBpbXBsZW1lbnRh dGlvbi4KPj4+IFdlbGwgdGhhdCdzIGFsbW9zdCB3aGF0IHdlIGhhdmUgcmlnaHQgbm93IHdpdGgg dGhlIGV4Y2VwdGlvbiB0aGF0Cj4+PiBkcml2ZXJzIGFyZSBhbGxvd2VkIChhY3R1YWxseSBtdXN0 IGZvciBjb3JyZWN0bmVzcyB3aGVuIHVwZGF0aW5nCj4+PiBmZW5jZXMpIHRoZSB3d19tdXRleGVz IGZvciBkbWEtYnVmcyAob3Igb3RoZXIgYnVmZmVyIG9iamVjdHMpLgo+PiBJbiB0aGlzIGNhc2Ug c29ycnkgZm9yIHNvIG11Y2ggbm9pc2UuIEkgcmVhbGx5IGhhdmVuJ3QgbG9va2VkIGluIHNvIG11 Y2ggZGV0YWlsIGludG8gYW55dGhpbmcgYnV0IE1hYXJ0ZW4ncyBSYWRlb24gcGF0Y2hlcy4KPj4K Pj4gQnV0IGhvdyBkb2VzIHRoYXQgdGhlbiB3b3JrIHJpZ2h0IG5vdz8gTXkgaW1wcmVzc2lvbiB3 YXMgdGhhdCBpdCdzIG1hbmRhdG9yeSBmb3IgZHJpdmVycyB0byBjYWxsIGZlbmNlX3NpZ25hbGVk KCk/Cj4gSXQncyBvbmx5IG1hbmRhdG9yeSB0byBjYWxsIGZlbmNlX3NpZ25hbCgpIGlmIHRoZSAu ZW5hYmxlX3NpZ25hbGluZyBjYWxsYmFjayBoYXMgYmVlbiBjYWxsZWQsIGVsc2UgeW91IGNhbiBn ZXQgYXdheSB3aXRoIG5ldmVyIGNhbGxpbmcgc2lnbmFsaW5nIGEgZmVuY2UgYXQgYWxsIGJlZm9y ZSBkcm9wcGluZyB0aGUgbGFzdCByZWZjb3VudCB0byBpdC4KPiBUaGlzIGFsbG93cyB5b3UgdG8g a2VlcCBpbnRlcnJ1cHRzIGRpc2FibGVkIHdoZW4geW91IGRvbid0IG5lZWQgdGhlbS4KCkNhbiB3 ZSBzb21laG93IGF2b2lkIHRoZSBuZWVkIHRvIGNhbGwgZmVuY2Vfc2lnbmFsKCkgYXQgYWxsPyBU aGUgCmludGVycnVwdHMgYXQgbGVhc3Qgb24gcmFkZW9uIGFyZSB3YXkgdG8gdW5yZWxpYWJsZSBm b3Igc3VjaCBhIHRoaW5nLiAKQ2FuIGVuYWJsZV9zaWduYWxsaW5nIGZhaWw/IFdoYXQncyB0aGUg cmVhc29uIGZvciBmZW5jZV9zaWduYWxlZCgpIGluIAp0aGUgZmlyc3QgcGxhY2U/Cgo+Pj4gQWdy ZWVkIHRoYXQgYW55IHNoYXJlZCBsb2NrcyBhcmUgb3V0IG9mIHRoZSB3YXkgKGVzcGVjaWFsbHkg c3R1ZmYgbGlrZQo+Pj4gZGV2LT5zdHJ1Y3RfbXV0ZXggb3Igb3RoZXIgbm9uLXN0cmljdGx5IGRy aXZlci1wcml2YXRlIHN0dWZmLCBpOTE1IGlzCj4+PiByZWFsbHkgYmFkIGhlcmUgc3RpbGwpLgo+ PiBZZWFoIHRoYXQncyBhbHNvIGFuIHBvaW50IEkndmUgd2FudGVkIHRvIG5vdGUgb24gTWFhcnRl bnMgcGF0Y2guIFJhZGVvbiBncmFicyB0aGUgcmVhZCBzaWRlIG9mIGl0J3MgZXhjbHVzaXZlIHNl bWFwaG9yZSB3aGlsZSB3YWl0aW5nIGZvciBmZW5jZXMgKGJlY2F1c2UgaXQgYXNzdW1lcyB0aGF0 IHRoZSBmZW5jZSBpdCB3YWl0cyBmb3IgaXMgYSBSYWRlb24gZmVuY2UpLgo+Pgo+PiBBc3N1bWlu ZyB0aGF0IHdlIG5lZWQgdG8gd2FpdCBpbiBib3RoIGRpcmVjdGlvbnMgd2l0aCBQcmltZSAoZS5n LiBJbnRlbCBkcml2ZXIgbmVlZHMgdG8gd2FpdCBmb3IgUmFkZW9uIHRvIGZpbmlzaCByZW5kZXJp bmcgYW5kIFJhZGVvbiBuZWVkcyB0byB3YWl0IGZvciBJbnRlbCB0byBmaW5pc2ggZGlzcGxheWlu ZyksIHRoaXMgbWlnaHQgYmVjb21lIGEgcGVyZmVjdCBleGFtcGxlIG9mIGxvY2tpbmcgaW52ZXJz aW9uLgo+IEluIHRoZSBwcmVsaW1pbmFyeSBwYXRjaGVzIHdoZXJlIEkgY2FuIHN5bmMgcmFkZW9u IHdpdGggb3RoZXIgR1BVJ3MgSSd2ZSBiZWVuIHZlcnkgY2FyZWZ1bCBpbiBhbGwgdGhlIHBsYWNl cyB0aGF0IGNhbGwgaW50byBmZW5jZXMsIHRvIG1ha2Ugc3VyZSB0aGF0IHJhZGVvbiB3b3VsZG4n dCB0cnkgdG8gaGFuZGxlIGxvY2t1cHMgZm9yIGEgZGlmZmVyZW50IChwb3NzaWJseSBhbHNvIHJh ZGVvbikgY2FyZC4KClRoYXQncyBhY3R1YWxseSBub3Qgc3VjaCBhIGdvb2QgaWRlYS4KCkluIGNh c2Ugb2YgYSBsb2NrdXAgd2UgbmVlZCB0byBoYW5kbGUgdGhlIGxvY2t1cCBjYXVzZSBvdGhlcndp c2UgaXQgCmNvdWxkIGhhcHBlbiB0aGF0IHJhZGVvbiB3YWl0cyBmb3IgdGhlIGxvY2t1cCB0byBi ZSByZXNvbHZlZCBhbmQgdGhlIApsb2NrdXAgaGFuZGxpbmcgbmVlZHMgdG8gd2FpdCBmb3IgYSBm ZW5jZSB0aGF0J3MgbmV2ZXIgc2lnbmFsZWQgYmVjYXVzZSAKb2YgdGhlIGxvY2t1cC4KCkNocmlz dGlhbi4KCj4KPiBUaGlzIGlzIGFsc28gd2h5IGZlbmNlX2lzX3NpZ25hbGVkIHNob3VsZCBuZXZl ciBibG9jaywgYW5kIHdoeSBpdCB0cnlsb2NrcyB0aGUgZXhjbHVzaXZlX2xvY2suIDotKSBJIHRo aW5rIGxvY2tkZXAgd291bGQgY29tcGxhaW4gaWYgSSBncmFiYmVkIGV4Y2x1c2l2ZV9sb2NrIHdo aWxlIGJsb2NraW5nIGluIGlzX3NpZ25hbGVkLgo+Cj4+PiBTbyBmcm9tIHRoZSBjb3JlIGZlbmNl IGZyYW1ld29yayBJIHRoaW5rIHdlIGFscmVhZHkgaGF2ZSBleGFjdGx5IHRoaXMsCj4+PiBhbmQg d2Ugb25seSBuZWVkIHRvIGFkanVzdCB0aGUgcmFkZW9uIGltcGxlbWVudGF0aW9uIGEgYml0IHRv IG1ha2UgaXQKPj4+IGxlc3Mgcmlza3kgYW5kIGludmFzaXZlIHRvIHRoZSByYWRlb24gZHJpdmVy IGxvZ2ljLgo+PiBBZ3JlZS4gV2VsbCB0aGUgYmlnZ2VzdCBwcm9ibGVtIEkgc2VlIGlzIHRoYXQg ZXhjbHVzaXZlIHNlbWFwaG9yZSBJIG5lZWQgdG8gdGFrZSB3aGVuIGFueXRoaW5nIGNhbGxzIGlu dG8gdGhlIGRyaXZlci4gRm9yIHRoZSBmZW5jZSBjb2RlIEkgbmVlZCB0byBtb3ZlIHRoYXQgZG93 biBpbnRvIHRoZSBmZW5jZS0+c2lnbmFsZWQgaGFuZGxlciwgY2F1c2UgdGhhdCBub3cgY2FuIGJl IGNhbGxlZCBmcm9tIG91dHNpZGUgdGhlIGRyaXZlci4KPj4KPj4gTWFhcnRlbiBzb2x2ZWQgdGhp cyBieSB0ZWxsaW5nIHRoZSBkcml2ZXIgaW4gdGhlIGxvY2t1cCBoYW5kbGVyICh3aGVyZSB3ZSBn cmFiIHRoZSB3cml0ZSBzaWRlIG9mIHRoZSBleGNsdXNpdmUgbG9jaykgdGhhdCBhbGwgaW50ZXJy dXB0cyBhcmUgYWxyZWFkeSBlbmFibGVkLCBzbyB0aGF0IGZlbmNlLT5zaWduYWxlZCBob3BlZnVs bHkgd291bGRuJ3QgbWVzcyB3aXRoIHRoZSBoYXJkd2FyZSBhdCBhbGwuIFdoaWxlIHRoaXMgcHJv YmFibHkgd29ya3MsIGl0IGp1c3QgbGVhdmVzIG1lIHdpdGggYSBmZWVsaW5nIHRoYXQgd2UgYXJl IGRvaW5nIHNvbWV0aGluZyB3cm9uZyBoZXJlLgo+IFRoZXJlIGlzIHVuZm9ydHVuYXRlbHkgbm8g Z2xvYmFsIG1lY2hhbmlzbSB0byBzYXkgJ3RoaXMgY2FyZCBpcyBsb2NrZWQgdXAsIHBsZWFzZSBk b24ndCBjYWxsIGludG8gYW55IG9mIG15IGZlbmNlcycsIGFuZCBJIGRvbid0IGFzc29jaWF0ZSBm ZW5jZXMgd2l0aCBkZXZpY2VzLCBhbmQgcmFkZW9uIGRvZXNuJ3Qga2VlcCBhIGdsb2JhbCBsaXN0 IG9mIGZlbmNlcy4KPiBJZiBhbGwgb2YgdGhhdCBleGlzdGVkLCBpdCB3b3VsZCBjb21wbGljYXRl IHRoZSBpbnRlcmZhY2UgYW5kIGl0cyBjYWxsZXJzIGEgbG90LCB3aGlsZSBJIGxpa2UgdG8ga2Vl cCB0aGluZ3Mgc2ltcGxlLgo+IFNvIEkgZGlkIHRoZSBiZXN0IEkgY291bGQsIGFuZCBzaW1wbHkg cHJldmVudGVkIHRoZSBmZW5jZSBjYWxscyBmcm9tIGZpZGRsaW5nIHdpdGggdGhlIGhhcmR3YXJl LiBGb3J0dW5hdGVseSBncHUgbG9ja3VwIGlzIG5vdCBhIGNvbW1vbiBvcGVyYXRpb24uIDotKQo+ Cj4gfk1hYXJ0ZW4KPgo+CgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fXwpOb3V2ZWF1IG1haWxpbmcgbGlzdApOb3V2ZWF1QGxpc3RzLmZyZWVkZXNrdG9wLm9y ZwpodHRwOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vbm91dmVhdQo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756637AbaGWGw2 (ORCPT ); Wed, 23 Jul 2014 02:52:28 -0400 Received: from mail-bn1blp0187.outbound.protection.outlook.com ([207.46.163.187]:12680 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753477AbaGWGw0 convert rfc822-to-8bit (ORCPT ); Wed, 23 Jul 2014 02:52:26 -0400 X-WSS-ID: 0N95KF7-08-VFO-02 X-M-MSG: Message-ID: <53CF5B9F.1050800@amd.com> Date: Wed, 23 Jul 2014 08:52:15 +0200 From: =?UTF-8?B?Q2hyaXN0aWFuIEvDtm5pZw==?= User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Maarten Lankhorst , =?UTF-8?B?Q2hyaXM=?= =?UTF-8?B?dGlhbiBLw7ZuaWc=?= , Daniel Vetter CC: Dave Airlie , Thomas Hellstrom , nouveau , LKML , dri-devel , Ben Skeggs , "Deucher, Alexander" Subject: Re: [Nouveau] [PATCH 09/17] drm/radeon: use common fence implementation for fences References: <20140709093124.11354.3774.stgit@patser> <20140709122953.11354.46381.stgit@patser> <53CE2421.5040906@amd.com> <20140722114607.GL15237@phenom.ffwll.local> <20140722115737.GN15237@phenom.ffwll.local> <53CE56ED.4040109@vodafone.de> <20140722132652.GO15237@phenom.ffwll.local> <53CE6AFA.1060807@vodafone.de> <53CE84AA.9030703@amd.com> <53CE8A57.2000803@vodafone.de> <53CF58FB.8070609@canonical.com> In-Reply-To: <53CF58FB.8070609@canonical.com> Content-Type: text/plain; charset="UTF-8"; format=flowed X-Originating-IP: [10.224.155.198] Content-Transfer-Encoding: 8BIT X-EOPAttributedMessage: 0 X-Forefront-Antispam-Report: CIP:165.204.84.222;CTRY:US;IPV:NLI;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(6009001)(428002)(199002)(189002)(377454003)(377424004)(51704005)(24454002)(36756003)(65816999)(86362001)(87266999)(83322001)(4396001)(76176999)(77982001)(106466001)(87936001)(85306003)(76482001)(93886003)(50466002)(80316001)(99396002)(84676001)(81542001)(44976005)(21056001)(46102001)(19580405001)(101416001)(23676002)(59896001)(50986999)(74662001)(83506001)(85202003)(54356999)(79102001)(83072002)(95666004)(97736001)(107046002)(33656002)(68736004)(64126003)(47776003)(85182001)(65806001)(92726001)(19580395003)(20776003)(81342001)(85852003)(80022001)(65956001)(105586002)(102836001)(64706001);DIR:OUT;SFP:;SCL:1;SRVR:BN1PR02MB037;H:atltwp02.amd.com;FPR:;MLV:sfv;PTR:InfoDomainNonexistent;MX:1;LANG:en; X-Microsoft-Antispam: BCL:0;PCL:0;RULEID: X-Forefront-PRVS: 028166BF91 Authentication-Results: spf=none (sender IP is 165.204.84.222) smtp.mailfrom=Christian.Koenig@amd.com; X-OriginatorOrg: amd4.onmicrosoft.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Am 23.07.2014 08:40, schrieb Maarten Lankhorst: > op 22-07-14 17:59, Christian König schreef: >> Am 22.07.2014 17:42, schrieb Daniel Vetter: >>> On Tue, Jul 22, 2014 at 5:35 PM, Christian König >>> wrote: >>>> Drivers exporting fences need to provide a fence->signaled and a fence->wait >>>> function, everything else like fence->enable_signaling or calling >>>> fence_signaled() from the driver is optional. >>>> >>>> Drivers wanting to use exported fences don't call fence->signaled or >>>> fence->wait in atomic or interrupt context, and not with holding any global >>>> locking primitives (like mmap_sem etc...). Holding locking primitives local >>>> to the driver is ok, as long as they don't conflict with anything possible >>>> used by their own fence implementation. >>> Well that's almost what we have right now with the exception that >>> drivers are allowed (actually must for correctness when updating >>> fences) the ww_mutexes for dma-bufs (or other buffer objects). >> In this case sorry for so much noise. I really haven't looked in so much detail into anything but Maarten's Radeon patches. >> >> But how does that then work right now? My impression was that it's mandatory for drivers to call fence_signaled()? > It's only mandatory to call fence_signal() if the .enable_signaling callback has been called, else you can get away with never calling signaling a fence at all before dropping the last refcount to it. > This allows you to keep interrupts disabled when you don't need them. Can we somehow avoid the need to call fence_signal() at all? The interrupts at least on radeon are way to unreliable for such a thing. Can enable_signalling fail? What's the reason for fence_signaled() in the first place? >>> Agreed that any shared locks are out of the way (especially stuff like >>> dev->struct_mutex or other non-strictly driver-private stuff, i915 is >>> really bad here still). >> Yeah that's also an point I've wanted to note on Maartens patch. Radeon grabs the read side of it's exclusive semaphore while waiting for fences (because it assumes that the fence it waits for is a Radeon fence). >> >> Assuming that we need to wait in both directions with Prime (e.g. Intel driver needs to wait for Radeon to finish rendering and Radeon needs to wait for Intel to finish displaying), this might become a perfect example of locking inversion. > In the preliminary patches where I can sync radeon with other GPU's I've been very careful in all the places that call into fences, to make sure that radeon wouldn't try to handle lockups for a different (possibly also radeon) card. That's actually not such a good idea. In case of a lockup we need to handle the lockup cause otherwise it could happen that radeon waits for the lockup to be resolved and the lockup handling needs to wait for a fence that's never signaled because of the lockup. Christian. > > This is also why fence_is_signaled should never block, and why it trylocks the exclusive_lock. :-) I think lockdep would complain if I grabbed exclusive_lock while blocking in is_signaled. > >>> So from the core fence framework I think we already have exactly this, >>> and we only need to adjust the radeon implementation a bit to make it >>> less risky and invasive to the radeon driver logic. >> Agree. Well the biggest problem I see is that exclusive semaphore I need to take when anything calls into the driver. For the fence code I need to move that down into the fence->signaled handler, cause that now can be called from outside the driver. >> >> Maarten solved this by telling the driver in the lockup handler (where we grab the write side of the exclusive lock) that all interrupts are already enabled, so that fence->signaled hopefully wouldn't mess with the hardware at all. While this probably works, it just leaves me with a feeling that we are doing something wrong here. > There is unfortunately no global mechanism to say 'this card is locked up, please don't call into any of my fences', and I don't associate fences with devices, and radeon doesn't keep a global list of fences. > If all of that existed, it would complicate the interface and its callers a lot, while I like to keep things simple. > So I did the best I could, and simply prevented the fence calls from fiddling with the hardware. Fortunately gpu lockup is not a common operation. :-) > > ~Maarten > >