Re: [Intel-gfx] [PATCH 8/8] drm/i915: Don't disable interrupts and pretend a lock as been acquired in __timeline_mark_lock().

[Peter Zijlstra <peterz@infradead.org>]

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);
>  	++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@infradead.org>