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 v5 3/4] drm/i915: Make the heartbeat play nice with long pre-emption timeouts
Date: Fri, 7 Oct 2022 09:44:40 +0100 [thread overview]
Message-ID: <c40f26ac-05e4-cb8d-8cd6-3354d8ce56bf@linux.intel.com> (raw)
In-Reply-To: <20221006213813.1563435-4-John.C.Harrison@Intel.com>
On 06/10/2022 22:38, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> Compute workloads are inherently 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
> magnitude.
>
> 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
> 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.
>
> v2: Fix for selftests which adjust the heartbeat period manually.
> v3: Add FIXME comment about selftests. Add extra FIXME comment and
> drm_notices when setting heartbeat to a non-default value (review
> feedback from Tvrtko)
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
> .../gpu/drm/i915/gt/intel_engine_heartbeat.c | 39 +++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> index a3698f611f457..9a527e1f5be65 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> @@ -22,9 +22,37 @@
>
> 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;
> +
> + /*
> + * FIXME: The final period extension is disabled if the period has been
> + * modified from the default. This is to prevent issues with certain
> + * selftests which override the value and expect specific behaviour.
> + * Once the selftests have been updated to either cope with variable
> + * heartbeat periods (or to override the pre-emption timeout as well,
> + * or just to add a selftest specific override of the extension), the
> + * generic override can be removed.
> + */
> + if (rq && rq->sched.attr.priority >= I915_PRIORITY_BARRIER &&
> + delay == engine->defaults.heartbeat_interval_ms) {
> + 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;
> + longer = intel_clamp_heartbeat_interval_ms(engine, longer);
> + if (longer > delay)
> + delay = longer;
> + }
> +
> if (!delay)
> return false;
>
> @@ -288,6 +316,17 @@ int intel_engine_set_heartbeat(struct intel_engine_cs *engine,
> if (!delay && !intel_engine_has_preempt_reset(engine))
> return -ENODEV;
>
> + /* FIXME: Remove together with equally marked hack in next_heartbeat. */
> + if (delay != engine->defaults.heartbeat_interval_ms &&
> + delay < 2 * engine->props.preempt_timeout_ms) {
> + if (intel_engine_uses_guc(engine))
> + drm_notice(&engine->i915->drm, "%s heartbeat interval adjusted to a non-default value which may downgrade individual engine resets to full GPU resets!\n",
> + engine->name);
> + else
> + drm_notice(&engine->i915->drm, "%s heartbeat interval adjusted to a non-default value which may cause engine resets to target innocent contexts!\n",
> + engine->name);
> + }
> +
> intel_engine_pm_get(engine);
>
> err = mutex_lock_interruptible(&ce->timeline->mutex);
LGTM - hope it is agreeable to you too.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
next prev parent reply other threads:[~2022-10-07 8:44 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-06 21:38 [PATCH v5 0/4] Improve anti-pre-emption w/a for compute workloads John.C.Harrison
2022-10-06 21:38 ` [PATCH v5 1/4] drm/i915/guc: Limit scheduling properties to avoid overflow John.C.Harrison
2022-10-18 0:06 ` Ceraolo Spurio, Daniele
2022-10-06 21:38 ` [PATCH v5 2/4] drm/i915: Fix compute pre-emption w/a to apply to compute engines John.C.Harrison
2022-10-06 21:38 ` [PATCH v5 3/4] drm/i915: Make the heartbeat play nice with long pre-emption timeouts John.C.Harrison
2022-10-07 8:44 ` Tvrtko Ursulin [this message]
2022-10-06 21:38 ` [PATCH v5 4/4] drm/i915: Improve long running compute w/a for GuC submission John.C.Harrison
2022-10-18 0:09 ` Ceraolo Spurio, Daniele
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=c40f26ac-05e4-cb8d-8cd6-3354d8ce56bf@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 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).