Re: [PATCH v7 2/2] drm/xe: Add a wrapper for SLPC set/unset params

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

On 2/3/2026 8:47 AM, Michal Wajdeczko wrote:
>
> On 1/28/2026 3:23 AM, Vinay Belgaumkar wrote:
>> Also, extract out the GuC RC related set/unset param functions
>> into xe_guc_rc file. GuC still allows us to override GuC RC mode
>> using an SLPC H2G interface. Continue to use that interface, but
>> move the related code to the newly created xe_guc_rc file.
>>
>> Cc: Riana Tauro <riana.tauro@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Reviewed-by: Riana Tauro <riana.tauro@intel.com>
>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>> ---
>> v2: xe_guc_rc functions to use guc pointer instead of gt (Michal W)
>> v3: Assert if runtime pm ref is not held (Michal W)
>> v4: Review comments (Riana)
>> v5: Use noresume instead of full resume, update title (Michal)
>> ---
>>   drivers/gpu/drm/xe/xe_guc_pc.c | 56 +++++++++++++++++++---------------
>>   drivers/gpu/drm/xe/xe_guc_pc.h |  5 ++-
>>   drivers/gpu/drm/xe/xe_guc_rc.c | 30 ++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_guc_rc.h |  3 ++
>>   drivers/gpu/drm/xe/xe_oa.c     |  9 +++---
>>   5 files changed, 70 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
>> index 878eb273c3e6..21fe73ab4583 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>> @@ -264,6 +264,37 @@ static int pc_action_unset_param(struct xe_guc_pc *pc, u8 id)
>>   	return ret;
>>   }
>>   
>> +/**
>> + * xe_guc_pc_action_set_param() - Set value of SLPC param
>> + * @pc: Xe_GuC_PC instance
>> + * @id: Param id
>> + * @value: Value to set
>> + *
>> + * This function can be used to set any SLPC param.
>> + *
>> + * Return: 0 on Success
>> + */
>> +int xe_guc_pc_action_set_param(struct xe_guc_pc *pc, u8 id, u32 value)
>> +{
>> +	xe_device_assert_mem_access(pc_to_xe(pc));
>> +	return pc_action_set_param(pc, id, value);
>> +}
>> +
>> +/**
>> + * xe_guc_pc_action_unset_param() - Revert to default value
>> + * @pc: Xe_GuC_PC instance
>> + * @id: Param id
>> + *
>> + * This function can be used revert any SLPC param to its default value.
>> + *
>> + * Return: 0 on Success
>> + */
>> +int xe_guc_pc_action_unset_param(struct xe_guc_pc *pc, u8 id)
>> +{
>> +	xe_device_assert_mem_access(pc_to_xe(pc));
>> +	return pc_action_unset_param(pc, id);
>> +}
>> +
>>   static u32 decode_freq(u32 raw)
>>   {
>>   	return DIV_ROUND_CLOSEST(raw * GT_FREQUENCY_MULTIPLIER,
>> @@ -1045,31 +1076,6 @@ int xe_guc_pc_restore_stashed_freq(struct xe_guc_pc *pc)
>>   	return ret;
>>   }
>>   
>> -/**
>> - * xe_guc_pc_override_gucrc_mode - override GUCRC mode
>> - * @pc: Xe_GuC_PC instance
>> - * @mode: new value of the mode.
>> - *
>> - * Return: 0 on success, negative error code on error
>> - */
>> -int xe_guc_pc_override_gucrc_mode(struct xe_guc_pc *pc, enum slpc_gucrc_mode mode)
>> -{
>> -	guard(xe_pm_runtime)(pc_to_xe(pc));
>> -	return pc_action_set_param(pc, SLPC_PARAM_PWRGATE_RC_MODE, mode);
>> -}
>> -
>> -/**
>> - * xe_guc_pc_unset_gucrc_mode - unset GUCRC mode override
>> - * @pc: Xe_GuC_PC instance
>> - *
>> - * Return: 0 on success, negative error code on error
>> - */
>> -int xe_guc_pc_unset_gucrc_mode(struct xe_guc_pc *pc)
>> -{
>> -	guard(xe_pm_runtime)(pc_to_xe(pc));
>> -	return pc_action_unset_param(pc, SLPC_PARAM_PWRGATE_RC_MODE);
>> -}
>> -
>>   static void pc_init_pcode_freq(struct xe_guc_pc *pc)
>>   {
>>   	u32 min = DIV_ROUND_CLOSEST(pc->rpn_freq, GT_FREQUENCY_MULTIPLIER);
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.h b/drivers/gpu/drm/xe/xe_guc_pc.h
>> index 1b95873b262e..0678a4e787b3 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.h
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.h
>> @@ -9,15 +9,14 @@
>>   #include <linux/types.h>
>>   
>>   struct xe_guc_pc;
>> -enum slpc_gucrc_mode;
>>   struct drm_printer;
>>   
>>   int xe_guc_pc_init(struct xe_guc_pc *pc);
>>   int xe_guc_pc_start(struct xe_guc_pc *pc);
>>   int xe_guc_pc_stop(struct xe_guc_pc *pc);
>> -int xe_guc_pc_override_gucrc_mode(struct xe_guc_pc *pc, enum slpc_gucrc_mode mode);
>> -int xe_guc_pc_unset_gucrc_mode(struct xe_guc_pc *pc);
>>   void xe_guc_pc_print(struct xe_guc_pc *pc, struct drm_printer *p);
>> +int xe_guc_pc_action_set_param(struct xe_guc_pc *pc, u8 id, u32 value);
>> +int xe_guc_pc_action_unset_param(struct xe_guc_pc *pc, u8 id);
>>   
>>   u32 xe_guc_pc_get_act_freq(struct xe_guc_pc *pc);
>>   int xe_guc_pc_get_cur_freq(struct xe_guc_pc *pc, u32 *freq);
>> diff --git a/drivers/gpu/drm/xe/xe_guc_rc.c b/drivers/gpu/drm/xe/xe_guc_rc.c
>> index 55eeee7b1011..a20b20e3dab3 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_rc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_rc.c
>> @@ -13,6 +13,7 @@
>>   #include "xe_gt_printk.h"
>>   #include "xe_guc.h"
>>   #include "xe_guc_ct.h"
>> +#include "xe_guc_pc.h"
>>   #include "xe_guc_rc.h"
>>   #include "xe_pm.h"
>>   
>> @@ -127,3 +128,32 @@ int xe_guc_rc_enable(struct xe_guc *guc)
>>   
>>   	return guc_action_setup_gucrc(guc, GUCRC_FIRMWARE_CONTROL);
>>   }
>> +
>> +/**
>> + * xe_guc_rc_set_mode() - set GUCRC mode
> nit:
> 	... - Set new GuC RC mode.
ok.
>
>> + * @guc: Xe GuC instance
>> + * @mode: new value of the mode.
>> + *
>> + * Function to set GuC RC mode to one of the enum values.
>> + *
>> + * Returns: 0 on success, negative error code on error
>> + */
>> +int xe_guc_rc_set_mode(struct xe_guc *guc, enum slpc_gucrc_mode mode)
> nit: maybe instead of exposing enum from the GuC ABI just provide explicitly named helper(s)?
>
> 	int xe_guc_rc_set_no_rc6_mode(guc);
> 	int xe_guc_rc_set_static_mode(guc);
> 	int xe_guc_rc_set_dynamic_mode(guc);
Can add this in a later patch. So far, setting this mode is only done by 
KMD, there is no debugfs/sysfs (yet) for this.
>
>
>> +{
>> +	guard(xe_pm_runtime_noresume)(guc_to_xe(guc));
>> +	return xe_guc_pc_action_set_param(&guc->pc, SLPC_PARAM_PWRGATE_RC_MODE, mode);
>> +}
>> +
>> +/**
>> + * xe_guc_rc_unset_mode() - revert to default mode
>> + * @guc: Xe GuC instance
>> + *
>> + * Function to revert GuC RC mode to platform defaults.
>> + *
>> + * Returns: 0 on success, negative error code on error
>> + */
>> +int xe_guc_rc_unset_mode(struct xe_guc *guc)
>> +{
>> +	guard(xe_pm_runtime_noresume)(guc_to_xe(guc));
>> +	return xe_guc_pc_action_unset_param(&guc->pc, SLPC_PARAM_PWRGATE_RC_MODE);
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_guc_rc.h b/drivers/gpu/drm/xe/xe_guc_rc.h
>> index 35fabb82cb0e..36d41329dd0a 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_rc.h
>> +++ b/drivers/gpu/drm/xe/xe_guc_rc.h
>> @@ -7,9 +7,12 @@
>>   #define _XE_GUC_RC_H_
>>   
>>   struct xe_guc;
>> +enum slpc_gucrc_mode;
>>   
>>   int xe_guc_rc_init(struct xe_guc *guc);
>>   void xe_guc_rc_disable(struct xe_guc *guc);
>>   int xe_guc_rc_enable(struct xe_guc *guc);
>> +int xe_guc_rc_set_mode(struct xe_guc *guc, enum slpc_gucrc_mode mode);
>> +int xe_guc_rc_unset_mode(struct xe_guc *guc);
>>   
>>   #endif
>> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
>> index abf87fe0b345..f7752719b74f 100644
>> --- a/drivers/gpu/drm/xe/xe_oa.c
>> +++ b/drivers/gpu/drm/xe/xe_oa.c
>> @@ -29,7 +29,7 @@
>>   #include "xe_gt.h"
>>   #include "xe_gt_mcr.h"
>>   #include "xe_gt_printk.h"
>> -#include "xe_guc_pc.h"
>> +#include "xe_guc_rc.h"
>>   #include "xe_macros.h"
>>   #include "xe_mmio.h"
>>   #include "xe_oa.h"
>> @@ -875,7 +875,7 @@ static void xe_oa_stream_destroy(struct xe_oa_stream *stream)
>>   
>>   	/* Wa_1509372804:pvc: Unset the override of GUCRC mode to enable rc6 */
>>   	if (stream->override_gucrc)
>> -		xe_gt_WARN_ON(gt, xe_guc_pc_unset_gucrc_mode(&gt->uc.guc.pc));
>> +		xe_gt_WARN_ON(gt, xe_guc_rc_unset_mode(&gt->uc.guc));
>>   
>>   	xe_oa_free_configs(stream);
>>   	xe_file_put(stream->xef);
>> @@ -1765,8 +1765,7 @@ static int xe_oa_stream_init(struct xe_oa_stream *stream,
>>   	 * state. Prevent this by overriding GUCRC mode.
>>   	 */
>>   	if (XE_GT_WA(stream->gt, 1509372804)) {
>> -		ret = xe_guc_pc_override_gucrc_mode(&gt->uc.guc.pc,
>> -						    SLPC_GUCRC_MODE_GUCRC_NO_RC6);
>> +		ret = xe_guc_rc_set_mode(&gt->uc.guc, SLPC_GUCRC_MODE_GUCRC_NO_RC6);
>>   		if (ret)
>>   			goto err_free_configs;
>>   
>> @@ -1824,7 +1823,7 @@ static int xe_oa_stream_init(struct xe_oa_stream *stream,
>>   	xe_force_wake_put(gt_to_fw(gt), stream->fw_ref);
>>   	xe_pm_runtime_put(stream->oa->xe);
>>   	if (stream->override_gucrc)
>> -		xe_gt_WARN_ON(gt, xe_guc_pc_unset_gucrc_mode(&gt->uc.guc.pc));
>> +		xe_gt_WARN_ON(gt, xe_guc_rc_unset_mode(&gt->uc.guc));
> nit: it looks that all callers of the unset_mode() always use only WARN,
> there is true error handling, so maybe this WARN should be moved to unset_mode()
Will modify this in a later patch.
>
> nit2: is there any solid reason why these 'unset' in OA are called _after_ RPM and FW are put?
We don't need fwake anyways for H2G messages, but might be a good idea 
to call unset before the runtime put. Will address this separately.
>
>>   err_free_configs:
>>   	xe_oa_free_configs(stream);
>>   exit:
> just nits, so
>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

Thanks,

Vinay.

>