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

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

On 12/18/2025 1:04 PM, Michal Wajdeczko wrote:
>
> On 12/18/2025 2:53 AM, 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.
>>
>> v2: Comments (Michal W), remove duplicate c6_enable calls from
>> xe_guc_pc.
>>
>> v3: Clarify crosss interactions between xe_guc_rc and xe_guc_pc (Michal W)
> nit: change log could be under ---
>
>> 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 |  70 +++--------------
>>   drivers/gpu/drm/xe/xe_guc_pc.h |   1 -
>>   drivers/gpu/drm/xe/xe_guc_rc.c | 138 +++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_guc_rc.h |  20 +++++
>>   drivers/gpu/drm/xe/xe_uc.c     |   8 +-
>>   7 files changed, 182 insertions(+), 61 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 6ecba27d85f7..90f86ad11498 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 f0407bab9a0c..d05380893f39 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);
>> +	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..e7c90b6244e0 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>> @@ -76,7 +76,6 @@
>>    * exposes a programming interface to the host for the control of SLPC.
>>    *
>>    * Frequency management:
>> - * =====================
>>    *
>>    * Xe driver enables SLPC with all of its defaults features and frequency
>>    * selection, which varies per platform.
>> @@ -86,12 +85,20 @@
>>    * thus saving power. Base profile is default and ensures balanced performance
>>    * for any workload.
>>    *
>> - * Render-C States:
>> - * ================
>> + * Render-C States (GuC RC):
>>    *
>>    * Render-C states is also a GuC PC feature that is now enabled in Xe for
>>    * all platforms.
>>    *
>> + * The implementation for GuC Power Management features is split as follows:
>> + *
>> + * xe_guc_rc:  Logic for handling GuC RC
>> + * xe_gt_idle: Host side logic for RC6 and Coarse Power gating (CPG)
>> + * xe_guc_pc:  Logic for all other SLPC related features
>> + *
>> + * There is some cross interaction between these where host C6 will need to be
>> + * enabled when we plan to skip GuC RC. Also, the GuC RC mode is currently
>> + * overridden through 0x3003 which is an SLPC H2G call.
>>    */
>>   
>>   static struct xe_guc *pc_to_guc(struct xe_guc_pc *pc)
>> @@ -253,22 +260,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 +1041,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
>> @@ -1217,9 +1184,6 @@ int xe_guc_pc_start(struct xe_guc_pc *pc)
>>   		return -ETIMEDOUT;
>>   
>>   	if (xe->info.skip_guc_pc) {
>> -		if (xe->info.platform != XE_PVC)
>> -			xe_gt_idle_enable_c6(gt);
>> -
>>   		/* Request max possible since dynamic freq mgmt is not enabled */
>>   		pc_set_cur_freq(pc, UINT_MAX);
>>   		return 0;
>> @@ -1257,15 +1221,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);
>>   
>> @@ -1285,10 +1240,8 @@ int xe_guc_pc_stop(struct xe_guc_pc *pc)
>>   {
>>   	struct xe_device *xe = pc_to_xe(pc);
>>   
>> -	if (xe->info.skip_guc_pc) {
>> -		xe_gt_idle_disable_c6(pc_to_gt(pc));
>> +	if (xe->info.skip_guc_pc)
>>   		return 0;
>> -	}
>>   
>>   	mutex_lock(&pc->freq_lock);
>>   	pc->freq_ready = false;
>> @@ -1310,7 +1263,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..321a071b7932
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_guc_rc.c
>> @@ -0,0 +1,138 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#include <linux/device/devres.h>
>> +#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_guc.h"
>> +#include "xe_gt_idle.h"
>> +#include "xe_gt_printk.h"
>> +#include "xe_guc_ct.h"
>> +#include "xe_guc_rc.h"
>> +#include "xe_pm.h"
>> +
>> +/**
>> + * DOC: GuC RC (Render C-states)
>> + *
>> + * GuC handles the GT transition to deeper C-states in conjunction with Pcode.
>> + * GuC RC can be enabled independently of the frequency component in SLPC, which is
>> + * also controlled by GuC.
>> + *
>> + * This file will contain all H2G related logic for handling Render C-states. There are
>> + * some calls to xe_gt_idle, where we enable host C6 when GuC RC is skipped. GuC RC
>> + * is mostly independent of xe_guc_pc with the exception of functions that override the mode
>> + * for which we have to rely on the SLPC H2G calls.
> nit: I guess we try to fit comments in regular 80 columns, and only let the code go up to 100
ok.
>
>> + */
>> +
>> +static int guc_action_setup_gucrc(struct xe_guc *guc, u32 control)
>> +{
>> +	u32 action[] = {
>> +		GUC_ACTION_HOST2GUC_SETUP_PC_GUCRC,
>> +		control,
>> +	};
>> +	int ret;
>> +
>> +	ret = xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action), 0, 0);
>> +	if (ret && !(xe_device_wedged(guc_to_xe(guc)) && ret == -ECANCELED))
>> +		xe_gt_err(guc_to_gt(guc),
>> +			  "GuC RC setup %s(%u) failed (%pe)\n",
>> +			   control == GUCRC_HOST_CONTROL ? "HOST_CONTROL" :
>> +			   control == GUCRC_FIRMWARE_CONTROL ? "FIRMWARE_CONTROL" :
>> +			   "UNKNOWN", control, ERR_PTR(ret));
>> +	return ret;
>> +}
>> +
>> +/**
>> + * xe_guc_rc_disable() - Disable GuC RC
>> + * @guc: Xe GuC instance
>> + *
>> + * 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_guc *guc)
>> +{
>> +	struct xe_device *xe = guc_to_xe(guc);
>> +	int ret;
>> +
>> +	if (xe->info.skip_guc_pc)
>> +		return 0;
>> +
>> +	ret = guc_action_setup_gucrc(guc, GUCRC_HOST_CONTROL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return xe_gt_idle_disable_c6(guc_to_gt(guc));
>> +}
>> +
>> +static void xe_guc_rc_fini_hw(void *arg)
>> +{
>> +	struct xe_guc *guc = arg;
>> +	struct xe_device *xe = guc_to_xe(guc);
>> +	struct xe_gt *gt = guc_to_gt(guc);
>> +
>> +	if (xe_device_wedged(xe))
>> +		return;
>> +
>> +	CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FW_GT);
>> +	xe_guc_rc_disable(guc);
>> +}
>> +
>> +/**
>> + * xe_guc_rc_init() - Initialize GuC RC
>> + * @guc: Xe GuC instance
>> + *
>> + * Initializes GuC RC feature.
>> + *
>> + * Return: 0 on success, negative error code on error.
>> + */
>> +int xe_guc_rc_init(struct xe_guc *guc)
>> +{
>> +	struct xe_device *xe = guc_to_xe(guc);
>> +
> should we care about xe_device_uc_enabled() like we have below?
will add.
>
>> +	if (xe->info.skip_guc_pc)
>> +		return 0;
>> +
>> +	return devm_add_action_or_reset(xe->drm.dev, xe_guc_rc_fini_hw, guc);
>> +}
>> +
>> +static int xe_guc_rc_enable(struct xe_guc *guc)
>> +{
>> +	return guc_action_setup_gucrc(guc, GUCRC_FIRMWARE_CONTROL);
>> +}
>> +
>> +/**
>> + * xe_guc_rc_start() - Enable GuC RC feature if applicable
>> + * @guc: Xe GuC instance
>> + *
>> + * Enables GuC RC feature.
>> + *
>> + * Return: 0 on success, negative error code on error.
>> + */
>> +int xe_guc_rc_start(struct xe_guc *guc)
>> +{
>> +	struct xe_device *xe = guc_to_xe(guc);
>> +	struct xe_gt *gt = guc_to_gt(guc);
>> +
>> +	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.platform == XE_PVC)
>> +		return xe_guc_rc_disable(guc);
>> +
>> +	if (xe->info.skip_guc_pc) {
>> +		xe_gt_idle_enable_c6(gt);
>> +		return 0;
>> +	}
>> +
>> +	return xe_guc_rc_enable(guc);
>> +}
>> 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..2500b0d1c151
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_guc_rc.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_GUC_RC_H_
>> +#define _XE_GUC_RC_H_
>> +
>> +#include <linux/types.h>
> still not needed
>
> did you miss [1] ?
>
> [1] https://patchwork.freedesktop.org/patch/694838/?series=159128&rev=1#comment_1276769
yes, will add.
>
>> +
>> +#include "xe_guc.h"
> still not needed
>
>> +
>> +struct xe_gt;
> still not needed
removed.
>
>> +struct xe_guc;
>> +
>> +int xe_guc_rc_disable(struct xe_guc *guc);
>> +int xe_guc_rc_start(struct xe_guc *guc);
>> +int xe_guc_rc_init(struct xe_guc *guc);
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
>> index 157520ea1783..91eaddad331e 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->guc);
>> +	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,8 @@ int xe_uc_reset_prepare(struct xe_uc *uc)
>>   
>>   void xe_uc_gucrc_disable(struct xe_uc *uc)
> again, do we really need this as separate uc call?
This was added in a previous patch, not needed now.
>
>>   {
>> -	XE_WARN_ON(xe_guc_pc_gucrc_disable(&uc->guc.pc));
>> +	xe_gt_WARN_ONCE(uc_to_gt(uc), xe_guc_rc_disable(&uc->guc) != 0,
>> +			"Unable to disable GuC RC for gt: %d", uc_to_gt(uc)->info.id);
> again, can't we have this in xe_guc_rc_disable?

will check there.

Thanks,

Vinay.

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