Re: [Intel-xe] [PATCH 2/3] drm/xe: Create a xe_gt_freq component for raw management and sysfs

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

On 12/5/2023 1:36 PM, Rodrigo Vivi wrote:
> Goals of this new xe_gt_freq component:
> 1. Detach sysfs controls and raw freq management from GuC SLPC.
> 2. Create a directory that could later be aligned with devfreq.
> 3. Encapsulate all the freq control in a single directory. Although
>     we only have one freq domain per GT, already start with a numbered
>     freq0 directory so it could be expanded in the future if multiple
>     domains or PLL are needed.
>
> Note: Although in the goal #1, the raw freq management control is
> mentioned, this patch only starts by the sysfs control. The RP freq
> configuration and init freq selection are still under the guc_pc, but
> should be moved to this component in a follow-up patch.
>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> Cc: Riana Tauro <riana.tauro@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>   drivers/gpu/drm/xe/Makefile      |   1 +
>   drivers/gpu/drm/xe/xe_gt.c       |   3 +
>   drivers/gpu/drm/xe/xe_gt_freq.c  | 217 +++++++++++++++++++++++++++++++
>   drivers/gpu/drm/xe/xe_gt_freq.h  |  13 ++
>   drivers/gpu/drm/xe/xe_gt_types.h |   3 +
>   drivers/gpu/drm/xe/xe_guc_pc.c   | 197 ++++++++++++++--------------
>   drivers/gpu/drm/xe/xe_guc_pc.h   |  10 ++
>   7 files changed, 344 insertions(+), 100 deletions(-)
>   create mode 100644 drivers/gpu/drm/xe/xe_gt_freq.c
>   create mode 100644 drivers/gpu/drm/xe/xe_gt_freq.h
>
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 87f3fca0c0ee..3bca43cdbe3d 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -72,6 +72,7 @@ xe-y += xe_bb.o \
>   	xe_gt.o \
>   	xe_gt_clock.o \
>   	xe_gt_debugfs.o \
> +	xe_gt_freq.o \
>   	xe_gt_idle.o \
>   	xe_gt_mcr.o \
>   	xe_gt_pagefault.o \
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index a9c71da985d3..38a1e9e80e53 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -23,6 +23,7 @@
>   #include "xe_ggtt.h"
>   #include "xe_gsc.h"
>   #include "xe_gt_clock.h"
> +#include "xe_gt_freq.h"
>   #include "xe_gt_idle.h"
>   #include "xe_gt_mcr.h"
>   #include "xe_gt_pagefault.h"
> @@ -494,6 +495,8 @@ int xe_gt_init(struct xe_gt *gt)
>   	if (err)
>   		return err;
>   
> +	xe_gt_freq_init(gt);
> +
>   	xe_force_wake_init_engines(gt, gt_to_fw(gt));
>   
>   	err = all_fw_domain_init(gt);
> diff --git a/drivers/gpu/drm/xe/xe_gt_freq.c b/drivers/gpu/drm/xe/xe_gt_freq.c
> new file mode 100644
> index 000000000000..769d59441988
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_gt_freq.c
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#include "xe_gt_freq.h"
> +
> +#include <linux/kobject.h>
> +#include <linux/sysfs.h>
> +
> +#include <drm/drm_managed.h>
> +#include <drm/drm_print.h>
> +
> +#include "xe_device_types.h"
> +#include "xe_gt_sysfs.h"
> +#include "xe_guc_pc.h"
> +
> +/**
> + * DOC: Xe GT Frequency Management
> + *
> + * This component is responsible for the raw GT frequency management, including
> + * the sysfs API.
> + *
> + * Underneath, Xe enables GuC SLPC automated frequency management. GuC is then
> + * allowed to request PCODE any frequency between the Minimum and the Maximum
> + * selected by this component. Furthermore, it is important to highlight that
> + * PCODE is the ultimate decision maker of the actual running frequency, based
> + * on thermal and other running conditions.
> + *
> + * Xe's Freq provides a sysfs API for frequency management:
> + *
> + * device/gt#/freq0/<item>_freq *read-only* files:
> + * - act_freq: The actual resolved frequency decided by PCODE.
> + * - cur_freq: The current one requested by GuC PC to the PCODE.
> + * - rpn_freq: The Render Performance (RP) N level, which is the minimal one.
> + * - rpe_freq: The Render Performance (RP) E level, which is the efficient one.
> + * - rp0_freq: The Render Performance (RP) 0 level, which is the maximum one.
> + *
> + * device/gt#/freq0/<item>_freq *read-write* files:
> + * - min_freq: Min frequency request.
> + * - max_freq: Max frequency request.
> + *             If max <= min, then freq_min becomes a fixed frequency request.
> + */
> +
> +static struct xe_guc_pc *
> +dev_to_pc(struct device *dev)
> +{
> +	return &kobj_to_gt(dev->kobj.parent)->uc.guc.pc;
> +}
> +
> +static ssize_t act_freq_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct xe_guc_pc *pc = dev_to_pc(dev);
> +
> +	return sysfs_emit(buf, "%d\n", xe_guc_pc_get_act_freq(pc));
> +}
> +static DEVICE_ATTR_RO(act_freq);
> +
> +static ssize_t cur_freq_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct xe_guc_pc *pc = dev_to_pc(dev);
> +	u32 freq;
> +	ssize_t ret;
> +
> +	ret = xe_guc_pc_get_cur_freq(pc, &freq);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%d\n", freq);
> +}
> +static DEVICE_ATTR_RO(cur_freq);
> +
> +static ssize_t rp0_freq_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct xe_guc_pc *pc = dev_to_pc(dev);
> +
> +	return sysfs_emit(buf, "%d\n", xe_guc_pc_get_rp0_freq(pc));
> +}
> +static DEVICE_ATTR_RO(rp0_freq);
> +
> +static ssize_t rpe_freq_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct xe_guc_pc *pc = dev_to_pc(dev);
> +
> +	return sysfs_emit(buf, "%d\n", xe_guc_pc_get_rpe_freq(pc));
> +}
> +static DEVICE_ATTR_RO(rpe_freq);
> +
> +static ssize_t rpn_freq_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct xe_guc_pc *pc = dev_to_pc(dev);
> +
> +	return sysfs_emit(buf, "%d\n", xe_guc_pc_get_rpn_freq(pc));
> +}
> +static DEVICE_ATTR_RO(rpn_freq);
> +
> +static ssize_t min_freq_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct xe_guc_pc *pc = dev_to_pc(dev);
> +	u32 freq;
> +	ssize_t ret;
> +
> +	ret = xe_guc_pc_get_min_freq(pc, &freq);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%d\n", freq);
> +}
> +
> +static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buff, size_t count)
> +{
> +	struct xe_guc_pc *pc = dev_to_pc(dev);
> +	u32 freq;
> +	ssize_t ret;
> +
> +	ret = kstrtou32(buff, 0, &freq);
> +	if (ret)
> +		return ret;
> +
> +	ret = xe_guc_pc_set_min_freq(pc, freq);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(min_freq);
> +
> +static ssize_t max_freq_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct xe_guc_pc *pc = dev_to_pc(dev);
> +	u32 freq;
> +	ssize_t ret;
> +
> +	ret = xe_guc_pc_get_max_freq(pc, &freq);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%d\n", freq);
> +}
> +
> +static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buff, size_t count)
> +{
> +	struct xe_guc_pc *pc = dev_to_pc(dev);
> +	u32 freq;
> +	ssize_t ret;
> +
> +	ret = kstrtou32(buff, 0, &freq);
> +	if (ret)
> +		return ret;
> +
> +	ret = xe_guc_pc_set_max_freq(pc, freq);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(max_freq);
> +
> +static const struct attribute *freq_attrs[] = {
> +	&dev_attr_act_freq.attr,
> +	&dev_attr_cur_freq.attr,
> +	&dev_attr_rp0_freq.attr,
> +	&dev_attr_rpe_freq.attr,
> +	&dev_attr_rpn_freq.attr,
> +	&dev_attr_min_freq.attr,
> +	&dev_attr_max_freq.attr,
> +	NULL
> +};
> +
> +static void freq_fini(struct drm_device *drm, void *arg)
> +{
> +	struct kobject *kobj = arg;
> +
> +	sysfs_remove_files(kobj, freq_attrs);
> +	kobject_put(kobj);
> +}
> +
> +/**
> + * xe_gt_freq_init - Initialize Xe Freq component
> + * @gt: Xe GT object
> + *
> + * It needs to be initialized after GT Sysfs and GuC PC components are ready.
> + */
> +void xe_gt_freq_init(struct xe_gt *gt)
> +{
> +	struct xe_device *xe = gt_to_xe(gt);
> +	int err;
> +
> +	gt->freq = kobject_create_and_add("freq0", gt->sysfs);
> +	if (!gt->freq) {
> +		drm_warn(&xe->drm, "failed to add freq0 directory to %s, err: %d\n",
> +			 kobject_name(gt->sysfs), err);
> +		return;
> +	}
> +
> +	err = drmm_add_action_or_reset(&xe->drm, freq_fini, gt->freq);
> +	if (err) {
> +		drm_warn(&xe->drm, "%s: drmm_add_action_or_reset failed, err: %d\n",
> +			 __func__, err);
> +		kobject_put(gt->freq);
> +		return;
> +	}
> +
> +	err = sysfs_create_files(gt->freq, freq_attrs);
> +	if (err)
> +		drm_warn(&xe->drm,  "failed to add freq attrs to %s, err: %d\n",
> +			 kobject_name(gt->freq), err);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_gt_freq.h b/drivers/gpu/drm/xe/xe_gt_freq.h
> new file mode 100644
> index 000000000000..f3fe3c90491a
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_gt_freq.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef _XE_GT_FREQ_H_
> +#define _XE_GT_FREQ_H_
> +
> +struct xe_gt;
> +
> +void xe_gt_freq_init(struct xe_gt *gt);
> +
> +#endif
> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
> index a7263738308e..4d24d0e78e6b 100644
> --- a/drivers/gpu/drm/xe/xe_gt_types.h
> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
> @@ -299,6 +299,9 @@ struct xe_gt {
>   	/** @sysfs: sysfs' kobj used by xe_gt_sysfs */
>   	struct kobject *sysfs;
>   
> +	/** @freq: Main GT freq sysfs control */
> +	struct kobject *freq;
> +
>   	/** @mocs: info */
>   	struct {
>   		/** @uc_index: UC index */
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index b1876fbea669..2bdabbab2d7a 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> @@ -57,19 +57,6 @@
>    *
>    * Xe driver enables SLPC with all of its defaults features and frequency
>    * selection, which varies per platform.
> - * Xe's GuC PC provides a sysfs API for frequency management:
> - *
> - * device/gt#/freq_* *read-only* files:
> - * - act_freq: The actual resolved frequency decided by PCODE.
> - * - cur_freq: The current one requested by GuC PC to the Hardware.
> - * - rpn_freq: The Render Performance (RP) N level, which is the minimal one.
> - * - rpe_freq: The Render Performance (RP) E level, which is the efficient one.
> - * - rp0_freq: The Render Performance (RP) 0 level, which is the maximum one.
> - *
> - * device/gt#/freq_* *read-write* files:
> - * - min_freq: GuC PC min request.
> - * - max_freq: GuC PC max request.
> - *             If max <= min, then freq_min becomes a fixed frequency request.
>    *
>    * Render-C States:
>    * ================
> @@ -100,12 +87,6 @@ pc_to_gt(struct xe_guc_pc *pc)
>   	return container_of(pc, struct xe_gt, uc.guc.pc);
>   }
>   
> -static struct xe_guc_pc *
> -dev_to_pc(struct device *dev)
> -{
> -	return &kobj_to_gt(&dev->kobj)->uc.guc.pc;
> -}
> -
>   static struct iosys_map *
>   pc_to_maps(struct xe_guc_pc *pc)
>   {
> @@ -388,14 +369,17 @@ static void pc_update_rp_values(struct xe_guc_pc *pc)
>   	pc->rpn_freq = min(pc->rpn_freq, pc->rpe_freq);
>   }
>   
> -static ssize_t act_freq_show(struct device *dev,
> -			     struct device_attribute *attr, char *buf)
> +/**
> + * xe_guc_pc_get_act_freq - Get Actual running frequency
> + * @pc: The GuC PC
> + *
> + * Returns: The Actual running frequency. Which might be 0 if GT is in Render-C sleep state (RC6).
> + */
> +u32 xe_guc_pc_get_act_freq(struct xe_guc_pc *pc)
>   {
> -	struct kobject *kobj = &dev->kobj;
> -	struct xe_gt *gt = kobj_to_gt(kobj);
> +	struct xe_gt *gt = pc_to_gt(pc);
>   	struct xe_device *xe = gt_to_xe(gt);
>   	u32 freq;
> -	ssize_t ret;
>   
>   	xe_device_mem_access_get(gt_to_xe(gt));
>   
> @@ -408,20 +392,25 @@ static ssize_t act_freq_show(struct device *dev,
>   		freq = REG_FIELD_GET(CAGF_MASK, freq);
>   	}
>   
> -	ret = sysfs_emit(buf, "%d\n", decode_freq(freq));
> +	freq = decode_freq(freq);
>   
>   	xe_device_mem_access_put(gt_to_xe(gt));
> -	return ret;
> +
> +	return freq;
>   }
> -static DEVICE_ATTR_RO(act_freq);
>   
> -static ssize_t cur_freq_show(struct device *dev,
> -			     struct device_attribute *attr, char *buf)
> +/**
> + * xe_guc_pc_get_cur_freq - Get Current requested frequency
> + * @pc: The GuC PC
> + * @freq: A pointer to a u32 where the freq value will be returned
> + *
> + * Returns: 0 on success,
> + *         -EAGAIN if GuC PC not ready (likely in middle of a reset).
> + */
> +int xe_guc_pc_get_cur_freq(struct xe_guc_pc *pc, u32 *freq)
>   {
> -	struct kobject *kobj = &dev->kobj;
> -	struct xe_gt *gt = kobj_to_gt(kobj);
> -	u32 freq;
> -	ssize_t ret;
> +	struct xe_gt *gt = pc_to_gt(pc);
> +	int ret;
>   
>   	xe_device_mem_access_get(gt_to_xe(gt));
>   	/*
> @@ -432,54 +421,67 @@ static ssize_t cur_freq_show(struct device *dev,
>   	if (ret)
>   		goto out;
>   
> -	freq = xe_mmio_read32(gt, RPNSWREQ);
> +	*freq = xe_mmio_read32(gt, RPNSWREQ);
>   
> -	freq = REG_FIELD_GET(REQ_RATIO_MASK, freq);
> -	ret = sysfs_emit(buf, "%d\n", decode_freq(freq));
> +	*freq = REG_FIELD_GET(REQ_RATIO_MASK, *freq);
> +	*freq = decode_freq(*freq);
>   
>   	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL));
>   out:
>   	xe_device_mem_access_put(gt_to_xe(gt));
>   	return ret;
>   }
> -static DEVICE_ATTR_RO(cur_freq);
>   
> -static ssize_t rp0_freq_show(struct device *dev,
> -			     struct device_attribute *attr, char *buf)
> +/**
> + * xe_guc_pc_get_rp0_freq - Get the RP0 freq
> + * @pc: The GuC PC
> + *
> + * Returns: RP0 freq.
> + */
> +u32 xe_guc_pc_get_rp0_freq(struct xe_guc_pc *pc)
>   {
> -	struct xe_guc_pc *pc = dev_to_pc(dev);
> -
> -	return sysfs_emit(buf, "%d\n", pc->rp0_freq);
> +	return pc->rp0_freq;
>   }
> -static DEVICE_ATTR_RO(rp0_freq);
>   
> -static ssize_t rpe_freq_show(struct device *dev,
> -			     struct device_attribute *attr, char *buf)
> +/**
> + * xe_guc_pc_get_rpe_freq - Get the RPe freq
> + * @pc: The GuC PC
> + *
> + * Returns: RPe freq.
> + */
> +u32 xe_guc_pc_get_rpe_freq(struct xe_guc_pc *pc)
>   {
> -	struct xe_guc_pc *pc = dev_to_pc(dev);
>   	struct xe_gt *gt = pc_to_gt(pc);
>   	struct xe_device *xe = gt_to_xe(gt);
>   
>   	xe_device_mem_access_get(xe);
>   	pc_update_rp_values(pc);
>   	xe_device_mem_access_put(xe);
> -	return sysfs_emit(buf, "%d\n", pc->rpe_freq);
> +
> +	return pc->rpe_freq;
>   }
> -static DEVICE_ATTR_RO(rpe_freq);
>   
> -static ssize_t rpn_freq_show(struct device *dev,
> -			     struct device_attribute *attr, char *buf)
> +/**
> + * xe_guc_pc_get_rpn_freq - Get the RPn freq
> + * @pc: The GuC PC
> + *
> + * Returns: RPn freq.
> + */
> +u32 xe_guc_pc_get_rpn_freq(struct xe_guc_pc *pc)
>   {
> -	struct xe_guc_pc *pc = dev_to_pc(dev);
> -
> -	return sysfs_emit(buf, "%d\n", pc->rpn_freq);
> +	return pc->rpn_freq;
>   }
> -static DEVICE_ATTR_RO(rpn_freq);
>   
> -static ssize_t min_freq_show(struct device *dev,
> -			     struct device_attribute *attr, char *buf)
> +/**
> + * xe_guc_pc_get_min_freq - Get the min operational frequency
> + * @pc: The GuC PC
> + * @freq: A pointer to a u32 where the freq value will be returned
> + *
> + * Returns: 0 on success,
> + *         -EAGAIN if GuC PC not ready (likely in middle of a reset).
> + */
> +int xe_guc_pc_get_min_freq(struct xe_guc_pc *pc, u32 *freq)
>   {
> -	struct xe_guc_pc *pc = dev_to_pc(dev);
>   	struct xe_gt *gt = pc_to_gt(pc);
>   	ssize_t ret;
>   
> @@ -503,7 +505,7 @@ static ssize_t min_freq_show(struct device *dev,
>   	if (ret)
>   		goto fw;
>   
> -	ret = sysfs_emit(buf, "%d\n", pc_get_min_freq(pc));
> +	*freq = pc_get_min_freq(pc);
>   
>   fw:
>   	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL));
> @@ -513,17 +515,19 @@ static ssize_t min_freq_show(struct device *dev,
>   	return ret;
>   }
>   
> -static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
> -			      const char *buff, size_t count)
> +/**
> + * xe_guc_pc_set_min_freq - Set the minimal operational frequency
> + * @pc: The GuC PC
> + * @freq: The selected minimal frequency
> + *
> + * Returns: 0 on success,
> + *         -EAGAIN if GuC PC not ready (likely in middle of a reset),
> + *         -EINVAL if value out of bounds.
> + */
> +int xe_guc_pc_set_min_freq(struct xe_guc_pc *pc, u32 freq)
>   {
> -	struct xe_guc_pc *pc = dev_to_pc(dev);
> -	u32 freq;
>   	ssize_t ret;
int ret;
>   
> -	ret = kstrtou32(buff, 0, &freq);
> -	if (ret)
> -		return ret;
> -
>   	xe_device_mem_access_get(pc_to_xe(pc));
>   	mutex_lock(&pc->freq_lock);
>   	if (!pc->freq_ready) {
> @@ -541,14 +545,20 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>   out:
>   	mutex_unlock(&pc->freq_lock);
>   	xe_device_mem_access_put(pc_to_xe(pc));
> -	return ret ?: count;
> +
> +	return ret;
>   }
> -static DEVICE_ATTR_RW(min_freq);
>   
> -static ssize_t max_freq_show(struct device *dev,
> -			     struct device_attribute *attr, char *buf)
> +/**
> + * xe_guc_pc_get_max_freq - Get Maximum operational frequency
> + * @pc: The GuC PC
> + * @freq: A pointer to a u32 where the freq value will be returned
> + *
> + * Returns: 0 on success,
> + *         -EAGAIN if GuC PC not ready (likely in middle of a reset).
> + */
> +int xe_guc_pc_get_max_freq(struct xe_guc_pc *pc, u32 *freq)
>   {
> -	struct xe_guc_pc *pc = dev_to_pc(dev);
>   	ssize_t ret;
ret can be int now
>   
>   	xe_device_mem_access_get(pc_to_xe(pc));
> @@ -563,7 +573,7 @@ static ssize_t max_freq_show(struct device *dev,
>   	if (ret)
>   		goto out;
>   
> -	ret = sysfs_emit(buf, "%d\n", pc_get_max_freq(pc));
> +	*freq = pc_get_max_freq(pc);
>   
>   out:
>   	mutex_unlock(&pc->freq_lock);
> @@ -571,17 +581,19 @@ static ssize_t max_freq_show(struct device *dev,
>   	return ret;
>   }
>   
> -static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
> -			      const char *buff, size_t count)
> +/**
> + * xe_guc_pc_set_max_freq - Set the maximum operational frequency
> + * @pc: The GuC PC
> + * @freq: The selected maximum frequency value
> + *
> + * Returns: 0 on success,
> + *         -EAGAIN if GuC PC not ready (likely in middle of a reset),
> + *         -EINVAL if value out of bounds.
> + */
> +int xe_guc_pc_set_max_freq(struct xe_guc_pc *pc, u32 freq)
>   {
> -	struct xe_guc_pc *pc = dev_to_pc(dev);
> -	u32 freq;
>   	ssize_t ret;
here as well..
>   
> -	ret = kstrtou32(buff, 0, &freq);
> -	if (ret)
> -		return ret;
> -
>   	xe_device_mem_access_get(pc_to_xe(pc));
>   	mutex_lock(&pc->freq_lock);
>   	if (!pc->freq_ready) {
> @@ -599,9 +611,8 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>   out:
>   	mutex_unlock(&pc->freq_lock);
>   	xe_device_mem_access_put(pc_to_xe(pc));
> -	return ret ?: count;
> +	return ret;
>   }
> -static DEVICE_ATTR_RW(max_freq);
>   
>   /**
>    * xe_guc_pc_c_status - get the current GT C state
> @@ -666,17 +677,6 @@ u64 xe_guc_pc_mc6_residency(struct xe_guc_pc *pc)
>   	return reg;
>   }
>   
> -static const struct attribute *pc_attrs[] = {
> -	&dev_attr_act_freq.attr,
> -	&dev_attr_cur_freq.attr,
> -	&dev_attr_rp0_freq.attr,
> -	&dev_attr_rpe_freq.attr,
> -	&dev_attr_rpn_freq.attr,
> -	&dev_attr_min_freq.attr,
> -	&dev_attr_max_freq.attr,
> -	NULL
> -};
> -
>   static void mtl_init_fused_rp_values(struct xe_guc_pc *pc)
>   {
>   	struct xe_gt *gt = pc_to_gt(pc);
> @@ -952,6 +952,10 @@ int xe_guc_pc_stop(struct xe_guc_pc *pc)
>   	return ret;
>   }
>   
> +/**
> + * xe_guc_pc_fini - Finalize GuC's Power Conservation component
> + * @pc: Xe_GuC_PC instance
> + */
>   void xe_guc_pc_fini(struct xe_guc_pc *pc)
>   {
>   	struct xe_device *xe = pc_to_xe(pc);
> @@ -963,7 +967,6 @@ void xe_guc_pc_fini(struct xe_guc_pc *pc)
>   
>   	XE_WARN_ON(xe_guc_pc_gucrc_disable(pc));
>   	XE_WARN_ON(xe_guc_pc_stop(pc));
> -	sysfs_remove_files(pc_to_gt(pc)->sysfs, pc_attrs);
>   	mutex_destroy(&pc->freq_lock);
>   }
>   
> @@ -978,7 +981,6 @@ int xe_guc_pc_init(struct xe_guc_pc *pc)
>   	struct xe_device *xe = gt_to_xe(gt);
>   	struct xe_bo *bo;
>   	u32 size = PAGE_ALIGN(sizeof(struct slpc_shared_data));
> -	int err;
>   
>   	if (xe->info.skip_guc_pc)
>   		return 0;
> @@ -992,10 +994,5 @@ int xe_guc_pc_init(struct xe_guc_pc *pc)
>   		return PTR_ERR(bo);
>   
>   	pc->bo = bo;
> -
> -	err = sysfs_create_files(gt->sysfs, pc_attrs);
> -	if (err)
> -		return err;
> -
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.h b/drivers/gpu/drm/xe/xe_guc_pc.h
> index 054788e006f3..cecad8e9300b 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.h
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.h
> @@ -14,6 +14,16 @@ 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);
>   
> +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);
> +u32 xe_guc_pc_get_rp0_freq(struct xe_guc_pc *pc);
> +u32 xe_guc_pc_get_rpe_freq(struct xe_guc_pc *pc);
> +u32 xe_guc_pc_get_rpn_freq(struct xe_guc_pc *pc);
> +int xe_guc_pc_get_min_freq(struct xe_guc_pc *pc, u32 *freq);
> +int xe_guc_pc_set_min_freq(struct xe_guc_pc *pc, u32 freq);
> +int xe_guc_pc_get_max_freq(struct xe_guc_pc *pc, u32 *freq);
> +int xe_guc_pc_set_max_freq(struct xe_guc_pc *pc, u32 freq);
> +

With the minor fixes above,

Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

>   enum xe_gt_idle_state xe_guc_pc_c_status(struct xe_guc_pc *pc);
>   u64 xe_guc_pc_rc6_residency(struct xe_guc_pc *pc);
>   u64 xe_guc_pc_mc6_residency(struct xe_guc_pc *pc);