[Intel-gfx] [PATCH 12/38] drm/i915: Move list of timelines under its own lock
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Jan 18 16:28:36 UTC 2019
On 18/01/2019 14:00, 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>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
Regards,
Tvrtko
> ---
> drivers/gpu/drm/i915/i915_drv.h | 5 +-
> drivers/gpu/drm/i915/i915_gem.c | 88 +++++++++----------
> drivers/gpu/drm/i915/i915_reset.c | 8 +-
> drivers/gpu/drm/i915/i915_timeline.c | 38 ++++++--
> drivers/gpu/drm/i915/i915_timeline.h | 3 +
> .../gpu/drm/i915/selftests/mock_gem_device.c | 7 +-
> .../gpu/drm/i915/selftests/mock_timeline.c | 3 +-
> 7 files changed, 94 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 */
> + struct list_head list;
> + } timelines;
>
> struct list_head active_rings;
> struct list_head closed_vma;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 15acd052da46..0b44059dfab2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3222,33 +3222,6 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> return ret;
> }
>
> -static long wait_for_timeline(struct i915_timeline *tl,
> - unsigned int flags, long timeout)
> -{
> - struct i915_request *rq;
> -
> - rq = i915_gem_active_get_unlocked(&tl->last_request);
> - if (!rq)
> - return timeout;
> -
> - /*
> - * "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);
> -
> - return timeout;
> -}
> -
> static int wait_for_engines(struct drm_i915_private *i915)
> {
> if (wait_for(intel_engines_are_idle(i915), I915_IDLE_ENGINES_TIMEOUT)) {
> @@ -3265,6 +3238,8 @@ static int wait_for_engines(struct drm_i915_private *i915)
> int i915_gem_wait_for_idle(struct drm_i915_private *i915,
> unsigned int flags, long timeout)
> {
> + struct i915_timeline *tl;
> +
> GEM_TRACE("flags=%x (%s), timeout=%ld%s\n",
> flags, flags & I915_WAIT_LOCKED ? "locked" : "unlocked",
> timeout, timeout == MAX_SCHEDULE_TIMEOUT ? " (forever)" : "");
> @@ -3273,17 +3248,44 @@ int i915_gem_wait_for_idle(struct drm_i915_private *i915,
> if (!READ_ONCE(i915->gt.awake))
> return 0;
>
> + 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;
> +
> + /* restart after reacquiring the lock */
> + mutex_lock(&i915->gt.timelines.mutex);
> + tl = list_entry(&i915->gt.timelines.list, typeof(*tl), link);
> + }
> + mutex_unlock(&i915->gt.timelines.mutex);
> +
> if (flags & I915_WAIT_LOCKED) {
> - struct i915_timeline *tl;
> int err;
>
> lockdep_assert_held(&i915->drm.struct_mutex);
>
> - list_for_each_entry(tl, &i915->gt.timelines, link) {
> - timeout = wait_for_timeline(tl, flags, timeout);
> - if (timeout < 0)
> - return timeout;
> - }
> if (GEM_SHOW_DEBUG() && !timeout) {
> /* Presume that timeout was non-zero to begin with! */
> dev_warn(&i915->drm.pdev->dev,
> @@ -3297,17 +3299,6 @@ int i915_gem_wait_for_idle(struct drm_i915_private *i915,
>
> i915_retire_requests(i915);
> GEM_BUG_ON(i915->gt.active_requests);
> - } else {
> - struct intel_engine_cs *engine;
> - enum intel_engine_id id;
> -
> - for_each_engine(engine, i915, id) {
> - struct i915_timeline *tl = &engine->timeline;
> -
> - timeout = wait_for_timeline(tl, flags, timeout);
> - if (timeout < 0)
> - return timeout;
> - }
> }
>
> return 0;
> @@ -5008,6 +4999,8 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
> dev_priv->gt.cleanup_engine = intel_engine_cleanup;
> }
>
> + i915_timelines_init(dev_priv);
> +
> ret = i915_gem_init_userptr(dev_priv);
> if (ret)
> return ret;
> @@ -5130,8 +5123,10 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
> err_uc_misc:
> intel_uc_fini_misc(dev_priv);
>
> - if (ret != -EIO)
> + if (ret != -EIO) {
> i915_gem_cleanup_userptr(dev_priv);
> + i915_timelines_fini(dev_priv);
> + }
>
> if (ret == -EIO) {
> mutex_lock(&dev_priv->drm.struct_mutex);
> @@ -5182,6 +5177,7 @@ void i915_gem_fini(struct drm_i915_private *dev_priv)
>
> intel_uc_fini_misc(dev_priv);
> i915_gem_cleanup_userptr(dev_priv);
> + i915_timelines_fini(dev_priv);
>
> i915_gem_drain_freed_objects(dev_priv);
>
> @@ -5284,7 +5280,6 @@ int i915_gem_init_early(struct drm_i915_private *dev_priv)
> if (!dev_priv->priorities)
> goto err_dependencies;
>
> - INIT_LIST_HEAD(&dev_priv->gt.timelines);
> INIT_LIST_HEAD(&dev_priv->gt.active_rings);
> INIT_LIST_HEAD(&dev_priv->gt.closed_vma);
>
> @@ -5328,7 +5323,6 @@ void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
> GEM_BUG_ON(!llist_empty(&dev_priv->mm.free_list));
> GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
> WARN_ON(dev_priv->mm.object_count);
> - WARN_ON(!list_empty(&dev_priv->gt.timelines));
>
> kmem_cache_destroy(dev_priv->priorities);
> kmem_cache_destroy(dev_priv->dependencies);
> diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> index d44b095e2860..12e5a2bc825c 100644
> --- a/drivers/gpu/drm/i915/i915_reset.c
> +++ b/drivers/gpu/drm/i915/i915_reset.c
> @@ -850,7 +850,8 @@ bool i915_gem_unset_wedged(struct drm_i915_private *i915)
> *
> * No more can be submitted until we reset the wedged bit.
> */
> - list_for_each_entry(tl, &i915->gt.timelines, link) {
> + mutex_lock(&i915->gt.timelines.mutex);
> + list_for_each_entry(tl, &i915->gt.timelines.list, link) {
> struct i915_request *rq;
> long timeout;
>
> @@ -872,9 +873,12 @@ bool i915_gem_unset_wedged(struct drm_i915_private *i915)
> timeout = dma_fence_default_wait(&rq->fence, true,
> MAX_SCHEDULE_TIMEOUT);
> i915_request_put(rq);
> - if (timeout < 0)
> + if (timeout < 0) {
> + mutex_unlock(&i915->gt.timelines.mutex);
> goto unlock;
> + }
> }
> + mutex_unlock(&i915->gt.timelines.mutex);
>
> intel_engines_sanitize(i915, false);
>
> diff --git a/drivers/gpu/drm/i915/i915_timeline.c b/drivers/gpu/drm/i915/i915_timeline.c
> index 4667cc08c416..84550f17d3df 100644
> --- a/drivers/gpu/drm/i915/i915_timeline.c
> +++ b/drivers/gpu/drm/i915/i915_timeline.c
> @@ -13,7 +13,7 @@ void i915_timeline_init(struct drm_i915_private *i915,
> struct i915_timeline *timeline,
> const char *name)
> {
> - lockdep_assert_held(&i915->drm.struct_mutex);
> + struct i915_gt_timelines *gt = &i915->gt.timelines;
>
> /*
> * Ideally we want a set of engines on a single leaf as we expect
> @@ -23,9 +23,12 @@ void i915_timeline_init(struct drm_i915_private *i915,
> */
> BUILD_BUG_ON(KSYNCMAP < I915_NUM_ENGINES);
>
> + timeline->i915 = i915;
> timeline->name = name;
>
> - list_add(&timeline->link, &i915->gt.timelines);
> + mutex_lock(>->mutex);
> + list_add(&timeline->link, >->list);
> + mutex_unlock(>->mutex);
>
> /* Called during early_init before we know how many engines there are */
>
> @@ -39,6 +42,17 @@ void i915_timeline_init(struct drm_i915_private *i915,
> i915_syncmap_init(&timeline->sync);
> }
>
> +void i915_timelines_init(struct drm_i915_private *i915)
> +{
> + struct i915_gt_timelines *gt = &i915->gt.timelines;
> +
> + mutex_init(>->mutex);
> + INIT_LIST_HEAD(>->list);
> +
> + /* via i915_gem_wait_for_idle() */
> + i915_gem_shrinker_taints_mutex(i915, >->mutex);
> +}
> +
> /**
> * i915_timelines_park - called when the driver idles
> * @i915: the drm_i915_private device
> @@ -51,11 +65,11 @@ void i915_timeline_init(struct drm_i915_private *i915,
> */
> void i915_timelines_park(struct drm_i915_private *i915)
> {
> + struct i915_gt_timelines *gt = &i915->gt.timelines;
> struct i915_timeline *timeline;
>
> - lockdep_assert_held(&i915->drm.struct_mutex);
> -
> - list_for_each_entry(timeline, &i915->gt.timelines, link) {
> + mutex_lock(>->mutex);
> + list_for_each_entry(timeline, >->list, link) {
> /*
> * All known fences are completed so we can scrap
> * the current sync point tracking and start afresh,
> @@ -64,15 +78,20 @@ void i915_timelines_park(struct drm_i915_private *i915)
> */
> i915_syncmap_free(&timeline->sync);
> }
> + mutex_unlock(>->mutex);
> }
>
> void i915_timeline_fini(struct i915_timeline *timeline)
> {
> + struct i915_gt_timelines *gt = &timeline->i915->gt.timelines;
> +
> GEM_BUG_ON(!list_empty(&timeline->requests));
>
> i915_syncmap_free(&timeline->sync);
>
> + mutex_lock(>->mutex);
> list_del(&timeline->link);
> + mutex_unlock(>->mutex);
> }
>
> struct i915_timeline *
> @@ -99,6 +118,15 @@ void __i915_timeline_free(struct kref *kref)
> kfree(timeline);
> }
>
> +void i915_timelines_fini(struct drm_i915_private *i915)
> +{
> + struct i915_gt_timelines *gt = &i915->gt.timelines;
> +
> + GEM_BUG_ON(!list_empty(>->list));
> +
> + mutex_destroy(>->mutex);
> +}
> +
> #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> #include "selftests/mock_timeline.c"
> #include "selftests/i915_timeline.c"
> diff --git a/drivers/gpu/drm/i915/i915_timeline.h b/drivers/gpu/drm/i915/i915_timeline.h
> index 38c1e15e927a..87ad2dd31c20 100644
> --- a/drivers/gpu/drm/i915/i915_timeline.h
> +++ b/drivers/gpu/drm/i915/i915_timeline.h
> @@ -66,6 +66,7 @@ struct i915_timeline {
>
> struct list_head link;
> const char *name;
> + struct drm_i915_private *i915;
>
> struct kref kref;
> };
> @@ -134,6 +135,8 @@ static inline bool i915_timeline_sync_is_later(struct i915_timeline *tl,
> return __i915_timeline_sync_is_later(tl, fence->context, fence->seqno);
> }
>
> +void i915_timelines_init(struct drm_i915_private *i915);
> void i915_timelines_park(struct drm_i915_private *i915);
> +void i915_timelines_fini(struct drm_i915_private *i915);
>
> #endif
> diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> index 888c6978bc54..41ae502361d7 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> @@ -68,13 +68,14 @@ static void mock_device_release(struct drm_device *dev)
> i915_gem_contexts_fini(i915);
> mutex_unlock(&i915->drm.struct_mutex);
>
> + i915_timelines_fini(i915);
> +
> drain_workqueue(i915->wq);
> i915_gem_drain_freed_objects(i915);
>
> mutex_lock(&i915->drm.struct_mutex);
> mock_fini_ggtt(i915);
> mutex_unlock(&i915->drm.struct_mutex);
> - WARN_ON(!list_empty(&i915->gt.timelines));
>
> destroy_workqueue(i915->wq);
>
> @@ -226,7 +227,8 @@ struct drm_i915_private *mock_gem_device(void)
> if (!i915->priorities)
> goto err_dependencies;
>
> - INIT_LIST_HEAD(&i915->gt.timelines);
> + i915_timelines_init(i915);
> +
> INIT_LIST_HEAD(&i915->gt.active_rings);
> INIT_LIST_HEAD(&i915->gt.closed_vma);
>
> @@ -253,6 +255,7 @@ struct drm_i915_private *mock_gem_device(void)
> i915_gem_contexts_fini(i915);
> err_unlock:
> mutex_unlock(&i915->drm.struct_mutex);
> + i915_timelines_fini(i915);
> kmem_cache_destroy(i915->priorities);
> err_dependencies:
> kmem_cache_destroy(i915->dependencies);
> diff --git a/drivers/gpu/drm/i915/selftests/mock_timeline.c b/drivers/gpu/drm/i915/selftests/mock_timeline.c
> index dcf3b16f5a07..cf39ccd9fc05 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_timeline.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_timeline.c
> @@ -10,6 +10,7 @@
>
> void mock_timeline_init(struct i915_timeline *timeline, u64 context)
> {
> + timeline->i915 = NULL;
> timeline->fence_context = context;
>
> spin_lock_init(&timeline->lock);
> @@ -24,5 +25,5 @@ void mock_timeline_init(struct i915_timeline *timeline, u64 context)
>
> void mock_timeline_fini(struct i915_timeline *timeline)
> {
> - i915_timeline_fini(timeline);
> + i915_syncmap_free(&timeline->sync);
> }
>
More information about the Intel-gfx
mailing list