[Intel-gfx] [PATCH 2/2] drm/i915: Remove impossibe checks from i915_gem_request_add_to_client

[Chris Wilson]

On Thu, Mar 24, 2016 at 09:49:25AM +0000, Tvrtko Ursulin wrote:
> 
> On 23/03/16 15:31, Chris Wilson wrote:
> >On Wed, Mar 23, 2016 at 03:15:03PM +0000, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>
> >>There is only one caller of this functions which calls it under
> >>strictly defined conditions. Error checking and return value
> >>can be removed.
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>---
> >>BTW it looks like the whole point of this request list is to implement
> >>the throttle ioctl? Looks like a strange ioctl with a fixed 20ms criteria,
> >>could it be re-implemented somehow without having to have this list?
> >>
> >>Does it have to be per-client or could it wait for any requests on any
> >>engine emitted 20ms ago?
> >
> >It is per-client, and I long wished the 20ms wasn't defined by the ABI.
> 
> Is it useful and/or widely used though?

X for throttling front buffer rendering.
 
> With multiple clients submitting work I don't see the semantics are
> deterministic. I was thinking if it could be replaced with a simpler
> implementation for the similar random effect, losing the list
> altogether?

No, it wants per-client semantics.
 
> Especially wonder how the scheduler will affect it.

It won't because that would be breaking ABI.

> >What I proposed doing is this:
> >
> >https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=breadcrumbs&id=e33947505b54582964c8c202b22f88fc5705f484
> 
> How is it safe? list_add modifies various pointers in multiple steps
> so I didn't know that is safe against concurrent iteration.

It is safe. list_add() is now properly ordered using the compiler
barrier (i.e. it gained the RCU semantics).
 
> RCU flavoured list API can do such things but in this case switching
> to that would only enable lockless throttle and not gain anything on
> the add_to_client path.

And add_to_client is the hotter of the pair.
 
> >Please note that this also fixes a race condition I've seen in the wild.
> 
> What is the race?

file_priv not being checked after acquiring the spinlock under which the
list may be reset.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre