| Message ID | 20190318095204.9913-5-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: > On unpinning the intel_context, we remove it from the active list > inside the GEM context. This list is supposed to be guarded by the GEM > context mutex, so remember to take it! Fixes: ? > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_context.c | 15 +++++++++++---- > drivers/gpu/drm/i915/intel_lrc.c | 3 --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 3 --- > drivers/gpu/drm/i915/selftests/mock_engine.c | 2 -- > 4 files changed, 11 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_context.c b/drivers/gpu/drm/i915/intel_context.c > index 5a16c9bb2778..0ab894a058f6 100644 > --- a/drivers/gpu/drm/i915/intel_context.c > +++ b/drivers/gpu/drm/i915/intel_context.c > @@ -165,13 +165,13 @@ intel_context_pin(struct i915_gem_context *ctx, > if (err) > goto err; > > + i915_gem_context_get(ctx); > + GEM_BUG_ON(ce->gem_context != ctx); > + > mutex_lock(&ctx->mutex); > list_add(&ce->active_link, &ctx->active_engines); > mutex_unlock(&ctx->mutex); > > - i915_gem_context_get(ctx); > - GEM_BUG_ON(ce->gem_context != ctx); > - > smp_mb__before_atomic(); /* flush pin before it is visible */ > } > > @@ -194,9 +194,16 @@ void intel_context_unpin(struct intel_context *ce) > /* We may be called from inside intel_context_pin() to evict another */ > mutex_lock_nested(&ce->pin_mutex, SINGLE_DEPTH_NESTING); Hm is the nested annotation and comment correct? Looking at it now, the allocations happen outside pin_mutex. Regards, Tvrtko > > - if (likely(atomic_dec_and_test(&ce->pin_count))) > + 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); > + } > + > mutex_unlock(&ce->pin_mutex); > } > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index d3f1fe06d013..13f5545fc1d2 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1306,9 +1306,6 @@ static void execlists_context_unpin(struct intel_context *ce) > > i915_gem_object_unpin_map(ce->state->obj); > __context_unpin(ce->state); > - > - list_del(&ce->active_link); > - i915_gem_context_put(ce->gem_context); > } > > static void > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 45a54fadc482..6d60bc258feb 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -1415,9 +1415,6 @@ static void ring_context_unpin(struct intel_context *ce) > { > __context_unpin_ppgtt(ce->gem_context); > __context_unpin(ce); > - > - list_del(&ce->active_link); > - i915_gem_context_put(ce->gem_context); > } > > static struct i915_vma * > diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c > index 881450c694e9..7641b74ada98 100644 > --- a/drivers/gpu/drm/i915/selftests/mock_engine.c > +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c > @@ -126,8 +126,6 @@ static void hw_delay_complete(struct timer_list *t) > static void mock_context_unpin(struct intel_context *ce) > { > mock_timeline_unpin(ce->ring->timeline); > - list_del(&ce->active_link); > - i915_gem_context_put(ce->gem_context); > } > > static void mock_context_destroy(struct intel_context *ce) >
Quoting Tvrtko Ursulin (2019-03-18 10:39:44) > > On 18/03/2019 09:51, Chris Wilson wrote: > > On unpinning the intel_context, we remove it from the active list > > inside the GEM context. This list is supposed to be guarded by the GEM > > context mutex, so remember to take it! > > Fixes: ? It is not broken yet :) > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/intel_context.c | 15 +++++++++++---- > > drivers/gpu/drm/i915/intel_lrc.c | 3 --- > > drivers/gpu/drm/i915/intel_ringbuffer.c | 3 --- > > drivers/gpu/drm/i915/selftests/mock_engine.c | 2 -- > > 4 files changed, 11 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_context.c b/drivers/gpu/drm/i915/intel_context.c > > index 5a16c9bb2778..0ab894a058f6 100644 > > --- a/drivers/gpu/drm/i915/intel_context.c > > +++ b/drivers/gpu/drm/i915/intel_context.c > > @@ -165,13 +165,13 @@ intel_context_pin(struct i915_gem_context *ctx, > > if (err) > > goto err; > > > > + i915_gem_context_get(ctx); > > + GEM_BUG_ON(ce->gem_context != ctx); > > + > > mutex_lock(&ctx->mutex); > > list_add(&ce->active_link, &ctx->active_engines); > > mutex_unlock(&ctx->mutex); > > > > - i915_gem_context_get(ctx); > > - GEM_BUG_ON(ce->gem_context != ctx); > > - > > smp_mb__before_atomic(); /* flush pin before it is visible */ > > } > > > > @@ -194,9 +194,16 @@ void intel_context_unpin(struct intel_context *ce) > > /* We may be called from inside intel_context_pin() to evict another */ > > mutex_lock_nested(&ce->pin_mutex, SINGLE_DEPTH_NESTING); > > Hm is the nested annotation and comment correct? Yes. pin -> vma bind -> evict -> retire requests -> unpin someone else. -Chris
On 18/03/2019 10:45, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-03-18 10:39:44) >> >> On 18/03/2019 09:51, Chris Wilson wrote: >>> On unpinning the intel_context, we remove it from the active list >>> inside the GEM context. This list is supposed to be guarded by the GEM >>> context mutex, so remember to take it! >> >> Fixes: ? > > It is not broken yet :) > >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> --- >>> drivers/gpu/drm/i915/intel_context.c | 15 +++++++++++---- >>> drivers/gpu/drm/i915/intel_lrc.c | 3 --- >>> drivers/gpu/drm/i915/intel_ringbuffer.c | 3 --- >>> drivers/gpu/drm/i915/selftests/mock_engine.c | 2 -- >>> 4 files changed, 11 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_context.c b/drivers/gpu/drm/i915/intel_context.c >>> index 5a16c9bb2778..0ab894a058f6 100644 >>> --- a/drivers/gpu/drm/i915/intel_context.c >>> +++ b/drivers/gpu/drm/i915/intel_context.c >>> @@ -165,13 +165,13 @@ intel_context_pin(struct i915_gem_context *ctx, >>> if (err) >>> goto err; >>> >>> + i915_gem_context_get(ctx); >>> + GEM_BUG_ON(ce->gem_context != ctx); >>> + >>> mutex_lock(&ctx->mutex); >>> list_add(&ce->active_link, &ctx->active_engines); >>> mutex_unlock(&ctx->mutex); >>> >>> - i915_gem_context_get(ctx); >>> - GEM_BUG_ON(ce->gem_context != ctx); >>> - >>> smp_mb__before_atomic(); /* flush pin before it is visible */ >>> } >>> >>> @@ -194,9 +194,16 @@ void intel_context_unpin(struct intel_context *ce) >>> /* We may be called from inside intel_context_pin() to evict another */ >>> mutex_lock_nested(&ce->pin_mutex, SINGLE_DEPTH_NESTING); >> >> Hm is the nested annotation and comment correct? > > Yes. pin -> vma bind -> evict -> retire requests -> unpin someone else. Yep, the ops->pin.. I missed it. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
Quoting Chris Wilson (2019-03-18 09:51:47) > On unpinning the intel_context, we remove it from the active list > inside the GEM context. This list is supposed to be guarded by the GEM > context mutex, so remember to take it! > Fixes: 7e3d9a59410d ("drm/i915: Track active engines within a context") -Chris
diff --git a/drivers/gpu/drm/i915/intel_context.c b/drivers/gpu/drm/i915/intel_context.c index 5a16c9bb2778..0ab894a058f6 100644 --- a/drivers/gpu/drm/i915/intel_context.c +++ b/drivers/gpu/drm/i915/intel_context.c @@ -165,13 +165,13 @@ intel_context_pin(struct i915_gem_context *ctx, if (err) goto err; + i915_gem_context_get(ctx); + GEM_BUG_ON(ce->gem_context != ctx); + mutex_lock(&ctx->mutex); list_add(&ce->active_link, &ctx->active_engines); mutex_unlock(&ctx->mutex); - i915_gem_context_get(ctx); - GEM_BUG_ON(ce->gem_context != ctx); - smp_mb__before_atomic(); /* flush pin before it is visible */ } @@ -194,9 +194,16 @@ void intel_context_unpin(struct intel_context *ce) /* We may be called from inside intel_context_pin() to evict another */ mutex_lock_nested(&ce->pin_mutex, SINGLE_DEPTH_NESTING); - if (likely(atomic_dec_and_test(&ce->pin_count))) + 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); + } + mutex_unlock(&ce->pin_mutex); } diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index d3f1fe06d013..13f5545fc1d2 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1306,9 +1306,6 @@ static void execlists_context_unpin(struct intel_context *ce) i915_gem_object_unpin_map(ce->state->obj); __context_unpin(ce->state); - - list_del(&ce->active_link); - i915_gem_context_put(ce->gem_context); } static void diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 45a54fadc482..6d60bc258feb 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1415,9 +1415,6 @@ static void ring_context_unpin(struct intel_context *ce) { __context_unpin_ppgtt(ce->gem_context); __context_unpin(ce); - - list_del(&ce->active_link); - i915_gem_context_put(ce->gem_context); } static struct i915_vma * diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c index 881450c694e9..7641b74ada98 100644 --- a/drivers/gpu/drm/i915/selftests/mock_engine.c +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c @@ -126,8 +126,6 @@ static void hw_delay_complete(struct timer_list *t) static void mock_context_unpin(struct intel_context *ce) { mock_timeline_unpin(ce->ring->timeline); - list_del(&ce->active_link); - i915_gem_context_put(ce->gem_context); } static void mock_context_destroy(struct intel_context *ce)
On unpinning the intel_context, we remove it from the active list inside the GEM context. This list is supposed to be guarded by the GEM context mutex, so remember to take it! Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_context.c | 15 +++++++++++---- drivers/gpu/drm/i915/intel_lrc.c | 3 --- drivers/gpu/drm/i915/intel_ringbuffer.c | 3 --- drivers/gpu/drm/i915/selftests/mock_engine.c | 2 -- 4 files changed, 11 insertions(+), 12 deletions(-)