From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [Linaro-mm-sig] [PATCH 2/4] dma-fence: dma-buf synchronization (v8 ) Date: Sat, 11 Aug 2012 21:22:47 +0200 Message-ID: <20120811192247.GB5132@phenom.ffwll.local> References: <20120810145728.5490.44707.stgit@patser.local> <20120810145750.5490.5639.stgit@patser.local> <20120810202916.GI5738@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-we0-f177.google.com (mail-we0-f177.google.com [74.125.82.177]) by gabe.freedesktop.org (Postfix) with ESMTP id DB0B19F4E5 for ; Sat, 11 Aug 2012 12:22:27 -0700 (PDT) Received: by weyr3 with SMTP id r3so1820911wey.36 for ; Sat, 11 Aug 2012 12:22:26 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Rob Clark Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org, sumit.semwal@linaro.org, linux-media@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org On Sat, Aug 11, 2012 at 10:14:40AM -0500, Rob Clark wrote: > On Fri, Aug 10, 2012 at 3:29 PM, Daniel Vetter wrote: > > On Fri, Aug 10, 2012 at 04:57:52PM +0200, Maarten Lankhorst wrote: > >> + > >> + if (!ret) { > >> + cb->base.flags = 0; > >> + cb->base.func = __dma_fence_wake_func; > >> + cb->base.private = priv; > >> + cb->fence = fence; > >> + cb->func = func; > >> + __add_wait_queue(&fence->event_queue, &cb->base); > >> + } > >> + spin_unlock_irqrestore(&fence->event_queue.lock, flags); > >> + > >> + return ret; > >> +} > >> +EXPORT_SYMBOL_GPL(dma_fence_add_callback); > > > > I think for api completenes we should also have a > > dma_fence_remove_callback function. > > We did originally but Maarten found it was difficult to deal with > properly when the gpu's hang. I think his alternative was just to > require the hung driver to signal the fence. I had kicked around the > idea of a dma_fence_cancel() alternative to signal that could pass an > error thru to the waiting driver.. although not sure if the other > driver could really do anything differently at that point. Well, the idea is not to cancel all callbacks, but just a single one, in case a driver wants to somehow abort the wait. E.g. when the own gpu dies I guess we should clear all these fence callbacks so that they can't clobber the hw state after the reset. > >> + > >> +/** > >> + * dma_fence_wait - wait for a fence to be signaled > >> + * > >> + * @fence: [in] The fence to wait on > >> + * @intr: [in] if true, do an interruptible wait > >> + * @timeout: [in] absolute time for timeout, in jiffies. > > > > I don't quite like this, I think we should keep the styl of all other > > wait_*_timeout functions and pass the arg as timeout in jiffies (and also > > the same return semantics). Otherwise well have funny code that needs to > > handle return values differently depending upon whether it waits upon a > > dma_fence or a native object (where it would us the wait_*_timeout > > functions directly). > > We did start out this way, but there was an ugly jiffies roll-over > problem that was difficult to deal with properly. Using an absolute > time avoided the problem. Well, as-is the api works differently than all the other _timeout apis I've seen in the kernel, which makes it confusing. Also, I don't quite see what jiffies wraparound issue there is? > > Also, I think we should add the non-_timeout variants, too, just for > > completeness. This request here has the same reasons, essentially: If we offer a dma_fence wait api that matches the usual wait apis closely, it's harder to get their usage wrong. I know that i915 has some major cludge for a wait_seqno interface internally, but that's no reason to copy that approach ;-) Cheers, Daniel -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48