diff mbox

[1/7] drm/i915: Flush submission tasklet after bumping priority

Message ID 20180507092527.7359-1-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson May 7, 2018, 9:25 a.m. UTC
When called from process context tasklet_schedule() defers itself to
ksoftirqd. From experience this may cause unacceptable latencies of over
200ms in executing the submission tasklet, our goal is to reprioritise
the HW execution queue and trigger preemption immediately, so convert
the rcu protection over to include a kick of the tasklet.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Mika Kuoppala May 7, 2018, 11:15 a.m. UTC | #1
Chris Wilson <chris@chris-wilson.co.uk> writes:

> When called from process context tasklet_schedule() defers itself to
> ksoftirqd. From experience this may cause unacceptable latencies of over
> 200ms in executing the submission tasklet, our goal is to reprioritise
> the HW execution queue and trigger preemption immediately, so convert

I would add 'execlists preemption' even tho the context points to it,
as we are also quite intimately entangled here with kernel preemption
too.

> the rcu protection over to include a kick of the tasklet.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5ece6ae4bdff..03cd30001b5d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -578,10 +578,10 @@ static void __fence_set_priority(struct dma_fence *fence,
>  	rq = to_request(fence);
>  	engine = rq->engine;
>  
> -	rcu_read_lock();
> +	local_bh_disable(); /* RCU serialisation for set-wedged protection */
>  	if (engine->schedule)
>  		engine->schedule(rq, attr);
> -	rcu_read_unlock();
> +	local_bh_enable(); /* kick the tasklets if queues were reprioritised */

Do we end up calling the tasklet->func() explicitly on the end of
engine->schedule()?

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

>  }
>  
>  static void fence_set_priority(struct dma_fence *fence,
> -- 
> 2.17.0
Chris Wilson May 7, 2018, 11:19 a.m. UTC | #2
Quoting Mika Kuoppala (2018-05-07 12:15:58)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > When called from process context tasklet_schedule() defers itself to
> > ksoftirqd. From experience this may cause unacceptable latencies of over
> > 200ms in executing the submission tasklet, our goal is to reprioritise
> > the HW execution queue and trigger preemption immediately, so convert
> 
> I would add 'execlists preemption' even tho the context points to it,
> as we are also quite intimately entangled here with kernel preemption
> too.

I'll settle for HW preemption.
 
> > the rcu protection over to include a kick of the tasklet.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 5ece6ae4bdff..03cd30001b5d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -578,10 +578,10 @@ static void __fence_set_priority(struct dma_fence *fence,
> >       rq = to_request(fence);
> >       engine = rq->engine;
> >  
> > -     rcu_read_lock();
> > +     local_bh_disable(); /* RCU serialisation for set-wedged protection */
> >       if (engine->schedule)
> >               engine->schedule(rq, attr);
> > -     rcu_read_unlock();
> > +     local_bh_enable(); /* kick the tasklets if queues were reprioritised */
> 
> Do we end up calling the tasklet->func() explicitly on the end of
> engine->schedule()?

We do tasklet_hi_schedule(). Well in the future, we might either call
 ->func() directly or schedule. In the former case, no softirq is raised
 and local_bh_enable is just a slightly heavier preempt_disable().
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5ece6ae4bdff..03cd30001b5d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -578,10 +578,10 @@  static void __fence_set_priority(struct dma_fence *fence,
 	rq = to_request(fence);
 	engine = rq->engine;
 
-	rcu_read_lock();
+	local_bh_disable(); /* RCU serialisation for set-wedged protection */
 	if (engine->schedule)
 		engine->schedule(rq, attr);
-	rcu_read_unlock();
+	local_bh_enable(); /* kick the tasklets if queues were reprioritised */
 }
 
 static void fence_set_priority(struct dma_fence *fence,