From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: John.C.Harrison@Intel.com, Intel-GFX@Lists.FreeDesktop.Org
Cc: DRI-Devel@Lists.FreeDesktop.Org
Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915/gt: Make the heartbeat play nice with long pre-emption timeouts
Date: Tue, 22 Feb 2022 11:19:06 +0000 [thread overview]
Message-ID: <2a486991-1bfd-9b23-0b43-9173d17b7e13@linux.intel.com> (raw)
In-Reply-To: <20220218213307.1338478-3-John.C.Harrison@Intel.com>
On 18/02/2022 21:33, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> Compute workloads are inherantly not pre-emptible for long periods on
> current hardware. As a workaround for this, the pre-emption timeout
> for compute capable engines was disabled. This is undesirable with GuC
> submission as it prevents per engine reset of hung contexts. Hence the
> next patch will re-enable the timeout but bumped up by an order of
> magnititude.
(Some typos above.)
> However, the heartbeat might not respect that. Depending upon current
> activity, a pre-emption to the heartbeat pulse might not even be
> attempted until the last heartbeat period. Which means that only one
Might not be attempted, but could be if something is running with lower
priority. In which case I think special casing the last heartbeat does
not feel right because it can end up resetting the engine before it was
intended.
Like if first heartbeat decides to preempt (the decision is backend
specific, could be same prio + timeslicing), and preempt timeout has
been set to heartbeat interval * 3, then 2nd heartbeat gets queued up,
then 3rd, and so reset is triggered even before the first preempt
timeout legitimately expires (or just as it is about to react).
Instead, how about preempt timeout is always considered when calculating
when to emit the next heartbeat? End result would be similar to your
patch, in terms of avoiding the direct problem, although hang detection
would be overall longer (but more correct I think).
And it also means in the next patch you don't have to add coupling
between preempt timeout and heartbeat to intel_engine_setup. Instead
just some long preempt timeout would be needed. Granted, the decoupling
argument is not super strong since then the heartbeat code has the
coupling instead, but that still feels better to me. (Since we can say
heartbeats only make sense on loaded engines, and so things like preempt
timeout can legitimately be considered from there.)
Incidentally, that would be similar to a patch which Chris had a year
ago (https://patchwork.freedesktop.org/patch/419783/?series=86841&rev=1)
to fix some CI issue.
On a related topic, if GuC engine resets stop working when preempt
timeout is set to zero - I think we need to somehow let the user know if
they try to tweak it via sysfs. Perhaps go as far as -EINVAL in GuC
mode, if i915.reset has not explicitly disabled engine resets.
Regards,
Tvrtko
> period is granted for the pre-emption to occur. With the aforesaid
> bump, the pre-emption timeout could be significantly larger than this
> heartbeat period.
>
> So adjust the heartbeat code to take the pre-emption timeout into
> account. When it reaches the final (high priority) period, it now
> ensures the delay before hitting reset is bigger than the pre-emption
> timeout.
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> index a3698f611f45..72a82a6085e0 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> @@ -22,9 +22,25 @@
>
> static bool next_heartbeat(struct intel_engine_cs *engine)
> {
> + struct i915_request *rq;
> long delay;
>
> delay = READ_ONCE(engine->props.heartbeat_interval_ms);
> +
> + rq = engine->heartbeat.systole;
> + if (rq && rq->sched.attr.priority >= I915_PRIORITY_BARRIER) {
> + long longer;
> +
> + /*
> + * The final try is at the highest priority possible. Up until now
> + * a pre-emption might not even have been attempted. So make sure
> + * this last attempt allows enough time for a pre-emption to occur.
> + */
> + longer = READ_ONCE(engine->props.preempt_timeout_ms) * 2;
> + if (longer > delay)
> + delay = longer;
> + }
> +
> if (!delay)
> return false;
>
next prev parent reply other threads:[~2022-02-22 11:19 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-18 21:33 [PATCH 0/3] Improve anti-pre-emption w/a for compute workloads John.C.Harrison
2022-02-18 21:33 ` [Intel-gfx] " John.C.Harrison
2022-02-18 21:33 ` [PATCH 1/3] drm/i915/guc: Limit scheduling properties to avoid overflow John.C.Harrison
2022-02-18 21:33 ` [Intel-gfx] " John.C.Harrison
2022-02-22 9:52 ` Tvrtko Ursulin
2022-02-22 10:39 ` Tvrtko Ursulin
2022-02-23 2:11 ` John Harrison
2022-02-23 12:13 ` Tvrtko Ursulin
2022-02-23 19:03 ` John Harrison
2022-02-24 9:59 ` Tvrtko Ursulin
2022-02-24 19:19 ` John Harrison
2022-02-24 19:51 ` John Harrison
2022-02-25 17:44 ` Tvrtko Ursulin
2022-02-25 17:06 ` Tvrtko Ursulin
2022-02-25 17:39 ` John Harrison
2022-02-28 16:11 ` Tvrtko Ursulin
2022-02-28 18:32 ` John Harrison
2022-03-01 10:50 ` Tvrtko Ursulin
2022-03-01 19:57 ` John Harrison
2022-03-02 9:20 ` Tvrtko Ursulin
2022-03-02 18:07 ` John Harrison
2022-02-23 0:52 ` Ceraolo Spurio, Daniele
2022-02-23 2:15 ` John Harrison
2022-02-18 21:33 ` [PATCH 2/3] drm/i915/gt: Make the heartbeat play nice with long pre-emption timeouts John.C.Harrison
2022-02-18 21:33 ` [Intel-gfx] " John.C.Harrison
2022-02-22 11:19 ` Tvrtko Ursulin [this message]
2022-02-23 2:45 ` John Harrison
2022-02-23 13:58 ` Tvrtko Ursulin
2022-02-23 20:00 ` John Harrison
2022-02-24 11:41 ` Tvrtko Ursulin
2022-02-24 19:45 ` John Harrison
2022-02-25 18:14 ` Tvrtko Ursulin
2022-02-25 18:48 ` John Harrison
2022-02-28 17:12 ` Tvrtko Ursulin
2022-02-28 18:55 ` John Harrison
2022-03-01 12:09 ` Tvrtko Ursulin
2022-03-01 20:59 ` John Harrison
2022-03-02 11:07 ` Tvrtko Ursulin
2022-03-02 17:55 ` John Harrison
2022-03-03 9:55 ` Tvrtko Ursulin
2022-03-03 19:09 ` John Harrison
2022-03-04 12:36 ` Tvrtko Ursulin
2022-02-18 21:33 ` [PATCH 3/3] drm/i915: Improve long running OCL w/a for GuC submission John.C.Harrison
2022-02-18 21:33 ` [Intel-gfx] " John.C.Harrison
2022-02-19 2:54 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for Improve anti-pre-emption w/a for compute workloads Patchwork
2022-02-19 3:33 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2022-02-22 9:53 ` [Intel-gfx] [PATCH 0/3] " Tvrtko Ursulin
2022-02-23 2:22 ` John Harrison
2022-02-23 12:00 ` Tvrtko Ursulin
2022-02-24 20:02 ` John Harrison
2022-02-25 16:36 ` Tvrtko Ursulin
2022-02-25 17:11 ` John Harrison
2022-02-25 17:39 ` Tvrtko Ursulin
2022-02-25 18:01 ` John Harrison
2022-02-25 18:29 ` Tvrtko Ursulin
2022-02-25 19:03 ` John Harrison
2022-02-28 15:32 ` Tvrtko Ursulin
2022-02-28 19:17 ` John Harrison
2022-03-02 11:21 ` Tvrtko Ursulin
2022-03-02 17:40 ` 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=2a486991-1bfd-9b23-0b43-9173d17b7e13@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=DRI-Devel@Lists.FreeDesktop.Org \
--cc=Intel-GFX@Lists.FreeDesktop.Org \
--cc=John.C.Harrison@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.