[Intel-gfx] [PATCH 11/16] drm/i915: Remove (struct_mutex) locking for busy-ioctl

[Chris Wilson]

On Fri, Aug 05, 2016 at 10:05:38AM +0300, Joonas Lahtinen wrote:
> On ma, 2016-08-01 at 19:22 +0100, Chris Wilson wrote:
> > By applying the same logic as for wait-ioctl, we can query whether a
> > request has completed without holding struct_mutex. The biggest impact
> > system-wide is removing the flush_active and the contention that causes.
> > 
> > Testcase: igt/gem_busy
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Akash Goel <akash.goel at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 110 +++++++++++++++++++++++++++++-----------
> >  1 file changed, 80 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 43069b05bdd2..f2f70f5ff9f4 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3721,49 +3721,99 @@ i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
> >  	i915_vma_unpin(i915_gem_obj_to_ggtt_view(obj, view));
> >  }
> >  
> > +static __always_inline unsigned
> > +__busy_read_flag(const struct drm_i915_gem_request *request)
> > +{
> > +	return 0x10000 << request->engine->exec_id;
> > +}
> > +
> > +static __always_inline unsigned int
> > +__busy_write_flag(const struct drm_i915_gem_request *request)
> > +{
> > +	return request->engine->exec_id;
> 
> Just realized (to my horror) this is not a flag, it's a bare ID, so
> better not call the function _flag, but rather _id?

Bah.

__busy_write_id
__busy_read_flag
__busy_set_if_active

busy_set_active_write_id() { __busy_set_if_active(ptr, __busy_write_id}; )
busy_set_active_read_flag() { __busy_set_if_active(ptr, __busy_read_flag); }

> > +		 * but before we add its engine into the busy set, the other
> > +		 * thread reallocates it and assigns it to a task on another
> > +		 * engine with a fresh and incomplete seqno.
> > +		 *
> > +		 * So after we lookup the engine's id, we double check that
> > +		 * the active request is the same and only then do we add it
> > +		 * into the busy set.
> > +		 */
> > +		rcu_read_lock();
> > +
> > +		for_each_active(active, idx)
> > +			args->busy |= busy_read_flag(&obj->last_read[idx]);
> 
> So you mean this is double check against __I915_BO_ACTIVE, right?

Yes. The ABI guarantees forward progress but __I915_BO_ACTIVE itself
does not, so we confirm each of the active requests with the hardware.

Will add.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre