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

Message ID 20190822121205.16711-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915: Hold irq-off for the entire fake lock period
Related show

Commit Message

Chris Wilson Aug. 22, 2019, 12:12 p.m. UTC
Sadly lockdep records when the irqs are reenabled 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!

Fixes: d67739268cf0 ("drm/i915/gt: Mark up the nested engine-pm timeline lock as irqsafe")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_pm.c | 28 +++++++++++++++--------
 1 file changed, 18 insertions(+), 10 deletions(-)

Comments

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

> Sadly lockdep records when the irqs are reenabled 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.
>

Tricky tweaks are tricky. It was not so easy
to lure lockdep to do our bidding :(

> Hopefully this is the last tweak required!

That's the spirit.

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

> Fixes: d67739268cf0 ("drm/i915/gt: Mark up the nested engine-pm timeline lock as irqsafe")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_pm.c | 28 +++++++++++++++--------
>  1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> index a372d4ea9370..65b5ca74b394 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> @@ -39,27 +39,32 @@ static int __engine_unpark(struct intel_wakeref *wf)
>  
>  #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;
>  
>  	local_irq_save(flags);
>  	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_);
> +	local_irq_restore(flags);
>  }
>  
>  #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)
>  {
>  	struct i915_request *rq;
> +	unsigned long flags;
> +	bool result = true;
>  
>  	/* Already inside the kernel context, safe to power down. */
>  	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
>  	 * 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);
>  	if (IS_ERR(rq))
>  		/* Context switch failed, hope for the best! Maybe reset? */
> -		return true;
> +		goto out_unlock;
>  
>  	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);
>  	__i915_request_queue(rq, NULL);
>  
> -	__timeline_mark_unlock(engine->kernel_context);
> -
> -	return false;
> +	result = false;
> +out_unlock:
> +	__timeline_mark_unlock(engine->kernel_context, flags);
> +	return result;
>  }
>  
>  static int __engine_park(struct intel_wakeref *wf)
> -- 
> 2.23.0

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
index a372d4ea9370..65b5ca74b394 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
@@ -39,27 +39,32 @@  static int __engine_unpark(struct intel_wakeref *wf)
 
 #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;
 
 	local_irq_save(flags);
 	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_);
+	local_irq_restore(flags);
 }
 
 #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)
 {
 	struct i915_request *rq;
+	unsigned long flags;
+	bool result = true;
 
 	/* Already inside the kernel context, safe to power down. */
 	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
 	 * 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);
 	if (IS_ERR(rq))
 		/* Context switch failed, hope for the best! Maybe reset? */
-		return true;
+		goto out_unlock;
 
 	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);
 	__i915_request_queue(rq, NULL);
 
-	__timeline_mark_unlock(engine->kernel_context);
-
-	return false;
+	result = false;
+out_unlock:
+	__timeline_mark_unlock(engine->kernel_context, flags);
+	return result;
 }
 
 static int __engine_park(struct intel_wakeref *wf)