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

Chris Wilson
2016-08-05T07:34:53Z

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); }

> > +  * 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