On Fri, 2023-01-13 at 09:51 +0000, Tvrtko Ursulin wrote:
>
> On 12/01/2023 20:40, John Harrison wrote:
> > On 1/12/2023 02:01, Tvrtko Ursulin wrote:
> > > On 12/01/2023 02:53, John.C.Harrison@xxxxxxxxx wrote:
> > > > From: John Harrison <John.C.Harrison@xxxxxxxxx>
> > > >
> > > > There was a report of error captures occurring without any hung
> > > > context being indicated despite the capture being initiated by
> > > > a 'hung
> > > > context notification' from GuC. The problem was not
> > > > reproducible.
> > > > However, it is possible to happen if the context in question
> > > > has no
> > > > active requests. For example, if the hang was in the context
> > > > switch
> > > > itself then the breadcrumb write would have occurred and the
> > > > KMD would
> > > > see an idle context.
> > > >
> > > > In the interests of attempting to provide as much information
> > > > as
> > > > possible about a hang, it seems wise to include the engine info
> > > > regardless of whether a request was found or not. As opposed to
> > > > just
> > > > prentending there was no hang at all.
> > > >
> > > > So update the error capture code to always record engine
> > > > information
> > > > if an engine is given. Which means updating record_context() to
> > > > take a
> > > > context instead of a request (which it only ever used to find
> > > > the
> > > > context anyway). And split the request agnostic parts of
> > > > intel_engine_coredump_add_request() out into a seaprate
> > > > function.
> > > >
> > > > v2: Remove a duplicate 'if' statement (Umesh) and fix a put of
> > > > a null
> > > > pointer.
> > > >
> > > > Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> > > > Reviewed-by: Umesh Nerlige Ramappa
> > > > <umesh.nerlige.ramappa@xxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_gpu_error.c | 61
> > > > +++++++++++++++++++--------
> > > > 1 file changed, 43 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > index 9d5d5a397b64e..bd2cf7d235df0 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > @@ -1370,14 +1370,14 @@ static void
> > > > engine_record_execlists(struct
> > > > intel_engine_coredump *ee)
> > > > }
> > > > static bool record_context(struct i915_gem_context_coredump
> > > > *e,
> > > > - const struct i915_request *rq)
> > > > + struct intel_context *ce)
> > > > {
> > > > struct i915_gem_context *ctx;
> > > > struct task_struct *task;
> > > > bool simulated;
> > > > rcu_read_lock();
> > > > - ctx = rcu_dereference(rq->context->gem_context);
> > > > + ctx = rcu_dereference(ce->gem_context);
> > > > if (ctx && !kref_get_unless_zero(&ctx->ref))
> > > > ctx = NULL;
> > > > rcu_read_unlock();
> > > > @@ -1396,8 +1396,8 @@ static bool record_context(struct
> > > > i915_gem_context_coredump *e,
> > > > e->guilty = atomic_read(&ctx->guilty_count);
> > > > e->active = atomic_read(&ctx->active_count);
> > > > - e->total_runtime =
> > > > intel_context_get_total_runtime_ns(rq->context);
> > > > - e->avg_runtime = intel_context_get_avg_runtime_ns(rq-
> > > > >context);
> > > > + e->total_runtime = intel_context_get_total_runtime_ns(ce);
> > > > + e->avg_runtime = intel_context_get_avg_runtime_ns(ce);
> > > > simulated = i915_gem_context_no_error_capture(ctx);
> > > > @@ -1532,15 +1532,37 @@ intel_engine_coredump_alloc(struct
> > > > intel_engine_cs *engine, gfp_t gfp, u32 dump_
> > > > return ee;
> > > > }
> > > > +static struct intel_engine_capture_vma *
> > > > +engine_coredump_add_context(struct intel_engine_coredump *ee,
> > > > + struct intel_context *ce,
> > > > + gfp_t gfp)
> > > > +{
> > > > + struct intel_engine_capture_vma *vma = NULL;
> > > > +
> > > > + ee->simulated |= record_context(&ee->context, ce);
> > > > + if (ee->simulated)
> > > > + return NULL;
> > > > +
> > > > + /*
> > > > + * We need to copy these to an anonymous buffer
> > > > + * as the simplest method to avoid being overwritten
> > > > + * by userspace.
> > > > + */
> > > > + vma = capture_vma(vma, ce->ring->vma, "ring", gfp);
> > > > + vma = capture_vma(vma, ce->state, "HW context", gfp);
> > > > +
> > > > + return vma;
> > > > +}
> > > > +
> > > > struct intel_engine_capture_vma *
> > > > intel_engine_coredump_add_request(struct
> > > > intel_engine_coredump *ee,
> > > > struct i915_request *rq,
> > > > gfp_t gfp)
> > > > {
> > > > - struct intel_engine_capture_vma *vma = NULL;
> > > > + struct intel_engine_capture_vma *vma;
> > > > - ee->simulated |= record_context(&ee->context, rq);
> > > > - if (ee->simulated)
> > > > + vma = engine_coredump_add_context(ee, rq->context, gfp);
> > > > + if (!vma)
> > > > return NULL;
> > > > /*
> > > > @@ -1550,8 +1572,6 @@ intel_engine_coredump_add_request(struct
> > > > intel_engine_coredump *ee,
> > > > */
> > > > vma = capture_vma_snapshot(vma, rq->batch_res, gfp,
> > > > "batch");
> > > > vma = capture_user(vma, rq, gfp);
> > > > - vma = capture_vma(vma, rq->ring->vma, "ring", gfp);
> > > > - vma = capture_vma(vma, rq->context->state, "HW context",
> > > > gfp);
> > > > ee->rq_head = rq->head;
> > > > ee->rq_post = rq->postfix;
> > > > @@ -1608,8 +1628,11 @@ capture_engine(struct intel_engine_cs
> > > > *engine,
> > > > if (ce) {
> > > > intel_engine_clear_hung_context(engine);
> > > > rq = intel_context_find_active_request(ce);
> > > > - if (!rq || !i915_request_started(rq))
> > > > - goto no_request_capture;
> > > > + if (rq && !i915_request_started(rq)) {
> > > > + drm_info(&engine->gt->i915->drm, "Got hung context
> > > > on %s
> > > > with no active request!\n",
> > >
> > > Suggest s/active/started/ since we have both i915_request_active
> > > and
> > > i915_request_started, so to align the terminology.
> > The message text was based on the intent of the activity not the
> > naming
> > of some internal helper function. Can change it if you really want
> > but
> > "with no started request" just reads like bad English to me. Plus
> > it
> > gets removed in the next patch anyway...
> >
> >
> > >
> > > > + engine->name);
> > > > + rq = NULL;
> > > > + }
> > > > } else {
> > > > /*
> > > > * Getting here with GuC enabled means it is a forced
> > > > error
> > > > capture
> > > > @@ -1622,22 +1645,24 @@ capture_engine(struct intel_engine_cs
> > > > *engine,
> > > > flags);
> > > > }
> > > > }
> > > > - if (rq)
> > > > + if (rq) {
> > > > rq = i915_request_get_rcu(rq);
> > > > + capture = intel_engine_coredump_add_request(ee, rq,
> > > > ATOMIC_MAYFAIL);
> > > > + } else if (ce) {
> > > > + capture = engine_coredump_add_context(ee, ce,
> > > > ATOMIC_MAYFAIL);
> > > > + }
> > > > - if (!rq)
> > > > - goto no_request_capture;
> > > > -
> > > > - capture = intel_engine_coredump_add_request(ee, rq,
> > > > ATOMIC_MAYFAIL);
> > > > if (!capture) {
> > > > - i915_request_put(rq);
> > > > + if (rq)
> > > > + i915_request_put(rq);
> > > > goto no_request_capture;
> > > > }
> > > > if (dump_flags & CORE_DUMP_FLAG_IS_GUC_CAPTURE)
> > > > intel_guc_capture_get_matching_node(engine->gt, ee,
> > > > ce);
> > >
> > > This step requires non-NULL ce, so if you move it under the "else
> > > if
> > > (ce)" above then I *think* exit from the function can be
> > > consolidated
> > > to just:
> > >
> > > if (capture) {
> > > intel_engine_coredump_add_vma(ee, capture, compress);
> > > if (rq)
> > > i915_request_put(rq);
> > Is there any reason the rq ref needs to be held during the add_vma
> > call?
> > Can it now just be moved earlier to be:
> > if (rq) {
> > rq = i915_request_get_rcu(rq);
> > capture = intel_engine_coredump_add_request(ee, rq,
> > ATOMIC_MAYFAIL);
> > i915_request_put(rq);
> > }
> >
> > The internals of the request object are only touched in the above
> > _add_request() code. The later _add_vma() call fiddles around with
> > vmas
> > that pulled from the request but the capture_vma code inside
> > _add_request() has already copied everything, hasn't it? Or rather,
> > it
> > has grabbed its own private vma resource locks. So there is no
> > requirement to keep the request itself around still?
That sounds correct. It was some time ago since I worked with this code
but when i started IIRC KASAN told me the request along with the whole
capture list could disappear under us due to a parallel capture.
So the request reference added then might cover a bit too much now that
we also hold references on vma resources, which it looks like we do in
intel_engine_coredump_add_vma().
Another thing which is crappy with the current error capture code is
that the request capture list needs to be freed with the request and
not when the request signals (We can't block request signalling in the
capture code to keep the capture list around). There might be many
signaled requests hanging around in non-pruned dma_resv objects and
thus many unused capture lists with many unused vma resources. :/
/Thomas
>
> Don't know.. it is a question if changes from 60dc43d1190d
> ("drm/i915:
> Use struct vma_resource instead of struct vma_snapshot") removed the
> need for holding the rq reference that "long" I guess? Adding Thomas
> and
> Matt to perhaps comment.
>
> Regards,
>
> Tvrtko
>
>
> > John.
> >
> >
> > > } else {
> > > kfree(ee);
> > > ee = NULL;
> > > }
> > >
> > > return ee;
> > >
> > > No "if (rq) i915_request_put()" twice, and goto label can be
> > > completely removed.
> > >
> > > Regards,
> > >
> > > Tvrtko
> > >
> > > > intel_engine_coredump_add_vma(ee, capture, compress);
> > > > - i915_request_put(rq);
> > > > + if (rq)
> > > > + i915_request_put(rq);
> > > > return ee;
> >
