[02/18] drm/i915/gt: Mark up the nested engine-pm timeline lock as irqsafe

Submitted by Chris Wilson on Aug. 19, 2019, 7:58 a.m.

Details

Message ID 20190819075835.20065-3-chris@chris-wilson.co.uk
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Chris Wilson Aug. 19, 2019, 7:58 a.m.
We use a fake timeline->mutex lock to reassure lockdep that the timeline
is always locked when emitting requests. However, the use inside
__engine_park() may be inside hardirq and so lockdep now complains about
the mixed irq-state of the nested locked. Disable irqs around the
lockdep tracking to keep it happy.

Fixes: 6c69a45445af ("drm/i915/gt: Mark context->active_count as protected by timeline->mutex")
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 | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
index 5f03f7dcad72..5d003751968b 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
@@ -37,9 +37,15 @@  static int __engine_unpark(struct intel_wakeref *wf)
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_LOCKDEP)
+
 static inline void __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);
 }
 
 static inline void __timeline_mark_unlock(struct intel_context *ce)
@@ -47,6 +53,18 @@  static inline void __timeline_mark_unlock(struct intel_context *ce)
 	mutex_release(&ce->timeline->mutex.dep_map, 0, _THIS_IP_);
 }
 
+#else
+
+static inline void __timeline_mark_lock(struct intel_context *ce)
+{
+}
+
+static inline void __timeline_mark_unlock(struct intel_context *ce)
+{
+}
+
+#endif
+
 static bool switch_to_kernel_context(struct intel_engine_cs *engine)
 {
 	struct i915_request *rq;

Comments

Chris Wilson <chris@chris-wilson.co.uk> writes:

> We use a fake timeline->mutex lock to reassure lockdep that the timeline
> is always locked when emitting requests. However, the use inside
> __engine_park() may be inside hardirq and so lockdep now complains about
> the mixed irq-state of the nested locked. Disable irqs around the
> lockdep tracking to keep it happy.
>
> Fixes: 6c69a45445af ("drm/i915/gt: Mark context->active_count as protected by timeline->mutex")
> 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 | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> index 5f03f7dcad72..5d003751968b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> @@ -37,9 +37,15 @@ static int __engine_unpark(struct intel_wakeref *wf)
>  	return 0;
>  }
>  
> +#if IS_ENABLED(CONFIG_LOCKDEP)
> +
>  static inline void __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);

I am starting to have second thoughts. One could argue that the
net effect is on positive side.

But we diverge on behaviour now.

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

>  }
>  
>  static inline void __timeline_mark_unlock(struct intel_context *ce)
> @@ -47,6 +53,18 @@ static inline void __timeline_mark_unlock(struct intel_context *ce)
>  	mutex_release(&ce->timeline->mutex.dep_map, 0, _THIS_IP_);
>  }
>  
> +#else
> +
> +static inline void __timeline_mark_lock(struct intel_context *ce)
> +{
> +}
> +
> +static inline void __timeline_mark_unlock(struct intel_context *ce)
> +{
> +}
> +
> +#endif
> +
>  static bool switch_to_kernel_context(struct intel_engine_cs *engine)
>  {
>  	struct i915_request *rq;
> -- 
> 2.23.0.rc1
Quoting Mika Kuoppala (2019-08-19 09:43:29)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > We use a fake timeline->mutex lock to reassure lockdep that the timeline
> > is always locked when emitting requests. However, the use inside
> > __engine_park() may be inside hardirq and so lockdep now complains about
> > the mixed irq-state of the nested locked. Disable irqs around the
> > lockdep tracking to keep it happy.
> >
> > Fixes: 6c69a45445af ("drm/i915/gt: Mark context->active_count as protected by timeline->mutex")
> > 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 | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > index 5f03f7dcad72..5d003751968b 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > @@ -37,9 +37,15 @@ static int __engine_unpark(struct intel_wakeref *wf)
> >       return 0;
> >  }
> >  
> > +#if IS_ENABLED(CONFIG_LOCKDEP)
> > +
> >  static inline void __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);
> 
> I am starting to have second thoughts. One could argue that the
> net effect is on positive side.
> 
> But we diverge on behaviour now.

Are you worrying about the #if-#else and accidentally sticking more code
in there?
-Chris
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2019-08-19 09:43:29)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > We use a fake timeline->mutex lock to reassure lockdep that the timeline
>> > is always locked when emitting requests. However, the use inside
>> > __engine_park() may be inside hardirq and so lockdep now complains about
>> > the mixed irq-state of the nested locked. Disable irqs around the
>> > lockdep tracking to keep it happy.
>> >
>> > Fixes: 6c69a45445af ("drm/i915/gt: Mark context->active_count as protected by timeline->mutex")
>> > 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 | 18 ++++++++++++++++++
>> >  1 file changed, 18 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
>> > index 5f03f7dcad72..5d003751968b 100644
>> > --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
>> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
>> > @@ -37,9 +37,15 @@ static int __engine_unpark(struct intel_wakeref *wf)
>> >       return 0;
>> >  }
>> >  
>> > +#if IS_ENABLED(CONFIG_LOCKDEP)
>> > +
>> >  static inline void __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);
>> 
>> I am starting to have second thoughts. One could argue that the
>> net effect is on positive side.
>> 
>> But we diverge on behaviour now.
>
> Are you worrying about the #if-#else and accidentally sticking more code
> in there?

Just the detail that now with lockdep we change the irq pattern
but now thinking about it more, with this block it should not matter
at all. So could be that my concern is totally bogus.

-Mika