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@xxxxxxxx> 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@xxxxxxxx
Mobile: +41 (0)79 365 57 48
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
