Re: [PATCH 2/4] drm/i915/guc: Cancel requests immediately

From: John Harrison <john.c.harrison@intel.com>
To: Matthew Brost <matthew.brost@intel.com>,
	<intel-gfx@lists.freedesktop.org>,
	<dri-devel@lists.freedesktop.org>
Cc: daniele.ceraolospurio@intel.com, tvrtko.ursulin@intel.com
Subject: Re: [PATCH 2/4] drm/i915/guc: Cancel requests immediately
Date: Wed, 26 Jan 2022 10:58:46 -0800	[thread overview]
Message-ID: <5c436bf7-0ed6-83b6-6061-fdc73d4cc081@intel.com> (raw)
In-Reply-To: <20220124150157.15758-3-matthew.brost@intel.com>

On 1/24/2022 07:01, Matthew Brost wrote:
> Change the preemption timeout to the smallest possible value (1 us) when
> disabling scheduling to cancel a request and restore it after
> cancellation. This not only cancels the request as fast as possible, it
> fixes a bug where the preemption timeout is 0 which results in the
> schedule disable hanging forever.
Shouldn't there be an 'if' in the above statement? The pre-emption 
timeout is not normally zero.

>
> Reported-by: Jani Saarinen <jani.saarinen@intel.com>
> Fixes: 62eaf0ae217d4 ("drm/i915/guc: Support request cancellation")
> Link: https://gitlab.freedesktop.org/drm/intel/-/issues/4960
> 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 | 46 +++++++++++--------
>   2 files changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> index 30cd81ad8911a..730998823dbea 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> @@ -198,6 +198,11 @@ struct intel_context {
>   		 * each priority bucket
>   		 */
>   		u32 prio_count[GUC_CLIENT_PRIORITY_NUM];
> +		/**
> +		 * @preemption_timeout: preemption timeout of the context, used
> +		 * to restore this value after request cancellation
> +		 */
> +		u32 preemption_timeout;
>   	} 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 3918f1be114fa..966947c450253 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -2147,7 +2147,8 @@ static inline u32 get_children_join_value(struct intel_context *ce,
>   	return __get_parent_scratch(ce)->join[child_index].semaphore;
>   }
>   
> -static void guc_context_policy_init(struct intel_engine_cs *engine,
> +static void guc_context_policy_init(struct intel_context *ce,
> +				    struct intel_engine_cs *engine,
>   				    struct guc_lrc_desc *desc)
Shouldn't engine be before ce? The more general structure usually goes 
first.

John.

>   {
>   	desc->policy_flags = 0;
> @@ -2157,7 +2158,8 @@ static void guc_context_policy_init(struct intel_engine_cs *engine,
>   
>   	/* NB: For both of these, zero means disabled. */
>   	desc->execution_quantum = engine->props.timeslice_duration_ms * 1000;
> -	desc->preemption_timeout = engine->props.preempt_timeout_ms * 1000;
> +	ce->guc_state.preemption_timeout = engine->props.preempt_timeout_ms * 1000;
> +	desc->preemption_timeout = ce->guc_state.preemption_timeout;
>   }
>   
>   static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
> @@ -2193,7 +2195,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
>   	desc->hw_context_desc = ce->lrc.lrca;
>   	desc->priority = ce->guc_state.prio;
>   	desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
> -	guc_context_policy_init(engine, desc);
> +	guc_context_policy_init(ce, engine, desc);
>   
>   	/*
>   	 * If context is a parent, we need to register a process descriptor
> @@ -2226,7 +2228,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
>   			desc->hw_context_desc = child->lrc.lrca;
>   			desc->priority = ce->guc_state.prio;
>   			desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
> -			guc_context_policy_init(engine, desc);
> +			guc_context_policy_init(child, engine, desc);
>   		}
>   
>   		clear_children_join_go_memory(ce);
> @@ -2409,6 +2411,19 @@ static u16 prep_context_pending_disable(struct intel_context *ce)
>   	return ce->guc_id.id;
>   }
>   
> +static void __guc_context_set_preemption_timeout(struct intel_guc *guc,
> +						 u16 guc_id,
> +						 u32 preemption_timeout)
> +{
> +	u32 action[] = {
> +		INTEL_GUC_ACTION_SET_CONTEXT_PREEMPTION_TIMEOUT,
> +		guc_id,
> +		preemption_timeout
> +	};
> +
> +	intel_guc_send_busy_loop(guc, action, ARRAY_SIZE(action), 0, true);
> +}
> +
>   static struct i915_sw_fence *guc_context_block(struct intel_context *ce)
>   {
>   	struct intel_guc *guc = ce_to_guc(ce);
> @@ -2442,8 +2457,10 @@ static struct i915_sw_fence *guc_context_block(struct intel_context *ce)
>   
>   	spin_unlock_irqrestore(&ce->guc_state.lock, flags);
>   
> -	with_intel_runtime_pm(runtime_pm, wakeref)
> +	with_intel_runtime_pm(runtime_pm, wakeref) {
> +		__guc_context_set_preemption_timeout(guc, guc_id, 1);
>   		__guc_context_sched_disable(guc, ce, guc_id);
> +	}
>   
>   	return &ce->guc_state.blocked;
>   }
> @@ -2492,8 +2509,10 @@ static void guc_context_unblock(struct intel_context *ce)
>   
>   	spin_unlock_irqrestore(&ce->guc_state.lock, flags);
>   
> -	if (enable) {
> -		with_intel_runtime_pm(runtime_pm, wakeref)
> +	with_intel_runtime_pm(runtime_pm, wakeref) {
> +		__guc_context_set_preemption_timeout(guc, ce->guc_id.id,
> +						     ce->guc_state.preemption_timeout);
> +		if (enable)
>   			__guc_context_sched_enable(guc, ce);
>   	}
>   }
> @@ -2521,19 +2540,6 @@ static void guc_context_cancel_request(struct intel_context *ce,
>   	}
>   }
>   
> -static void __guc_context_set_preemption_timeout(struct intel_guc *guc,
> -						 u16 guc_id,
> -						 u32 preemption_timeout)
> -{
> -	u32 action[] = {
> -		INTEL_GUC_ACTION_SET_CONTEXT_PREEMPTION_TIMEOUT,
> -		guc_id,
> -		preemption_timeout
> -	};
> -
> -	intel_guc_send_busy_loop(guc, action, ARRAY_SIZE(action), 0, true);
> -}
> -
>   static void guc_context_ban(struct intel_context *ce, struct i915_request *rq)
>   {
>   	struct intel_guc *guc = ce_to_guc(ce);