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

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

On 10/29/2025 12:58 PM, Vivi, Rodrigo wrote:
> On Thu, 2025-10-30 at 00:05 +0530, Anirban, Sk wrote:
>> Hi,
>>
>> On 29-10-2025 23:34, Rodrigo Vivi wrote:
>>> On Wed, Oct 29, 2025 at 04:50:16PM +0530, 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: Update RPn while fetching RPe frequency
>>>>
>>>> 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       | 75 ++++++++++++++++-----
>>>> -------
>>>>    drivers/gpu/drm/xe/xe_guc_pc_types.h |  2 -
>>>>    2 files changed, 42 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c
>>>> b/drivers/gpu/drm/xe/xe_guc_pc.c
>>>> index 3c0feb50a1e2..08deaa64aa85 100644
>>>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>>>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>>>> @@ -330,7 +330,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,
>>>> @@ -375,7 +375,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;
>>>> @@ -385,7 +385,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)
>>>> @@ -408,24 +408,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)
>>>> @@ -433,20 +431,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);
>>>>    }
>>>>    
>>>>    /**
>>>> @@ -560,9 +548,30 @@ 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);
>>>> +	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
>>>> +	 */
>>>> +	if (xe->info.platform == XE_PVC)
>>> I believe it would be better to convert this to the graphics
>>> version here
>>> instead of the platform name. But no block since it was already a
>>> platform check
>>> above.
>> sure, I will look into this.
>>>> +		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);
>>>> +
>>>> +	/*
>>>> +	 * 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, freq);
>>> setting rpn_freq inside this get_rpe function makes no sense.
>>>
>>> I'm sorry I forgot about this when I told you to kill the other
>>> function.
>>> It should stay there, but be called update_rpn instead...
>> Since RPn updates depend on RPe, my intention was to update RPn
>> whenever
>> RPe is fetched.
>> Alternatively, I can maintain a separate update_rpn() function, but
>> in
>> either case, it will be invoked during initialization and while
>> retrieving RPe frequencies.
>> Another option could be to simply rename the function to
>> xe_guc_pc_get_rpe_rpn_freq() to reflect its dual purpose.
>> What are your thoughts on this approach?
> I think you need to get to the drawing board and think on another
> proposal. This is becoming to get really messier.
>
> update_rp simply update them all before the actual get.
>
> you don't update another thing inside a get of a specific thing
> of interest.
>
> think of what are the cases where the rpn update is really needed.
> think if we should have a constant workqueue refreshing it if not
> idle/suspended. Think about all the consequences of the updates
> and removal that you are proposing. And always making sure that
> that that igt freq test is reliably passing.
>
> But something is strange in the current proposal.

Agreed. The issue was we needed to get updated RPe. Why do we need to 
update RPn? That is a fused value and never changes. SLPC min freq 
changes with RPe. So we need to ensure we read SLPC min every time (if 
we are not doing that already).

Thanks,

Vinay.

>
>> Thanks,
>> Anirban
>>>> +
>>>> +	return freq;
>>>>    }
>>>>    
>>>>    /**
>>>> @@ -1021,7 +1030,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)));
>>>>    
>>>> @@ -1339,7 +1348,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 */
>>>> -- 
>>>> 2.43.0
>>>>