Re: [PATCH 1/2] drm/xe: Decouple GuC RC code from xe_guc_pc

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

On 12/5/2025 9:50 PM, Michal Wajdeczko wrote:
>
> On 12/5/2025 3:38 PM, Vinay Belgaumkar wrote:
>> Move enable/disable GuC RC logic into the new file. This will
>> allow us to independently enable/disable GuC RC and not rely
>> on SLPC related functions. GuC already provides separate H2G
>> interfaces to setup GuC RC and SLPC.
>>
>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>> ---
>>   drivers/gpu/drm/xe/Makefile    |   1 +
>>   drivers/gpu/drm/xe/xe_guc.c    |   5 ++
>>   drivers/gpu/drm/xe/xe_guc_pc.c |  50 -------------
>>   drivers/gpu/drm/xe/xe_guc_pc.h |   1 -
>>   drivers/gpu/drm/xe/xe_guc_rc.c | 128 +++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_guc_rc.h |  18 +++++
>>   drivers/gpu/drm/xe/xe_uc.c     |   7 +-
>>   7 files changed, 158 insertions(+), 52 deletions(-)
>>   create mode 100644 drivers/gpu/drm/xe/xe_guc_rc.c
>>   create mode 100644 drivers/gpu/drm/xe/xe_guc_rc.h
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index a7e13a676f7d..d1258bd22752 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -74,6 +74,7 @@ xe-y += xe_bb.o \
>>   	xe_guc_log.o \
>>   	xe_guc_pagefault.o \
>>   	xe_guc_pc.o \
>> +	xe_guc_rc.o \
>>   	xe_guc_submit.o \
>>   	xe_guc_tlb_inval.o \
>>   	xe_heci_gsc.o \
>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>> index 88376bc2a483..72740d17c769 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>> @@ -35,6 +35,7 @@
>>   #include "xe_guc_klv_helpers.h"
>>   #include "xe_guc_log.h"
>>   #include "xe_guc_pc.h"
>> +#include "xe_guc_rc.h"
>>   #include "xe_guc_relay.h"
>>   #include "xe_guc_submit.h"
>>   #include "xe_memirq.h"
>> @@ -865,6 +866,10 @@ int xe_guc_init_post_hwconfig(struct xe_guc *guc)
>>   	if (ret)
>>   		return ret;
>>   
>> +	ret = xe_guc_rc_init(guc_to_gt(guc));
>> +	if (ret)
>> +		return ret;
>> +
>>   	ret = xe_guc_engine_activity_init(guc);
>>   	if (ret)
>>   		return ret;
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
>> index e2e6edb851ae..5ced9ef37cf3 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>> @@ -253,22 +253,6 @@ static int pc_action_unset_param(struct xe_guc_pc *pc, u8 id)
>>   	return ret;
>>   }
>>   
>> -static int pc_action_setup_gucrc(struct xe_guc_pc *pc, u32 mode)
>> -{
>> -	struct xe_guc_ct *ct = pc_to_ct(pc);
>> -	u32 action[] = {
>> -		GUC_ACTION_HOST2GUC_SETUP_PC_GUCRC,
>> -		mode,
>> -	};
>> -	int ret;
>> -
>> -	ret = xe_guc_ct_send(ct, action, ARRAY_SIZE(action), 0, 0);
>> -	if (ret && !(xe_device_wedged(pc_to_xe(pc)) && ret == -ECANCELED))
>> -		xe_gt_err(pc_to_gt(pc), "GuC RC enable mode=%u failed: %pe\n",
>> -			  mode, ERR_PTR(ret));
>> -	return ret;
>> -}
>> -
>>   static u32 decode_freq(u32 raw)
>>   {
>>   	return DIV_ROUND_CLOSEST(raw * GT_FREQUENCY_MULTIPLIER,
>> @@ -1050,30 +1034,6 @@ int xe_guc_pc_restore_stashed_freq(struct xe_guc_pc *pc)
>>   	return ret;
>>   }
>>   
>> -/**
>> - * xe_guc_pc_gucrc_disable - Disable GuC RC
>> - * @pc: Xe_GuC_PC instance
>> - *
>> - * Disables GuC RC by taking control of RC6 back from GuC.
>> - *
>> - * Return: 0 on success, negative error code on error.
>> - */
>> -int xe_guc_pc_gucrc_disable(struct xe_guc_pc *pc)
>> -{
>> -	struct xe_device *xe = pc_to_xe(pc);
>> -	struct xe_gt *gt = pc_to_gt(pc);
>> -	int ret = 0;
>> -
>> -	if (xe->info.skip_guc_pc)
>> -		return 0;
>> -
>> -	ret = pc_action_setup_gucrc(pc, GUCRC_HOST_CONTROL);
>> -	if (ret)
>> -		return ret;
>> -
>> -	return xe_gt_idle_disable_c6(gt);
>> -}
>> -
>>   /**
>>    * xe_guc_pc_override_gucrc_mode - override GUCRC mode
>>    * @pc: Xe_GuC_PC instance
>> @@ -1257,15 +1217,6 @@ int xe_guc_pc_start(struct xe_guc_pc *pc)
>>   	if (ret)
>>   		return ret;
>>   
>> -	if (xe->info.platform == XE_PVC) {
>> -		xe_guc_pc_gucrc_disable(pc);
>> -		return 0;
>> -	}
>> -
>> -	ret = pc_action_setup_gucrc(pc, GUCRC_FIRMWARE_CONTROL);
>> -	if (ret)
>> -		return ret;
>> -
>>   	/* Enable SLPC Optimized Strategy for compute */
>>   	ret = pc_action_set_strategy(pc, SLPC_OPTIMIZED_STRATEGY_COMPUTE);
>>   
>> @@ -1310,7 +1261,6 @@ static void xe_guc_pc_fini_hw(void *arg)
>>   		return;
>>   
>>   	CLASS(xe_force_wake, fw_ref)(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL);
>> -	xe_guc_pc_gucrc_disable(pc);
>>   	XE_WARN_ON(xe_guc_pc_stop(pc));
>>   
>>   	/* Bind requested freq to mert_freq_cap before unload */
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.h b/drivers/gpu/drm/xe/xe_guc_pc.h
>> index 0e31396f103c..1b95873b262e 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.h
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.h
>> @@ -15,7 +15,6 @@ 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_gucrc_disable(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);
>> diff --git a/drivers/gpu/drm/xe/xe_guc_rc.c b/drivers/gpu/drm/xe/xe_guc_rc.c
>> new file mode 100644
>> index 000000000000..945433ef35f0
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_guc_rc.c
>> @@ -0,0 +1,128 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#include "xe_guc_rc.h"
>> +
>> +#include <drm/drm_managed.h>
> since you are using dev-managed action, maybe it should be:
>
> #include <linux/device/devres.h>
ok.
>
>> +#include <drm/drm_print.h>
>> +
>> +#include "abi/guc_actions_slpc_abi.h"
>> +#include "xe_device.h"
>> +#include "xe_force_wake.h"
>> +#include "xe_gt.h"
>> +#include "xe_gt_idle.h"
>> +#include "xe_gt_printk.h"
>> +#include "xe_guc_ct.h"
>> +#include "xe_pm.h"
>> +
>> +/**
>> + * DOC: GuC RC
>> + * ================
> remove this underline, this is not a RST
ok.
>
>> + *
>> + * GuC handles the GT transition to deeper C-states in conjunction with Pcode.
>> + * GuC RC can be enabled independently of DVFS, which is also controlled
>> + * by GuC.
>> + *
>> + */
>> +
>> +static int guc_action_setup_gucrc(struct xe_gt *gt, u32 control)
> for "guc_action" functions we usually pass "guc" as param, not "gt"
ok, will change to use xe_guc * instead.
>
>> +{
>> +	struct xe_guc_ct *ct = &gt->uc.guc.ct;
>> +	u32 action[] = {
>> +		GUC_ACTION_HOST2GUC_SETUP_PC_GUCRC,
>> +		control,
>> +	};
>> +	int ret;
>> +
>> +	ret = xe_guc_ct_send(ct, action, ARRAY_SIZE(action), 0, 0);
>> +	if (ret && !(xe_device_wedged(gt_to_xe(gt)) && ret == -ECANCELED))
> btw, is this wedged/ECANCELED check a GuC RC specific case or generic?
> if latter maybe we should return dedicated error code from CTB ?
It is the latter. I think we had added this patch a while ago since we 
didn't want to flag an additional error during wedge state. Not sure 
what you mean by dedicated error? Since CTB code returns ECANCELED when 
this happens.
>
>> +		xe_gt_err(gt, "GuC RC enable control=%u failed: %pe\n",
>> +			  control, ERR_PTR(ret));
>> +	return ret;
>> +}
>> +
>> +/**
>> + * xe_guc_rc_disable - Disable GuC RC
> for function doc, we should add () to function name
>
>      * xe_guc_rc_disable() - Disable GuC RC
>
>> + * @gt: Xe GT instance
> hmm, as feature and file have "GuC" in their names, why functions take "gt"?
> they should take "guc" like all other "xe_guc" functions
GuC RC(RC6) is per GT, so felt more intuitive to have it that way 
instead of per guc. But can change to use guc instead since it is 
enabled through guc.
>
>> + *
>> + * Disables GuC RC by taking control of RC6 back from GuC.
>> + *
>> + * Return: 0 on success, negative error code on error.
>> + */
>> +int xe_guc_rc_disable(struct xe_gt *gt)
>> +{
>> +	struct xe_device *xe = gt_to_xe(gt);
>> +	int ret = 0;
>> +
>> +	if (xe->info.skip_guc_pc)
>> +		return 0;
>> +
>> +	ret = guc_action_setup_gucrc(gt, GUCRC_HOST_CONTROL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return xe_gt_idle_disable_c6(gt);
>> +}
>> +
>> +static void xe_guc_rc_fini_hw(void *arg)
>> +{
>> +	struct xe_gt *gt = arg;
>> +	struct xe_device *xe = gt_to_xe(gt);
>> +
>> +	if (xe_device_wedged(xe))
>> +		return;
>> +
>> +	CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FW_GT);
>> +	xe_guc_rc_disable(gt);
>> +}
>> +
>> +/**
>> + * xe_guc_rc_init - Initialize GuC RC
>> + * @gt: Xe GT instance
>> + *
>> + * Initializes GuC RC feature.
>> + *
>> + * Return: 0 on success, negative error code on error.
>> + */
>> +int xe_guc_rc_init(struct xe_gt *gt)
>> +{
>> +	struct xe_device *xe = gt_to_xe(gt);
>> +
>> +	if (xe->info.skip_guc_pc)
>> +		return 0;
>> +
>> +	return devm_add_action_or_reset(xe->drm.dev, xe_guc_rc_fini_hw, gt);
>> +}
>> +
>> +static int xe_guc_rc_enable(struct xe_gt *gt)
>> +{
>> +	return  guc_action_setup_gucrc(gt, GUCRC_FIRMWARE_CONTROL);
>> +}
>> +
> missing kernel-doc
will add.
>
>> +int xe_guc_rc_start(struct xe_gt *gt)
>> +{
>> +	struct xe_device *xe = gt_to_xe(gt);
>> +	int ret;
>> +
>> +	xe_gt_assert(gt, xe_device_uc_enabled(xe));
>> +
>> +	CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FW_GT);
>> +	if (!xe_force_wake_ref_has_domain(fw_ref.domains, XE_FW_GT))
>> +		return -ETIMEDOUT;
>> +
>> +	if (xe->info.skip_guc_pc) {
>> +		if (xe->info.platform != XE_PVC)
>> +			xe_gt_idle_enable_c6(gt);
>> +
>> +		return ret;
>> +	}
>> +
>> +	if (xe->info.platform == XE_PVC) {
>> +		xe_guc_rc_disable(gt);
>> +		return 0;
>> +	}
>> +
>> +	return xe_guc_rc_enable(gt);
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_guc_rc.h b/drivers/gpu/drm/xe/xe_guc_rc.h
>> new file mode 100644
>> index 000000000000..3a8c8d50313f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_guc_rc.h
>> @@ -0,0 +1,18 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_GUC_RC_H_
>> +#define _XE_GUC_RC_H_
>> +
>> +#include <linux/types.h>
>> +
>> +struct xe_gt;
>> +enum slpc_gucrc_mode;
> enum forward not needed here
ok.
>
>> +
>> +int xe_guc_rc_disable(struct xe_gt *gt);
>> +int xe_guc_rc_start(struct xe_gt *gt);
>> +int xe_guc_rc_init(struct xe_gt *gt);
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
>> index 157520ea1783..44e5fabe2a50 100644
>> --- a/drivers/gpu/drm/xe/xe_uc.c
>> +++ b/drivers/gpu/drm/xe/xe_uc.c
>> @@ -14,6 +14,7 @@
>>   #include "xe_gt_sriov_vf.h"
>>   #include "xe_guc.h"
>>   #include "xe_guc_pc.h"
>> +#include "xe_guc_rc.h"
>>   #include "xe_guc_engine_activity.h"
>>   #include "xe_huc.h"
>>   #include "xe_sriov.h"
>> @@ -216,6 +217,10 @@ int xe_uc_load_hw(struct xe_uc *uc)
>>   	if (ret)
>>   		goto err_out;
>>   
>> +	ret = xe_guc_rc_start(uc_to_gt(uc));
>> +	if (ret)
>> +		goto err_out;
>> +
>>   	xe_guc_engine_activity_enable_stats(&uc->guc);
>>   
>>   	/* We don't fail the driver load if HuC fails to auth */
>> @@ -246,7 +251,7 @@ int xe_uc_reset_prepare(struct xe_uc *uc)
>>   
>>   void xe_uc_gucrc_disable(struct xe_uc *uc)
>>   {
>> -	XE_WARN_ON(xe_guc_pc_gucrc_disable(&uc->guc.pc));
>> +	XE_WARN_ON(xe_guc_rc_disable(uc_to_gt(uc)));
> maybe use xe_gt_WARN_ONCE_ON(gt, ...) here

ok.


>
>>   }
>>   
>>   void xe_uc_stop_prepare(struct xe_uc *uc)