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@xxxxxxxxxxxxx>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
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);
> ++ce->active_count;
> }
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@xxxxxxxxxxxxx>
