From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from metis.ext.pengutronix.de ([85.220.165.71]:43917 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751402AbeEPK0M (ORCPT ); Wed, 16 May 2018 06:26:12 -0400 Message-ID: <1526466365.3494.26.camel@pengutronix.de> Subject: Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error From: Lucas Stach To: Daniel Vetter , Chris Wilson Cc: dri-devel@lists.freedesktop.org, kernel@collabora.com, Ezequiel Garcia , linux-media@vger.kernel.org Date: Wed, 16 May 2018 12:26:05 +0200 In-Reply-To: <20180516094224.GD3438@phenom.ffwll.local> References: <20180509201449.27452-1-ezequiel@collabora.com> <152602366168.22269.11696001916463464983@mail.alporthouse.com> <20180514164823.GH28661@phenom.ffwll.local> <152638659036.18532.13662508480413451560@mail.alporthouse.com> <20180516094224.GD3438@phenom.ffwll.local> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-media-owner@vger.kernel.org List-ID: Am Mittwoch, den 16.05.2018, 11:42 +0200 schrieb Daniel Vetter: > On Tue, May 15, 2018 at 01:16:30PM +0100, Chris Wilson wrote: > > Quoting Ezequiel Garcia (2018-05-14 22:28:31) > > > On Mon, 2018-05-14 at 18:48 +0200, Daniel Vetter wrote: > > > > On Fri, May 11, 2018 at 08:27:41AM +0100, Chris Wilson wrote: > > > > > Quoting Ezequiel Garcia (2018-05-09 21:14:49) > > > > > > Change how dma_fence_add_callback() behaves, when the fence > > > > > > has error-signaled by the time it is being add. After this commit, > > > > > > dma_fence_add_callback() returns the fence error, if it > > > > > > has error-signaled before dma_fence_add_callback() is called. > > > > > > > > > > Why? What problem are you trying to solve? fence->error does not imply > > > > > that the fence has yet been signaled, and the caller wants a callback > > > > > when it is signaled. > > > > > > > > On top this is incosistent, e.g. we don't do the same for any of the other > > > > dma_fence interfaces. Plus there's the issue that you might alias errno > > > > values with fence errno values. > > > > > > > > > > Right. > > > > > > > I think keeping the error codes from the functions you're calling distinct > > > > from the error code of the fence itself makes a lot of sense. The first > > > > tells you whether your request worked out (or why not), the second tells > > > > you whether the asynchronous dma operation (gpu rendering, page flip, > > > > whatever) that the dma_fence represents worked out (or why not). That's 2 > > > > distinct things imo. > > > > > > > > Might be good to show us the driver code that needs this behaviour so we > > > > can discuss how to best handle your use-case. > > > > > > > > > > This change arose while discussing the in-fences support for video4linux. > > > Here's the patch that calls dma_fence_add_callback https://lkml.org/lkml/2018/5/4/766. > > > > > > The code snippet currently looks something like this: > > > > > >         if (vb->in_fence) { > > >                 ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb, > > >                                  > > >              vb2_qbuf_fence_cb); > > >                 /* is the fence signaled? */ > > >                 if (ret == -ENOENT) { > > >          > > >                 dma_fence_put(vb->in_fence); > > >                         vb->in_fence = NULL; > > >                 } else if (ret) > > > { > > >                         goto unlock; > > >                 } > > >         } > > > > > > In this use case, if the callback is added successfully, > > > the video4linux core defers the activation of the buffer > > > until the fence signals. > > > > > > If the fence is signaled (currently disregarding of errors) > > > then the buffer is assumed to be ready to be activated, > > > and so it gets queued for hardware usage. > > > > > > Giving some more thought to this, I'm not so sure what is > > > the right action if a fence signaled with error. In this case, > > > it appears to me that we shouldn't be using this buffer > > > if its in-fence is in error, but perhaps I'm missing > > > something. > > > > What I have in mind for async errors is to skip the operation and > > propagate the error onto the next fence. Mostly because those async > > errors may include fatal errors such as unable to pin the backing > > storage for the operation, but even "trivial" errors such as an early > > operation failing means that this request is then subject to garbage-in, > > garbage-out. However, for trivial errors I would just propagate the > > error status (so the caller knows something went wrong if they care, but > > in all likelihood no one will notice) and continue on with the glitchy > > operation. > > In general, there's not really any hard rule about propagating fence > errors across devices. It's mostly just used by drivers internally to keep > track of failed stuff (gpu hangs or anything else async like Chris > describes here). > > For v4l I'm not sure you want to care much about this, since right now the > main use of fence errors is gpu hang recovery (whether it's the driver or > hw that's hung doesn't matter here). Yes, my understanding is that fence signal and errors are two distinct things that should not be conflated like it is done in this patch. In my understanding signaling a fence means the HW or SW component which added the fence is done with the buffer and will not touch it anymore. In case of an unrecoverable error the fence will be signaled with error status set, so we correctly reflect the buffer status as being free to be used by whoever is waiting for it, but may contain garbage. If a fence waiter cares about the buffer content and may wish to skip its operation if the fence is signaled with an error it should do it by explicitly checking the fence error status, instead of making this implicit behavior. Regards, Lucas From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lucas Stach Subject: Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error Date: Wed, 16 May 2018 12:26:05 +0200 Message-ID: <1526466365.3494.26.camel@pengutronix.de> References: <20180509201449.27452-1-ezequiel@collabora.com> <152602366168.22269.11696001916463464983@mail.alporthouse.com> <20180514164823.GH28661@phenom.ffwll.local> <152638659036.18532.13662508480413451560@mail.alporthouse.com> <20180516094224.GD3438@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [IPv6:2001:67c:670:201:290:27ff:fe1d:cc33]) by gabe.freedesktop.org (Postfix) with ESMTPS id 413916E3B3 for ; Wed, 16 May 2018 10:26:10 +0000 (UTC) In-Reply-To: <20180516094224.GD3438@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter , Chris Wilson Cc: kernel@collabora.com, Ezequiel Garcia , dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org QW0gTWl0dHdvY2gsIGRlbiAxNi4wNS4yMDE4LCAxMTo0MiArMDIwMCBzY2hyaWViIERhbmllbCBW ZXR0ZXI6Cj4gT24gVHVlLCBNYXkgMTUsIDIwMTggYXQgMDE6MTY6MzBQTSArMDEwMCwgQ2hyaXMg V2lsc29uIHdyb3RlOgo+ID4gUXVvdGluZyBFemVxdWllbCBHYXJjaWEgKDIwMTgtMDUtMTQgMjI6 Mjg6MzEpCj4gPiA+IE9uIE1vbiwgMjAxOC0wNS0xNCBhdCAxODo0OCArMDIwMCwgRGFuaWVsIFZl dHRlciB3cm90ZToKPiA+ID4gPiBPbiBGcmksIE1heSAxMSwgMjAxOCBhdCAwODoyNzo0MUFNICsw MTAwLCBDaHJpcyBXaWxzb24gd3JvdGU6Cj4gPiA+ID4gPiBRdW90aW5nIEV6ZXF1aWVsIEdhcmNp YSAoMjAxOC0wNS0wOSAyMToxNDo0OSkKPiA+ID4gPiA+ID4gQ2hhbmdlIGhvdyBkbWFfZmVuY2Vf YWRkX2NhbGxiYWNrKCkgYmVoYXZlcywgd2hlbiB0aGUgZmVuY2UKPiA+ID4gPiA+ID4gaGFzIGVy cm9yLXNpZ25hbGVkIGJ5IHRoZSB0aW1lIGl0IGlzIGJlaW5nIGFkZC4gQWZ0ZXIgdGhpcyBjb21t aXQsCj4gPiA+ID4gPiA+IGRtYV9mZW5jZV9hZGRfY2FsbGJhY2soKSByZXR1cm5zIHRoZSBmZW5j ZSBlcnJvciwgaWYgaXQKPiA+ID4gPiA+ID4gaGFzIGVycm9yLXNpZ25hbGVkIGJlZm9yZSBkbWFf ZmVuY2VfYWRkX2NhbGxiYWNrKCkgaXMgY2FsbGVkLgo+ID4gPiA+ID4gCj4gPiA+ID4gPiBXaHk/ IFdoYXQgcHJvYmxlbSBhcmUgeW91IHRyeWluZyB0byBzb2x2ZT8gZmVuY2UtPmVycm9yIGRvZXMg bm90IGltcGx5Cj4gPiA+ID4gPiB0aGF0IHRoZSBmZW5jZSBoYXMgeWV0IGJlZW4gc2lnbmFsZWQs IGFuZCB0aGUgY2FsbGVyIHdhbnRzIGEgY2FsbGJhY2sKPiA+ID4gPiA+IHdoZW4gaXQgaXMgc2ln bmFsZWQuCj4gPiA+ID4gCj4gPiA+ID4gT24gdG9wIHRoaXMgaXMgaW5jb3Npc3RlbnQsIGUuZy4g d2UgZG9uJ3QgZG8gdGhlIHNhbWUgZm9yIGFueSBvZiB0aGUgb3RoZXIKPiA+ID4gPiBkbWFfZmVu Y2UgaW50ZXJmYWNlcy4gUGx1cyB0aGVyZSdzIHRoZSBpc3N1ZSB0aGF0IHlvdSBtaWdodCBhbGlh cyBlcnJubwo+ID4gPiA+IHZhbHVlcyB3aXRoIGZlbmNlIGVycm5vIHZhbHVlcy4KPiA+ID4gPiAK PiA+ID4gCj4gPiA+IFJpZ2h0Lgo+ID4gPiAKPiA+ID4gPiBJIHRoaW5rIGtlZXBpbmcgdGhlIGVy cm9yIGNvZGVzIGZyb20gdGhlIGZ1bmN0aW9ucyB5b3UncmUgY2FsbGluZyBkaXN0aW5jdAo+ID4g PiA+IGZyb20gdGhlIGVycm9yIGNvZGUgb2YgdGhlIGZlbmNlIGl0c2VsZiBtYWtlcyBhIGxvdCBv ZiBzZW5zZS4gVGhlIGZpcnN0Cj4gPiA+ID4gdGVsbHMgeW91IHdoZXRoZXIgeW91ciByZXF1ZXN0 IHdvcmtlZCBvdXQgKG9yIHdoeSBub3QpLCB0aGUgc2Vjb25kIHRlbGxzCj4gPiA+ID4geW91IHdo ZXRoZXIgdGhlIGFzeW5jaHJvbm91cyBkbWEgb3BlcmF0aW9uIChncHUgcmVuZGVyaW5nLCBwYWdl IGZsaXAsCj4gPiA+ID4gd2hhdGV2ZXIpIHRoYXQgdGhlIGRtYV9mZW5jZSByZXByZXNlbnRzIHdv cmtlZCBvdXQgKG9yIHdoeSBub3QpLiBUaGF0J3MgMgo+ID4gPiA+IGRpc3RpbmN0IHRoaW5ncyBp bW8uCj4gPiA+ID4gCj4gPiA+ID4gTWlnaHQgYmUgZ29vZCB0byBzaG93IHVzIHRoZSBkcml2ZXIg Y29kZSB0aGF0IG5lZWRzIHRoaXMgYmVoYXZpb3VyIHNvIHdlCj4gPiA+ID4gY2FuIGRpc2N1c3Mg aG93IHRvIGJlc3QgaGFuZGxlIHlvdXIgdXNlLWNhc2UuCj4gPiA+ID4gCj4gPiA+IAo+ID4gPiBU aGlzIGNoYW5nZSBhcm9zZSB3aGlsZSBkaXNjdXNzaW5nIHRoZSBpbi1mZW5jZXMgc3VwcG9ydCBm b3IgdmlkZW80bGludXguCj4gPiA+IEhlcmUncyB0aGUgcGF0Y2ggdGhhdCBjYWxscyBkbWFfZmVu Y2VfYWRkX2NhbGxiYWNrIGh0dHBzOi8vbGttbC5vcmcvbGttbC8yMDE4LzUvNC83NjYuCj4gPiA+ IAo+ID4gPiBUaGUgY29kZSBzbmlwcGV0IGN1cnJlbnRseSBsb29rcyBzb21ldGhpbmcgbGlrZSB0 aGlzOgo+ID4gPiAKPiA+ID4gwqDCoMKgwqDCoMKgwqDCoGlmICh2Yi0+aW5fZmVuY2UpIHsKPiA+ ID4gwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqByZXQgPSBkbWFfZmVuY2VfYWRkX2Nh bGxiYWNrKHZiLT5pbl9mZW5jZSwgJnZiLT5mZW5jZV9jYiwKPiA+ID4gwqDCoMKgwqDCoMKgwqDC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoAo+ID4gPiDC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoHZiMl9xYnVmX2ZlbmNlX2NiKTsKPiA+ID4gwqDCoMKg wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAvKiBpcyB0aGUgZmVuY2Ugc2lnbmFsZWQ/ICovCj4g PiA+IMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgaWYgKHJldCA9PSAtRU5PRU5UKSB7 Cj4gPiA+IMKgwqDCoMKgwqDCoMKgwqAKPiA+ID4gwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC oMKgwqBkbWFfZmVuY2VfcHV0KHZiLT5pbl9mZW5jZSk7Cj4gPiA+IMKgwqDCoMKgwqDCoMKgwqDC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoHZiLT5pbl9mZW5jZSA9IE5VTEw7Cj4gPiA+ IMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgfSBlbHNlIGlmIChyZXQpCj4gPiA+IHsK PiA+ID4gwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgZ290 byB1bmxvY2s7Cj4gPiA+IMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgfQo+ID4gPiDC oMKgwqDCoMKgwqDCoMKgfQo+ID4gPiAKPiA+ID4gSW4gdGhpcyB1c2UgY2FzZSwgaWYgdGhlIGNh bGxiYWNrIGlzIGFkZGVkIHN1Y2Nlc3NmdWxseSwKPiA+ID4gdGhlIHZpZGVvNGxpbnV4IGNvcmUg ZGVmZXJzIHRoZSBhY3RpdmF0aW9uIG9mIHRoZSBidWZmZXIKPiA+ID4gdW50aWwgdGhlIGZlbmNl IHNpZ25hbHMuCj4gPiA+IAo+ID4gPiBJZiB0aGUgZmVuY2UgaXMgc2lnbmFsZWQgKGN1cnJlbnRs eSBkaXNyZWdhcmRpbmcgb2YgZXJyb3JzKQo+ID4gPiB0aGVuIHRoZSBidWZmZXIgaXMgYXNzdW1l ZCB0byBiZSByZWFkeSB0byBiZSBhY3RpdmF0ZWQsCj4gPiA+IGFuZCBzbyBpdCBnZXRzIHF1ZXVl ZCBmb3IgaGFyZHdhcmUgdXNhZ2UuCj4gPiA+IAo+ID4gPiBHaXZpbmcgc29tZSBtb3JlIHRob3Vn aHQgdG8gdGhpcywgSSdtIG5vdCBzbyBzdXJlIHdoYXQgaXMKPiA+ID4gdGhlIHJpZ2h0IGFjdGlv biBpZiBhIGZlbmNlIHNpZ25hbGVkIHdpdGggZXJyb3IuIEluIHRoaXMgY2FzZSwKPiA+ID4gaXQg YXBwZWFycyB0byBtZSB0aGF0IHdlIHNob3VsZG4ndCBiZSB1c2luZyB0aGlzIGJ1ZmZlcgo+ID4g PiBpZiBpdHMgaW4tZmVuY2UgaXMgaW4gZXJyb3IsIGJ1dCBwZXJoYXBzIEknbSBtaXNzaW5nCj4g PiA+IHNvbWV0aGluZy4KPiA+IAo+ID4gV2hhdCBJIGhhdmUgaW4gbWluZCBmb3IgYXN5bmMgZXJy b3JzIGlzIHRvIHNraXAgdGhlIG9wZXJhdGlvbiBhbmQKPiA+IHByb3BhZ2F0ZSB0aGUgZXJyb3Ig b250byB0aGUgbmV4dCBmZW5jZS4gTW9zdGx5IGJlY2F1c2UgdGhvc2UgYXN5bmMKPiA+IGVycm9y cyBtYXkgaW5jbHVkZSBmYXRhbCBlcnJvcnMgc3VjaCBhcyB1bmFibGUgdG8gcGluIHRoZSBiYWNr aW5nCj4gPiBzdG9yYWdlIGZvciB0aGUgb3BlcmF0aW9uLCBidXQgZXZlbiAidHJpdmlhbCIgZXJy b3JzIHN1Y2ggYXMgYW4gZWFybHkKPiA+IG9wZXJhdGlvbiBmYWlsaW5nIG1lYW5zIHRoYXQgdGhp cyByZXF1ZXN0IGlzIHRoZW4gc3ViamVjdCB0byBnYXJiYWdlLWluLAo+ID4gZ2FyYmFnZS1vdXQu IEhvd2V2ZXIsIGZvciB0cml2aWFsIGVycm9ycyBJIHdvdWxkIGp1c3QgcHJvcGFnYXRlIHRoZQo+ ID4gZXJyb3Igc3RhdHVzIChzbyB0aGUgY2FsbGVyIGtub3dzIHNvbWV0aGluZyB3ZW50IHdyb25n IGlmIHRoZXkgY2FyZSwgYnV0Cj4gPiBpbiBhbGwgbGlrZWxpaG9vZCBubyBvbmUgd2lsbCBub3Rp Y2UpIGFuZCBjb250aW51ZSBvbiB3aXRoIHRoZSBnbGl0Y2h5Cj4gPiBvcGVyYXRpb24uCj4gCj4g SW4gZ2VuZXJhbCwgdGhlcmUncyBub3QgcmVhbGx5IGFueSBoYXJkIHJ1bGUgYWJvdXQgcHJvcGFn YXRpbmcgZmVuY2UKPiBlcnJvcnMgYWNyb3NzIGRldmljZXMuIEl0J3MgbW9zdGx5IGp1c3QgdXNl ZCBieSBkcml2ZXJzIGludGVybmFsbHkgdG8ga2VlcAo+IHRyYWNrIG9mIGZhaWxlZCBzdHVmZiAo Z3B1IGhhbmdzIG9yIGFueXRoaW5nIGVsc2UgYXN5bmMgbGlrZSBDaHJpcwo+IGRlc2NyaWJlcyBo ZXJlKS4KPiAKPiBGb3IgdjRsIEknbSBub3Qgc3VyZSB5b3Ugd2FudCB0byBjYXJlIG11Y2ggYWJv dXQgdGhpcywgc2luY2UgcmlnaHQgbm93IHRoZQo+IG1haW4gdXNlIG9mIGZlbmNlIGVycm9ycyBp cyBncHUgaGFuZyByZWNvdmVyeSAod2hldGhlciBpdCdzIHRoZSBkcml2ZXIgb3IKPiBodyB0aGF0 J3MgaHVuZyBkb2Vzbid0IG1hdHRlciBoZXJlKS4KClllcywgbXkgdW5kZXJzdGFuZGluZyBpcyB0 aGF0IGZlbmNlIHNpZ25hbCBhbmQgZXJyb3JzIGFyZSB0d28gZGlzdGluY3QKdGhpbmdzIHRoYXQg c2hvdWxkIG5vdCBiZSBjb25mbGF0ZWQgbGlrZSBpdCBpcyBkb25lIGluIHRoaXMgcGF0Y2guCgpJ biBteSB1bmRlcnN0YW5kaW5nIHNpZ25hbGluZyBhIGZlbmNlIG1lYW5zIHRoZSBIVyBvciBTVyBj b21wb25lbnQKd2hpY2ggYWRkZWQgdGhlIGZlbmNlIGlzIGRvbmUgd2l0aCB0aGUgYnVmZmVyIGFu ZCB3aWxsIG5vdCB0b3VjaCBpdAphbnltb3JlLiBJbiBjYXNlIG9mIGFuIHVucmVjb3ZlcmFibGUg ZXJyb3IgdGhlIGZlbmNlIHdpbGwgYmUgc2lnbmFsZWQKd2l0aCBlcnJvciBzdGF0dXMgc2V0LCBz byB3ZSBjb3JyZWN0bHkgcmVmbGVjdCB0aGUgYnVmZmVyIHN0YXR1cyBhcwpiZWluZyBmcmVlIHRv IGJlIHVzZWQgYnkgd2hvZXZlciBpcyB3YWl0aW5nIGZvciBpdCwgYnV0IG1heSBjb250YWluCmdh cmJhZ2UuCgpJZiBhIGZlbmNlIHdhaXRlciBjYXJlcyBhYm91dCB0aGUgYnVmZmVyIGNvbnRlbnQg YW5kIG1heSB3aXNoIHRvIHNraXAKaXRzIG9wZXJhdGlvbiBpZiB0aGUgZmVuY2UgaXMgc2lnbmFs ZWQgd2l0aCBhbiBlcnJvciBpdCBzaG91bGQgZG8gaXQgYnkKZXhwbGljaXRseSBjaGVja2luZyB0 aGUgZmVuY2UgZXJyb3Igc3RhdHVzLCBpbnN0ZWFkIG9mIG1ha2luZyB0aGlzCmltcGxpY2l0IGJl aGF2aW9yLgoKUmVnYXJkcywKTHVjYXMKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJl ZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGlu Zm8vZHJpLWRldmVsCg==