[PATCH 1/3] drm/i915: Cancel outstanding work after disabling heartbeats on an engine

List overview All Threads
Download

newer

older

[PATCH 2/4] usb: typec: ucsi: Fix...

[PATCH 0/2] input/hid: Fix bitmap...

Chris Wilson

12 Aug 2020 12 Aug '20

10:36 p.m.

We only allow persistent requests to remain on the GPU past the closure of their containing context (and process) so long as they are continuously checked for hangs or allow other requests to preempt them, as we need to ensure forward progress of the system. If we allow persistent contexts to remain on the system after the the hangcheck mechanism is disabled, the system may grind to a halt. On disabling the mechanism, we sent a pulse along the engine to remove all executing contexts from the engine which would check for hung contexts -- but we did not prevent those contexts from being resubmitted if they survived the final hangcheck.

Fixes: 9a40bddd47ca ("drm/i915/gt: Expose heartbeat interval via sysfs") Testcase: igt/gem_ctx_persistence/heartbeat-stop Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: stable@vger.kernel.org # v5.7+ --- drivers/gpu/drm/i915/gt/intel_engine.h | 9 +++++++++ drivers/gpu/drm/i915/i915_request.c | 5 +++++ 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h index 08e2c000dcc3..a90cb91c8246 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine.h +++ b/drivers/gpu/drm/i915/gt/intel_engine.h @@ -337,4 +337,13 @@ intel_engine_has_preempt_reset(const struct intel_engine_cs *engine) return intel_engine_has_preemption(engine); }

+static inline bool +intel_engine_has_heartbeat(const struct intel_engine_cs *engine) +{ + if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL)) + return false; + + return engine->props.heartbeat_interval_ms; +} + #endif /* _INTEL_RINGBUFFER_H_ */ diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index 0208e917d14a..92efca606f91 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -542,8 +542,13 @@ bool __i915_request_submit(struct i915_request *request) if (i915_request_completed(request)) goto xfer;

+ if (unlikely(intel_context_is_closed(request->context) && + !intel_engine_has_heartbeat(engine))) + intel_context_set_banned(request->context); + if (unlikely(intel_context_is_banned(request->context))) i915_request_set_error_once(request, -EIO); + if (unlikely(fatal_error(request->fence.error))) __i915_request_skip(request);

-- 2.20.1

Show replies by date

Chris Wilson

12 Aug 12 Aug

10:36 p.m.

New subject: [PATCH 2/3] drm/i915/gt: Always send a pulse down the engine after disabling heartbeat

Currently, we check we can send a pulse prior to disabling the heartbeat to verify that we can change the heartbeat, but since we may re-evaluate execution upon changing the heartbeat interval we need another pulse afterwards to refresh execution.

Fixes: 9a40bddd47ca ("drm/i915/gt: Expose heartbeat interval via sysfs") Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: stable@vger.kernel.org # v5.7+ --- drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c index 8ffdf676c0a0..d09df370f7cd 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c @@ -192,10 +192,12 @@ int intel_engine_set_heartbeat(struct intel_engine_cs *engine, WRITE_ONCE(engine->props.heartbeat_interval_ms, delay);

if (intel_engine_pm_get_if_awake(engine)) { - if (delay) + if (delay) { intel_engine_unpark_heartbeat(engine); - else + } else { intel_engine_park_heartbeat(engine); + intel_engine_pulse(engine); /* recheck execution */ + } intel_engine_pm_put(engine); }

-- 2.20.1

Chris Wilson

10:36 p.m.

New subject: [PATCH 3/3] drm/i915/gem: Always test execution status on closing the context

Verify that if a context is active at the time it is closed, that it is either persistent and preemptible (with hangcheck running) or it shall be removed from execution.

Fixes: 9a40bddd47ca ("drm/i915/gt: Expose heartbeat interval via sysfs") Testcase: igt/gem_ctx_persistence/heartbeat-close Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: stable@vger.kernel.org # v5.7+ --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 24 ++++++++------------- 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index db893f6c516b..49715ae71386 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -431,8 +431,7 @@ static bool __cancel_engine(struct intel_engine_cs *engine) * kill the banned context, we fallback to doing a local reset * instead. */ - if (IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT) && - !intel_engine_pulse(engine)) + if (intel_engine_pulse(engine) == 0) return true;

/* If we are unable to send a pulse, try resetting this engine. */ @@ -493,7 +492,7 @@ static struct intel_engine_cs *active_engine(struct intel_context *ce) return engine; }

-static void kill_engines(struct i915_gem_engines *engines) +static void kill_engines(struct i915_gem_engines *engines, bool ban) { struct i915_gem_engines_iter it; struct intel_context *ce; @@ -508,7 +507,7 @@ static void kill_engines(struct i915_gem_engines *engines) for_each_gem_engine(ce, engines, it) { struct intel_engine_cs *engine;

- if (intel_context_set_banned(ce)) + if (ban && intel_context_set_banned(ce)) continue;

/* @@ -531,8 +530,10 @@ static void kill_engines(struct i915_gem_engines *engines) } }

-static void kill_stale_engines(struct i915_gem_context *ctx) +static void kill_context(struct i915_gem_context *ctx) { + bool ban = (!i915_gem_context_is_persistent(ctx) || + !ctx->i915->params.enable_hangcheck); struct i915_gem_engines *pos, *next;

spin_lock_irq(&ctx->stale.lock); @@ -545,7 +546,7 @@ static void kill_stale_engines(struct i915_gem_context *ctx)

spin_unlock_irq(&ctx->stale.lock);

- kill_engines(pos); + kill_engines(pos, ban);

spin_lock_irq(&ctx->stale.lock); GEM_BUG_ON(i915_sw_fence_signaled(&pos->fence)); @@ -557,11 +558,6 @@ static void kill_stale_engines(struct i915_gem_context *ctx) spin_unlock_irq(&ctx->stale.lock); }

-static void kill_context(struct i915_gem_context *ctx) -{ - kill_stale_engines(ctx); -} - static void engines_idle_release(struct i915_gem_context *ctx, struct i915_gem_engines *engines) { @@ -596,7 +592,7 @@ static void engines_idle_release(struct i915_gem_context *ctx,

kill: if (list_empty(&engines->link)) /* raced, already closed */ - kill_engines(engines); + kill_engines(engines, true);

i915_sw_fence_commit(&engines->fence); } @@ -654,9 +650,7 @@ static void context_close(struct i915_gem_context *ctx) * case we opt to forcibly kill off all remaining requests on * context close. */ - if (!i915_gem_context_is_persistent(ctx) || - !ctx->i915->params.enable_hangcheck) - kill_context(ctx); + kill_context(ctx);

i915_gem_context_put(ctx); }

-- 2.20.1

Chris Wilson

10:59 p.m.

New subject: [PATCH] drm/i915/gem: Always test execution status on closing the context

Verify that if a context is active at the time it is closed, that it is either persistent and preemptible (with hangcheck running) or it shall be removed from execution.

Fixes: 9a40bddd47ca ("drm/i915/gt: Expose heartbeat interval via sysfs") Testcase: igt/gem_ctx_persistence/heartbeat-close Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: stable@vger.kernel.org # v5.7+ --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 26 ++++++++------------- 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index db893f6c516b..ba8ef1225f58 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -431,8 +431,7 @@ static bool __cancel_engine(struct intel_engine_cs *engine) * kill the banned context, we fallback to doing a local reset * instead. */ - if (IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT) && - !intel_engine_pulse(engine)) + if (intel_engine_pulse(engine) == 0) return true;

/* If we are unable to send a pulse, try resetting this engine. */ @@ -493,7 +492,7 @@ static struct intel_engine_cs *active_engine(struct intel_context *ce) return engine; }

- if (intel_context_set_banned(ce)) + if (ban && intel_context_set_banned(ce)) continue;

/* @@ -521,7 +520,7 @@ static void kill_engines(struct i915_gem_engines *engines) engine = active_engine(ce);

/* First attempt to gracefully cancel the context */ - if (engine && !__cancel_engine(engine)) + if (engine && !__cancel_engine(engine) && ban) /* * If we are unable to send a preemptive pulse to bump * the context from the GPU, we have to resort to a full @@ -531,8 +530,10 @@ static void kill_engines(struct i915_gem_engines *engines) } }

spin_lock_irq(&ctx->stale.lock); @@ -545,7 +546,7 @@ static void kill_stale_engines(struct i915_gem_context *ctx)

spin_unlock_irq(&ctx->stale.lock);

- kill_engines(pos); + kill_engines(pos, ban);

kill: if (list_empty(&engines->link)) /* raced, already closed */ - kill_engines(engines); + kill_engines(engines, true);

i915_gem_context_put(ctx); }

-- 2.20.1

Chris Wilson

13 Aug 13 Aug

9:28 a.m.

New subject: [PATCH] drm/i915/gem: Always test execution status on closing the context

Verify that if a context is active at the time it is closed, that it is either persistent and preemptible (with hangcheck running) or it shall be removed from execution.

Fixes: 9a40bddd47ca ("drm/i915/gt: Expose heartbeat interval via sysfs") Testcase: igt/gem_ctx_persistence/heartbeat-close Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: stable@vger.kernel.org # v5.7+ --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 48 +++++---------------- 1 file changed, 10 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index db893f6c516b..15d3ccb8f164 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -390,24 +390,6 @@ __context_engines_static(const struct i915_gem_context *ctx) return rcu_dereference_protected(ctx->engines, true); }

-static bool __reset_engine(struct intel_engine_cs *engine) -{ - struct intel_gt *gt = engine->gt; - bool success = false; - - if (!intel_has_reset_engine(gt)) - return false; - - if (!test_and_set_bit(I915_RESET_ENGINE + engine->id, - &gt->reset.flags)) { - success = intel_engine_reset(engine, NULL) == 0; - clear_and_wake_up_bit(I915_RESET_ENGINE + engine->id, - &gt->reset.flags); - } - - return success; -} - static void __reset_context(struct i915_gem_context *ctx, struct intel_engine_cs *engine) { @@ -431,12 +413,7 @@ static bool __cancel_engine(struct intel_engine_cs *engine) * kill the banned context, we fallback to doing a local reset * instead. */ - if (IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT) && - !intel_engine_pulse(engine)) - return true; - - /* If we are unable to send a pulse, try resetting this engine. */ - return __reset_engine(engine); + return intel_engine_pulse(engine) == 0; }

static bool @@ -493,7 +470,7 @@ static struct intel_engine_cs *active_engine(struct intel_context *ce) return engine; }

-static void kill_engines(struct i915_gem_engines *engines) +static void kill_engines(struct i915_gem_engines *engines, bool ban) { struct i915_gem_engines_iter it; struct intel_context *ce; @@ -508,7 +485,7 @@ static void kill_engines(struct i915_gem_engines *engines) for_each_gem_engine(ce, engines, it) { struct intel_engine_cs *engine;

- if (intel_context_set_banned(ce)) + if (ban && intel_context_set_banned(ce)) continue;

/* @@ -521,7 +498,7 @@ static void kill_engines(struct i915_gem_engines *engines) engine = active_engine(ce);

/* First attempt to gracefully cancel the context */ - if (engine && !__cancel_engine(engine)) + if (engine && !__cancel_engine(engine) && ban) /* * If we are unable to send a preemptive pulse to bump * the context from the GPU, we have to resort to a full @@ -531,8 +508,10 @@ static void kill_engines(struct i915_gem_engines *engines) } }

spin_lock_irq(&ctx->stale.lock); @@ -545,7 +524,7 @@ static void kill_stale_engines(struct i915_gem_context *ctx)

spin_unlock_irq(&ctx->stale.lock);

- kill_engines(pos); + kill_engines(pos, ban);

spin_lock_irq(&ctx->stale.lock); GEM_BUG_ON(i915_sw_fence_signaled(&pos->fence)); @@ -557,11 +536,6 @@ static void kill_stale_engines(struct i915_gem_context *ctx) spin_unlock_irq(&ctx->stale.lock); }

-static void kill_context(struct i915_gem_context *ctx) -{ - kill_stale_engines(ctx); -} - static void engines_idle_release(struct i915_gem_context *ctx, struct i915_gem_engines *engines) { @@ -596,7 +570,7 @@ static void engines_idle_release(struct i915_gem_context *ctx,

kill: if (list_empty(&engines->link)) /* raced, already closed */ - kill_engines(engines); + kill_engines(engines, true);

i915_sw_fence_commit(&engines->fence); } @@ -654,9 +628,7 @@ static void context_close(struct i915_gem_context *ctx) * case we opt to forcibly kill off all remaining requests on * context close. */ - if (!i915_gem_context_is_persistent(ctx) || - !ctx->i915->params.enable_hangcheck) - kill_context(ctx); + kill_context(ctx);

i915_gem_context_put(ctx); }

-- 2.20.1

Sasha Levin

19 Aug 19 Aug

11:56 p.m.

New subject: [PATCH 3/3] drm/i915/gem: Always test execution status on closing the context

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag fixing commit: 9a40bddd47ca ("drm/i915/gt: Expose heartbeat interval via sysfs").

The bot has tested the following trees: v5.8.1, v5.7.15.

v5.8.1: Failed to apply! Possible dependencies: 1b90e4a43b74 ("drm/i915/selftests: Enable selftesting of busy-stats") 2d3879950f8a ("drm/i915: Add psr_safest_params") 4fe13f28d66a ("drm/i915/selftests: Add tests for timeslicing virtual engines") 67a64e51ba92 ("drm/i915/selftests: Refactor sibling selection") 8a25c4be583d ("drm/i915/params: switch to device specific parameters") 9199c070cdde ("drm/i915/selftests: Exercise far preemption rollbacks") ad6586850b6d ("drm/i915/selftests: Change priority overflow detection") d4b02a4c613e ("drm/i915/selftests: Trim execlists runtime") f4bb45f72734 ("drm/i915: Trim set_timer_ms() intervals")

v5.7.15: Failed to apply! Possible dependencies: 0c1abaa7fbfb ("drm: Constify adjusted_mode a bit") 13ea6db2cf24 ("drm/i915/edp: Ignore short pulse when panel powered off") 3dfd8d710419 ("drm/i915/display: use struct drm_device based logging") 81b55ef1f47b ("drm/i915: drop a bunch of superfluous inlines") 8a25c4be583d ("drm/i915/params: switch to device specific parameters") af67009c1439 ("drm/i915/dp: use struct drm_device based logging")

NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- Thanks Sasha

Sasha Levin

26 Aug 26 Aug

1:53 p.m.

New subject: [PATCH 3/3] drm/i915/gem: Always test execution status on closing the context

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag fixing commit: 9a40bddd47ca ("drm/i915/gt: Expose heartbeat interval via sysfs").

The bot has tested the following trees: v5.8.2, v5.7.16.

v5.8.2: Failed to apply! Possible dependencies: 1b90e4a43b74 ("drm/i915/selftests: Enable selftesting of busy-stats") 2d3879950f8a ("drm/i915: Add psr_safest_params") 4fe13f28d66a ("drm/i915/selftests: Add tests for timeslicing virtual engines") 67a64e51ba92 ("drm/i915/selftests: Refactor sibling selection") 8a25c4be583d ("drm/i915/params: switch to device specific parameters") 9199c070cdde ("drm/i915/selftests: Exercise far preemption rollbacks") ad6586850b6d ("drm/i915/selftests: Change priority overflow detection") d4b02a4c613e ("drm/i915/selftests: Trim execlists runtime") f4bb45f72734 ("drm/i915: Trim set_timer_ms() intervals")

v5.7.16: Failed to apply! Possible dependencies: 0c1abaa7fbfb ("drm: Constify adjusted_mode a bit") 13ea6db2cf24 ("drm/i915/edp: Ignore short pulse when panel powered off") 3dfd8d710419 ("drm/i915/display: use struct drm_device based logging") 81b55ef1f47b ("drm/i915: drop a bunch of superfluous inlines") 8a25c4be583d ("drm/i915/params: switch to device specific parameters") af67009c1439 ("drm/i915/dp: use struct drm_device based logging")

NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- Thanks Sasha

1589

days inactive

1603

days old

linux-stable-mirror@lists.linaro.org

6 comments

participants

tags (0)

participants (2)

Chris Wilson
Sasha Levin