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

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx