Re: [PATCH 2/3] drm/i915/guc: Update scheduling policies to new GuC API

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@xxxxxxxxx>]

On 4/8/2022 11:03 AM, John.C.Harrison@xxxxxxxxx wrote:
> From: John Harrison <John.C.Harrison@xxxxxxxxx>
>
> The latest GuC firmware drops the individual scheduling policy update
> H2G commands in favour of a single KLV based H2G. So, change the
> update wrappers accordingly.
>
> Unfortunately, the API changes also mean losing the ability to set any
> scheduling policy values during context registration. Instead the same
> KLV based H2G must be sent after the registration. Of course, that
> second H2G per registration might fail due to being backed up. The
> registration code has a complicated state machine to cope with the
> actual registration call failing. However, if that works then there is
> no support for unwinding if a further call should fail. Unwinding
> would require sending a H2G to de-register - but that can't be done
> because the CTB is already backed up.
>
> So instead, add a new flag to say whether the context has a pending
> policy update. This is set if the policy H2G fails at registration
> time. The submission code checks for this flag and retries the policy
> update if set. If that call fails, the submission path early exists
> with a retry error. This is something that is already supported for
> other reasons.
>
> Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>

Daniele

> ---
>   .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   4 +-
>   drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h |  15 ++
>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |  19 +-
>   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 176 ++++++++++++++----
>   4 files changed, 175 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
> index 9ad6df1b6fbc..be9ac47fa9d0 100644
> --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
> +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
> @@ -122,11 +122,9 @@ enum intel_guc_action {
>   	INTEL_GUC_ACTION_SCHED_CONTEXT_MODE_DONE = 0x1002,
>   	INTEL_GUC_ACTION_SCHED_ENGINE_MODE_SET = 0x1003,
>   	INTEL_GUC_ACTION_SCHED_ENGINE_MODE_DONE = 0x1004,
> -	INTEL_GUC_ACTION_SET_CONTEXT_PRIORITY = 0x1005,
> -	INTEL_GUC_ACTION_SET_CONTEXT_EXECUTION_QUANTUM = 0x1006,
> -	INTEL_GUC_ACTION_SET_CONTEXT_PREEMPTION_TIMEOUT = 0x1007,
>   	INTEL_GUC_ACTION_CONTEXT_RESET_NOTIFICATION = 0x1008,
>   	INTEL_GUC_ACTION_ENGINE_FAILURE_NOTIFICATION = 0x1009,
> +	INTEL_GUC_ACTION_HOST2GUC_UPDATE_CONTEXT_POLICIES = 0x100B,
>   	INTEL_GUC_ACTION_SETUP_PC_GUCRC = 0x3004,
>   	INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000,
>   	INTEL_GUC_ACTION_GET_HWCONFIG = 0x4100,
> diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h
> index f0814a57c191..4a59478c3b5c 100644
> --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h
> +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h
> @@ -6,6 +6,8 @@
>   #ifndef _ABI_GUC_KLVS_ABI_H
>   #define _ABI_GUC_KLVS_ABI_H
>   
> +#include <linux/types.h>
> +
>   /**
>    * DOC: GuC KLV
>    *
> @@ -79,4 +81,17 @@
>   #define GUC_KLV_SELF_CFG_G2H_CTB_SIZE_KEY		0x0907
>   #define GUC_KLV_SELF_CFG_G2H_CTB_SIZE_LEN		1u
>   
> +/*
> + * Per context scheduling policy update keys.
> + */
> +enum  {
> +	GUC_CONTEXT_POLICIES_KLV_ID_EXECUTION_QUANTUM			= 0x2001,
> +	GUC_CONTEXT_POLICIES_KLV_ID_PREEMPTION_TIMEOUT			= 0x2002,
> +	GUC_CONTEXT_POLICIES_KLV_ID_SCHEDULING_PRIORITY			= 0x2003,
> +	GUC_CONTEXT_POLICIES_KLV_ID_PREEMPT_TO_IDLE_ON_QUANTUM_EXPIRY	= 0x2004,
> +	GUC_CONTEXT_POLICIES_KLV_ID_SLPM_GT_FREQUENCY			= 0x2005,
> +
> +	GUC_CONTEXT_POLICIES_KLV_NUM_IDS = 5,
> +};
> +
>   #endif /* _ABI_GUC_KLVS_ABI_H */
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> index 0e1e8d0079b5..c154b5efccde 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> @@ -221,11 +221,22 @@ struct guc_ctxt_registration_info {
>   };
>   #define CONTEXT_REGISTRATION_FLAG_KMD	BIT(0)
>   
> -#define CONTEXT_POLICY_DEFAULT_EXECUTION_QUANTUM_US 1000000
> -#define CONTEXT_POLICY_DEFAULT_PREEMPTION_TIME_US 500000
> +/* 32-bit KLV structure as used by policy updates and others */
> +struct guc_klv_generic_dw_t {
> +	u32 kl;
> +	u32 value;
> +} __packed;
>   
> -/* Preempt to idle on quantum expiry */
> -#define CONTEXT_POLICY_FLAG_PREEMPT_TO_IDLE	BIT(0)
> +/* Format of the UPDATE_CONTEXT_POLICIES H2G data packet */
> +struct guc_update_context_policy_header {
> +	u32 action;
> +	u32 ctx_id;
> +} __packed;
> +
> +struct guc_update_context_policy {
> +	struct guc_update_context_policy_header header;
> +	struct guc_klv_generic_dw_t klv[GUC_CONTEXT_POLICIES_KLV_NUM_IDS];
> +} __packed;
>   
>   #define GUC_POWER_UNSPECIFIED	0
>   #define GUC_POWER_D0		1
> 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 bd0584d7d489..2bd680064942 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -162,7 +162,8 @@ guc_create_parallel(struct intel_engine_cs **engines,
>   #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_POLICY_REQUIRED			BIT(7)
> +#define SCHED_STATE_BLOCKED_SHIFT			8
>   #define SCHED_STATE_BLOCKED		BIT(SCHED_STATE_BLOCKED_SHIFT)
>   #define SCHED_STATE_BLOCKED_MASK	(0xfff << SCHED_STATE_BLOCKED_SHIFT)
>   
> @@ -301,6 +302,23 @@ static inline void clr_context_registered(struct intel_context *ce)
>   	ce->guc_state.sched_state &= ~SCHED_STATE_REGISTERED;
>   }
>   
> +static inline bool context_policy_required(struct intel_context *ce)
> +{
> +	return ce->guc_state.sched_state & SCHED_STATE_POLICY_REQUIRED;
> +}
> +
> +static inline void set_context_policy_required(struct intel_context *ce)
> +{
> +	lockdep_assert_held(&ce->guc_state.lock);
> +	ce->guc_state.sched_state |= SCHED_STATE_POLICY_REQUIRED;
> +}
> +
> +static inline void clr_context_policy_required(struct intel_context *ce)
> +{
> +	lockdep_assert_held(&ce->guc_state.lock);
> +	ce->guc_state.sched_state &= ~SCHED_STATE_POLICY_REQUIRED;
> +}
> +
>   static inline u32 context_blocked(struct intel_context *ce)
>   {
>   	return (ce->guc_state.sched_state & SCHED_STATE_BLOCKED_MASK) >>
> @@ -593,6 +611,7 @@ int intel_guc_wait_for_idle(struct intel_guc *guc, long timeout)
>   					      true, timeout);
>   }
>   
> +static int guc_context_policy_init(struct intel_context *ce, bool loop);
>   static int try_context_registration(struct intel_context *ce, bool loop);
>   
>   static int __guc_add_request(struct intel_guc *guc, struct i915_request *rq)
> @@ -619,6 +638,12 @@ static int __guc_add_request(struct intel_guc *guc, struct i915_request *rq)
>   	GEM_BUG_ON(!atomic_read(&ce->guc_id.ref));
>   	GEM_BUG_ON(context_guc_id_invalid(ce));
>   
> +	if (context_policy_required(ce)) {
> +		err = guc_context_policy_init(ce, false);
> +		if (err)
> +			return err;
> +	}
> +
>   	spin_lock(&ce->guc_state.lock);
>   
>   	/*
> @@ -2142,6 +2167,8 @@ static int register_context(struct intel_context *ce, bool loop)
>   		spin_lock_irqsave(&ce->guc_state.lock, flags);
>   		set_context_registered(ce);
>   		spin_unlock_irqrestore(&ce->guc_state.lock, flags);
> +
> +		guc_context_policy_init(ce, loop);
>   	}
>   
>   	return ret;
> @@ -2191,21 +2218,111 @@ static inline u32 get_children_join_value(struct intel_context *ce,
>   	return __get_parent_scratch(ce)->join[child_index].semaphore;
>   }
>   
> -#if 0
> -/* FIXME: This needs to be updated for new v70 interface... */
> -static void guc_context_policy_init(struct intel_engine_cs *engine,
> -				    struct guc_lrc_desc *desc)
> +struct context_policy {
> +	u32 count;
> +	struct guc_update_context_policy h2g;
> +};
> +
> +static u32 __guc_context_policy_action_size(struct context_policy *policy)
>   {
> -	desc->policy_flags = 0;
> +	size_t bytes = sizeof(policy->h2g.header) +
> +		       (sizeof(policy->h2g.klv[0]) * policy->count);
>   
> -	if (engine->flags & I915_ENGINE_WANT_FORCED_PREEMPTION)
> -		desc->policy_flags |= CONTEXT_POLICY_FLAG_PREEMPT_TO_IDLE;
> +	return bytes / sizeof(u32);
> +}
> +
> +static void __guc_context_policy_start_klv(struct context_policy *policy, u16 guc_id)
> +{
> +	policy->h2g.header.action = INTEL_GUC_ACTION_HOST2GUC_UPDATE_CONTEXT_POLICIES;
> +	policy->h2g.header.ctx_id = guc_id;
> +	policy->count = 0;
> +}
> +
> +#define MAKE_CONTEXT_POLICY_ADD(func, id) \
> +static void __guc_context_policy_add_##func(struct context_policy *policy, u32 data) \
> +{ \
> +	GEM_BUG_ON(policy->count >= GUC_CONTEXT_POLICIES_KLV_NUM_IDS); \
> +	policy->h2g.klv[policy->count].kl = \
> +		FIELD_PREP(GUC_KLV_0_KEY, GUC_CONTEXT_POLICIES_KLV_ID_##id) | \
> +		FIELD_PREP(GUC_KLV_0_LEN, 1); \
> +	policy->h2g.klv[policy->count].value = data; \
> +	policy->count++; \
> +}
> +
> +MAKE_CONTEXT_POLICY_ADD(execution_quantum, EXECUTION_QUANTUM)
> +MAKE_CONTEXT_POLICY_ADD(preemption_timeout, PREEMPTION_TIMEOUT)
> +MAKE_CONTEXT_POLICY_ADD(priority, SCHEDULING_PRIORITY)
> +MAKE_CONTEXT_POLICY_ADD(preempt_to_idle, PREEMPT_TO_IDLE_ON_QUANTUM_EXPIRY)
> +
> +#undef MAKE_CONTEXT_POLICY_ADD
> +
> +static int __guc_context_set_context_policies(struct intel_guc *guc,
> +					      struct context_policy *policy,
> +					      bool loop)
> +{
> +	return guc_submission_send_busy_loop(guc, (u32 *)&policy->h2g,
> +					__guc_context_policy_action_size(policy),
> +					0, loop);
> +}
> +
> +static int guc_context_policy_init(struct intel_context *ce, bool loop)
> +{
> +	struct intel_engine_cs *engine = ce->engine;
> +	struct intel_guc *guc = &engine->gt->uc.guc;
> +	struct context_policy policy;
> +	u32 execution_quantum;
> +	u32 preemption_timeout;
> +	bool missing = false;
> +	unsigned long flags;
> +	int ret;
>   
>   	/* 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;
> +	execution_quantum = engine->props.timeslice_duration_ms * 1000;
> +	preemption_timeout = engine->props.preempt_timeout_ms * 1000;
> +
> +	__guc_context_policy_start_klv(&policy, ce->guc_id.id);
> +
> +	__guc_context_policy_add_priority(&policy, ce->guc_state.prio);
> +	__guc_context_policy_add_execution_quantum(&policy, execution_quantum);
> +	__guc_context_policy_add_preemption_timeout(&policy, preemption_timeout);
> +
> +	if (engine->flags & I915_ENGINE_WANT_FORCED_PREEMPTION)
> +		__guc_context_policy_add_preempt_to_idle(&policy, 1);
> +
> +	ret = __guc_context_set_context_policies(guc, &policy, loop);
> +	missing = ret != 0;
> +
> +	if (!missing && intel_context_is_parent(ce)) {
> +		struct intel_context *child;
> +
> +		for_each_child(ce, child) {
> +			__guc_context_policy_start_klv(&policy, child->guc_id.id);
> +
> +			if (engine->flags & I915_ENGINE_WANT_FORCED_PREEMPTION)
> +				__guc_context_policy_add_preempt_to_idle(&policy, 1);
> +
> +			child->guc_state.prio = ce->guc_state.prio;
> +			__guc_context_policy_add_priority(&policy, ce->guc_state.prio);
> +			__guc_context_policy_add_execution_quantum(&policy, execution_quantum);
> +			__guc_context_policy_add_preemption_timeout(&policy, preemption_timeout);
> +
> +			ret = __guc_context_set_context_policies(guc, &policy, loop);
> +			if (ret) {
> +				missing = true;
> +				break;
> +			}
> +		}
> +	}
> +
> +	spin_lock_irqsave(&ce->guc_state.lock, flags);
> +	if (missing)
> +		set_context_policy_required(ce);
> +	else
> +		clr_context_policy_required(ce);
> +	spin_unlock_irqrestore(&ce->guc_state.lock, flags);
> +
> +	return ret;
>   }
> -#endif
>   
>   static void prepare_context_registration_info(struct intel_context *ce,
>   					      struct guc_ctxt_registration_info *info)
> @@ -2234,9 +2351,6 @@ static void prepare_context_registration_info(struct intel_context *ce,
>   	info->hwlrca_lo = lower_32_bits(ce->lrc.lrca);
>   	info->hwlrca_hi = upper_32_bits(ce->lrc.lrca);
>   	info->flags = CONTEXT_REGISTRATION_FLAG_KMD;
> -	/* FIXME: This needs to be updated for new v70 interface... */
> -	//desc->priority = ce->guc_state.prio;
> -	//guc_context_policy_init(engine, desc);
>   
>   	/*
>   	 * If context is a parent, we need to register a process descriptor
> @@ -2263,10 +2377,6 @@ static void prepare_context_registration_info(struct intel_context *ce,
>   		memset(wq_desc, 0, sizeof(*wq_desc));
>   		wq_desc->wq_status = WQ_STATUS_ACTIVE;
>   
> -		/* FIXME: This needs to be updated for new v70 interface... */
> -		//desc->priority = ce->guc_state.prio;
> -		//guc_context_policy_init(engine, desc);
> -
>   		clear_children_join_go_memory(ce);
>   	}
>   }
> @@ -2581,13 +2691,11 @@ 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
> -	};
> +	struct context_policy policy;
>   
> -	intel_guc_send_busy_loop(guc, action, ARRAY_SIZE(action), 0, true);
> +	__guc_context_policy_start_klv(&policy, guc_id);
> +	__guc_context_policy_add_preemption_timeout(&policy, preemption_timeout);
> +	__guc_context_set_context_policies(guc, &policy, true);
>   }
>   
>   static void guc_context_ban(struct intel_context *ce, struct i915_request *rq)
> @@ -2832,16 +2940,20 @@ static int guc_context_alloc(struct intel_context *ce)
>   	return lrc_alloc(ce, ce->engine);
>   }
>   
> +static void __guc_context_set_prio(struct intel_guc *guc,
> +				   struct intel_context *ce)
> +{
> +	struct context_policy policy;
> +
> +	__guc_context_policy_start_klv(&policy, ce->guc_id.id);
> +	__guc_context_policy_add_priority(&policy, ce->guc_state.prio);
> +	__guc_context_set_context_policies(guc, &policy, true);
> +}
> +
>   static void guc_context_set_prio(struct intel_guc *guc,
>   				 struct intel_context *ce,
>   				 u8 prio)
>   {
> -	u32 action[] = {
> -		INTEL_GUC_ACTION_SET_CONTEXT_PRIORITY,
> -		ce->guc_id.id,
> -		prio,
> -	};
> -
>   	GEM_BUG_ON(prio < GUC_CLIENT_PRIORITY_KMD_HIGH ||
>   		   prio > GUC_CLIENT_PRIORITY_NORMAL);
>   	lockdep_assert_held(&ce->guc_state.lock);
> @@ -2852,9 +2964,9 @@ static void guc_context_set_prio(struct intel_guc *guc,
>   		return;
>   	}
>   
> -	guc_submission_send_busy_loop(guc, action, ARRAY_SIZE(action), 0, true);
> -
>   	ce->guc_state.prio = prio;
> +	__guc_context_set_prio(guc, ce);
> +
>   	trace_intel_context_set_prio(ce);
>   }
>