* [PATCH] drm/i915: Don't disable interrupts and pretend a lock as been acquired in __timeline_mark_lock().
@ 2021-11-18 16:59 Sebastian Andrzej Siewior
2021-11-19 16:04 ` Daniel Vetter
2021-12-08 9:10 ` [Intel-gfx] " Tvrtko Ursulin
0 siblings, 2 replies; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-11-18 16:59 UTC (permalink / raw)
To: intel-gfx, dri-devel
Cc: David Airlie, Peter Zijlstra, Rodrigo Vivi, Thomas Gleixner
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 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_PARKED 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>
---
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(-)
--- 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(s
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_PARKED, &ce->flags));
++ce->active_count;
}
--- 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_PARKED 12
struct {
u64 timeout_us;
--- 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_
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(str
{
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(str
* 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_PARKED, &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(str
result = false;
out_unlock:
- __timeline_mark_unlock(ce, flags);
+ clear_bit(CONTEXT_IS_PARKED, &ce->flags);
return result;
}
--- 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_
{
/* 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_PARKED, &rq->context->flags));
}
static inline struct i915_gem_context *
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Don't disable interrupts and pretend a lock as been acquired in __timeline_mark_lock().
2021-11-18 16:59 [PATCH] drm/i915: Don't disable interrupts and pretend a lock as been acquired in __timeline_mark_lock() Sebastian Andrzej Siewior
@ 2021-11-19 16:04 ` Daniel Vetter
2021-11-30 16:33 ` Sebastian Andrzej Siewior
2021-12-08 9:10 ` [Intel-gfx] " Tvrtko Ursulin
1 sibling, 1 reply; 7+ messages in thread
From: Daniel Vetter @ 2021-11-19 16:04 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: David Airlie, intel-gfx, Peter Zijlstra, dri-devel, Rodrigo Vivi,
Thomas Gleixner
On Thu, Nov 18, 2021 at 05:59:14PM +0100, 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 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_PARKED 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>
Yeah if we can simplify this with reverts then I'm all for this.
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
I've asked drm/i915 maintainers to check&merge.
-Daniel
> ---
> 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(-)
>
> --- 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(s
>
> 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_PARKED, &ce->flags));
> ++ce->active_count;
> }
>
> --- 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_PARKED 12
>
> struct {
> u64 timeout_us;
> --- 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_
> 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(str
> {
> 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(str
> * 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_PARKED, &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(str
>
> result = false;
> out_unlock:
> - __timeline_mark_unlock(ce, flags);
> + clear_bit(CONTEXT_IS_PARKED, &ce->flags);
> return result;
> }
>
> --- 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_
> {
> /* 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_PARKED, &rq->context->flags));
> }
>
> static inline struct i915_gem_context *
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Don't disable interrupts and pretend a lock as been acquired in __timeline_mark_lock().
2021-11-19 16:04 ` Daniel Vetter
@ 2021-11-30 16:33 ` Sebastian Andrzej Siewior
2021-12-07 18:00 ` Daniel Vetter
0 siblings, 1 reply; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-11-30 16:33 UTC (permalink / raw)
To: Daniel Vetter
Cc: David Airlie, intel-gfx, Peter Zijlstra, dri-devel, Rodrigo Vivi,
Thomas Gleixner
On 2021-11-19 17:04:00 [+0100], Daniel Vetter wrote:
> Yeah if we can simplify this with reverts then I'm all for this.
>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> I've asked drm/i915 maintainers to check&merge.
Thanks. Should I repost my queue (excluding this one) or should wait
until this one has been taken care?
> -Daniel
Sebastian
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Don't disable interrupts and pretend a lock as been acquired in __timeline_mark_lock().
2021-11-30 16:33 ` Sebastian Andrzej Siewior
@ 2021-12-07 18:00 ` Daniel Vetter
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2021-12-07 18:00 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: David Airlie, intel-gfx, Peter Zijlstra, dri-devel, Rodrigo Vivi,
Thomas Gleixner
On Tue, Nov 30, 2021 at 05:33:09PM +0100, Sebastian Andrzej Siewior wrote:
> On 2021-11-19 17:04:00 [+0100], Daniel Vetter wrote:
> > Yeah if we can simplify this with reverts then I'm all for this.
> >
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > I've asked drm/i915 maintainers to check&merge.
>
> Thanks. Should I repost my queue (excluding this one) or should wait
> until this one has been taken care?
No idea, Tvrtko (check latest MAINTAINERS) and Joonas need to take care of
this. Holler again if it's falling through the cracks.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Don't disable interrupts and pretend a lock as been acquired in __timeline_mark_lock().
2021-11-18 16:59 [PATCH] drm/i915: Don't disable interrupts and pretend a lock as been acquired in __timeline_mark_lock() Sebastian Andrzej Siewior
2021-11-19 16:04 ` Daniel Vetter
@ 2021-12-08 9:10 ` Tvrtko Ursulin
2021-12-10 20:44 ` [PATCH v2] " Sebastian Andrzej Siewior
1 sibling, 1 reply; 7+ messages in thread
From: Tvrtko Ursulin @ 2021-12-08 9:10 UTC (permalink / raw)
To: Sebastian Andrzej Siewior, intel-gfx, dri-devel
Cc: David Airlie, Peter Zijlstra, Thomas Gleixner
On 18/11/2021 16:59, 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")
6dcb85a0ad99 ("drm/i915: Hold irq-off for the entire fake lock period")
is very relevant as well.
> 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.
Agreed this is not good.
> 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.
Hmm commit message of d67739268cf0 ("drm/i915/gt: Mark up the nested
engine-pm timeline lock as irqsafe") makes it sound like parking can run
with interrupts disabled already. I mean outside of the irq disable hack
in here.
I don't see it possible though, both due might_lock in intel_wakeref_put
and GFP_NOWAIT in switch_to_kernel_context. So I will assume that commit
message refers to an earlier state of the code base.
So the approach in this patch looks good to me.
> Add a CONTEXT_IS_PARKED 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>
> ---
> 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(-)
>
> --- 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(s
>
> 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_PARKED, &ce->flags));
> ++ce->active_count;
> }
>
> --- 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_PARKED 12
Pedantic comment is that semantics of the bit flag suggest it should be
name CONTEXT_IS_PARKING, or along those lines. Since the flag gets clear
as soon as the parking completes. I'd suggest doing that change for the
self-documenting benefit.
Regards,
Tvrtko
>
> struct {
> u64 timeout_us;
> --- 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_
> 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(str
> {
> 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(str
> * 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_PARKED, &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(str
>
> result = false;
> out_unlock:
> - __timeline_mark_unlock(ce, flags);
> + clear_bit(CONTEXT_IS_PARKED, &ce->flags);
> return result;
> }
>
> --- 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_
> {
> /* 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_PARKED, &rq->context->flags));
> }
>
> static inline struct i915_gem_context *
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] drm/i915: Don't disable interrupts and pretend a lock as been acquired in __timeline_mark_lock().
2021-12-08 9:10 ` [Intel-gfx] " Tvrtko Ursulin
@ 2021-12-10 20:44 ` Sebastian Andrzej Siewior
2021-12-13 9:25 ` Tvrtko Ursulin
0 siblings, 1 reply; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-12-10 20:44 UTC (permalink / raw)
To: Tvrtko Ursulin
Cc: David Airlie, Peter Zijlstra, intel-gfx, Thomas Gleixner, dri-devel
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.
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 *
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm/i915: Don't disable interrupts and pretend a lock as been acquired in __timeline_mark_lock().
2021-12-10 20:44 ` [PATCH v2] " Sebastian Andrzej Siewior
@ 2021-12-13 9:25 ` Tvrtko Ursulin
0 siblings, 0 replies; 7+ messages in thread
From: Tvrtko Ursulin @ 2021-12-13 9:25 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: David Airlie, Peter Zijlstra, intel-gfx, Thomas Gleixner, dri-devel
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 *
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-12-13 9:25 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-18 16:59 [PATCH] drm/i915: Don't disable interrupts and pretend a lock as been acquired in __timeline_mark_lock() Sebastian Andrzej Siewior
2021-11-19 16:04 ` Daniel Vetter
2021-11-30 16:33 ` Sebastian Andrzej Siewior
2021-12-07 18:00 ` Daniel Vetter
2021-12-08 9:10 ` [Intel-gfx] " Tvrtko Ursulin
2021-12-10 20:44 ` [PATCH v2] " Sebastian Andrzej Siewior
2021-12-13 9:25 ` Tvrtko Ursulin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).