From 221628386a33225bf844740ec7c221450715b0f7 Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Fri, 21 Jun 2019 19:38:00 +0100 Subject: [PATCH] drm/i915: Provide an i915_active.acquire callback If we introduce a callback for i915_active that is only called the first time we use the i915_active and is symmetrically paired with the i915_active.retire callback, we can replace the open-coded and non-atomic implementations -- which will be very fragile (i.e. broken) upon removing the struct_mutex serialisation. Signed-off-by: Chris Wilson Reviewed-by: Matthew Auld Link: https://patchwork.freedesktop.org/patch/msgid/20190621183801.23252-4-chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 8 +- drivers/gpu/drm/i915/gt/intel_context.c | 65 +++--- drivers/gpu/drm/i915/gt/intel_context.h | 14 +- drivers/gpu/drm/i915/gt/intel_lrc.c | 6 +- drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 2 +- drivers/gpu/drm/i915/gt/intel_timeline.c | 16 +- drivers/gpu/drm/i915/gt/mock_engine.c | 2 +- drivers/gpu/drm/i915/i915_active.c | 225 ++++++++++--------- drivers/gpu/drm/i915/i915_active.h | 25 ++- drivers/gpu/drm/i915/i915_active_types.h | 10 +- drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- drivers/gpu/drm/i915/i915_vma.c | 22 +- drivers/gpu/drm/i915/selftests/i915_active.c | 53 ++++- 13 files changed, 258 insertions(+), 192 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 628673d1d7f85..8a9787cf0cd05 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -923,8 +923,12 @@ static int context_barrier_task(struct i915_gem_context *ctx, if (!cb) return -ENOMEM; - i915_active_init(i915, &cb->base, cb_retire); - i915_active_acquire(&cb->base); + i915_active_init(i915, &cb->base, NULL, cb_retire); + err = i915_active_acquire(&cb->base); + if (err) { + kfree(cb); + return err; + } for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) { struct i915_request *rq; diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c index 23120901c55f4..938dd032b8201 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.c +++ b/drivers/gpu/drm/i915/gt/intel_context.c @@ -95,11 +95,15 @@ void intel_context_unpin(struct intel_context *ce) intel_context_put(ce); } -static int __context_pin_state(struct i915_vma *vma, unsigned long flags) +static int __context_pin_state(struct i915_vma *vma) { + u64 flags; int err; - err = i915_vma_pin(vma, 0, 0, flags | PIN_GLOBAL); + flags = i915_ggtt_pin_bias(vma) | PIN_OFFSET_BIAS; + flags |= PIN_HIGH | PIN_GLOBAL; + + err = i915_vma_pin(vma, 0, 0, flags); if (err) return err; @@ -119,7 +123,7 @@ static void __context_unpin_state(struct i915_vma *vma) __i915_vma_unpin(vma); } -static void intel_context_retire(struct i915_active *active) +static void __intel_context_retire(struct i915_active *active) { struct intel_context *ce = container_of(active, typeof(*ce), active); @@ -130,35 +134,11 @@ static void intel_context_retire(struct i915_active *active) intel_context_put(ce); } -void -intel_context_init(struct intel_context *ce, - struct i915_gem_context *ctx, - struct intel_engine_cs *engine) -{ - GEM_BUG_ON(!engine->cops); - - kref_init(&ce->ref); - - ce->gem_context = ctx; - ce->engine = engine; - ce->ops = engine->cops; - ce->sseu = engine->sseu; - - INIT_LIST_HEAD(&ce->signal_link); - INIT_LIST_HEAD(&ce->signals); - - mutex_init(&ce->pin_mutex); - - i915_active_init(ctx->i915, &ce->active, intel_context_retire); -} - -int intel_context_active_acquire(struct intel_context *ce, unsigned long flags) +static int __intel_context_active(struct i915_active *active) { + struct intel_context *ce = container_of(active, typeof(*ce), active); int err; - if (!i915_active_acquire(&ce->active)) - return 0; - intel_context_get(ce); err = intel_ring_pin(ce->ring); @@ -168,7 +148,7 @@ int intel_context_active_acquire(struct intel_context *ce, unsigned long flags) if (!ce->state) return 0; - err = __context_pin_state(ce->state, flags); + err = __context_pin_state(ce->state); if (err) goto err_ring; @@ -188,15 +168,30 @@ err_ring: intel_ring_unpin(ce->ring); err_put: intel_context_put(ce); - i915_active_cancel(&ce->active); return err; } -void intel_context_active_release(struct intel_context *ce) +void +intel_context_init(struct intel_context *ce, + struct i915_gem_context *ctx, + struct intel_engine_cs *engine) { - /* Nodes preallocated in intel_context_active() */ - i915_active_acquire_barrier(&ce->active); - i915_active_release(&ce->active); + GEM_BUG_ON(!engine->cops); + + kref_init(&ce->ref); + + ce->gem_context = ctx; + ce->engine = engine; + ce->ops = engine->cops; + ce->sseu = engine->sseu; + + INIT_LIST_HEAD(&ce->signal_link); + INIT_LIST_HEAD(&ce->signals); + + mutex_init(&ce->pin_mutex); + + i915_active_init(ctx->i915, &ce->active, + __intel_context_active, __intel_context_retire); } static void i915_global_context_shrink(void) diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h index a47275bc4f010..40cd8320fcc39 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.h +++ b/drivers/gpu/drm/i915/gt/intel_context.h @@ -9,6 +9,7 @@ #include +#include "i915_active.h" #include "intel_context_types.h" #include "intel_engine_types.h" @@ -102,8 +103,17 @@ static inline void intel_context_exit(struct intel_context *ce) ce->ops->exit(ce); } -int intel_context_active_acquire(struct intel_context *ce, unsigned long flags); -void intel_context_active_release(struct intel_context *ce); +static inline int intel_context_active_acquire(struct intel_context *ce) +{ + return i915_active_acquire(&ce->active); +} + +static inline void intel_context_active_release(struct intel_context *ce) +{ + /* Nodes preallocated in intel_context_active() */ + i915_active_acquire_barrier(&ce->active); + i915_active_release(&ce->active); +} static inline struct intel_context *intel_context_get(struct intel_context *ce) { diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index ad7638da785d4..c8a0c9b327646 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -1542,12 +1542,10 @@ __execlists_context_pin(struct intel_context *ce, goto err; GEM_BUG_ON(!ce->state); - ret = intel_context_active_acquire(ce, - engine->i915->ggtt.pin_bias | - PIN_OFFSET_BIAS | - PIN_HIGH); + ret = intel_context_active_acquire(ce); if (ret) goto err; + GEM_BUG_ON(!i915_vma_is_pinned(ce->state)); vaddr = i915_gem_object_pin_map(ce->state->obj, i915_coherent_map_type(engine->i915) | diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c index c9337e4b5ee08..f094406dcc56c 100644 --- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c @@ -1455,7 +1455,7 @@ static int ring_context_pin(struct intel_context *ce) ce->state = vma; } - err = intel_context_active_acquire(ce, PIN_HIGH); + err = intel_context_active_acquire(ce); if (err) return err; diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.c b/drivers/gpu/drm/i915/gt/intel_timeline.c index 44273b7c96f8f..4782582749867 100644 --- a/drivers/gpu/drm/i915/gt/intel_timeline.c +++ b/drivers/gpu/drm/i915/gt/intel_timeline.c @@ -146,6 +146,15 @@ static void __cacheline_retire(struct i915_active *active) __idle_cacheline_free(cl); } +static int __cacheline_active(struct i915_active *active) +{ + struct intel_timeline_cacheline *cl = + container_of(active, typeof(*cl), active); + + __i915_vma_pin(cl->hwsp->vma); + return 0; +} + static struct intel_timeline_cacheline * cacheline_alloc(struct intel_timeline_hwsp *hwsp, unsigned int cacheline) { @@ -168,15 +177,16 @@ cacheline_alloc(struct intel_timeline_hwsp *hwsp, unsigned int cacheline) cl->hwsp = hwsp; cl->vaddr = page_pack_bits(vaddr, cacheline); - i915_active_init(hwsp->gt->i915, &cl->active, __cacheline_retire); + i915_active_init(hwsp->gt->i915, &cl->active, + __cacheline_active, __cacheline_retire); return cl; } static void cacheline_acquire(struct intel_timeline_cacheline *cl) { - if (cl && i915_active_acquire(&cl->active)) - __i915_vma_pin(cl->hwsp->vma); + if (cl) + i915_active_acquire(&cl->active); } static void cacheline_release(struct intel_timeline_cacheline *cl) diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c b/drivers/gpu/drm/i915/gt/mock_engine.c index bf0974b12f3d6..490ebd121f4c9 100644 --- a/drivers/gpu/drm/i915/gt/mock_engine.c +++ b/drivers/gpu/drm/i915/gt/mock_engine.c @@ -155,7 +155,7 @@ static int mock_context_pin(struct intel_context *ce) return -ENOMEM; } - ret = intel_context_active_acquire(ce, PIN_HIGH); + ret = intel_context_active_acquire(ce); if (ret) return ret; diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c index eb91a625c71fd..cb6a1eadf7df8 100644 --- a/drivers/gpu/drm/i915/i915_active.c +++ b/drivers/gpu/drm/i915/i915_active.c @@ -39,7 +39,7 @@ static void *active_debug_hint(void *addr) { struct i915_active *ref = addr; - return (void *)ref->retire ?: (void *)ref; + return (void *)ref->active ?: (void *)ref->retire ?: (void *)ref; } static struct debug_obj_descr active_debug_desc = { @@ -83,50 +83,58 @@ static inline void debug_active_assert(struct i915_active *ref) { } #endif static void -__active_park(struct i915_active *ref) +__active_retire(struct i915_active *ref) { struct active_node *it, *n; + struct rb_root root; + bool retire = false; + + lockdep_assert_held(&ref->mutex); + + /* return the unused nodes to our slabcache -- flushing the allocator */ + if (atomic_dec_and_test(&ref->count)) { + debug_active_deactivate(ref); + root = ref->tree; + ref->tree = RB_ROOT; + ref->cache = NULL; + retire = true; + } - rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) { + mutex_unlock(&ref->mutex); + if (!retire) + return; + + ref->retire(ref); + + rbtree_postorder_for_each_entry_safe(it, n, &root, node) { GEM_BUG_ON(i915_active_request_isset(&it->base)); kmem_cache_free(global.slab_cache, it); } - ref->tree = RB_ROOT; } static void -__active_retire(struct i915_active *ref) +active_retire(struct i915_active *ref) { - GEM_BUG_ON(!ref->count); - if (--ref->count) + GEM_BUG_ON(!atomic_read(&ref->count)); + if (atomic_add_unless(&ref->count, -1, 1)) return; - debug_active_deactivate(ref); - - /* return the unused nodes to our slabcache */ - __active_park(ref); - - ref->retire(ref); + /* One active may be flushed from inside the acquire of another */ + mutex_lock_nested(&ref->mutex, SINGLE_DEPTH_NESTING); + __active_retire(ref); } static void node_retire(struct i915_active_request *base, struct i915_request *rq) { - __active_retire(container_of(base, struct active_node, base)->ref); -} - -static void -last_retire(struct i915_active_request *base, struct i915_request *rq) -{ - __active_retire(container_of(base, struct i915_active, last)); + active_retire(container_of(base, struct active_node, base)->ref); } static struct i915_active_request * active_instance(struct i915_active *ref, u64 idx) { - struct active_node *node; + struct active_node *node, *prealloc; struct rb_node **p, *parent; - struct i915_request *old; /* * We track the most recently used timeline to skip a rbtree search @@ -134,20 +142,18 @@ active_instance(struct i915_active *ref, u64 idx) * at all. We can reuse the last slot if it is empty, that is * after the previous activity has been retired, or if it matches the * current timeline. - * - * Note that we allow the timeline to be active simultaneously in - * the rbtree and the last cache. We do this to avoid having - * to search and replace the rbtree element for a new timeline, with - * the cost being that we must be aware that the ref may be retired - * twice for the same timeline (as the older rbtree element will be - * retired before the new request added to last). */ - old = i915_active_request_raw(&ref->last, BKL(ref)); - if (!old || old->fence.context == idx) - goto out; + node = READ_ONCE(ref->cache); + if (node && node->timeline == idx) + return &node->base; - /* Move the currently active fence into the rbtree */ - idx = old->fence.context; + /* Preallocate a replacement, just in case */ + prealloc = kmem_cache_alloc(global.slab_cache, GFP_KERNEL); + if (!prealloc) + return NULL; + + mutex_lock(&ref->mutex); + GEM_BUG_ON(i915_active_is_idle(ref)); parent = NULL; p = &ref->tree.rb_node; @@ -155,8 +161,10 @@ active_instance(struct i915_active *ref, u64 idx) parent = *p; node = rb_entry(parent, struct active_node, node); - if (node->timeline == idx) - goto replace; + if (node->timeline == idx) { + kmem_cache_free(global.slab_cache, prealloc); + goto out; + } if (node->timeline < idx) p = &parent->rb_right; @@ -164,17 +172,7 @@ active_instance(struct i915_active *ref, u64 idx) p = &parent->rb_left; } - node = kmem_cache_alloc(global.slab_cache, GFP_KERNEL); - - /* kmalloc may retire the ref->last (thanks shrinker)! */ - if (unlikely(!i915_active_request_raw(&ref->last, BKL(ref)))) { - kmem_cache_free(global.slab_cache, node); - goto out; - } - - if (unlikely(!node)) - return ERR_PTR(-ENOMEM); - + node = prealloc; i915_active_request_init(&node->base, NULL, node_retire); node->ref = ref; node->timeline = idx; @@ -182,40 +180,29 @@ active_instance(struct i915_active *ref, u64 idx) rb_link_node(&node->node, parent, p); rb_insert_color(&node->node, &ref->tree); -replace: - /* - * Overwrite the previous active slot in the rbtree with last, - * leaving last zeroed. If the previous slot is still active, - * we must be careful as we now only expect to receive one retire - * callback not two, and so much undo the active counting for the - * overwritten slot. - */ - if (i915_active_request_isset(&node->base)) { - /* Retire ourselves from the old rq->active_list */ - __list_del_entry(&node->base.link); - ref->count--; - GEM_BUG_ON(!ref->count); - } - GEM_BUG_ON(list_empty(&ref->last.link)); - list_replace_init(&ref->last.link, &node->base.link); - node->base.request = fetch_and_zero(&ref->last.request); - out: - return &ref->last; + ref->cache = node; + mutex_unlock(&ref->mutex); + + return &node->base; } -void i915_active_init(struct drm_i915_private *i915, - struct i915_active *ref, - void (*retire)(struct i915_active *ref)) +void __i915_active_init(struct drm_i915_private *i915, + struct i915_active *ref, + int (*active)(struct i915_active *ref), + void (*retire)(struct i915_active *ref), + struct lock_class_key *key) { debug_active_init(ref); ref->i915 = i915; + ref->active = active; ref->retire = retire; ref->tree = RB_ROOT; - i915_active_request_init(&ref->last, NULL, last_retire); + ref->cache = NULL; init_llist_head(&ref->barriers); - ref->count = 0; + atomic_set(&ref->count, 0); + __mutex_init(&ref->mutex, "i915_active", key); } int i915_active_ref(struct i915_active *ref, @@ -223,68 +210,84 @@ int i915_active_ref(struct i915_active *ref, struct i915_request *rq) { struct i915_active_request *active; - int err = 0; + int err; /* Prevent reaping in case we malloc/wait while building the tree */ - i915_active_acquire(ref); + err = i915_active_acquire(ref); + if (err) + return err; active = active_instance(ref, timeline); - if (IS_ERR(active)) { - err = PTR_ERR(active); + if (!active) { + err = -ENOMEM; goto out; } if (!i915_active_request_isset(active)) - ref->count++; + atomic_inc(&ref->count); __i915_active_request_set(active, rq); - GEM_BUG_ON(!ref->count); out: i915_active_release(ref); return err; } -bool i915_active_acquire(struct i915_active *ref) +int i915_active_acquire(struct i915_active *ref) { + int err; + debug_active_assert(ref); - lockdep_assert_held(BKL(ref)); + if (atomic_add_unless(&ref->count, 1, 0)) + return 0; + + err = mutex_lock_interruptible(&ref->mutex); + if (err) + return err; - if (ref->count++) - return false; + if (!atomic_read(&ref->count) && ref->active) + err = ref->active(ref); + if (!err) { + debug_active_activate(ref); + atomic_inc(&ref->count); + } + + mutex_unlock(&ref->mutex); - debug_active_activate(ref); - return true; + return err; } void i915_active_release(struct i915_active *ref) { debug_active_assert(ref); - lockdep_assert_held(BKL(ref)); - - __active_retire(ref); + active_retire(ref); } int i915_active_wait(struct i915_active *ref) { struct active_node *it, *n; - int ret = 0; + int err; - if (i915_active_acquire(ref)) - goto out_release; + might_sleep(); + if (RB_EMPTY_ROOT(&ref->tree)) + return 0; - ret = i915_active_request_retire(&ref->last, BKL(ref)); - if (ret) - goto out_release; + err = mutex_lock_interruptible(&ref->mutex); + if (err) + return err; + + if (!atomic_add_unless(&ref->count, 1, 0)) { + mutex_unlock(&ref->mutex); + return 0; + } rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) { - ret = i915_active_request_retire(&it->base, BKL(ref)); - if (ret) + err = i915_active_request_retire(&it->base, BKL(ref)); + if (err) break; } -out_release: - i915_active_release(ref); - return ret; + __active_retire(ref); + return err; } int i915_request_await_active_request(struct i915_request *rq, @@ -299,23 +302,24 @@ int i915_request_await_active_request(struct i915_request *rq, int i915_request_await_active(struct i915_request *rq, struct i915_active *ref) { struct active_node *it, *n; - int err = 0; + int err; - /* await allocates and so we need to avoid hitting the shrinker */ - if (i915_active_acquire(ref)) - goto out; /* was idle */ + if (RB_EMPTY_ROOT(&ref->tree)) + return 0; - err = i915_request_await_active_request(rq, &ref->last); + /* await allocates and so we need to avoid hitting the shrinker */ + err = i915_active_acquire(ref); if (err) - goto out; + return err; + mutex_lock(&ref->mutex); rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) { err = i915_request_await_active_request(rq, &it->base); if (err) - goto out; + break; } + mutex_unlock(&ref->mutex); -out: i915_active_release(ref); return err; } @@ -324,9 +328,9 @@ out: void i915_active_fini(struct i915_active *ref) { debug_active_fini(ref); - GEM_BUG_ON(i915_active_request_isset(&ref->last)); GEM_BUG_ON(!RB_EMPTY_ROOT(&ref->tree)); - GEM_BUG_ON(ref->count); + GEM_BUG_ON(atomic_read(&ref->count)); + mutex_destroy(&ref->mutex); } #endif @@ -353,7 +357,7 @@ int i915_active_acquire_preallocate_barrier(struct i915_active *ref, (void *)engine, node_retire); node->timeline = kctx->ring->timeline->fence_context; node->ref = ref; - ref->count++; + atomic_inc(&ref->count); intel_engine_pm_get(engine); llist_add((struct llist_node *)&node->base.link, @@ -380,8 +384,9 @@ void i915_active_acquire_barrier(struct i915_active *ref) { struct llist_node *pos, *next; - i915_active_acquire(ref); + GEM_BUG_ON(i915_active_is_idle(ref)); + mutex_lock_nested(&ref->mutex, SINGLE_DEPTH_NESTING); llist_for_each_safe(pos, next, llist_del_all(&ref->barriers)) { struct intel_engine_cs *engine; struct active_node *node; @@ -411,7 +416,7 @@ void i915_active_acquire_barrier(struct i915_active *ref) &engine->barrier_tasks); intel_engine_pm_put(engine); } - i915_active_release(ref); + mutex_unlock(&ref->mutex); } void i915_request_add_barriers(struct i915_request *rq) diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h index c14eebf6d0747..134166d312512 100644 --- a/drivers/gpu/drm/i915/i915_active.h +++ b/drivers/gpu/drm/i915/i915_active.h @@ -369,9 +369,16 @@ i915_active_request_retire(struct i915_active_request *active, * synchronisation. */ -void i915_active_init(struct drm_i915_private *i915, - struct i915_active *ref, - void (*retire)(struct i915_active *ref)); +void __i915_active_init(struct drm_i915_private *i915, + struct i915_active *ref, + int (*active)(struct i915_active *ref), + void (*retire)(struct i915_active *ref), + struct lock_class_key *key); +#define i915_active_init(i915, ref, active, retire) do { \ + static struct lock_class_key __key; \ + \ + __i915_active_init(i915, ref, active, retire, &__key); \ +} while (0) int i915_active_ref(struct i915_active *ref, u64 timeline, @@ -384,20 +391,14 @@ int i915_request_await_active(struct i915_request *rq, int i915_request_await_active_request(struct i915_request *rq, struct i915_active_request *active); -bool i915_active_acquire(struct i915_active *ref); - -static inline void i915_active_cancel(struct i915_active *ref) -{ - GEM_BUG_ON(ref->count != 1); - ref->count = 0; -} - +int i915_active_acquire(struct i915_active *ref); void i915_active_release(struct i915_active *ref); +void __i915_active_release_nested(struct i915_active *ref, int subclass); static inline bool i915_active_is_idle(const struct i915_active *ref) { - return !ref->count; + return !atomic_read(&ref->count); } #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) diff --git a/drivers/gpu/drm/i915/i915_active_types.h b/drivers/gpu/drm/i915/i915_active_types.h index c025991b9233e..5b0a3024ce242 100644 --- a/drivers/gpu/drm/i915/i915_active_types.h +++ b/drivers/gpu/drm/i915/i915_active_types.h @@ -7,7 +7,9 @@ #ifndef _I915_ACTIVE_TYPES_H_ #define _I915_ACTIVE_TYPES_H_ +#include #include +#include #include #include @@ -24,13 +26,17 @@ struct i915_active_request { i915_active_retire_fn retire; }; +struct active_node; + struct i915_active { struct drm_i915_private *i915; + struct active_node *cache; struct rb_root tree; - struct i915_active_request last; - unsigned int count; + struct mutex mutex; + atomic_t count; + int (*active)(struct i915_active *ref); void (*retire)(struct i915_active *ref); struct llist_head barriers; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 90f3673976564..ff1d5008a256a 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2055,7 +2055,7 @@ static struct i915_vma *pd_vma_create(struct gen6_ppgtt *ppgtt, int size) if (!vma) return ERR_PTR(-ENOMEM); - i915_active_init(i915, &vma->active, NULL); + i915_active_init(i915, &vma->active, NULL, NULL); INIT_ACTIVE_REQUEST(&vma->last_fence); vma->vm = &ggtt->vm; diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index c13b86e6ef1f9..c20a3022cd80e 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -78,11 +78,20 @@ static void vma_print_allocator(struct i915_vma *vma, const char *reason) #endif -static void __i915_vma_retire(struct i915_active *ref) +static inline struct i915_vma *active_to_vma(struct i915_active *ref) { - struct i915_vma *vma = container_of(ref, typeof(*vma), active); + return container_of(ref, typeof(struct i915_vma), active); +} - i915_vma_put(vma); +static int __i915_vma_active(struct i915_active *ref) +{ + i915_vma_get(active_to_vma(ref)); + return 0; +} + +static void __i915_vma_retire(struct i915_active *ref) +{ + i915_vma_put(active_to_vma(ref)); } static struct i915_vma * @@ -107,7 +116,8 @@ vma_create(struct drm_i915_gem_object *obj, vma->size = obj->base.size; vma->display_alignment = I915_GTT_MIN_ALIGNMENT; - i915_active_init(vm->i915, &vma->active, __i915_vma_retire); + i915_active_init(vm->i915, &vma->active, + __i915_vma_active, __i915_vma_retire); INIT_ACTIVE_REQUEST(&vma->last_fence); INIT_LIST_HEAD(&vma->closed_link); @@ -904,11 +914,7 @@ int i915_vma_move_to_active(struct i915_vma *vma, * add the active reference first and queue for it to be dropped * *last*. */ - if (i915_active_acquire(&vma->active)) - i915_vma_get(vma); - err = i915_active_ref(&vma->active, rq->fence.context, rq); - i915_active_release(&vma->active); if (unlikely(err)) return err; diff --git a/drivers/gpu/drm/i915/selftests/i915_active.c b/drivers/gpu/drm/i915/selftests/i915_active.c index 98493bcc91f29..84fce379c0def 100644 --- a/drivers/gpu/drm/i915/selftests/i915_active.c +++ b/drivers/gpu/drm/i915/selftests/i915_active.c @@ -4,6 +4,8 @@ * Copyright © 2018 Intel Corporation */ +#include + #include "gem/i915_gem_pm.h" #include "i915_selftest.h" @@ -13,20 +15,47 @@ struct live_active { struct i915_active base; + struct kref ref; bool retired; }; +static void __live_get(struct live_active *active) +{ + kref_get(&active->ref); +} + static void __live_free(struct live_active *active) { i915_active_fini(&active->base); kfree(active); } +static void __live_release(struct kref *ref) +{ + struct live_active *active = container_of(ref, typeof(*active), ref); + + __live_free(active); +} + +static void __live_put(struct live_active *active) +{ + kref_put(&active->ref, __live_release); +} + +static int __live_active(struct i915_active *base) +{ + struct live_active *active = container_of(base, typeof(*active), base); + + __live_get(active); + return 0; +} + static void __live_retire(struct i915_active *base) { struct live_active *active = container_of(base, typeof(*active), base); active->retired = true; + __live_put(active); } static struct live_active *__live_alloc(struct drm_i915_private *i915) @@ -37,7 +66,8 @@ static struct live_active *__live_alloc(struct drm_i915_private *i915) if (!active) return NULL; - i915_active_init(i915, &active->base, __live_retire); + kref_init(&active->ref); + i915_active_init(i915, &active->base, __live_active, __live_retire); return active; } @@ -62,11 +92,9 @@ __live_active_setup(struct drm_i915_private *i915) return ERR_PTR(-ENOMEM); } - if (!i915_active_acquire(&active->base)) { - pr_err("First i915_active_acquire should report being idle\n"); - err = -EINVAL; + err = i915_active_acquire(&active->base); + if (err) goto out; - } for_each_engine(engine, i915, id) { struct i915_request *rq; @@ -97,18 +125,21 @@ __live_active_setup(struct drm_i915_private *i915) pr_err("i915_active retired before submission!\n"); err = -EINVAL; } - if (active->base.count != count) { + if (atomic_read(&active->base.count) != count) { pr_err("i915_active not tracking all requests, found %d, expected %d\n", - active->base.count, count); + atomic_read(&active->base.count), count); err = -EINVAL; } out: i915_sw_fence_commit(submit); heap_fence_put(submit); + if (err) { + __live_put(active); + active = ERR_PTR(err); + } - /* XXX leaks live_active on error */ - return err ? ERR_PTR(err) : active; + return active; } static int live_active_wait(void *arg) @@ -135,7 +166,7 @@ static int live_active_wait(void *arg) err = -EINVAL; } - __live_free(active); + __live_put(active); if (igt_flush_test(i915, I915_WAIT_LOCKED)) err = -EIO; @@ -174,7 +205,7 @@ static int live_active_retire(void *arg) err = -EINVAL; } - __live_free(active); + __live_put(active); err: intel_runtime_pm_put(&i915->runtime_pm, wakeref); -- 2.39.5