[02/16] drm/i915: Drop assertion that ce->pin_mutex guards state updates
diff mbox series

Message ID 20191021080226.537-2-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [01/16] drm/i915: Don't set queue_priority_hint if we don't kick the submission
Related show

Commit Message

Chris Wilson Oct. 21, 2019, 8:02 a.m. UTC
The actual conditions are that we know the GPU is not accessing the
context, and we hold a pin on the context image to allow CPU access. We
used a fake lock on ce->pin_mutex so that we could try and use lockdep
to assert that access is serialised, but the various different
hardirq/softirq contexts where we need to *fake* holding the pin_mutex
are causing more trouble.

Still it would be nice if we did have a way to reassure ourselves that
the direct update to the context image is serialised with GPU execution.
In the meantime, stop lockdep complaining about false irq inversions.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111923
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c    | 16 ----------------
 drivers/gpu/drm/i915/gt/selftest_lrc.c |  5 -----
 drivers/gpu/drm/i915/i915_perf.c       |  1 -
 3 files changed, 22 deletions(-)

Comments

Mika Kuoppala Oct. 22, 2019, 12:25 p.m. UTC | #1
Chris Wilson <chris@chris-wilson.co.uk> writes:

> The actual conditions are that we know the GPU is not accessing the
> context, and we hold a pin on the context image to allow CPU access. We
> used a fake lock on ce->pin_mutex so that we could try and use lockdep
> to assert that access is serialised, but the various different
> hardirq/softirq contexts where we need to *fake* holding the pin_mutex
> are causing more trouble.
>
> Still it would be nice if we did have a way to reassure ourselves that
> the direct update to the context image is serialised with GPU execution.
> In the meantime, stop lockdep complaining about false irq inversions.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111923
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

A tad sad that the price/payout didn't provide.

Acked-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/gt/intel_lrc.c    | 16 ----------------
>  drivers/gpu/drm/i915/gt/selftest_lrc.c |  5 -----
>  drivers/gpu/drm/i915/i915_perf.c       |  1 -
>  3 files changed, 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index d0088d020220..f9f3e985bb79 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -235,16 +235,6 @@ static void execlists_init_reg_state(u32 *reg_state,
>  				     const struct intel_ring *ring,
>  				     bool close);
>  
> -static void __context_pin_acquire(struct intel_context *ce)
> -{
> -	mutex_acquire(&ce->pin_mutex.dep_map, 2, 0, _RET_IP_);
> -}
> -
> -static void __context_pin_release(struct intel_context *ce)
> -{
> -	mutex_release(&ce->pin_mutex.dep_map, 0, _RET_IP_);
> -}
> -
>  static void mark_eio(struct i915_request *rq)
>  {
>  	if (i915_request_completed(rq))
> @@ -2792,9 +2782,6 @@ static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
>  	ce = rq->hw_context;
>  	GEM_BUG_ON(!i915_vma_is_pinned(ce->state));
>  
> -	/* Proclaim we have exclusive access to the context image! */
> -	__context_pin_acquire(ce);
> -
>  	rq = active_request(rq);
>  	if (!rq) {
>  		/* Idle context; tidy up the ring so we can restart afresh */
> @@ -2860,7 +2847,6 @@ static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
>  	__execlists_reset_reg_state(ce, engine);
>  	__execlists_update_reg_state(ce, engine);
>  	ce->lrc_desc |= CTX_DESC_FORCE_RESTORE; /* paranoid: GPU was reset! */
> -	__context_pin_release(ce);
>  
>  unwind:
>  	/* Push back any incomplete requests for replay after the reset. */
> @@ -4502,7 +4488,6 @@ void intel_lr_context_reset(struct intel_engine_cs *engine,
>  			    bool scrub)
>  {
>  	GEM_BUG_ON(!intel_context_is_pinned(ce));
> -	__context_pin_acquire(ce);
>  
>  	/*
>  	 * We want a simple context + ring to execute the breadcrumb update.
> @@ -4528,7 +4513,6 @@ void intel_lr_context_reset(struct intel_engine_cs *engine,
>  	intel_ring_update_space(ce->ring);
>  
>  	__execlists_update_reg_state(ce, engine);
> -	__context_pin_release(ce);
>  }
>  
>  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> index 5dc679781a08..7516d1c90925 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> @@ -168,12 +168,7 @@ static int live_unlite_restore(struct intel_gt *gt, int prio)
>  		}
>  		GEM_BUG_ON(!ce[1]->ring->size);
>  		intel_ring_reset(ce[1]->ring, ce[1]->ring->size / 2);
> -
> -		local_irq_disable(); /* appease lockdep */
> -		__context_pin_acquire(ce[1]);
>  		__execlists_update_reg_state(ce[1], engine);
> -		__context_pin_release(ce[1]);
> -		local_irq_enable();
>  
>  		rq[0] = igt_spinner_create_request(&spin, ce[0], MI_ARB_CHECK);
>  		if (IS_ERR(rq[0])) {
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index d2ac51fe4f04..3130b0c7ed83 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -2615,7 +2615,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
>  	struct i915_perf_stream *stream;
>  
>  	/* perf.exclusive_stream serialised by gen8_configure_all_contexts() */
> -	lockdep_assert_held(&ce->pin_mutex);
>  
>  	if (engine->class != RENDER_CLASS)
>  		return;
> -- 
> 2.24.0.rc0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index d0088d020220..f9f3e985bb79 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -235,16 +235,6 @@  static void execlists_init_reg_state(u32 *reg_state,
 				     const struct intel_ring *ring,
 				     bool close);
 
-static void __context_pin_acquire(struct intel_context *ce)
-{
-	mutex_acquire(&ce->pin_mutex.dep_map, 2, 0, _RET_IP_);
-}
-
-static void __context_pin_release(struct intel_context *ce)
-{
-	mutex_release(&ce->pin_mutex.dep_map, 0, _RET_IP_);
-}
-
 static void mark_eio(struct i915_request *rq)
 {
 	if (i915_request_completed(rq))
@@ -2792,9 +2782,6 @@  static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
 	ce = rq->hw_context;
 	GEM_BUG_ON(!i915_vma_is_pinned(ce->state));
 
-	/* Proclaim we have exclusive access to the context image! */
-	__context_pin_acquire(ce);
-
 	rq = active_request(rq);
 	if (!rq) {
 		/* Idle context; tidy up the ring so we can restart afresh */
@@ -2860,7 +2847,6 @@  static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
 	__execlists_reset_reg_state(ce, engine);
 	__execlists_update_reg_state(ce, engine);
 	ce->lrc_desc |= CTX_DESC_FORCE_RESTORE; /* paranoid: GPU was reset! */
-	__context_pin_release(ce);
 
 unwind:
 	/* Push back any incomplete requests for replay after the reset. */
@@ -4502,7 +4488,6 @@  void intel_lr_context_reset(struct intel_engine_cs *engine,
 			    bool scrub)
 {
 	GEM_BUG_ON(!intel_context_is_pinned(ce));
-	__context_pin_acquire(ce);
 
 	/*
 	 * We want a simple context + ring to execute the breadcrumb update.
@@ -4528,7 +4513,6 @@  void intel_lr_context_reset(struct intel_engine_cs *engine,
 	intel_ring_update_space(ce->ring);
 
 	__execlists_update_reg_state(ce, engine);
-	__context_pin_release(ce);
 }
 
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
index 5dc679781a08..7516d1c90925 100644
--- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
@@ -168,12 +168,7 @@  static int live_unlite_restore(struct intel_gt *gt, int prio)
 		}
 		GEM_BUG_ON(!ce[1]->ring->size);
 		intel_ring_reset(ce[1]->ring, ce[1]->ring->size / 2);
-
-		local_irq_disable(); /* appease lockdep */
-		__context_pin_acquire(ce[1]);
 		__execlists_update_reg_state(ce[1], engine);
-		__context_pin_release(ce[1]);
-		local_irq_enable();
 
 		rq[0] = igt_spinner_create_request(&spin, ce[0], MI_ARB_CHECK);
 		if (IS_ERR(rq[0])) {
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index d2ac51fe4f04..3130b0c7ed83 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2615,7 +2615,6 @@  void i915_oa_init_reg_state(const struct intel_context *ce,
 	struct i915_perf_stream *stream;
 
 	/* perf.exclusive_stream serialised by gen8_configure_all_contexts() */
-	lockdep_assert_held(&ce->pin_mutex);
 
 	if (engine->class != RENDER_CLASS)
 		return;