From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915/gt: Move execlists_reset() out of line
Date: Thu, 21 Jan 2021 10:55:49 +0000 [thread overview]
Message-ID: <4c24b53a-2db5-39c5-c735-a0f732b7245d@linux.intel.com> (raw)
In-Reply-To: <20210121003253.27225-1-chris@chris-wilson.co.uk>
On 21/01/2021 00:32, Chris Wilson wrote:
> Reduce the bulk of execlists_submission_tasklet by moving the unlikely
> reset function out of line.
>
> add/remove: 1/0 grow/shrink: 0/1 up/down: 960/-935 (25)
> Function old new delta
> execlists_reset - 960 +960
> execlists_submission_tasklet 6629 5694 -935
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> .../drm/i915/gt/intel_execlists_submission.c | 36 +++++++++----------
> 1 file changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 740ff05fd692..43cc85241886 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -2299,10 +2299,13 @@ static void execlists_capture(struct intel_engine_cs *engine)
> kfree(cap);
> }
>
> -static void execlists_reset(struct intel_engine_cs *engine, const char *msg)
> +static noinline void execlists_reset(struct intel_engine_cs *engine)
> {
> + struct intel_engine_execlists *el = &engine->execlists;
> const unsigned int bit = I915_RESET_ENGINE + engine->id;
> unsigned long *lock = &engine->gt->reset.flags;
> + unsigned long eir = fetch_and_zero(&el->error_interrupt);
We got the locking wrong for this one. Irq handler side is under
gt->irq_lock, but there are unlocked rmw cycles in the tasklet. Not
counting this fetch_and_zero which is also unlocked.
If we nest gt->irq_lock under the engine->active.lock here, or the
opposite from the irq_handler, plus short locked sections in the tasklet
and csb handling. I *think* both options could be fine from locking order.
Regards,
Tvrtko
> + const char *msg;
>
> if (!intel_has_reset_engine(engine->gt))
> return;
> @@ -2310,16 +2313,25 @@ static void execlists_reset(struct intel_engine_cs *engine, const char *msg)
> if (test_and_set_bit(bit, lock))
> return;
>
> + /* Generate the error message in priority wrt to the user! */
> + if (eir & GENMASK(15, 0))
> + msg = "CS error"; /* thrown by a user payload */
> + else if (eir & ERROR_CSB)
> + msg = "invalid CSB event";
> + else if (eir & ERROR_PREEMPT)
> + msg = "preemption time out";
> + else
> + msg = "internal error";
> ENGINE_TRACE(engine, "reset for %s\n", msg);
>
> /* Mark this tasklet as disabled to avoid waiting for it to complete */
> - tasklet_disable_nosync(&engine->execlists.tasklet);
> + tasklet_disable_nosync(&el->tasklet);
>
> ring_set_paused(engine, 1); /* Freeze the current request in place */
> execlists_capture(engine);
> intel_engine_reset(engine, msg);
>
> - tasklet_enable(&engine->execlists.tasklet);
> + tasklet_enable(&el->tasklet);
> clear_and_wake_up_bit(bit, lock);
> }
>
> @@ -2355,22 +2367,8 @@ static void execlists_submission_tasklet(unsigned long data)
> engine->execlists.error_interrupt |= ERROR_PREEMPT;
> }
>
> - if (unlikely(READ_ONCE(engine->execlists.error_interrupt))) {
> - const char *msg;
> -
> - /* Generate the error message in priority wrt to the user! */
> - if (engine->execlists.error_interrupt & GENMASK(15, 0))
> - msg = "CS error"; /* thrown by a user payload */
> - else if (engine->execlists.error_interrupt & ERROR_CSB)
> - msg = "invalid CSB event";
> - else if (engine->execlists.error_interrupt & ERROR_PREEMPT)
> - msg = "preemption time out";
> - else
> - msg = "internal error";
> -
> - engine->execlists.error_interrupt = 0;
> - execlists_reset(engine, msg);
> - }
> + if (unlikely(READ_ONCE(engine->execlists.error_interrupt)))
> + execlists_reset(engine);
>
> if (!engine->execlists.pending[0]) {
> execlists_dequeue_irq(engine);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2021-01-21 10:55 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-21 0:32 [Intel-gfx] [PATCH] drm/i915/gt: Move execlists_reset() out of line Chris Wilson
2021-01-21 1:40 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2021-01-21 4:15 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2021-01-21 10:55 ` Tvrtko Ursulin [this message]
2021-01-21 10:58 ` [Intel-gfx] [PATCH] " Chris Wilson
2021-01-21 11:17 ` Tvrtko Ursulin
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=4c24b53a-2db5-39c5-c735-a0f732b7245d@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=chris@chris-wilson.co.uk \
--cc=intel-gfx@lists.freedesktop.org \
/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