[Intel-gfx] [PATCH] drm/i915: Do an optimistic is-busy? check first

[Daniel Vetter]

On Thu, Sep 5, 2013 at 4:50 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>> > +   /* Do an optimistic check for activity - we don't care about userspace
>> > +    * racing with itself, that is always problematic.
>> > +    */
>> > +   ring = obj->ring;
>> > +   if (ring && obj->last_read_seqno == ring->outstanding_lazy_seqno)
>> > +           goto lock_and_flush;
>>
>> Feels a bit too tricky ... How useful is just an
>>
>>       if (ACCESS_ONCE(obj->active))
>>               goto lock_and_flush;
>
> Not good enough, still ends up fighting for the lock.

Pondering this some more the problem is that I don't think we can
ignore racing userspace since it can legitimately race here, e.g.
- dri2 client uses the busy check to figure out whether it can access
the frontbuffer for pixel readback
- X server does a pageflip/blt which results in a ring switch

If now the ring we switch to is ahead of the old ring with signalling
seqno completion and  the lockless busy reads the ring and seqno out
of order we'll report the buffer falsely as non-busy. And the above
scenario doesn't involve anything userspace shouldn't do.

So I think reading just obj->ring would be ok, but doing more outside
of proper locking won't work out. For that we need to bite the bullet
and install real per-object locking, or something else suitable. Also
I'm reluctant to merge clever code that would be superseeded by a
locking rework. The shrinker madness is an exception since we simply
need those quick fixes for correctness. Checking for obj->ring outside
of any locks would imo still be useful as a micro-optimization when we
have per-object locking.

The lockless waiting in set_domain is already a bit on the fence for
me, but this here feels like too much trickery just to paper over the
performance issues of our too simplistic locking scheme ...

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch