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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 10/12/2021 20:44, 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")
>     6dcb85a0ad990 ("drm/i915: Hold irq-off for the entire fake lock period")
> 
> The existing code leads to a different behaviour depending on whether
> 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 purpose 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 a CONTEXT_IS_PARKING bit to intel_engine_cs and set_bit/clear_bit it
> instead of mutex_acquire/mutex_release. Use test_bit in the two
> identified spots which relied on the lockdep annotation.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> v1…v2:
>   - Add commit 6dcb85a0ad990 as reference.
>   - Name the bit CONTEXT_IS_PARKING.

Thanks for the tweak;

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I'll pull it in in a minute.

Regards,

Tvrtko

>   drivers/gpu/drm/i915/gt/intel_context.h       |  3 +-
>   drivers/gpu/drm/i915/gt/intel_context_types.h |  1 +
>   drivers/gpu/drm/i915/gt/intel_engine_pm.c     | 38 +------------------
>   drivers/gpu/drm/i915/i915_request.h           |  3 +-
>   4 files changed, 7 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> index 246c37d72cd73..d8c74bbf9aae2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -211,7 +211,8 @@ static inline void intel_context_enter(struct intel_context *ce)
>   
>   static inline void intel_context_mark_active(struct intel_context *ce)
>   {
> -	lockdep_assert_held(&ce->timeline->mutex);
> +	lockdep_assert(lockdep_is_held(&ce->timeline->mutex) ||
> +		       test_bit(CONTEXT_IS_PARKING, &ce->flags));
>   	++ce->active_count;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> index 9e0177dc5484e..30cd81ad8911a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> @@ -118,6 +118,7 @@ struct intel_context {
>   #define CONTEXT_LRCA_DIRTY		9
>   #define CONTEXT_GUC_INIT		10
>   #define CONTEXT_PERMA_PIN		11
> +#define CONTEXT_IS_PARKING		12
>   
>   	struct {
>   		u64 timeout_us;
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> index a1334b48dde7b..a8a2ad44b7e39 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> @@ -80,39 +80,6 @@ static int __engine_unpark(struct intel_wakeref *wf)
>   	return 0;
>   }
>   
> -#if IS_ENABLED(CONFIG_LOCKDEP)
> -
> -static unsigned long __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_);
> -
> -	return flags;
> -}
> -
> -static void __timeline_mark_unlock(struct intel_context *ce,
> -				   unsigned long flags)
> -{
> -	mutex_release(&ce->timeline->mutex.dep_map, _THIS_IP_);
> -	local_irq_restore(flags);
> -}
> -
> -#else
> -
> -static unsigned long __timeline_mark_lock(struct intel_context *ce)
> -{
> -	return 0;
> -}
> -
> -static void __timeline_mark_unlock(struct intel_context *ce,
> -				   unsigned long flags)
> -{
> -}
> -
> -#endif /* !IS_ENABLED(CONFIG_LOCKDEP) */
> -
>   static void duration(struct dma_fence *fence, struct dma_fence_cb *cb)
>   {
>   	struct i915_request *rq = to_request(fence);
> @@ -159,7 +126,6 @@ static bool switch_to_kernel_context(struct intel_engine_cs *engine)
>   {
>   	struct intel_context *ce = engine->kernel_context;
>   	struct i915_request *rq;
> -	unsigned long flags;
>   	bool result = true;
>   
>   	/*
> @@ -214,7 +180,7 @@ static bool switch_to_kernel_context(struct intel_engine_cs *engine)
>   	 * engine->wakeref.count, we may see the request completion and retire
>   	 * it causing an underflow of the engine->wakeref.
>   	 */
> -	flags = __timeline_mark_lock(ce);
> +	set_bit(CONTEXT_IS_PARKING, &ce->flags);
>   	GEM_BUG_ON(atomic_read(&ce->timeline->active_count) < 0);
>   
>   	rq = __i915_request_create(ce, GFP_NOWAIT);
> @@ -246,7 +212,7 @@ static bool switch_to_kernel_context(struct intel_engine_cs *engine)
>   
>   	result = false;
>   out_unlock:
> -	__timeline_mark_unlock(ce, flags);
> +	clear_bit(CONTEXT_IS_PARKING, &ce->flags);
>   	return result;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
> index dc359242d1aec..b7fe67405fd32 100644
> --- a/drivers/gpu/drm/i915/i915_request.h
> +++ b/drivers/gpu/drm/i915/i915_request.h
> @@ -642,7 +642,8 @@ i915_request_timeline(const struct i915_request *rq)
>   {
>   	/* Valid only while the request is being constructed (or retired). */
>   	return rcu_dereference_protected(rq->timeline,
> -					 lockdep_is_held(&rcu_access_pointer(rq->timeline)->mutex));
> +					 lockdep_is_held(&rcu_access_pointer(rq->timeline)->mutex) ||
> +					 test_bit(CONTEXT_IS_PARKING, &rq->context->flags));
>   }
>   
>   static inline struct i915_gem_context *
>