[Intel-xe] [PATCH] drm/xe: add a new sysfs directory for gtidle properties
Riana Tauro
riana.tauro at intel.com
Wed May 31 04:58:28 UTC 2023
Hi Anshuman
On 5/30/2023 4:00 PM, Gupta, Anshuman wrote:
>
>
>> -----Original Message-----
>> From: Tauro, Riana <riana.tauro at intel.com>
>> Sent: Wednesday, May 24, 2023 11:30 AM
>> To: intel-xe at lists.freedesktop.org
>> Cc: Tauro, Riana <riana.tauro at intel.com>; Gupta, Anshuman
>> <anshuman.gupta at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>; Dixit,
>> Ashutosh <ashutosh.dixit at intel.com>; Nilawar, Badal
>> <badal.nilawar at intel.com>; Wajdeczko, Michal
>> <Michal.Wajdeczko at intel.com>
>> Subject: [PATCH] drm/xe: add a new sysfs directory for gtidle properties
>>
>> 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)
>>
>> v3:
>> - return error for drmm_add_action_or_reset
>> - replace file and functions with gt_idle prefix
>> to gt_idle_sysfs (Himal)
>> - use enum for gt idle state
>> - move multiplier to gt idle and initialize (Anshuman)
>> - correct doc annotation (Rodrigo)
>> - remove return variable
>> - use kobj_gt instead of new gtidle kobj
>> - move residency_ms to gtidle file
>> - retain xe_guc_pc prefix for functions in guc_rc file (Michal)
>>
>> Signed-off-by: Riana Tauro <riana.tauro at intel.com>
>> Acked-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> ---
>> drivers/gpu/drm/xe/Makefile | 1 +
>> drivers/gpu/drm/xe/xe_gt.c | 5 +
>> drivers/gpu/drm/xe/xe_gt_idle_sysfs.c | 157 ++++++++++++++++++++
>> drivers/gpu/drm/xe/xe_gt_idle_sysfs.h | 13 ++
>> drivers/gpu/drm/xe/xe_gt_idle_sysfs_types.h | 38 +++++
>> drivers/gpu/drm/xe/xe_gt_types.h | 4 +
>> drivers/gpu/drm/xe/xe_guc_pc.c | 46 +++---
>> drivers/gpu/drm/xe/xe_guc_pc.h | 2 +
>> 8 files changed, 237 insertions(+), 29 deletions(-) create mode 100644
>> drivers/gpu/drm/xe/xe_gt_idle_sysfs.c
>> create mode 100644 drivers/gpu/drm/xe/xe_gt_idle_sysfs.h
>> create mode 100644 drivers/gpu/drm/xe/xe_gt_idle_sysfs_types.h
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index cd1614b68734..84f0985054fa 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -45,6 +45,7 @@ xe-y += xe_bb.o \
>> xe_gt.o \
>> xe_gt_clock.o \
>> xe_gt_debugfs.o \
>> + xe_gt_idle_sysfs.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
>> 80d42c7c7cfa..eb1e1e3d2d71 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_sysfs.h"
>> #include "xe_gt_mcr.h"
>> #include "xe_gt_pagefault.h"
>> #include "xe_gt_printk.h"
>> @@ -383,6 +384,10 @@ static int gt_fw_domain_init(struct xe_gt *gt)
>> if (err)
>> goto err_force_wake;
>>
>> + err = xe_gt_idle_sysfs_init(>->gtidle);
> Can't we keep this simple to pass just GT and inside function, initialize the GT ?
This was to keep consistency with other files having struct specific to
the files as parameters. IMO gtidle as a parameter is appropriate as all
the functions in the file use xe_gt_idle
>> + 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_sysfs.c
>> b/drivers/gpu/drm/xe/xe_gt_idle_sysfs.c
>> new file mode 100644
>> index 000000000000..b7de53287f6b
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_gt_idle_sysfs.c
>> @@ -0,0 +1,157 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#include <drm/drm_managed.h>
>> +
>> +#include "xe_device.h"
>> +#include "xe_gt.h"
>> +#include "xe_gt_idle_sysfs.h"
>> +#include "xe_gt_sysfs.h"
>> +#include "xe_guc_pc.h"
>> +
>> +/**
>> + * DOC: Xe GT Idle
>> + *
>> + * Provides sysfs entries for idle properties 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 *dev_to_gtidle(struct device *dev) {
>> + struct kobject *kobj = &dev->kobj;
>> +
>> + return &kobj_to_gt(kobj->parent)->gtidle;
>> +}
>> +
>> +static struct xe_gt *gtidle_to_gt(struct xe_gt_idle *gtidle) {
>> + return container_of(gtidle, struct xe_gt, gtidle); }
>> +
>> +static struct xe_guc_pc *gtidle_to_pc(struct xe_gt_idle *gtidle) {
>> + return >idle_to_gt(gtidle)->uc.guc.pc; }
>> +
>> +static const char *gt_idle_state_to_string(enum xe_gt_idle_state state)
>> +{
>> + switch (state) {
>> + case GT_IDLE_RC0:
>> + return "rc0";
>> + case GT_IDLE_RC6:
>> + return "rc6";
>> + default:
>> + return "unknown";
>> + }
>> +}
>> +
>> +static u64 get_residency_ms(struct xe_gt_idle *gtidle, u64
>> +cur_residency) {
>> + 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;
> Multiple user threads can race against each other while reading this attribute.
> So probably we need some sort of locking ?
Thanks for pointing this.
Will add this in next rev
>> +
>> + /* 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, gtidle-
>>> residency_multiplier,
>> +1e6); }
>> +
>> +static ssize_t name_show(struct device *dev,
>> + struct device_attribute *attr, char *buff) {
>> + struct xe_gt_idle *gtidle = dev_to_gtidle(dev);
>> +
>> + return sysfs_emit(buff, gtidle->name); } static
>> DEVICE_ATTR_RO(name);
>> +
>> +static ssize_t idle_status_show(struct device *dev,
>> + struct device_attribute *attr, char *buff) {
>> + struct xe_gt_idle *gtidle = dev_to_gtidle(dev);
>> + struct xe_guc_pc *pc = gtidle_to_pc(gtidle);
>> + enum xe_gt_idle_state state;
>> +
>> + state = gtidle->idle_status(pc);
>> +
>> + return sysfs_emit(buff, "%s\n", gt_idle_state_to_string(state)); }
>> +static DEVICE_ATTR_RO(idle_status);
>> +
>> +static ssize_t idle_residency_ms_show(struct device *dev,
>> + struct device_attribute *attr, char *buff) {
>> + struct xe_gt_idle *gtidle = dev_to_gtidle(dev);
>> + struct xe_guc_pc *pc = gtidle_to_pc(gtidle);
>> + u64 residency;
>> +
>> + residency = gtidle->idle_residency(pc);
>> + return sysfs_emit(buff, "%llu\n", get_residency_ms(gtidle,
>> +residency)); } 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_sysfs_fini(struct drm_device *drm, void *arg) {
>> + struct kobject *kobj = arg;
>> +
>> + sysfs_remove_files(kobj, gt_idle_attrs);
>> + kobject_put(kobj);
>> +}
>> +
>> +int xe_gt_idle_sysfs_init(struct xe_gt_idle *gtidle) {
>> + struct xe_gt *gt = gtidle_to_gt(gtidle);
>> + struct xe_device *xe = gt_to_xe(gt);
>> + struct kobject *kobj;
>> + int err;
>> +
>> + kobj = kobject_create_and_add("gtidle", gt->sysfs);
>> + if (!kobj)
>> + return -ENOMEM;
> IMO ENOMEM does not suits here, How about EINVAL or ENODEV ?
kobject_create_and_add returns NULL if kzalloc fails or when the add fails.
Most of the files return -ENOMEM when the function fails
>> +
>> + sprintf(gtidle->name, "gt%d-rc\n", gt->info.id);
>> + /* Multiplier for RC6 Residency counter in units of 1.28us */
>> + gtidle->residency_multiplier = 1280;
>> + gtidle->idle_residency = xe_guc_pc_rc6_residency;
>> + gtidle->idle_status = xe_guc_pc_rc_status;
>> +
>> + err = sysfs_create_files(kobj, gt_idle_attrs);
>> + if (err) {
>> + kobject_put(kobj);
>> + return err;
>> + }
>> +
>> + err = drmm_add_action_or_reset(&xe->drm, gt_idle_sysfs_fini,
>> kobj);
>> + if (err)
>> + return err;
>> +
>> + return 0;
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_gt_idle_sysfs.h
>> b/drivers/gpu/drm/xe/xe_gt_idle_sysfs.h
>> new file mode 100644
>> index 000000000000..a654561fda19
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_gt_idle_sysfs.h
>> @@ -0,0 +1,13 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_GT_IDLE_SYSFS_H_
>> +#define _XE_GT_IDLE_SYSFS_H_
>> +
>> +#include "xe_gt_idle_sysfs_types.h"
>> +
>> +int xe_gt_idle_sysfs_init(struct xe_gt_idle *gtidle);
>> +
>> +#endif /* _XE_GT_IDLE_SYSFS_H_ */
>> diff --git a/drivers/gpu/drm/xe/xe_gt_idle_sysfs_types.h
>> b/drivers/gpu/drm/xe/xe_gt_idle_sysfs_types.h
>> new file mode 100644
>> index 000000000000..b9cfb37aae75
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_gt_idle_sysfs_types.h
>> @@ -0,0 +1,38 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_GT_IDLE_SYSFS_TYPES_H_
>> +#define _XE_GT_IDLE_SYSFS_TYPES_H_
>> +
>> +#include <linux/types.h>
>> +
>> +struct xe_guc_pc;
>> +
>> +/* States of GT Idle */
>> +enum xe_gt_idle_state {
>> + GT_IDLE_RC0,
>> + GT_IDLE_RC6,
>> + GT_IDLE_UNKNOWN,
>> +};
>> +
>> +/**
>> + * struct xe_gt_idle - A struct that contains idle properties based of
>> +gt */ struct xe_gt_idle {
>> + /** @name: name */
>> + char name[16];
>> + /** @residency_multiplier: residency multiplier in ns */
>> + u32 residency_multiplier;
>> + /** @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 */
>> + enum xe_gt_idle_state (*idle_status)(struct xe_guc_pc *pc);
>> + /** @idle_residency: get idle residency counter */
>> + u64 (*idle_residency)(struct xe_guc_pc *pc); };
>> +
>> +#endif /* _XE_GT_IDLE_SYSFS_TYPES_H_ */
>> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h
>> b/drivers/gpu/drm/xe/xe_gt_types.h
>> index 7c47d67aa8be..8dec660f5fc6 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_sysfs_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 e799faa1c6b8..31c56a0dc210
>> 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>> @@ -76,12 +76,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,10 +575,12 @@ 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)
>> +/**
>> + * xe_guc_pc_rc_status - get the current Render C state
>> + * @pc - XE_GuC_PC instance
>> + */
>> +enum xe_gt_idle_state xe_guc_pc_rc_status(struct xe_guc_pc *pc)
>> {
>> - struct xe_guc_pc *pc = dev_to_pc(dev);
>> struct xe_gt *gt = pc_to_gt(pc);
>> u32 reg;
>>
>> @@ -593,37 +590,30 @@ 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");
>> + return GT_IDLE_RC6;
>> case GT_RC0:
>> - return sysfs_emit(buff, "rc0\n");
>> + return GT_IDLE_RC0;
>> default:
>> - return -ENOENT;
>> + return GT_IDLE_UNKNOWN;
>> }
>> }
>> -static DEVICE_ATTR_RO(rc_status);
>>
>> -static ssize_t rc6_residency_show(struct device *dev,
>> - struct device_attribute *attr, char *buff)
>> +/**
>> + * xe_guc_pc_rc6_residency - rc6 residency counter
>> + * @pc - Xe_GuC_PC instance
>> + */
>> +u64 xe_guc_pc_rc6_residency(struct xe_guc_pc *pc)
>> {
>> - struct xe_guc_pc *pc = dev_to_pc(dev);
>> struct xe_gt *gt = pc_to_gt(pc);
>> - u32 reg;
>> - ssize_t ret;
>> + u64 reg;
> Why do we need u64 here as we are reading 32 bit register, caller can do the implicit type conversion.
Will change this to u32
Thanks
Riana
> Thanks,
> Anshuman Gupta.
>>
>> - 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;
>> + xe_device_mem_access_get(gt_to_xe(gt));
>>
>> reg = xe_mmio_read32(gt, GT_GFX_RC6);
>> - 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;
>> + xe_device_mem_access_put(gt_to_xe(gt));
>> + return reg;
>> }
>> -static DEVICE_ATTR_RO(rc6_residency);
>>
>> static const struct attribute *pc_attrs[] = {
>> &dev_attr_freq_act.attr,
>> @@ -633,8 +623,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..976179dbc9a8
>> 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.h
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.h
>> @@ -12,4 +12,6 @@ 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);
>>
>> +enum xe_gt_idle_state xe_guc_pc_rc_status(struct xe_guc_pc *pc);
>> +u64 xe_guc_pc_rc6_residency(struct xe_guc_pc *pc);
>> #endif /* _XE_GUC_PC_H_ */
>> --
>> 2.40.0
>
More information about the Intel-xe
mailing list