Re: [Linaro-mm-sig] [PATCH 2/4] dma-fence: dma-buf synchronization (v8 )

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Aug 11, 2012 at 10:14:40AM -0500, Rob Clark wrote:
> On Fri, Aug 10, 2012 at 3:29 PM, Daniel Vetter <daniel@ffwll.ch> 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