[Intel-gfx] [PATCH 13/23] drm/i915: Move list of timelines under its own lock

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-17 17:54:38)
> 
> On 17/01/2019 14:34, Chris Wilson wrote:
> > Currently, the list of timelines is serialised by the struct_mutex, but
> > to alleviate difficulties with using that mutex in future, move the
> > list management under its own dedicated mutex.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h               |  5 +-
> >   drivers/gpu/drm/i915/i915_gem.c               | 89 +++++++++----------
> >   drivers/gpu/drm/i915/i915_reset.c             |  8 +-
> >   drivers/gpu/drm/i915/i915_timeline.c          | 38 ++++++--
> >   drivers/gpu/drm/i915/i915_timeline.h          |  3 +
> >   drivers/gpu/drm/i915/i915_vma.c               |  6 ++
> >   .../gpu/drm/i915/selftests/mock_gem_device.c  |  7 +-
> >   .../gpu/drm/i915/selftests/mock_timeline.c    |  3 +-
> >   8 files changed, 101 insertions(+), 58 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 94680b15bed0..3913900600b7 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1975,7 +1975,10 @@ struct drm_i915_private {
> >               void (*resume)(struct drm_i915_private *);
> >               void (*cleanup_engine)(struct intel_engine_cs *engine);
> >   
> > -             struct list_head timelines;
> > +             struct i915_gt_timelines {
> > +                     struct mutex mutex; /* protects list, tainted by GPU */
> 
> What does it mean "tainted by GPU"?

Shorthand for we wait under this lock from inside the shrinker; a
reminder about i915_gem_shrinker_taints_mutex() affecting this mutex.

> > +     mutex_lock(&i915->gt.timelines.mutex);
> > +     list_for_each_entry(tl, &i915->gt.timelines.list, link) {
> > +             struct i915_request *rq;
> > +
> > +             rq = i915_gem_active_get_unlocked(&tl->last_request);
> > +             if (!rq)
> > +                     continue;
> > +
> > +             mutex_unlock(&i915->gt.timelines.mutex);
> > +
> > +             /*
> > +              * "Race-to-idle".
> > +              *
> > +              * Switching to the kernel context is often used a synchronous
> > +              * step prior to idling, e.g. in suspend for flushing all
> > +              * current operations to memory before sleeping. These we
> > +              * want to complete as quickly as possible to avoid prolonged
> > +              * stalls, so allow the gpu to boost to maximum clocks.
> > +              */
> > +             if (flags & I915_WAIT_FOR_IDLE_BOOST)
> > +                     gen6_rps_boost(rq, NULL);
> > +
> > +             timeout = i915_request_wait(rq, flags, timeout);
> > +             i915_request_put(rq);
> > +             if (timeout < 0)
> > +                     return timeout;
> > +
> > +             mutex_lock(&i915->gt.timelines.mutex);
> > +
> > +             /* restart after dropping the lock */
> > +             tl = list_entry(&i915->gt.timelines.list, typeof(*tl), link);
> > +     }
> > +     mutex_unlock(&i915->gt.timelines.mutex);
> 
> Hm, since the loop above bothers restarting after dropping the lock, 
> that implies when we drop the lock here we may not be idle any longer. 
> Or we actually still depend on struct_mutex and this is another small 
> charade? I guess so, since without this patch we also have two path with 
> different levels of idleness guarantee.

Yes. The difference between I915_WAIT_LOCKED and not is that we can only
guarantee we idle if LOCKED (to be replaced by a rw semaphore around
request emission I think).

Iirc we are down to only one "unlocked" user of
i915_gem_wait_for_idle(), that's the vt-d w/a for gen5, which is
probably broken anyway (one poor fool tried it apparently) precisely
because we can't make the guarantee that it remains idle without
struct_mutex.

[snip]

> Looks okay.
> 
> Apart that I am 9/10 worried of how the long game of fine grained 
> locking will untangle, or in other words, how much you managed to nail 
> all the new locks and how much you'll have to re-fiddle with them. :I 
> But maybe you see the end game so I won't project my inability to do so.

There'll always be refiddling! The prospect of KTSAN is a welcome relief
to playing whack-a-mole. My short term goal is be able to generate a
kernel_context request (where we know everything is pinned a priori)
without struct_mutex which should be a much simpler task than the whole
execbuf user interface (but still we have to rearrange various tasks
such as context locking/pinning and acquiring runtime references).
-Chris