Re: [PATCH 1/2] drm/i915/guc/slpc: Allow GuC SLPC default strategies on MTL+

["Belgaumkar, Vinay" <vinay.belgaumkar@xxxxxxxxx>]

On 1/10/2025 6:46 AM, Rodrigo Vivi wrote:
> The Balancer and DCC strategies were left off on a fear that
> these strategies would conflict with the i915's waitboost.
>
> However, on MTL and Beyond these strategies are only active in
> certain conditions where the system is TDP limited.
> So, they don't conflict, but help the
> waitboost by guaranteeing a bit more of GT frequency.
>
> Without these strategies we were likely leaving some performance
> behind on some scenarios.
>
> With this change in place, the enabling/disabling of DCC and Balancer
> will now be chosen by GuC, on a platform/GT basis.
>
> v2: - Fix typos and be clear on GuC decision on platform basis (Vinay)
>      - Limit change to MTL and beyond, where GuC started to take
>        TDP limit into consideration.

LGTM,

Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx>

> v3: Fix compilation. Actually amend the changes...
>
> Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx> #v1
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 22 ++++++++++++++-------
>   1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> index 706fffca698b..1f8e6f7c2c67 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> @@ -357,21 +357,29 @@ static u32 slpc_decode_max_freq(struct intel_guc_slpc *slpc)
>   				  GT_FREQUENCY_MULTIPLIER, GEN9_FREQ_SCALER);
>   }
>   
> -static void slpc_shared_data_reset(struct slpc_shared_data *data)
> +static void slpc_shared_data_reset(struct intel_guc_slpc *slpc)
>   {
> -	memset(data, 0, sizeof(struct slpc_shared_data));
> +	struct drm_i915_private *i915 = slpc_to_i915(slpc);
> +	struct slpc_shared_data *data = slpc->vaddr;
>   
> +	memset(data, 0, sizeof(struct slpc_shared_data));
>   	data->header.size = sizeof(struct slpc_shared_data);
>   
>   	/* Enable only GTPERF task, disable others */
>   	slpc_mem_set_enabled(data, SLPC_PARAM_TASK_ENABLE_GTPERF,
>   			     SLPC_PARAM_TASK_DISABLE_GTPERF);
>   
> -	slpc_mem_set_disabled(data, SLPC_PARAM_TASK_ENABLE_BALANCER,
> -			      SLPC_PARAM_TASK_DISABLE_BALANCER);
> +	/*
> +	 * Don't allow balancer related algorithms on platforms before
> +	 * Xe_LPG, where GuC started to restrict it to TDP limited scenarios.
> +	 */
> +	if (GRAPHICS_VER_FULL(i915) < IP_VER(12, 70)) {
> +		slpc_mem_set_disabled(data, SLPC_PARAM_TASK_ENABLE_BALANCER,
> +				      SLPC_PARAM_TASK_DISABLE_BALANCER);
>   
> -	slpc_mem_set_disabled(data, SLPC_PARAM_TASK_ENABLE_DCC,
> -			      SLPC_PARAM_TASK_DISABLE_DCC);
> +		slpc_mem_set_disabled(data, SLPC_PARAM_TASK_ENABLE_DCC,
> +				      SLPC_PARAM_TASK_DISABLE_DCC);
> +	}
>   }
>   
>   /**
> @@ -686,7 +694,7 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc)
>   
>   	GEM_BUG_ON(!slpc->vma);
>   
> -	slpc_shared_data_reset(slpc->vaddr);
> +	slpc_shared_data_reset(slpc);
>   
>   	ret = slpc_reset(slpc);
>   	if (unlikely(ret < 0)) {