[Intel-xe] [PATCH] drm/xe: add a new sysfs directory for gtidle properties
Riana Tauro
riana.tauro at intel.com
Mon Jun 5 15:00:38 UTC 2023
On 5/31/2023 10:28 AM, Riana Tauro wrote:
> 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 ?
Hi Anshuman
seq_read_iter takes a lock while calling sysfs_show
https://elixir.bootlin.com/linux/latest/source/fs/seq_file.c#L171
Call Trace:
[ 1079.006358] <TASK>
[ 1079.006363] dump_stack_lvl+0x92/0xb0
[ 1079.006373] idle_residency_ms_show+0x34/0x110 [xe]
[ 1079.006563] sysfs_kf_seq_show+0x14f/0x1e0
[ 1079.006571] ? __pfx_kobj_attr_show+0x10/0x10
[ 1079.006582] seq_read_iter+0x3b6/0x880
So locking will not be needed
Thanks
Riana
> 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