| Message ID | 20200115083346.2601512-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Use common priotree lists for virtual engine | expand |
On 15/01/2020 08:33, Chris Wilson wrote: > Currently, we skip error capture upon forced preemption. We apply forced > preemption when there is a higher priority request that should be > running but is being blocked, and we skip inline error capture so that > the preemption request is not further delayed by a user controlled > capture -- extending the denial of service. > > However, preemption reset is also used for heartbeats and regular GPU > hangs. By skipping the error capture, we remove the ability to debug GPU > hangs. > > In order to capture the error without delaying the preemption request > further, we can do an out-of-line capture by removing the guilty request > from the execution queue and scheduling a work to dump that request. > When removing a request, we need to remove the entire context and all > descendants from the execution queue, so that they do not jump past. > > Closes: https://gitlab.freedesktop.org/drm/intel/issues/738 > Fixes: 3a7a92aba8fb ("drm/i915/execlists: Force preemption") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_lrc.c | 120 +++++++++++++++++++++++++++- > 1 file changed, 118 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > index 43c19dc9c0c7..a84477df32bd 100644 > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > @@ -2392,7 +2392,6 @@ static void __execlists_hold(struct i915_request *rq) > } while(rq); > } > > -__maybe_unused > static void execlists_hold(struct intel_engine_cs *engine, > struct i915_request *rq) > { > @@ -2472,7 +2471,6 @@ static void __execlists_unhold(struct i915_request *rq) > } while(rq); > } > > -__maybe_unused > static void execlists_unhold(struct intel_engine_cs *engine, > struct i915_request *rq) > { > @@ -2492,6 +2490,121 @@ static void execlists_unhold(struct intel_engine_cs *engine, > spin_unlock_irq(&engine->active.lock); > } > > +struct execlists_capture { > + struct work_struct work; > + struct i915_request *rq; > + struct i915_gpu_coredump *error; > +}; > + > +static void execlists_capture_work(struct work_struct *work) > +{ > + struct execlists_capture *cap = container_of(work, typeof(*cap), work); > + const gfp_t gfp = GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN; > + struct intel_engine_cs *engine = cap->rq->engine; > + struct intel_gt_coredump *gt = cap->error->gt; > + struct intel_engine_capture_vma *vma; > + > + /* Compress all the objects attached to the request, slow! */ > + vma = intel_engine_coredump_add_request(gt->engine, cap->rq, gfp); > + if (vma) { > + struct i915_vma_compress *compress = > + i915_vma_capture_prepare(gt); > + > + intel_engine_coredump_add_vma(gt->engine, vma, compress); > + i915_vma_capture_finish(gt, compress); > + } > + > + gt->simulated = gt->engine->simulated; > + cap->error->simulated = gt->simulated; > + > + /* Publish the error state, and announce it to the world */ > + i915_error_state_store(cap->error); > + i915_gpu_coredump_put(cap->error); > + > + /* Return this request and all that depend upon it for signaling */ > + execlists_unhold(engine, cap->rq); > + > + kfree(cap); > +} > + > +static struct i915_gpu_coredump *capture_regs(struct intel_engine_cs *engine) > +{ > + const gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN; > + struct i915_gpu_coredump *e; > + > + e = i915_gpu_coredump_alloc(engine->i915, gfp); > + if (!e) > + return NULL; > + > + e->gt = intel_gt_coredump_alloc(engine->gt, gfp); > + if (!e->gt) > + goto err; > + > + e->gt->engine = intel_engine_coredump_alloc(engine, gfp); > + if (!e->gt->engine) > + goto err_gt; > + > + return e; > + > +err_gt: > + kfree(e->gt); > +err: > + kfree(e); > + return NULL; > +} > + > +static void execlists_capture(struct intel_engine_cs *engine) > +{ > + struct execlists_capture *cap; > + > + if (!IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)) > + return; > + > + cap = kmalloc(sizeof(*cap), GFP_ATOMIC); > + if (!cap) > + return; > + > + cap->rq = execlists_active(&engine->execlists); > + GEM_BUG_ON(!cap->rq); > + > + cap->rq = active_request(cap->rq->context->timeline, cap->rq); Old code, but why is active_request taking the timeline as a separate param when it always seems to be rq->context->timeline? > + > + /* > + * We need to _quickly_ capture the engine state before we reset. > + * We are inside an atomic section (softirq) here and we are delaying > + * the forced preemption event. > + */ > + cap->error = capture_regs(engine); > + if (!cap->error) > + goto err_free; > + > + if (i915_request_completed(cap->rq)) /* oops, not so guilty! */ > + goto err_store; Should this be a bug on? Doesn't look active_request() can return a non-completed request. Hm I guess we can make a wrong decision to reset the engine. But in any case, if request has completed in the meantime, why go to i915_error_state_store which will log a hang in dmesg? > + > + /* > + * Remove the request from the execlists queue, and take ownership > + * of the request. We pass it to our worker who will _slowly_ compress > + * all the pages the _user_ requested for debugging their batch, after > + * which we return it to the queue for signaling. > + * > + * By removing them from the execlists queue, we also remove the > + * requests from being processed by __unwind_incomplete_requests() > + * during the intel_engine_reset(), and so they will *not* be replayed > + * afterwards. > + */ > + execlists_hold(engine, cap->rq); > + > + INIT_WORK(&cap->work, execlists_capture_work); > + schedule_work(&cap->work); > + return; > + > +err_store: > + i915_error_state_store(cap->error); > + i915_gpu_coredump_put(cap->error); > +err_free: > + kfree(cap); > +} > + > static noinline void preempt_reset(struct intel_engine_cs *engine) > { > const unsigned int bit = I915_RESET_ENGINE + engine->id; > @@ -2509,6 +2622,9 @@ static noinline void preempt_reset(struct intel_engine_cs *engine) > ENGINE_TRACE(engine, "preempt timeout %lu+%ums\n", > READ_ONCE(engine->props.preempt_timeout_ms), > jiffies_to_msecs(jiffies - engine->execlists.preempt.expires)); > + > + ring_set_paused(engine, 1); /* Freeze the request in place */ Who unsets this flags? > + execlists_capture(engine); > intel_engine_reset(engine, "preemption time out"); > > tasklet_enable(&engine->execlists.tasklet); > Regards, Tvrtko
Quoting Tvrtko Ursulin (2020-01-16 17:22:10) > > On 15/01/2020 08:33, Chris Wilson wrote: > > Currently, we skip error capture upon forced preemption. We apply forced > > preemption when there is a higher priority request that should be > > running but is being blocked, and we skip inline error capture so that > > the preemption request is not further delayed by a user controlled > > capture -- extending the denial of service. > > > > However, preemption reset is also used for heartbeats and regular GPU > > hangs. By skipping the error capture, we remove the ability to debug GPU > > hangs. > > > > In order to capture the error without delaying the preemption request > > further, we can do an out-of-line capture by removing the guilty request > > from the execution queue and scheduling a work to dump that request. > > When removing a request, we need to remove the entire context and all > > descendants from the execution queue, so that they do not jump past. > > > > Closes: https://gitlab.freedesktop.org/drm/intel/issues/738 > > Fixes: 3a7a92aba8fb ("drm/i915/execlists: Force preemption") > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > drivers/gpu/drm/i915/gt/intel_lrc.c | 120 +++++++++++++++++++++++++++- > > 1 file changed, 118 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > > index 43c19dc9c0c7..a84477df32bd 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > > @@ -2392,7 +2392,6 @@ static void __execlists_hold(struct i915_request *rq) > > } while(rq); > > } > > > > -__maybe_unused > > static void execlists_hold(struct intel_engine_cs *engine, > > struct i915_request *rq) > > { > > @@ -2472,7 +2471,6 @@ static void __execlists_unhold(struct i915_request *rq) > > } while(rq); > > } > > > > -__maybe_unused > > static void execlists_unhold(struct intel_engine_cs *engine, > > struct i915_request *rq) > > { > > @@ -2492,6 +2490,121 @@ static void execlists_unhold(struct intel_engine_cs *engine, > > spin_unlock_irq(&engine->active.lock); > > } > > > > +struct execlists_capture { > > + struct work_struct work; > > + struct i915_request *rq; > > + struct i915_gpu_coredump *error; > > +}; > > + > > +static void execlists_capture_work(struct work_struct *work) > > +{ > > + struct execlists_capture *cap = container_of(work, typeof(*cap), work); > > + const gfp_t gfp = GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN; > > + struct intel_engine_cs *engine = cap->rq->engine; > > + struct intel_gt_coredump *gt = cap->error->gt; > > + struct intel_engine_capture_vma *vma; > > + > > + /* Compress all the objects attached to the request, slow! */ > > + vma = intel_engine_coredump_add_request(gt->engine, cap->rq, gfp); > > + if (vma) { > > + struct i915_vma_compress *compress = > > + i915_vma_capture_prepare(gt); > > + > > + intel_engine_coredump_add_vma(gt->engine, vma, compress); > > + i915_vma_capture_finish(gt, compress); > > + } > > + > > + gt->simulated = gt->engine->simulated; > > + cap->error->simulated = gt->simulated; > > + > > + /* Publish the error state, and announce it to the world */ > > + i915_error_state_store(cap->error); > > + i915_gpu_coredump_put(cap->error); > > + > > + /* Return this request and all that depend upon it for signaling */ > > + execlists_unhold(engine, cap->rq); > > + > > + kfree(cap); > > +} > > + > > +static struct i915_gpu_coredump *capture_regs(struct intel_engine_cs *engine) > > +{ > > + const gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN; > > + struct i915_gpu_coredump *e; > > + > > + e = i915_gpu_coredump_alloc(engine->i915, gfp); > > + if (!e) > > + return NULL; > > + > > + e->gt = intel_gt_coredump_alloc(engine->gt, gfp); > > + if (!e->gt) > > + goto err; > > + > > + e->gt->engine = intel_engine_coredump_alloc(engine, gfp); > > + if (!e->gt->engine) > > + goto err_gt; > > + > > + return e; > > + > > +err_gt: > > + kfree(e->gt); > > +err: > > + kfree(e); > > + return NULL; > > +} > > + > > +static void execlists_capture(struct intel_engine_cs *engine) > > +{ > > + struct execlists_capture *cap; > > + > > + if (!IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)) > > + return; > > + > > + cap = kmalloc(sizeof(*cap), GFP_ATOMIC); > > + if (!cap) > > + return; > > + > > + cap->rq = execlists_active(&engine->execlists); > > + GEM_BUG_ON(!cap->rq); > > + > > + cap->rq = active_request(cap->rq->context->timeline, cap->rq); > > Old code, but why is active_request taking the timeline as a separate > param when it always seems to be rq->context->timeline? It grew out of walking along the engine without a request. Old habits. > > + /* > > + * We need to _quickly_ capture the engine state before we reset. > > + * We are inside an atomic section (softirq) here and we are delaying > > + * the forced preemption event. > > + */ > > + cap->error = capture_regs(engine); > > + if (!cap->error) > > + goto err_free; > > + > > + if (i915_request_completed(cap->rq)) /* oops, not so guilty! */ > > + goto err_store; > > Should this be a bug on? Doesn't look active_request() can return a > non-completed request. Hm I guess we can make a wrong decision to reset > the engine. Aye. Until we actually invoke the reset, the engine is still active and so may have advanced. We call ring_set_paused() so it doesn't get too far ahead, but that still lets the breadcrumb tick over, so it is still possible for the active_request() to complete (but no more). > But in any case, if request has completed in the meantime, why go to > i915_error_state_store which will log a hang in dmesg? Because we are about to call intel_reset_engine(), so want some debug clue as to why we got into a situation where we invoked the forced preemption. I thought it might be useful to see the engine state, and to drop the "oops, please file a bug request" because of the reset. > > + /* > > + * Remove the request from the execlists queue, and take ownership > > + * of the request. We pass it to our worker who will _slowly_ compress > > + * all the pages the _user_ requested for debugging their batch, after > > + * which we return it to the queue for signaling. > > + * > > + * By removing them from the execlists queue, we also remove the > > + * requests from being processed by __unwind_incomplete_requests() > > + * during the intel_engine_reset(), and so they will *not* be replayed > > + * afterwards. > > + */ > > + execlists_hold(engine, cap->rq); > > + > > + INIT_WORK(&cap->work, execlists_capture_work); > > + schedule_work(&cap->work); > > + return; > > + > > +err_store: > > + i915_error_state_store(cap->error); > > + i915_gpu_coredump_put(cap->error); > > +err_free: > > + kfree(cap); > > +} > > + > > static noinline void preempt_reset(struct intel_engine_cs *engine) > > { > > const unsigned int bit = I915_RESET_ENGINE + engine->id; > > @@ -2509,6 +2622,9 @@ static noinline void preempt_reset(struct intel_engine_cs *engine) > > ENGINE_TRACE(engine, "preempt timeout %lu+%ums\n", > > READ_ONCE(engine->props.preempt_timeout_ms), > > jiffies_to_msecs(jiffies - engine->execlists.preempt.expires)); > > + > > + ring_set_paused(engine, 1); /* Freeze the request in place */ > > Who unsets this flags? Reset -> reset_csb_pointers -> ring_set_paused(0). -Chris
On 16/01/2020 17:48, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2020-01-16 17:22:10) >> >> On 15/01/2020 08:33, Chris Wilson wrote: >>> Currently, we skip error capture upon forced preemption. We apply forced >>> preemption when there is a higher priority request that should be >>> running but is being blocked, and we skip inline error capture so that >>> the preemption request is not further delayed by a user controlled >>> capture -- extending the denial of service. >>> >>> However, preemption reset is also used for heartbeats and regular GPU >>> hangs. By skipping the error capture, we remove the ability to debug GPU >>> hangs. >>> >>> In order to capture the error without delaying the preemption request >>> further, we can do an out-of-line capture by removing the guilty request >>> from the execution queue and scheduling a work to dump that request. >>> When removing a request, we need to remove the entire context and all >>> descendants from the execution queue, so that they do not jump past. >>> >>> Closes: https://gitlab.freedesktop.org/drm/intel/issues/738 >>> Fixes: 3a7a92aba8fb ("drm/i915/execlists: Force preemption") >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> drivers/gpu/drm/i915/gt/intel_lrc.c | 120 +++++++++++++++++++++++++++- >>> 1 file changed, 118 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c >>> index 43c19dc9c0c7..a84477df32bd 100644 >>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c >>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c >>> @@ -2392,7 +2392,6 @@ static void __execlists_hold(struct i915_request *rq) >>> } while(rq); >>> } >>> >>> -__maybe_unused >>> static void execlists_hold(struct intel_engine_cs *engine, >>> struct i915_request *rq) >>> { >>> @@ -2472,7 +2471,6 @@ static void __execlists_unhold(struct i915_request *rq) >>> } while(rq); >>> } >>> >>> -__maybe_unused >>> static void execlists_unhold(struct intel_engine_cs *engine, >>> struct i915_request *rq) >>> { >>> @@ -2492,6 +2490,121 @@ static void execlists_unhold(struct intel_engine_cs *engine, >>> spin_unlock_irq(&engine->active.lock); >>> } >>> >>> +struct execlists_capture { >>> + struct work_struct work; >>> + struct i915_request *rq; >>> + struct i915_gpu_coredump *error; >>> +}; >>> + >>> +static void execlists_capture_work(struct work_struct *work) >>> +{ >>> + struct execlists_capture *cap = container_of(work, typeof(*cap), work); >>> + const gfp_t gfp = GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN; >>> + struct intel_engine_cs *engine = cap->rq->engine; >>> + struct intel_gt_coredump *gt = cap->error->gt; >>> + struct intel_engine_capture_vma *vma; >>> + >>> + /* Compress all the objects attached to the request, slow! */ >>> + vma = intel_engine_coredump_add_request(gt->engine, cap->rq, gfp); >>> + if (vma) { >>> + struct i915_vma_compress *compress = >>> + i915_vma_capture_prepare(gt); >>> + >>> + intel_engine_coredump_add_vma(gt->engine, vma, compress); >>> + i915_vma_capture_finish(gt, compress); >>> + } >>> + >>> + gt->simulated = gt->engine->simulated; >>> + cap->error->simulated = gt->simulated; >>> + >>> + /* Publish the error state, and announce it to the world */ >>> + i915_error_state_store(cap->error); >>> + i915_gpu_coredump_put(cap->error); >>> + >>> + /* Return this request and all that depend upon it for signaling */ >>> + execlists_unhold(engine, cap->rq); >>> + >>> + kfree(cap); >>> +} >>> + >>> +static struct i915_gpu_coredump *capture_regs(struct intel_engine_cs *engine) >>> +{ >>> + const gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN; >>> + struct i915_gpu_coredump *e; >>> + >>> + e = i915_gpu_coredump_alloc(engine->i915, gfp); >>> + if (!e) >>> + return NULL; >>> + >>> + e->gt = intel_gt_coredump_alloc(engine->gt, gfp); >>> + if (!e->gt) >>> + goto err; >>> + >>> + e->gt->engine = intel_engine_coredump_alloc(engine, gfp); >>> + if (!e->gt->engine) >>> + goto err_gt; >>> + >>> + return e; >>> + >>> +err_gt: >>> + kfree(e->gt); >>> +err: >>> + kfree(e); >>> + return NULL; >>> +} >>> + >>> +static void execlists_capture(struct intel_engine_cs *engine) >>> +{ >>> + struct execlists_capture *cap; >>> + >>> + if (!IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)) >>> + return; >>> + >>> + cap = kmalloc(sizeof(*cap), GFP_ATOMIC); >>> + if (!cap) >>> + return; >>> + >>> + cap->rq = execlists_active(&engine->execlists); >>> + GEM_BUG_ON(!cap->rq); >>> + >>> + cap->rq = active_request(cap->rq->context->timeline, cap->rq); >> >> Old code, but why is active_request taking the timeline as a separate >> param when it always seems to be rq->context->timeline? > > It grew out of walking along the engine without a request. Old habits. > >>> + /* >>> + * We need to _quickly_ capture the engine state before we reset. >>> + * We are inside an atomic section (softirq) here and we are delaying >>> + * the forced preemption event. >>> + */ >>> + cap->error = capture_regs(engine); >>> + if (!cap->error) >>> + goto err_free; >>> + >>> + if (i915_request_completed(cap->rq)) /* oops, not so guilty! */ >>> + goto err_store; >> >> Should this be a bug on? Doesn't look active_request() can return a >> non-completed request. Hm I guess we can make a wrong decision to reset >> the engine. > > Aye. Until we actually invoke the reset, the engine is still active and > so may have advanced. We call ring_set_paused() so it doesn't get too > far ahead, but that still lets the breadcrumb tick over, so it is still > possible for the active_request() to complete (but no more). ... >> But in any case, if request has completed in the meantime, why go to >> i915_error_state_store which will log a hang in dmesg? > > Because we are about to call intel_reset_engine(), so want some debug > clue as to why we got into a situation where we invoked the forced > preemption. I thought it might be useful to see the engine state, and to > drop the "oops, please file a bug request" because of the reset. ... so we could still decide to bail out if request completed in the meantime and give up on the whole reset business. Why not if not? I guess it is of little practical difference, micro-second here or there before a potential false positive. >>> + /* >>> + * Remove the request from the execlists queue, and take ownership >>> + * of the request. We pass it to our worker who will _slowly_ compress >>> + * all the pages the _user_ requested for debugging their batch, after >>> + * which we return it to the queue for signaling. >>> + * >>> + * By removing them from the execlists queue, we also remove the >>> + * requests from being processed by __unwind_incomplete_requests() >>> + * during the intel_engine_reset(), and so they will *not* be replayed >>> + * afterwards. >>> + */ >>> + execlists_hold(engine, cap->rq); >>> + >>> + INIT_WORK(&cap->work, execlists_capture_work); >>> + schedule_work(&cap->work); >>> + return; >>> + >>> +err_store: >>> + i915_error_state_store(cap->error); >>> + i915_gpu_coredump_put(cap->error); >>> +err_free: >>> + kfree(cap); >>> +} >>> + >>> static noinline void preempt_reset(struct intel_engine_cs *engine) >>> { >>> const unsigned int bit = I915_RESET_ENGINE + engine->id; >>> @@ -2509,6 +2622,9 @@ static noinline void preempt_reset(struct intel_engine_cs *engine) >>> ENGINE_TRACE(engine, "preempt timeout %lu+%ums\n", >>> READ_ONCE(engine->props.preempt_timeout_ms), >>> jiffies_to_msecs(jiffies - engine->execlists.preempt.expires)); >>> + >>> + ring_set_paused(engine, 1); /* Freeze the request in place */ >> >> Who unsets this flags? > > Reset -> reset_csb_pointers -> ring_set_paused(0). Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
Quoting Tvrtko Ursulin (2020-01-16 18:14:24) > > On 16/01/2020 17:48, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2020-01-16 17:22:10) > >> > >> On 15/01/2020 08:33, Chris Wilson wrote: > >>> + /* > >>> + * We need to _quickly_ capture the engine state before we reset. > >>> + * We are inside an atomic section (softirq) here and we are delaying > >>> + * the forced preemption event. > >>> + */ > >>> + cap->error = capture_regs(engine); > >>> + if (!cap->error) > >>> + goto err_free; > >>> + > >>> + if (i915_request_completed(cap->rq)) /* oops, not so guilty! */ > >>> + goto err_store; > >> > >> Should this be a bug on? Doesn't look active_request() can return a > >> non-completed request. Hm I guess we can make a wrong decision to reset > >> the engine. > > > > Aye. Until we actually invoke the reset, the engine is still active and > > so may have advanced. We call ring_set_paused() so it doesn't get too > > far ahead, but that still lets the breadcrumb tick over, so it is still > > possible for the active_request() to complete (but no more). > > ... > > >> But in any case, if request has completed in the meantime, why go to > >> i915_error_state_store which will log a hang in dmesg? > > > > Because we are about to call intel_reset_engine(), so want some debug > > clue as to why we got into a situation where we invoked the forced > > preemption. I thought it might be useful to see the engine state, and to > > drop the "oops, please file a bug request" because of the reset. > > ... so we could still decide to bail out if request completed in the > meantime and give up on the whole reset business. Why not if not? I > guess it is of little practical difference, micro-second here or there > before a potential false positive. (When I first added the check here, it was following a hacky __intel_gt_reset() to ensure the engine had stopped, so I needed to always do a real reset to cleanup the mess.) Hmm. I was about to say "but the preemption window expired and we need to reset". However, if we have completed this request and having done since our earlier inspection, it must also hit an arbitration point where the preemption will take place. So yes, we can bail out here quietly if we find ourselves with a completed request at the last moment. For simplicity, I'm just going to ignore the troublemaker and put it on the hold list. * Note that because we have not yet reset the engine at this point, * it is possible for the request that we have identified as being * guilty, did in fact complete and we will then hit an arbitration * point allowing the preemption to succeed. The likelihood of that * is very low (as the capturing of the engine registers should be * fast enough to run inside an irq-off atomic section!), so we will * simply hold that request accountable for being non-preemptible * long enough to force the reset. We will then skip the completed request when it comes time to dequeue. Business as usual in the land of preempt-to-busy. -Chris
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index 43c19dc9c0c7..a84477df32bd 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -2392,7 +2392,6 @@ static void __execlists_hold(struct i915_request *rq) } while(rq); } -__maybe_unused static void execlists_hold(struct intel_engine_cs *engine, struct i915_request *rq) { @@ -2472,7 +2471,6 @@ static void __execlists_unhold(struct i915_request *rq) } while(rq); } -__maybe_unused static void execlists_unhold(struct intel_engine_cs *engine, struct i915_request *rq) { @@ -2492,6 +2490,121 @@ static void execlists_unhold(struct intel_engine_cs *engine, spin_unlock_irq(&engine->active.lock); } +struct execlists_capture { + struct work_struct work; + struct i915_request *rq; + struct i915_gpu_coredump *error; +}; + +static void execlists_capture_work(struct work_struct *work) +{ + struct execlists_capture *cap = container_of(work, typeof(*cap), work); + const gfp_t gfp = GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN; + struct intel_engine_cs *engine = cap->rq->engine; + struct intel_gt_coredump *gt = cap->error->gt; + struct intel_engine_capture_vma *vma; + + /* Compress all the objects attached to the request, slow! */ + vma = intel_engine_coredump_add_request(gt->engine, cap->rq, gfp); + if (vma) { + struct i915_vma_compress *compress = + i915_vma_capture_prepare(gt); + + intel_engine_coredump_add_vma(gt->engine, vma, compress); + i915_vma_capture_finish(gt, compress); + } + + gt->simulated = gt->engine->simulated; + cap->error->simulated = gt->simulated; + + /* Publish the error state, and announce it to the world */ + i915_error_state_store(cap->error); + i915_gpu_coredump_put(cap->error); + + /* Return this request and all that depend upon it for signaling */ + execlists_unhold(engine, cap->rq); + + kfree(cap); +} + +static struct i915_gpu_coredump *capture_regs(struct intel_engine_cs *engine) +{ + const gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN; + struct i915_gpu_coredump *e; + + e = i915_gpu_coredump_alloc(engine->i915, gfp); + if (!e) + return NULL; + + e->gt = intel_gt_coredump_alloc(engine->gt, gfp); + if (!e->gt) + goto err; + + e->gt->engine = intel_engine_coredump_alloc(engine, gfp); + if (!e->gt->engine) + goto err_gt; + + return e; + +err_gt: + kfree(e->gt); +err: + kfree(e); + return NULL; +} + +static void execlists_capture(struct intel_engine_cs *engine) +{ + struct execlists_capture *cap; + + if (!IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)) + return; + + cap = kmalloc(sizeof(*cap), GFP_ATOMIC); + if (!cap) + return; + + cap->rq = execlists_active(&engine->execlists); + GEM_BUG_ON(!cap->rq); + + cap->rq = active_request(cap->rq->context->timeline, cap->rq); + + /* + * We need to _quickly_ capture the engine state before we reset. + * We are inside an atomic section (softirq) here and we are delaying + * the forced preemption event. + */ + cap->error = capture_regs(engine); + if (!cap->error) + goto err_free; + + if (i915_request_completed(cap->rq)) /* oops, not so guilty! */ + goto err_store; + + /* + * Remove the request from the execlists queue, and take ownership + * of the request. We pass it to our worker who will _slowly_ compress + * all the pages the _user_ requested for debugging their batch, after + * which we return it to the queue for signaling. + * + * By removing them from the execlists queue, we also remove the + * requests from being processed by __unwind_incomplete_requests() + * during the intel_engine_reset(), and so they will *not* be replayed + * afterwards. + */ + execlists_hold(engine, cap->rq); + + INIT_WORK(&cap->work, execlists_capture_work); + schedule_work(&cap->work); + return; + +err_store: + i915_error_state_store(cap->error); + i915_gpu_coredump_put(cap->error); +err_free: + kfree(cap); +} + static noinline void preempt_reset(struct intel_engine_cs *engine) { const unsigned int bit = I915_RESET_ENGINE + engine->id; @@ -2509,6 +2622,9 @@ static noinline void preempt_reset(struct intel_engine_cs *engine) ENGINE_TRACE(engine, "preempt timeout %lu+%ums\n", READ_ONCE(engine->props.preempt_timeout_ms), jiffies_to_msecs(jiffies - engine->execlists.preempt.expires)); + + ring_set_paused(engine, 1); /* Freeze the request in place */ + execlists_capture(engine); intel_engine_reset(engine, "preemption time out"); tasklet_enable(&engine->execlists.tasklet);
Currently, we skip error capture upon forced preemption. We apply forced preemption when there is a higher priority request that should be running but is being blocked, and we skip inline error capture so that the preemption request is not further delayed by a user controlled capture -- extending the denial of service. However, preemption reset is also used for heartbeats and regular GPU hangs. By skipping the error capture, we remove the ability to debug GPU hangs. In order to capture the error without delaying the preemption request further, we can do an out-of-line capture by removing the guilty request from the execution queue and scheduling a work to dump that request. When removing a request, we need to remove the entire context and all descendants from the execution queue, so that they do not jump past. Closes: https://gitlab.freedesktop.org/drm/intel/issues/738 Fixes: 3a7a92aba8fb ("drm/i915/execlists: Force preemption") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/gt/intel_lrc.c | 120 +++++++++++++++++++++++++++- 1 file changed, 118 insertions(+), 2 deletions(-)