Re: [Intel-gfx] [PATCH 22/27] drm/i915/guc: Drop pin count check trick between sched_disable and re-pin

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

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;