[PATCH] drm/i915: Don't disable interrupts independently of the lock

[PATCH] drm/i915: Don't disable interrupts independently of the lock

[Sebastian Andrzej Siewior]

The locks (active.lock and rq->lock) need to be taken with disabled
interrupts. This is done in i915_request_retire() by disabling the
interrupts independently of the locks itself.
While local_irq_disable()+spin_lock() equals spin_lock_irq() on vanilla
it does not on PREEMPT_RT. Also, it is not obvious if there is a special reason
to why the interrupts are disabled independently of the lock.

Enable/disable interrupts as part of the locking instruction.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpu/drm/i915/i915_request.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -251,15 +251,13 @@ static bool i915_request_retire(struct i
 		active->retire(active, rq);
 	}
 
-	local_irq_disable();
-
 	/*
 	 * We only loosely track inflight requests across preemption,
 	 * and so we may find ourselves attempting to retire a _completed_
 	 * request that we have removed from the HW and put back on a run
 	 * queue.
 	 */
-	spin_lock(&rq->engine->active.lock);
+	spin_lock_irq(&rq->engine->active.lock);
 	list_del(&rq->sched.link);
 	spin_unlock(&rq->engine->active.lock);
 
@@ -278,9 +276,7 @@ static bool i915_request_retire(struct i
 		__notify_execute_cb(rq);
 	}
 	GEM_BUG_ON(!list_empty(&rq->execute_cb));
-	spin_unlock(&rq->lock);
-
-	local_irq_enable();
+	spin_unlock_irq(&rq->lock);
 
 	remove_from_client(rq);
 	list_del(&rq->link);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Don't disable interrupts independently of the lock

[Chris Wilson]

Quoting Sebastian Andrzej Siewior (2019-10-10 17:06:40)
> The locks (active.lock and rq->lock) need to be taken with disabled
> interrupts. This is done in i915_request_retire() by disabling the
> interrupts independently of the locks itself.
> While local_irq_disable()+spin_lock() equals spin_lock_irq() on vanilla
> it does not on PREEMPT_RT. Also, it is not obvious if there is a special reason
> to why the interrupts are disabled independently of the lock.
> 
> Enable/disable interrupts as part of the locking instruction.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/gpu/drm/i915/i915_request.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -251,15 +251,13 @@ static bool i915_request_retire(struct i
>                 active->retire(active, rq);
>         }
>  
> -       local_irq_disable();
> -
>         /*
>          * We only loosely track inflight requests across preemption,
>          * and so we may find ourselves attempting to retire a _completed_
>          * request that we have removed from the HW and put back on a run
>          * queue.
>          */
> -       spin_lock(&rq->engine->active.lock);
> +       spin_lock_irq(&rq->engine->active.lock);
>         list_del(&rq->sched.link);
>         spin_unlock(&rq->engine->active.lock);
>  
> @@ -278,9 +276,7 @@ static bool i915_request_retire(struct i
>                 __notify_execute_cb(rq);
>         }
>         GEM_BUG_ON(!list_empty(&rq->execute_cb));
> -       spin_unlock(&rq->lock);
> -
> -       local_irq_enable();
> +       spin_unlock_irq(&rq->lock);

Nothing screams about the imbalance? irq off from one lock to the other?
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: Don't disable interrupts independently of the lock

[Sebastian Andrzej Siewior]

On 2019-10-10 19:11:27 [+0100], Chris Wilson wrote:
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -251,15 +251,13 @@ static bool i915_request_retire(struct i
> >                 active->retire(active, rq);
> >         }
> >  
> > -       local_irq_disable();
> > -
> >         /*
> >          * We only loosely track inflight requests across preemption,
> >          * and so we may find ourselves attempting to retire a _completed_
> >          * request that we have removed from the HW and put back on a run
> >          * queue.
> >          */
> > -       spin_lock(&rq->engine->active.lock);
> > +       spin_lock_irq(&rq->engine->active.lock);
> >         list_del(&rq->sched.link);
> >         spin_unlock(&rq->engine->active.lock);
> >  
> > @@ -278,9 +276,7 @@ static bool i915_request_retire(struct i
> >                 __notify_execute_cb(rq);
> >         }
> >         GEM_BUG_ON(!list_empty(&rq->execute_cb));
> > -       spin_unlock(&rq->lock);
> > -
> > -       local_irq_enable();
> > +       spin_unlock_irq(&rq->lock);
> 
> Nothing screams about the imbalance? irq off from one lock to the other?

There is no imbalance, is there? Interrupts are disabled as part of
acquiring the first lock and enabled again as part of releasing the
second lock.
It may not look beautiful. 

I'm just not sure if this

|         spin_lock_irq(&rq->engine->active.lock);
|         list_del(&rq->sched.link);
|         spin_unlock_irq(&rq->engine->active.lock);
| 
|         spin_lock_irq(&rq->lock);
|         i915_request_mark_complete(rq);
…
|         spin_unlock_irq(&rq->lock);

has been avoided because an interrupt here could change something or if
this is just an optimisation.

> -Chris

Sebastian
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Don't disable interrupts independently of the lock

[Chris Wilson]

Quoting Sebastian Andrzej Siewior (2019-10-10 19:26:10)
> On 2019-10-10 19:11:27 [+0100], Chris Wilson wrote:
> > > --- a/drivers/gpu/drm/i915/i915_request.c
> > > +++ b/drivers/gpu/drm/i915/i915_request.c
> > > @@ -251,15 +251,13 @@ static bool i915_request_retire(struct i
> > >                 active->retire(active, rq);
> > >         }
> > >  
> > > -       local_irq_disable();
> > > -
> > >         /*
> > >          * We only loosely track inflight requests across preemption,
> > >          * and so we may find ourselves attempting to retire a _completed_
> > >          * request that we have removed from the HW and put back on a run
> > >          * queue.
> > >          */
> > > -       spin_lock(&rq->engine->active.lock);
> > > +       spin_lock_irq(&rq->engine->active.lock);
> > >         list_del(&rq->sched.link);
> > >         spin_unlock(&rq->engine->active.lock);
> > >  
> > > @@ -278,9 +276,7 @@ static bool i915_request_retire(struct i
> > >                 __notify_execute_cb(rq);
> > >         }
> > >         GEM_BUG_ON(!list_empty(&rq->execute_cb));
> > > -       spin_unlock(&rq->lock);
> > > -
> > > -       local_irq_enable();
> > > +       spin_unlock_irq(&rq->lock);
> > 
> > Nothing screams about the imbalance? irq off from one lock to the other?
> 
> There is no imbalance, is there? Interrupts are disabled as part of
> acquiring the first lock and enabled again as part of releasing the
> second lock.
> It may not look beautiful. 

Sure, it's at the same scope, I just expect at some point lockdep to
complain :)
 
> I'm just not sure if this
> 
> |         spin_lock_irq(&rq->engine->active.lock);
> |         list_del(&rq->sched.link);
> |         spin_unlock_irq(&rq->engine->active.lock);
> | 
> |         spin_lock_irq(&rq->lock);
> |         i915_request_mark_complete(rq);
> …
> |         spin_unlock_irq(&rq->lock);
> 
> has been avoided because an interrupt here could change something or if
> this is just an optimisation.

Just avoiding the back-to-back enable/disable.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: Don't disable interrupts independently of the lock

[Sebastian Andrzej Siewior]

On 2019-10-10 21:30:35 [+0100], Chris Wilson wrote:
> > |         spin_lock_irq(&rq->engine->active.lock);
> > |         list_del(&rq->sched.link);
> > |         spin_unlock_irq(&rq->engine->active.lock);
> > | 
> > |         spin_lock_irq(&rq->lock);
> > |         i915_request_mark_complete(rq);
> > …
> > |         spin_unlock_irq(&rq->lock);
> > 
> > has been avoided because an interrupt here could change something or if
> > this is just an optimisation.
> 
> Just avoiding the back-to-back enable/disable.

as I assumed. Is the patch okay?

> -Chris

Sebastian
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BUILD: failure for drm/i915: Don't disable interrupts independently of the lock (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Don't disable interrupts independently of the lock (rev2)
URL   : https://patchwork.freedesktop.org/series/59289/
State : failure

== Summary ==

Applying: drm/i915: Don't disable interrupts independently of the lock
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/i915_request.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 drm/i915: Don't disable interrupts independently of the lock
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Don't disable interrupts independently of the lock

[Sebastian Andrzej Siewior]

The locks (timeline->lock and rq->lock) need to be taken with disabled
interrupts. This is done in __retire_engine_request() by disabling the
interrupts independently of the locks itself.
While local_irq_disable()+spin_lock() equals spin_lock_irq() on vanilla
it does not on RT. Also, it is not obvious if there is a special reason
to why the interrupts are disabled independently of the lock.

Enable/disable interrupts as part of the locking instruction.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpu/drm/i915/i915_request.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index ca95ab2f4cfa3..8744d20ac1681 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -278,9 +278,7 @@ static void __retire_engine_request(struct intel_engine_cs *engine,
 
 	GEM_BUG_ON(!i915_request_completed(rq));
 
-	local_irq_disable();
-
-	spin_lock(&engine->timeline.lock);
+	spin_lock_irq(&engine->timeline.lock);
 	GEM_BUG_ON(!list_is_first(&rq->link, &engine->timeline.requests));
 	list_del_init(&rq->link);
 	spin_unlock(&engine->timeline.lock);
@@ -294,9 +292,7 @@ static void __retire_engine_request(struct intel_engine_cs *engine,
 		GEM_BUG_ON(!atomic_read(&rq->i915->gt_pm.rps.num_waiters));
 		atomic_dec(&rq->i915->gt_pm.rps.num_waiters);
 	}
-	spin_unlock(&rq->lock);
-
-	local_irq_enable();
+	spin_unlock_irq(&rq->lock);
 
 	/*
 	 * The backing object for the context is done after switching to the
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx