[Intel-gfx] [PATCH 03/20] drm/i915/gem: Don't drop the timeline lock during execbuf

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-07-08 17:54:51)
> 
> On 06/07/2020 07:19, Chris Wilson wrote:
> > @@ -662,18 +692,22 @@ static int eb_reserve(struct i915_execbuffer *eb)
> >        * room for the earlier objects *unless* we need to defragment.
> >        */
> >   
> > -     if (mutex_lock_interruptible(&eb->i915->drm.struct_mutex))
> > -             return -EINTR;
> > -
> >       pass = 0;
> >       do {
> > +             int err = 0;
> > +
> > +             if (mutex_lock_interruptible(&eb->i915->drm.struct_mutex))
> > +                     return -EINTR;
> 
> Recently you explained to me why we still use struct mutex here so 
> maybe, while moving the code, document that in a comment.

Part of the work here is to eliminate the need for the struct_mutex,
that will be replaced by not dropping the vm->mutex while binding
multiple vma.

It's the interaction with the waits to flush other vm users when under
pressure that are the most annoying. This area is not straightforward,
and at least deserves some comments so that the thinking behind it can
be fixed.

> > +static struct i915_request *
> > +nested_request_create(struct intel_context *ce)
> > +{
> > +     struct i915_request *rq;
> > +
> > +     /* XXX This only works once; replace with shared timeline */
> 
> Once as in attempt to use the same local intel_context from another eb 
> would upset lockdep? It's not a problem I think.

"Once" as in this is the only time we can do this nested locking between
engines of the same context in the whole driver, or else lockdep would
have been right to complain. [i.e. if we ever do the reserve nesting, we
are screwed.]

Fwiw, I have posted patches that will eliminate the need for a nested
timeline here :)

> > +     mutex_lock_nested(&ce->timeline->mutex, SINGLE_DEPTH_NESTING);
> > +     intel_context_enter(ce);


> >   static int __eb_pin_engine(struct i915_execbuffer *eb, struct intel_context *ce)
> >   {
> >       struct intel_timeline *tl;
> > @@ -2087,9 +2174,7 @@ static int __eb_pin_engine(struct i915_execbuffer *eb, struct intel_context *ce)
> >       intel_context_enter(ce);
> >       rq = eb_throttle(ce);
> >   
> > -     intel_context_timeline_unlock(tl);
> > -
> > -     if (rq) {
> > +     while (rq) {
> >               bool nonblock = eb->file->filp->f_flags & O_NONBLOCK;
> >               long timeout;
> >   
> > @@ -2097,23 +2182,34 @@ static int __eb_pin_engine(struct i915_execbuffer *eb, struct intel_context *ce)
> >               if (nonblock)
> >                       timeout = 0;
> >   
> > +             mutex_unlock(&tl->mutex);
> 
> "Don't drop the timeline lock during execbuf"? Is the "during execbuf" 
> actually a smaller subset

We are before execbuf in my book :)

This is throttle the hog before we start, and reserve enough space in
the ring (we make sure there's a page, or thereabouts) to build a batch
without interruption.
 
> > +
> >               timeout = i915_request_wait(rq,
> >                                           I915_WAIT_INTERRUPTIBLE,
> >                                           timeout);
> >               i915_request_put(rq);
> >   
> > +             mutex_lock(&tl->mutex);
> > +
> >               if (timeout < 0) {
> >                       err = nonblock ? -EWOULDBLOCK : timeout;
> >                       goto err_exit;
> >               }
> > +
> > +             retire_requests(tl, NULL);
> > +             rq = eb_throttle(ce);
> 
> Alternative to avoid two call sites to eb_throttle of
> 
>    while (rq = eb_throttle(ce)) {
> 
> Or checkpatch does not like it?

Ta, that loop was annoying me, and I couldn't quite put my finger on
what.

checkpatch.pl --strict is quiet. Appears it only hates if (x = y).
-Chris