On Tue, Oct 05, 2021 at 05:00:46PM +0200, Sebastian Andrzej Siewior wrote:
This is a revert of commits d67739268cf0e ("drm/i915/gt: Mark up the nested engine-pm timeline lock as irqsafe") 6c69a45445af9 ("drm/i915/gt: Mark context->active_count as protected by timeline->mutex")
The existing code leads to a different behaviour depending on wheather lockdep is enabled or not. Any following lock that is acquired without disabling interrupts (but needs to) will not be noticed by lockdep.
This it not just a lockdep annotation but is used but an actual mutex_t that is properly used as a lock but in case of __timeline_mark_lock() lockdep is only told that it is acquired but no lock has been acquired.
It appears that its purporse is just satisfy the lockdep_assert_held() check in intel_context_mark_active(). The other problem with disabling interrupts is that on PREEMPT_RT interrupts are also disabled which leads to problems for instance later during memory allocation.
Add an argument to intel_context_mark_active() which is true if the lock must have been acquired, false if other magic is involved and the lock is not needed. Use the `false' argument only from within switch_to_kernel_context() and remove __timeline_mark_lock().
Cc: Peter Zijlstra peterz@infradead.org Signed-off-by: Sebastian Andrzej Siewior bigeasy@linutronix.de
Eeew, nice find.
-static inline void intel_context_mark_active(struct intel_context *ce) +static inline void intel_context_mark_active(struct intel_context *ce,
bool timeline_mutex_needed){
- lockdep_assert_held(&ce->timeline->mutex);
- if (timeline_mutex_needed)
lockdep_assert_held(&ce->timeline->mutex);}
Chris, might it be possible to write that something like:
lockdep_assert(lockdep_is_held(&ce->timeline->mutex) || engine_is_parked(ce));
instead?
Otherwise,
Acked-by: Peter Zijlstra (Intel) peterz@infradead.org