From: Matthew Brost <matthew.brost@intel.com>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
daniele.ceraolospurio@intel.com
Subject: Re: [Intel-gfx] [PATCH 08/27] drm/i915/selftests: Add a cancel request selftest that triggers a reset
Date: Thu, 26 Aug 2021 07:00:18 -0700 [thread overview]
Message-ID: <20210826140016.GA19362@jons-linux-dev-box> (raw)
In-Reply-To: <2aa468eb-e7a8-1617-1b92-7a8f8b6ae015@linux.intel.com>
On Thu, Aug 26, 2021 at 10:32:54AM +0100, Tvrtko Ursulin wrote:
>
> On 26/08/2021 04:23, Matthew Brost wrote:
> > Add a cancel request selftest that results in an engine reset to cancel
> > the request as it is non-preemptable. Also insert a NOP request after
> > the cancelled request and confirm that it completely successfully.
>
> Which patch fixes a problem this exposes in the execlists implementation?
>
https://patchwork.freedesktop.org/patch/451421/?series=93704&rev=6
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> > drivers/gpu/drm/i915/selftests/i915_request.c | 100 ++++++++++++++++++
> > 1 file changed, 100 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/selftests/i915_request.c b/drivers/gpu/drm/i915/selftests/i915_request.c
> > index d67710d10615..e2c5db77f087 100644
> > --- a/drivers/gpu/drm/i915/selftests/i915_request.c
> > +++ b/drivers/gpu/drm/i915/selftests/i915_request.c
> > @@ -772,6 +772,98 @@ static int __cancel_completed(struct intel_engine_cs *engine)
> > return err;
> > }
> > +static int __cancel_reset(struct intel_engine_cs *engine)
> > +{
> > + struct intel_context *ce;
> > + struct igt_spinner spin;
> > + struct i915_request *rq, *nop;
> > + unsigned long preempt_timeout_ms;
> > + int err = 0;
> > +
>
> You may need to skip the test if preempt timeout is compiled out or if GPU
> reset is altogether disabled.
>
Yes, probably. Will fix this.
> > + preempt_timeout_ms = engine->props.preempt_timeout_ms;
> > + engine->props.preempt_timeout_ms = 100;
> > +
> > + if (igt_spinner_init(&spin, engine->gt))
> > + goto out_restore;
> > +
> > + ce = intel_context_create(engine);
> > + if (IS_ERR(ce)) {
> > + err = PTR_ERR(ce);
> > + goto out_spin;
> > + }
> > +
> > + rq = igt_spinner_create_request(&spin, ce, MI_NOOP);
> > + if (IS_ERR(rq)) {
> > + err = PTR_ERR(rq);
> > + goto out_ce;
> > + }
> > +
> > + pr_debug("%s: Cancelling active request\n", engine->name);
>
> "active non-preemptable" perhaps?
>
Sure.
> > + i915_request_get(rq);
> > + i915_request_add(rq);
> > + if (!igt_wait_for_spinner(&spin, rq)) {
> > + struct drm_printer p = drm_info_printer(engine->i915->drm.dev);
> > +
> > + pr_err("Failed to start spinner on %s\n", engine->name);
> > + intel_engine_dump(engine, &p, "%s\n", engine->name);
> > + err = -ETIME;
> > + goto out_rq;
> > + }
> > +
> > + nop = intel_context_create_request(ce);
> > + if (IS_ERR(nop))
> > + goto out_nop;
> > + i915_request_get(nop);
> > + i915_request_add(nop);
> > +
> > + i915_request_cancel(rq, -EINTR);
> > +
> > + if (i915_request_wait(rq, 0, HZ) < 0) {
> > + struct drm_printer p = drm_info_printer(engine->i915->drm.dev);
> > +
> > + pr_err("%s: Failed to cancel hung request\n", engine->name);
> > + intel_engine_dump(engine, &p, "%s\n", engine->name);
> > + err = -ETIME;
> > + goto out_nop;
> > + }
> > +
> > + if (rq->fence.error != -EINTR) {
> > + pr_err("%s: fence not cancelled (%u)\n",
> > + engine->name, rq->fence.error);
> > + err = -EINVAL;
> > + goto out_nop;
> > + }
> > +
> > + if (i915_request_wait(nop, 0, HZ) < 0) {
> > + struct drm_printer p = drm_info_printer(engine->i915->drm.dev);
> > +
> > + pr_err("%s: Failed to complete nop request\n", engine->name);
> > + intel_engine_dump(engine, &p, "%s\n", engine->name);
> > + err = -ETIME;
> > + goto out_nop;
> > + }
> > +
> > + if (nop->fence.error != 0) {
> > + pr_err("%s: Nop request errored (%u)\n",
>
> Maybe s/nop/innocent/ in the respective log messages?
>
I kinda perfer NOP.
> > + engine->name, nop->fence.error);
> > + err = -EINVAL;
> > + }
> > +
> > +out_nop:
> > + i915_request_put(nop);
> > +out_rq:
> > + i915_request_put(rq);
> > +out_ce:
> > + intel_context_put(ce);
> > +out_spin:
> > + igt_spinner_fini(&spin);
> > +out_restore:
> > + engine->props.preempt_timeout_ms = preempt_timeout_ms;
> > + if (err)
> > + pr_err("%s: %s error %d\n", __func__, engine->name, err);
> > + return err;
> > +}
> > +
> > static int live_cancel_request(void *arg)
> > {
> > struct drm_i915_private *i915 = arg;
> > @@ -804,6 +896,14 @@ static int live_cancel_request(void *arg)
> > return err;
> > if (err2)
> > return err2;
> > +
> > + /* Expects reset so call outside of igt_live_test_* */
>
> Hm there are live tests like live_preempt_cancel which seemingly manage to
> do resets under the live test block.
>
You can increment t->reset_global if a GT reset is expected, problem is
only execlists do a GT while GuC submission does a GuC engine based
reset so we'd have to put in a statement like this if was within the
begin / end block:
if !guc
t->reset_global++
I'd just rather leave it as is rather than baking in execlists / guc
backend specific knowledge into the test.
Matt
> Regards,
>
> Tvrtko
>
> > + err = __cancel_reset(engine);
> > + if (err)
> > + return err;
> > +
> > + if (igt_flush_test(i915))
> > + return -EIO;
> > }
> > return 0;
> >
next prev parent reply other threads:[~2021-08-26 14:05 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-26 3:23 [PATCH 00/27] Clean up GuC CI failures, simplify locking, and kernel DOC Matthew Brost
2021-08-26 3:23 ` [PATCH 01/27] drm/i915/guc: Fix blocked context accounting Matthew Brost
2021-08-26 3:23 ` [PATCH 02/27] drm/i915/guc: Fix outstanding G2H accounting Matthew Brost
2021-08-26 23:09 ` Daniele Ceraolo Spurio
2021-08-27 1:36 ` Matthew Brost
2021-08-26 3:23 ` [PATCH 03/27] drm/i915/guc: Unwind context requests in reverse order Matthew Brost
2021-08-26 3:23 ` [PATCH 04/27] drm/i915/guc: Don't drop ce->guc_active.lock when unwinding context Matthew Brost
2021-08-26 3:23 ` [PATCH 05/27] drm/i915/guc: Process all G2H message at once in work queue Matthew Brost
2021-08-26 3:23 ` [PATCH 06/27] drm/i915/guc: Workaround reset G2H is received after schedule done G2H Matthew Brost
2021-08-26 23:11 ` Daniele Ceraolo Spurio
2021-08-26 3:23 ` [PATCH 07/27] Revert "drm/i915/gt: Propagate change in error status to children on unhold" Matthew Brost
2021-08-26 3:23 ` [PATCH 08/27] drm/i915/selftests: Add a cancel request selftest that triggers a reset Matthew Brost
2021-08-26 9:32 ` [Intel-gfx] " Tvrtko Ursulin
2021-08-26 14:00 ` Matthew Brost [this message]
2021-08-26 3:23 ` [PATCH 09/27] drm/i915/guc: Kick tasklet after queuing a request Matthew Brost
2021-08-26 3:23 ` [PATCH 10/27] drm/i915/guc: Don't enable scheduling on a banned context, guc_id invalid, not registered Matthew Brost
2021-08-26 3:23 ` [PATCH 11/27] drm/i915/guc: Copy whole golden context, set engine state size of subset Matthew Brost
2021-08-26 23:21 ` Daniele Ceraolo Spurio
2021-08-26 23:33 ` [Intel-gfx] " John Harrison
2021-08-26 3:23 ` [PATCH 12/27] drm/i915/selftests: Add initial GuC selftest for scrubbing lost G2H Matthew Brost
2021-08-26 3:23 ` [PATCH 13/27] drm/i915/guc: Take context ref when cancelling request Matthew Brost
2021-08-26 3:23 ` [PATCH 14/27] drm/i915/guc: Don't touch guc_state.sched_state without a lock Matthew Brost
2021-08-26 3:23 ` [PATCH 15/27] drm/i915/guc: Reset LRC descriptor if register returns -ENODEV Matthew Brost
2021-08-26 3:23 ` [PATCH 16/27] drm/i915: Allocate error capture in nowait context Matthew Brost
2021-08-26 16:18 ` [Intel-gfx] " Matthew Brost
2021-08-26 16:21 ` Daniel Vetter
2021-08-26 3:23 ` [PATCH 17/27] drm/i915/guc: Flush G2H work queue during reset Matthew Brost
2021-08-26 3:23 ` [PATCH 18/27] drm/i915/guc: Release submit fence from an irq_work Matthew Brost
2021-08-26 3:23 ` [PATCH 19/27] drm/i915/guc: Move guc_blocked fence to struct guc_state Matthew Brost
2021-08-26 3:23 ` [PATCH 20/27] drm/i915/guc: Rework and simplify locking Matthew Brost
2021-08-26 3:23 ` [PATCH 21/27] drm/i915/guc: Proper xarray usage for contexts_lookup Matthew Brost
2021-08-26 3:23 ` [PATCH 22/27] drm/i915/guc: Drop pin count check trick between sched_disable and re-pin Matthew Brost
2021-08-26 3:23 ` [PATCH 23/27] drm/i915/guc: Move GuC priority fields in context under guc_active Matthew Brost
2021-08-26 23:26 ` Daniele Ceraolo Spurio
2021-08-26 3:23 ` [PATCH 24/27] drm/i915/guc: Move fields protected by guc->contexts_lock into sub structure Matthew Brost
2021-08-26 3:23 ` [PATCH 25/27] drm/i915/guc: Drop guc_active move everything into guc_state Matthew Brost
2021-08-26 3:23 ` [PATCH 26/27] drm/i915/guc: Add GuC kernel doc Matthew Brost
2021-08-31 19:04 ` [Intel-gfx] " John Harrison
2021-08-26 3:23 ` [PATCH 27/27] drm/i915/guc: Drop static inline functions intel_guc_submission.c Matthew Brost
2021-08-31 19:09 ` [Intel-gfx] " John Harrison
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210826140016.GA19362@jons-linux-dev-box \
--to=matthew.brost@intel.com \
--cc=daniele.ceraolospurio@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=tvrtko.ursulin@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).