On Sun, Aug 08, 2021 at 11:07:55AM -0700, Matthew Brost wrote:

Resets are notoriously hard to get fully working and notoriously racey, especially with selftests / IGTs that do all sorts of wild things that would be near impossible to hit during normal use cases. Even though likely impossible to hit, anything selftests / IGTs uncover needs to be fixed. This patch addresses 7 such issues.

1. A small race that could result in incorrect accounting of the number

of outstanding G2H. Basically prior to this patch we did not increment the number of outstanding G2H if we encoutered a GT reset while sending a H2G. This was incorrect as the context state had already been updated to anticipate a G2H response thus the counter should be incremented.

2. When unwinding requests on a reset context, if other requests in the

context are in the priority list the requests could be resubmitted out of seqno order. Traverse the list of active requests in reserve and append to the head of the priority list to fix this.

3. Don't drop ce->guc_active.lock when unwinding a context after reset.

At one point we had to drop this because of a lock inversion but that is no longer the case. It is much safer to hold the lock so let's do that.

4. Prior to this patch the blocked context counter was cleared on

init_sched_state (used during registering a context & resets) which is incorrect. This state needs to be persistent or the counter can read the incorrect value.

5. Flush the work queue for GuC generated G2H messages during a GT reset.

6. Do not clear enable during a context reset if a schedule enable is in

flight.

7. When unblocking a context, do not enable scheduling if the context is

banned.

I think each of the above should be a separate patch. I think it would also be good if each fix references the commits that introduced/changed something.

Most of this stuff is extremely hard to get right, and unfortunately our current code is way too fond of lockless trickery (which really isn't a great idea in the reset code). We need to apply as much care as possible here.

Also expect me to ask a lot of annoying questions about all the atomic_t you touch :-) -Daniel

Fixes: f4eb1f3fe946 ("drm/i915/guc: Ensure G2H response has space in buffer") Fixes: 62eaf0ae217d ("drm/i915/guc: Support request cancellation") Fixes: eb5e7da736f3 ("drm/i915/guc: Reset implementation for new GuC interface") Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: stable@vger.kernel.org

---

.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 43 ++++++++++++------- 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 87d8dc8f51b9..cd8df078ca87 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -152,7 +152,7 @@ static inline void init_sched_state(struct intel_context *ce) { /* Only should be called from guc_lrc_desc_pin() */ atomic_set(&ce->guc_sched_state_no_lock, 0);

• ce->guc_state.sched_state = 0;

• ce->guc_state.sched_state &= SCHED_STATE_BLOCKED_MASK;

}

static inline bool @@ -360,11 +360,13 @@ static int guc_submission_send_busy_loop(struct intel_guc *guc, { int err;

• err = intel_guc_send_busy_loop(guc, action, len, g2h_len_dw, loop);
• if (!err && g2h_len_dw)

• if (g2h_len_dw) atomic_inc(&guc->outstanding_submission_g2h);

• err = intel_guc_send_busy_loop(guc, action, len, g2h_len_dw, loop);

• if (err == -EBUSY && g2h_len_dw)

• atomic_dec(&guc->outstanding_submission_g2h);
  

• return err;

}

@@ -725,6 +727,11 @@ void intel_guc_submission_reset_prepare(struct intel_guc *guc) wait_for_reset(guc, &guc->outstanding_submission_g2h); } while (!list_empty(&guc->ct.requests.incoming)); }

• /* Flush any GuC generated G2H */
• while (!list_empty(&guc->ct.requests.incoming))

• msleep(1);
  

• scrub_guc_desc_for_outstanding_g2h(guc);

}

@@ -797,14 +804,13 @@ __unwind_incomplete_requests(struct intel_context *ce)

spin_lock_irqsave(&sched_engine->lock, flags); spin_lock(&ce->guc_active.lock);

• list_for_each_entry_safe(rq, rn,

• 		 &ce->guc_active.requests,
  

• 		 sched.link) {
  

• list_for_each_entry_safe_reverse(rq, rn,

• 			 &ce->guc_active.requests,
  

• 			 sched.link) {
  

  if (i915_request_completed(rq)) continue;

  list_del_init(&rq->sched.link);

• spin_unlock(&ce->guc_active.lock);
  
  

  __i915_request_unsubmit(rq);

@@ -816,10 +822,8 @@ __unwind_incomplete_requests(struct intel_context *ce) } GEM_BUG_ON(i915_sched_engine_is_empty(sched_engine));

• list_add_tail(&rq->sched.link, pl);
  

• list_add(&rq->sched.link, pl);
  

  set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);

• spin_lock(&ce->guc_active.lock);
  

  } spin_unlock(&ce->guc_active.lock); spin_unlock_irqrestore(&sched_engine->lock, flags);

@@ -828,17 +832,23 @@ __unwind_incomplete_requests(struct intel_context *ce) static void __guc_reset_context(struct intel_context *ce, bool stalled) { struct i915_request *rq;

• unsigned long flags; u32 head;

  intel_context_get(ce);

  /*

• * GuC will implicitly mark the context as non-schedulable
  

• * when it sends the reset notification. Make sure our state
  

• * reflects this change. The context will be marked enabled
  

• * on resubmission.
  

• * GuC will implicitly mark the context as non-schedulable when it sends
  

• * the reset notification. Make sure our state reflects this change. The
  

• * context will be marked enabled on resubmission. A small window exists
  

• * where the context could be block & unblocked (scheduling enable) while
  

• * this reset was inflight. If a scheduling enable is already is in
  

• * flight do not clear the enable.
  

  */

• clr_context_enabled(ce);

• spin_lock_irqsave(&ce->guc_state.lock, flags);

• if (!context_pending_enable(ce))

• clr_context_enabled(ce);
  

• spin_unlock_irqrestore(&ce->guc_state.lock, flags);

  rq = intel_context_find_active_request(ce); if (!rq) {

@@ -1562,6 +1572,7 @@ static void guc_context_unblock(struct intel_context *ce) spin_lock_irqsave(&ce->guc_state.lock, flags);

if (unlikely(submission_disabled(guc) ||

•      intel_context_is_banned(ce) ||
  !intel_context_is_pinned(ce) ||
  context_pending_disable(ce) ||
  context_blocked(ce) > 1)) {
  

-- 2.28.0

On Mon, Aug 09, 2021 at 07:35:22PM +0000, Matthew Brost wrote:

On Mon, Aug 09, 2021 at 03:35:26PM +0200, Daniel Vetter wrote:

...

On Sun, Aug 08, 2021 at 11:07:55AM -0700, Matthew Brost wrote:

...

Resets are notoriously hard to get fully working and notoriously racey, especially with selftests / IGTs that do all sorts of wild things that would be near impossible to hit during normal use cases. Even though likely impossible to hit, anything selftests / IGTs uncover needs to be fixed. This patch addresses 7 such issues.

1. A small race that could result in incorrect accounting of the number

of outstanding G2H. Basically prior to this patch we did not increment the number of outstanding G2H if we encoutered a GT reset while sending a H2G. This was incorrect as the context state had already been updated to anticipate a G2H response thus the counter should be incremented.

2. When unwinding requests on a reset context, if other requests in the

context are in the priority list the requests could be resubmitted out of seqno order. Traverse the list of active requests in reserve and append to the head of the priority list to fix this.

3. Don't drop ce->guc_active.lock when unwinding a context after reset.

At one point we had to drop this because of a lock inversion but that is no longer the case. It is much safer to hold the lock so let's do that.

4. Prior to this patch the blocked context counter was cleared on

init_sched_state (used during registering a context & resets) which is incorrect. This state needs to be persistent or the counter can read the incorrect value.

5. Flush the work queue for GuC generated G2H messages during a GT reset.

6. Do not clear enable during a context reset if a schedule enable is in

flight.

7. When unblocking a context, do not enable scheduling if the context is

banned.

I think each of the above should be a separate patch. I think it would also be good if each fix references the commits that introduced/changed something.

Sure, just was trying to cheat and make our lives easier with less patches to backport into DII.

...

Most of this stuff is extremely hard to get right, and unfortunately our current code is way too fond of lockless trickery (which really isn't a great idea in the reset code). We need to apply as much care as possible here.

Yep, resets are hard. It is hard because like ten other async things (e.g. a new submission, registering a context, banning a context, canceling a request, processing a G2H, trying to idle the GPU, unpinning a context) can all be happening at the same time. Hopefully when we move the DRM scheduler we can remove some of these async operations, perma-pinned contexts would also help too. Have a story for that + a story to simplify the locking.

A bit an aside, but drm/sched has a pretty solid story around resets, including what to do if your reset domain escalates (probably more useful for the execlist backend than GuC) and how it's all synchronized.

I do need to review the barriers for when you permanently wedge an engine, and the support for that isn't well encapsulated nor documented. But it's there (amdgpu uses that when the reset fails, kinda like we do), and that's about the only gap I've found thus far around drm/sched reset support.

So should all be substantially clarified once we get there. -Daniel

...

Also expect me to ask a lot of annoying questions about all the atomic_t you touch :-)

Looking forward to it.

Matt

...

-Daniel

...

Fixes: f4eb1f3fe946 ("drm/i915/guc: Ensure G2H response has space in buffer") Fixes: 62eaf0ae217d ("drm/i915/guc: Support request cancellation") Fixes: eb5e7da736f3 ("drm/i915/guc: Reset implementation for new GuC interface") Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: stable@vger.kernel.org

---

.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 43 ++++++++++++------- 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 87d8dc8f51b9..cd8df078ca87 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -152,7 +152,7 @@ static inline void init_sched_state(struct intel_context *ce) { /* Only should be called from guc_lrc_desc_pin() */ atomic_set(&ce->guc_sched_state_no_lock, 0);

• ce->guc_state.sched_state = 0;

• ce->guc_state.sched_state &= SCHED_STATE_BLOCKED_MASK;

}

static inline bool @@ -360,11 +360,13 @@ static int guc_submission_send_busy_loop(struct intel_guc *guc, { int err;

• err = intel_guc_send_busy_loop(guc, action, len, g2h_len_dw, loop);
• if (!err && g2h_len_dw)

• if (g2h_len_dw) atomic_inc(&guc->outstanding_submission_g2h);

• err = intel_guc_send_busy_loop(guc, action, len, g2h_len_dw, loop);

• if (err == -EBUSY && g2h_len_dw)

• atomic_dec(&guc->outstanding_submission_g2h);
  

• return err;

}

@@ -725,6 +727,11 @@ void intel_guc_submission_reset_prepare(struct intel_guc *guc) wait_for_reset(guc, &guc->outstanding_submission_g2h); } while (!list_empty(&guc->ct.requests.incoming)); }

• /* Flush any GuC generated G2H */
• while (!list_empty(&guc->ct.requests.incoming))

• msleep(1);
  

• scrub_guc_desc_for_outstanding_g2h(guc);

}

@@ -797,14 +804,13 @@ __unwind_incomplete_requests(struct intel_context *ce)

spin_lock_irqsave(&sched_engine->lock, flags); spin_lock(&ce->guc_active.lock);

• list_for_each_entry_safe(rq, rn,

• 		 &ce->guc_active.requests,
  

• 		 sched.link) {
  

• list_for_each_entry_safe_reverse(rq, rn,

• 			 &ce->guc_active.requests,
  

• 			 sched.link) {
  

  if (i915_request_completed(rq)) continue;

  list_del_init(&rq->sched.link);

• spin_unlock(&ce->guc_active.lock);
  
  

  __i915_request_unsubmit(rq);

@@ -816,10 +822,8 @@ __unwind_incomplete_requests(struct intel_context *ce) } GEM_BUG_ON(i915_sched_engine_is_empty(sched_engine));

• list_add_tail(&rq->sched.link, pl);
  

• list_add(&rq->sched.link, pl);
  

  set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);

• spin_lock(&ce->guc_active.lock);
  

  } spin_unlock(&ce->guc_active.lock); spin_unlock_irqrestore(&sched_engine->lock, flags);

@@ -828,17 +832,23 @@ __unwind_incomplete_requests(struct intel_context *ce) static void __guc_reset_context(struct intel_context *ce, bool stalled) { struct i915_request *rq;

• unsigned long flags; u32 head;

  intel_context_get(ce);

  /*

• * GuC will implicitly mark the context as non-schedulable
  

• * when it sends the reset notification. Make sure our state
  

• * reflects this change. The context will be marked enabled
  

• * on resubmission.
  

• * GuC will implicitly mark the context as non-schedulable when it sends
  

• * the reset notification. Make sure our state reflects this change. The
  

• * context will be marked enabled on resubmission. A small window exists
  

• * where the context could be block & unblocked (scheduling enable) while
  

• * this reset was inflight. If a scheduling enable is already is in
  

• * flight do not clear the enable.
  

  */

• clr_context_enabled(ce);

• spin_lock_irqsave(&ce->guc_state.lock, flags);

• if (!context_pending_enable(ce))

• clr_context_enabled(ce);
  

• spin_unlock_irqrestore(&ce->guc_state.lock, flags);

  rq = intel_context_find_active_request(ce); if (!rq) {

@@ -1562,6 +1572,7 @@ static void guc_context_unblock(struct intel_context *ce) spin_lock_irqsave(&ce->guc_state.lock, flags);

if (unlikely(submission_disabled(guc) ||

•      intel_context_is_banned(ce) ||
  !intel_context_is_pinned(ce) ||
  context_pending_disable(ce) ||
  context_blocked(ce) > 1)) {
  

-- 2.28.0

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On Sun, Aug 08, 2021 at 11:07:56AM -0700, Matthew Brost wrote:

GuC submission has exposed an existing memory corruption in live_lrc_isolation. We believe that some writes to the watchdog offsets in the LRC (0x178 & 0x17c) can result in trashing of portions of the address space. With GuC submission there are additional objects which can move the context redzone into the space that is trashed. To workaround this avoid poisoning the watchdog.

A Bspec reference here would be good (we can quote anything that's marked for public release, so doesn't have one of the IP markers).

Also I think the above should be replicated in condensed form instead of the XXX comment.

With those: Acked-by: Daniel Vetter daniel.vetter@ffwll.ch since I definitely have enough clue here for a detailed review. -Daniel

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/selftest_lrc.c | 29 +++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c index b0977a3b699b..6500e9fce8a0 100644 --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c @@ -1074,6 +1074,32 @@ record_registers(struct intel_context *ce, goto err_after; }

+static u32 safe_offset(u32 offset, u32 reg) +{

• /* XXX skip testing of watchdog */
• if (offset == 0x178 || offset == 0x17c)

• reg = 0;
  

• return reg;

+}

+static int get_offset_mask(struct intel_engine_cs *engine) +{

• if (GRAPHICS_VER(engine->i915) < 12)

• return 0xfff;
  

• switch (engine->class) {
• default:
• case RENDER_CLASS:

• return 0x07ff;
  

• case COPY_ENGINE_CLASS:

• return 0x0fff;
  

• case VIDEO_DECODE_CLASS:
• case VIDEO_ENHANCEMENT_CLASS:

• return 0x3fff;
  

• }

+}

static struct i915_vma *load_context(struct intel_context *ce, u32 poison) { struct i915_vma *batch; @@ -1117,7 +1143,8 @@ static struct i915_vma *load_context(struct intel_context *ce, u32 poison) len = (len + 1) / 2; *cs++ = MI_LOAD_REGISTER_IMM(len); while (len--) {

• 	*cs++ = hw[dw];
  

• 	*cs++ = safe_offset(hw[dw] & get_offset_mask(ce->engine),
  

• 			    hw[dw]);
  *cs++ = poison;
  dw += 2;
  

  }

-- 2.28.0

On Mon, Aug 09, 2021 at 07:37:39PM +0000, Matthew Brost wrote:

On Mon, Aug 09, 2021 at 03:38:38PM +0200, Daniel Vetter wrote:

...

On Sun, Aug 08, 2021 at 11:07:56AM -0700, Matthew Brost wrote:

...

GuC submission has exposed an existing memory corruption in live_lrc_isolation. We believe that some writes to the watchdog offsets in the LRC (0x178 & 0x17c) can result in trashing of portions of the address space. With GuC submission there are additional objects which can move the context redzone into the space that is trashed. To workaround this avoid poisoning the watchdog.

A Bspec reference here would be good (we can quote anything that's marked for public release, so doesn't have one of the IP markers).

Let me see what I dig up in the bspec.

BTW - Hopefully we can root cause this soon with a proper fix.

Well if it's work-in-progress duct-tape without reference that's fine too, then perhaps sprinkle a JIRA number here (just not the full link, intel IT doesn't like those leaking). Just something that in a few months when someone reads that code they can stitch together the story again. -Daniel

...

Also I think the above should be replicated in condensed form instead of the XXX comment.

Sure.

Matt

...

With those: Acked-by: Daniel Vetter daniel.vetter@ffwll.ch since I definitely have enough clue here for a detailed review. -Daniel

...

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/selftest_lrc.c | 29 +++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c index b0977a3b699b..6500e9fce8a0 100644 --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c @@ -1074,6 +1074,32 @@ record_registers(struct intel_context *ce, goto err_after; }

+static u32 safe_offset(u32 offset, u32 reg) +{

• /* XXX skip testing of watchdog */
• if (offset == 0x178 || offset == 0x17c)

• reg = 0;
  

• return reg;

+}

+static int get_offset_mask(struct intel_engine_cs *engine) +{

• if (GRAPHICS_VER(engine->i915) < 12)

• return 0xfff;
  

• switch (engine->class) {
• default:
• case RENDER_CLASS:

• return 0x07ff;
  

• case COPY_ENGINE_CLASS:

• return 0x0fff;
  

• case VIDEO_DECODE_CLASS:
• case VIDEO_ENHANCEMENT_CLASS:

• return 0x3fff;
  

• }

+}

static struct i915_vma *load_context(struct intel_context *ce, u32 poison) { struct i915_vma *batch; @@ -1117,7 +1143,8 @@ static struct i915_vma *load_context(struct intel_context *ce, u32 poison) len = (len + 1) / 2; *cs++ = MI_LOAD_REGISTER_IMM(len); while (len--) {

• 	*cs++ = hw[dw];
  

• 	*cs++ = safe_offset(hw[dw] & get_offset_mask(ce->engine),
  

• 			    hw[dw]);
  *cs++ = poison;
  dw += 2;
  

  }

-- 2.28.0

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On Sun, Aug 08, 2021 at 11:07:57AM -0700, Matthew Brost wrote:

While debugging an issue with full GT resets I went down a rabbit hole thinking the scrubbing of lost G2H wasn't working correctly. This proved to be incorrect as this was working just fine but this chase inspired me to write a selftest to prove that this works. This simple selftest injects errors dropping various G2H and then issues a full GT reset proving that the scrubbing of these G2H doesn't blow up.

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/intel_context_types.h | 4 + .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 18 +++ drivers/gpu/drm/i915/gt/uc/selftest_guc.c | 126 ++++++++++++++++++ .../drm/i915/selftests/i915_live_selftests.h | 1 + .../i915/selftests/intel_scheduler_helpers.c | 12 ++ .../i915/selftests/intel_scheduler_helpers.h | 2 + 6 files changed, 163 insertions(+) create mode 100644 drivers/gpu/drm/i915/gt/uc/selftest_guc.c

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index e54351a170e2..fec5ff7ef168 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -198,6 +198,10 @@ struct intel_context { */ u8 guc_prio; u32 guc_prio_count[GUC_CLIENT_PRIORITY_NUM];

I know the existing stuff isn't following this at all, but for anything new we really should put some kerneldoc into structures. This probably means you need to open-code the #ifdef here, since this macro will likely upset kerneldoc parsing.

• I915_SELFTEST_DECLARE(bool drop_schedule_enable);
• I915_SELFTEST_DECLARE(bool drop_schedule_disable);
• I915_SELFTEST_DECLARE(bool drop_deregister);

};

#endif /* __INTEL_CONTEXT_TYPES__ */ diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index cd8df078ca87..d13dc56bae43 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2618,6 +2618,11 @@ int intel_guc_deregister_done_process_msg(struct intel_guc *guc,

trace_intel_context_deregister_done(ce);

• if (I915_SELFTEST_ONLY(ce->drop_deregister)) {

• I915_SELFTEST_DECLARE(ce->drop_deregister = false;)
  

This macro wrapping is quite nasty, can't we just #ifdef this? Especially the _DECLARE name really doesn't expect a statement.

Aside from these bikesheds I don't have a much to say on the test logic itself, since I'm far from knowledgable on guc stuff ... -Daniel

• return 0;
  

• }
• if (context_wait_for_deregister_to_register(ce)) { struct intel_runtime_pm *runtime_pm = &ce->engine->gt->i915->runtime_pm;

@@ -2672,10 +2677,19 @@ int intel_guc_sched_done_process_msg(struct intel_guc *guc, trace_intel_context_sched_done(ce);

if (context_pending_enable(ce)) {

• if (I915_SELFTEST_ONLY(ce->drop_schedule_enable)) {
  

• 	I915_SELFTEST_DECLARE(ce->drop_schedule_enable = false;)
  

• 	return 0;
  

• }
  

  clr_context_pending_enable(ce); } else if (context_pending_disable(ce)) { bool banned;

• if (I915_SELFTEST_ONLY(ce->drop_schedule_disable)) {
  

• 	I915_SELFTEST_DECLARE(ce->drop_schedule_disable = false;)
  

• 	return 0;
  

• }
  

• /*

  • Unpin must be done before __guc_signal_context_fence,
  • otherwise a race exists between the requests getting

@@ -3047,3 +3061,7 @@ bool intel_guc_virtual_engine_has_heartbeat(const struct intel_engine_cs *ve)

return false; }

+#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) +#include "selftest_guc.c" +#endif diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c new file mode 100644 index 000000000000..46ca6554f65d --- /dev/null +++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: MIT +/*

• • Copyright �� 2021 Intel Corporation
• */

+#include "selftests/intel_scheduler_helpers.h"

+static struct i915_request *nop_user_request(struct intel_context *ce,

• 			     struct i915_request *from)
  

+{

• struct i915_request *rq;
• int ret;
• rq = intel_context_create_request(ce);
• if (IS_ERR(rq))

• return rq;
  

• if (from) {

• ret = i915_sw_fence_await_dma_fence(&rq->submit,
  

• 				    &from->fence, 0,
  

• 				    I915_FENCE_GFP);
  

• if (ret < 0) {
  

• 	i915_request_put(rq);
  

• 	return ERR_PTR(ret);
  

• }
  

• }
• i915_request_get(rq);
• i915_request_add(rq);
• return rq;

+}

+static int intel_guc_scrub_ctbs(void *arg) +{

• struct intel_gt *gt = arg;
• int ret = 0;
• int i;
• struct i915_request *last[3] = {NULL, NULL, NULL}, *rq;
• intel_wakeref_t wakeref;
• struct intel_engine_cs *engine;
• struct intel_context *ce;
• wakeref = intel_runtime_pm_get(gt->uncore->rpm);
• engine = intel_selftest_find_any_engine(gt);
• /* Submit requests and inject errors forcing G2H to be dropped */
• for (i = 0; i < 3; ++i) {

• ce = intel_context_create(engine);
  

• if (IS_ERR(ce)) {
  

• 	ret = PTR_ERR(ce);
  

• 	pr_err("Failed to create context, %d: %d\n", i, ret);
  

• 	goto err;
  

• }
  

• switch(i) {
  

• case 0:
  

• 	ce->drop_schedule_enable = true;
  

• 	break;
  

• case 1:
  

• 	ce->drop_schedule_disable = true;
  

• 	break;
  

• case 2:
  

• 	ce->drop_deregister = true;
  

• 	break;
  

• }
  

• rq = nop_user_request(ce, NULL);
  

• intel_context_put(ce);
  

• if (IS_ERR(rq)) {
  

• 	ret = PTR_ERR(rq);
  

• 	pr_err("Failed to create request, %d: %d\n", i, ret);
  

• 	goto err;
  

• }
  

• last[i] = rq;
  

• }
• for (i = 0; i < 3; ++i) {

• ret = i915_request_wait(last[i], 0, HZ);
  

• if (ret < 0) {
  

• 	pr_err("Last request failed to complete: %d\n", ret);
  

• 	goto err;
  

• }
  

• i915_request_put(last[i]);
  

• last[i] = NULL;
  

• }
• /* Force all H2G / G2H to be submitted / processed */
• intel_gt_retire_requests(gt);
• msleep(500);
• /* Scrub missing G2H */
• intel_gt_handle_error(engine->gt, -1, 0, "selftest reset");
• ret = intel_gt_wait_for_idle(gt, HZ);
• if (ret < 0) {

• pr_err("GT failed to idle: %d\n", ret);
  

• goto err;
  

• }

+err:

• for (i = 0; i < 3; ++i)

• if (last[i])
  

• 	i915_request_put(last[i]);
  

• intel_runtime_pm_put(gt->uncore->rpm, wakeref);
• return ret;

+}

+int intel_guc_live_selftests(struct drm_i915_private *i915) +{

• static const struct i915_subtest tests[] = {

• SUBTEST(intel_guc_scrub_ctbs),
  

• };
• struct intel_gt *gt = &i915->gt;
• if (intel_gt_is_wedged(gt))

• return 0;
  

• if (!intel_uc_uses_guc_submission(&gt->uc))

• return 0;
  

• return intel_gt_live_subtests(tests, gt);

+} diff --git a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h index cfa5c4165a4f..3cf6758931f9 100644 --- a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h +++ b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h @@ -47,5 +47,6 @@ selftest(execlists, intel_execlists_live_selftests) selftest(ring_submission, intel_ring_submission_live_selftests) selftest(perf, i915_perf_live_selftests) selftest(slpc, intel_slpc_live_selftests) +selftest(guc, intel_guc_live_selftests) /* Here be dragons: keep last to run last! */ selftest(late_gt_pm, intel_gt_pm_late_selftests) diff --git a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c index 4b328346b48a..310fb83c527e 100644 --- a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c +++ b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c @@ -14,6 +14,18 @@ #define REDUCED_PREEMPT 10 #define WAIT_FOR_RESET_TIME 10000

+struct intel_engine_cs *intel_selftest_find_any_engine(struct intel_gt *gt) +{

• struct intel_engine_cs *engine;
• enum intel_engine_id id;
• for_each_engine(engine, gt, id)

• return engine;
  

• pr_err("No valid engine found!\n");
• return NULL;

+}

int intel_selftest_modify_policy(struct intel_engine_cs *engine, struct intel_selftest_saved_policy *saved, u32 modify_type) diff --git a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h index 35c098601ac0..ae60bb507f45 100644 --- a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h +++ b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h @@ -10,6 +10,7 @@

struct i915_request; struct intel_engine_cs; +struct intel_gt;

struct intel_selftest_saved_policy { u32 flags; @@ -23,6 +24,7 @@ enum selftest_scheduler_modify { SELFTEST_SCHEDULER_MODIFY_FAST_RESET, };

+struct intel_engine_cs *intel_selftest_find_any_engine(struct intel_gt *gt); int intel_selftest_modify_policy(struct intel_engine_cs *engine, struct intel_selftest_saved_policy *saved, enum selftest_scheduler_modify modify_type); -- 2.28.0

On Mon, Aug 09, 2021 at 07:41:29PM +0000, Matthew Brost wrote:

On Mon, Aug 09, 2021 at 04:03:28PM +0200, Daniel Vetter wrote:

...

On Sun, Aug 08, 2021 at 11:07:57AM -0700, Matthew Brost wrote:

...

While debugging an issue with full GT resets I went down a rabbit hole thinking the scrubbing of lost G2H wasn't working correctly. This proved to be incorrect as this was working just fine but this chase inspired me to write a selftest to prove that this works. This simple selftest injects errors dropping various G2H and then issues a full GT reset proving that the scrubbing of these G2H doesn't blow up.

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/intel_context_types.h | 4 + .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 18 +++ drivers/gpu/drm/i915/gt/uc/selftest_guc.c | 126 ++++++++++++++++++ .../drm/i915/selftests/i915_live_selftests.h | 1 + .../i915/selftests/intel_scheduler_helpers.c | 12 ++ .../i915/selftests/intel_scheduler_helpers.h | 2 + 6 files changed, 163 insertions(+) create mode 100644 drivers/gpu/drm/i915/gt/uc/selftest_guc.c

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index e54351a170e2..fec5ff7ef168 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -198,6 +198,10 @@ struct intel_context { */ u8 guc_prio; u32 guc_prio_count[GUC_CLIENT_PRIORITY_NUM];

I know the existing stuff isn't following this at all, but for anything new we really should put some kerneldoc into structures. This probably means you need to open-code the #ifdef here, since this macro will likely upset kerneldoc parsing.

Ok, got it.

...

...

• I915_SELFTEST_DECLARE(bool drop_schedule_enable);
• I915_SELFTEST_DECLARE(bool drop_schedule_disable);
• I915_SELFTEST_DECLARE(bool drop_deregister);

};

#endif /* __INTEL_CONTEXT_TYPES__ */ diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index cd8df078ca87..d13dc56bae43 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2618,6 +2618,11 @@ int intel_guc_deregister_done_process_msg(struct intel_guc *guc,

trace_intel_context_deregister_done(ce);

• if (I915_SELFTEST_ONLY(ce->drop_deregister)) {

• I915_SELFTEST_DECLARE(ce->drop_deregister = false;)
  

This macro wrapping is quite nasty, can't we just #ifdef this? Especially the _DECLARE name really doesn't expect a statement.

Had it like that originally then remember these marcos and in the past people have complained when I didn't use them, so yes pretty much a bikeshed. I personally like the ifdef myself.

I think the plain #ifdef is much clearer in the code. The I915_SELFTEST_ONLY macro makes some sense to compile stuff out in some cases and make sure it's wrapped in unlikely when enabled, and often that's good enough. But not here.

Also because it breaks kerneldoc I don't like the macro really in structs either. Anything that discourages people from writing solid comments is Not Good at All :-) So another reason to not like I915_SELFTEST_DECLARE() macro. -Daniel

Matt

...

Aside from these bikesheds I don't have a much to say on the test logic itself, since I'm far from knowledgable on guc stuff ... -Daniel

...

• return 0;
  

• }
• if (context_wait_for_deregister_to_register(ce)) { struct intel_runtime_pm *runtime_pm = &ce->engine->gt->i915->runtime_pm;

@@ -2672,10 +2677,19 @@ int intel_guc_sched_done_process_msg(struct intel_guc *guc, trace_intel_context_sched_done(ce);

if (context_pending_enable(ce)) {

• if (I915_SELFTEST_ONLY(ce->drop_schedule_enable)) {
  

• 	I915_SELFTEST_DECLARE(ce->drop_schedule_enable = false;)
  

• 	return 0;
  

• }
  

  clr_context_pending_enable(ce); } else if (context_pending_disable(ce)) { bool banned;

• if (I915_SELFTEST_ONLY(ce->drop_schedule_disable)) {
  

• 	I915_SELFTEST_DECLARE(ce->drop_schedule_disable = false;)
  

• 	return 0;
  

• }
  

• /*

  • Unpin must be done before __guc_signal_context_fence,
  • otherwise a race exists between the requests getting

@@ -3047,3 +3061,7 @@ bool intel_guc_virtual_engine_has_heartbeat(const struct intel_engine_cs *ve)

return false; }

+#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) +#include "selftest_guc.c" +#endif diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c new file mode 100644 index 000000000000..46ca6554f65d --- /dev/null +++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: MIT +/*

• • Copyright �� 2021 Intel Corporation
• */

+#include "selftests/intel_scheduler_helpers.h"

+static struct i915_request *nop_user_request(struct intel_context *ce,

• 			     struct i915_request *from)
  

+{

• struct i915_request *rq;
• int ret;
• rq = intel_context_create_request(ce);
• if (IS_ERR(rq))

• return rq;
  

• if (from) {

• ret = i915_sw_fence_await_dma_fence(&rq->submit,
  

• 				    &from->fence, 0,
  

• 				    I915_FENCE_GFP);
  

• if (ret < 0) {
  

• 	i915_request_put(rq);
  

• 	return ERR_PTR(ret);
  

• }
  

• }
• i915_request_get(rq);
• i915_request_add(rq);
• return rq;

+}

+static int intel_guc_scrub_ctbs(void *arg) +{

• struct intel_gt *gt = arg;
• int ret = 0;
• int i;
• struct i915_request *last[3] = {NULL, NULL, NULL}, *rq;
• intel_wakeref_t wakeref;
• struct intel_engine_cs *engine;
• struct intel_context *ce;
• wakeref = intel_runtime_pm_get(gt->uncore->rpm);
• engine = intel_selftest_find_any_engine(gt);
• /* Submit requests and inject errors forcing G2H to be dropped */
• for (i = 0; i < 3; ++i) {

• ce = intel_context_create(engine);
  

• if (IS_ERR(ce)) {
  

• 	ret = PTR_ERR(ce);
  

• 	pr_err("Failed to create context, %d: %d\n", i, ret);
  

• 	goto err;
  

• }
  

• switch(i) {
  

• case 0:
  

• 	ce->drop_schedule_enable = true;
  

• 	break;
  

• case 1:
  

• 	ce->drop_schedule_disable = true;
  

• 	break;
  

• case 2:
  

• 	ce->drop_deregister = true;
  

• 	break;
  

• }
  

• rq = nop_user_request(ce, NULL);
  

• intel_context_put(ce);
  

• if (IS_ERR(rq)) {
  

• 	ret = PTR_ERR(rq);
  

• 	pr_err("Failed to create request, %d: %d\n", i, ret);
  

• 	goto err;
  

• }
  

• last[i] = rq;
  

• }
• for (i = 0; i < 3; ++i) {

• ret = i915_request_wait(last[i], 0, HZ);
  

• if (ret < 0) {
  

• 	pr_err("Last request failed to complete: %d\n", ret);
  

• 	goto err;
  

• }
  

• i915_request_put(last[i]);
  

• last[i] = NULL;
  

• }
• /* Force all H2G / G2H to be submitted / processed */
• intel_gt_retire_requests(gt);
• msleep(500);
• /* Scrub missing G2H */
• intel_gt_handle_error(engine->gt, -1, 0, "selftest reset");
• ret = intel_gt_wait_for_idle(gt, HZ);
• if (ret < 0) {

• pr_err("GT failed to idle: %d\n", ret);
  

• goto err;
  

• }

+err:

• for (i = 0; i < 3; ++i)

• if (last[i])
  

• 	i915_request_put(last[i]);
  

• intel_runtime_pm_put(gt->uncore->rpm, wakeref);
• return ret;

+}

+int intel_guc_live_selftests(struct drm_i915_private *i915) +{

• static const struct i915_subtest tests[] = {

• SUBTEST(intel_guc_scrub_ctbs),
  

• };
• struct intel_gt *gt = &i915->gt;
• if (intel_gt_is_wedged(gt))

• return 0;
  

• if (!intel_uc_uses_guc_submission(&gt->uc))

• return 0;
  

• return intel_gt_live_subtests(tests, gt);

+} diff --git a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h index cfa5c4165a4f..3cf6758931f9 100644 --- a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h +++ b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h @@ -47,5 +47,6 @@ selftest(execlists, intel_execlists_live_selftests) selftest(ring_submission, intel_ring_submission_live_selftests) selftest(perf, i915_perf_live_selftests) selftest(slpc, intel_slpc_live_selftests) +selftest(guc, intel_guc_live_selftests) /* Here be dragons: keep last to run last! */ selftest(late_gt_pm, intel_gt_pm_late_selftests) diff --git a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c index 4b328346b48a..310fb83c527e 100644 --- a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c +++ b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c @@ -14,6 +14,18 @@ #define REDUCED_PREEMPT 10 #define WAIT_FOR_RESET_TIME 10000

+struct intel_engine_cs *intel_selftest_find_any_engine(struct intel_gt *gt) +{

• struct intel_engine_cs *engine;
• enum intel_engine_id id;
• for_each_engine(engine, gt, id)

• return engine;
  

• pr_err("No valid engine found!\n");
• return NULL;

+}

int intel_selftest_modify_policy(struct intel_engine_cs *engine, struct intel_selftest_saved_policy *saved, u32 modify_type) diff --git a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h index 35c098601ac0..ae60bb507f45 100644 --- a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h +++ b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h @@ -10,6 +10,7 @@

struct i915_request; struct intel_engine_cs; +struct intel_gt;

struct intel_selftest_saved_policy { u32 flags; @@ -23,6 +24,7 @@ enum selftest_scheduler_modify { SELFTEST_SCHEDULER_MODIFY_FAST_RESET, };

+struct intel_engine_cs *intel_selftest_find_any_engine(struct intel_gt *gt); int intel_selftest_modify_policy(struct intel_engine_cs *engine, struct intel_selftest_saved_policy *saved, enum selftest_scheduler_modify modify_type); -- 2.28.0

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch