Re: [PATCH v2] 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_mert_freq_cap() stashes RPe and pins
  min_freq to RPe
- [Medium] drm/xe: race in xe_guc_pc_set_use_rpe() with
  pc_set_mert_freq_cap()
- [Low] drm/xe/guc_pc: repeated RPe MMIO reads in pc_set_mert_freq_cap()
--

commit 7e119245b60ecad85ca2f154bc2f98a7b90fc53a
Author: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

drm/xe: Add sysfs to disable RPe

This commit adds a use_rpe sysfs attribute to toggle SLPC efficient
frequency usage independently of min_freq requests.

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

[ ... ]

> @@ -1072,6 +1080,7 @@ void xe_guc_pc_remove_flush_freq_limit(struct xe_guc_pc *pc)
>  static int pc_set_mert_freq_cap(struct xe_guc_pc *pc)
>  {
>  	int ret;
> +	u32 mert_freq_cap;
>  
>  	if (!XE_GT_WA(pc_to_gt(pc), 22019338487))
>  		return 0;
> @@ -1090,6 +1099,14 @@ static int pc_set_mert_freq_cap(struct xe_guc_pc *pc)
>  	/*
>  	 * Ensure min and max are bound by MERT_FREQ_CAP until driver loads.
>  	 */
> +	mert_freq_cap = min(xe_guc_pc_get_rpe_freq(pc), pc_max_freq_cap(pc));
> +	if (mert_freq_cap < xe_guc_pc_get_rpe_freq(pc) && pc->use_rpe) {
> +		/* Temporarily let GuC drop min below RPe so the cap can be applied. */
> +		ret = pc_set_ignore_eff_freq(pc, 1);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = pc_set_min_freq(pc, min(xe_guc_pc_get_rpe_freq(pc), pc_max_freq_cap(pc)));
>  	if (!ret)
>  		ret = pc_set_max_freq(pc, min(pc->rp0_freq, pc_max_freq_cap(pc)));

[Severity: Low]
Why call xe_guc_pc_get_rpe_freq(pc) three times here instead of reading it
once and passing mert_freq_cap to pc_set_min_freq()?

Each call reads the live RPe register from MMIO, which PCODE updates
dynamically at runtime.  If RPe rises above pc_max_freq_cap() between the
first two reads or while the cap is active, pc_set_ignore_eff_freq(pc, 1)
is skipped and GuC can still raise the minimum frequency to RPe above the
Wa_22019338487 cap.

> @@ -1113,6 +1130,11 @@ int xe_guc_pc_restore_stashed_freq(struct xe_guc_pc *pc)
>  
>  	mutex_lock(&pc->freq_lock);
>  	ret = pc_set_max_freq(pc, pc->stashed_max_freq);
> +
> +	/* Undo any temporary ignore_eff_freq override from pc_set_mert_freq_cap(). */
> +	if (!ret)
> +		ret = pc_set_ignore_eff_freq(pc, !pc->use_rpe);
> +
>  	if (!ret)
>  		ret = pc_set_min_freq(pc, pc->stashed_min_freq);
>  	mutex_unlock(&pc->freq_lock);

[Severity: High]
Does pc_set_mert_freq_cap() -> xe_guc_pc_restore_stashed_freq() overwrite
the underlying minimum frequency with RPe on platforms with
Wa_22019338487?

In xe_guc_pc_start(), pc_set_ignore_eff_freq(pc, 0) runs before
pc_set_mert_freq_cap().  With ignore_eff_freq disabled, GuC elevates the
minimum frequency reported by xe_guc_pc_get_min_freq_locked() to RPe, so
pc->stashed_min_freq records RPe instead of RPn or a lower
pc->user_requested_min.

When xe_guc_pc_restore_stashed_freq() later calls pc_set_min_freq(pc,
pc->stashed_min_freq), it explicitly sets
SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ in GuC to that stashed RPe
value.  Writing 0 to use_rpe after probe, GSC load, or a GT reset only
toggles SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY, leaving min_freq pinned at
RPe.

> @@ -1166,6 +1188,28 @@ static int pc_action_set_strategy(struct xe_guc_pc *pc, u32 val)
>  	return ret;
>  }
>  
> +int xe_guc_pc_set_use_rpe(struct xe_guc_pc *pc, u32 val)
> +{
> +	int ret;
> +
> +	guard(mutex)(&pc->freq_lock);
> +
> +	/* Might be in the middle of a gt reset */
> +	if (!pc->freq_ready)
> +		return -EAGAIN;
> +
> +	/**
> +	 * If use_rpe == 1, ignore_eff_freq = false. GuC elevates min_freq to
> +	 * RPe when enabled, and resets it back to the previously set min
> +	 * when disabled, so no explicit set_min_freq request is needed here.
> +	 */
> +	ret = pc_set_ignore_eff_freq(pc, !val);
> +	if (!ret)
> +		pc->use_rpe = val;
> +
> +	return ret;
> +}

[Severity: Medium]
Can a sysfs write to use_rpe clear the temporary ignore_eff_freq override
while the Wa_22019338487 MERT frequency cap is still active?

CPU0                                    CPU1 (sysfs / udev)
xe_guc_pc_start()
  pc_init_freqs()
    mutex_lock(&pc->freq_lock)
    pc->freq_ready = true
    mutex_unlock(&pc->freq_lock)
  pc_set_mert_freq_cap()
    mutex_lock(&pc->freq_lock)
    pc_set_ignore_eff_freq(pc, 1)
    pc_set_min_freq(pc, mert_freq_cap)
    pc_set_max_freq(pc, mert_freq_cap)
    mutex_unlock(&pc->freq_lock)
                                        use_rpe_store(..., "1", ...)
                                          xe_guc_pc_set_use_rpe(pc, 1)
                                            mutex_lock(&pc->freq_lock)
                                            pc_set_ignore_eff_freq(pc, 0)
                                              <-- elevates min to RPe > cap
                                            mutex_unlock(&pc->freq_lock)
...
gsc_upload_and_init()
  xe_gt_sanitize_freq()
    xe_guc_pc_restore_stashed_freq()

pc_init_freqs() sets pc->freq_ready = true and drops pc->freq_lock before
pc_set_mert_freq_cap() applies the cap, and on platforms with GSC firmware
(such as Lunar Lake media GT) xe_guc_pc_restore_stashed_freq() does not
run until asynchronous GSC upload completes.  If userspace writes 1 to
use_rpe during that window (or during a GT reset/resume before
xe_gt_sanitize_freq()), xe_guc_pc_set_use_rpe() immediately sends
SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY=0 to GuC, raising the minimum
frequency back to RPe and bypassing the Wa_22019338487 cap.

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