Re: [PATCH 1/4] drm/i915: Allow error capture without a request

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

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?

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;