On Thu, Aug 19, 2021 at 02:31:51PM -0700, Daniele Ceraolo Spurio wrote:

On 8/18/2021 11:16 PM, Matthew Brost wrote:

...

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.

Also always use helper when decrementing this value.

Fixes: f4eb1f3fe946 ("drm/i915/guc: Ensure G2H response has space in buffer") Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: stable@vger.kernel.org

---

.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 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 69faa39da178..32c414aa9009 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -352,6 +352,12 @@ static inline void set_lrc_desc_registered(struct intel_guc *guc, u32 id, xa_unlock_irqrestore(&guc->context_lookup, flags); } +static void decr_outstanding_submission_g2h(struct intel_guc *guc) +{

• if (atomic_dec_and_test(&guc->outstanding_submission_g2h))

• wake_up_all(&guc->ct.wq);
  

+}

• static int guc_submission_send_busy_loop(struct intel_guc *guc, const u32 *action, u32 len,

@@ -360,11 +366,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)

• decr_outstanding_submission_g2h(guc);
  

here you're special casing  -EBUSY, which kind of implies that the caller needs to handle this differently, but most callers seem to ignore the return code. Is the counter handled somewhere else in case of EBUSY? if so, please add a comment about it.

Good catch, this is a dead code path. Will delete.

Matt

Daniele

...

return err; } @@ -616,7 +624,7 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) init_sched_state(ce); if (pending_enable || destroyed || deregister) {

• 	atomic_dec(&guc->outstanding_submission_g2h);
  

• 	decr_outstanding_submission_g2h(guc);
  if (deregister)
  	guc_signal_context_fence(ce);
  if (destroyed) {
  

@@ -635,7 +643,7 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) intel_engine_signal_breadcrumbs(ce->engine); } intel_context_sched_disable_unpin(ce);

• 	atomic_dec(&guc->outstanding_submission_g2h);
  

• 	decr_outstanding_submission_g2h(guc);
  spin_lock_irqsave(&ce->guc_state.lock, flags);
  guc_blocked_fence_complete(ce);
  spin_unlock_irqrestore(&ce->guc_state.lock, flags);
  

@@ -2583,12 +2591,6 @@ g2h_context_lookup(struct intel_guc *guc, u32 desc_idx) return ce; } -static void decr_outstanding_submission_g2h(struct intel_guc *guc) -{

• if (atomic_dec_and_test(&guc->outstanding_submission_g2h))

• wake_up_all(&guc->ct.wq);
  

-}

• int intel_guc_deregister_done_process_msg(struct intel_guc *guc, const u32 *msg, u32 len)

On 8/18/2021 11:16 PM, Matthew Brost wrote:

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 reverse and append to the head of the priority list to fix this.

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

---

drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 32c414aa9009..9ca0ba4ea85a 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -805,9 +805,9 @@ __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))

The execlists unwind function has a list_del if the request is completed. Any reason not to do that here?

	continue;

@@ -824,7 +824,7 @@ __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);
  

Since you always do both list_del and list_add and it doesn't look like you use the fact that the list is empty between the 2 calls, you can merge them in a list_move.

Apart from these nits, the change to navigate the list in reverse and append here at the top LGTM.

Daniele

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

spin_lock(&ce->guc_active.lock);

On 8/19/2021 4:53 PM, Matthew Brost wrote:

On Thu, Aug 19, 2021 at 04:54:00PM -0700, Daniele Ceraolo Spurio wrote:

...

On 8/18/2021 11:16 PM, Matthew Brost wrote:

...

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 reverse and append to the head of the priority list to fix this.

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

---

drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 32c414aa9009..9ca0ba4ea85a 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -805,9 +805,9 @@ __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))
  

The execlists unwind function has a list_del if the request is completed. Any reason not to do that here?

Def isn't needed here as this is done in remove_from_context(), probably not needed in execlists mode either.

...

...

		continue;

@@ -824,7 +824,7 @@ __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);
  

Since you always do both list_del and list_add and it doesn't look like you use the fact that the list is empty between the 2 calls, you can merge them in a list_move.

Can't use a list move here because we drop spin_lock(&ce->guc_active.lock), that gets fixed later in the series and at that point we likely can use a list_move.

fair enough. I'll leave it to you to decide if it is worth moving this patch after the next one and using a list_move. With or without that, this is:

Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

Daniele

Matt

...

Apart from these nits, the change to navigate the list in reverse and append here at the top LGTM.

Daniele

...

	set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);
	spin_lock(&ce->guc_active.lock);

On 8/18/2021 11:16 PM, Matthew Brost wrote:

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.

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

Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

Do we have a trybot of this series with GuC enabled? I've checked the functions called in the previously unlocked chunk and didn't spot anything requiring the lock to be dropped, but I'd feel safer if we had lockdep results as well.

Daniele

---

drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 ---- 1 file changed, 4 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 9ca0ba4ea85a..e4a099f8f820 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -812,8 +812,6 @@ __unwind_incomplete_requests(struct intel_context *ce) continue;

list_del_init(&rq->sched.link);

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

• __i915_request_unsubmit(rq);

  /* Push the request back into the queue for later resubmission. */

@@ -826,8 +824,6 @@ __unwind_incomplete_requests(struct intel_context *ce)

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

On Tue, Aug 24, 2021 at 04:31:21PM -0700, Daniele Ceraolo Spurio wrote:

On 8/18/2021 11:16 PM, Matthew Brost wrote:

...

If the context is reset as a result of the request cancelation the context reset G2H is received after schedule disable done G2H which is likely the wrong order. The schedule disable done G2H release the waiting request cancelation code which resubmits the context. This races with the context reset G2H which also wants to resubmit the context but in this case it really should be a NOP as request cancelation code owns the resubmit. Use some clever tricks of checking the context state to seal this race until if / when the GuC firmware is fixed.

Did you raise this with the GuC team? If it's a GuC issue we definitely want a fix there ASAP so we can drop any i915-side WAs.

Yep, def an issue with the GuC firmware behavior. Will get fixed, just not sure when.

...

v2: (Checkpatch)

• Fix typos

Fixes: 62eaf0ae217d ("drm/i915/guc: Support request cancellation") 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, 37 insertions(+), 6 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 e4a099f8f820..8f7a11e65ef5 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -832,17 +832,35 @@ __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;
• bool skip = false; 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.
  

• *
  

• * XXX: If the context is reset as a result of the request cancellation
  

• * this G2H is received after the schedule disable complete G2H which is
  

• * likely wrong as this creates a race between the request cancellation
  

• * code re-submitting the context and this G2H handler. This likely
  

• * should be fixed in the GuC but until if / when that gets fixed we
  

• * need to workaround this. Convert this function to a NOP if a pending
  

• * enable is in flight as this indicates that a request cancellation has
  

• * occurred.
  

  */

IMO this comment sounds like we're not clear on expected behavior. Either the ordering is wrong, in which case we have a GuC bug and this is a temporary WA, or the ordering is allowed and we need to cope with it. The way the comment is written sounds like we're not sure.

Comments written prior to confirmation that GuC behavior was wrong, will reword.

Code changes look ok.

Ty. I'll think we have to carry this until we upgrade the GuC firmware with a the proper behavior - until then without this workaround canceling non-preemptable requests is 100% broken, hence why I added a selftest. Will add a FIXME / XXX comment so we can remove this in the future.

Matt

Daniele

...

• clr_context_enabled(ce);

• spin_lock_irqsave(&ce->guc_state.lock, flags);
• if (likely(!context_pending_enable(ce))) {

• clr_context_enabled(ce);
  

• } else {

• skip = true;
  

• }
• spin_unlock_irqrestore(&ce->guc_state.lock, flags);
• if (unlikely(skip))

• goto out_put;
  

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

@@ -861,6 +879,7 @@ static void __guc_reset_context(struct intel_context *ce, bool stalled) out_replay: guc_reset_state(ce, head, stalled); __unwind_incomplete_requests(ce); +out_put: intel_context_put(ce); } @@ -1605,6 +1624,13 @@ static void guc_context_cancel_request(struct intel_context *ce, guc_reset_state(ce, intel_ring_wrap(ce->ring, rq->head), true); }

• /*
  

•  * XXX: Racey if context is reset, see comment in
  

•  * __guc_reset_context().
  

•  */
  

• flush_work(&ce_to_guc(ce)->ct.requests.worker);
  

• guc_context_unblock(ce); } }

@@ -2719,7 +2745,12 @@ static void guc_handle_context_reset(struct intel_guc *guc, { trace_intel_context_reset(ce);

• if (likely(!intel_context_is_banned(ce))) {

• /*

• * XXX: Racey if request cancellation has occurred, see comment in
  

• * __guc_reset_context().
  

• */
  

• if (likely(!intel_context_is_banned(ce) &&

•    !context_blocked(ce))) {
  

  capture_error_state(guc, ce); guc_context_replay(ce); }

On Thu, Aug 19, 2021 at 1:22 AM Matthew Brost matthew.brost@intel.com wrote:

Propagating errors to dependent fences is wrong, don't do it. A selftest in the following exposed the propagating of an error to a dependent fence after an engine reset.

I feel like we could still have a bit of a better message. Maybe something like this:

Propagating errors to dependent fences is broken and can lead to errors from one client ending up in another. In 3761baae908a (Revert "drm/i915: Propagate errors on awaiting already signaled fences"), we attempted to get rid of fence error propagation but missed the case added in 8e9f84cf5cac ("drm/i915/gt: Propagate change in error status to children on unhold"). Revert that one too. This error was found by an up-and-coming selftest which <salient information here>.

Otherwise, looks good to me.

--Jason

This reverts commit 8e9f84cf5cac248a1c6a5daa4942879c8b765058.

v2: (Daniel Vetter)

• Use revert

References: 3761baae908a (Revert "drm/i915: Propagate errors on awaiting already signaled fences") Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 4 ---- 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index de5f9c86b9a4..cafb0608ffb4 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -2140,10 +2140,6 @@ static void __execlists_unhold(struct i915_request *rq) if (p->flags & I915_DEPENDENCY_WEAK) continue;

•                   /* Propagate any change in error status */
  

•                   if (rq->fence.error)
  

•                           i915_request_set_error_once(w, rq->fence.error);
  

•                   if (w->engine != rq->engine)
                            continue;
  
  

-- 2.32.0

On Fri, Aug 20, 2021 at 11:31:56AM -0700, Daniele Ceraolo Spurio wrote:

On 8/18/2021 11:16 PM, Matthew Brost wrote:

...

Kick tasklet after queuing a request so it submitted in a timely manner.

Fixes: 3a4cdf1982f0 ("drm/i915/guc: Implement GuC context operations for new inteface")

Is this actually a bug or just a performance issue? in the latter case I don't think we need a fixes tag.

Basically the tasklet won't get queued in certain ituations until the heartbeat ping. Didn't notice it as the tasklet is only used during flow control or after a full GT reset which both are rather rare. We can probably drop the fixes tag as GuC submission isn't on by default is still works without this fix.

...

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

drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 1 + 1 file changed, 1 insertion(+)

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 8f7a11e65ef5..d61f906105ef 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1050,6 +1050,7 @@ static inline void queue_request(struct i915_sched_engine *sched_engine, list_add_tail(&rq->sched.link, i915_sched_lookup_priolist(sched_engine, prio)); set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);

• tasklet_hi_schedule(&sched_engine->tasklet);

the caller of queue_request() already has a tasklet_hi_schedule in another branch of the if/else statement. Maybe we can have the caller own the kick to keep it in one place? Not a blocker.

I guess it could be:

bool kick = need_taklet()

if (kick) queue_requst() else kick = bypass() if (kick) kick_tasklet()

Idk, it that is better? I'll think on this and decide before the next post.

Matt

Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

Daniele

...

} static int guc_bypass_tasklet_submit(struct intel_guc *guc,

On 8/18/2021 11:16 PM, Matthew Brost wrote:

When unblocking a context, do not enable scheduling if the context is banned, guc_id invalid, or not registered.

Fixes: 62eaf0ae217d ("drm/i915/guc: Support request cancellation") Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: stable@vger.kernel.org

---

drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 3 +++ 1 file changed, 3 insertions(+)

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 d61f906105ef..e53a4ef7d442 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1586,6 +1586,9 @@ 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) ||
  

•      context_guc_id_invalid(ce) ||
  

•      !lrc_desc_registered(guc, ce->guc_id) ||
  !intel_context_is_pinned(ce) ||
  context_pending_disable(ce) ||
  context_blocked(ce) > 1)) {
  

This is getting to a lot of conditions. Maybe we can simplify it a bit? E.g it should be possible to check context_blocked, context_banned and context_pending_disable as a single op:

#define SCHED_STATE_MULTI_BLOCKED_MASK \  /* 2 or more blocks */     (SCHED_STATE_BLOCKED_MASK & ~SCHED_STATE_BLOCKED) #define SCHED_STATE_NO_UNBLOCK \     SCHED_STATE_MULTI_BLOCKED_MASK | \     SCHED_STATE_PENDING_DISABLE | \     SCHED_STATE_BANNED

Not a blocker.

Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

Daniele

On Tue, Aug 24, 2021 at 05:07:13PM -0700, Daniele Ceraolo Spurio wrote:

On 8/18/2021 11:16 PM, 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.

This is kind of a worrying explanation, as it implies an HW issue. AFAICS we no longer increase the context size with GuC submission, so the redzone should be in the same place relative to the base address of the context; although it is true that we have more objects in memory due to support the GuC, hitting the redzone consistently feels too much like a coincidence. When we write the watchdog regs there is a risk we're triggering a watchdog interrupt, which will cause the GuC to handle that; on a media reset, the GuC overwrites the context with the golden context in the ADS, are we sure that's not what is causing this problem? Looking in the ADS we set the context memcpy size to:

real_size = intel_engine_context_size(gt, engine_class);

but then we only initialize real_size - SKIP_SIZE(gt->i915), which IMO could be the real cause of the bug as the GuC memcpy starts at SKIP_SIZE().

Good analysis Daniele. This definitely seems to be the issue as the below patch appears to have fixed the failing selftest:

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c index 9f5f43a16182..c19ce71c9de9 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c @@ -358,6 +358,11 @@ static int guc_prep_golden_context(struct intel_guc *guc, u8 engine_class, guc_class; struct guc_gt_system_info *info, local_info;

+ /* Skip execlist and PPGTT registers + HWSP */ + const u32 lr_hw_context_size = 80 * sizeof(u32); + const u32 skip_size = LRC_PPHWSP_SZ * PAGE_SIZE + + lr_hw_context_size; + /* * Reserve the memory for the golden contexts and point GuC at it but * leave it empty for now. The context data will be filled in later @@ -396,7 +401,7 @@ static int guc_prep_golden_context(struct intel_guc *guc, if (!blob) continue;

- blob->ads.eng_state_size[guc_class] = real_size; + blob->ads.eng_state_size[guc_class] = real_size - skip_size; blob->ads.golden_context_lrca[guc_class] = addr_ggtt; addr_ggtt += alloc_size; } @@ -476,7 +481,8 @@ static void guc_init_golden_context(struct intel_guc *guc) continue; }

- GEM_BUG_ON(blob->ads.eng_state_size[guc_class] != real_size); + GEM_BUG_ON(blob->ads.eng_state_size[guc_class] != + real_size - skip_size); GEM_BUG_ON(blob->ads.golden_context_lrca[guc_class] != addr_ggtt); addr_ggtt += alloc_size;

This being said, IMO this actually a bug in the GuC firmware as it basically is doing:

memcpy(some_guc_dest, blob->ads.golden_context_lrca + guc_calculated_skip_size, blob->ads.eng_state_size);

IMO if the GuC is applying an internally calculated offset to blob->ads.golden_context_lrca it should substract that calculated size from blob->ads.eng_state_size.

e.g. the GuC should be doing:

memcpy(some_guc_dest, blob->ads.golden_context_lrca + guc_calculated_skip_size, blob->ads.eng_state_size - guc_calculated_skip_size);

We can bring this up with the GuC firmware team today, but in the meantime I'll include the above patch in the respin of this series as a workaround.

Matt

Daniele

...

v2: (Daniel Vetter)

• Add VLK ref in code to workaround

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..cdc6ae48a1e1 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 - VLK-22772 */
• 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;
  

  }

On 8/18/2021 11:16 PM, 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.

v2: (Daniel Vetter)

• Use ifdef instead of macros for selftests

v3: (Checkpatch)

• A space after 'switch' statement

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

drivers/gpu/drm/i915/gt/intel_context_types.h | 18 +++ .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 25 ++++ 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, 184 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..3a73f3117873 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -198,6 +198,24 @@ struct intel_context { */ u8 guc_prio; u32 guc_prio_count[GUC_CLIENT_PRIORITY_NUM];

+#ifdef CONFIG_DRM_I915_SELFTEST

• /**

• * @drop_schedule_enable: Force drop of schedule enable G2H for selftest
  

• */
  

• bool drop_schedule_enable;
• /**

• * @drop_schedule_disable: Force drop of schedule disable G2H for
  

• * selftest
  

• */
  

• bool drop_schedule_disable;
• /**

• * @drop_deregister: Force drop of deregister G2H for selftest
  

• */
  

• bool drop_deregister;

+#endif };

#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 e53a4ef7d442..e0e85e4ad512 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2635,6 +2635,13 @@ int intel_guc_deregister_done_process_msg(struct intel_guc *guc,

trace_intel_context_deregister_done(ce);

+#ifdef CONFIG_DRM_I915_SELFTEST

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

• ce->drop_deregister = false;
  

• return 0;
  

• }

+#endif

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

@@ -2689,10 +2696,24 @@ int intel_guc_sched_done_process_msg(struct intel_guc *guc, trace_intel_context_sched_done(ce);

if (context_pending_enable(ce)) { +#ifdef CONFIG_DRM_I915_SELFTEST

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

• 	ce->drop_schedule_enable = false;
  

• 	return 0;
  

• }
  

+#endif

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

+#ifdef CONFIG_DRM_I915_SELFTEST

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

• 	ce->drop_schedule_disable = false;
  

• 	return 0;
  

• }
  

+#endif

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

@@ -3069,3 +3090,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..264e2f705c17 --- /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;
  

• }
  

Would it be worth making the drop bitmask?

#ifdef CONFIG_DRM_I915_SELFTEST /** * @drop_g2h: Force drop of selected G2H for selftest */ u32 drop_g2h; #define I915_SELFTEST_DROP_GUC_SCHED_ENABLE BIT(0) .... #endif

So in the test instead of a switch you can use:

ce->drop_g2h = BIT(i);

Not a blocker.

• 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);

I think here we could use a small comment where we explain that the GT won't go idle if the scrubbing was not done correctly. With that:

Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

Daniele

• 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);

On 8/18/2021 11:16 PM, Matthew Brost wrote:

A context can get destroyed after cancelling a request so take a reference to context when cancelling a request.

What's the exact race? AFAICS __i915_request_skip does not have a context_put().

Daniele

Fixes: 62eaf0ae217d ("drm/i915/guc: Support request cancellation") Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

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 e0e85e4ad512..85f96d325048 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1620,8 +1620,10 @@ static void guc_context_cancel_request(struct intel_context *ce, struct i915_request *rq) { if (i915_sw_fence_signaled(&rq->submit)) {

• struct i915_sw_fence *fence = guc_context_block(ce);
  

• struct i915_sw_fence *fence;
  
  

• intel_context_get(ce);
  

• fence = guc_context_block(ce);
  

  i915_sw_fence_wait(fence); if (!i915_request_completed(rq)) { __i915_request_skip(rq);

@@ -1636,6 +1638,7 @@ static void guc_context_cancel_request(struct intel_context *ce, flush_work(&ce_to_guc(ce)->ct.requests.worker);

guc_context_unblock(ce);

• intel_context_put(ce);
  

  } }

On 8/24/2021 8:42 AM, Matthew Brost wrote:

On Fri, Aug 20, 2021 at 05:07:27PM -0700, Daniele Ceraolo Spurio wrote:

...

On 8/18/2021 11:16 PM, Matthew Brost wrote:

...

A context can get destroyed after cancelling a request so take a reference to context when cancelling a request.

What's the exact race? AFAICS __i915_request_skip does not have a context_put().

This commit message isn't quite right, it is really a context reset or a GT reset which could result in the context getting destroyed. I haven't actually seen this happen but this just being paranoid about ref counting. Can fix up the commit message.

ok, with an updated commit message:

Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

Daniele

Matt

...

Daniele

...

Fixes: 62eaf0ae217d ("drm/i915/guc: Support request cancellation") Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

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 e0e85e4ad512..85f96d325048 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1620,8 +1620,10 @@ static void guc_context_cancel_request(struct intel_context *ce, struct i915_request *rq) { if (i915_sw_fence_signaled(&rq->submit)) {

• struct i915_sw_fence *fence = guc_context_block(ce);
  

• struct i915_sw_fence *fence;
  

• intel_context_get(ce);
  

• fence = guc_context_block(ce);
  i915_sw_fence_wait(fence);
  if (!i915_request_completed(rq)) {
  	__i915_request_skip(rq);
  

@@ -1636,6 +1638,7 @@ static void guc_context_cancel_request(struct intel_context *ce, flush_work(&ce_to_guc(ce)->ct.requests.worker); guc_context_unblock(ce);

• intel_context_put(ce);
  

  } }

On 8/18/2021 11:16 PM, Matthew Brost wrote:

Before we did some clever tricks to not use the a lock when touching guc_state.sched_state in certain cases. Don't do that, enforce the use of the lock.

Part of this is removing a dead code path from guc_lrc_desc_pin where a context could be deregistered when the aforementioned function was called from the submission path. Remove this dead code and add a GEM_BUG_ON if this path is ever attempted to be used.

v2: (kernel test robo )

• Add __maybe_unused to sched_state_is_init()

Signed-off-by: Matthew Brost matthew.brost@intel.com Reported-by: kernel test robot lkp@intel.com

---

.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 58 ++++++++++--------- 1 file changed, 32 insertions(+), 26 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 85f96d325048..fa87470ea576 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -150,11 +150,23 @@ static inline void clr_context_registered(struct intel_context *ce) #define SCHED_STATE_BLOCKED_MASK (0xfff << SCHED_STATE_BLOCKED_SHIFT) static inline void init_sched_state(struct intel_context *ce) {

• /* Only should be called from guc_lrc_desc_pin() */

• lockdep_assert_held(&ce->guc_state.lock); atomic_set(&ce->guc_sched_state_no_lock, 0); ce->guc_state.sched_state &= SCHED_STATE_BLOCKED_MASK; }

+__maybe_unused +static bool sched_state_is_init(struct intel_context *ce) +{

• /*

• * XXX: Kernel contexts can have SCHED_STATE_NO_LOCK_REGISTERED after
  

• * suspend.
  

• */
  

This seems like something we want to fix. Not a blocker for this, but we can add it to the list.

• return !(atomic_read(&ce->guc_sched_state_no_lock) &

•  ~SCHED_STATE_NO_LOCK_REGISTERED) &&
  

• !(ce->guc_state.sched_state &= ~SCHED_STATE_BLOCKED_MASK);
  

+}

• static inline bool context_wait_for_deregister_to_register(struct intel_context *ce) {

@@ -165,7 +177,7 @@ context_wait_for_deregister_to_register(struct intel_context *ce) static inline void set_context_wait_for_deregister_to_register(struct intel_context *ce) {

• /* Only should be called from guc_lrc_desc_pin() without lock */

• lockdep_assert_held(&ce->guc_state.lock); ce->guc_state.sched_state |= SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER; }

@@ -605,9 +617,7 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) bool pending_disable, pending_enable, deregister, destroyed, banned;

xa_for_each(&guc->context_lookup, index, ce) {

• /* Flush context */
  

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

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

  /*

  • Once we are at this point submission_disabled() is guaranteed

@@ -623,6 +633,8 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) banned = context_banned(ce); init_sched_state(ce);

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

• if (pending_enable || destroyed || deregister) { decr_outstanding_submission_g2h(guc); if (deregister)

@@ -1325,6 +1337,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) int ret = 0;

GEM_BUG_ON(!engine->mask);

• GEM_BUG_ON(!sched_state_is_init(ce));

  /*

  • Ensure LRC + CT vmas are is same region as write barrier is done

@@ -1353,7 +1366,6 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) desc->priority = ce->guc_prio; desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD; guc_context_policy_init(engine, desc);

• init_sched_state(ce);

  /*

  • The context_lookup xarray is used to determine if the hardware

@@ -1364,26 +1376,23 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) * registering this context. */ if (context_registered) {

• bool disabled;
  

• unsigned long flags;
  

• trace_intel_context_steal_guc_id(ce);

• if (!loop) {
  

• GEM_BUG_ON(!loop);
  

• /* Seal race with Reset */
  

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

• disabled = submission_disabled(guc);
  

• if (likely(!disabled)) {
  set_context_wait_for_deregister_to_register(ce);
  intel_context_get(ce);
  

• } else {
  

• 	bool disabled;
  

• 	unsigned long flags;
  

• 	/* Seal race with Reset */
  

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

• 	disabled = submission_disabled(guc);
  

• 	if (likely(!disabled)) {
  

• 		set_context_wait_for_deregister_to_register(ce);
  

• 		intel_context_get(ce);
  

• 	}
  

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

• 	if (unlikely(disabled)) {
  

• 		reset_lrc_desc(guc, desc_idx);
  

• 		return 0;	/* Will get registered later */
  

• 	}
  

• }
  

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

• if (unlikely(disabled)) {
  

• 	reset_lrc_desc(guc, desc_idx);
  

• 	return 0;	/* Will get registered later */
  

  }

  /*

@@ -1392,10 +1401,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) */ with_intel_runtime_pm(runtime_pm, wakeref) ret = deregister_context(ce, ce->guc_id, loop);

• if (unlikely(ret == -EBUSY)) {
  

• 	clr_context_wait_for_deregister_to_register(ce);
  

• 	intel_context_put(ce);
  

Why is the EBUSY case not applicable anymore?

Daniele

• } else if (unlikely(ret == -ENODEV)) {
  

• if (unlikely(ret == -ENODEV)) {
  ret = 0;	/* Will get registered later */
  

  } } else {

On 8/24/2021 6:44 PM, Matthew Brost wrote:

On Tue, Aug 24, 2021 at 06:20:49PM -0700, Daniele Ceraolo Spurio wrote:

...

On 8/18/2021 11:16 PM, Matthew Brost wrote:

...

Before we did some clever tricks to not use the a lock when touching guc_state.sched_state in certain cases. Don't do that, enforce the use of the lock.

Part of this is removing a dead code path from guc_lrc_desc_pin where a context could be deregistered when the aforementioned function was called from the submission path. Remove this dead code and add a GEM_BUG_ON if this path is ever attempted to be used.

v2: (kernel test robo ) - Add __maybe_unused to sched_state_is_init()

Signed-off-by: Matthew Brost matthew.brost@intel.com Reported-by: kernel test robot lkp@intel.com

---

.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 58 ++++++++++--------- 1 file changed, 32 insertions(+), 26 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 85f96d325048..fa87470ea576 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -150,11 +150,23 @@ static inline void clr_context_registered(struct intel_context *ce) #define SCHED_STATE_BLOCKED_MASK (0xfff << SCHED_STATE_BLOCKED_SHIFT) static inline void init_sched_state(struct intel_context *ce) {

• /* Only should be called from guc_lrc_desc_pin() */

• lockdep_assert_held(&ce->guc_state.lock); atomic_set(&ce->guc_sched_state_no_lock, 0); ce->guc_state.sched_state &= SCHED_STATE_BLOCKED_MASK; }

+__maybe_unused +static bool sched_state_is_init(struct intel_context *ce) +{

• /*

• * XXX: Kernel contexts can have SCHED_STATE_NO_LOCK_REGISTERED after
  

• * suspend.
  

• */
  

This seems like something we want to fix. Not a blocker for this, but we can add it to the list.

Right, hence the comment in the code.

...

...

• return !(atomic_read(&ce->guc_sched_state_no_lock) &

•  ~SCHED_STATE_NO_LOCK_REGISTERED) &&
  

• !(ce->guc_state.sched_state &= ~SCHED_STATE_BLOCKED_MASK);
  

+}

• static inline bool context_wait_for_deregister_to_register(struct intel_context *ce) {

@@ -165,7 +177,7 @@ context_wait_for_deregister_to_register(struct intel_context *ce) static inline void set_context_wait_for_deregister_to_register(struct intel_context *ce) {

• /* Only should be called from guc_lrc_desc_pin() without lock */

• lockdep_assert_held(&ce->guc_state.lock); ce->guc_state.sched_state |= SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER; }

@@ -605,9 +617,7 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) bool pending_disable, pending_enable, deregister, destroyed, banned; xa_for_each(&guc->context_lookup, index, ce) {

• /* Flush context */
  spin_lock_irqsave(&ce->guc_state.lock, flags);
  

• spin_unlock_irqrestore(&ce->guc_state.lock, flags);
  /*
   * Once we are at this point submission_disabled() is guaranteed
  

@@ -623,6 +633,8 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) banned = context_banned(ce); init_sched_state(ce);

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

• if (pending_enable || destroyed || deregister) {
  	decr_outstanding_submission_g2h(guc);
  	if (deregister)
  

@@ -1325,6 +1337,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) int ret = 0; GEM_BUG_ON(!engine->mask);

• GEM_BUG_ON(!sched_state_is_init(ce)); /*
  • Ensure LRC + CT vmas are is same region as write barrier is done

@@ -1353,7 +1366,6 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) desc->priority = ce->guc_prio; desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD; guc_context_policy_init(engine, desc);

• init_sched_state(ce); /*
  • The context_lookup xarray is used to determine if the hardware

@@ -1364,26 +1376,23 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) * registering this context. */ if (context_registered) {

• bool disabled;
  

• unsigned long flags;
  

• trace_intel_context_steal_guc_id(ce);
  

• if (!loop) {
  

• GEM_BUG_ON(!loop);
  

• /* Seal race with Reset */
  

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

• disabled = submission_disabled(guc);
  

• if (likely(!disabled)) {
  	set_context_wait_for_deregister_to_register(ce);
  	intel_context_get(ce);
  

• } else {
  

• 	bool disabled;
  

• 	unsigned long flags;
  

• 	/* Seal race with Reset */
  

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

• 	disabled = submission_disabled(guc);
  

• 	if (likely(!disabled)) {
  

• 		set_context_wait_for_deregister_to_register(ce);
  

• 		intel_context_get(ce);
  

• 	}
  

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

• 	if (unlikely(disabled)) {
  

• 		reset_lrc_desc(guc, desc_idx);
  

• 		return 0;	/* Will get registered later */
  

• 	}
  

• }
  

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

• if (unlikely(disabled)) {
  

• 	reset_lrc_desc(guc, desc_idx);
  

• 	return 0;	/* Will get registered later */
  }
  /*
  

@@ -1392,10 +1401,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) */ with_intel_runtime_pm(runtime_pm, wakeref) ret = deregister_context(ce, ce->guc_id, loop);

• if (unlikely(ret == -EBUSY)) {
  

• 	clr_context_wait_for_deregister_to_register(ce);
  

• 	intel_context_put(ce);
  

Why is the EBUSY case not applicable anymore?

Commmit message cover this - this is dead code that can't be reached in the current code nor can be it be reached in upcoming code. Or put another way loop is always true thus we can't get -EBUSY from deregister_context().

ok, I hadn't realized that we could get -EBUSY only if loop=false.

Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

Daniele

Matt

...

Daniele

...

• } else if (unlikely(ret == -ENODEV)) {
  

• if (unlikely(ret == -ENODEV)) {
  	ret = 0;	/* Will get registered later */
  }
  

  } else {

On 8/18/2021 11:16 PM, Matthew Brost wrote:

Reset LRC descriptor if a context register returns -ENODEV as this means we are mid-reset.

Fixes: eb5e7da736f3 ("drm/i915/guc: Reset implementation for new GuC interface") Signed-off-by: Matthew Brost matthew.brost@intel.com

Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

Daniele

---

drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 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 fa87470ea576..4cf5a565f08e 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1407,10 +1407,12 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) } else { with_intel_runtime_pm(runtime_pm, wakeref) ret = register_context(ce, loop);

• if (unlikely(ret == -EBUSY))
  

• if (unlikely(ret == -EBUSY)) {
  

• 	reset_lrc_desc(guc, desc_idx);
  

• } else if (unlikely(ret == -ENODEV)) {
  reset_lrc_desc(guc, desc_idx);
  

• else if (unlikely(ret == -ENODEV))
  ret = 0;	/* Will get registered later */
  

• }
  

  }

  return ret;

On Fri, Aug 20, 2021 at 05:25:41PM -0700, Daniele Ceraolo Spurio wrote:

On 8/18/2021 11:16 PM, Matthew Brost wrote:

...

It isn't safe to scrub for missing G2H or continue with the reset until all G2H processing is complete. Flush the G2H work queue during reset to ensure it is done running.

Might be worth moving this patch closer to "drm/i915/guc: Process all G2H message at once in work queue".

Sure.

...

Fixes: eb5e7da736f3 ("drm/i915/guc: Reset implementation for new GuC interface") Signed-off-by: Matthew Brost matthew.brost@intel.com

---

.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 18 ++---------------- 1 file changed, 2 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 4cf5a565f08e..9a53bae367b1 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -714,8 +714,6 @@ static void guc_flush_submissions(struct intel_guc *guc) void intel_guc_submission_reset_prepare(struct intel_guc *guc) {

• int i;
• if (unlikely(!guc_submission_initialized(guc))) { /* Reset called during driver load? GuC not yet initialised! */ return;

@@ -731,20 +729,8 @@ void intel_guc_submission_reset_prepare(struct intel_guc *guc) guc_flush_submissions(guc);

• /*

• * Handle any outstanding G2Hs before reset. Call IRQ handler directly
  

• * each pass as interrupt have been disabled. We always scrub for
  

• * outstanding G2H as it is possible for outstanding_submission_g2h to
  

• * be incremented after the context state update.
  

• */
  

• for (i = 0; i < 4 && atomic_read(&guc->outstanding_submission_g2h); ++i) {

• intel_guc_to_host_event_handler(guc);
  

-#define wait_for_reset(guc, wait_var) \

• intel_guc_wait_for_pending_msg(guc, wait_var, false, (HZ / 20))
  

• do {
  

• 	wait_for_reset(guc, &guc->outstanding_submission_g2h);
  

• } while (!list_empty(&guc->ct.requests.incoming));
  

• }

• flush_work(&guc->ct.requests.worker);

We're now not waiting in the requests anymore, just ensuring that the processing of the ones we already received is done. Is this intended? We do still handle the remaining oustanding submission in the scrub so it's functionally correct, but the commit message doesn't state the change in waiting behavior, so wanted to double check it was planned.

Yes, it is planned as scrub code should be able to cope with any missing G2H. Will update the commit message to reflect that.

Matt

Daniele

...

scrub_guc_desc_for_outstanding_g2h(guc); }

On Wed, Aug 25, 2021 at 09:52:06AM -0700, Daniele Ceraolo Spurio wrote:

On 8/18/2021 11:16 PM, Matthew Brost wrote:

...

Rework and simplify the locking with GuC subission. Drop sched_state_no_lock and move all fields under the guc_state.sched_state and protect all these fields with guc_state.lock . This requires changing the locking hierarchy from guc_state.lock -> sched_engine.lock to sched_engine.lock -> guc_state.lock.

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

drivers/gpu/drm/i915/gt/intel_context_types.h | 5 +- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 186 ++++++++---------- drivers/gpu/drm/i915/i915_trace.h | 6 +- 3 files changed, 89 insertions(+), 108 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index c06171ee8792..d5d643b04d54 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -161,7 +161,7 @@ struct intel_context { * sched_state: scheduling state of this context using GuC * submission */

• u16 sched_state;
  

• u32 sched_state;
  

  /*
  • fences: maintains of list of requests that have a submit
  • fence related to GuC submission

@@ -178,9 +178,6 @@ struct intel_context { struct list_head requests; } guc_active;

• /* GuC scheduling state flags that do not require a lock. */
• atomic_t guc_sched_state_no_lock;
• /* GuC LRC descriptor ID */ u16 guc_id;

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 053f4485d6e9..509b298e7cf3 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -72,86 +72,23 @@ guc_create_virtual(struct intel_engine_cs **siblings, unsigned int count); #define GUC_REQUEST_SIZE 64 /* bytes */ -/*

• • Below is a set of functions which control the GuC scheduling state which do
• • not require a lock as all state transitions are mutually exclusive. i.e. It
• • is not possible for the context pinning code and submission, for the same
• • context, to be executing simultaneously. We still need an atomic as it is
• • possible for some of the bits to changing at the same time though.
• */

-#define SCHED_STATE_NO_LOCK_ENABLED BIT(0) -#define SCHED_STATE_NO_LOCK_PENDING_ENABLE BIT(1) -#define SCHED_STATE_NO_LOCK_REGISTERED BIT(2) -static inline bool context_enabled(struct intel_context *ce) -{

• return (atomic_read(&ce->guc_sched_state_no_lock) &

• SCHED_STATE_NO_LOCK_ENABLED);
  

-}

-static inline void set_context_enabled(struct intel_context *ce) -{

• atomic_or(SCHED_STATE_NO_LOCK_ENABLED, &ce->guc_sched_state_no_lock);

-}

-static inline void clr_context_enabled(struct intel_context *ce) -{

• atomic_and((u32)~SCHED_STATE_NO_LOCK_ENABLED,

•    &ce->guc_sched_state_no_lock);
  

-}

-static inline bool context_pending_enable(struct intel_context *ce) -{

• return (atomic_read(&ce->guc_sched_state_no_lock) &

• SCHED_STATE_NO_LOCK_PENDING_ENABLE);
  

-}

-static inline void set_context_pending_enable(struct intel_context *ce) -{

• atomic_or(SCHED_STATE_NO_LOCK_PENDING_ENABLE,

•   &ce->guc_sched_state_no_lock);
  

-}

-static inline void clr_context_pending_enable(struct intel_context *ce) -{

• atomic_and((u32)~SCHED_STATE_NO_LOCK_PENDING_ENABLE,

•    &ce->guc_sched_state_no_lock);
  

-}

-static inline bool context_registered(struct intel_context *ce) -{

• return (atomic_read(&ce->guc_sched_state_no_lock) &

• SCHED_STATE_NO_LOCK_REGISTERED);
  

-}

-static inline void set_context_registered(struct intel_context *ce) -{

• atomic_or(SCHED_STATE_NO_LOCK_REGISTERED,

•   &ce->guc_sched_state_no_lock);
  

-}

-static inline void clr_context_registered(struct intel_context *ce) -{

• atomic_and((u32)~SCHED_STATE_NO_LOCK_REGISTERED,

•    &ce->guc_sched_state_no_lock);
  

-}

• /*
  • Below is a set of functions which control the GuC scheduling state which
• • require a lock, aside from the special case where the functions are called
• • from guc_lrc_desc_pin(). In that case it isn't possible for any other code
• • path to be executing on the context.

• • require a lock.
  */ #define SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER BIT(0) #define SCHED_STATE_DESTROYED BIT(1) #define SCHED_STATE_PENDING_DISABLE BIT(2) #define SCHED_STATE_BANNED BIT(3)

-#define SCHED_STATE_BLOCKED_SHIFT 4 +#define SCHED_STATE_ENABLED BIT(4) +#define SCHED_STATE_PENDING_ENABLE BIT(5) +#define SCHED_STATE_REGISTERED BIT(6) +#define SCHED_STATE_BLOCKED_SHIFT 7 #define SCHED_STATE_BLOCKED BIT(SCHED_STATE_BLOCKED_SHIFT) #define SCHED_STATE_BLOCKED_MASK (0xfff << SCHED_STATE_BLOCKED_SHIFT) static inline void init_sched_state(struct intel_context *ce) { lockdep_assert_held(&ce->guc_state.lock);

• atomic_set(&ce->guc_sched_state_no_lock, 0); ce->guc_state.sched_state &= SCHED_STATE_BLOCKED_MASK; }

@@ -162,9 +99,8 @@ static bool sched_state_is_init(struct intel_context *ce) * XXX: Kernel contexts can have SCHED_STATE_NO_LOCK_REGISTERED after * suspend. */

• return !(atomic_read(&ce->guc_sched_state_no_lock) &

•  ~SCHED_STATE_NO_LOCK_REGISTERED) &&
  

• !(ce->guc_state.sched_state &= ~SCHED_STATE_BLOCKED_MASK);
  

• return !(ce->guc_state.sched_state &=

•  ~(SCHED_STATE_BLOCKED_MASK | SCHED_STATE_REGISTERED));
  

  } static inline bool

@@ -237,6 +173,57 @@ static inline void clr_context_banned(struct intel_context *ce) ce->guc_state.sched_state &= ~SCHED_STATE_BANNED; } +static inline bool context_enabled(struct intel_context *ce) +{

• return ce->guc_state.sched_state & SCHED_STATE_ENABLED;

+}

+static inline void set_context_enabled(struct intel_context *ce) +{

• lockdep_assert_held(&ce->guc_state.lock);
• ce->guc_state.sched_state |= SCHED_STATE_ENABLED;

+}

+static inline void clr_context_enabled(struct intel_context *ce) +{

• lockdep_assert_held(&ce->guc_state.lock);
• ce->guc_state.sched_state &= ~SCHED_STATE_ENABLED;

+}

+static inline bool context_pending_enable(struct intel_context *ce) +{

• return ce->guc_state.sched_state & SCHED_STATE_PENDING_ENABLE;

+}

+static inline void set_context_pending_enable(struct intel_context *ce) +{

• lockdep_assert_held(&ce->guc_state.lock);
• ce->guc_state.sched_state |= SCHED_STATE_PENDING_ENABLE;

+}

+static inline void clr_context_pending_enable(struct intel_context *ce) +{

• lockdep_assert_held(&ce->guc_state.lock);
• ce->guc_state.sched_state &= ~SCHED_STATE_PENDING_ENABLE;

+}

+static inline bool context_registered(struct intel_context *ce) +{

• return ce->guc_state.sched_state & SCHED_STATE_REGISTERED;

+}

+static inline void set_context_registered(struct intel_context *ce) +{

• lockdep_assert_held(&ce->guc_state.lock);
• ce->guc_state.sched_state |= SCHED_STATE_REGISTERED;

+}

+static inline void clr_context_registered(struct intel_context *ce) +{

• lockdep_assert_held(&ce->guc_state.lock);
• ce->guc_state.sched_state &= ~SCHED_STATE_REGISTERED;

+}

• static inline u32 context_blocked(struct intel_context *ce) { return (ce->guc_state.sched_state & SCHED_STATE_BLOCKED_MASK) >>

@@ -245,7 +232,6 @@ static inline u32 context_blocked(struct intel_context *ce) static inline void incr_context_blocked(struct intel_context *ce) {

• lockdep_assert_held(&ce->engine->sched_engine->lock); lockdep_assert_held(&ce->guc_state.lock); ce->guc_state.sched_state += SCHED_STATE_BLOCKED;

@@ -255,7 +241,6 @@ static inline void incr_context_blocked(struct intel_context *ce) static inline void decr_context_blocked(struct intel_context *ce) {

• lockdep_assert_held(&ce->engine->sched_engine->lock); lockdep_assert_held(&ce->guc_state.lock); GEM_BUG_ON(!context_blocked(ce)); /* Underflow check */

@@ -450,6 +435,8 @@ static int guc_add_request(struct intel_guc *guc, struct i915_request *rq) u32 g2h_len_dw = 0; bool enabled;

• lockdep_assert_held(&rq->engine->sched_engine->lock);
• /*
  • Corner case where requests were sitting in the priority list or a
  • request resubmitted after the context was banned.

@@ -457,7 +444,7 @@ static int guc_add_request(struct intel_guc *guc, struct i915_request *rq) if (unlikely(intel_context_is_banned(ce))) { i915_request_put(i915_request_mark_eio(rq)); intel_engine_signal_breadcrumbs(ce->engine);

• goto out;
  

• return 0;
  

  } GEM_BUG_ON(!atomic_read(&ce->guc_id_ref));

@@ -470,9 +457,11 @@ static int guc_add_request(struct intel_guc *guc, struct i915_request *rq) if (unlikely(!lrc_desc_registered(guc, ce->guc_id))) { err = guc_lrc_desc_pin(ce, false); if (unlikely(err))

• 	goto out;
  

• 	return err;
  

  }
• spin_lock(&ce->guc_state.lock);
• /*
  • The request / context will be run on the hardware when scheduling
  • gets enabled in the unblock.

@@ -507,6 +496,7 @@ static int guc_add_request(struct intel_guc *guc, struct i915_request *rq) trace_i915_request_guc_submit(rq); out:

• spin_unlock(&ce->guc_state.lock); return err; }

@@ -727,8 +717,6 @@ void intel_guc_submission_reset_prepare(struct intel_guc *guc) spin_lock_irq(&guc_to_gt(guc)->irq_lock); spin_unlock_irq(&guc_to_gt(guc)->irq_lock);

• guc_flush_submissions(guc);
• flush_work(&guc->ct.requests.worker); scrub_guc_desc_for_outstanding_g2h(guc);

@@ -1133,7 +1121,11 @@ static int steal_guc_id(struct intel_guc *guc) list_del_init(&ce->guc_id_link); guc_id = ce->guc_id;

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

  clr_context_registered(ce);

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

• set_context_guc_id_invalid(ce); return guc_id; } else {

@@ -1169,6 +1161,8 @@ static int pin_guc_id(struct intel_guc *guc, struct intel_context *ce) try_again: spin_lock_irqsave(&guc->contexts_lock, flags);

• might_lock(&ce->guc_state.lock);
• if (context_guc_id_invalid(ce)) { ret = assign_guc_id(guc, &ce->guc_id); if (ret)

@@ -1248,8 +1242,13 @@ static int register_context(struct intel_context *ce, bool loop) trace_intel_context_register(ce); ret = __guc_action_register_context(guc, ce->guc_id, offset, loop);

• if (likely(!ret))

• if (likely(!ret)) {

• unsigned long flags;
  

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

  set_context_registered(ce);

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

• } return ret; }

@@ -1525,7 +1524,6 @@ static u16 prep_context_pending_disable(struct intel_context *ce) static struct i915_sw_fence *guc_context_block(struct intel_context *ce) { struct intel_guc *guc = ce_to_guc(ce);

• struct i915_sched_engine *sched_engine = ce->engine->sched_engine; unsigned long flags; struct intel_runtime_pm *runtime_pm = ce->engine->uncore->rpm; intel_wakeref_t wakeref;

@@ -1534,13 +1532,7 @@ static struct i915_sw_fence *guc_context_block(struct intel_context *ce) spin_lock_irqsave(&ce->guc_state.lock, flags);

• /*

• * Sync with submission path, increment before below changes to context
  

• * state.
  

• */
  

• spin_lock(&sched_engine->lock); incr_context_blocked(ce);
• spin_unlock(&sched_engine->lock); enabled = context_enabled(ce); if (unlikely(!enabled || submission_disabled(guc))) {

@@ -1569,7 +1561,6 @@ static struct i915_sw_fence *guc_context_block(struct intel_context *ce) static void guc_context_unblock(struct intel_context *ce) { struct intel_guc *guc = ce_to_guc(ce);

• struct i915_sched_engine *sched_engine = ce->engine->sched_engine; unsigned long flags; struct intel_runtime_pm *runtime_pm = ce->engine->uncore->rpm; intel_wakeref_t wakeref;

@@ -1594,13 +1585,7 @@ static void guc_context_unblock(struct intel_context *ce) intel_context_get(ce); }

• /*

• * Sync with submission path, decrement after above changes to context
  

• * state.
  

• */
  

• spin_lock(&sched_engine->lock); decr_context_blocked(ce);
• spin_unlock(&sched_engine->lock); spin_unlock_irqrestore(&ce->guc_state.lock, flags);

@@ -1710,7 +1695,9 @@ static void guc_context_sched_disable(struct intel_context *ce) if (submission_disabled(guc) || context_guc_id_invalid(ce) || !lrc_desc_registered(guc, ce->guc_id)) {

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

We do take this lock a few lines below this. Would it be worth just moving that up and do everything under the lock?

Good catch. Yes, we should move everything under the which actually makes all of this code quite a bit simpler too. Will fix.

Matt

Anyway, all calls to the updated set/clr functions have been updated to be correctly locked, so:

Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

Daniele

...

clr_context_enabled(ce);

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

  goto unpin; }

@@ -1760,7 +1747,6 @@ static inline void guc_lrc_desc_unpin(struct intel_context *ce) GEM_BUG_ON(ce != __get_context(guc, ce->guc_id)); GEM_BUG_ON(context_enabled(ce));

• clr_context_registered(ce); deregister_context(ce, ce->guc_id, true); }

@@ -1833,8 +1819,10 @@ static void guc_context_destroy(struct kref *kref) /* Seal race with Reset */ spin_lock_irqsave(&ce->guc_state.lock, flags); disabled = submission_disabled(guc);

• if (likely(!disabled))

• if (likely(!disabled)) { set_context_destroyed(ce);

• clr_context_registered(ce);
  

• } spin_unlock_irqrestore(&ce->guc_state.lock, flags); if (unlikely(disabled)) { release_guc_id(guc, ce);

@@ -2697,8 +2685,7 @@ int intel_guc_sched_done_process_msg(struct intel_guc *guc, (!context_pending_enable(ce) && !context_pending_disable(ce)))) { drm_err(&guc_to_gt(guc)->i915->drm,

• 	"Bad context sched_state 0x%x, 0x%x, desc_idx %u",
  

• 	atomic_read(&ce->guc_sched_state_no_lock),
  

• 	"Bad context sched_state 0x%x, desc_idx %u",
  ce->guc_state.sched_state, desc_idx);
  

  return -EPROTO; }

@@ -2713,7 +2700,9 @@ int intel_guc_sched_done_process_msg(struct intel_guc *guc, } #endif

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

  clr_context_pending_enable(ce);

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

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

@@ -2987,9 +2976,8 @@ void intel_guc_submission_print_context_info(struct intel_guc *guc, atomic_read(&ce->pin_count)); drm_printf(p, "\t\tGuC ID Ref Count: %u\n", atomic_read(&ce->guc_id_ref));

• drm_printf(p, "\t\tSchedule State: 0x%x, 0x%x\n\n",
  

• 	   ce->guc_state.sched_state,
  

• 	   atomic_read(&ce->guc_sched_state_no_lock));
  

• drm_printf(p, "\t\tSchedule State: 0x%x\n\n",
  

• 	   ce->guc_state.sched_state);
  

  guc_log_context_priority(p, ce); }

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index 806ad688274b..0a77eb2944b5 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -903,7 +903,6 @@ DECLARE_EVENT_CLASS(intel_context, __field(u32, guc_id) __field(int, pin_count) __field(u32, sched_state)

• 	     __field(u32, guc_sched_state_no_lock)
       __field(u8, guc_prio)
       ),
  

@@ -911,15 +910,12 @@ DECLARE_EVENT_CLASS(intel_context, __entry->guc_id = ce->guc_id; __entry->pin_count = atomic_read(&ce->pin_count); __entry->sched_state = ce->guc_state.sched_state;

• 	   __entry->guc_sched_state_no_lock =
  

• 	   atomic_read(&ce->guc_sched_state_no_lock);
     __entry->guc_prio = ce->guc_prio;
     ),
  

•     TP_printk("guc_id=%d, pin_count=%d sched_state=0x%x,0x%x, guc_prio=%u",
  

•     TP_printk("guc_id=%d, pin_count=%d sched_state=0x%x, guc_prio=%u",
        __entry->guc_id, __entry->pin_count,
        __entry->sched_state,
  

• 	      __entry->guc_sched_state_no_lock,
        __entry->guc_prio)
  

  );

On 8/18/2021 11:16 PM, Matthew Brost wrote:

Lock the xarray and take ref to the context if needed.

v2: (Checkpatch)

• Add new line after declaration

(Daniel Vetter)

• Correct put / get accounting in xa_for_loops

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

.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 103 +++++++++++++++--- 1 file changed, 88 insertions(+), 15 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 509b298e7cf3..5f77f25322ca 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -606,8 +606,18 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) unsigned long index, flags; bool pending_disable, pending_enable, deregister, destroyed, banned;

• xa_lock_irqsave(&guc->context_lookup, flags); xa_for_each(&guc->context_lookup, index, ce) {

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

• /*
  

•  * Corner case where the ref count on the object is zero but and
  

•  * deregister G2H was lost. In this case we don't touch the ref
  

•  * count and finish the destroy of the context.
  

•  */
  

• bool do_put = kref_get_unless_zero(&ce->ref);
  

• xa_unlock(&guc->context_lookup);
  

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

  /*

  • Once we are at this point submission_disabled() is guaranteed

@@ -623,7 +633,9 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) banned = context_banned(ce); init_sched_state(ce);

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

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

• GEM_BUG_ON(!do_put && !destroyed);
  
  

  if (pending_enable || destroyed || deregister) { decr_outstanding_submission_g2h(guc);

@@ -646,13 +658,19 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) } intel_context_sched_disable_unpin(ce); decr_outstanding_submission_g2h(guc);

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

• 	spin_lock(&ce->guc_state.lock);
  guc_blocked_fence_complete(ce);
  

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

• 	spin_unlock(&ce->guc_state.lock);
  
  intel_context_put(ce);
  

  }

• if (do_put)
  

• 	intel_context_put(ce);
  

is it safe to do the put outside the xa_lock, in case the refcount goes to zero with this? I know it is unlikely because the refcount was > 0 if do_put is true, but it might've gone down between us checking earlier and now.

• xa_lock(&guc->context_lookup);
  

  }

• xa_unlock_irqrestore(&guc->context_lookup, flags); }

  static inline bool

@@ -873,16 +891,29 @@ void intel_guc_submission_reset(struct intel_guc *guc, bool stalled) { struct intel_context *ce; unsigned long index;

• unsigned long flags;

  if (unlikely(!guc_submission_initialized(guc))) { /* Reset called during driver load? GuC not yet initialised! */ return; }

• xa_for_each(&guc->context_lookup, index, ce)

• xa_lock_irqsave(&guc->context_lookup, flags);

• xa_for_each(&guc->context_lookup, index, ce) {

• if (!kref_get_unless_zero(&ce->ref))
  

• 	continue;
  

• xa_unlock(&guc->context_lookup);
  

• if (intel_context_is_pinned(ce)) __guc_reset_context(ce, stalled);

• intel_context_put(ce);
  

• xa_lock(&guc->context_lookup);
  

• }

• xa_unlock_irqrestore(&guc->context_lookup, flags);

• /* GuC is blown away, drop all references to contexts */ xa_destroy(&guc->context_lookup); }

@@ -957,11 +988,24 @@ void intel_guc_submission_cancel_requests(struct intel_guc *guc) { struct intel_context *ce; unsigned long index;

• unsigned long flags;
• xa_lock_irqsave(&guc->context_lookup, flags);
• xa_for_each(&guc->context_lookup, index, ce) {

• if (!kref_get_unless_zero(&ce->ref))
  

• 	continue;
  

• xa_unlock(&guc->context_lookup);
  
  

• xa_for_each(&guc->context_lookup, index, ce) if (intel_context_is_pinned(ce)) guc_cancel_context_requests(ce);

• intel_context_put(ce);
  

• xa_lock(&guc->context_lookup);
  

• }

• xa_unlock_irqrestore(&guc->context_lookup, flags);

• guc_cancel_sched_engine_requests(guc->sched_engine);

  /* GuC is blown away, drop all references to contexts */

@@ -2850,21 +2894,28 @@ void intel_guc_find_hung_context(struct intel_engine_cs *engine) struct intel_context *ce; struct i915_request *rq; unsigned long index;

• unsigned long flags;

  /* Reset called during driver load? GuC not yet initialised! */ if (unlikely(!guc_submission_initialized(guc))) return;

• xa_lock_irqsave(&guc->context_lookup, flags); xa_for_each(&guc->context_lookup, index, ce) {

• if (!intel_context_is_pinned(ce))
  

• if (!kref_get_unless_zero(&ce->ref))
  continue;
  
  

• xa_unlock(&guc->context_lookup);
  

• if (!intel_context_is_pinned(ce))
  

• 	goto next;
  

• if (intel_engine_is_virtual(ce->engine)) { if (!(ce->engine->mask & engine->mask))

• 		continue;
  

• 		goto next;
  

  } else { if (ce->engine != engine)

• 		continue;
  

• 		goto next;
  

  }

  list_for_each_entry(rq, &ce->guc_active.requests, sched.link) {

@@ -2874,9 +2925,17 @@ void intel_guc_find_hung_context(struct intel_engine_cs *engine) intel_engine_set_hung_context(engine, ce);

	/* Can only cope with one hang at a time... */

• 	return;
  

• 	intel_context_put(ce);
  

• 	xa_lock(&guc->context_lookup);
  

• 	goto done;
  

  }

+next:

• intel_context_put(ce);
  

• xa_lock(&guc->context_lookup);
  

nit: extra newline

Daniele

} +done:

• xa_unlock_irqrestore(&guc->context_lookup, flags); }

  void intel_guc_dump_active_requests(struct intel_engine_cs *engine,

@@ -2892,23 +2951,34 @@ void intel_guc_dump_active_requests(struct intel_engine_cs *engine, if (unlikely(!guc_submission_initialized(guc))) return;

• xa_lock_irqsave(&guc->context_lookup, flags); xa_for_each(&guc->context_lookup, index, ce) {

• if (!intel_context_is_pinned(ce))
  

• if (!kref_get_unless_zero(&ce->ref))
  continue;
  
  

• xa_unlock(&guc->context_lookup);
  

• if (!intel_context_is_pinned(ce))
  

• 	goto next;
  

• if (intel_engine_is_virtual(ce->engine)) { if (!(ce->engine->mask & engine->mask))

• 		continue;
  

• 		goto next;
  

  } else { if (ce->engine != engine)

• 		continue;
  

• 		goto next;
  

  }

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

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

  intel_engine_dump_active_requests(&ce->guc_active.requests, hung_rq, m);

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

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

+next:

• intel_context_put(ce);
  

• xa_lock(&guc->context_lookup);
  

  }

• xa_unlock_irqrestore(&guc->context_lookup, flags); }

  void intel_guc_submission_print_info(struct intel_guc *guc,

@@ -2962,7 +3032,9 @@ void intel_guc_submission_print_context_info(struct intel_guc *guc, { struct intel_context *ce; unsigned long index;

• unsigned long flags;

• xa_lock_irqsave(&guc->context_lookup, flags); xa_for_each(&guc->context_lookup, index, ce) { drm_printf(p, "GuC lrc descriptor %u:\n", ce->guc_id); drm_printf(p, "\tHW Context Desc: 0x%08x\n", ce->lrc.lrca);

@@ -2981,6 +3053,7 @@ void intel_guc_submission_print_context_info(struct intel_guc *guc,

guc_log_context_priority(p, ce);

}

• xa_unlock_irqrestore(&guc->context_lookup, flags); }

  static struct intel_context *

On 8/25/2021 5:41 PM, Matthew Brost wrote:

On Wed, Aug 25, 2021 at 05:44:11PM -0700, Daniele Ceraolo Spurio wrote:

...

On 8/18/2021 11:16 PM, Matthew Brost wrote:

...

Lock the xarray and take ref to the context if needed.

v2: (Checkpatch) - Add new line after declaration (Daniel Vetter) - Correct put / get accounting in xa_for_loops

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

.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 103 +++++++++++++++--- 1 file changed, 88 insertions(+), 15 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 509b298e7cf3..5f77f25322ca 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -606,8 +606,18 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) unsigned long index, flags; bool pending_disable, pending_enable, deregister, destroyed, banned;

• xa_lock_irqsave(&guc->context_lookup, flags); xa_for_each(&guc->context_lookup, index, ce) {

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

• /*
  

•  * Corner case where the ref count on the object is zero but and
  

•  * deregister G2H was lost. In this case we don't touch the ref
  

•  * count and finish the destroy of the context.
  

•  */
  

• bool do_put = kref_get_unless_zero(&ce->ref);
  

• xa_unlock(&guc->context_lookup);
  

• spin_lock(&ce->guc_state.lock);
  /*
   * Once we are at this point submission_disabled() is guaranteed
  

@@ -623,7 +633,9 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) banned = context_banned(ce); init_sched_state(ce);

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

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

• GEM_BUG_ON(!do_put && !destroyed);
  if (pending_enable || destroyed || deregister) {
  	decr_outstanding_submission_g2h(guc);
  

@@ -646,13 +658,19 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) } intel_context_sched_disable_unpin(ce); decr_outstanding_submission_g2h(guc);

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

• 	spin_lock(&ce->guc_state.lock);
  	guc_blocked_fence_complete(ce);
  

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

• 	spin_unlock(&ce->guc_state.lock);
  	intel_context_put(ce);
  }
  

• if (do_put)
  

• 	intel_context_put(ce);
  

is it safe to do the put outside the xa_lock, in case the refcount goes to zero with this? I know it is unlikely because the refcount was > 0 if do_put is true, but it might've gone down between us checking earlier and now.

It is safe as xa_for_each indicates it is safe to destroy / delete objects from the array while traversing it.

ok.

Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

Daniele

...

...

• xa_lock(&guc->context_lookup);
  

  }
• xa_unlock_irqrestore(&guc->context_lookup, flags); } static inline bool

@@ -873,16 +891,29 @@ void intel_guc_submission_reset(struct intel_guc *guc, bool stalled) { struct intel_context *ce; unsigned long index;

• unsigned long flags; if (unlikely(!guc_submission_initialized(guc))) { /* Reset called during driver load? GuC not yet initialised! */ return; }

• xa_for_each(&guc->context_lookup, index, ce)

• xa_lock_irqsave(&guc->context_lookup, flags);
• xa_for_each(&guc->context_lookup, index, ce) {

• if (!kref_get_unless_zero(&ce->ref))
  

• 	continue;
  

• xa_unlock(&guc->context_lookup);
  

• if (intel_context_is_pinned(ce))
  	__guc_reset_context(ce, stalled);
  

• intel_context_put(ce);
  

• xa_lock(&guc->context_lookup);
  

• }
• xa_unlock_irqrestore(&guc->context_lookup, flags);
• /* GuC is blown away, drop all references to contexts */ xa_destroy(&guc->context_lookup); }

@@ -957,11 +988,24 @@ void intel_guc_submission_cancel_requests(struct intel_guc *guc) { struct intel_context *ce; unsigned long index;

• unsigned long flags;
• xa_lock_irqsave(&guc->context_lookup, flags);
• xa_for_each(&guc->context_lookup, index, ce) {

• if (!kref_get_unless_zero(&ce->ref))
  

• 	continue;
  

• xa_unlock(&guc->context_lookup);
  

• xa_for_each(&guc->context_lookup, index, ce) if (intel_context_is_pinned(ce)) guc_cancel_context_requests(ce);

• intel_context_put(ce);
  

• xa_lock(&guc->context_lookup);
  

• }
• xa_unlock_irqrestore(&guc->context_lookup, flags);
• guc_cancel_sched_engine_requests(guc->sched_engine); /* GuC is blown away, drop all references to contexts */

@@ -2850,21 +2894,28 @@ void intel_guc_find_hung_context(struct intel_engine_cs *engine) struct intel_context *ce; struct i915_request *rq; unsigned long index;

• unsigned long flags; /* Reset called during driver load? GuC not yet initialised! */ if (unlikely(!guc_submission_initialized(guc))) return;
• xa_lock_irqsave(&guc->context_lookup, flags); xa_for_each(&guc->context_lookup, index, ce) {

• if (!intel_context_is_pinned(ce))
  

• if (!kref_get_unless_zero(&ce->ref))
  	continue;
  

• xa_unlock(&guc->context_lookup);
  

• if (!intel_context_is_pinned(ce))
  

• 	goto next;
  

• if (intel_engine_is_virtual(ce->engine)) {
  	if (!(ce->engine->mask & engine->mask))
  

• 		continue;
  

• 		goto next;
  } else {
  	if (ce->engine != engine)
  

• 		continue;
  

• 		goto next;
  }
  list_for_each_entry(rq, &ce->guc_active.requests, sched.link) {
  

@@ -2874,9 +2925,17 @@ void intel_guc_find_hung_context(struct intel_engine_cs *engine) intel_engine_set_hung_context(engine, ce); /* Can only cope with one hang at a time... */

• 	return;
  

• 	intel_context_put(ce);
  

• 	xa_lock(&guc->context_lookup);
  

• 	goto done;
  }
  

+next:

• intel_context_put(ce);
  

• xa_lock(&guc->context_lookup);
  

nit: extra newline

Checkpatch got that one too. Already fixed.

Matt

...

Daniele

...

}

+done:

• xa_unlock_irqrestore(&guc->context_lookup, flags); } void intel_guc_dump_active_requests(struct intel_engine_cs *engine,

@@ -2892,23 +2951,34 @@ void intel_guc_dump_active_requests(struct intel_engine_cs *engine, if (unlikely(!guc_submission_initialized(guc))) return;

• xa_lock_irqsave(&guc->context_lookup, flags); xa_for_each(&guc->context_lookup, index, ce) {

• if (!intel_context_is_pinned(ce))
  

• if (!kref_get_unless_zero(&ce->ref))
  	continue;
  

• xa_unlock(&guc->context_lookup);
  

• if (!intel_context_is_pinned(ce))
  

• 	goto next;
  

• if (intel_engine_is_virtual(ce->engine)) {
  	if (!(ce->engine->mask & engine->mask))
  

• 		continue;
  

• 		goto next;
  } else {
  	if (ce->engine != engine)
  

• 		continue;
  

• 		goto next;
  }
  

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

• spin_lock(&ce->guc_active.lock);
  intel_engine_dump_active_requests(&ce->guc_active.requests,
  				  hung_rq, m);
  

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

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

+next:

• intel_context_put(ce);
  

• xa_lock(&guc->context_lookup);
  

  }
• xa_unlock_irqrestore(&guc->context_lookup, flags); } void intel_guc_submission_print_info(struct intel_guc *guc,

@@ -2962,7 +3032,9 @@ void intel_guc_submission_print_context_info(struct intel_guc *guc, { struct intel_context *ce; unsigned long index;

• unsigned long flags;
• xa_lock_irqsave(&guc->context_lookup, flags); xa_for_each(&guc->context_lookup, index, ce) { drm_printf(p, "GuC lrc descriptor %u:\n", ce->guc_id); drm_printf(p, "\tHW Context Desc: 0x%08x\n", ce->lrc.lrca);

@@ -2981,6 +3053,7 @@ void intel_guc_submission_print_context_info(struct intel_guc *guc, guc_log_context_priority(p, ce); }

• xa_unlock_irqrestore(&guc->context_lookup, flags); } static struct intel_context *

On 8/18/2021 11:16 PM, Matthew Brost wrote:

Drop pin count check trick between a sched_disable and re-pin, now rely on the lock and counter of the number of committed requests to determine if scheduling should be disabled on the context.

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

Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

Daniele

---

drivers/gpu/drm/i915/gt/intel_context_types.h | 2 + .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 49 ++++++++++++------- 2 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index d5d643b04d54..524a35a78bf4 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -169,6 +169,8 @@ struct intel_context { struct list_head fences; /* GuC context blocked fence */ struct i915_sw_fence blocked_fence;

• /* GuC committed requests */
  

• int number_committed_requests;
  

  } guc_state;

  struct {

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 5f77f25322ca..3e90985b0c1b 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -248,6 +248,25 @@ static inline void decr_context_blocked(struct intel_context *ce) ce->guc_state.sched_state -= SCHED_STATE_BLOCKED; }

+static inline bool context_has_committed_requests(struct intel_context *ce) +{

• return !!ce->guc_state.number_committed_requests;

+}

+static inline void incr_context_committed_requests(struct intel_context *ce) +{

• lockdep_assert_held(&ce->guc_state.lock);
• ++ce->guc_state.number_committed_requests;
• GEM_BUG_ON(ce->guc_state.number_committed_requests < 0);

+}

+static inline void decr_context_committed_requests(struct intel_context *ce) +{

• lockdep_assert_held(&ce->guc_state.lock);
• --ce->guc_state.number_committed_requests;
• GEM_BUG_ON(ce->guc_state.number_committed_requests < 0);

+}

• static inline bool context_guc_id_invalid(struct intel_context *ce) { return ce->guc_id == GUC_INVALID_LRC_ID;

@@ -1751,14 +1770,11 @@ static void guc_context_sched_disable(struct intel_context *ce) spin_lock_irqsave(&ce->guc_state.lock, flags);

/*

• * We have to check if the context has been disabled by another thread.
  

• * We also have to check if the context has been pinned again as another
  

• * pin operation is allowed to pass this function. Checking the pin
  

• * count, within ce->guc_state.lock, synchronizes this function with
  

• * guc_request_alloc ensuring a request doesn't slip through the
  

• * 'context_pending_disable' fence. Checking within the spin lock (can't
  

• * sleep) ensures another process doesn't pin this context and generate
  

• * a request before we set the 'context_pending_disable' flag here.
  

• * We have to check if the context has been disabled by another thread,
  

• * check if submssion has been disabled to seal a race with reset and
  

• * finally check if any more requests have been committed to the
  

• * context ensursing that a request doesn't slip through the
  

• * 'context_pending_disable' fence.
  

  */ enabled = context_enabled(ce); if (unlikely(!enabled || submission_disabled(guc))) {

@@ -1767,7 +1783,8 @@ static void guc_context_sched_disable(struct intel_context *ce) spin_unlock_irqrestore(&ce->guc_state.lock, flags); goto unpin; }

• if (unlikely(atomic_add_unless(&ce->pin_count, -2, 2))) {

• if (unlikely(context_has_committed_requests(ce))) {

• intel_context_sched_disable_unpin(ce);
  

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

@@ -1800,6 +1817,7 @@ static void __guc_context_destroy(struct intel_context *ce) ce->guc_prio_count[GUC_CLIENT_PRIORITY_HIGH] || ce->guc_prio_count[GUC_CLIENT_PRIORITY_KMD_NORMAL] || ce->guc_prio_count[GUC_CLIENT_PRIORITY_NORMAL]);

• GEM_BUG_ON(ce->guc_state.number_committed_requests);

  lrc_fini(ce); intel_context_fini(ce);

@@ -2030,6 +2048,10 @@ static void remove_from_context(struct i915_request *rq)

spin_unlock_irq(&ce->guc_active.lock);

• spin_lock_irq(&ce->guc_state.lock);
• decr_context_committed_requests(ce);
• spin_unlock_irq(&ce->guc_state.lock);
• atomic_dec(&ce->guc_id_ref); i915_request_notify_execute_cb_imm(rq); }

@@ -2177,15 +2199,7 @@ static int guc_request_alloc(struct i915_request *rq) * schedule enable or context registration if either G2H is pending * respectfully. Once a G2H returns, the fence is released that is * blocking these requests (see guc_signal_context_fence).

• *
  

• * We can safely check the below fields outside of the lock as it isn't
  

• * possible for these fields to transition from being clear to set but
  

• * converse is possible, hence the need for the check within the lock.
  

  */
• if (likely(!context_wait_for_deregister_to_register(ce) &&

•    !context_pending_disable(ce)))
  

• return 0;
  

• spin_lock_irqsave(&ce->guc_state.lock, flags); if (context_wait_for_deregister_to_register(ce) || context_pending_disable(ce)) {

@@ -2194,6 +2208,7 @@ static int guc_request_alloc(struct i915_request *rq)

list_add_tail(&rq->guc_fence_link, &ce->guc_state.fences);

}

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

  return 0;

On 8/18/2021 11:16 PM, Matthew Brost wrote:

Move GuC management fields in context under guc_active struct as this is where the lock that protects theses fields lives. Also only set guc_prio field once during context init.

Can you explain what we gain by setting that only on first pin? AFAICS re-setting it doesn't hurt and we would cover the case where a context priority gets updated while the context is idle. I know the request submission would eventually update the prio so there is no bug, but that then requires an extra H2G.

Fixes: ee242ca704d3 ("drm/i915/guc: Implement GuC priority management") Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: stable@vger.kernel.org

---

drivers/gpu/drm/i915/gt/intel_context_types.h | 12 ++-- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 68 +++++++++++-------- drivers/gpu/drm/i915/i915_trace.h | 2 +- 3 files changed, 45 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index 524a35a78bf4..9fb0480ccf3b 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -112,6 +112,7 @@ struct intel_context { #define CONTEXT_FORCE_SINGLE_SUBMISSION 7 #define CONTEXT_NOPREEMPT 8 #define CONTEXT_LRCA_DIRTY 9 +#define CONTEXT_GUC_INIT 10

struct { u64 timeout_us; @@ -178,6 +179,11 @@ struct intel_context { spinlock_t lock; /** requests: active requests on this context */ struct list_head requests;

• /*
  

•  * GuC priority management
  

•  */
  

• u8 prio;
  

• u32 prio_count[GUC_CLIENT_PRIORITY_NUM];
  

  } guc_active;

  /* GuC LRC descriptor ID */

@@ -191,12 +197,6 @@ struct intel_context { */ struct list_head guc_id_link;

• /*

• * GuC priority management
  

• */
  

• u8 guc_prio;
• u32 guc_prio_count[GUC_CLIENT_PRIORITY_NUM];
• #ifdef CONFIG_DRM_I915_SELFTEST /**
  • @drop_schedule_enable: Force drop of schedule enable G2H for selftest

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 3e90985b0c1b..bb90bedb1305 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1369,8 +1369,6 @@ static void guc_context_policy_init(struct intel_engine_cs *engine, desc->preemption_timeout = engine->props.preempt_timeout_ms * 1000; }

-static inline u8 map_i915_prio_to_guc_prio(int prio);

• static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) { struct intel_engine_cs *engine = ce->engine;

@@ -1378,8 +1376,6 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) struct intel_guc *guc = &engine->gt->uc.guc; u32 desc_idx = ce->guc_id; struct guc_lrc_desc *desc;

• const struct i915_gem_context *ctx;
• int prio = I915_CONTEXT_DEFAULT_PRIORITY; bool context_registered; intel_wakeref_t wakeref; int ret = 0;

@@ -1396,12 +1392,6 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)

context_registered = lrc_desc_registered(guc, desc_idx);

• rcu_read_lock();
• ctx = rcu_dereference(ce->gem_context);
• if (ctx)

• prio = ctx->sched.priority;
  

• rcu_read_unlock();
• reset_lrc_desc(guc, desc_idx); set_lrc_desc_registered(guc, desc_idx, ce);

@@ -1410,8 +1400,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) desc->engine_submit_mask = adjust_engine_mask(engine->class, engine->mask); desc->hw_context_desc = ce->lrc.lrca;

• ce->guc_prio = map_i915_prio_to_guc_prio(prio);
• desc->priority = ce->guc_prio;

• desc->priority = ce->guc_active.prio; desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD; guc_context_policy_init(engine, desc);

@@ -1813,10 +1802,10 @@ static inline void guc_lrc_desc_unpin(struct intel_context *ce)

static void __guc_context_destroy(struct intel_context *ce) {

• GEM_BUG_ON(ce->guc_prio_count[GUC_CLIENT_PRIORITY_KMD_HIGH] ||

•    ce->guc_prio_count[GUC_CLIENT_PRIORITY_HIGH] ||
  

•    ce->guc_prio_count[GUC_CLIENT_PRIORITY_KMD_NORMAL] ||
  

•    ce->guc_prio_count[GUC_CLIENT_PRIORITY_NORMAL]);
  

• GEM_BUG_ON(ce->guc_active.prio_count[GUC_CLIENT_PRIORITY_KMD_HIGH] ||

•    ce->guc_active.prio_count[GUC_CLIENT_PRIORITY_HIGH] ||
  

•    ce->guc_active.prio_count[GUC_CLIENT_PRIORITY_KMD_NORMAL] ||
  

•    ce->guc_active.prio_count[GUC_CLIENT_PRIORITY_NORMAL]);
  

  GEM_BUG_ON(ce->guc_state.number_committed_requests);

  lrc_fini(ce);

@@ -1926,14 +1915,17 @@ static void guc_context_set_prio(struct intel_guc *guc,

GEM_BUG_ON(prio < GUC_CLIENT_PRIORITY_KMD_HIGH || prio > GUC_CLIENT_PRIORITY_NORMAL);

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

• if (ce->guc_prio == prio || submission_disabled(guc) ||

•    !context_registered(ce))
  

• if (ce->guc_active.prio == prio || submission_disabled(guc) ||

•    !context_registered(ce)) {
  

• ce->guc_active.prio = prio;
  

  return;

• }

  guc_submission_send_busy_loop(guc, action, ARRAY_SIZE(action), 0, true);

• ce->guc_prio = prio;

• ce->guc_active.prio = prio; trace_intel_context_set_prio(ce); }

@@ -1953,24 +1945,24 @@ static inline void add_context_inflight_prio(struct intel_context *ce, u8 guc_prio) { lockdep_assert_held(&ce->guc_active.lock);

• GEM_BUG_ON(guc_prio >= ARRAY_SIZE(ce->guc_prio_count));

• GEM_BUG_ON(guc_prio >= ARRAY_SIZE(ce->guc_active.prio_count));

• ++ce->guc_prio_count[guc_prio];

• ++ce->guc_active.prio_count[guc_prio];

  /* Overflow protection */

• GEM_WARN_ON(!ce->guc_prio_count[guc_prio]);

• GEM_WARN_ON(!ce->guc_active.prio_count[guc_prio]); }

  static inline void sub_context_inflight_prio(struct intel_context *ce, u8 guc_prio) { lockdep_assert_held(&ce->guc_active.lock);

• GEM_BUG_ON(guc_prio >= ARRAY_SIZE(ce->guc_prio_count));

• GEM_BUG_ON(guc_prio >= ARRAY_SIZE(ce->guc_active.prio_count));

  /* Underflow protection */

• GEM_WARN_ON(!ce->guc_prio_count[guc_prio]);

• GEM_WARN_ON(!ce->guc_active.prio_count[guc_prio]);

• --ce->guc_prio_count[guc_prio];

• --ce->guc_active.prio_count[guc_prio]; }

  static inline void update_context_prio(struct intel_context *ce)

@@ -1983,8 +1975,8 @@ static inline void update_context_prio(struct intel_context *ce)

lockdep_assert_held(&ce->guc_active.lock);

• for (i = 0; i < ARRAY_SIZE(ce->guc_prio_count); ++i) {

• if (ce->guc_prio_count[i]) {
  

• for (i = 0; i < ARRAY_SIZE(ce->guc_active.prio_count); ++i) {

• if (ce->guc_active.prio_count[i]) {
  guc_context_set_prio(guc, ce, i);
  break;
  

  }

@@ -2123,6 +2115,20 @@ static bool context_needs_register(struct intel_context *ce, bool new_guc_id) !submission_disabled(ce_to_guc(ce)); }

+static void guc_context_init(struct intel_context *ce) +{

• const struct i915_gem_context *ctx;
• int prio = I915_CONTEXT_DEFAULT_PRIORITY;
• rcu_read_lock();
• ctx = rcu_dereference(ce->gem_context);
• if (ctx)

• prio = ctx->sched.priority;
  

• rcu_read_unlock();
• ce->guc_active.prio = map_i915_prio_to_guc_prio(prio);

+}

• static int guc_request_alloc(struct i915_request *rq) { struct intel_context *ce = rq->context;

@@ -2154,6 +2160,9 @@ static int guc_request_alloc(struct i915_request *rq)

rq->reserved_space -= GUC_REQUEST_SIZE;

• if (unlikely(!test_bit(CONTEXT_GUC_INIT, &ce->flags)))

Where is CONTEXT_GUC_INIT set? Can't find it

Daniele

• guc_context_init(ce);
  

• /*
  • Call pin_guc_id here rather than in the pinning step as with
  • dma_resv, contexts can be repeatedly pinned / unpinned trashing the

@@ -3031,13 +3040,12 @@ static inline void guc_log_context_priority(struct drm_printer *p, { int i;

• drm_printf(p, "\t\tPriority: %d\n",

•    ce->guc_prio);
  

• drm_printf(p, "\t\tPriority: %d\n", ce->guc_active.prio); drm_printf(p, "\t\tNumber Requests (lower index == higher priority)\n"); for (i = GUC_CLIENT_PRIORITY_KMD_HIGH; i < GUC_CLIENT_PRIORITY_NUM; ++i) { drm_printf(p, "\t\tNumber requests in priority band[%d]: %d\n",

• 	   i, ce->guc_prio_count[i]);
  

• 	   i, ce->guc_active.prio_count[i]);
  

  } drm_printf(p, "\n"); }

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index 0a77eb2944b5..6f882e72ed11 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -910,7 +910,7 @@ DECLARE_EVENT_CLASS(intel_context, __entry->guc_id = ce->guc_id; __entry->pin_count = atomic_read(&ce->pin_count); __entry->sched_state = ce->guc_state.sched_state;

• 	   __entry->guc_prio = ce->guc_prio;
  

• 	   __entry->guc_prio = ce->guc_active.prio;
     ),
  
  

  TP_printk("guc_id=%d, pin_count=%d sched_state=0x%x, guc_prio=%u",

On Wed, Aug 25, 2021 at 02:51:11PM -0700, Daniele Ceraolo Spurio wrote:

On 8/18/2021 11:16 PM, Matthew Brost wrote:

...

Move GuC management fields in context under guc_active struct as this is where the lock that protects theses fields lives. Also only set guc_prio field once during context init.

Can you explain what we gain by setting that only on first pin? AFAICS re-setting it doesn't hurt and we would cover the case where a context priority gets updated while the context is idle. I know the request submission would eventually update the prio so there is no bug, but that then requires an extra H2G.

...

Fixes: ee242ca704d3 ("drm/i915/guc: Implement GuC priority management") Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: stable@vger.kernel.org

---

drivers/gpu/drm/i915/gt/intel_context_types.h | 12 ++-- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 68 +++++++++++-------- drivers/gpu/drm/i915/i915_trace.h | 2 +- 3 files changed, 45 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index 524a35a78bf4..9fb0480ccf3b 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -112,6 +112,7 @@ struct intel_context { #define CONTEXT_FORCE_SINGLE_SUBMISSION 7 #define CONTEXT_NOPREEMPT 8 #define CONTEXT_LRCA_DIRTY 9 +#define CONTEXT_GUC_INIT 10 struct { u64 timeout_us; @@ -178,6 +179,11 @@ struct intel_context { spinlock_t lock; /** requests: active requests on this context */ struct list_head requests;

• /*
  

•  * GuC priority management
  

•  */
  

• u8 prio;
  

• u32 prio_count[GUC_CLIENT_PRIORITY_NUM];
  

  } guc_active; /* GuC LRC descriptor ID */

@@ -191,12 +197,6 @@ struct intel_context { */ struct list_head guc_id_link;

• /*

• * GuC priority management
  

• */
  

• u8 guc_prio;
• u32 guc_prio_count[GUC_CLIENT_PRIORITY_NUM];
• #ifdef CONFIG_DRM_I915_SELFTEST /**
  • @drop_schedule_enable: Force drop of schedule enable G2H for selftest

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 3e90985b0c1b..bb90bedb1305 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1369,8 +1369,6 @@ static void guc_context_policy_init(struct intel_engine_cs *engine, desc->preemption_timeout = engine->props.preempt_timeout_ms * 1000; } -static inline u8 map_i915_prio_to_guc_prio(int prio);

• static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) { struct intel_engine_cs *engine = ce->engine;

@@ -1378,8 +1376,6 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) struct intel_guc *guc = &engine->gt->uc.guc; u32 desc_idx = ce->guc_id; struct guc_lrc_desc *desc;

• const struct i915_gem_context *ctx;
• int prio = I915_CONTEXT_DEFAULT_PRIORITY; bool context_registered; intel_wakeref_t wakeref; int ret = 0;

@@ -1396,12 +1392,6 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) context_registered = lrc_desc_registered(guc, desc_idx);

• rcu_read_lock();
• ctx = rcu_dereference(ce->gem_context);
• if (ctx)

• prio = ctx->sched.priority;
  

• rcu_read_unlock();
• reset_lrc_desc(guc, desc_idx); set_lrc_desc_registered(guc, desc_idx, ce);

@@ -1410,8 +1400,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) desc->engine_submit_mask = adjust_engine_mask(engine->class, engine->mask); desc->hw_context_desc = ce->lrc.lrca;

• ce->guc_prio = map_i915_prio_to_guc_prio(prio);
• desc->priority = ce->guc_prio;

• desc->priority = ce->guc_active.prio; desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD; guc_context_policy_init(engine, desc);

@@ -1813,10 +1802,10 @@ static inline void guc_lrc_desc_unpin(struct intel_context *ce) static void __guc_context_destroy(struct intel_context *ce) {

• GEM_BUG_ON(ce->guc_prio_count[GUC_CLIENT_PRIORITY_KMD_HIGH] ||

•    ce->guc_prio_count[GUC_CLIENT_PRIORITY_HIGH] ||
  

•    ce->guc_prio_count[GUC_CLIENT_PRIORITY_KMD_NORMAL] ||
  

•    ce->guc_prio_count[GUC_CLIENT_PRIORITY_NORMAL]);
  

• GEM_BUG_ON(ce->guc_active.prio_count[GUC_CLIENT_PRIORITY_KMD_HIGH] ||

•    ce->guc_active.prio_count[GUC_CLIENT_PRIORITY_HIGH] ||
  

•    ce->guc_active.prio_count[GUC_CLIENT_PRIORITY_KMD_NORMAL] ||
  

•    ce->guc_active.prio_count[GUC_CLIENT_PRIORITY_NORMAL]);
  

  GEM_BUG_ON(ce->guc_state.number_committed_requests); lrc_fini(ce);

@@ -1926,14 +1915,17 @@ static void guc_context_set_prio(struct intel_guc *guc, GEM_BUG_ON(prio < GUC_CLIENT_PRIORITY_KMD_HIGH || prio > GUC_CLIENT_PRIORITY_NORMAL);

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

• if (ce->guc_prio == prio || submission_disabled(guc) ||

•    !context_registered(ce))
  

• if (ce->guc_active.prio == prio || submission_disabled(guc) ||

•    !context_registered(ce)) {
  

• ce->guc_active.prio = prio;
  

  return;
• } guc_submission_send_busy_loop(guc, action, ARRAY_SIZE(action), 0, true);

• ce->guc_prio = prio;

• ce->guc_active.prio = prio; trace_intel_context_set_prio(ce); }

@@ -1953,24 +1945,24 @@ static inline void add_context_inflight_prio(struct intel_context *ce, u8 guc_prio) { lockdep_assert_held(&ce->guc_active.lock);

• GEM_BUG_ON(guc_prio >= ARRAY_SIZE(ce->guc_prio_count));

• GEM_BUG_ON(guc_prio >= ARRAY_SIZE(ce->guc_active.prio_count));

• ++ce->guc_prio_count[guc_prio];

• ++ce->guc_active.prio_count[guc_prio]; /* Overflow protection */

• GEM_WARN_ON(!ce->guc_prio_count[guc_prio]);

• GEM_WARN_ON(!ce->guc_active.prio_count[guc_prio]); } static inline void sub_context_inflight_prio(struct intel_context *ce, u8 guc_prio) { lockdep_assert_held(&ce->guc_active.lock);

• GEM_BUG_ON(guc_prio >= ARRAY_SIZE(ce->guc_prio_count));

• GEM_BUG_ON(guc_prio >= ARRAY_SIZE(ce->guc_active.prio_count)); /* Underflow protection */

• GEM_WARN_ON(!ce->guc_prio_count[guc_prio]);

• GEM_WARN_ON(!ce->guc_active.prio_count[guc_prio]);

• --ce->guc_prio_count[guc_prio];

• --ce->guc_active.prio_count[guc_prio]; } static inline void update_context_prio(struct intel_context *ce)

@@ -1983,8 +1975,8 @@ static inline void update_context_prio(struct intel_context *ce) lockdep_assert_held(&ce->guc_active.lock);

• for (i = 0; i < ARRAY_SIZE(ce->guc_prio_count); ++i) {

• if (ce->guc_prio_count[i]) {
  

• for (i = 0; i < ARRAY_SIZE(ce->guc_active.prio_count); ++i) {

• if (ce->guc_active.prio_count[i]) {
  guc_context_set_prio(guc, ce, i);
  break;
  

  }

@@ -2123,6 +2115,20 @@ static bool context_needs_register(struct intel_context *ce, bool new_guc_id) !submission_disabled(ce_to_guc(ce)); } +static void guc_context_init(struct intel_context *ce) +{

• const struct i915_gem_context *ctx;
• int prio = I915_CONTEXT_DEFAULT_PRIORITY;
• rcu_read_lock();
• ctx = rcu_dereference(ce->gem_context);
• if (ctx)

• prio = ctx->sched.priority;
  

• rcu_read_unlock();
• ce->guc_active.prio = map_i915_prio_to_guc_prio(prio);

+}

• static int guc_request_alloc(struct i915_request *rq) { struct intel_context *ce = rq->context;

@@ -2154,6 +2160,9 @@ static int guc_request_alloc(struct i915_request *rq) rq->reserved_space -= GUC_REQUEST_SIZE;

• if (unlikely(!test_bit(CONTEXT_GUC_INIT, &ce->flags)))

Where is CONTEXT_GUC_INIT set? Can't find it

Missed this commit. Opps, this should be set in guc_context_init.

Matt

Daniele

...

• guc_context_init(ce);
  

• /*
  • Call pin_guc_id here rather than in the pinning step as with
  • dma_resv, contexts can be repeatedly pinned / unpinned trashing the

@@ -3031,13 +3040,12 @@ static inline void guc_log_context_priority(struct drm_printer *p, { int i;

• drm_printf(p, "\t\tPriority: %d\n",

•    ce->guc_prio);
  

• drm_printf(p, "\t\tPriority: %d\n", ce->guc_active.prio); drm_printf(p, "\t\tNumber Requests (lower index == higher priority)\n"); for (i = GUC_CLIENT_PRIORITY_KMD_HIGH; i < GUC_CLIENT_PRIORITY_NUM; ++i) { drm_printf(p, "\t\tNumber requests in priority band[%d]: %d\n",

• 	   i, ce->guc_prio_count[i]);
  

• 	   i, ce->guc_active.prio_count[i]);
  

  } drm_printf(p, "\n"); }

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index 0a77eb2944b5..6f882e72ed11 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -910,7 +910,7 @@ DECLARE_EVENT_CLASS(intel_context, __entry->guc_id = ce->guc_id; __entry->pin_count = atomic_read(&ce->pin_count); __entry->sched_state = ce->guc_state.sched_state;

• 	   __entry->guc_prio = ce->guc_prio;
  

• 	   __entry->guc_prio = ce->guc_active.prio;
     ),
  

  TP_printk("guc_id=%d, pin_count=%d sched_state=0x%x, guc_prio=%u",