From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Christian_K=c3=b6nig?= Subject: Re: [PATCH 2/2] dma-buf/fence: add fence_array fences v4 Date: Mon, 23 May 2016 09:32:24 +0200 Message-ID: <5742B208.70103@vodafone.de> References: <1463752571-28688-1-git-send-email-deathsimple@vodafone.de> <1463752571-28688-2-git-send-email-deathsimple@vodafone.de> <20160520144203.GW3590@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: Received: from pegasos-out.vodafone.de (pegasos-out.vodafone.de [80.84.1.38]) by gabe.freedesktop.org (Postfix) with ESMTP id 141126E419 for ; Mon, 23 May 2016 07:32:33 +0000 (UTC) In-Reply-To: <20160520144203.GW3590@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Chris Wilson , dri-devel@lists.freedesktop.org, daniel.vetter@ffwll.ch, linux-kernel@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org QW0gMjAuMDUuMjAxNiB1bSAxNjo0MiBzY2hyaWViIENocmlzIFdpbHNvbjoKPiBPbiBGcmksIE1h eSAyMCwgMjAxNiBhdCAwMzo1NjoxMVBNICswMjAwLCBDaHJpc3RpYW4gS8O2bmlnIHdyb3RlOgo+ PiBGcm9tOiBHdXN0YXZvIFBhZG92YW4gPGd1c3Rhdm8ucGFkb3ZhbkBjb2xsYWJvcmEuY28udWs+ Cj4+Cj4+IHN0cnVjdCBmZW5jZV9jb2xsZWN0aW9uIGluaGVyaXRzIGZyb20gc3RydWN0IGZlbmNl IGFuZCBjYXJyaWVzIGEKPj4gY29sbGVjdGlvbiBvZiBmZW5jZXMgdGhhdCBuZWVkcyB0byBiZSB3 YWl0ZWQgdG9nZXRoZXIuCj4+Cj4+IEl0IGlzIHVzZWZ1bCB0byB0cmFuc2xhdGUgYSBzeW5jX2Zp bGUgdG8gYSBmZW5jZSB0byByZW1vdmUgdGhlIGNvbXBsZXhpdHkKPj4gb2YgZGVhbGluZyB3aXRo IHN5bmNfZmlsZXMgb24gRFJNIGRyaXZlcnMuIFNvIGV2ZW4gaWYgdGhlcmUgYXJlIG1hbnkKPj4g ZmVuY2VzIGluIHRoZSBzeW5jX2ZpbGUgdGhhdCBuZWVkcyB0byB3YWl0ZWQgZm9yIGEgY29tbWl0 IHRvIGhhcHBlbiwKPj4gdGhleSBhbGwgZ2V0IGFkZGVkIHRvIHRoZSBmZW5jZV9jb2xsZWN0aW9u IGFuZCBwYXNzZWQgZm9yIERSTSB1c2UgYXMKPj4gYSBzdGFuZGFyZCBzdHJ1Y3QgZmVuY2UuCj4+ Cj4+IFRoYXQgbWVhbnMgdGhhdCBubyBjaGFuZ2VzIG5lZWRlZCB0byBhbnkgZHJpdmVyIGJlc2lk ZXMgc3VwcG9ydGluZyBmZW5jZXMuCj4+Cj4+IGZlbmNlX2NvbGxlY3Rpb24ncyBmZW5jZSBkb2Vz bid0IGJlbG9uZyB0byBhbnkgdGltZWxpbmUgY29udGV4dCwgc28KPj4gZmVuY2VfaXNfbGF0ZXIo KSBhbmQgZmVuY2VfbGF0ZXIoKSBhcmUgbm90IG1lYW50IHRvIGJlIGNhbGxlZCB3aXRoCj4+IGZl bmNlX2NvbGxlY3Rpb25zIGZlbmNlcy4KPj4KPj4gdjI6IENvbW1lbnRzIGJ5IERhbmllbCBWZXR0 ZXI6Cj4+IAktIG1lcmdlIGZlbmNlX2NvbGxlY3Rpb25faW5pdCgpIGFuZCBmZW5jZV9jb2xsZWN0 aW9uX2FkZCgpCj4+IAktIG9ubHkgYWRkIGNhbGxiYWNrcyBhdCAtPmVuYWJsZV9zaWduYWxsaW5n KCkKPj4gCS0gcmVtb3ZlIGZlbmNlX2NvbGxlY3Rpb25fcHV0KCkKPj4gCS0gY2hlY2sgZm9yIHR5 cGUgb24gdG9fZmVuY2VfY29sbGVjdGlvbigpCj4+IAktIGFkanVzdCBmZW5jZV9pc19sYXRlcigp IGFuZCBmZW5jZV9sYXRlcigpIHRvIFdBUk5fT04oKSBpZiB0aGV5Cj4+IAlhcmUgdXNlZCB3aXRo IGNvbGxlY3Rpb24gZmVuY2VzLgo+Pgo+PiB2MzogLSBJbml0aWFsaXplIGZlbmNlX2NiLm5vZGUg YXQgZmVuY2UgaW5pdC4KPj4KPj4gICAgICBDb21tZW50cyBieSBDaHJpcyBXaWxzb246Cj4+IAkt IHJldHVybiAidW5ib3VuZCIgb24gZmVuY2VfY29sbGVjdGlvbl9nZXRfdGltZWxpbmVfbmFtZSgp Cj4+IAktIGRvbid0IHN0b3AgYWRkaW5nIGNhbGxiYWNrcyBpZiBvbmUgZmFpbHMKPj4gCS0gcmVt b3ZlIHJlZHVuZGFudCAhISBvbiBmZW5jZV9jb2xsZWN0aW9uX2VuYWJsZV9zaWduYWxpbmcoKQo+ PiAJLSByZW1vdmUgcmVkdW5kYW50ICgpIG9uIGZlbmNlX2NvbGxlY3Rpb25fc2lnbmFsZWQKPj4g CS0gdXNlIGZlbmNlX2RlZmF1bHRfd2FpdCgpIGluc3RlYWQKPj4KPj4gdjQgKGNoayk6IFJld29y aywgc2ltcGxpZmljYXRpb24gYW5kIGNsZWFudXA6Cj4+IAktIERyb3AgRkVOQ0VfTk9fQ09OVEVY VCBoYW5kbGluZywgYWx3YXlzIGFsbG9jYXRlIGEgY29udGV4dC4KPj4gCS0gUmVuYW1lIHRvIGZl bmNlX2FycmF5Lgo+PiAJLSBSZXR1cm4gZml4ZWQgZHJpdmVyIG5hbWUuCj4+IAktIFJlZ2lzdGVy IG9ubHkgb25lIGNhbGxiYWNrIGF0IGEgdGltZS4KPiBXaHk/IEV2ZW4gd2l0aGluIGEgZHJpdmVy IEkgZXhwZWN0ZWQgdGhlcmUgdG8gYmUgc29tZSBhbW9yaXRpemF0aW9uIG9mCj4gdGhlIHNpZ25h bGluZyBjb3N0cyBmb3IgaGFuZGxpbmcgbXVsdGlwbGUgZmVuY2VzIGF0IG9uY2UgKGF0IGxlYXN0 IHRoZQo+IGRyaXZlciBJJ20gZmFtaWxhciB3aXRoISkuCj4KPiBTbyBtb3JlIGp1c3QgY3VyaW91 c2l0eSBhcyB0byB5b3VyIGV4cGVyaWVuY2UgdGhhdCBmYXZvdXJzIHNlcXVlbnRpYWwKPiBlbmFi bGluZy4KCkp1c3QgdGhlIHByb2ZhbmUgcmVhc29uIHRoYXQgSSB3YW50IHRvIHNhdmUgdGhlIG1l bW9yeSBmb3IgYWxsIHRoZSAKY2FsbGJhY2tzLgoKQnV0IHRoaW5raW5nIGFib3V0IGl0IHlvdSBh cmUgcHJvYmFibHkgcmlnaHQgdGhhdCB3ZSBzaG91bGQgZW5hYmxlIHRoZSAKc2lnbmFsaW5nIGZv ciBhbGwgZmVuY2VzIGltbWVkaWF0ZWx5LiBHb2luZyB0byBmaXggdGhpcyBpbiB0aGUgbmV4dCAK dmVyc2lvbiBvZiB0aGUgcGF0Y2guCgo+Cj4+ICtzdGF0aWMgYm9vbCBmZW5jZV9hcnJheV9hZGRf bmV4dF9jYWxsYmFjayhzdHJ1Y3QgZmVuY2VfYXJyYXkgKmFycmF5KQo+PiArewo+PiArCXdoaWxl IChhcnJheS0+bnVtX3NpZ25hbGVkIDwgYXJyYXktPm51bV9mZW5jZXMpIHsKPj4gKwkJc3RydWN0 IGZlbmNlICpuZXh0ID0gYXJyYXktPmZlbmNlc1thcnJheS0+bnVtX3NpZ25hbGVkXTsKPj4gKwo+ PiArCQlpZiAoIWZlbmNlX2FkZF9jYWxsYmFjayhuZXh0LCAmYXJyYXktPmNiLCBmZW5jZV9hcnJh eV9jYl9mdW5jKSkKPj4gKwkJCXJldHVybiB0cnVlOwo+PiArCj4+ICsJCSsrYXJyYXktPm51bV9z aWduYWxlZDsKPj4gKwl9Cj4+ICsKPj4gKwlyZXR1cm4gZmFsc2U7Cj4+ICt9Cj4+ICsKPj4gK3N0 YXRpYyB2b2lkIGZlbmNlX2FycmF5X2NiX2Z1bmMoc3RydWN0IGZlbmNlICpmLCBzdHJ1Y3QgZmVu Y2VfY2IgKmNiKQo+PiArewo+PiArCXN0cnVjdCBmZW5jZV9hcnJheSAqYXJyYXkgPSBjb250YWlu ZXJfb2YoY2IsIHN0cnVjdCBmZW5jZV9hcnJheSwgY2IpOwo+IFNvbWUgY2hhc2luZyBhcm91bmQg d291bGQgaGF2ZSBiZWVuIHNhdmVkIGJ5IGEKPgo+IAlhc3NlcnRfc3Bpbl9sb2NrZWQoJmFycmF5 LT5sb2NrKTsKPgo+IGhlcmUuCgpNaG0sIGFjdHVhbGx5IHRoZSBhcnJheSBsb2NrIGlzbid0IGhl bGQgaGVyZS4gVGhpbmtpbmcgbW9yZSBhYm91dCBpdCAKYWRkaW5nIGEgbmV3IGNhbGxiYWNrIGZy b20gYSBmZW5jZSBjYWxsYmFjayBjYW4gYmFkbHkgZGVhZGxvY2sgdW5kZXIgCmNlcnRhaW4gc2l0 dWF0aW9ucy4KCkkgbmVlZCB0byBkb3VibGUgY2hlY2sgd2h5IHRoZSBjYWxsYmFjayBpcyBjYWxs ZWQgd2l0aCB0aGUgZmVuY2UgbG9jayAKaGVsZCBoZXJlLgoKPgo+PiArCj4+ICsJKythcnJheS0+ bnVtX3NpZ25hbGVkOwo+PiArCWlmICghZmVuY2VfYXJyYXlfYWRkX25leHRfY2FsbGJhY2soYXJy YXkpKQo+PiArCQlmZW5jZV9zaWduYWwoJmFycmF5LT5iYXNlKTsKPj4gK30KPj4gKwo+PiArc3Rh dGljIGJvb2wgZmVuY2VfYXJyYXlfZW5hYmxlX3NpZ25hbGluZyhzdHJ1Y3QgZmVuY2UgKmZlbmNl KQo+PiArewo+PiArCXN0cnVjdCBmZW5jZV9hcnJheSAqYXJyYXkgPSB0b19mZW5jZV9hcnJheShm ZW5jZSk7Cj4+ICsKPj4gKwlyZXR1cm4gZmVuY2VfYXJyYXlfYWRkX25leHRfY2FsbGJhY2soYXJy YXkpOwo+PiArfQo+PiArCj4+ICtzdGF0aWMgYm9vbCBmZW5jZV9hcnJheV9zaWduYWxlZChzdHJ1 Y3QgZmVuY2UgKmZlbmNlKQo+PiArewo+PiArCXN0cnVjdCBmZW5jZV9hcnJheSAqYXJyYXkgPSB0 b19mZW5jZV9hcnJheShmZW5jZSk7Cj4+ICsKPj4gKwlyZXR1cm4gQUNDRVNTX09OQ0UoYXJyYXkt Pm51bV9zaWduYWxlZCkgPT0gYXJyYXktPm51bV9mZW5jZXM7Cj4gQ2FuIGp1c3QgYmUgUkVBRF9P TkNFKCkKCkdvb2QgcG9pbnQsIGdvaW5nIHRvIGZpeCB0aGF0LgoKQ2hyaXN0aWFuLgoKPiAtQ2hy aXMKPgoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJp LWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBz Oi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932128AbcEWHch (ORCPT ); Mon, 23 May 2016 03:32:37 -0400 Received: from pegasos-out.vodafone.de ([80.84.1.38]:45350 "EHLO pegasos-out.vodafone.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752914AbcEWHcf (ORCPT ); Mon, 23 May 2016 03:32:35 -0400 X-Spam-Flag: NO X-Spam-Score: -0.045 Authentication-Results: rohrpostix1.prod.vfnet.de (amavisd-new); dkim=pass header.i=@vodafone.de X-DKIM: OpenDKIM Filter v2.6.8 pegasos-out.vodafone.de 1919126175F Subject: Re: [PATCH 2/2] dma-buf/fence: add fence_array fences v4 To: Chris Wilson , dri-devel@lists.freedesktop.org, daniel.vetter@ffwll.ch, linux-kernel@vger.kernel.org References: <1463752571-28688-1-git-send-email-deathsimple@vodafone.de> <1463752571-28688-2-git-send-email-deathsimple@vodafone.de> <20160520144203.GW3590@nuc-i3427.alporthouse.com> From: =?UTF-8?Q?Christian_K=c3=b6nig?= Message-ID: <5742B208.70103@vodafone.de> Date: Mon, 23 May 2016 09:32:24 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <20160520144203.GW3590@nuc-i3427.alporthouse.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Am 20.05.2016 um 16:42 schrieb Chris Wilson: > On Fri, May 20, 2016 at 03:56:11PM +0200, Christian König wrote: >> From: Gustavo Padovan >> >> struct fence_collection inherits from struct fence and carries a >> collection of fences that needs to be waited together. >> >> It is useful to translate a sync_file to a fence to remove the complexity >> of dealing with sync_files on DRM drivers. So even if there are many >> fences in the sync_file that needs to waited for a commit to happen, >> they all get added to the fence_collection and passed for DRM use as >> a standard struct fence. >> >> That means that no changes needed to any driver besides supporting fences. >> >> fence_collection's fence doesn't belong to any timeline context, so >> fence_is_later() and fence_later() are not meant to be called with >> fence_collections fences. >> >> v2: Comments by Daniel Vetter: >> - merge fence_collection_init() and fence_collection_add() >> - only add callbacks at ->enable_signalling() >> - remove fence_collection_put() >> - check for type on to_fence_collection() >> - adjust fence_is_later() and fence_later() to WARN_ON() if they >> are used with collection fences. >> >> v3: - Initialize fence_cb.node at fence init. >> >> Comments by Chris Wilson: >> - return "unbound" on fence_collection_get_timeline_name() >> - don't stop adding callbacks if one fails >> - remove redundant !! on fence_collection_enable_signaling() >> - remove redundant () on fence_collection_signaled >> - use fence_default_wait() instead >> >> v4 (chk): Rework, simplification and cleanup: >> - Drop FENCE_NO_CONTEXT handling, always allocate a context. >> - Rename to fence_array. >> - Return fixed driver name. >> - Register only one callback at a time. > Why? Even within a driver I expected there to be some amoritization of > the signaling costs for handling multiple fences at once (at least the > driver I'm familar with!). > > So more just curiousity as to your experience that favours sequential > enabling. Just the profane reason that I want to save the memory for all the callbacks. But thinking about it you are probably right that we should enable the signaling for all fences immediately. Going to fix this in the next version of the patch. > >> +static bool fence_array_add_next_callback(struct fence_array *array) >> +{ >> + while (array->num_signaled < array->num_fences) { >> + struct fence *next = array->fences[array->num_signaled]; >> + >> + if (!fence_add_callback(next, &array->cb, fence_array_cb_func)) >> + return true; >> + >> + ++array->num_signaled; >> + } >> + >> + return false; >> +} >> + >> +static void fence_array_cb_func(struct fence *f, struct fence_cb *cb) >> +{ >> + struct fence_array *array = container_of(cb, struct fence_array, cb); > Some chasing around would have been saved by a > > assert_spin_locked(&array->lock); > > here. Mhm, actually the array lock isn't held here. Thinking more about it adding a new callback from a fence callback can badly deadlock under certain situations. I need to double check why the callback is called with the fence lock held here. > >> + >> + ++array->num_signaled; >> + if (!fence_array_add_next_callback(array)) >> + fence_signal(&array->base); >> +} >> + >> +static bool fence_array_enable_signaling(struct fence *fence) >> +{ >> + struct fence_array *array = to_fence_array(fence); >> + >> + return fence_array_add_next_callback(array); >> +} >> + >> +static bool fence_array_signaled(struct fence *fence) >> +{ >> + struct fence_array *array = to_fence_array(fence); >> + >> + return ACCESS_ONCE(array->num_signaled) == array->num_fences; > Can just be READ_ONCE() Good point, going to fix that. Christian. > -Chris >