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

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

On 1/27/2026 3:32 PM, Michal Wajdeczko wrote:
> subject is too generic, maybe mention GuC/PC/RC ?
>
> 	"drm/xe/guc: Add wrappers for set/unset SLPC params"
ok.
>
>
> On 1/24/2026 12:50 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.
>>
>> 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)
>>
>> Cc: Riana Tauro <riana.tauro@intel.com>
>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>> ---
>>   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 cf02c53ca3c5..31c6cb111916 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.
>> + *
>> + * Returns: 0 on Success
>      * Return: 0 on success or a negative error code on failure.
ok.
>
>> + */
>> +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.
>> + *
>> + * Returns: 0 on Success
> ditto
ok.
>
>> + */
>> +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 fc57e5290635..0a900c309ecf 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_rc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_rc.c
>> @@ -14,6 +14,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"
>>   
>> @@ -112,3 +113,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
>> + * @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)
>> +{
>> +	guard(xe_pm_runtime)(guc_to_xe(guc));
> are we sure we want to do full resume here?
> maybe it should be:
>
> 	guard(xe_pm_runtime_noresume)
>
>> +	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)(guc_to_xe(guc));
> ditto
ok.
>
>> +	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 c980b0fce472..d76dfad8e26c 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_rc.h
>> +++ b/drivers/gpu/drm/xe/xe_guc_rc.h
>> @@ -7,8 +7,11 @@
>>   #define _XE_GUC_RC_H_
>>   
>>   struct xe_guc;
>> +enum slpc_gucrc_mode;
>>   
>>   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));
> as return real error code, not bool, maybe we should also print it?
We already print it in the lower level function pc_action_unset_param().
>
>>   
>>   	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);
> we should just place guc_rc calls when we still are holding rpm

For now, that seems to be true. But if someone introduces a sysfs 
interface later on and forgets to add a runtime_get(), it's better to 
have it in the guc_rc function as well?

Thanks,

Vinay.

>
>>   	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));
>>   err_free_configs:
>>   	xe_oa_free_configs(stream);
>>   exit: