| Message ID | 20180517074055.14638-5-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17/05/2018 08:40, Chris Wilson wrote: > As we want to be able to call i915_reset_engine and co from a softirq or Just by glancing i915_reset_engine looks to heavy weight to ever be callable from softirq/timer context. There is even a flush_workqueue in there. > timer context, we need to be irqsafe at all timers. So we have to forgo > the simple spin_lock_irq for the full spin_lock_irqsave. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 03874b50ada9..a3885adec78a 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3137,15 +3137,17 @@ i915_gem_reset_request(struct intel_engine_cs *engine, Or you meant to write i915_gem_reset_request / i915_gem_reset_engine in the commit message? Regards, Tvrtko > */ > request = i915_gem_find_active_request(engine); > if (request) { > + unsigned long flags; > + > i915_gem_context_mark_innocent(request->gem_context); > dma_fence_set_error(&request->fence, -EAGAIN); > > /* Rewind the engine to replay the incomplete rq */ > - spin_lock_irq(&engine->timeline.lock); > + spin_lock_irqsave(&engine->timeline.lock, flags); > request = list_prev_entry(request, link); > if (&request->link == &engine->timeline.requests) > request = NULL; > - spin_unlock_irq(&engine->timeline.lock); > + spin_unlock_irqrestore(&engine->timeline.lock, flags); > } > } > >
Quoting Tvrtko Ursulin (2018-05-17 11:27:20) > > On 17/05/2018 08:40, Chris Wilson wrote: > > As we want to be able to call i915_reset_engine and co from a softirq or > > Just by glancing i915_reset_engine looks to heavy weight to ever be > callable from softirq/timer context. There is even a flush_workqueue in > there. There isn't by the time we finish :) > > timer context, we need to be irqsafe at all timers. So we have to forgo > > the simple spin_lock_irq for the full spin_lock_irqsave. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 03874b50ada9..a3885adec78a 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -3137,15 +3137,17 @@ i915_gem_reset_request(struct intel_engine_cs *engine, > > Or you meant to write i915_gem_reset_request / i915_gem_reset_engine in > the commit message? static int try_preempt_reset(struct intel_engine_execlists *execlists) { struct tasklet_struct * const t = &execlists->tasklet; int err = -EBUSY; if (tasklet_trylock(t)) { struct intel_engine_cs *engine = container_of(execlists, typeof(*engine), execlists); const unsigned int bit = I915_RESET_ENGINE + engine->id; unsigned long *lock = &engine->i915->gpu_error.flags; t->func(t->data); if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT_TIMEOUT)) { /* Nothing to do; the tasklet was just delayed. */ err = 0; } else if (!test_and_set_bit(bit, lock)) { tasklet_disable_nosync(t); err = i915_reset_engine(engine, "preemption time out"); tasklet_enable(t); clear_bit(bit, lock); wake_up_bit(lock, bit); } tasklet_unlock(t); } return err; } is what I'm aiming for. And even a test case to call it in irq-off and other atomic contexts. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 03874b50ada9..a3885adec78a 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3137,15 +3137,17 @@ i915_gem_reset_request(struct intel_engine_cs *engine, */ request = i915_gem_find_active_request(engine); if (request) { + unsigned long flags; + i915_gem_context_mark_innocent(request->gem_context); dma_fence_set_error(&request->fence, -EAGAIN); /* Rewind the engine to replay the incomplete rq */ - spin_lock_irq(&engine->timeline.lock); + spin_lock_irqsave(&engine->timeline.lock, flags); request = list_prev_entry(request, link); if (&request->link == &engine->timeline.requests) request = NULL; - spin_unlock_irq(&engine->timeline.lock); + spin_unlock_irqrestore(&engine->timeline.lock, flags); } }
As we want to be able to call i915_reset_engine and co from a softirq or timer context, we need to be irqsafe at all timers. So we have to forgo the simple spin_lock_irq for the full spin_lock_irqsave. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)