On 2/24/2022 11:19, John Harrison wrote:

[snip]

I'll change it to _uses_ and repost, then.

[    7.683149] kernel BUG at drivers/gpu/drm/i915/gt/uc/intel_guc.h:367!

Told you that one went bang.

John.

On 01/03/2022 19:57, John Harrison wrote:

On 3/1/2022 02:50, Tvrtko Ursulin wrote:

...

On 28/02/2022 18:32, John Harrison wrote:

...

On 2/28/2022 08:11, Tvrtko Ursulin wrote:

...

On 25/02/2022 17:39, John Harrison wrote:

...

On 2/25/2022 09:06, Tvrtko Ursulin wrote:

...

On 24/02/2022 19:19, John Harrison wrote:

[snip]

>>>>>> ./gt/uc/intel_guc_fwif.h: u32 execution_quantum; >>>>>> >>>>>> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = >>>>>> engine->props.timeslice_duration_ms * 1000; >>>>>> >>>>>> ./gt/intel_engine_types.h: unsigned long timeslice_duration_ms; >>>>>> >>>>>> timeslice_store/preempt_timeout_store: >>>>>> err = kstrtoull(buf, 0, &duration); >>>>>> >>>>>> So both kconfig and sysfs can already overflow GuC, not only >>>>>> because of tick conversion internally but because at backend >>>>>> level nothing was done for assigning 64-bit into 32-bit. Or >>>>>> I failed to find where it is handled. >>>>> That's why I'm adding this range check to make sure we don't >>>>> allow overflows. >>>> >>>> Yes and no, this fixes it, but the first bug was not only due >>>> GuC internal tick conversion. It was present ever since the >>>> u64 from i915 was shoved into u32 sent to GuC. So even if GuC >>>> used the value without additional multiplication, bug was be >>>> there. My point being when GuC backend was added timeout_ms >>>> values should have been limited/clamped to U32_MAX. The tick >>>> discovery is additional limit on top. >>> I'm not disagreeing. I'm just saying that the truncation wasn't >>> noticed until I actually tried using very long timeouts to >>> debug a particular problem. Now that it is noticed, we need >>> some method of range checking and this simple clamp solves all >>> the truncation problems. >> >> Agreed in principle, just please mention in the commit message >> all aspects of the problem. >> >> I think we can get away without a Fixes: tag since it requires >> user fiddling to break things in unexpected ways. >> >> I would though put in a code a clamping which expresses both, >> something like min(u32, ..GUC LIMIT..). So the full story is >> documented forever. Or "if > u32 || > ..GUC LIMIT..) return >> -EINVAL". Just in case GuC limit one day changes but u32 stays. >> Perhaps internal ticks go away or anything and we are left with >> plain 1:1 millisecond relationship. > Can certainly add a comment along the lines of "GuC API only > takes a 32bit field but that is further reduced to GUC_LIMIT due > to internal calculations which would otherwise overflow". > > But if the GuC limit is > u32 then, by definition, that means the > GuC API has changed to take a u64 instead of a u32. So there will > no u32 truncation any more. So I'm not seeing a need to > explicitly test the integer size when the value check covers that.

Hmm I was thinking if the internal conversion in the GuC fw changes so that GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS goes above u32, then to be extra safe by documenting in code there is the additional limit of the data structure field. Say the field was changed to take some unit larger than a millisecond. Then the check against the GuC MAX limit define would not be enough, unless that would account both for internal implementation and u32 in the protocol. Maybe that is overdefensive but I don't see that it harms. 50-50, but it's do it once and forget so I'd do it.

Huh?

How can the limit be greater than a u32 if the interface only takes a u32? By definition the limit would be clamped to u32 size.

If you mean that the GuC policy is in different units and those units might not overflow but ms units do, then actually that is already the case. The GuC works in us not ms. That's part of why the wrap around is so low, we have to multiply by 1000 before sending to GuC. However, that is actually irrelevant because the comparison is being done on the i915 side in i915's units. We have to scale the GuC limit to match what i915 is using. And the i915 side is u64 so if the scaling to i915 numbers overflows a u32 then who cares because that comparison can be done at 64 bits wide.

If the units change then that is a backwards breaking API change that will require a manual driver code update. You can't just recompile with a new header and magically get an ms to us or ms to s conversion in your a = b assignment. The code will need to be changed to do the new unit conversion (note we already convert from ms to us, the GuC API is all expressed in us). And that code change will mean having to revisit any and all scaling, type conversions, etc. I.e. any pre-existing checks will not necessarily be valid and will need to be re-visted anyway. But as above, any scaling to GuC units has to be incorporated into the limit already because otherwise the limit would not fit in the GuC's own API.

Yes I get that, I was just worried that u32 field in the protocol and GUC_POLICY_MAX_EXEC_QUANTUM_MS defines are separate in the source code and then how to protect against forgetting to update both in sync.

Like if the protocol was changed to take nanoseconds, and firmware implementation changed to support the full range, but define left/forgotten at 100s. That would then overflow u32.

Huh? If the API was updated to 'support the full range' then how can you get overflow by forgetting to update the limit? You could get unnecessary clamping, which hopefully would be noticed by whoever is testing the new API and/or whoever requested the change. But you can't get u32 overflow errors if all the code has been updated to u64.

Change the protocol so that "u32 desc->execution_quantum" now takes nano seconds.

This now makes the maximum time 4.29.. seconds.

You seriously think this is likely to happen?

That the GuC people would force an API change on us that is completely backwards from what we have been asking? And that such a massive backwards step would not get implemented correctly because someone didn't notice just how huge an impact it was?

I don't know what we have been asking or what GuC people would do.

...

Forget to update GUC_POLICY_MAX_EXEC_QUANTUM_MS from 100s, since for instance that part at that point still not part of the interface contract.

There is zero chance of the us -> ns change occurring in the foreseeable future whereas the expectation is to have the limits be part of the spec in the next firmware release. So this scenario is just not going to happen. And as above, it would be such a big change with such a huge amount of push back and discussion going on that it would be impossible for the limit update to be missed/forgotten.

...

User passes in 5 seconds.

Clamping check says all is good.

"engine->props.timeslice_duration_ms > GUC_POLICY_MAX_EXEC_QUANTUM_MS"

Assignment was updated:

gt/uc/intel_guc_submission.c:

desc->execution_quantum = engine->props.timeslice_duration_ms * 1e6;

But someone did not realize field is u32.

desc->execution_quantum = engine->props.timeslice_duration_ms * 1e6;

Defensive solution:

if (overflows_type(engine->props.timeslice_duration_ms * 1e6, desc->execution_quantum))     drm_WARN_ON...

All you are saying is that bugs can happen. The above is just one more place to have a bug.

The purpose of the limit is to take into account all reasons for there being a limit. Having a bunch of different tests that are all testing the same thing is pointless.

I am saying this:

1) The code I pointed out is a boundary layer between two components which have independent design and development teams.

2) The limit in question is currently not explicitly defined by the interface provider.

3) The limit in question is also implicitly defined by the hidden internal firmware implementation details not relating to the units of the interface.

4) The source code location of the clamping check is far away (different file, different layer) from the assignment to the interface data structure.

From this it sounds plausible to me to have the check at the assignment site and don't have to think about it further.

Regards,

Tvrtko

On 2/23/2022 05:58, Tvrtko Ursulin wrote:

On 23/02/2022 02:45, John Harrison wrote:

...

On 2/22/2022 03:19, Tvrtko Ursulin wrote:

...

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.)

I'm spotting 'inherently' but not anything else.

Magnititude! O;)

Doh!

[snip]

...

Whereas, bumping all heartbeat periods to be greater than the pre-emption timeout is wasteful and unnecessary. That leads to a total heartbeat time of about a minute. Which is a very long time to wait for a hang to be detected and recovered. Especially when the official limit on a context responding to an 'are you dead' query is only 7.5 seconds.

Not sure how did you get one minute?

7.5 * 2 (to be safe) = 15. 15 * 5 (number of heartbeat periods) = 75 => 1 minute 15 seconds

Even ignoring any safety factor and just going with 7.5 * 5 still gets you to 37.5 seconds which is over a half a minute and likely to race.

Regardless, crux of argument was to avoid GuC engine reset and heartbeat reset racing with each other, and to do that by considering the preempt timeout with the heartbeat interval. I was thinking about this scenario in this series:

[Please use fixed width font and no line wrap to view.]

A)

tP = preempt timeout tH = hearbeat interval

tP = 3 * tH

1. Background load = I915_PRIORITY_DISPLAY

<-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2 * tH] ----> FULL RESET                                                        |                                                        - preemption triggered, tP = 3 * tH ------\ -> preempt timeout would hit here

Here we have collateral damage due full reset, since we can't tell GuC to reset just one engine and we fudged tP just to "account" for heartbeats.

You are missing the whole point of the patch series which is that the last heartbeat period is '2 * tP' not '2 * tH'. +        longer = READ_ONCE(engine->props.preempt_timeout_ms) * 2;

By making the last period double the pre-emption timeout, it is guaranteed that the FULL RESET stage cannot be hit before the hardware has attempted and timed-out on at least one pre-emption.

[snip]

<-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2 * tH] ----> full reset would be here                |                - preemption triggered, tP = 3 * tH ----------------\ -> Preempt timeout reset

Here is is kind of least worse, but question is why we fudged tP when it gives us nothing good in this case.

The point of fudging tP(RCS) is to give compute workloads longer to reach a pre-emptible point (given that EU walkers are basically not pre-emptible). The reason for doing the fudge is not connected to the heartbeat at all. The fact that it causes problems for the heartbeat is an undesired side effect.

Note that the use of 'tP(RCS) = tH * 3' was just an arbitrary calculation that gave us something that all interested parties were vaguely happy with. It could just as easily be a fixed, hard coded value of 7.5s but having it based on something configurable seemed more sensible. The other option was 'tP(RCS) = tP * 12' but that felt more arbitrary than basing it on the average heartbeat timeout. As in, three heartbeat periods is about what a normal prio task gets before it gets pre-empted by the heartbeat. So using that for general purpose pre-emptions (e.g. time slicing between multiple user apps) seems reasonable.

B)

Instead, my idea to account for preempt timeout when calculating when to schedule next hearbeat would look like this:

First of all tP can be left at a large value unrelated to tH. Lets say tP = 640ms. tH stays 2.5s.

640ms is not 'large'. The requirement is either zero (disabled) or region of 7.5s. The 640ms figure is the default for non-compute engines. Anything that can run EUs needs to be 'huge'.

1. Background load = I915_PRIORITY_DISPLAY

<-- [tH + tP] --> Pulse1 <-- [tH + tP] --> Pulse2 <-- [tH + tP] --> Pulse3 <-- [tH + tP] --> full reset would be here

Sure, this works but each period is now 2.5 + 7.5 = 10s. The full five periods is therefore 50s, which is practically a minute.

[snip]

Am I missing some requirement or you see another problem with this idea?

...

...

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.

Define 'stops working'. The definition of the sysfs interface is that a value of zero disables pre-emption. If you don't have pre-emption and your hang detection mechanism relies on pre-emption then you don't have a hang detection mechanism either. If the user really wants to allow

By stops working I meant that it stops working. :)

With execlist one can disable preempt timeout and "stopped heartbeat" can still reset the stuck engine and so avoid collateral damage. With GuC it appears this is not possible. So I was thinking this is something worthy a log notice.

...

their context to run forever and never be pre-empted then that means they also don't want it to be reset arbitrarily. Which means they would also be disabling the heartbeat timer as well. Indeed, this is what we

I don't think so. Preempt timeout is disabled already on TGL/RCS upstream but hearbeat is not and so hangcheck still works.

The pre-emption disable in upstream is not a valid solution for compute customers. It is a worst-of-all-worlds hack for general usage. As noted already, any actual compute specific customer is advised to disable all forms of reset and do their hang detection manually. A slightly less worse hack for customers that are not actually running long compute workloads (i.e. the vast majority of end users) is to just use a long pre-emption timeout.

...

advise compute customers to do. It is then up to the user themselves to spot a hang and to manually kill (Ctrl+C, kill ###, etc.) their task. Killing the CPU task will automatically clear up any GPU resources allocated to that task (excepting context persistence, which is a) broken and b) something we also tell compute customers to disable).

What is broken with context persistence? I noticed one patch claiming to be fixing something in that area which looked suspect. Has it been established no userspace relies on it?

One major issue is that it has hooks into the execlist scheduler backend. I forget the exact details right now. The implementation as a whole is incredibly complex and convoluted :(. But there's stuff about what happens when you disable the heartbeat after having closed a persistence context's handle (and thus made it persisting). There's also things like it sends a super high priority heartbeat pulse at the point of becoming persisting. That plays havoc for platforms with dependent engines and/or compute workloads. A context becomes persisting on RCS and results in your unrealted CCS work being reset. It's a mess.

The comment from Daniel Vetter is that persistence should have no connection to the heartbeat at all. All of that dynamic behaviour and complexity should just be removed.

Persistence itself can stay. There are valid UMD use cases. It is just massively over complicated and doesn't work in all corner cases when not using execlist submission or on newer platforms. The simplification that is planned is to allow contexts to persist until the associated DRM master handle is closed. At that point, all contexts associated with that DRM handle are killed. That is what AMD and others apparently implement.

John.

Regards,

Tvrtko

On 2/24/2022 03:41, Tvrtko Ursulin wrote:

On 23/02/2022 20:00, John Harrison wrote:

...

On 2/23/2022 05:58, Tvrtko Ursulin wrote:

...

On 23/02/2022 02:45, John Harrison wrote:

...

On 2/22/2022 03:19, Tvrtko Ursulin wrote:

...

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.)

I'm spotting 'inherently' but not anything else.

Magnititude! O;)

Doh!

[snip]

...

...

Whereas, bumping all heartbeat periods to be greater than the pre-emption timeout is wasteful and unnecessary. That leads to a total heartbeat time of about a minute. Which is a very long time to wait for a hang to be detected and recovered. Especially when the official limit on a context responding to an 'are you dead' query is only 7.5 seconds.

Not sure how did you get one minute?

7.5 * 2 (to be safe) = 15. 15 * 5 (number of heartbeat periods) = 75 => 1 minute 15 seconds

Even ignoring any safety factor and just going with 7.5 * 5 still gets you to 37.5 seconds which is over a half a minute and likely to race.

Ah because my starting point is there should be no preempt timeout = heartbeat * 3, I just think that's too ugly.

Then complain at the hardware designers to give us mid-thread pre-emption back. The heartbeat is only one source of pre-emption events. For example, a user can be running multiple contexts in parallel and expecting them to time slice on a single engine. Or maybe a user is just running one compute task in the background but is doing render work in the foreground. Etc.

There was a reason the original hack was to disable pre-emption rather than increase the heartbeat. This is simply a slightly less ugly version of the same hack. And unfortunately, the basic idea of the hack is non-negotiable.

As per other comments, 'tP(RCS) = tH *3' or 'tP(RCS) = tP(default) * 12' or 'tP(RCS) = 7500' are the available options. Given that the heartbeat is the ever present hard limit, it seems most plausible to base the hack on that. Any of the others works, though. Although I think a explicit hardcoded value is the most ugly. I guess the other option is to add CONFIG_DRM_I915_PREEMPT_TIMEOUT_COMPUTE and default that to 7500.

Take your pick. But 640ms is not allowed.

...

...

Regardless, crux of argument was to avoid GuC engine reset and heartbeat reset racing with each other, and to do that by considering the preempt timeout with the heartbeat interval. I was thinking about this scenario in this series:

[Please use fixed width font and no line wrap to view.]

A)

tP = preempt timeout tH = hearbeat interval

tP = 3 * tH

1. Background load = I915_PRIORITY_DISPLAY

<-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2

• tH] ----> FULL RESET

|                                                        - preemption triggered, tP = 3 * tH ------\ -> preempt timeout would hit here

Here we have collateral damage due full reset, since we can't tell GuC to reset just one engine and we fudged tP just to "account" for heartbeats.

You are missing the whole point of the patch series which is that the last heartbeat period is '2 * tP' not '2 * tH'. +        longer = READ_ONCE(engine->props.preempt_timeout_ms) * 2;

By making the last period double the pre-emption timeout, it is guaranteed that the FULL RESET stage cannot be hit before the hardware has attempted and timed-out on at least one pre-emption.

Oh well :) that probably means the overall scheme is too odd for me. tp = 3tH and last pulse after 2tP I mean.

To be accurate, it is 'tP(RCS) = 3 * tH(default); tH(final) = tP(current) * 2;'. Seems fairly straight forward to me. It's not a recursive definition or anything like that. It gives us a total heartbeat timeout that is close to the original version but still allows at least one pre-emption event.

...

[snip]

...

<-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2

• tH] ----> full reset would be here

|                - preemption triggered, tP = 3 * tH ----------------\ -> Preempt timeout reset

Here is is kind of least worse, but question is why we fudged tP when it gives us nothing good in this case.

The point of fudging tP(RCS) is to give compute workloads longer to reach a pre-emptible point (given that EU walkers are basically not pre-emptible). The reason for doing the fudge is not connected to the heartbeat at all. The fact that it causes problems for the heartbeat is an undesired side effect.

Note that the use of 'tP(RCS) = tH * 3' was just an arbitrary calculation that gave us something that all interested parties were vaguely happy with. It could just as easily be a fixed, hard coded value of 7.5s but having it based on something configurable seemed more sensible. The other option was 'tP(RCS) = tP * 12' but that felt more arbitrary than basing it on the average heartbeat timeout. As in, three heartbeat periods is about what a normal prio task gets before it gets pre-empted by the heartbeat. So using that for general purpose pre-emptions (e.g. time slicing between multiple user apps) seems reasonable.

I think the fact you say tP fudge is not related to heartbeats and then go to mention heartbeat even in the "formula" which uses no tH is saying something (at least that's how I read the 7.5s option). :)

I said the tP fudge is not because of the heartbeat. It is obviously related.

As per comment above, the fudge factor is based on the heartbeat because the heartbeat is the ultimate limit. But the *reason* for the fudge fact has nothing to do with the heartbeat. It is required even if heartbeats are disabled.

...

...

B)

Instead, my idea to account for preempt timeout when calculating when to schedule next hearbeat would look like this:

First of all tP can be left at a large value unrelated to tH. Lets say tP = 640ms. tH stays 2.5s.

640ms is not 'large'. The requirement is either zero (disabled) or region of 7.5s. The 640ms figure is the default for non-compute engines. Anything that can run EUs needs to be 'huge'.

...

1. Background load = I915_PRIORITY_DISPLAY

<-- [tH + tP] --> Pulse1 <-- [tH + tP] --> Pulse2 <-- [tH + tP] --> Pulse3 <-- [tH + tP] --> full reset would be here

Sure, this works but each period is now 2.5 + 7.5 = 10s. The full five periods is therefore 50s, which is practically a minute.

No, in my proposal it is 3 * (2.5s + 640ms) =~ 9.3s.

Not good enough. After 2.5s, we send a pulse. After a further 640ms we perform an engine reset. That means your compute workload had only 640ms after being told to pre-empt to reach a pre-emption point. That won't work. It needs to be multiple seconds.

...

[snip]

...

Am I missing some requirement or you see another problem with this idea?

...

...

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.

Define 'stops working'. The definition of the sysfs interface is that a value of zero disables pre-emption. If you don't have pre-emption and your hang detection mechanism relies on pre-emption then you don't have a hang detection mechanism either. If the user really wants to allow

By stops working I meant that it stops working. :)

With execlist one can disable preempt timeout and "stopped heartbeat" can still reset the stuck engine and so avoid collateral damage. With GuC it appears this is not possible. So I was thinking this is something worthy a log notice.

...

their context to run forever and never be pre-empted then that means they also don't want it to be reset arbitrarily. Which means they would also be disabling the heartbeat timer as well. Indeed, this is what we

I don't think so. Preempt timeout is disabled already on TGL/RCS upstream but hearbeat is not and so hangcheck still works.

The pre-emption disable in upstream is not a valid solution for compute customers. It is a worst-of-all-worlds hack for general usage. As noted already, any actual compute specific customer is advised to disable all forms of reset and do their hang detection manually. A slightly less worse hack for customers that are not actually running long compute workloads (i.e. the vast majority of end users) is to just use a long pre-emption timeout.

If disabled preemption timeout is worst of all words and compute needs to disable heartbeat as well then why did we put it in? Perhaps it was not know at the time it would not be good enough. But anyway, do I read you correct that current thinking is it would be better to leave it at default 640ms?

No. We cannot have the RCS default to 640ms.

Note that there is a difference between 'general end user who might run some compute' and 'compute focused customer'. The driver defaults (disabled or 7500ms) are for the general user who gets the out-of-the-box experience and expects to be able to run 'normal' workloads without hitting problems. I.e. they expect hung tasks to get reset in a timely manner and while they might run some AI or other compute workloads, they are not a HPC datacenter. Whereas the compute datacenter customer expects their workloads to run for arbitrarily long times (minutes, hours, maybe even days) without being arbitrarily killed. Those customers will be explicitly configuring their datacenter server for that scenario and thus don't care what the defaults are.

If so, if we went with my proposal, would everyone be happy? If yes, isn't it a simpler scheme? No special casing when setting the preempt timeout, no special casing of the last heartbeat pulse. Works predictably regardless of the priority of the hypothetical non-preemptible workload.

No, we have to have the increased pre-emption timeout. And that has ripple effects of making very long heartbeats or risking races with the heartbeat beating the per engine reset.

...

...

...

advise compute customers to do. It is then up to the user themselves to spot a hang and to manually kill (Ctrl+C, kill ###, etc.) their task. Killing the CPU task will automatically clear up any GPU resources allocated to that task (excepting context persistence, which is a) broken and b) something we also tell compute customers to disable).

What is broken with context persistence? I noticed one patch claiming to be fixing something in that area which looked suspect. Has it been established no userspace relies on it?

One major issue is that it has hooks into the execlist scheduler backend. I forget the exact details right now. The implementation as a whole is incredibly complex and convoluted :(. But there's stuff about what happens when you disable the heartbeat after having closed a persistence context's handle (and thus made it persisting). There's also things like it sends a super high priority heartbeat pulse at the point of becoming persisting. That plays havoc for platforms with dependent engines and/or compute workloads. A context becomes persisting on RCS and results in your unrealted CCS work being reset. It's a mess.

The comment from Daniel Vetter is that persistence should have no connection to the heartbeat at all. All of that dynamic behaviour and complexity should just be removed.

Dependent engines is definitely a topic on it's own, outside hearbeats, persistence and all.

Except that it has implications for persistence which the current driver does not take into account.

Otherwise there is definitely complexity in the execlists backend but I am not sure if logic persistence and heartbeats are so very connected. It does send a pulse when heartbeat interval is changed, which if going to zero, it will kick of closed contexts if it can:

static struct intel_engine_cs * __execlists_schedule_in(struct i915_request *rq) {     struct intel_engine_cs * const engine = rq->engine;     struct intel_context * const ce = rq->context;

intel_context_get(ce);

if (unlikely(intel_context_is_closed(ce) &&              !intel_engine_has_heartbeat(engine)))         intel_context_set_banned(ce);

if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))         reset_active(rq, engine);

Is this what you mean? The point of this is to make sure persistent context does not hog the engine forever if hangcheck has been disabled.

Reminds me of my improvement to customer experience which never got in (https://patchwork.freedesktop.org/patch/451491/?series=93420&rev=2). Point of that one was to avoid engine reset (or worse) after user presses "Ctrl-C" if something takes just over 1ms to cleanly complete.

The plan is that the persistent contexts would still get the default grace period (pre-emption timeout at least) to finish but Ctrl+C will kill it within a timely manner if it does not finish.

...

Persistence itself can stay. There are valid UMD use cases. It is just massively over complicated and doesn't work in all corner cases when not using execlist submission or on newer platforms. The simplification that is planned is to allow contexts to persist until the associated DRM master handle is closed. At that point, all contexts associated with that DRM handle are killed. That is what AMD and others apparently implement.

Okay, that goes against one recent IGT patch which appeared to work around something by moving the position of _context_ close.

No it does not. The context close is not the trigger. The trigger is closing the top level DRM handle. If your context has persistence enabled (the default) then closing the context handle will have no effect. No pulse, no pre-emption, no kill, nothing. But when the top level handle is closed (application exit through whatever mechanism) then all GPU resources will be cleaned up. As above, with at least a pre-emption timeout grace period, but after that it is termination time.

The media use cases for persistence are all happy with this scheme. I don't actually recall if we got a reply back from the OGL people. They were definitely on the email thread/Jira task and did not complain. OCL obviously don't care as their first action is to explicitly disable persistence.

John.

Regards,

Tvrtko

On 2/25/2022 10:14, Tvrtko Ursulin wrote:

I'll try to simplify the discussion here:

On 24/02/2022 19:45, John Harrison wrote:

...

On 2/24/2022 03:41, Tvrtko Ursulin wrote:

...

On 23/02/2022 20:00, John Harrison wrote:

...

On 2/23/2022 05:58, Tvrtko Ursulin wrote:

...

On 23/02/2022 02:45, John Harrison wrote:

...

On 2/22/2022 03:19, Tvrtko Ursulin wrote: > 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.) I'm spotting 'inherently' but not anything else.

Magnititude! O;)

Doh!

[snip]

...

...

Whereas, bumping all heartbeat periods to be greater than the pre-emption timeout is wasteful and unnecessary. That leads to a total heartbeat time of about a minute. Which is a very long time to wait for a hang to be detected and recovered. Especially when the official limit on a context responding to an 'are you dead' query is only 7.5 seconds.

Not sure how did you get one minute?

7.5 * 2 (to be safe) = 15. 15 * 5 (number of heartbeat periods) = 75 => 1 minute 15 seconds

Even ignoring any safety factor and just going with 7.5 * 5 still gets you to 37.5 seconds which is over a half a minute and likely to race.

Ah because my starting point is there should be no preempt timeout = heartbeat * 3, I just think that's too ugly.

Then complain at the hardware designers to give us mid-thread pre-emption back. The heartbeat is only one source of pre-emption events. For example, a user can be running multiple contexts in parallel and expecting them to time slice on a single engine. Or maybe a user is just running one compute task in the background but is doing render work in the foreground. Etc.

There was a reason the original hack was to disable pre-emption rather than increase the heartbeat. This is simply a slightly less ugly version of the same hack. And unfortunately, the basic idea of the hack is non-negotiable.

As per other comments, 'tP(RCS) = tH *3' or 'tP(RCS) = tP(default) * 12' or 'tP(RCS) = 7500' are the available options. Given that the heartbeat is the ever present hard limit, it seems most plausible to base the hack on that. Any of the others works, though. Although I think a explicit hardcoded value is the most ugly. I guess the other option is to add CONFIG_DRM_I915_PREEMPT_TIMEOUT_COMPUTE and default that to 7500.

Take your pick. But 640ms is not allowed.

...

...

...

Regardless, crux of argument was to avoid GuC engine reset and heartbeat reset racing with each other, and to do that by considering the preempt timeout with the heartbeat interval. I was thinking about this scenario in this series:

[Please use fixed width font and no line wrap to view.]

A)

tP = preempt timeout tH = hearbeat interval

tP = 3 * tH

1. Background load = I915_PRIORITY_DISPLAY

<-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2 * tH] ----> FULL RESET                                                        |                                                        - preemption triggered, tP = 3 * tH ------\ -> preempt timeout would hit here

Here we have collateral damage due full reset, since we can't tell GuC to reset just one engine and we fudged tP just to "account" for heartbeats.

You are missing the whole point of the patch series which is that the last heartbeat period is '2 * tP' not '2 * tH'. +        longer = READ_ONCE(engine->props.preempt_timeout_ms) * 2;

By making the last period double the pre-emption timeout, it is guaranteed that the FULL RESET stage cannot be hit before the hardware has attempted and timed-out on at least one pre-emption.

Oh well :) that probably means the overall scheme is too odd for me. tp = 3tH and last pulse after 2tP I mean.

To be accurate, it is 'tP(RCS) = 3 * tH(default); tH(final) = tP(current) * 2;'. Seems fairly straight forward to me. It's not a recursive definition or anything like that. It gives us a total heartbeat timeout that is close to the original version but still allows at least one pre-emption event.

...

...

[snip]

...

<-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2 * tH] ----> full reset would be here                |                - preemption triggered, tP = 3 * tH ----------------\ -> Preempt timeout reset

Here is is kind of least worse, but question is why we fudged tP when it gives us nothing good in this case.

The point of fudging tP(RCS) is to give compute workloads longer to reach a pre-emptible point (given that EU walkers are basically not pre-emptible). The reason for doing the fudge is not connected to the heartbeat at all. The fact that it causes problems for the heartbeat is an undesired side effect.

Note that the use of 'tP(RCS) = tH * 3' was just an arbitrary calculation that gave us something that all interested parties were vaguely happy with. It could just as easily be a fixed, hard coded value of 7.5s but having it based on something configurable seemed more sensible. The other option was 'tP(RCS) = tP * 12' but that felt more arbitrary than basing it on the average heartbeat timeout. As in, three heartbeat periods is about what a normal prio task gets before it gets pre-empted by the heartbeat. So using that for general purpose pre-emptions (e.g. time slicing between multiple user apps) seems reasonable.

I think the fact you say tP fudge is not related to heartbeats and then go to mention heartbeat even in the "formula" which uses no tH is saying something (at least that's how I read the 7.5s option). :)

I said the tP fudge is not because of the heartbeat. It is obviously related.

As per comment above, the fudge factor is based on the heartbeat because the heartbeat is the ultimate limit. But the *reason* for the fudge fact has nothing to do with the heartbeat. It is required even if heartbeats are disabled.

...

...

...

B)

Instead, my idea to account for preempt timeout when calculating when to schedule next hearbeat would look like this:

First of all tP can be left at a large value unrelated to tH. Lets say tP = 640ms. tH stays 2.5s.

640ms is not 'large'. The requirement is either zero (disabled) or region of 7.5s. The 640ms figure is the default for non-compute engines. Anything that can run EUs needs to be 'huge'.

...

1. Background load = I915_PRIORITY_DISPLAY

<-- [tH + tP] --> Pulse1 <-- [tH + tP] --> Pulse2 <-- [tH + tP] --> Pulse3 <-- [tH + tP] --> full reset would be here

Sure, this works but each period is now 2.5 + 7.5 = 10s. The full five periods is therefore 50s, which is practically a minute.

No, in my proposal it is 3 * (2.5s + 640ms) =~ 9.3s.

Not good enough. After 2.5s, we send a pulse. After a further 640ms we perform an engine reset. That means your compute workload had only 640ms after being told to pre-empt to reach a pre-emption point. That won't work. It needs to be multiple seconds.

...

...

[snip]

...

Am I missing some requirement or you see another problem with this idea?

...

> 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. Define 'stops working'. The definition of the sysfs interface is that a value of zero disables pre-emption. If you don't have pre-emption and your hang detection mechanism relies on pre-emption then you don't have a hang detection mechanism either. If the user really wants to allow

By stops working I meant that it stops working. :)

With execlist one can disable preempt timeout and "stopped heartbeat" can still reset the stuck engine and so avoid collateral damage. With GuC it appears this is not possible. So I was thinking this is something worthy a log notice.

...

their context to run forever and never be pre-empted then that means they also don't want it to be reset arbitrarily. Which means they would also be disabling the heartbeat timer as well. Indeed, this is what we

I don't think so. Preempt timeout is disabled already on TGL/RCS upstream but hearbeat is not and so hangcheck still works.

The pre-emption disable in upstream is not a valid solution for compute customers. It is a worst-of-all-worlds hack for general usage. As noted already, any actual compute specific customer is advised to disable all forms of reset and do their hang detection manually. A slightly less worse hack for customers that are not actually running long compute workloads (i.e. the vast majority of end users) is to just use a long pre-emption timeout.

If disabled preemption timeout is worst of all words and compute needs to disable heartbeat as well then why did we put it in? Perhaps it was not know at the time it would not be good enough. But anyway, do I read you correct that current thinking is it would be better to leave it at default 640ms?

No. We cannot have the RCS default to 640ms.

Note that there is a difference between 'general end user who might run some compute' and 'compute focused customer'. The driver defaults (disabled or 7500ms) are for the general user who gets the out-of-the-box experience and expects to be able to run 'normal' workloads without hitting problems. I.e. they expect hung tasks to get reset in a timely manner and while they might run some AI or other compute workloads, they are not a HPC datacenter. Whereas the compute datacenter customer expects their workloads to run for arbitrarily long times (minutes, hours, maybe even days) without being arbitrarily killed. Those customers will be explicitly configuring their datacenter server for that scenario and thus don't care what the defaults are.

Okay maybe I misunderstood what you were saying earlier about worst of all worlds and all. But tell me this, if preemption timeout on RCS is not directly related to hearbeats, but to some pessimistic expected user workloads, what is wrong with my scheme of calculating the next heartbeat pulse as tH + tP?

We can leave tH as default 2.5s and tP you set for RCS to 12s if that is what you say is required. Or whatever long value really.

Your only objection is that ends up with too long total time before reset? Or something else as well?

An unnecessarily long total heartbeat timeout is the main objection. (2.5 + 12) * 5 = 72.5 seconds. That is a massive change from the current 12.5s.

If we are happy with that huge increase then fine. But I'm pretty sure you are going to get a lot more bug reports about hung systems not recovering. 10-20s is just about long enough for someone to wait before leaning on the power button of their machine. Over a minute is not. That kind of delay is going to cause support issues.

It's long but it is correct in a way. Because we can't expect hearbeat to react quicker than the interval + preempt timeout (or timeslice for equal priority) + some scheduling latency.

I conceptually disagree with the last hearbeat pulse being special. If the user concept is "after N heartbeats you are out" and you want to make it "after N-1 heartbeats plus 2 preemption periods you are out", where preemption period actually depends on heartbeat period, then that sounds really convoluted to me.

And we don't know which of the pulses will trigger preemption since user priority we don't control. So low priority compute task gets reset after 5s, normal priority gets to run for 12s. Effectively making context priority a variable in hangcheck.

It already is. That is no different. The pre-emption is not triggered until the pulse is of equal or higher priority than the busy task. That is the case no matter whether you are running GuC or execlist, whether you have the original driver or an updated one.

And it doesn't matter which pulse triggers the pre-emption. All that matters is that once a pre-emption is attempted, if the busy context fails to relinquish the hardware within the pre-emption timeout limit then it will be forcibly evicted.

...

...

If so, if we went with my proposal, would everyone be happy? If yes, isn't it a simpler scheme? No special casing when setting the preempt timeout, no special casing of the last heartbeat pulse. Works predictably regardless of the priority of the hypothetical non-preemptible workload.

No, we have to have the increased pre-emption timeout. And that has ripple effects of making very long heartbeats or risking races with the heartbeat beating the per engine reset.

...

...

...

...

advise compute customers to do. It is then up to the user themselves to spot a hang and to manually kill (Ctrl+C, kill ###, etc.) their task. Killing the CPU task will automatically clear up any GPU resources allocated to that task (excepting context persistence, which is a) broken and b) something we also tell compute customers to disable).

What is broken with context persistence? I noticed one patch claiming to be fixing something in that area which looked suspect. Has it been established no userspace relies on it?

One major issue is that it has hooks into the execlist scheduler backend. I forget the exact details right now. The implementation as a whole is incredibly complex and convoluted :(. But there's stuff about what happens when you disable the heartbeat after having closed a persistence context's handle (and thus made it persisting). There's also things like it sends a super high priority heartbeat pulse at the point of becoming persisting. That plays havoc for platforms with dependent engines and/or compute workloads. A context becomes persisting on RCS and results in your unrealted CCS work being reset. It's a mess.

The comment from Daniel Vetter is that persistence should have no connection to the heartbeat at all. All of that dynamic behaviour and complexity should just be removed.

Dependent engines is definitely a topic on it's own, outside hearbeats, persistence and all.

Except that it has implications for persistence which the current driver does not take into account.

Well current driver does not take RCS+CCS dependency into account so that should come first, or all in one package at least.

Not sure what you are arguing for here? Simplifying persistence as described will fix all the problems in one easy go. There is no point in adding yet more hideously complex code to make one corner case work when the real fix is to rip it all out.

...

...

Otherwise there is definitely complexity in the execlists backend but I am not sure if logic persistence and heartbeats are so very connected. It does send a pulse when heartbeat interval is changed, which if going to zero, it will kick of closed contexts if it can:

static struct intel_engine_cs * __execlists_schedule_in(struct i915_request *rq) {     struct intel_engine_cs * const engine = rq->engine;     struct intel_context * const ce = rq->context;

intel_context_get(ce);

if (unlikely(intel_context_is_closed(ce) &&              !intel_engine_has_heartbeat(engine)))         intel_context_set_banned(ce);

if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))         reset_active(rq, engine);

Is this what you mean? The point of this is to make sure persistent context does not hog the engine forever if hangcheck has been disabled.

Reminds me of my improvement to customer experience which never got in (https://patchwork.freedesktop.org/patch/451491/?series=93420&rev=2). Point of that one was to avoid engine reset (or worse) after user presses "Ctrl-C" if something takes just over 1ms to cleanly complete.

The plan is that the persistent contexts would still get the default grace period (pre-emption timeout at least) to finish but Ctrl+C will kill it within a timely manner if it does not finish.

Yes my patch does that. ;) Currently non-persistent is killed to quickly triggering pointless and alarming engine resets. Users reported this last year and I tried to fix it.

Except that your patch is adding yet more complexity to an already complex feature. The desire is to simplify the driver and make it more maintainable not complicated it further.

...

...

...

Persistence itself can stay. There are valid UMD use cases. It is just massively over complicated and doesn't work in all corner cases when not using execlist submission or on newer platforms. The simplification that is planned is to allow contexts to persist until the associated DRM master handle is closed. At that point, all contexts associated with that DRM handle are killed. That is what AMD and others apparently implement.

Okay, that goes against one recent IGT patch which appeared to work around something by moving the position of _context_ close.

No it does not. The context close is not the trigger. The trigger is

Well patch says: """ The spin all test relied on context persistence unecessarily by trying to destroy contexts while keeping spinners active. The current implementation of context persistence in i915 can cause failures with GuC enabled, and persistence is not needed for this test.

Moving intel_ctx_destroy after igt_spin_end. """

Implying moving context close to after spin end fixes things for GuC, not fd close.

That's because persistence is currently a big pile of poo and does not work in all the corner cases. The correct solution is to leave the IGT alone and just fix the implementation of persistence. However, the IGT update to not use the broken feature is a trivial test change (two lines?) whereas fixing the broken feature is a significant KMD re-work. It needs to be done but no-one currently has the time to do it. But trivially changing the test allows the test to work and test the features it is meant to be testing (which is not persistence).

John.

Regards,

Tvrtko

...

closing the top level DRM handle. If your context has persistence enabled (the default) then closing the context handle will have no effect. No pulse, no pre-emption, no kill, nothing. But when the top level handle is closed (application exit through whatever mechanism) then all GPU resources will be cleaned up. As above, with at least a pre-emption timeout grace period, but after that it is termination time.

The media use cases for persistence are all happy with this scheme. I don't actually recall if we got a reply back from the OGL people. They were definitely on the email thread/Jira task and did not complain. OCL obviously don't care as their first action is to explicitly disable persistence.

John.

...

Regards,

Tvrtko

On 2/28/2022 09:12, Tvrtko Ursulin wrote:

On 25/02/2022 18:48, John Harrison wrote:

...

On 2/25/2022 10:14, Tvrtko Ursulin wrote:

...

I'll try to simplify the discussion here:

On 24/02/2022 19:45, John Harrison wrote:

...

On 2/24/2022 03:41, Tvrtko Ursulin wrote:

...

On 23/02/2022 20:00, John Harrison wrote:

...

On 2/23/2022 05:58, Tvrtko Ursulin wrote: > On 23/02/2022 02:45, John Harrison wrote: >> On 2/22/2022 03:19, Tvrtko Ursulin wrote: >>> 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.) >> I'm spotting 'inherently' but not anything else. > > Magnititude! O;) Doh!

[snip]

>> Whereas, bumping all heartbeat periods to be greater than the >> pre-emption timeout is wasteful and unnecessary. That leads to >> a total heartbeat time of about a minute. Which is a very long >> time to wait for a hang to be detected and recovered. >> Especially when the official limit on a context responding to >> an 'are you dead' query is only 7.5 seconds. > > Not sure how did you get one minute? 7.5 * 2 (to be safe) = 15. 15 * 5 (number of heartbeat periods) = 75 => 1 minute 15 seconds

Even ignoring any safety factor and just going with 7.5 * 5 still gets you to 37.5 seconds which is over a half a minute and likely to race.

Ah because my starting point is there should be no preempt timeout = heartbeat * 3, I just think that's too ugly.

Then complain at the hardware designers to give us mid-thread pre-emption back. The heartbeat is only one source of pre-emption events. For example, a user can be running multiple contexts in parallel and expecting them to time slice on a single engine. Or maybe a user is just running one compute task in the background but is doing render work in the foreground. Etc.

There was a reason the original hack was to disable pre-emption rather than increase the heartbeat. This is simply a slightly less ugly version of the same hack. And unfortunately, the basic idea of the hack is non-negotiable.

As per other comments, 'tP(RCS) = tH *3' or 'tP(RCS) = tP(default)

• 12' or 'tP(RCS) = 7500' are the available options. Given that the

heartbeat is the ever present hard limit, it seems most plausible to base the hack on that. Any of the others works, though. Although I think a explicit hardcoded value is the most ugly. I guess the other option is to add CONFIG_DRM_I915_PREEMPT_TIMEOUT_COMPUTE and default that to 7500.

Take your pick. But 640ms is not allowed.

...

...

> Regardless, crux of argument was to avoid GuC engine reset and > heartbeat reset racing with each other, and to do that by > considering the preempt timeout with the heartbeat interval. I > was thinking about this scenario in this series: > > [Please use fixed width font and no line wrap to view.] > > A) > > tP = preempt timeout > tH = hearbeat interval > > tP = 3 * tH > > 1) Background load = I915_PRIORITY_DISPLAY > > <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 > <---- [2 * tH] ----> FULL RESET > | > - preemption triggered, tP = 3 * tH ------\ > -> preempt timeout would hit here > > Here we have collateral damage due full reset, since we can't > tell GuC to reset just one engine and we fudged tP just to > "account" for heartbeats. You are missing the whole point of the patch series which is that the last heartbeat period is '2 * tP' not '2 * tH'. +        longer = READ_ONCE(engine->props.preempt_timeout_ms) * 2;

By making the last period double the pre-emption timeout, it is guaranteed that the FULL RESET stage cannot be hit before the hardware has attempted and timed-out on at least one pre-emption.

Oh well :) that probably means the overall scheme is too odd for me. tp = 3tH and last pulse after 2tP I mean.

To be accurate, it is 'tP(RCS) = 3 * tH(default); tH(final) = tP(current) * 2;'. Seems fairly straight forward to me. It's not a recursive definition or anything like that. It gives us a total heartbeat timeout that is close to the original version but still allows at least one pre-emption event.

...

...

[snip]

> <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 > <---- [2 * tH] ----> full reset would be here >                | >                - preemption triggered, tP = 3 * tH > ----------------\ > -> Preempt timeout reset > > Here is is kind of least worse, but question is why we fudged tP > when it gives us nothing good in this case. > The point of fudging tP(RCS) is to give compute workloads longer to reach a pre-emptible point (given that EU walkers are basically not pre-emptible). The reason for doing the fudge is not connected to the heartbeat at all. The fact that it causes problems for the heartbeat is an undesired side effect.

Note that the use of 'tP(RCS) = tH * 3' was just an arbitrary calculation that gave us something that all interested parties were vaguely happy with. It could just as easily be a fixed, hard coded value of 7.5s but having it based on something configurable seemed more sensible. The other option was 'tP(RCS) = tP * 12' but that felt more arbitrary than basing it on the average heartbeat timeout. As in, three heartbeat periods is about what a normal prio task gets before it gets pre-empted by the heartbeat. So using that for general purpose pre-emptions (e.g. time slicing between multiple user apps) seems reasonable.

I think the fact you say tP fudge is not related to heartbeats and then go to mention heartbeat even in the "formula" which uses no tH is saying something (at least that's how I read the 7.5s option). :)

I said the tP fudge is not because of the heartbeat. It is obviously related.

As per comment above, the fudge factor is based on the heartbeat because the heartbeat is the ultimate limit. But the *reason* for the fudge fact has nothing to do with the heartbeat. It is required even if heartbeats are disabled.

...

...

> B) > > Instead, my idea to account for preempt timeout when calculating > when to schedule next hearbeat would look like this: > > First of all tP can be left at a large value unrelated to tH. > Lets say tP = 640ms. tH stays 2.5s. 640ms is not 'large'. The requirement is either zero (disabled) or region of 7.5s. The 640ms figure is the default for non-compute engines. Anything that can run EUs needs to be 'huge'.

> > 1) Background load = I915_PRIORITY_DISPLAY > > <-- [tH + tP] --> Pulse1 <-- [tH + tP] --> Pulse2 <-- [tH + tP] > --> Pulse3 <-- [tH + tP] --> full reset would be here Sure, this works but each period is now 2.5 + 7.5 = 10s. The full five periods is therefore 50s, which is practically a minute.

No, in my proposal it is 3 * (2.5s + 640ms) =~ 9.3s.

Not good enough. After 2.5s, we send a pulse. After a further 640ms we perform an engine reset. That means your compute workload had only 640ms after being told to pre-empt to reach a pre-emption point. That won't work. It needs to be multiple seconds.

...

...

[snip]

> Am I missing some requirement or you see another problem with > this idea? > >>> 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. >> Define 'stops working'. The definition of the sysfs interface >> is that a value of zero disables pre-emption. If you don't have >> pre-emption and your hang detection mechanism relies on >> pre-emption then you don't have a hang detection mechanism >> either. If the user really wants to allow > > By stops working I meant that it stops working. :) > > With execlist one can disable preempt timeout and "stopped > heartbeat" can still reset the stuck engine and so avoid > collateral damage. With GuC it appears this is not possible. So > I was thinking this is something worthy a log notice. > >> their context to run forever and never be pre-empted then that >> means they also don't want it to be reset arbitrarily. Which >> means they would also be disabling the heartbeat timer as well. >> Indeed, this is what we > > I don't think so. Preempt timeout is disabled already on TGL/RCS > upstream but hearbeat is not and so hangcheck still works. The pre-emption disable in upstream is not a valid solution for compute customers. It is a worst-of-all-worlds hack for general usage. As noted already, any actual compute specific customer is advised to disable all forms of reset and do their hang detection manually. A slightly less worse hack for customers that are not actually running long compute workloads (i.e. the vast majority of end users) is to just use a long pre-emption timeout.

If disabled preemption timeout is worst of all words and compute needs to disable heartbeat as well then why did we put it in? Perhaps it was not know at the time it would not be good enough. But anyway, do I read you correct that current thinking is it would be better to leave it at default 640ms?

No. We cannot have the RCS default to 640ms.

Note that there is a difference between 'general end user who might run some compute' and 'compute focused customer'. The driver defaults (disabled or 7500ms) are for the general user who gets the out-of-the-box experience and expects to be able to run 'normal' workloads without hitting problems. I.e. they expect hung tasks to get reset in a timely manner and while they might run some AI or other compute workloads, they are not a HPC datacenter. Whereas the compute datacenter customer expects their workloads to run for arbitrarily long times (minutes, hours, maybe even days) without being arbitrarily killed. Those customers will be explicitly configuring their datacenter server for that scenario and thus don't care what the defaults are.

Okay maybe I misunderstood what you were saying earlier about worst of all worlds and all. But tell me this, if preemption timeout on RCS is not directly related to hearbeats, but to some pessimistic expected user workloads, what is wrong with my scheme of calculating the next heartbeat pulse as tH + tP?

We can leave tH as default 2.5s and tP you set for RCS to 12s if that is what you say is required. Or whatever long value really.

Your only objection is that ends up with too long total time before reset? Or something else as well?

An unnecessarily long total heartbeat timeout is the main objection. (2.5 + 12) * 5 = 72.5 seconds. That is a massive change from the current 12.5s.

If we are happy with that huge increase then fine. But I'm pretty sure you are going to get a lot more bug reports about hung systems not recovering. 10-20s is just about long enough for someone to wait before leaning on the power button of their machine. Over a minute is not. That kind of delay is going to cause support issues.

Sorry I wrote 12s, while you actually said tP * 12, so 7.68s, chosen just so it is longer than tH * 3?

And how do you keep coming up with factor of five? Isn't it four periods before "heartbeat stopped"? (Prio normal, hearbeat, barrier and then reset.)

Prio starts at low not normal.

From the point of view of user experience I agree reasonable responsiveness is needed before user "reaches for the power button".

In your proposal we are talking about 3 * 2.5s + 2 * 7.5s, so 22.5s.

Question of workloads.. what is the actual preempt timeout compute is happy with? And I don't mean compute setups with disabled hangcheck, which you say they want anyway, but if we target defaults for end users. Do we have some numbers on what they are likely to run?

Not that I have ever seen. This is all just finger in the air stuff. I don't recall if we invented the number and the compute people agreed with it or if they proposed the number to us.

What if we gave them a default of 2.5s? That would be 4 * (2.5s + 2.5s) = 20s worst case until reset, comparable to your proposal. Are there realistic workloads which are non-preemptable for 2.5s? I am having hard time imagining someone would run them on a general purpose desktop since it would mean frozen and unusable UI anyway.

Advantage still being in my mind that there would be no fudging of timeouts during driver load and heartbeat periods depending on priority. To me it feels more plausible to account for preempt timeout in heartbeat pulses that to calculate preempt timeout to be longer than hearbeat pulses, just to avoid races between the two.

Except that when the user asks for a heartbeat period of 2.5s you are actually setting it to 5s. How is that not a major fudge that is totally disregarding the user's request?

...

...

It's long but it is correct in a way. Because we can't expect hearbeat to react quicker than the interval + preempt timeout (or timeslice for equal priority) + some scheduling latency.

I conceptually disagree with the last hearbeat pulse being special. If the user concept is "after N heartbeats you are out" and you want to make it "after N-1 heartbeats plus 2 preemption periods you are out", where preemption period actually depends on heartbeat period, then that sounds really convoluted to me.

And we don't know which of the pulses will trigger preemption since user priority we don't control. So low priority compute task gets reset after 5s, normal priority gets to run for 12s. Effectively making context priority a variable in hangcheck.

It already is. That is no different. The pre-emption is not triggered until the pulse is of equal or higher priority than the busy task. That is the case no matter whether you are running GuC or execlist, whether you have the original driver or an updated one.

And it doesn't matter which pulse triggers the pre-emption. All that matters is that once a pre-emption is attempted, if the busy context fails to relinquish the hardware within the pre-emption timeout limit then it will be forcibly evicted.

...

...

...

If so, if we went with my proposal, would everyone be happy? If yes, isn't it a simpler scheme? No special casing when setting the preempt timeout, no special casing of the last heartbeat pulse. Works predictably regardless of the priority of the hypothetical non-preemptible workload.

No, we have to have the increased pre-emption timeout. And that has ripple effects of making very long heartbeats or risking races with the heartbeat beating the per engine reset.

...

...

>> advise compute customers to do. It is then up to the user >> themselves to spot a hang and to manually kill (Ctrl+C, kill >> ###, etc.) their task. Killing the CPU task will automatically >> clear up any GPU resources allocated to that task (excepting >> context persistence, which is a) broken and b) something we >> also tell compute customers to disable). > > What is broken with context persistence? I noticed one patch > claiming to be fixing something in that area which looked > suspect. Has it been established no userspace relies on it? One major issue is that it has hooks into the execlist scheduler backend. I forget the exact details right now. The implementation as a whole is incredibly complex and convoluted :(. But there's stuff about what happens when you disable the heartbeat after having closed a persistence context's handle (and thus made it persisting). There's also things like it sends a super high priority heartbeat pulse at the point of becoming persisting. That plays havoc for platforms with dependent engines and/or compute workloads. A context becomes persisting on RCS and results in your unrealted CCS work being reset. It's a mess.

The comment from Daniel Vetter is that persistence should have no connection to the heartbeat at all. All of that dynamic behaviour and complexity should just be removed.

Dependent engines is definitely a topic on it's own, outside hearbeats, persistence and all.

Except that it has implications for persistence which the current driver does not take into account.

Well current driver does not take RCS+CCS dependency into account so that should come first, or all in one package at least.

Not sure what you are arguing for here? Simplifying persistence as described will fix all the problems in one easy go. There is no point in adding yet more hideously complex code to make one corner case work when the real fix is to rip it all out.

It sounded like you were arguing there was an issue with context persistence and dependent engines so my reply was that if there are no dependent engines in upstream there is no issue with persistence. Once dependent engines are brought upstream then any reset caused by persistence can be looked at and determined if there is any conceptual difference between that and any random reset.

There are issues with context persistence and *everything*. Dependent engines is just one of may problems with persistence.

What I am arguing for is ripping out all the hideously complex persistence code that it is extremely fragile and does not work with modern platforms. Instead, replacing it with some massively simpler, much more maintainable and compatible with all platforms - past, present and future.

...

...

...

...

Otherwise there is definitely complexity in the execlists backend but I am not sure if logic persistence and heartbeats are so very connected. It does send a pulse when heartbeat interval is changed, which if going to zero, it will kick of closed contexts if it can:

static struct intel_engine_cs * __execlists_schedule_in(struct i915_request *rq) {     struct intel_engine_cs * const engine = rq->engine;     struct intel_context * const ce = rq->context;

intel_context_get(ce);

if (unlikely(intel_context_is_closed(ce) &&              !intel_engine_has_heartbeat(engine)))         intel_context_set_banned(ce);

if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))         reset_active(rq, engine);

Is this what you mean? The point of this is to make sure persistent context does not hog the engine forever if hangcheck has been disabled.

Reminds me of my improvement to customer experience which never got in (https://patchwork.freedesktop.org/patch/451491/?series=93420&rev=2). Point of that one was to avoid engine reset (or worse) after user presses "Ctrl-C" if something takes just over 1ms to cleanly complete.

The plan is that the persistent contexts would still get the default grace period (pre-emption timeout at least) to finish but Ctrl+C will kill it within a timely manner if it does not finish.

Yes my patch does that. ;) Currently non-persistent is killed to quickly triggering pointless and alarming engine resets. Users reported this last year and I tried to fix it.

Except that your patch is adding yet more complexity to an already complex feature. The desire is to simplify the driver and make it more maintainable not complicated it further.

It is fixing an user reported issue and is not really adding complexity apart from enabling us to distinguish the reason for revoking a request (ban vs soft close). I think we should care to fix user reported issues in a reasonable time frame. It's a patch from last summer or so..

...

...

...

...

...

Persistence itself can stay. There are valid UMD use cases. It is just massively over complicated and doesn't work in all corner cases when not using execlist submission or on newer platforms. The simplification that is planned is to allow contexts to persist until the associated DRM master handle is closed. At that point, all contexts associated with that DRM handle are killed. That is what AMD and others apparently implement.

Okay, that goes against one recent IGT patch which appeared to work around something by moving the position of _context_ close.

No it does not. The context close is not the trigger. The trigger is

Well patch says: """ The spin all test relied on context persistence unecessarily by trying to destroy contexts while keeping spinners active. The current implementation of context persistence in i915 can cause failures with GuC enabled, and persistence is not needed for this test.

Moving intel_ctx_destroy after igt_spin_end. """

Implying moving context close to after spin end fixes things for GuC, not fd close.

That's because persistence is currently a big pile of poo and does not work in all the corner cases. The correct solution is to leave the IGT alone and just fix the implementation of persistence. However, the IGT update to not use the broken feature is a trivial test change (two lines?) whereas fixing the broken feature is a significant KMD re-work. It needs to be done but no-one currently has the time to do it. But trivially changing the test allows the test to work and test the features it is meant to be testing (which is not persistence).

Clear as mud. If the statement is that persistence cannot simply be removed, then for sure it cannot be said that anything can be fixed or unblocked by allowing some test to pass with GuC, by making them avoid using persistence (and not even explicitly with a context param). It implies persistence does not work with the GuC, which is then in contradiction with the statement that we cannot just remove persistence. I truly have no idea what the argument is here.

Persistence works in the right set of circumstances. Those circumstances do not involve dynamically changing heartbeat settings, platforms with dependent engines, etc. The correct fix is to leave the IGT test alone and fix the persistence implementation. However, that is not trivial and we have many other high priority holes still to plug. Whereas changing the IGT to not use a feature it is not intended to be testing anyway is a trivial change and gets us the test coverage of what that IGT is meant to be for.

John.

Regards,

Tvrtko

On 3/1/2022 04:09, Tvrtko Ursulin wrote:

I'll trim it a bit again..

On 28/02/2022 18:55, John Harrison wrote:

...

On 2/28/2022 09:12, Tvrtko Ursulin wrote:

...

On 25/02/2022 18:48, John Harrison wrote:

...

On 2/25/2022 10:14, Tvrtko Ursulin wrote:

[snip]

...

...

...

...

Your only objection is that ends up with too long total time before reset? Or something else as well?

An unnecessarily long total heartbeat timeout is the main objection. (2.5 + 12) * 5 = 72.5 seconds. That is a massive change from the current 12.5s.

If we are happy with that huge increase then fine. But I'm pretty sure you are going to get a lot more bug reports about hung systems not recovering. 10-20s is just about long enough for someone to wait before leaning on the power button of their machine. Over a minute is not. That kind of delay is going to cause support issues.

Sorry I wrote 12s, while you actually said tP * 12, so 7.68s, chosen just so it is longer than tH * 3?

And how do you keep coming up with factor of five? Isn't it four periods before "heartbeat stopped"? (Prio normal, hearbeat, barrier and then reset.)

Prio starts at low not normal.

Right, slipped my mind since I only keep seeing that one priority ladder block in intel_engine_heartbeat.c/heartbeat()..

...

...

From the point of view of user experience I agree reasonable responsiveness is needed before user "reaches for the power button".

In your proposal we are talking about 3 * 2.5s + 2 * 7.5s, so 22.5s.

Question of workloads.. what is the actual preempt timeout compute is happy with? And I don't mean compute setups with disabled hangcheck, which you say they want anyway, but if we target defaults for end users. Do we have some numbers on what they are likely to run?

Not that I have ever seen. This is all just finger in the air stuff. I don't recall if we invented the number and the compute people agreed with it or if they proposed the number to us.

Yeah me neither. And found nothing in my email archives. :(

Thinking about it today I don't see that disabled timeout is a practical default.

With it, if users have something un-preemptable to run (assuming prio normal), it would get killed after ~13s (5 * 2.5).

If we go for my scheme it gets killed in ~17.5s (3 * (2.5 + 2.5) + 2.5 (third pulse triggers preempt timeout)).

And if we go for your scheme it gets killed in ~22.5s (4 * 2.5 + 2 * 3

• 2.5).

Erm, that is not an apples to apples comparison. Your 17.5 is for an engine reset tripped by the pre-emption timeout, but your 22.5s is for a GT reset tripped by the heartbeat reaching the end and nuking the universe.

If you are saying that the first pulse at sufficient priority (third being normal prio) is what causes the reset because the system is working as expected and the pre-emption timeout trips the reset. In that case, you have two periods to get to normal prio plus one pre-emption timeout to trip the reset. I.e. (tH * 2) + tP.

Your scheme is then tH(actual) = tH(user) + tP, yes? So pre-emption based reset is after ((tH(user) + tP) * 2) + tP => (3 * tP) + (2 * tH) And GT based reset is after (tH(user) + tP) * 5 => (5 * tP) + (5 * tH)

My scheme is tH(actual) = tH(user) for first four, then max(tH(user), tP) for fifth. So pre-emption based reset is after tH(user) * 2 + tP = > tP + (2 * tH); And GT based reset is after (tH(user) * 4) + (max(tH(user), tP) * 1) => greater of ((4 * tH) + tP) or (5 * tH)

Either way your scheme is longer. With tH(user) = 2.5s, tP(RCS) = 7.5s, we get 27.5s for engine and 50s for GT versus my 12.5s for engine and 17.5s for GT. With tP(RCS) = 2.5s, yours is 12.5s for engine and 25s for GT versus my 7.5s for engine and 12.5s for GT.

Plus, not sure why your calculations above are using 2.5 for tP? Are you still arguing that 7.5s is too long? That is a separate issue and not related to the heartbeat algorithms. tP must be long enough to allow 'out of box OpenCL workloads to complete'. That doesn't just mean not being killed by the heartbeat, it also means not being killed by running two of them concurrently (or one plus desktop OpenGL rendering) and not having it killed by basic time slicing between the two contexts. The heartbeat is not involved in that process. That is purely the pre-emption timeout. And that is the fundamental reason why tP needs to be much larger on RCS/CCS.

If I did not confuse any calculation this time round, then the differences for default case for normal priority sound pretty immaterial to me.

...

...

What if we gave them a default of 2.5s? That would be 4 * (2.5s + 2.5s) = 20s worst case until reset, comparable to your proposal. Are there realistic workloads which are non-preemptable for 2.5s? I am having hard time imagining someone would run them on a general purpose desktop since it would mean frozen and unusable UI anyway.

Advantage still being in my mind that there would be no fudging of timeouts during driver load and heartbeat periods depending on priority. To me it feels more plausible to account for preempt timeout in heartbeat pulses that to calculate preempt timeout to be longer than hearbeat pulses, just to avoid races between the two.

Except that when the user asks for a heartbeat period of 2.5s you are actually setting it to 5s. How is that not a major fudge that is totally disregarding the user's request?

This is indeed the core question. My thinking:

It is not defined in the heartbeat ABI that N pulses should do anything, just that they are periodic pulses which check the health of an engine.

If we view user priority as not under our control then we can say that any heartbeat pulse can trigger preempt timeout and we should let it do that.

From that it follows that it is justified to account for preempt timeout in the decision when to schedule heartbeat pulses and that it is legitimate to do it for all of them.

But it can be optimised to say that it doesn't matter if you bump the priority of a pulse before waiting for the pre-emption period to expire. If the pulse was already high enough prio then the pre-emption has already been triggered and bumping the prio has no effect. If was too low then waiting for longer has no benefit at all.

All that matters is that you don't hit the end stop and trigger the GT reset too early.

It also avoids the double reset problem, regardless of the backend and regardless of how the user configured the timeouts. Without the need to fudge them neither during driver load or during sysfs store.

User has configured that heartbeat pulses should be sent every N seconds, yes, but I think we are free to account for inherent hardware and software latencies in our calculations. Especially since other than flawed Gen12 RCS, other engines will be much closer to the configured period.

It is just the same as user asking for preempt timeout N and we say on driver load "oh no you won't get it". Same for heartbeats, they said 2.5s, we said 2.5s + broken engine factor...

Why would you not get the pre-emption timeout? Because it is too large? That is a physical limitation (of the GuC firmware) not an override because we think we know better. If we can obey the user then we should do so.

I don't see a problem there. Nothing timing sensitive relies on the heartbeat interval nor we provided any guarantees.

That patch from Chris for instance AFAIR accounted for scheduling or context switch latencies. Because what is the point of sending further elevated priority pulses if we did not leave enough time to the engine to schedule them in, react with preemption, or signalling completion?

...

...

...

...

...

>> Persistence itself can stay. There are valid UMD use cases. It >> is just massively over complicated and doesn't work in all >> corner cases when not using execlist submission or on newer >> platforms. The simplification that is planned is to allow >> contexts to persist until the associated DRM master handle is >> closed. At that point, all contexts associated with that DRM >> handle are killed. That is what AMD and others apparently >> implement. > > Okay, that goes against one recent IGT patch which appeared to > work around something by moving the position of _context_ close. No it does not. The context close is not the trigger. The trigger is

Well patch says: """ The spin all test relied on context persistence unecessarily by trying to destroy contexts while keeping spinners active. The current implementation of context persistence in i915 can cause failures with GuC enabled, and persistence is not needed for this test.

Moving intel_ctx_destroy after igt_spin_end. """

Implying moving context close to after spin end fixes things for GuC, not fd close.

That's because persistence is currently a big pile of poo and does not work in all the corner cases. The correct solution is to leave the IGT alone and just fix the implementation of persistence. However, the IGT update to not use the broken feature is a trivial test change (two lines?) whereas fixing the broken feature is a significant KMD re-work. It needs to be done but no-one currently has the time to do it. But trivially changing the test allows the test to work and test the features it is meant to be testing (which is not persistence).

Clear as mud. If the statement is that persistence cannot simply be removed, then for sure it cannot be said that anything can be fixed or unblocked by allowing some test to pass with GuC, by making them avoid using persistence (and not even explicitly with a context param). It implies persistence does not work with the GuC, which is then in contradiction with the statement that we cannot just remove persistence. I truly have no idea what the argument is here.

Persistence works in the right set of circumstances. Those circumstances do not involve dynamically changing heartbeat settings, platforms with dependent engines, etc. The correct fix is to leave the IGT test alone and fix the persistence implementation. However, that is not trivial and we have many other high priority holes still to plug. Whereas changing the IGT to not use a feature it is not intended to be testing anyway is a trivial change and gets us the test coverage of what that IGT is meant to be for.

It may be acceptable if someone is reviewing overall coverage and making sure all is not removed and so a missing ABI in GuC backend not swept under the carpet. That's my main concern. If it is acknowledged persistence is a needed ABI, then how can we upstream dependent engine support without making sure this ABI is respected? Removing it's use from random tests does not fill me with confidence that we are on top of this topic.

Maybe if it didn't take three+ weeks to get a trivial change merged then I might have time to work on the non-trivial tasks that are in the backlog.

John.

Regards,

Tvrtko

On 3/2/2022 03:07, Tvrtko Ursulin wrote:

On 01/03/2022 20:59, John Harrison wrote:

...

On 3/1/2022 04:09, Tvrtko Ursulin wrote:

...

I'll trim it a bit again..

On 28/02/2022 18:55, John Harrison wrote:

...

On 2/28/2022 09:12, Tvrtko Ursulin wrote:

...

On 25/02/2022 18:48, John Harrison wrote:

...

On 2/25/2022 10:14, Tvrtko Ursulin wrote:

[snip]

...

...

...

> Your only objection is that ends up with too long total time > before reset? Or something else as well? An unnecessarily long total heartbeat timeout is the main objection. (2.5 + 12) * 5 = 72.5 seconds. That is a massive change from the current 12.5s.

If we are happy with that huge increase then fine. But I'm pretty sure you are going to get a lot more bug reports about hung systems not recovering. 10-20s is just about long enough for someone to wait before leaning on the power button of their machine. Over a minute is not. That kind of delay is going to cause support issues.

Sorry I wrote 12s, while you actually said tP * 12, so 7.68s, chosen just so it is longer than tH * 3?

And how do you keep coming up with factor of five? Isn't it four periods before "heartbeat stopped"? (Prio normal, hearbeat, barrier and then reset.)

Prio starts at low not normal.

Right, slipped my mind since I only keep seeing that one priority ladder block in intel_engine_heartbeat.c/heartbeat()..

...

...

From the point of view of user experience I agree reasonable responsiveness is needed before user "reaches for the power button".

In your proposal we are talking about 3 * 2.5s + 2 * 7.5s, so 22.5s.

Question of workloads.. what is the actual preempt timeout compute is happy with? And I don't mean compute setups with disabled hangcheck, which you say they want anyway, but if we target defaults for end users. Do we have some numbers on what they are likely to run?

Not that I have ever seen. This is all just finger in the air stuff. I don't recall if we invented the number and the compute people agreed with it or if they proposed the number to us.

Yeah me neither. And found nothing in my email archives. :(

Thinking about it today I don't see that disabled timeout is a practical default.

With it, if users have something un-preemptable to run (assuming prio normal), it would get killed after ~13s (5 * 2.5).

If we go for my scheme it gets killed in ~17.5s (3 * (2.5 + 2.5) + 2.5 (third pulse triggers preempt timeout)).

And if we go for your scheme it gets killed in ~22.5s (4 * 2.5 + 2 * 3 * 2.5).

Erm, that is not an apples to apples comparison. Your 17.5 is for an engine reset tripped by the pre-emption timeout, but your 22.5s is for a GT reset tripped by the heartbeat reaching the end and nuking the universe.

Right, in your scheme I did get it wrong. It would wait for GuC to reset the engine at the end, rather than hit the fake "hearbeat stopped" in that case, full reset path.

4 * 2.5 to trigger a max prio pulse, then 3 * 2.5 preempt timeout for GuC to reset (last hearbeat delay extended so it does not trigger). So 17.5 as well.

Again, apples or oranges? I was using your tP(RCS) == 2.5s assumption in all the above calculations given that the discussion was about the heartbeat algorithm, not the choice of pre-emption timeout. In which case the last heartbeat is max(tP * 2, tH) == 2 * 2.5s.

...

If you are saying that the first pulse at sufficient priority (third being normal prio) is what causes the reset because the system is working as expected and the pre-emption timeout trips the reset. In that case, you have two periods to get to normal prio plus one pre-emption timeout to trip the reset. I.e. (tH * 2) + tP.

Your scheme is then tH(actual) = tH(user) + tP, yes? So pre-emption based reset is after ((tH(user) + tP) * 2) + tP => (3

• tP) + (2 * tH)

And GT based reset is after (tH(user) + tP) * 5 => (5 * tP) + (5 * tH)

My scheme is tH(actual) = tH(user) for first four, then max(tH(user), tP) for fifth. So pre-emption based reset is after tH(user) * 2 + tP = > tP + (2 * tH); And GT based reset is after (tH(user) * 4) + (max(tH(user), tP) * 1) => greater of ((4 * tH) + tP) or (5 * tH)

Either way your scheme is longer. With tH(user) = 2.5s, tP(RCS) = 7.5s, we get 27.5s for engine and 50s for GT versus my 12.5s for engine and 17.5s for GT. With tP(RCS) = 2.5s, yours is 12.5s for engine and 25s for GT versus my 7.5s for engine and 12.5s for GT.

Plus, not sure why your calculations above are using 2.5 for tP? Are you still arguing that 7.5s is too long? That is a separate issue and not related to the heartbeat algorithms. tP must be long enough to allow 'out of box OpenCL workloads to complete'. That doesn't just mean not being killed by the heartbeat, it also means not being killed by running two of them concurrently (or one plus desktop OpenGL rendering) and not having it killed by basic time slicing between the two contexts. The heartbeat is not involved in that process. That is purely the pre-emption timeout. And that is the fundamental reason why tP needs to be much larger on RCS/CCS.

I was assuming 2.5s tP is enough and basing all calculation on that. Heartbeat or timeslicing regardless. I thought we established neither of us knows how long is enough.

Are you now saying 2.5s is definitely not enough? How is that usable for a default out of the box desktop?

Show me your proof that 2.5s is enough.

7.5s is what we have been using internally for a very long time. It has approval from all relevant parties. If you wish to pick a new random number then please provide data to back it up along with buy in from all UMD teams and project management.

...

...

If I did not confuse any calculation this time round, then the differences for default case for normal priority sound pretty immaterial to me.

...

...

What if we gave them a default of 2.5s? That would be 4 * (2.5s + 2.5s) = 20s worst case until reset, comparable to your proposal. Are there realistic workloads which are non-preemptable for 2.5s? I am having hard time imagining someone would run them on a general purpose desktop since it would mean frozen and unusable UI anyway.

Advantage still being in my mind that there would be no fudging of timeouts during driver load and heartbeat periods depending on priority. To me it feels more plausible to account for preempt timeout in heartbeat pulses that to calculate preempt timeout to be longer than hearbeat pulses, just to avoid races between the two.

Except that when the user asks for a heartbeat period of 2.5s you are actually setting it to 5s. How is that not a major fudge that is totally disregarding the user's request?

This is indeed the core question. My thinking:

It is not defined in the heartbeat ABI that N pulses should do anything, just that they are periodic pulses which check the health of an engine.

If we view user priority as not under our control then we can say that any heartbeat pulse can trigger preempt timeout and we should let it do that.

From that it follows that it is justified to account for preempt timeout in the decision when to schedule heartbeat pulses and that it is legitimate to do it for all of them.

But it can be optimised to say that it doesn't matter if you bump the priority of a pulse before waiting for the pre-emption period to expire. If the pulse was already high enough prio then the pre-emption has already been triggered and bumping the prio has no effect. If was too low then waiting for longer has no benefit at all.

All that matters is that you don't hit the end stop and trigger the GT reset too early.

Yes I agree that it can also be argued what you are saying.

I was trying to weigh pros&cons of both approaches by bringing into the discussing the question of what are heartbeats. Given they are loosely/vaguely defined I think we have freedom to tweak things.

And I don't have a problem with extending the last pulse. It is fundamentally correct to do regardless of the backend. I just raised the question of whether to extend all heartbeats to account for preemption (and scheduling delays). (What is the point of bumping their priority and re-scheduling if we didn't give enough time to the engine to react? So opposite of the question you raise.)

The point is that it we are giving enough time to react. Raising the priority of a pre-emption that has already been triggered will have no effect. So as long as the total time from when the pre-emption is triggered (prio becomes sufficiently high) to the point when the reset is decided is longer than the pre-emption timeout then it works. Given that, it is unnecessary to increase the intermediate periods. It has no advantage and has the disadvantage of making the total time unreasonably long.

So again, what is the point of making every period longer? What benefit does it *actually* give?

What I do have a problem with is deriving the preempt timeout from the hearbeat period. Hence I am looking if we can instead find a fixed number which works, and so avoid having bi-directional coupling.

Fine. "tP(RCS) = 7500;" can I merge the patch now?

...

...

It also avoids the double reset problem, regardless of the backend and regardless of how the user configured the timeouts. Without the need to fudge them neither during driver load or during sysfs store.

User has configured that heartbeat pulses should be sent every N seconds, yes, but I think we are free to account for inherent hardware and software latencies in our calculations. Especially since other than flawed Gen12 RCS, other engines will be much closer to the configured period.

It is just the same as user asking for preempt timeout N and we say on driver load "oh no you won't get it". Same for heartbeats, they said 2.5s, we said 2.5s + broken engine factor...

Why would you not get the pre-emption timeout? Because it is too large? That is a physical limitation (of the GuC firmware) not an override because we think we know better. If we can obey the user then we should do so.

I was simply referring to the override in intel_engine_setup.

Meaning the boost rather than the clamp? The other option would be to add a CONFIG_DRM_I915_PREEMPT_TIMEOUT_COMPUTE option for specifying tP(RCS). I can do that as a follow up patch if other maintainers agree to adding yet more CONFIG options. My understanding was that they are frowned upon and only to be added when there is no other way.

John.

Regards,

Tvrtko

On 3/3/2022 01:55, Tvrtko Ursulin wrote:

On 02/03/2022 17:55, John Harrison wrote:

...

...

I was assuming 2.5s tP is enough and basing all calculation on that. Heartbeat or timeslicing regardless. I thought we established neither of us knows how long is enough.

Are you now saying 2.5s is definitely not enough? How is that usable for a default out of the box desktop?

Show me your proof that 2.5s is enough.

7.5s is what we have been using internally for a very long time. It has approval from all relevant parties. If you wish to pick a new random number then please provide data to back it up along with buy in from all UMD teams and project management.

And upstream disabled preemption has acks from compute. "Internally" is as far away from out of the box desktop experiences we have been arguing about. In fact you have argued compute disables the hearbeat anyway.

Lets jump to the end of this reply please for actions.

...

...

And I don't have a problem with extending the last pulse. It is fundamentally correct to do regardless of the backend. I just raised the question of whether to extend all heartbeats to account for preemption (and scheduling delays). (What is the point of bumping their priority and re-scheduling if we didn't give enough time to the engine to react? So opposite of the question you raise.)

The point is that it we are giving enough time to react. Raising the priority of a pre-emption that has already been triggered will have no effect. So as long as the total time from when the pre-emption is triggered (prio becomes sufficiently high) to the point when the reset is decided is longer than the pre-emption timeout then it works. Given that, it is unnecessary to increase the intermediate periods. It has no advantage and has the disadvantage of making the total time unreasonably long.

So again, what is the point of making every period longer? What benefit does it *actually* give?

Less special casing and pointless prio bumps ahead of giving time to engine to even react. You wouldn't have to have the last pulse 2 * tP but normal tH + tP. So again, it is nicer for me to derive all heartbeat pulses from the same input parameters.

The whole "it is very long" argument is IMO moot because now proposed 7.5s preempt period is I suspect wholly impractical for desktop. Combined with the argument that real compute disables heartbeats anyway even extra so.

The whole thing is totally fubar already. Right now pre-emption is totally disabled. So you are currently waiting for the entire heartbeat sequence to complete and then nuking the entire machine. So arguing that 7.5s is too long is pointless. 7.5s is a big improvement over what is currently enabled.

And 'nice' would be having hardware that worked in a sensible manner. There is no nice here. There is only 'what is the least worst option'. And the least worst option for an end user is a long pre-emption timeout with a not massively long heartbeat. If that means a very slight complication in the heartbeat code, that is a trivial concern.

...

Fine. "tP(RCS) = 7500;" can I merge the patch now?

I could live with setting preempt timeout to 7.5s. The downside is slower time to restoring frozen desktops. Worst case today 5 * 2.5s, with changes 4 * 2.5s + 2 * 7.5s; so from 12.5s to 25s, doubling..

But that is worst case scenario (when something much more severe than an application hang has occurred). Regular case would be second heartbeat period + pre-emption timeout and an engine only reset not a full GT reset. So it's still better than what we have at present.

Actions:

Get a number from compute/OpenCL people for what they say is minimum preempt timeout for default out of the box Linux desktop experience.

That would be the one that has been agreed upon by both linux software arch and all UMD teams and has been in use for the past year or more in the internal tree.

This does not mean them running some tests and can't be bothered to setup up the machine for the extreme use cases, but workloads average users can realistically be expected to run.

Say for instance some image manipulation software which is OpenCL accelerated or similar. How long unpreemptable sections are expected there. Or similar. I am not familiar what all OpenCL accelerated use cases there are on Linux.

And this number should be purely about minimum preempt timeout, not considering heartbeats. This is because preempt timeout may kick in sooner than stopped heartbeat if the user workload is low priority.

And driver is simply hosed in the intervening six months or more that it takes for the right people to find the time to do this.

Right now, it is broken. This patch set improves things. Actual numbers can be refined later as/when some random use case that we hadn't previously thought of pops up. But not fixing the basic problem at all until we have an absolutely perfect for all parties solution is pointless. Not least because there is no perfect solution. No matter what number you pick it is going to be wrong for someone.

2.5s, 7.5s, X.Ys, I really don't care. 2.5s is a number you seem to have picked out of the air totally at random, or maybe based on it being the heartbeat period (except that you keep arguing that basing tP on tH is wrong). 7.5s is a number that has been in active use for a lot of testing for quite some time - KMD CI, UMD CI, E2E, etc. But either way, the initial number is almost irrelevant as long as it is not zero. So can we please just get something merged now as a starting point?

Commit message should explain the effect on the worst case time until engine reset.

OpenCL/compute should ack the change publicly as well since they acked the disabling of preemption.

This patch set has already been publicly acked by the compute team. See the 'acked-by' tag.

I really want overflows_type in the first patch.

In the final GuC assignment? Only if it is a BUG_ON. If we get a failure there it is an internal driver error and cannot be corrected for. It is too late for any plausible range check action.

And if you mean in the the actual helper function with the rest of the clamping then you are bleeding internal GuC API structure details into non-GuC code. Plus the test would be right next to the 'if (size < OFFICIAL_GUC_RANGE_LIMIT)' test which just looks dumb as well as being redundant duplication - "if ((value < GUC_LIMIT) && (value < NO_WE_REALLY_MEAN_IT_GUC_LIMIT))". And putting it inside the GuC limit definition looks even worse "#define LIMIT min(MAX_U32, 100*1000) /* because the developer doesn't know how big a u32 is */".

John.

My position is that with the above satisfied it is okay to merge.

Regards,

Tvrtko

On 2/22/2022 01:53, Tvrtko Ursulin wrote:

On 18/02/2022 21:33, John.C.Harrison@Intel.com wrote:

...

From: John Harrison John.C.Harrison@Intel.com

Compute workloads are inherently not pre-emptible on current hardware. Thus the pre-emption timeout was disabled as a workaround to prevent unwanted resets. Instead, the hang detection was left to the heartbeat and its (longer) timeout. This is undesirable with GuC submission as the heartbeat is a full GT reset rather than a per engine reset and so is much more destructive. Instead, just bump the pre-emption timeout

Can we have a feature request to allow asking GuC for an engine reset?

For what purpose?

GuC manages the scheduling of contexts across engines. With virtual engines, the KMD has no knowledge of which engine a context might be executing on. Even without virtual engines, the KMD still has no knowledge of which context is currently executing on any given engine at any given time.

There is a reason why hang detection should be left to the entity that is doing the scheduling. Any other entity is second guessing at best.

The reason for keeping the heartbeat around even when GuC submission is enabled is for the case where the KMD/GuC have got out of sync with either other somehow or GuC itself has just crashed. I.e. when no submission at all is working and we need to reset the GuC itself and start over.

John.

Regards,

Tvrtko

...

to a big value. Also, update the heartbeat to allow such a long pre-emption delay in the final heartbeat period.

Signed-off-by: John Harrison John.C.Harrison@Intel.com

John Harrison (3):    drm/i915/guc: Limit scheduling properties to avoid overflow    drm/i915/gt: Make the heartbeat play nice with long pre-emption      timeouts    drm/i915: Improve long running OCL w/a for GuC submission

drivers/gpu/drm/i915/gt/intel_engine_cs.c     | 37 +++++++++++++++++--   .../gpu/drm/i915/gt/intel_engine_heartbeat.c  | 16 ++++++++   drivers/gpu/drm/i915/gt/sysfs_engines.c       | 14 +++++++   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |  9 +++++   4 files changed, 73 insertions(+), 3 deletions(-)

On 2/23/2022 04:00, Tvrtko Ursulin wrote:

On 23/02/2022 02:22, John Harrison wrote:

...

On 2/22/2022 01:53, Tvrtko Ursulin wrote:

...

On 18/02/2022 21:33, John.C.Harrison@Intel.com wrote:

...

From: John Harrison John.C.Harrison@Intel.com

Compute workloads are inherently not pre-emptible on current hardware. Thus the pre-emption timeout was disabled as a workaround to prevent unwanted resets. Instead, the hang detection was left to the heartbeat and its (longer) timeout. This is undesirable with GuC submission as the heartbeat is a full GT reset rather than a per engine reset and so is much more destructive. Instead, just bump the pre-emption timeout

Can we have a feature request to allow asking GuC for an engine reset?

For what purpose?

To allow "stopped heartbeat" to reset the engine, however..

...

GuC manages the scheduling of contexts across engines. With virtual engines, the KMD has no knowledge of which engine a context might be executing on. Even without virtual engines, the KMD still has no knowledge of which context is currently executing on any given engine at any given time.

There is a reason why hang detection should be left to the entity that is doing the scheduling. Any other entity is second guessing at best.

The reason for keeping the heartbeat around even when GuC submission is enabled is for the case where the KMD/GuC have got out of sync with either other somehow or GuC itself has just crashed. I.e. when no submission at all is working and we need to reset the GuC itself and start over.

.. I wasn't really up to speed to know/remember heartbeats are nerfed already in GuC mode.

Not sure what you mean by that claim. Engine resets are handled by GuC because GuC handles the scheduling. You can't do the former if you aren't doing the latter. However, the heartbeat is still present and is still the watchdog by which engine resets are triggered. As per the rest of the submission process, the hang detection and recovery is split between i915 and GuC.

I am not sure it was the best way since full reset penalizes everyone for one hanging engine. Better question would be why leave heartbeats around to start with with GuC? If you want to use it to health check GuC, as you say, maybe just send a CT message and expect replies? Then full reset would make sense. It also achieves the goal of not seconding guessing the submission backend you raise.

Adding yet another hang detection mechanism is yet more complication and is unnecessary when we already have one that works well enough. As above, the heartbeat is still required for sending the pulses that cause pre-emptions and so let GuC detect hangs. It also provides a fallback against a dead GuC by default. So why invent yet another wheel?

Like it is now, and the need for this series demonstrates it, the whole thing has a pretty poor "impedance" match. Not even sure what intel_guc_find_hung_context is doing in intel_engine_hearbeat.c - why is that not in intel_gt_handle_error at least? Why is hearbeat code special and other callers of intel_gt_handle_error don't need it?

There is no guilty context if the reset was triggered via debugfs or similar. And as stated ad nauseam, i915 is no longer handling the scheduling and so cannot make assumptions about what is or is not running on the hardware at any given time. And obviously, if the reset initiated by GuC itself then i915 should not be second guessing the guilty context as the GuC notification has already told us who was responsible.

And to be clear, the 'poor impedance match' is purely because we don't have mid-thread pre-emption and so need a stupidly huge timeout on compute capable engines. Whereas, we don't want a total heatbeat timeout of a minute or more. That is the impedance mis-match. If the 640ms was acceptable for RCS then none of this hacky timeout algorithm mush would be needed.

John.

Regards,

Tvrtko

On 2/25/2022 08:36, Tvrtko Ursulin wrote:

On 24/02/2022 20:02, John Harrison wrote:

...

On 2/23/2022 04:00, Tvrtko Ursulin wrote:

...

On 23/02/2022 02:22, John Harrison wrote:

...

On 2/22/2022 01:53, Tvrtko Ursulin wrote:

...

On 18/02/2022 21:33, John.C.Harrison@Intel.com wrote:

...

From: John Harrison John.C.Harrison@Intel.com

Compute workloads are inherently not pre-emptible on current hardware. Thus the pre-emption timeout was disabled as a workaround to prevent unwanted resets. Instead, the hang detection was left to the heartbeat and its (longer) timeout. This is undesirable with GuC submission as the heartbeat is a full GT reset rather than a per engine reset and so is much more destructive. Instead, just bump the pre-emption timeout

Can we have a feature request to allow asking GuC for an engine reset?

For what purpose?

To allow "stopped heartbeat" to reset the engine, however..

...

GuC manages the scheduling of contexts across engines. With virtual engines, the KMD has no knowledge of which engine a context might be executing on. Even without virtual engines, the KMD still has no knowledge of which context is currently executing on any given engine at any given time.

There is a reason why hang detection should be left to the entity that is doing the scheduling. Any other entity is second guessing at best.

The reason for keeping the heartbeat around even when GuC submission is enabled is for the case where the KMD/GuC have got out of sync with either other somehow or GuC itself has just crashed. I.e. when no submission at all is working and we need to reset the GuC itself and start over.

.. I wasn't really up to speed to know/remember heartbeats are nerfed already in GuC mode.

Not sure what you mean by that claim. Engine resets are handled by GuC because GuC handles the scheduling. You can't do the former if you aren't doing the latter. However, the heartbeat is still present and is still the watchdog by which engine resets are triggered. As per the rest of the submission process, the hang detection and recovery is split between i915 and GuC.

I meant that "stopped heartbeat on engine XXX" can only do a full GPU reset on GuC.

I mean that there is no 'stopped heartbeat on engine XXX' when i915 is not handling the recovery part of the process.

intel_gt_handle_error(engine->gt, engine->mask,                   I915_ERROR_CAPTURE,                   "stopped heartbeat on %s",                   engine->name);

intel_gt_handle_error:

/*      * Try engine reset when available. We fall back to full reset if      * single reset fails.      */     if (!intel_uc_uses_guc_submission(&gt->uc) &&         intel_has_reset_engine(gt) && !intel_gt_is_wedged(gt)) {         local_bh_disable();         for_each_engine_masked(engine, gt, engine_mask, tmp) {

You said "However, the heartbeat is still present and is still the watchdog by which engine resets are triggered", now I don't know what you meant by this. It actually triggers a single engine reset in GuC mode? Where in code does that happen if this block above shows it not taking the engine reset path?

i915 sends down the per engine pulse. GuC schedules the pulse GuC attempts to pre-empt the currently active context GuC detects the pre-emption timeout GuC resets the engine

The fundamental process is exactly the same as in execlist mode. It's just that the above blocks of code (calls to intel_gt_handle_error and such) are now inside the GuC not i915.

Without the heartbeat going ping, there would be no context switching and thus no pre-emption, no pre-emption timeout and so no hang and reset recovery. And GuC cannot sent pulses by itself - it has no ability to generate context workloads. So we need i915 to send the pings and to gradually raise their priority. But the back half of the heartbeat code is now inside the GuC. It will simply never reach the i915 side timeout if GuC is doing the recovery (unless the heartbeat's final period is too short). We should only reach the i915 side timeout if GuC itself is toast. At which point we need the full GT reset to recover the GuC.

John.

...

...

I am not sure it was the best way since full reset penalizes everyone for one hanging engine. Better question would be why leave heartbeats around to start with with GuC? If you want to use it to health check GuC, as you say, maybe just send a CT message and expect replies? Then full reset would make sense. It also achieves the goal of not seconding guessing the submission backend you raise.

Adding yet another hang detection mechanism is yet more complication and is unnecessary when we already have one that works well enough. As above, the heartbeat is still required for sending the pulses that cause pre-emptions and so let GuC detect hangs. It also provides a fallback against a dead GuC by default. So why invent yet another wheel?

Lets first clarify the previous block to make sure there aren't any misunderstandings there.

Regards,

Tvrtko

...

...

Like it is now, and the need for this series demonstrates it, the whole thing has a pretty poor "impedance" match. Not even sure what intel_guc_find_hung_context is doing in intel_engine_hearbeat.c - why is that not in intel_gt_handle_error at least? Why is hearbeat code special and other callers of intel_gt_handle_error don't need it?

There is no guilty context if the reset was triggered via debugfs or similar. And as stated ad nauseam, i915 is no longer handling the scheduling and so cannot make assumptions about what is or is not running on the hardware at any given time. And obviously, if the reset initiated by GuC itself then i915 should not be second guessing the guilty context as the GuC notification has already told us who was responsible.

And to be clear, the 'poor impedance match' is purely because we don't have mid-thread pre-emption and so need a stupidly huge timeout on compute capable engines. Whereas, we don't want a total heatbeat timeout of a minute or more. That is the impedance mis-match. If the 640ms was acceptable for RCS then none of this hacky timeout algorithm mush would be needed.

John.

...

Regards,

Tvrtko

On 2/25/2022 09:39, Tvrtko Ursulin wrote:

On 25/02/2022 17:11, John Harrison wrote:

...

On 2/25/2022 08:36, Tvrtko Ursulin wrote:

...

On 24/02/2022 20:02, John Harrison wrote:

...

On 2/23/2022 04:00, Tvrtko Ursulin wrote:

...

On 23/02/2022 02:22, John Harrison wrote:

...

On 2/22/2022 01:53, Tvrtko Ursulin wrote: > On 18/02/2022 21:33, John.C.Harrison@Intel.com wrote: >> From: John Harrison John.C.Harrison@Intel.com >> >> Compute workloads are inherently not pre-emptible on current >> hardware. >> Thus the pre-emption timeout was disabled as a workaround to >> prevent >> unwanted resets. Instead, the hang detection was left to the >> heartbeat >> and its (longer) timeout. This is undesirable with GuC >> submission as >> the heartbeat is a full GT reset rather than a per engine reset >> and so >> is much more destructive. Instead, just bump the pre-emption >> timeout > > Can we have a feature request to allow asking GuC for an engine > reset? For what purpose?

To allow "stopped heartbeat" to reset the engine, however..

...

GuC manages the scheduling of contexts across engines. With virtual engines, the KMD has no knowledge of which engine a context might be executing on. Even without virtual engines, the KMD still has no knowledge of which context is currently executing on any given engine at any given time.

There is a reason why hang detection should be left to the entity that is doing the scheduling. Any other entity is second guessing at best.

The reason for keeping the heartbeat around even when GuC submission is enabled is for the case where the KMD/GuC have got out of sync with either other somehow or GuC itself has just crashed. I.e. when no submission at all is working and we need to reset the GuC itself and start over.

.. I wasn't really up to speed to know/remember heartbeats are nerfed already in GuC mode.

Not sure what you mean by that claim. Engine resets are handled by GuC because GuC handles the scheduling. You can't do the former if you aren't doing the latter. However, the heartbeat is still present and is still the watchdog by which engine resets are triggered. As per the rest of the submission process, the hang detection and recovery is split between i915 and GuC.

I meant that "stopped heartbeat on engine XXX" can only do a full GPU reset on GuC.

I mean that there is no 'stopped heartbeat on engine XXX' when i915 is not handling the recovery part of the process.

Hmmmm?

static void reset_engine(struct intel_engine_cs *engine, struct i915_request *rq) {     if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))         show_heartbeat(rq, engine);

if (intel_engine_uses_guc(engine))         /*          * GuC itself is toast or GuC's hang detection          * is disabled. Either way, need to find the          * hang culprit manually.          */         intel_guc_find_hung_context(engine);

intel_gt_handle_error(engine->gt, engine->mask,                   I915_ERROR_CAPTURE,                   "stopped heartbeat on %s",                   engine->name); }

How there is no "stopped hearbeat" in guc mode? From this code it certainly looks there is.

Only when the GuC is toast and it is no longer an engine reset but a full GT reset that is required. So technically, it is not a 'stopped heartbeat on engine XXX' it is 'stopped heartbeat on GT#'.

You say below heartbeats are going in GuC mode. Now I totally don't understand how they are going but there is allegedly no "stopped hearbeat".

Because if GuC is handling the detection and recovery then i915 will not reach that point. GuC will do the engine reset and start scheduling the next context before the heartbeat period expires. So the notification will be a G2H about a specific context being reset rather than the i915 notification about a stopped heartbeat.

...

...

intel_gt_handle_error(engine->gt, engine->mask,                   I915_ERROR_CAPTURE,                   "stopped heartbeat on %s",                   engine->name);

intel_gt_handle_error:

/*      * Try engine reset when available. We fall back to full reset if      * single reset fails.      */     if (!intel_uc_uses_guc_submission(&gt->uc) &&         intel_has_reset_engine(gt) && !intel_gt_is_wedged(gt)) {         local_bh_disable();         for_each_engine_masked(engine, gt, engine_mask, tmp) {

You said "However, the heartbeat is still present and is still the watchdog by which engine resets are triggered", now I don't know what you meant by this. It actually triggers a single engine reset in GuC mode? Where in code does that happen if this block above shows it not taking the engine reset path?

i915 sends down the per engine pulse. GuC schedules the pulse GuC attempts to pre-empt the currently active context GuC detects the pre-emption timeout GuC resets the engine

The fundamental process is exactly the same as in execlist mode. It's just that the above blocks of code (calls to intel_gt_handle_error and such) are now inside the GuC not i915.

Without the heartbeat going ping, there would be no context switching and thus no pre-emption, no pre-emption timeout and so no hang and reset recovery. And GuC cannot sent pulses by itself - it has no ability to generate context workloads. So we need i915 to send the pings and to gradually raise their priority. But the back half of the heartbeat code is now inside the GuC. It will simply never reach the i915 side timeout if GuC is doing the recovery (unless the heartbeat's final period is too short). We should only reach the i915 side timeout if GuC itself is toast. At which point we need the full GT reset to recover the GuC.

If workload is not preempting and reset does not work, like engine is truly stuck, does the current code hit "stopped heartbeat" or not in GuC mode?

Hang on, where did 'reset does not work' come into this?

If GuC is alive and the hardware is not broken then no, it won't. That's the whole point. GuC does the detection and recovery. The KMD will never reach 'stopped heartbeat'.

If the hardware is broken and the reset does not work then GuC will send a 'failed reset' notification to the KMD. The KMD treats that as a major error and immediately does a full GT reset. So there is still no 'stopped heartbeat'.

If GuC has died (or a KMD bug has caused sufficient confusion to make it think the GuC has died) then yes, you will reach that code. But in that case it is not an engine reset that is required, it is a full GT reset including a reset of the GuC.

Regards,

Tvrtko

On 2/25/2022 10:29, Tvrtko Ursulin wrote:

On 25/02/2022 18:01, John Harrison wrote:

...

On 2/25/2022 09:39, Tvrtko Ursulin wrote:

...

On 25/02/2022 17:11, John Harrison wrote:

...

On 2/25/2022 08:36, Tvrtko Ursulin wrote:

...

On 24/02/2022 20:02, John Harrison wrote:

...

On 2/23/2022 04:00, Tvrtko Ursulin wrote: > On 23/02/2022 02:22, John Harrison wrote: >> On 2/22/2022 01:53, Tvrtko Ursulin wrote: >>> On 18/02/2022 21:33, John.C.Harrison@Intel.com wrote: >>>> From: John Harrison John.C.Harrison@Intel.com >>>> >>>> Compute workloads are inherently not pre-emptible on current >>>> hardware. >>>> Thus the pre-emption timeout was disabled as a workaround to >>>> prevent >>>> unwanted resets. Instead, the hang detection was left to the >>>> heartbeat >>>> and its (longer) timeout. This is undesirable with GuC >>>> submission as >>>> the heartbeat is a full GT reset rather than a per engine >>>> reset and so >>>> is much more destructive. Instead, just bump the pre-emption >>>> timeout >>> >>> Can we have a feature request to allow asking GuC for an >>> engine reset? >> For what purpose? > > To allow "stopped heartbeat" to reset the engine, however.. > >> GuC manages the scheduling of contexts across engines. With >> virtual engines, the KMD has no knowledge of which engine a >> context might be executing on. Even without virtual engines, >> the KMD still has no knowledge of which context is currently >> executing on any given engine at any given time. >> >> There is a reason why hang detection should be left to the >> entity that is doing the scheduling. Any other entity is second >> guessing at best. >> >> The reason for keeping the heartbeat around even when GuC >> submission is enabled is for the case where the KMD/GuC have >> got out of sync with either other somehow or GuC itself has >> just crashed. I.e. when no submission at all is working and we >> need to reset the GuC itself and start over. > > .. I wasn't really up to speed to know/remember heartbeats are > nerfed already in GuC mode. Not sure what you mean by that claim. Engine resets are handled by GuC because GuC handles the scheduling. You can't do the former if you aren't doing the latter. However, the heartbeat is still present and is still the watchdog by which engine resets are triggered. As per the rest of the submission process, the hang detection and recovery is split between i915 and GuC.

I meant that "stopped heartbeat on engine XXX" can only do a full GPU reset on GuC.

I mean that there is no 'stopped heartbeat on engine XXX' when i915 is not handling the recovery part of the process.

Hmmmm?

static void reset_engine(struct intel_engine_cs *engine, struct i915_request *rq) {     if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))         show_heartbeat(rq, engine);

if (intel_engine_uses_guc(engine))         /*          * GuC itself is toast or GuC's hang detection          * is disabled. Either way, need to find the          * hang culprit manually.          */         intel_guc_find_hung_context(engine);

intel_gt_handle_error(engine->gt, engine->mask,                   I915_ERROR_CAPTURE,                   "stopped heartbeat on %s",                   engine->name); }

How there is no "stopped hearbeat" in guc mode? From this code it certainly looks there is.

Only when the GuC is toast and it is no longer an engine reset but a full GT reset that is required. So technically, it is not a 'stopped heartbeat on engine XXX' it is 'stopped heartbeat on GT#'.

...

You say below heartbeats are going in GuC mode. Now I totally don't understand how they are going but there is allegedly no "stopped hearbeat".

Because if GuC is handling the detection and recovery then i915 will not reach that point. GuC will do the engine reset and start scheduling the next context before the heartbeat period expires. So the notification will be a G2H about a specific context being reset rather than the i915 notification about a stopped heartbeat.

...

...

...

intel_gt_handle_error(engine->gt, engine->mask,                   I915_ERROR_CAPTURE,                   "stopped heartbeat on %s",                   engine->name);

intel_gt_handle_error:

/*      * Try engine reset when available. We fall back to full reset if      * single reset fails.      */     if (!intel_uc_uses_guc_submission(&gt->uc) &&         intel_has_reset_engine(gt) && !intel_gt_is_wedged(gt)) {         local_bh_disable();         for_each_engine_masked(engine, gt, engine_mask, tmp) {

You said "However, the heartbeat is still present and is still the watchdog by which engine resets are triggered", now I don't know what you meant by this. It actually triggers a single engine reset in GuC mode? Where in code does that happen if this block above shows it not taking the engine reset path?

i915 sends down the per engine pulse. GuC schedules the pulse GuC attempts to pre-empt the currently active context GuC detects the pre-emption timeout GuC resets the engine

The fundamental process is exactly the same as in execlist mode. It's just that the above blocks of code (calls to intel_gt_handle_error and such) are now inside the GuC not i915.

Without the heartbeat going ping, there would be no context switching and thus no pre-emption, no pre-emption timeout and so no hang and reset recovery. And GuC cannot sent pulses by itself - it has no ability to generate context workloads. So we need i915 to send the pings and to gradually raise their priority. But the back half of the heartbeat code is now inside the GuC. It will simply never reach the i915 side timeout if GuC is doing the recovery (unless the heartbeat's final period is too short). We should only reach the i915 side timeout if GuC itself is toast. At which point we need the full GT reset to recover the GuC.

If workload is not preempting and reset does not work, like engine is truly stuck, does the current code hit "stopped heartbeat" or not in GuC mode?

Hang on, where did 'reset does not work' come into this?

If GuC is alive and the hardware is not broken then no, it won't. That's the whole point. GuC does the detection and recovery. The KMD will never reach 'stopped heartbeat'.

If the hardware is broken and the reset does not work then GuC will send a 'failed reset' notification to the KMD. The KMD treats that as a major error and immediately does a full GT reset. So there is still no 'stopped heartbeat'.

If GuC has died (or a KMD bug has caused sufficient confusion to make it think the GuC has died) then yes, you will reach that code. But in that case it is not an engine reset that is required, it is a full GT reset including a reset of the GuC.

Got it, so what is actually wrong is calling intel_gt_handle_error with an engine->mask in GuC mode. intel_engine_hearbeat.c/reset_engine should fork into two (in some way), depending on backend, so in the case of GuC it can call a variant of intel_gt_handle_error which would be explicitly about a full GPU reset only, instead of a sprinkle of intel_uc_uses_guc_submission in that function. Possibly even off load the reset to a single per gt worker, so that if multiple active engines trigger the reset roughly simultaneously, there is only one full GPU reset. And it gets correctly labeled as "dead GuC" or something.

Sure. Feel free to re-write the reset code yet again. That's another exceedingly fragile area of the driver.

However, that is unrelated to this patch set.

John.

Regards,

Tvrtko

On 2/28/2022 07:32, Tvrtko Ursulin wrote:

On 25/02/2022 19:03, John Harrison wrote:

...

On 2/25/2022 10:29, Tvrtko Ursulin wrote:

...

On 25/02/2022 18:01, John Harrison wrote:

...

On 2/25/2022 09:39, Tvrtko Ursulin wrote:

...

On 25/02/2022 17:11, John Harrison wrote:

...

On 2/25/2022 08:36, Tvrtko Ursulin wrote: > On 24/02/2022 20:02, John Harrison wrote: >> On 2/23/2022 04:00, Tvrtko Ursulin wrote: >>> On 23/02/2022 02:22, John Harrison wrote: >>>> On 2/22/2022 01:53, Tvrtko Ursulin wrote: >>>>> On 18/02/2022 21:33, John.C.Harrison@Intel.com wrote: >>>>>> From: John Harrison John.C.Harrison@Intel.com >>>>>> >>>>>> Compute workloads are inherently not pre-emptible on >>>>>> current hardware. >>>>>> Thus the pre-emption timeout was disabled as a workaround >>>>>> to prevent >>>>>> unwanted resets. Instead, the hang detection was left to >>>>>> the heartbeat >>>>>> and its (longer) timeout. This is undesirable with GuC >>>>>> submission as >>>>>> the heartbeat is a full GT reset rather than a per engine >>>>>> reset and so >>>>>> is much more destructive. Instead, just bump the >>>>>> pre-emption timeout >>>>> >>>>> Can we have a feature request to allow asking GuC for an >>>>> engine reset? >>>> For what purpose? >>> >>> To allow "stopped heartbeat" to reset the engine, however.. >>> >>>> GuC manages the scheduling of contexts across engines. With >>>> virtual engines, the KMD has no knowledge of which engine a >>>> context might be executing on. Even without virtual engines, >>>> the KMD still has no knowledge of which context is currently >>>> executing on any given engine at any given time. >>>> >>>> There is a reason why hang detection should be left to the >>>> entity that is doing the scheduling. Any other entity is >>>> second guessing at best. >>>> >>>> The reason for keeping the heartbeat around even when GuC >>>> submission is enabled is for the case where the KMD/GuC have >>>> got out of sync with either other somehow or GuC itself has >>>> just crashed. I.e. when no submission at all is working and >>>> we need to reset the GuC itself and start over. >>> >>> .. I wasn't really up to speed to know/remember heartbeats are >>> nerfed already in GuC mode. >> Not sure what you mean by that claim. Engine resets are handled >> by GuC because GuC handles the scheduling. You can't do the >> former if you aren't doing the latter. However, the heartbeat >> is still present and is still the watchdog by which engine >> resets are triggered. As per the rest of the submission >> process, the hang detection and recovery is split between i915 >> and GuC. > > I meant that "stopped heartbeat on engine XXX" can only do a > full GPU reset on GuC. I mean that there is no 'stopped heartbeat on engine XXX' when i915 is not handling the recovery part of the process.

Hmmmm?

static void reset_engine(struct intel_engine_cs *engine, struct i915_request *rq) {     if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))         show_heartbeat(rq, engine);

if (intel_engine_uses_guc(engine))         /*          * GuC itself is toast or GuC's hang detection          * is disabled. Either way, need to find the          * hang culprit manually.          */         intel_guc_find_hung_context(engine);

intel_gt_handle_error(engine->gt, engine->mask,                   I915_ERROR_CAPTURE,                   "stopped heartbeat on %s",                   engine->name); }

How there is no "stopped hearbeat" in guc mode? From this code it certainly looks there is.

Only when the GuC is toast and it is no longer an engine reset but a full GT reset that is required. So technically, it is not a 'stopped heartbeat on engine XXX' it is 'stopped heartbeat on GT#'.

...

You say below heartbeats are going in GuC mode. Now I totally don't understand how they are going but there is allegedly no "stopped hearbeat".

Because if GuC is handling the detection and recovery then i915 will not reach that point. GuC will do the engine reset and start scheduling the next context before the heartbeat period expires. So the notification will be a G2H about a specific context being reset rather than the i915 notification about a stopped heartbeat.

...

...

> >     intel_gt_handle_error(engine->gt, engine->mask, >                   I915_ERROR_CAPTURE, >                   "stopped heartbeat on %s", >                   engine->name); > > intel_gt_handle_error: > >     /* >      * Try engine reset when available. We fall back to full > reset if >      * single reset fails. >      */ >     if (!intel_uc_uses_guc_submission(&gt->uc) && >         intel_has_reset_engine(gt) && !intel_gt_is_wedged(gt)) { >         local_bh_disable(); >         for_each_engine_masked(engine, gt, engine_mask, tmp) { > > You said "However, the heartbeat is still present and is still > the watchdog by which engine resets are triggered", now I don't > know what you meant by this. It actually triggers a single > engine reset in GuC mode? Where in code does that happen if this > block above shows it not taking the engine reset path? i915 sends down the per engine pulse. GuC schedules the pulse GuC attempts to pre-empt the currently active context GuC detects the pre-emption timeout GuC resets the engine

The fundamental process is exactly the same as in execlist mode. It's just that the above blocks of code (calls to intel_gt_handle_error and such) are now inside the GuC not i915.

Without the heartbeat going ping, there would be no context switching and thus no pre-emption, no pre-emption timeout and so no hang and reset recovery. And GuC cannot sent pulses by itself

• it has no ability to generate context workloads. So we need

i915 to send the pings and to gradually raise their priority. But the back half of the heartbeat code is now inside the GuC. It will simply never reach the i915 side timeout if GuC is doing the recovery (unless the heartbeat's final period is too short). We should only reach the i915 side timeout if GuC itself is toast. At which point we need the full GT reset to recover the GuC.

If workload is not preempting and reset does not work, like engine is truly stuck, does the current code hit "stopped heartbeat" or not in GuC mode?

Hang on, where did 'reset does not work' come into this?

If GuC is alive and the hardware is not broken then no, it won't. That's the whole point. GuC does the detection and recovery. The KMD will never reach 'stopped heartbeat'.

If the hardware is broken and the reset does not work then GuC will send a 'failed reset' notification to the KMD. The KMD treats that as a major error and immediately does a full GT reset. So there is still no 'stopped heartbeat'.

If GuC has died (or a KMD bug has caused sufficient confusion to make it think the GuC has died) then yes, you will reach that code. But in that case it is not an engine reset that is required, it is a full GT reset including a reset of the GuC.

Got it, so what is actually wrong is calling intel_gt_handle_error with an engine->mask in GuC mode. intel_engine_hearbeat.c/reset_engine should fork into two (in some way), depending on backend, so in the case of GuC it can call a variant of intel_gt_handle_error which would be explicitly about a full GPU reset only, instead of a sprinkle of intel_uc_uses_guc_submission in that function. Possibly even off load the reset to a single per gt worker, so that if multiple active engines trigger the reset roughly simultaneously, there is only one full GPU reset. And it gets correctly labeled as "dead GuC" or something.

Sure. Feel free to re-write the reset code yet again. That's another exceedingly fragile area of the driver.

...

However, that is unrelated to this patch set.

It's still okay to talk about improving things in my opinion. Especially since I think it is the same issue I raised late 2019 during internal review.

And I don't think it is fair to say that I should go and rewrite it, since I do not own the GuC backend area. Especially given the above.

If there is no motivation to improve it now then please at least track this, if it isn't already, in that internal Jira which was tracking all the areas of the GuC backend which could be improved.

I am also pretty sure if the design was cleaner it would have been more obvious to me, or anyone who happens to stumble there, what the purpose of intel_engine_heartbeat.c/reset_engine() is in GuC mode. So we wouldn't have to spend this long explaining things.

My point is that redesigning it to be cleaner is not just a GuC task. It means redesigning the entire reset sequence to more compatible with externally handled resets. That's a big task. Sure it can be added to the todo list but it is totally not in the scope of this patch set.

This is purely about enabling per engine resets again so that end users have a better experience when GPU hangs occur. Or at least, don't have a greatly worse experience on our newest platforms than they did on all the previous ones because we have totally hacked that feature out. And actually getting that feature enabled before we ship too many products and the maintainers/architects decide we are no longer allowed to turn it on because it is now a behaviour change that end users are not expecting. So forever more ADL-P/DG2 are stuck on full GT only resets.

John.

Regards,

Tvrtko

On 3/2/2022 03:21, Tvrtko Ursulin wrote:

On 28/02/2022 19:17, John Harrison wrote:

...

On 2/28/2022 07:32, Tvrtko Ursulin wrote:

...

On 25/02/2022 19:03, John Harrison wrote:

...

On 2/25/2022 10:29, Tvrtko Ursulin wrote:

...

On 25/02/2022 18:01, John Harrison wrote:

...

On 2/25/2022 09:39, Tvrtko Ursulin wrote: > On 25/02/2022 17:11, John Harrison wrote: >> On 2/25/2022 08:36, Tvrtko Ursulin wrote: >>> On 24/02/2022 20:02, John Harrison wrote: >>>> On 2/23/2022 04:00, Tvrtko Ursulin wrote: >>>>> On 23/02/2022 02:22, John Harrison wrote: >>>>>> On 2/22/2022 01:53, Tvrtko Ursulin wrote: >>>>>>> On 18/02/2022 21:33, John.C.Harrison@Intel.com wrote: >>>>>>>> From: John Harrison John.C.Harrison@Intel.com >>>>>>>> >>>>>>>> Compute workloads are inherently not pre-emptible on >>>>>>>> current hardware. >>>>>>>> Thus the pre-emption timeout was disabled as a workaround >>>>>>>> to prevent >>>>>>>> unwanted resets. Instead, the hang detection was left to >>>>>>>> the heartbeat >>>>>>>> and its (longer) timeout. This is undesirable with GuC >>>>>>>> submission as >>>>>>>> the heartbeat is a full GT reset rather than a per engine >>>>>>>> reset and so >>>>>>>> is much more destructive. Instead, just bump the >>>>>>>> pre-emption timeout >>>>>>> >>>>>>> Can we have a feature request to allow asking GuC for an >>>>>>> engine reset? >>>>>> For what purpose? >>>>> >>>>> To allow "stopped heartbeat" to reset the engine, however.. >>>>> >>>>>> GuC manages the scheduling of contexts across engines. With >>>>>> virtual engines, the KMD has no knowledge of which engine a >>>>>> context might be executing on. Even without virtual >>>>>> engines, the KMD still has no knowledge of which context is >>>>>> currently executing on any given engine at any given time. >>>>>> >>>>>> There is a reason why hang detection should be left to the >>>>>> entity that is doing the scheduling. Any other entity is >>>>>> second guessing at best. >>>>>> >>>>>> The reason for keeping the heartbeat around even when GuC >>>>>> submission is enabled is for the case where the KMD/GuC >>>>>> have got out of sync with either other somehow or GuC >>>>>> itself has just crashed. I.e. when no submission at all is >>>>>> working and we need to reset the GuC itself and start over. >>>>> >>>>> .. I wasn't really up to speed to know/remember heartbeats >>>>> are nerfed already in GuC mode. >>>> Not sure what you mean by that claim. Engine resets are >>>> handled by GuC because GuC handles the scheduling. You can't >>>> do the former if you aren't doing the latter. However, the >>>> heartbeat is still present and is still the watchdog by which >>>> engine resets are triggered. As per the rest of the >>>> submission process, the hang detection and recovery is split >>>> between i915 and GuC. >>> >>> I meant that "stopped heartbeat on engine XXX" can only do a >>> full GPU reset on GuC. >> I mean that there is no 'stopped heartbeat on engine XXX' when >> i915 is not handling the recovery part of the process. > > Hmmmm? > > static void > reset_engine(struct intel_engine_cs *engine, struct i915_request > *rq) > { >     if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) >         show_heartbeat(rq, engine); > >     if (intel_engine_uses_guc(engine)) >         /* >          * GuC itself is toast or GuC's hang detection >          * is disabled. Either way, need to find the >          * hang culprit manually. >          */ >         intel_guc_find_hung_context(engine); > >     intel_gt_handle_error(engine->gt, engine->mask, >                   I915_ERROR_CAPTURE, >                   "stopped heartbeat on %s", >                   engine->name); > } > > How there is no "stopped hearbeat" in guc mode? From this code > it certainly looks there is. Only when the GuC is toast and it is no longer an engine reset but a full GT reset that is required. So technically, it is not a 'stopped heartbeat on engine XXX' it is 'stopped heartbeat on GT#'.

> > You say below heartbeats are going in GuC mode. Now I totally > don't understand how they are going but there is allegedly no > "stopped hearbeat". Because if GuC is handling the detection and recovery then i915 will not reach that point. GuC will do the engine reset and start scheduling the next context before the heartbeat period expires. So the notification will be a G2H about a specific context being reset rather than the i915 notification about a stopped heartbeat.

> >>> >>>     intel_gt_handle_error(engine->gt, engine->mask, >>>                   I915_ERROR_CAPTURE, >>>                   "stopped heartbeat on %s", >>>                   engine->name); >>> >>> intel_gt_handle_error: >>> >>>     /* >>>      * Try engine reset when available. We fall back to full >>> reset if >>>      * single reset fails. >>>      */ >>>     if (!intel_uc_uses_guc_submission(&gt->uc) && >>>         intel_has_reset_engine(gt) && !intel_gt_is_wedged(gt)) { >>>         local_bh_disable(); >>>         for_each_engine_masked(engine, gt, engine_mask, tmp) { >>> >>> You said "However, the heartbeat is still present and is still >>> the watchdog by which engine resets are triggered", now I >>> don't know what you meant by this. It actually triggers a >>> single engine reset in GuC mode? Where in code does that >>> happen if this block above shows it not taking the engine >>> reset path? >> i915 sends down the per engine pulse. >> GuC schedules the pulse >> GuC attempts to pre-empt the currently active context >> GuC detects the pre-emption timeout >> GuC resets the engine >> >> The fundamental process is exactly the same as in execlist >> mode. It's just that the above blocks of code (calls to >> intel_gt_handle_error and such) are now inside the GuC not i915. >> >> Without the heartbeat going ping, there would be no context >> switching and thus no pre-emption, no pre-emption timeout and >> so no hang and reset recovery. And GuC cannot sent pulses by >> itself - it has no ability to generate context workloads. So we >> need i915 to send the pings and to gradually raise their >> priority. But the back half of the heartbeat code is now inside >> the GuC. It will simply never reach the i915 side timeout if >> GuC is doing the recovery (unless the heartbeat's final period >> is too short). We should only reach the i915 side timeout if >> GuC itself is toast. At which point we need the full GT reset >> to recover the GuC. > > If workload is not preempting and reset does not work, like > engine is truly stuck, does the current code hit "stopped > heartbeat" or not in GuC mode? Hang on, where did 'reset does not work' come into this?

If GuC is alive and the hardware is not broken then no, it won't. That's the whole point. GuC does the detection and recovery. The KMD will never reach 'stopped heartbeat'.

If the hardware is broken and the reset does not work then GuC will send a 'failed reset' notification to the KMD. The KMD treats that as a major error and immediately does a full GT reset. So there is still no 'stopped heartbeat'.

If GuC has died (or a KMD bug has caused sufficient confusion to make it think the GuC has died) then yes, you will reach that code. But in that case it is not an engine reset that is required, it is a full GT reset including a reset of the GuC.

Got it, so what is actually wrong is calling intel_gt_handle_error with an engine->mask in GuC mode. intel_engine_hearbeat.c/reset_engine should fork into two (in some way), depending on backend, so in the case of GuC it can call a variant of intel_gt_handle_error which would be explicitly about a full GPU reset only, instead of a sprinkle of intel_uc_uses_guc_submission in that function. Possibly even off load the reset to a single per gt worker, so that if multiple active engines trigger the reset roughly simultaneously, there is only one full GPU reset. And it gets correctly labeled as "dead GuC" or something.

Sure. Feel free to re-write the reset code yet again. That's another exceedingly fragile area of the driver.

...

However, that is unrelated to this patch set.

It's still okay to talk about improving things in my opinion. Especially since I think it is the same issue I raised late 2019 during internal review.

And I don't think it is fair to say that I should go and rewrite it, since I do not own the GuC backend area. Especially given the above.

If there is no motivation to improve it now then please at least track this, if it isn't already, in that internal Jira which was tracking all the areas of the GuC backend which could be improved.

I am also pretty sure if the design was cleaner it would have been more obvious to me, or anyone who happens to stumble there, what the purpose of intel_engine_heartbeat.c/reset_engine() is in GuC mode. So we wouldn't have to spend this long explaining things.

My point is that redesigning it to be cleaner is not just a GuC task. It means redesigning the entire reset sequence to more compatible with externally handled resets. That's a big task. Sure it can be added to the todo list but it is totally not in the scope of this patch set.

My point was that was something which was raised years ago ("don't just shoe-horn, redesign, refactor"). Anyway, stopping flogging of this dead horse.. onto below..

...

This is purely about enabling per engine resets again so that end users have a better experience when GPU hangs occur. Or at least, don't have a greatly worse experience on our newest platforms than they did on all the previous ones because we have totally hacked that feature out. And actually getting that feature enabled before we ship too many products and the maintainers/architects decide we are no longer allowed to turn it on because it is now a behaviour change that end users are not expecting. So forever more ADL-P/DG2 are stuck on full GT only resets.

Is any platform with GuC outside force probe already? Either way blocking re-addition of engine resets will not be a thing from maintainers point of view. Whether or not the fail of not having them is a conscious or accidental miss, we certainly want it back ASAP.

ADL-P is released and running GuC submission.

John.

Regards,

Tvrtko