Re: [PATCH] drm/xe: Add sysfs to disable RPe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] drm/xe/guc_pc: pc_set_min_freq() breaks Wa_22019338487 and pins
  min_freq to RPe
- [Medium] drm/xe/guc_pc: race and missing freq_ready check in
  xe_guc_pc_set_use_rpe()
- [Low] drm/xe/guc_pc: xe_guc_pc_set_use_rpe() logs inverted value !val
  on error
--

commit 64d4db3f839aa54f062c511fa3457dc84a1c5dbf
Author: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

drm/xe: Add sysfs to disable RPe

This commit adds a per-GT use_rpe sysfs attribute to control
SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY directly and removes the automatic
SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY update from pc_set_min_freq().

> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index 9558d7196a51a..8f7044b49e135 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c

[ ... ]

> @@ -353,13 +368,6 @@ static int pc_set_min_freq(struct xe_guc_pc *pc, u32 freq)
>  	if (freq < pc->rpn_freq || freq > pc->rp0_freq)
>  		return -EINVAL;
>  
> -	/*
> -	 * GuC policy is to elevate minimum frequency to the efficient levels
> -	 * Our goal is to have the admin choices respected.
> -	 */
> -	pc_action_set_param(pc, SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
> -			    freq < xe_guc_pc_get_rpe_freq(pc));
> -
>  	return pc_action_set_param(pc,
>  				   SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
>  				   freq);
> @@ -1166,6 +1174,26 @@ static int pc_action_set_strategy(struct xe_guc_pc *pc, u32 val)

[Severity: High]
Does removing SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY from pc_set_min_freq()
break Wa_22019338487 in pc_set_mert_freq_cap() and leave min_freq pinned
at RPe after xe_guc_pc_restore_stashed_freq()?

drivers/gpu/drm/xe/xe_guc_pc.c:pc_set_mert_freq_cap() {
	...
	ret = xe_guc_pc_get_min_freq_locked(pc, &pc->stashed_min_freq);
	if (!ret)
		ret = xe_guc_pc_get_max_freq_locked(pc, &pc->stashed_max_freq);
	if (ret)
		return ret;

	/*
	 * Ensure min and max are bound by MERT_FREQ_CAP until driver loads.
	 */
	ret = pc_set_min_freq(pc, min(xe_guc_pc_get_rpe_freq(pc), pc_max_freq_cap(pc)));
	...
}

When pc->use_rpe is true, SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY is 0, so
pc_set_min_freq() can no longer lower the minimum frequency to
pc_max_freq_cap(pc) when RPe > pc_max_freq_cap(pc) during driver load, GSC
load, or GT reset.

In addition, xe_guc_pc_get_min_freq_locked() reads the RPe-elevated
minimum frequency from GuC into pc->stashed_min_freq, and
xe_gt_sanitize_freq() -> xe_guc_pc_restore_stashed_freq() then explicitly
programs SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ to RPe.  Because
xe_guc_pc_set_use_rpe() only toggles SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
later writing 0 to use_rpe will not drop min_freq back to RPn or
pc->user_requested_min.

>  	return ret;
>  }
>  
> +int xe_guc_pc_set_use_rpe(struct xe_guc_pc *pc, u32 val)
> +{
> +	int ret;
> +
> +	guard(mutex)(&pc->freq_lock);
> +
> +	/* If use_rpe == 1, ignore_eff_freq = false */
> +	ret = pc_set_ignore_eff_freq(pc, !val);
> +	if (ret) {
> +		xe_gt_err_once(pc_to_gt(pc),
> +			       "Failed to set use_rpe to %d: %pe\n",
> +			       !val, ERR_PTR(ret));
> +		return ret;
> +	}

[Severity: Low]
Does this error message log !val (the inverted ignore_eff_freq value)
instead of val (the requested use_rpe value), while duplicating the
xe_gt_err_once() already logged inside pc_set_ignore_eff_freq()?

> +
> +	pc->use_rpe = val;
> +
> +	return ret;
> +}
> +
>  static const char *power_profile_to_string(struct xe_guc_pc *pc)
>  {
>  	switch (pc->power_profile) {
> @@ -1314,6 +1342,10 @@ int xe_guc_pc_start(struct xe_guc_pc *pc)
>  	if (ret)
>  		return ret;
>  
> +	ret = pc_set_ignore_eff_freq(pc, !pc->use_rpe);
> +	if (unlikely(ret))
> +		return ret;
> +
>  	ret = pc_init_freqs(pc);
>  	if (ret)
>  		return ret;

[Severity: Medium]
Can xe_guc_pc_set_use_rpe() race with GT reset and xe_guc_pc_start()
because xe_guc_pc_set_use_rpe() does not check pc->freq_ready and
xe_guc_pc_start() updates SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY outside
pc->freq_lock?

CPU0                              CPU1
xe_guc_pc_start()
  !pc->use_rpe (reads 0)
                                  use_rpe_store()
                                    xe_guc_pc_set_use_rpe(pc, 0)
                                      mutex_lock(&pc->freq_lock)
                                      pc_set_ignore_eff_freq(pc, 1)
                                      pc->use_rpe = 0
                                      mutex_unlock(&pc->freq_lock)
  pc_set_ignore_eff_freq(pc, 0)
    <-- overwrites GuC state with stale value
  pc_init_freqs()
    mutex_lock(&pc->freq_lock)
    pc->freq_ready = true
    mutex_unlock(&pc->freq_lock)

Unlike xe_guc_pc_set_min_freq() and xe_guc_pc_set_max_freq(),
xe_guc_pc_set_use_rpe() also does not check pc->freq_ready under
pc->freq_lock, so a sysfs write after xe_guc_pc_stop() can send an H2G
request while GuC PC is stopped instead of returning -EAGAIN.  Checking
pc->freq_ready in xe_guc_pc_set_use_rpe() and moving
pc_set_ignore_eff_freq() inside pc_init_freqs() under pc->freq_lock would
avoid both issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111212150.3738637-2-vinay.belgaumkar@intel.com?part=1