Message ID | 20190412085410.10392-19-chris@chris-wilson.co.uk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [01/50] drm/i915: Introduce struct class_instance for engines across the uAPI | expand |
On 12/04/2019 09:53, Chris Wilson wrote: > We no longer need to track the active intel_contexts within each engine, > allowing us to drop a tricky mutex_lock from inside unpin (which may > occur inside fs_reclaim). > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/gt/intel_context.c | 11 +---------- > drivers/gpu/drm/i915/gt/intel_context_types.h | 1 - > drivers/gpu/drm/i915/i915_debugfs.c | 13 +++++++++---- > drivers/gpu/drm/i915/i915_gem_context.c | 2 -- > drivers/gpu/drm/i915/i915_gem_context_types.h | 1 - > drivers/gpu/drm/i915/selftests/i915_gem_context.c | 1 - > drivers/gpu/drm/i915/selftests/mock_context.c | 1 - > 7 files changed, 10 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c > index 5e506e648454..1f1761fc6597 100644 > --- a/drivers/gpu/drm/i915/gt/intel_context.c > +++ b/drivers/gpu/drm/i915/gt/intel_context.c > @@ -49,7 +49,6 @@ int __intel_context_do_pin(struct intel_context *ce) > return -EINTR; > > if (likely(!atomic_read(&ce->pin_count))) { > - struct i915_gem_context *ctx = ce->gem_context; > intel_wakeref_t wakeref; > > err = 0; > @@ -58,11 +57,7 @@ int __intel_context_do_pin(struct intel_context *ce) > if (err) > goto err; > > - i915_gem_context_get(ctx); > - > - mutex_lock(&ctx->mutex); > - list_add(&ce->active_link, &ctx->active_engines); > - mutex_unlock(&ctx->mutex); > + i915_gem_context_get(ce->gem_context); /* for ctx->ppgtt */ > > intel_context_get(ce); > smp_mb__before_atomic(); /* flush pin before it is visible */ > @@ -91,10 +86,6 @@ void intel_context_unpin(struct intel_context *ce) > if (likely(atomic_dec_and_test(&ce->pin_count))) { > ce->ops->unpin(ce); > > - mutex_lock(&ce->gem_context->mutex); > - list_del(&ce->active_link); > - mutex_unlock(&ce->gem_context->mutex); > - > i915_gem_context_put(ce->gem_context); > intel_context_put(ce); > } > diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h > index 3579c2708321..d5a7dbd0daee 100644 > --- a/drivers/gpu/drm/i915/gt/intel_context_types.h > +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h > @@ -38,7 +38,6 @@ struct intel_context { > struct intel_engine_cs *engine; > struct intel_engine_cs *active; > > - struct list_head active_link; > struct list_head signal_link; > struct list_head signals; > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 58956b49f392..6c6bd50d87c9 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -34,6 +34,7 @@ > > #include "gt/intel_reset.h" > > +#include "i915_gem_context.h" > #include "intel_dp.h" > #include "intel_drv.h" > #include "intel_fbc.h" > @@ -396,9 +397,11 @@ static void print_context_stats(struct seq_file *m, > struct i915_gem_context *ctx; > > list_for_each_entry(ctx, &i915->contexts.list, link) { > - struct intel_context *ce; > + struct i915_gem_engines_iter it; > + > + for_each_gem_engine(it, ctx) { Why no i915_gem_context_lock_engines in debugfs? Regards, Tvrtko > + struct intel_context *ce = it.context; > > - list_for_each_entry(ce, &ctx->active_engines, active_link) { > if (ce->state) > per_file_stats(0, ce->state->obj, &kstats); > if (ce->ring) > @@ -1893,7 +1896,7 @@ static int i915_context_status(struct seq_file *m, void *unused) > return ret; > > list_for_each_entry(ctx, &dev_priv->contexts.list, link) { > - struct intel_context *ce; > + struct i915_gem_engines_iter it; > > seq_puts(m, "HW context "); > if (!list_empty(&ctx->hw_id_link)) > @@ -1917,7 +1920,9 @@ static int i915_context_status(struct seq_file *m, void *unused) > seq_putc(m, ctx->remap_slice ? 'R' : 'r'); > seq_putc(m, '\n'); > > - list_for_each_entry(ce, &ctx->active_engines, active_link) { > + for_each_gem_engine(it, ctx) { > + struct intel_context *ce = it.context; > + > seq_printf(m, "%s: ", ce->engine->name); > if (ce->state) > describe_obj(m, ce->state->obj); > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 99c89dc7533e..889cbc0554e5 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -289,7 +289,6 @@ static void i915_gem_context_free(struct i915_gem_context *ctx) > { > lockdep_assert_held(&ctx->i915->drm.struct_mutex); > GEM_BUG_ON(!i915_gem_context_is_closed(ctx)); > - GEM_BUG_ON(!list_empty(&ctx->active_engines)); > > release_hw_id(ctx); > i915_ppgtt_put(ctx->ppgtt); > @@ -416,7 +415,6 @@ __create_context(struct drm_i915_private *dev_priv) > list_add_tail(&ctx->link, &dev_priv->contexts.list); > ctx->i915 = dev_priv; > ctx->sched.priority = I915_USER_PRIORITY(I915_PRIORITY_NORMAL); > - INIT_LIST_HEAD(&ctx->active_engines); > mutex_init(&ctx->mutex); > > mutex_init(&ctx->engines_mutex); > diff --git a/drivers/gpu/drm/i915/i915_gem_context_types.h b/drivers/gpu/drm/i915/i915_gem_context_types.h > index 5d146b7b84ec..33e5a0e36e75 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context_types.h > +++ b/drivers/gpu/drm/i915/i915_gem_context_types.h > @@ -158,7 +158,6 @@ struct i915_gem_context { > atomic_t hw_id_pin_count; > struct list_head hw_id_link; > > - struct list_head active_engines; > struct mutex mutex; > > struct i915_sched_attr sched; > diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c > index 7fd224a4ca4c..deedd1898fe5 100644 > --- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c > @@ -1675,7 +1675,6 @@ static int mock_context_barrier(void *arg) > goto out; > } > i915_request_add(rq); > - GEM_BUG_ON(list_empty(&ctx->active_engines)); > > counter = 0; > context_barrier_inject_fault = BIT(RCS0); > diff --git a/drivers/gpu/drm/i915/selftests/mock_context.c b/drivers/gpu/drm/i915/selftests/mock_context.c > index 71c750693585..10e67c931ed1 100644 > --- a/drivers/gpu/drm/i915/selftests/mock_context.c > +++ b/drivers/gpu/drm/i915/selftests/mock_context.c > @@ -50,7 +50,6 @@ mock_context(struct drm_i915_private *i915, > INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL); > INIT_LIST_HEAD(&ctx->handles_list); > INIT_LIST_HEAD(&ctx->hw_id_link); > - INIT_LIST_HEAD(&ctx->active_engines); > mutex_init(&ctx->mutex); > > ret = i915_gem_context_pin_hw_id(ctx); >
Quoting Tvrtko Ursulin (2019-04-15 12:10:17) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > > index 58956b49f392..6c6bd50d87c9 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -34,6 +34,7 @@ > > > > #include "gt/intel_reset.h" > > > > +#include "i915_gem_context.h" > > #include "intel_dp.h" > > #include "intel_drv.h" > > #include "intel_fbc.h" > > @@ -396,9 +397,11 @@ static void print_context_stats(struct seq_file *m, > > struct i915_gem_context *ctx; > > > > list_for_each_entry(ctx, &i915->contexts.list, link) { > > - struct intel_context *ce; > > + struct i915_gem_engines_iter it; > > + > > + for_each_gem_engine(it, ctx) { > > Why no i915_gem_context_lock_engines in debugfs? Because this version of for_each_gem_engine() provided the locking. -Chris
On 15/04/2019 13:42, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-04-15 12:10:17) >>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >>> index 58956b49f392..6c6bd50d87c9 100644 >>> --- a/drivers/gpu/drm/i915/i915_debugfs.c >>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >>> @@ -34,6 +34,7 @@ >>> >>> #include "gt/intel_reset.h" >>> >>> +#include "i915_gem_context.h" >>> #include "intel_dp.h" >>> #include "intel_drv.h" >>> #include "intel_fbc.h" >>> @@ -396,9 +397,11 @@ static void print_context_stats(struct seq_file *m, >>> struct i915_gem_context *ctx; >>> >>> list_for_each_entry(ctx, &i915->contexts.list, link) { >>> - struct intel_context *ce; >>> + struct i915_gem_engines_iter it; >>> + >>> + for_each_gem_engine(it, ctx) { >> >> Why no i915_gem_context_lock_engines in debugfs? > > Because this version of for_each_gem_engine() provided the locking. Oopsie (me). Will you be sending the new series soon then? Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c index 5e506e648454..1f1761fc6597 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.c +++ b/drivers/gpu/drm/i915/gt/intel_context.c @@ -49,7 +49,6 @@ int __intel_context_do_pin(struct intel_context *ce) return -EINTR; if (likely(!atomic_read(&ce->pin_count))) { - struct i915_gem_context *ctx = ce->gem_context; intel_wakeref_t wakeref; err = 0; @@ -58,11 +57,7 @@ int __intel_context_do_pin(struct intel_context *ce) if (err) goto err; - i915_gem_context_get(ctx); - - mutex_lock(&ctx->mutex); - list_add(&ce->active_link, &ctx->active_engines); - mutex_unlock(&ctx->mutex); + i915_gem_context_get(ce->gem_context); /* for ctx->ppgtt */ intel_context_get(ce); smp_mb__before_atomic(); /* flush pin before it is visible */ @@ -91,10 +86,6 @@ void intel_context_unpin(struct intel_context *ce) if (likely(atomic_dec_and_test(&ce->pin_count))) { ce->ops->unpin(ce); - mutex_lock(&ce->gem_context->mutex); - list_del(&ce->active_link); - mutex_unlock(&ce->gem_context->mutex); - i915_gem_context_put(ce->gem_context); intel_context_put(ce); } diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index 3579c2708321..d5a7dbd0daee 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -38,7 +38,6 @@ struct intel_context { struct intel_engine_cs *engine; struct intel_engine_cs *active; - struct list_head active_link; struct list_head signal_link; struct list_head signals; diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 58956b49f392..6c6bd50d87c9 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -34,6 +34,7 @@ #include "gt/intel_reset.h" +#include "i915_gem_context.h" #include "intel_dp.h" #include "intel_drv.h" #include "intel_fbc.h" @@ -396,9 +397,11 @@ static void print_context_stats(struct seq_file *m, struct i915_gem_context *ctx; list_for_each_entry(ctx, &i915->contexts.list, link) { - struct intel_context *ce; + struct i915_gem_engines_iter it; + + for_each_gem_engine(it, ctx) { + struct intel_context *ce = it.context; - list_for_each_entry(ce, &ctx->active_engines, active_link) { if (ce->state) per_file_stats(0, ce->state->obj, &kstats); if (ce->ring) @@ -1893,7 +1896,7 @@ static int i915_context_status(struct seq_file *m, void *unused) return ret; list_for_each_entry(ctx, &dev_priv->contexts.list, link) { - struct intel_context *ce; + struct i915_gem_engines_iter it; seq_puts(m, "HW context "); if (!list_empty(&ctx->hw_id_link)) @@ -1917,7 +1920,9 @@ static int i915_context_status(struct seq_file *m, void *unused) seq_putc(m, ctx->remap_slice ? 'R' : 'r'); seq_putc(m, '\n'); - list_for_each_entry(ce, &ctx->active_engines, active_link) { + for_each_gem_engine(it, ctx) { + struct intel_context *ce = it.context; + seq_printf(m, "%s: ", ce->engine->name); if (ce->state) describe_obj(m, ce->state->obj); diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 99c89dc7533e..889cbc0554e5 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -289,7 +289,6 @@ static void i915_gem_context_free(struct i915_gem_context *ctx) { lockdep_assert_held(&ctx->i915->drm.struct_mutex); GEM_BUG_ON(!i915_gem_context_is_closed(ctx)); - GEM_BUG_ON(!list_empty(&ctx->active_engines)); release_hw_id(ctx); i915_ppgtt_put(ctx->ppgtt); @@ -416,7 +415,6 @@ __create_context(struct drm_i915_private *dev_priv) list_add_tail(&ctx->link, &dev_priv->contexts.list); ctx->i915 = dev_priv; ctx->sched.priority = I915_USER_PRIORITY(I915_PRIORITY_NORMAL); - INIT_LIST_HEAD(&ctx->active_engines); mutex_init(&ctx->mutex); mutex_init(&ctx->engines_mutex); diff --git a/drivers/gpu/drm/i915/i915_gem_context_types.h b/drivers/gpu/drm/i915/i915_gem_context_types.h index 5d146b7b84ec..33e5a0e36e75 100644 --- a/drivers/gpu/drm/i915/i915_gem_context_types.h +++ b/drivers/gpu/drm/i915/i915_gem_context_types.h @@ -158,7 +158,6 @@ struct i915_gem_context { atomic_t hw_id_pin_count; struct list_head hw_id_link; - struct list_head active_engines; struct mutex mutex; struct i915_sched_attr sched; diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c index 7fd224a4ca4c..deedd1898fe5 100644 --- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c @@ -1675,7 +1675,6 @@ static int mock_context_barrier(void *arg) goto out; } i915_request_add(rq); - GEM_BUG_ON(list_empty(&ctx->active_engines)); counter = 0; context_barrier_inject_fault = BIT(RCS0); diff --git a/drivers/gpu/drm/i915/selftests/mock_context.c b/drivers/gpu/drm/i915/selftests/mock_context.c index 71c750693585..10e67c931ed1 100644 --- a/drivers/gpu/drm/i915/selftests/mock_context.c +++ b/drivers/gpu/drm/i915/selftests/mock_context.c @@ -50,7 +50,6 @@ mock_context(struct drm_i915_private *i915, INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL); INIT_LIST_HEAD(&ctx->handles_list); INIT_LIST_HEAD(&ctx->hw_id_link); - INIT_LIST_HEAD(&ctx->active_engines); mutex_init(&ctx->mutex); ret = i915_gem_context_pin_hw_id(ctx);
We no longer need to track the active intel_contexts within each engine, allowing us to drop a tricky mutex_lock from inside unpin (which may occur inside fs_reclaim). Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/gt/intel_context.c | 11 +---------- drivers/gpu/drm/i915/gt/intel_context_types.h | 1 - drivers/gpu/drm/i915/i915_debugfs.c | 13 +++++++++---- drivers/gpu/drm/i915/i915_gem_context.c | 2 -- drivers/gpu/drm/i915/i915_gem_context_types.h | 1 - drivers/gpu/drm/i915/selftests/i915_gem_context.c | 1 - drivers/gpu/drm/i915/selftests/mock_context.c | 1 - 7 files changed, 10 insertions(+), 20 deletions(-)