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 <ch...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
