[Intel-xe] [PATCH] drm/xe: add a new sysfs directory for gtidle properties
Gupta, Anshuman
anshuman.gupta at intel.com
Tue May 30 10:30:30 UTC 2023
> -----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 ?
> + 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 ?
> +
> + /* 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 ?
> +
> + 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.
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