[Intel-xe] [RFC PATCH] drm/xe: add a new sysfs directory for gtidle properties
Riana Tauro
riana.tauro at intel.com
Tue May 23 07:13:49 UTC 2023
Hi Michal
On 5/20/2023 3:41 PM, Michal Wajdeczko wrote:
>
>
> On 12.05.2023 07:20, Riana Tauro wrote:
>> Add a new sysfs directory under devices/gt#/ called gtidle
>> to contain idle properties of GT such as name, idle_status,
>> idle_residency_ms
>>
>> v2: abstract using function pointers (Anshuman)
>> use device_attr (Badal)
>> move rc functions to guc_pc
>> change name to gt_idle (Rodrigo)
>>
>> Signed-off-by: Riana Tauro <riana.tauro at intel.com>
>> ---
>> drivers/gpu/drm/xe/Makefile | 1 +
>> drivers/gpu/drm/xe/xe_gt.c | 5 +
>> drivers/gpu/drm/xe/xe_gt_idle.c | 134 ++++++++++++++++++++++++++
>> drivers/gpu/drm/xe/xe_gt_idle.h | 18 ++++
>> drivers/gpu/drm/xe/xe_gt_idle_types.h | 39 ++++++++
>> drivers/gpu/drm/xe/xe_gt_types.h | 4 +
>> drivers/gpu/drm/xe/xe_guc_pc.c | 87 +++++++++++------
>> drivers/gpu/drm/xe/xe_guc_pc.h | 5 +
>> 8 files changed, 261 insertions(+), 32 deletions(-)
>> create mode 100644 drivers/gpu/drm/xe/xe_gt_idle.c
>> create mode 100644 drivers/gpu/drm/xe/xe_gt_idle.h
>> create mode 100644 drivers/gpu/drm/xe/xe_gt_idle_types.h
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index 84ad0e949044..93bec17ee0de 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -44,6 +44,7 @@ xe-y += xe_bb.o \
>> xe_gt.o \
>> xe_gt_clock.o \
>> xe_gt_debugfs.o \
>> + xe_gt_idle.o \
>> xe_gt_mcr.o \
>> xe_gt_pagefault.o \
>> xe_gt_sysfs.o \
>> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>> index 0d4664e344da..9e1f7343f256 100644
>> --- a/drivers/gpu/drm/xe/xe_gt.c
>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>> @@ -18,6 +18,7 @@
>> #include "xe_force_wake.h"
>> #include "xe_ggtt.h"
>> #include "xe_gt_clock.h"
>> +#include "xe_gt_idle.h"
>> #include "xe_gt_mcr.h"
>> #include "xe_gt_pagefault.h"
>> #include "xe_gt_sysfs.h"
>> @@ -381,6 +382,10 @@ static int gt_fw_domain_init(struct xe_gt *gt)
>> if (err)
>> goto err_force_wake;
>>
>> + err = xe_gt_idle_init(>->gtidle);
>> + if (err)
>> + goto err_force_wake;
>> +
>> /* XXX: Fake that we pull the engine mask from hwconfig blob */
>> gt->info.engine_mask = gt->info.__engine_mask;
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_idle.c b/drivers/gpu/drm/xe/xe_gt_idle.c
>> new file mode 100644
>> index 000000000000..0060d8584485
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_gt_idle.c
>> @@ -0,0 +1,134 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#include <drm/drm_managed.h>
>> +#include "regs/xe_gt_regs.h"
>> +
>> +#include "xe_device.h"
>> +#include "xe_gt.h"
>> +#include "xe_gt_idle.h"
>> +#include "xe_guc_pc.h"
>> +#include "xe_mmio.h"
>> +
>> +/**
>> + * GT Idle:
>> + * ================
>> + *
>> + * Provides sysfs entries for idle states of GT
>> + *
>> + * device/gt#/gtidle/name - name of the state
>> + * device/gt#/gtidle/idle_residency_ms - Provides residency of the idle state in ms
>> + * device/gt#/gtidle/idle_status - Provides current idle state
>> + */
>> +
>> +static struct xe_gt_idle *kobj_to_gtidle(struct kobject *kobj)
>> +{
>> + return container_of(kobj, struct gtidle_kobj, base)->gtidle;
>> +}
>> +
>> +static ssize_t name_show(struct device *dev,
>> + struct device_attribute *attr, char *buff)
>> +{
>> + struct kobject *kobj = &dev->kobj;
>> + struct xe_gt_idle *gtidle = kobj_to_gtidle(kobj);
>> + ssize_t ret;
>> +
>> + ret = sysfs_emit(buff, gtidle->name);
>
> no need for 'ret' variable, just:
>
> return sysfs_emit(buff, gtidle->name);
>
>> +
>> + return ret;
>> +}
>> +static DEVICE_ATTR_RO(name);
>> +
>> +static ssize_t idle_status_show(struct device *dev,
>> + struct device_attribute *attr, char *buff)
>> +{
>> + struct kobject *kobj = &dev->kobj;
>> + struct xe_gt_idle *gtidle = kobj_to_gtidle(kobj);
>> + ssize_t ret;
>> +
>> + gtidle->idle_status(gtidle);
>> + ret = sysfs_emit(buff, gtidle->state);
>
> sysfs_emit takes "fmt" so this should be rather
>
> return sysfs_emit(buf, "%s\n", idle->state);
>
> but I'm wondering whether it is good idea to keep state as a string,
> maybe it should be done simpler/direct as:
>
>
> state = xe_gt_idle_state(gt);
> return sysfs_emit(buf, "%s\n", gt_idle_state_to_string(state));
>
>
>> +
>> + return ret;
>> +}
>> +static DEVICE_ATTR_RO(idle_status);
>> +
>> +static ssize_t idle_residency_ms_show(struct device *dev,
>> + struct device_attribute *attr, char *buff)
>> +{
>> + struct kobject *kobj = &dev->kobj;
>> + struct xe_gt_idle *gtidle = kobj_to_gtidle(kobj);
>> + ssize_t ret;
>> +
>> + ret = sysfs_emit(buff, "%llu\n", gtidle->residency_ms(gtidle));
>> +
>> + return ret;
>> +}
>> +static DEVICE_ATTR_RO(idle_residency_ms);
>> +
>> +static const struct attribute *gt_idle_attrs[] = {
>> + &dev_attr_name.attr,
>> + &dev_attr_idle_status.attr,
>> + &dev_attr_idle_residency_ms.attr,
>> + NULL,
>> +};
>> +
>> +static void gt_idle_kobj_release(struct kobject *kobj)
>> +{
>> + kfree(kobj);
>> +}
>> +
>> +static struct kobj_type gt_idle_kobj_type = {
>> + .release = gt_idle_kobj_release,
>> + .sysfs_ops = &kobj_sysfs_ops,
>> +};
>> +
>> +static void gt_idle_fini(struct drm_device *drm, void *arg)
>> +{
>> + struct gtidle_kobj *gtidle_kobj = arg;
>> +
>> + sysfs_remove_files(>idle_kobj->base, gt_idle_attrs);
>> + kobject_put(>idle_kobj->base);
>> +}
>> +
>> +int xe_gt_idle_init(struct xe_gt_idle *gtidle)
>> +{
>> + struct xe_gt *gt = gtidle_to_gt(gtidle);
>> + struct xe_device *xe = gt_to_xe(gt);
>> + struct gtidle_kobj *gtidle_kobj;
>> + int err;
>> +
>> + gtidle_kobj = kzalloc(sizeof(*gtidle_kobj), GFP_KERNEL);
>> +
>> + if (!gtidle_kobj)
>> + return -ENOMEM;
>> +
>> + err = kobject_init_and_add(>idle_kobj->base, >_idle_kobj_type, gt->sysfs, "gtidle");
>> + if (err)
>> + goto exit_kobj;
>> +
>> + gtidle_kobj->gtidle = gtidle;
>> +
>> + gtidle->residency_ms = xe_rc6_residency_ms;
>> + gtidle->idle_status = xe_rc_status;
>> + sprintf(gtidle->name, "gt%d-rc\n", gt->info.id);
>> +
>> + err = sysfs_create_files(>idle_kobj->base, gt_idle_attrs);
>> + if (err)
>> + goto exit_kobj;
>> +
>> + err = drmm_add_action_or_reset(&xe->drm, gt_idle_fini, gtidle_kobj);
>> + if (err)
>> + goto exit;
>
> gt_idle_fini() will be called here on error, so below
> sysfs_remove_files() will be called twice
>
>> +
>> + return 0;
>> +
>> +exit:
>> + sysfs_remove_files(>idle_kobj->base, gt_idle_attrs);
>> +
>> +exit_kobj:
>> + kobject_put(>idle_kobj->base);
>> + return err;
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_gt_idle.h b/drivers/gpu/drm/xe/xe_gt_idle.h
>> new file mode 100644
>> index 000000000000..ac0ab970a3f7
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_gt_idle.h
>> @@ -0,0 +1,18 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_GT_IDLE_H_
>> +#define _XE_GT_IDLE_H_
>> +
>> +#include "xe_gt.h"
>> +#include "xe_gt_idle_types.h"
>> +
>> +int xe_gt_idle_init(struct xe_gt_idle *gtidle);
>> +
>> +static inline struct xe_gt *gtidle_to_gt(struct xe_gt_idle *gtidle)
>> +{
>> + return container_of(gtidle, struct xe_gt, gtidle);
>> +}
>> +#endif /* _XE_GT_IDLE_H_ */
>> diff --git a/drivers/gpu/drm/xe/xe_gt_idle_types.h b/drivers/gpu/drm/xe/xe_gt_idle_types.h
>> new file mode 100644
>> index 000000000000..bc9c59249b6c
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_gt_idle_types.h
>> @@ -0,0 +1,39 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_GT_IDLE_TYPES_H_
>> +#define _XE_GT_IDLE_TYPES_H_
>> +
>> +#include <linux/kobject.h>
>> +#include <linux/types.h>
>> +
>> +/**
>> + * struct xe_gt_idle - A struct that contains idle properties based on gt
>> + */
>> +struct xe_gt_idle {
>> + /** @name: name */
>> + char name[16];
>> + /** @state: current idle state */
>> + const char *state;
>> + /** @cur_residency: raw driver copy of idle residency */
>> + u64 cur_residency;
>> + /** @prev_residency: previous residency counter */
>> + u64 prev_residency;
>> + /** @idle_status: get the current idle state */
>> + void (*idle_status)(struct xe_gt_idle *gtidle);
>> + /** @residency_ms: get idle residency in ms */
>> + u64 (*residency_ms)(struct xe_gt_idle *gtidle);
>> +};
>> +
>> +/**
>> + * struct gtidle_kobj - A kobject struct that connects the kobject and xe_gt_idle
>> + */
>> +struct gtidle_kobj {
>> + /** @base: The actual kobject */
>> + struct kobject base;
>> + /** @gtidle: A pointer to the struct xe_gt_idle */
>> + struct xe_gt_idle *gtidle;
>> +};
>
> maybe all you need is to reuse kobj_gt from xe_gt_sysfs_types.h ?
>
>> +#endif /* _XE_GT_IDLE_TYPES_H_ */
>> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
>> index 7c47d67aa8be..90b2d34efe5b 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_types.h
>> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
>> @@ -7,6 +7,7 @@
>> #define _XE_GT_TYPES_H_
>>
>> #include "xe_force_wake_types.h"
>> +#include "xe_gt_idle_types.h"
>> #include "xe_hw_engine_types.h"
>> #include "xe_hw_fence_types.h"
>> #include "xe_reg_sr_types.h"
>> @@ -287,6 +288,9 @@ struct xe_gt {
>> /** @uc: micro controllers on the GT */
>> struct xe_uc uc;
>>
>> + /** @gtidle: idle properties of GT */
>> + struct xe_gt_idle gtidle;
>> +
>> /** @engine_ops: submission backend engine operations */
>> const struct xe_engine_ops *engine_ops;
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
>> index 72d460d5323b..e83283cbcb00 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>> @@ -14,6 +14,7 @@
>> #include "xe_bo.h"
>> #include "xe_device.h"
>> #include "xe_gt.h"
>> +#include "xe_gt_idle.h"
>> #include "xe_gt_sysfs.h"
>> #include "xe_gt_types.h"
>> #include "xe_guc_ct.h"
>> @@ -40,6 +41,9 @@
>> #define GT_FREQUENCY_MULTIPLIER 50
>> #define GEN9_FREQ_SCALER 3
>>
>> +/* Multiplier for RC6 Residency counter in units of 1.28us */
>> +#define XE_RC6_MULTIPLIER 1280
>> +
>> /**
>> * DOC: GuC Power Conservation (PC)
>> *
>> @@ -76,12 +80,7 @@
>> *
>> * Render-C states is also a GuC PC feature that is now enabled in Xe for
>> * all platforms.
>> - * Xe's GuC PC provides a sysfs API for Render-C States:
>> *
>> - * device/gt#/rc* *read-only* files:
>> - * - rc_status: Provide the actual immediate status of Render-C: (rc0 or rc6)
>> - * - rc6_residency: Provide the rc6_residency counter in units of 1.28 uSec.
>> - * Prone to overflows.
>> */
>>
>> static struct xe_guc *
>> @@ -580,11 +579,41 @@ static ssize_t freq_max_store(struct device *dev, struct device_attribute *attr,
>> }
>> static DEVICE_ATTR_RW(freq_max);
>>
>> -static ssize_t rc_status_show(struct device *dev,
>> - struct device_attribute *attr, char *buff)
>> +u64 get_residency_ms(struct xe_gt_idle *gtidle, u64 cur_residency,
>> + u32 residency_multiplier)
>
> as this function is not used elsewhere it should static
> but OTOH it operates on gtidle so not really belongs to guc_pc.c
> and should be properly exported from xe_gt_idle.[ch]
>
>> {
>> - struct xe_guc_pc *pc = dev_to_pc(dev);
>> - struct xe_gt *gt = pc_to_gt(pc);
>> + u64 delta, overflow_residency, prev_residency;
>> +
>> + overflow_residency = BIT_ULL(32);
>> + /*
>> + * Counter wrap handling
>> + * Store previous hw counter values for counter wrap-around handling
>> + * Relying on sufficient frequency of queries otherwise counters can still wrap.
>> + */
>> + prev_residency = gtidle->prev_residency;
>> + gtidle->prev_residency = cur_residency;
>> +
>> + /* delta */
>> + if (cur_residency >= prev_residency)
>> + delta = cur_residency - prev_residency;
>> + else
>> + delta = cur_residency + (overflow_residency - prev_residency);
>> +
>> + /* Add delta to extended raw driver copy of idle residency */
>> + cur_residency = gtidle->cur_residency + delta;
>> + gtidle->cur_residency = cur_residency;
>> +
>> + /* residency multiplier in ns, convert to ms */
>> + return mul_u64_u32_div(cur_residency, residency_multiplier, 1e6);
>> +}
>> +
>> +/**
>> + * xe_rc_status - get the current Render C state
>> + * @gtidle - xe_gt_idle instance
>> + */
>> +void xe_rc_status(struct xe_gt_idle *gtidle)
>
> hmm, this function seems to accessing GT registers only, why it is
> placed in guc_pc.c ? it really doesn't belong to GuC IMO
>
>> +{
>> + struct xe_gt *gt = gtidle_to_gt(gtidle);
>> u32 reg;
>>
>> xe_device_mem_access_get(gt_to_xe(gt));
>> @@ -593,37 +622,33 @@ static ssize_t rc_status_show(struct device *dev,
>>
>> switch (REG_FIELD_GET(RCN_MASK, reg)) {
>> case GT_RC6:
>> - return sysfs_emit(buff, "rc6\n");
>> + gtidle->state = "rc6\n";
>> + return;
>> case GT_RC0:
>> - return sysfs_emit(buff, "rc0\n");
>> + gtidle->state = "rc0\n";
>> + return;
>> default:
>> - return -ENOENT;
>> + gtidle->state = "unknown\n";
>
> kind strange design pattern to pass status as string ...
> IMO we should rather define enum and convert to string as needed
>
>> }
>> }
>> -static DEVICE_ATTR_RO(rc_status);
>>
>> -static ssize_t rc6_residency_show(struct device *dev,
>> - struct device_attribute *attr, char *buff)
>> +/**
>> + * xe_rc6_residency_ms - rc6 residency in ms
>> + * @gtidle - xe_gt_idle instance
>> + */
>> +u64 xe_rc6_residency_ms(struct xe_gt_idle *gtidle)
>> {
>> - struct xe_guc_pc *pc = dev_to_pc(dev);
>> - struct xe_gt *gt = pc_to_gt(pc);
>> - u32 reg;
>> - ssize_t ret;
>> -
>> - xe_device_mem_access_get(pc_to_xe(pc));
>> - ret = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> - if (ret)
>> - goto out;
>> + struct xe_gt *gt = gtidle_to_gt(gtidle);
>> + u64 reg, rc6_residency;
>>
>> + xe_device_mem_access_get(gt_to_xe(gt));
>> reg = xe_mmio_read32(gt, GT_GFX_RC6.reg);
>> - ret = sysfs_emit(buff, "%u\n", reg);
>>
>> - XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL));
>> -out:
>> - xe_device_mem_access_put(pc_to_xe(pc));
>> - return ret;
>> + rc6_residency = get_residency_ms(gtidle, reg, XE_RC6_MULTIPLIER);
>> +
>> + xe_device_mem_access_put(gt_to_xe(gt));
>> + return rc6_residency;
>> }
>> -static DEVICE_ATTR_RO(rc6_residency);
>>
>> static const struct attribute *pc_attrs[] = {
>> &dev_attr_freq_act.attr,
>> @@ -633,8 +658,6 @@ static const struct attribute *pc_attrs[] = {
>> &dev_attr_freq_rpn.attr,
>> &dev_attr_freq_min.attr,
>> &dev_attr_freq_max.attr,
>> - &dev_attr_rc_status.attr,
>> - &dev_attr_rc6_residency.attr,
>> NULL
>> };
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.h b/drivers/gpu/drm/xe/xe_guc_pc.h
>> index da29e4934868..a6abac189c8a 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.h
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.h
>> @@ -8,8 +8,13 @@
>>
>> #include "xe_guc_pc_types.h"
>>
>> +struct xe_gt_idle;
>> +
>> 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);
>>
>> +void xe_rc_status(struct xe_gt_idle *gtidle);
>> +u64 xe_rc6_residency_ms(struct xe_gt_idle *gtidle);
>
> if you look around you should clearly see that such function prototypes
> do not fit here, all other functions have "xe_guc_pc" prefix and takes
> "xe_guc_pc" as param ...
>
> please reconsider function placement and the flow
Thanks for the review. Will fix the above review comments.
Rev1 had the rc functions in gt_idle. Based on the review, all low level
functions were moved to this file as it is a guc pc feature
Will retain the prefix and change the parameter to guc_pc
Thanks
Riana
>
> Michal
>
>> +
>> #endif /* _XE_GUC_PC_H_ */
More information about the Intel-xe
mailing list