Re: [RFC] drm/i915: Add sync framework support to execbuff IOCTL

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jul 06, 2015 at 05:34:22PM +0100, Tvrtko Ursulin wrote:
> 
> On 07/06/2015 04:37 PM, Daniel Vetter wrote:
> >On Mon, Jul 06, 2015 at 04:21:28PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 07/06/2015 04:12 PM, Daniel Vetter wrote:
> >>>On Mon, Jul 06, 2015 at 03:46:49PM +0100, Tvrtko Ursulin wrote:
> >>
> >>>If you have weak references somewhere and need to prevent the object from
> >>>disappearing untimely while chasing that weak reference then imo the
> >>>better design pattern is to use kref_get_unless_zero. If you need the
> >>>serialization the mutex provides for some other reason (someone is only
> >>>hodling the mutex instead of grabbing a proper refernce when they really
> >>>should grab one) then your refcounting scheme probably needs another kind
> >>>of fixup patch.
> >>
> >>I don't see how weak references can work since if the request goes
> >>information is lost, unless stored somewhere else.
> >
> >So weak refernce is a pointer which doesn't hold a reference to the object
> >controlled with kref, but protected by some lock. Latest when the object
> >is destroyed we need to clear that pointer in the free callback of
> >kref_put (and so also grab the lock that protects that pointer). The
> >problem is that anyone else chasing that weak reference might race with
> >the final kref_put and increase the refcount from 0 to 1, which isn't
> >good.
> >
> >There's two ways to do that:
> >- kref_put_mutex on the release side.
> >- in the acquire side do a kref_get_unless_zero _while_ holding that lock.
> >
> >Two upsides of the later approach:
> >- You can have an unrestricted amount of weak references, each protected
> >   with their own lock. kref_put_mutex only works up to one.
> >- It doesn't serialize the final kref_put with the lock - doing that
> >   allows folks to rely on the mutex instead of a proper refcount to make
> >   the obj stick around, which is imo a design antipattern that I suffered
> >   through trying to cleaning it up agian a lot.
> >
> >But that's really a tangent to the discussion here, I have no idea whether
> >this applies here since I didn't read your patch in detail.
> 
> I think it is not about my patch but whether the Android native sync
> implementation handles losing the weak reference on a fence.
> 
> I don't think it does and I am not sure how easy or hard would it be to
> change it right now. (The whole area traditionally confuses me since there
> are too many things called sync something and fence something.)
> 
> I would need to handle it to be able at least report the status of a fence
> to userspace and gracefully fail other operations. And that is assuming that
> wouldn't break existing userspace.

For reporting status it should grab a strong reference. I thought this was
about i915-internal lists and stuff we use to keep track of our own
requests, for which we'd need struct_mutex. struct_mutex won't protect any
of the syncpt internal state. I guess I'm rather confused now what this is
about ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx