Re: [PATCH v6 1/2] drm/xe/guc: Eliminate RPe caching for SLPC parameter handling

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

On 11/4/2025 11:57 AM, Sk Anirban wrote:
> RPe is runtime-determined by PCODE and caching it caused stale values,
> leading to incorrect GuC SLPC parameter settings.
> Drop the cached rpe_freq field and query fresh values from hardware
> on each use to ensure GuC SLPC parameters reflect current RPe.
>
> v2: Remove cached RPe frequency field (Rodrigo)
> v3: Remove extra variable (Vinay)
>      Modify function name (Vinay)
> v4: Maintain a separate function for PVC (Rodrigo)
> v5: Avoid RPn update while fetching RPe frequency (Rodrigo)
>
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/5166
> Signed-off-by: Sk Anirban <sk.anirban@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_guc_pc.c       | 68 ++++++++++++++--------------
>   drivers/gpu/drm/xe/xe_guc_pc_types.h |  2 -
>   2 files changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index ff22235857f8..efa9318c4587 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> @@ -331,7 +331,7 @@ static int pc_set_min_freq(struct xe_guc_pc *pc, u32 freq)
>   	 * Our goal is to have the admin choices respected.
>   	 */
>   	pc_action_set_param(pc, SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
> -			    freq < pc->rpe_freq);
> +			    freq < xe_guc_pc_get_rpe_freq(pc));
>   
>   	return pc_action_set_param(pc,
>   				   SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
> @@ -376,7 +376,7 @@ static void mtl_update_rpa_value(struct xe_guc_pc *pc)
>   	pc->rpa_freq = decode_freq(REG_FIELD_GET(MTL_RPA_MASK, reg));
>   }
>   
> -static void mtl_update_rpe_value(struct xe_guc_pc *pc)
> +static u32 mtl_get_rpe_freq(struct xe_guc_pc *pc)
>   {
>   	struct xe_gt *gt = pc_to_gt(pc);
>   	u32 reg;
> @@ -386,7 +386,7 @@ static void mtl_update_rpe_value(struct xe_guc_pc *pc)
>   	else
>   		reg = xe_mmio_read32(&gt->mmio, MTL_GT_RPE_FREQUENCY);
>   
> -	pc->rpe_freq = decode_freq(REG_FIELD_GET(MTL_RPE_MASK, reg));
> +	return decode_freq(REG_FIELD_GET(MTL_RPE_MASK, reg));
>   }
>   
>   static void tgl_update_rpa_value(struct xe_guc_pc *pc)
> @@ -409,24 +409,22 @@ static void tgl_update_rpa_value(struct xe_guc_pc *pc)
>   	}
>   }
>   
> -static void tgl_update_rpe_value(struct xe_guc_pc *pc)
> +static u32 pvc_get_rpe_freq(struct xe_guc_pc *pc)
>   {
>   	struct xe_gt *gt = pc_to_gt(pc);
> -	struct xe_device *xe = gt_to_xe(gt);
>   	u32 reg;
>   
> -	/*
> -	 * For PVC we still need to use fused RP1 as the approximation for RPe
> -	 * For other platforms than PVC we get the resolved RPe directly from
> -	 * PCODE at a different register
> -	 */
> -	if (xe->info.platform == XE_PVC) {
> -		reg = xe_mmio_read32(&gt->mmio, PVC_RP_STATE_CAP);
> -		pc->rpe_freq = REG_FIELD_GET(RP1_MASK, reg) * GT_FREQUENCY_MULTIPLIER;
> -	} else {
> -		reg = xe_mmio_read32(&gt->mmio, FREQ_INFO_REC);
> -		pc->rpe_freq = REG_FIELD_GET(RPE_MASK, reg) * GT_FREQUENCY_MULTIPLIER;
> -	}
> +	reg = xe_mmio_read32(&gt->mmio, PVC_RP_STATE_CAP);
> +	return REG_FIELD_GET(RP1_MASK, reg) * GT_FREQUENCY_MULTIPLIER;
> +}
> +
> +static u32 tgl_get_rpe_freq(struct xe_guc_pc *pc)
> +{
> +	struct xe_gt *gt = pc_to_gt(pc);
> +	u32 reg;
> +
> +	reg = xe_mmio_read32(&gt->mmio, FREQ_INFO_REC);
> +	return REG_FIELD_GET(RPE_MASK, reg) * GT_FREQUENCY_MULTIPLIER;
>   }
>   
>   static void pc_update_rp_values(struct xe_guc_pc *pc)
> @@ -434,20 +432,10 @@ static void pc_update_rp_values(struct xe_guc_pc *pc)
>   	struct xe_gt *gt = pc_to_gt(pc);
>   	struct xe_device *xe = gt_to_xe(gt);
>   
> -	if (GRAPHICS_VERx100(xe) >= 1270) {
> +	if (GRAPHICS_VERx100(xe) >= 1270)
>   		mtl_update_rpa_value(pc);
> -		mtl_update_rpe_value(pc);
> -	} else {
> +	else
>   		tgl_update_rpa_value(pc);
> -		tgl_update_rpe_value(pc);
> -	}
> -
> -	/*
> -	 * RPe is decided at runtime by PCODE. In the rare case where that's
> -	 * smaller than the fused min, we will trust the PCODE and use that
> -	 * as our minimum one.
> -	 */
> -	pc->rpn_freq = min(pc->rpn_freq, pc->rpe_freq);
>   }
>   
>   /**
> @@ -561,9 +549,23 @@ u32 xe_guc_pc_get_rpa_freq(struct xe_guc_pc *pc)
>    */
>   u32 xe_guc_pc_get_rpe_freq(struct xe_guc_pc *pc)
>   {
> -	pc_update_rp_values(pc);
> +	struct xe_gt *gt = pc_to_gt(pc);
> +	struct xe_device *xe = gt_to_xe(gt);
just use pc_to_xe(pc) here, we don't need gt.
> +	u32 freq;
>   
> -	return pc->rpe_freq;
> +	/*
> +	 * For PVC we still need to use fused RP1 as the approximation for RPe
> +	 * For other platforms than PVC we get the resolved RPe directly from
> +	 * PCODE at a different register

This comment makes more sense in the pvc function where we are reading 
RP1. So, maybe we can split the comment and put in the respective 
functions?

Other than that,

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

> +	 */
> +	if (GRAPHICS_VERx100(xe) == 1260)
> +		freq = pvc_get_rpe_freq(pc);
> +	else if (GRAPHICS_VERx100(xe) >= 1270)
> +		freq = mtl_get_rpe_freq(pc);
> +	else
> +		freq = tgl_get_rpe_freq(pc);
> +
> +	return freq;
>   }
>   
>   /**
> @@ -1022,7 +1024,7 @@ 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.
>   	 */
> -	ret = pc_set_min_freq(pc, min(pc->rpe_freq, pc_max_freq_cap(pc)));
> +	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)));
>   
> @@ -1340,7 +1342,7 @@ static void xe_guc_pc_fini_hw(void *arg)
>   	XE_WARN_ON(xe_guc_pc_stop(pc));
>   
>   	/* Bind requested freq to mert_freq_cap before unload */
> -	pc_set_cur_freq(pc, min(pc_max_freq_cap(pc), pc->rpe_freq));
> +	pc_set_cur_freq(pc, min(pc_max_freq_cap(pc), xe_guc_pc_get_rpe_freq(pc)));
>   
>   	xe_force_wake_put(gt_to_fw(pc_to_gt(pc)), fw_ref);
>   }
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc_types.h b/drivers/gpu/drm/xe/xe_guc_pc_types.h
> index 5e4ea53fbee6..f27c05d81706 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc_types.h
> +++ b/drivers/gpu/drm/xe/xe_guc_pc_types.h
> @@ -21,8 +21,6 @@ struct xe_guc_pc {
>   	u32 rp0_freq;
>   	/** @rpa_freq: HW RPa frequency - The Achievable one */
>   	u32 rpa_freq;
> -	/** @rpe_freq: HW RPe frequency - The Efficient one */
> -	u32 rpe_freq;
>   	/** @rpn_freq: HW RPN frequency - The Minimum one */
>   	u32 rpn_freq;
>   	/** @user_requested_min: Stash the minimum requested freq by user */