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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On pe, 2016-08-05 at 08:34 +0100, Chris Wilson wrote:
> 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@chris-wilson.co.uk>
> > > Cc: Akash Goel <akash.goel@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); }
> 

This would be OK,

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> > 
> > > 
> > > +		 * 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
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx