Re: [Intel-gfx] [bug report] drm/i915/guc: Implement multi-lrc submission

[John Harrison <john.c.harrison@intel.com>]

On 9/22/2022 07:26, Dan Carpenter wrote:
> Hello Matthew Brost,
>
> The patch 6b540bf6f143: "drm/i915/guc: Implement multi-lrc
> submission" from Oct 14, 2021, leads to the following Smatch static
> checker warning:
>
> 	drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:752 __guc_add_request()
> 	warn: refcount leak 'ce->ref.refcount.refs.counter': lines='752'
>
> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>      672 static int __guc_add_request(struct intel_guc *guc, struct i915_request *rq)
>      673 {
>      674         int err = 0;
>      675         struct intel_context *ce = request_to_scheduling_context(rq);
>      676         u32 action[3];
>      677         int len = 0;
>      678         u32 g2h_len_dw = 0;
>      679         bool enabled;
>      680
>      681         lockdep_assert_held(&rq->engine->sched_engine->lock);
>      682
>      683         /*
>      684          * Corner case where requests were sitting in the priority list or a
>      685          * request resubmitted after the context was banned.
>      686          */
>      687         if (unlikely(intel_context_is_banned(ce))) {
>      688                 i915_request_put(i915_request_mark_eio(rq));
>      689                 intel_engine_signal_breadcrumbs(ce->engine);
>      690                 return 0;
>      691         }
>      692
>      693         GEM_BUG_ON(!atomic_read(&ce->guc_id.ref));
>      694         GEM_BUG_ON(context_guc_id_invalid(ce));
>      695
>      696         if (context_policy_required(ce)) {
>      697                 err = guc_context_policy_init_v70(ce, false);
>      698                 if (err)
>      699                         return err;
>      700         }
>      701
>      702         spin_lock(&ce->guc_state.lock);
>      703
>      704         /*
>      705          * The request / context will be run on the hardware when scheduling
>      706          * gets enabled in the unblock. For multi-lrc we still submit the
>      707          * context to move the LRC tails.
>      708          */
>      709         if (unlikely(context_blocked(ce) && !intel_context_is_parent(ce)))
>      710                 goto out;
>      711
>      712         enabled = context_enabled(ce) || context_blocked(ce);
>      713
>      714         if (!enabled) {
>      715                 action[len++] = INTEL_GUC_ACTION_SCHED_CONTEXT_MODE_SET;
>      716                 action[len++] = ce->guc_id.id;
>      717                 action[len++] = GUC_CONTEXT_ENABLE;
>      718                 set_context_pending_enable(ce);
>      719                 intel_context_get(ce);
>      720                 g2h_len_dw = G2H_LEN_DW_SCHED_CONTEXT_MODE_SET;
>      721         } else {
>      722                 action[len++] = INTEL_GUC_ACTION_SCHED_CONTEXT;
>      723                 action[len++] = ce->guc_id.id;
>      724         }
>      725
>      726         err = intel_guc_send_nb(guc, action, len, g2h_len_dw);
>      727         if (!enabled && !err) {
>      728                 trace_intel_context_sched_enable(ce);
>      729                 atomic_inc(&guc->outstanding_submission_g2h);
>      730                 set_context_enabled(ce);
>      731
>      732                 /*
>      733                  * Without multi-lrc KMD does the submission step (moving the
>      734                  * lrc tail) so enabling scheduling is sufficient to submit the
>      735                  * context. This isn't the case in multi-lrc submission as the
>      736                  * GuC needs to move the tails, hence the need for another H2G
>      737                  * to submit a multi-lrc context after enabling scheduling.
>      738                  */
>      739                 if (intel_context_is_parent(ce)) {
>      740                         action[0] = INTEL_GUC_ACTION_SCHED_CONTEXT;
>      741                         err = intel_guc_send_nb(guc, action, len - 1, 0);
>
> Should this have a something like:
>
> 				if (err)
> 					intel_context_put(ce);
No.

The ce has been marked as enabled above because the actual enable call 
succeeded.  This is a secondary call to 'poke' the context. If this 
fails, the context might not get scheduled in a timely manner but the 
tracking state is still correct. The context is now in use by GuC and 
therefore must be reference locked. And the 'context_enabled' flag is 
set on the i915 side to note that the reference count is held.

If you want to unwind at this point, you would need to send a 
CONTEXT_MODE_SET(DISABLE) H2G message (amongst other things) and only 
once that call has completed, can you call intel_context_put(ce).

I don't get why the analyser would be claiming a leak here. I'm not sure 
if it is just that the analyser is confused or if there is some other 
potential route to a leak. Is it possible to get more details as to how 
it thinks the leak can occur?

John.

>
>      742                 }
>      743         } else if (!enabled) {
>      744                 clr_context_pending_enable(ce);
>      745                 intel_context_put(ce);
>      746         }
>      747         if (likely(!err))
>      748                 trace_i915_request_guc_submit(rq);
>      749
>      750 out:
>      751         spin_unlock(&ce->guc_state.lock);
> --> 752         return err;
>      753 }
>
> regards,
> dan carpenter