Commit 6dcb85a0 authored by Chris Wilson's avatar Chris Wilson
Browse files

drm/i915: Hold irq-off for the entire fake lock period 



Sadly lockdep records when the irqs are re-enabled and then marks up the
fake lock as being irq-unsafe. Our hand is forced and so we must mark up
the entire fake lock critical section as irq-off.

Hopefully this is the last tweak required!

v2: Not quite, we need to mark the timeline spinlock as irqsafe. That
was a genuine bug being hidden by the earlier lockdep splat.

Fixes: d6773926 ("drm/i915/gt: Mark up the nested engine-pm timeline lock as irqsafe")
Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: default avatarMika Kuoppala <mika.kuoppala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190823132700.25286-2-chris@chris-wilson.co.uk
parent 0b14d968

drivers/gpu/drm/i915/gt/intel_engine_pm.c

+18 −10
Original line number Original line Diff line number Diff line
@@ -39,27 +39,32 @@ static int __engine_unpark(struct intel_wakeref *wf) 




#if IS_ENABLED(CONFIG_LOCKDEP)

#if IS_ENABLED(CONFIG_LOCKDEP)





static inline void __timeline_mark_lock(struct intel_context *ce)

static inline unsigned long __timeline_mark_lock(struct intel_context *ce)

{

{

	unsigned long flags;

	unsigned long flags;





	local_irq_save(flags);

	local_irq_save(flags);

	mutex_acquire(&ce->timeline->mutex.dep_map, 2, 0, _THIS_IP_);

	mutex_acquire(&ce->timeline->mutex.dep_map, 2, 0, _THIS_IP_);

	local_irq_restore(flags);



	return flags;

}

}





static inline void __timeline_mark_unlock(struct intel_context *ce)

static inline void __timeline_mark_unlock(struct intel_context *ce,

					  unsigned long flags)

{

{

	mutex_release(&ce->timeline->mutex.dep_map, 0, _THIS_IP_);

	mutex_release(&ce->timeline->mutex.dep_map, 0, _THIS_IP_);

	local_irq_restore(flags);

}

}





#else

#else





static inline void __timeline_mark_lock(struct intel_context *ce)

static inline unsigned long __timeline_mark_lock(struct intel_context *ce)

{

{

	return 0;

}

}





static inline void __timeline_mark_unlock(struct intel_context *ce)

static inline void __timeline_mark_unlock(struct intel_context *ce,

					  unsigned long flags)

{

{

}

}
@@ -68,6 +73,8 @@ static inline void __timeline_mark_unlock(struct intel_context *ce) 
static bool switch_to_kernel_context(struct intel_engine_cs *engine)

static bool switch_to_kernel_context(struct intel_engine_cs *engine)

{

{

	struct i915_request *rq;

	struct i915_request *rq;

	unsigned long flags;

	bool result = true;





	/* Already inside the kernel context, safe to power down. */

	/* Already inside the kernel context, safe to power down. */

	if (engine->wakeref_serial == engine->serial)

	if (engine->wakeref_serial == engine->serial)
@@ -89,12 +96,12 @@ static bool switch_to_kernel_context(struct intel_engine_cs *engine) 
	 * retiring the last request, thus all rings should be empty and

	 * retiring the last request, thus all rings should be empty and

	 * all timelines idle.

	 * all timelines idle.

	 */

	 */

	__timeline_mark_lock(engine->kernel_context);

	flags = __timeline_mark_lock(engine->kernel_context);





	rq = __i915_request_create(engine->kernel_context, GFP_NOWAIT);

	rq = __i915_request_create(engine->kernel_context, GFP_NOWAIT);

	if (IS_ERR(rq))

	if (IS_ERR(rq))

		/* Context switch failed, hope for the best! Maybe reset? */

		/* Context switch failed, hope for the best! Maybe reset? */

		return true;

		goto out_unlock;





	intel_timeline_enter(rq->timeline);

	intel_timeline_enter(rq->timeline);
@@ -110,9 +117,10 @@ static bool switch_to_kernel_context(struct intel_engine_cs *engine) 
	__intel_wakeref_defer_park(&engine->wakeref);

	__intel_wakeref_defer_park(&engine->wakeref);

	__i915_request_queue(rq, NULL);

	__i915_request_queue(rq, NULL);





	__timeline_mark_unlock(engine->kernel_context);

	result = false;



out_unlock:

	return false;

	__timeline_mark_unlock(engine->kernel_context, flags);

	return result;

}

}





static int __engine_park(struct intel_wakeref *wf)

static int __engine_park(struct intel_wakeref *wf)

drivers/gpu/drm/i915/gt/intel_reset.c

+5 −4
Original line number Original line Diff line number Diff line
@@ -792,6 +792,7 @@ static bool __intel_gt_unset_wedged(struct intel_gt *gt) 
{

{

	struct intel_gt_timelines *timelines = &gt->timelines;

	struct intel_gt_timelines *timelines = &gt->timelines;

	struct intel_timeline *tl;

	struct intel_timeline *tl;

	unsigned long flags;





	if (!test_bit(I915_WEDGED, &gt->reset.flags))

	if (!test_bit(I915_WEDGED, &gt->reset.flags))

		return true;

		return true;
@@ -811,7 +812,7 @@ static bool __intel_gt_unset_wedged(struct intel_gt *gt) 
	 *

	 *

	 * No more can be submitted until we reset the wedged bit.

	 * No more can be submitted until we reset the wedged bit.

	 */

	 */

	spin_lock(&timelines->lock);

	spin_lock_irqsave(&timelines->lock, flags);

	list_for_each_entry(tl, &timelines->active_list, link) {

	list_for_each_entry(tl, &timelines->active_list, link) {

		struct i915_request *rq;

		struct i915_request *rq;
@@ -819,7 +820,7 @@ static bool __intel_gt_unset_wedged(struct intel_gt *gt) 
		if (!rq)

		if (!rq)

			continue;

			continue;





		spin_unlock(&timelines->lock);

		spin_unlock_irqrestore(&timelines->lock, flags);





		/*

		/*

		 * All internal dependencies (i915_requests) will have

		 * All internal dependencies (i915_requests) will have
@@ -832,10 +833,10 @@ static bool __intel_gt_unset_wedged(struct intel_gt *gt) 
		i915_request_put(rq);

		i915_request_put(rq);





		/* Restart iteration after droping lock */

		/* Restart iteration after droping lock */

		spin_lock(&timelines->lock);

		spin_lock_irqsave(&timelines->lock, flags);

		tl = list_entry(&timelines->active_list, typeof(*tl), link);

		tl = list_entry(&timelines->active_list, typeof(*tl), link);

	}

	}

	spin_unlock(&timelines->lock);

	spin_unlock_irqrestore(&timelines->lock, flags);





	intel_gt_sanitize(gt, false);

	intel_gt_sanitize(gt, false);

drivers/gpu/drm/i915/gt/intel_timeline.c

+6 −4
Original line number Original line Diff line number Diff line
@@ -337,6 +337,7 @@ int intel_timeline_pin(struct intel_timeline *tl) 
void intel_timeline_enter(struct intel_timeline *tl)

void intel_timeline_enter(struct intel_timeline *tl)

{

{

	struct intel_gt_timelines *timelines = &tl->gt->timelines;

	struct intel_gt_timelines *timelines = &tl->gt->timelines;

	unsigned long flags;





	lockdep_assert_held(&tl->mutex);

	lockdep_assert_held(&tl->mutex);
@@ -345,14 +346,15 @@ void intel_timeline_enter(struct intel_timeline *tl) 
		return;

		return;

	GEM_BUG_ON(!tl->active_count); /* overflow? */

	GEM_BUG_ON(!tl->active_count); /* overflow? */





	spin_lock(&timelines->lock);

	spin_lock_irqsave(&timelines->lock, flags);

	list_add(&tl->link, &timelines->active_list);

	list_add(&tl->link, &timelines->active_list);

	spin_unlock(&timelines->lock);

	spin_unlock_irqrestore(&timelines->lock, flags);

}

}





void intel_timeline_exit(struct intel_timeline *tl)

void intel_timeline_exit(struct intel_timeline *tl)

{

{

	struct intel_gt_timelines *timelines = &tl->gt->timelines;

	struct intel_gt_timelines *timelines = &tl->gt->timelines;

	unsigned long flags;





	lockdep_assert_held(&tl->mutex);

	lockdep_assert_held(&tl->mutex);
@@ -360,9 +362,9 @@ void intel_timeline_exit(struct intel_timeline *tl) 
	if (--tl->active_count)

	if (--tl->active_count)

		return;

		return;





	spin_lock(&timelines->lock);

	spin_lock_irqsave(&timelines->lock, flags);

	list_del(&tl->link);

	list_del(&tl->link);

	spin_unlock(&timelines->lock);

	spin_unlock_irqrestore(&timelines->lock, flags);





	/*

	/*

	 * Since this timeline is idle, all bariers upon which we were waiting

	 * Since this timeline is idle, all bariers upon which we were waiting

drivers/gpu/drm/i915/i915_gem.c

+8 −7
Original line number Original line Diff line number Diff line
@@ -889,12 +889,13 @@ void i915_gem_runtime_suspend(struct drm_i915_private *i915) 




static long

static long

wait_for_timelines(struct drm_i915_private *i915,

wait_for_timelines(struct drm_i915_private *i915,

		   unsigned int flags, long timeout)

		   unsigned int wait, long timeout)

{

{

	struct intel_gt_timelines *timelines = &i915->gt.timelines;

	struct intel_gt_timelines *timelines = &i915->gt.timelines;

	struct intel_timeline *tl;

	struct intel_timeline *tl;

	unsigned long flags;





	spin_lock(&timelines->lock);

	spin_lock_irqsave(&timelines->lock, flags);

	list_for_each_entry(tl, &timelines->active_list, link) {

	list_for_each_entry(tl, &timelines->active_list, link) {

		struct i915_request *rq;

		struct i915_request *rq;
@@ -902,7 +903,7 @@ wait_for_timelines(struct drm_i915_private *i915, 
		if (!rq)

		if (!rq)

			continue;

			continue;





		spin_unlock(&timelines->lock);

		spin_unlock_irqrestore(&timelines->lock, flags);





		/*

		/*

		 * "Race-to-idle".

		 * "Race-to-idle".
@@ -913,19 +914,19 @@ wait_for_timelines(struct drm_i915_private *i915, 
		 * want to complete as quickly as possible to avoid prolonged

		 * want to complete as quickly as possible to avoid prolonged

		 * stalls, so allow the gpu to boost to maximum clocks.

		 * stalls, so allow the gpu to boost to maximum clocks.

		 */

		 */

		if (flags & I915_WAIT_FOR_IDLE_BOOST)

		if (wait & I915_WAIT_FOR_IDLE_BOOST)

			gen6_rps_boost(rq);

			gen6_rps_boost(rq);





		timeout = i915_request_wait(rq, flags, timeout);

		timeout = i915_request_wait(rq, wait, timeout);

		i915_request_put(rq);

		i915_request_put(rq);

		if (timeout < 0)

		if (timeout < 0)

			return timeout;

			return timeout;





		/* restart after reacquiring the lock */

		/* restart after reacquiring the lock */

		spin_lock(&timelines->lock);

		spin_lock_irqsave(&timelines->lock, flags);

		tl = list_entry(&timelines->active_list, typeof(*tl), link);

		tl = list_entry(&timelines->active_list, typeof(*tl), link);

	}

	}

	spin_unlock(&timelines->lock);

	spin_unlock_irqrestore(&timelines->lock, flags);





	return timeout;

	return timeout;

}

}

drivers/gpu/drm/i915/i915_request.c

+5 −4
Original line number Original line Diff line number Diff line
@@ -1465,9 +1465,10 @@ bool i915_retire_requests(struct drm_i915_private *i915) 
{

{

	struct intel_gt_timelines *timelines = &i915->gt.timelines;

	struct intel_gt_timelines *timelines = &i915->gt.timelines;

	struct intel_timeline *tl, *tn;

	struct intel_timeline *tl, *tn;

	unsigned long flags;

	LIST_HEAD(free);

	LIST_HEAD(free);





	spin_lock(&timelines->lock);

	spin_lock_irqsave(&timelines->lock, flags);

	list_for_each_entry_safe(tl, tn, &timelines->active_list, link) {

	list_for_each_entry_safe(tl, tn, &timelines->active_list, link) {

		if (!mutex_trylock(&tl->mutex))

		if (!mutex_trylock(&tl->mutex))

			continue;

			continue;
@@ -1475,11 +1476,11 @@ bool i915_retire_requests(struct drm_i915_private *i915) 
		intel_timeline_get(tl);

		intel_timeline_get(tl);

		GEM_BUG_ON(!tl->active_count);

		GEM_BUG_ON(!tl->active_count);

		tl->active_count++; /* pin the list element */

		tl->active_count++; /* pin the list element */

		spin_unlock(&timelines->lock);

		spin_unlock_irqrestore(&timelines->lock, flags);





		retire_requests(tl);

		retire_requests(tl);





		spin_lock(&timelines->lock);

		spin_lock_irqsave(&timelines->lock, flags);





		/* Resume iteration after dropping lock */

		/* Resume iteration after dropping lock */

		list_safe_reset_next(tl, tn, link);

		list_safe_reset_next(tl, tn, link);
@@ -1494,7 +1495,7 @@ bool i915_retire_requests(struct drm_i915_private *i915) 
			list_add(&tl->link, &free);

			list_add(&tl->link, &free);

		}

		}

	}

	}

	spin_unlock(&timelines->lock);

	spin_unlock_irqrestore(&timelines->lock, flags);





	list_for_each_entry_safe(tl, tn, &free, link)

	list_for_each_entry_safe(tl, tn, &free, link)

		__intel_timeline_free(&tl->kref);

		__intel_timeline_free(&tl->kref);