| Message ID | 20190318095204.9913-6-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/22] drm/i915: Flush pages on acquisition | expand |
On 18/03/2019 09:51, Chris Wilson wrote: > For virtual engines, we need to keep the HW context alive while it > remains in use. For regular HW contexts, they are created and kept alive > until the end of the GEM context. For simplicity, generalise the > requirements and keep an active reference to each HW context. Is there a functional effect from this patch? Later with veng added? Regards, Tvrtko > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 2 +- > drivers/gpu/drm/i915/intel_context.c | 6 ++++++ > drivers/gpu/drm/i915/intel_context.h | 11 +++++++++++ > drivers/gpu/drm/i915/intel_context_types.h | 6 +++++- > drivers/gpu/drm/i915/intel_lrc.c | 4 +++- > drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +++- > drivers/gpu/drm/i915/selftests/mock_engine.c | 7 ++++++- > 7 files changed, 35 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 21208a865380..d776d43707e0 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -232,7 +232,7 @@ static void i915_gem_context_free(struct i915_gem_context *ctx) > i915_ppgtt_put(ctx->ppgtt); > > rbtree_postorder_for_each_entry_safe(it, n, &ctx->hw_contexts, node) > - it->ops->destroy(it); > + intel_context_put(it); > > kfree(ctx->name); > put_pid(ctx->pid); > diff --git a/drivers/gpu/drm/i915/intel_context.c b/drivers/gpu/drm/i915/intel_context.c > index 0ab894a058f6..8931e0fee873 100644 > --- a/drivers/gpu/drm/i915/intel_context.c > +++ b/drivers/gpu/drm/i915/intel_context.c > @@ -172,6 +172,7 @@ intel_context_pin(struct i915_gem_context *ctx, > list_add(&ce->active_link, &ctx->active_engines); > mutex_unlock(&ctx->mutex); > > + intel_context_get(ce); > smp_mb__before_atomic(); /* flush pin before it is visible */ > } > > @@ -192,6 +193,7 @@ void intel_context_unpin(struct intel_context *ce) > return; > > /* We may be called from inside intel_context_pin() to evict another */ > + intel_context_get(ce); > mutex_lock_nested(&ce->pin_mutex, SINGLE_DEPTH_NESTING); > > if (likely(atomic_dec_and_test(&ce->pin_count))) { > @@ -202,9 +204,11 @@ void intel_context_unpin(struct intel_context *ce) > mutex_unlock(&ce->gem_context->mutex); > > i915_gem_context_put(ce->gem_context); > + intel_context_put(ce); > } > > mutex_unlock(&ce->pin_mutex); > + intel_context_put(ce); > } > > static void intel_context_retire(struct i915_active_request *active, > @@ -221,6 +225,8 @@ intel_context_init(struct intel_context *ce, > struct i915_gem_context *ctx, > struct intel_engine_cs *engine) > { > + kref_init(&ce->ref); > + > ce->gem_context = ctx; > ce->engine = engine; > ce->ops = engine->cops; > diff --git a/drivers/gpu/drm/i915/intel_context.h b/drivers/gpu/drm/i915/intel_context.h > index 9546d932406a..ebc861b1a49e 100644 > --- a/drivers/gpu/drm/i915/intel_context.h > +++ b/drivers/gpu/drm/i915/intel_context.h > @@ -73,4 +73,15 @@ static inline void __intel_context_pin(struct intel_context *ce) > > void intel_context_unpin(struct intel_context *ce); > > +static inline struct intel_context *intel_context_get(struct intel_context *ce) > +{ > + kref_get(&ce->ref); > + return ce; > +} > + > +static inline void intel_context_put(struct intel_context *ce) > +{ > + kref_put(&ce->ref, ce->ops->destroy); > +} > + > #endif /* __INTEL_CONTEXT_H__ */ > diff --git a/drivers/gpu/drm/i915/intel_context_types.h b/drivers/gpu/drm/i915/intel_context_types.h > index 6dc9b4b9067b..624729a35875 100644 > --- a/drivers/gpu/drm/i915/intel_context_types.h > +++ b/drivers/gpu/drm/i915/intel_context_types.h > @@ -7,6 +7,7 @@ > #ifndef __INTEL_CONTEXT_TYPES__ > #define __INTEL_CONTEXT_TYPES__ > > +#include <linux/kref.h> > #include <linux/list.h> > #include <linux/mutex.h> > #include <linux/rbtree.h> > @@ -22,7 +23,8 @@ struct intel_ring; > struct intel_context_ops { > int (*pin)(struct intel_context *ce); > void (*unpin)(struct intel_context *ce); > - void (*destroy)(struct intel_context *ce); > + > + void (*destroy)(struct kref *kref); > }; > > /* > @@ -36,6 +38,8 @@ struct intel_sseu { > }; > > struct intel_context { > + struct kref ref; > + > struct i915_gem_context *gem_context; > struct intel_engine_cs *engine; > struct intel_engine_cs *active; > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 13f5545fc1d2..fbf67105f040 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1242,8 +1242,10 @@ static void __execlists_context_fini(struct intel_context *ce) > i915_gem_object_put(ce->state->obj); > } > > -static void execlists_context_destroy(struct intel_context *ce) > +static void execlists_context_destroy(struct kref *kref) > { > + struct intel_context *ce = container_of(kref, typeof(*ce), ref); > + > GEM_BUG_ON(intel_context_is_pinned(ce)); > > if (ce->state) > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 6d60bc258feb..35fdebd67e5f 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -1345,8 +1345,10 @@ static void __ring_context_fini(struct intel_context *ce) > i915_gem_object_put(ce->state->obj); > } > > -static void ring_context_destroy(struct intel_context *ce) > +static void ring_context_destroy(struct kref *ref) > { > + struct intel_context *ce = container_of(ref, typeof(*ce), ref); > + > GEM_BUG_ON(intel_context_is_pinned(ce)); > > if (ce->state) > diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c > index 7641b74ada98..639d36eb904a 100644 > --- a/drivers/gpu/drm/i915/selftests/mock_engine.c > +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c > @@ -128,12 +128,16 @@ static void mock_context_unpin(struct intel_context *ce) > mock_timeline_unpin(ce->ring->timeline); > } > > -static void mock_context_destroy(struct intel_context *ce) > +static void mock_context_destroy(struct kref *ref) > { > + struct intel_context *ce = container_of(ref, typeof(*ce), ref); > + > GEM_BUG_ON(intel_context_is_pinned(ce)); > > if (ce->ring) > mock_ring_free(ce->ring); > + > + intel_context_free(ce); > } > > static int mock_context_pin(struct intel_context *ce) > @@ -151,6 +155,7 @@ static int mock_context_pin(struct intel_context *ce) > static const struct intel_context_ops mock_context_ops = { > .pin = mock_context_pin, > .unpin = mock_context_unpin, > + > .destroy = mock_context_destroy, > }; > >
Quoting Tvrtko Ursulin (2019-03-18 12:54:00) > > On 18/03/2019 09:51, Chris Wilson wrote: > > For virtual engines, we need to keep the HW context alive while it > > remains in use. For regular HW contexts, they are created and kept alive > > until the end of the GEM context. For simplicity, generalise the > > requirements and keep an active reference to each HW context. > > Is there a functional effect from this patch? Later with veng added? If by functional do you mean prevents the code from eating itself on use-after-free after the engines are freed, then yes. -Chris
Quoting Chris Wilson (2019-03-18 12:56:12) > Quoting Tvrtko Ursulin (2019-03-18 12:54:00) > > > > On 18/03/2019 09:51, Chris Wilson wrote: > > > For virtual engines, we need to keep the HW context alive while it > > > remains in use. For regular HW contexts, they are created and kept alive > > > until the end of the GEM context. For simplicity, generalise the > > > requirements and keep an active reference to each HW context. > > > > Is there a functional effect from this patch? Later with veng added? > > If by functional do you mean prevents the code from eating itself on > use-after-free after the engines are freed, then yes. A variation of this used to be inside the veng patch, but that only applied itself to veng. After the discussion there, I felt it would be more obvious if it was applied as a standalone patch by generalising the requirements to all HW context. -Chris
On 18/03/2019 12:57, Chris Wilson wrote: > Quoting Chris Wilson (2019-03-18 12:56:12) >> Quoting Tvrtko Ursulin (2019-03-18 12:54:00) >>> >>> On 18/03/2019 09:51, Chris Wilson wrote: >>>> For virtual engines, we need to keep the HW context alive while it >>>> remains in use. For regular HW contexts, they are created and kept alive >>>> until the end of the GEM context. For simplicity, generalise the >>>> requirements and keep an active reference to each HW context. >>> >>> Is there a functional effect from this patch? Later with veng added? >> >> If by functional do you mean prevents the code from eating itself on >> use-after-free after the engines are freed, then yes. > > A variation of this used to be inside the veng patch, but that only > applied itself to veng. After the discussion there, I felt it would be > more obvious if it was applied as a standalone patch by generalising the > requirements to all HW context. Yep. Guess my previous review was too sloppy. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 21208a865380..d776d43707e0 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -232,7 +232,7 @@ static void i915_gem_context_free(struct i915_gem_context *ctx) i915_ppgtt_put(ctx->ppgtt); rbtree_postorder_for_each_entry_safe(it, n, &ctx->hw_contexts, node) - it->ops->destroy(it); + intel_context_put(it); kfree(ctx->name); put_pid(ctx->pid); diff --git a/drivers/gpu/drm/i915/intel_context.c b/drivers/gpu/drm/i915/intel_context.c index 0ab894a058f6..8931e0fee873 100644 --- a/drivers/gpu/drm/i915/intel_context.c +++ b/drivers/gpu/drm/i915/intel_context.c @@ -172,6 +172,7 @@ intel_context_pin(struct i915_gem_context *ctx, list_add(&ce->active_link, &ctx->active_engines); mutex_unlock(&ctx->mutex); + intel_context_get(ce); smp_mb__before_atomic(); /* flush pin before it is visible */ } @@ -192,6 +193,7 @@ void intel_context_unpin(struct intel_context *ce) return; /* We may be called from inside intel_context_pin() to evict another */ + intel_context_get(ce); mutex_lock_nested(&ce->pin_mutex, SINGLE_DEPTH_NESTING); if (likely(atomic_dec_and_test(&ce->pin_count))) { @@ -202,9 +204,11 @@ void intel_context_unpin(struct intel_context *ce) mutex_unlock(&ce->gem_context->mutex); i915_gem_context_put(ce->gem_context); + intel_context_put(ce); } mutex_unlock(&ce->pin_mutex); + intel_context_put(ce); } static void intel_context_retire(struct i915_active_request *active, @@ -221,6 +225,8 @@ intel_context_init(struct intel_context *ce, struct i915_gem_context *ctx, struct intel_engine_cs *engine) { + kref_init(&ce->ref); + ce->gem_context = ctx; ce->engine = engine; ce->ops = engine->cops; diff --git a/drivers/gpu/drm/i915/intel_context.h b/drivers/gpu/drm/i915/intel_context.h index 9546d932406a..ebc861b1a49e 100644 --- a/drivers/gpu/drm/i915/intel_context.h +++ b/drivers/gpu/drm/i915/intel_context.h @@ -73,4 +73,15 @@ static inline void __intel_context_pin(struct intel_context *ce) void intel_context_unpin(struct intel_context *ce); +static inline struct intel_context *intel_context_get(struct intel_context *ce) +{ + kref_get(&ce->ref); + return ce; +} + +static inline void intel_context_put(struct intel_context *ce) +{ + kref_put(&ce->ref, ce->ops->destroy); +} + #endif /* __INTEL_CONTEXT_H__ */ diff --git a/drivers/gpu/drm/i915/intel_context_types.h b/drivers/gpu/drm/i915/intel_context_types.h index 6dc9b4b9067b..624729a35875 100644 --- a/drivers/gpu/drm/i915/intel_context_types.h +++ b/drivers/gpu/drm/i915/intel_context_types.h @@ -7,6 +7,7 @@ #ifndef __INTEL_CONTEXT_TYPES__ #define __INTEL_CONTEXT_TYPES__ +#include <linux/kref.h> #include <linux/list.h> #include <linux/mutex.h> #include <linux/rbtree.h> @@ -22,7 +23,8 @@ struct intel_ring; struct intel_context_ops { int (*pin)(struct intel_context *ce); void (*unpin)(struct intel_context *ce); - void (*destroy)(struct intel_context *ce); + + void (*destroy)(struct kref *kref); }; /* @@ -36,6 +38,8 @@ struct intel_sseu { }; struct intel_context { + struct kref ref; + struct i915_gem_context *gem_context; struct intel_engine_cs *engine; struct intel_engine_cs *active; diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 13f5545fc1d2..fbf67105f040 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1242,8 +1242,10 @@ static void __execlists_context_fini(struct intel_context *ce) i915_gem_object_put(ce->state->obj); } -static void execlists_context_destroy(struct intel_context *ce) +static void execlists_context_destroy(struct kref *kref) { + struct intel_context *ce = container_of(kref, typeof(*ce), ref); + GEM_BUG_ON(intel_context_is_pinned(ce)); if (ce->state) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 6d60bc258feb..35fdebd67e5f 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1345,8 +1345,10 @@ static void __ring_context_fini(struct intel_context *ce) i915_gem_object_put(ce->state->obj); } -static void ring_context_destroy(struct intel_context *ce) +static void ring_context_destroy(struct kref *ref) { + struct intel_context *ce = container_of(ref, typeof(*ce), ref); + GEM_BUG_ON(intel_context_is_pinned(ce)); if (ce->state) diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c index 7641b74ada98..639d36eb904a 100644 --- a/drivers/gpu/drm/i915/selftests/mock_engine.c +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c @@ -128,12 +128,16 @@ static void mock_context_unpin(struct intel_context *ce) mock_timeline_unpin(ce->ring->timeline); } -static void mock_context_destroy(struct intel_context *ce) +static void mock_context_destroy(struct kref *ref) { + struct intel_context *ce = container_of(ref, typeof(*ce), ref); + GEM_BUG_ON(intel_context_is_pinned(ce)); if (ce->ring) mock_ring_free(ce->ring); + + intel_context_free(ce); } static int mock_context_pin(struct intel_context *ce) @@ -151,6 +155,7 @@ static int mock_context_pin(struct intel_context *ce) static const struct intel_context_ops mock_context_ops = { .pin = mock_context_pin, .unpin = mock_context_unpin, + .destroy = mock_context_destroy, };
For virtual engines, we need to keep the HW context alive while it remains in use. For regular HW contexts, they are created and kept alive until the end of the GEM context. For simplicity, generalise the requirements and keep an active reference to each HW context. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem_context.c | 2 +- drivers/gpu/drm/i915/intel_context.c | 6 ++++++ drivers/gpu/drm/i915/intel_context.h | 11 +++++++++++ drivers/gpu/drm/i915/intel_context_types.h | 6 +++++- drivers/gpu/drm/i915/intel_lrc.c | 4 +++- drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +++- drivers/gpu/drm/i915/selftests/mock_engine.c | 7 ++++++- 7 files changed, 35 insertions(+), 5 deletions(-)