[Intel-gfx] [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface
Andrzej Hajda
andrzej.hajda at intel.com
Thu Mar 3 10:19:48 UTC 2022
On 17.02.2022 15:41, Andi Shyti wrote:
> Now tiles have their own sysfs interfaces under the gt/
> directory. Because RC6 is a property that can be configured on a
> tile basis, then each tile should have its own interface
>
> The new sysfs structure will have a similar layout for the 4 tile
> case:
>
> /sys/.../card0
> ├── gt
> │ ├── gt0
> │ │ ├── id
> │ │ ├── rc6_enable
> │ │ ├── rc6_residency_ms
> . . .
> . . .
> . .
> │ └── gtN
> │ ├── id
> │ ├── rc6_enable
> │ ├── rc6_residency_ms
> │ .
> │ .
> │
> └── power/ -+
> ├── rc6_enable | Original interface
> ├── rc6_residency_ms +-> kept as existing ABI;
> . | it multiplexes over
> . | the GTs
> -+
>
> The existing interfaces have been kept in their original location
> to preserve the existing ABI. They act on all the GTs: when
> reading they provide the average value from all the GTs.
If ABI should be kept forever, depreciation msg should be removed from
intel_gt_sysfs_get_drvdata.
>
> This patch is not really adding exposing new interfaces (new
> ABI) other than adapting the existing one to more tiles. In any
> case this new set of interfaces will be a basic tool for system
> managers and administrators when using i915.
> Signed-off-by: Andi Shyti <andi.shyti at linux.intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Matt Roper <matthew.d.roper at intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 19 ++
> drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 220 ++++++++++++++++++++
> drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h | 15 ++
> drivers/gpu/drm/i915/i915_sysfs.c | 128 ------------
> 5 files changed, 255 insertions(+), 128 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 277064b00afd..7104558b81d5 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -106,6 +106,7 @@ gt-y += \
> gt/intel_gt_pm_irq.o \
> gt/intel_gt_requests.o \
> gt/intel_gt_sysfs.o \
> + gt/intel_gt_sysfs_pm.o \
> gt/intel_gtt.o \
> gt/intel_llc.o \
> gt/intel_lrc.o \
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
> index 0206e9aa4867..132b90247a41 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
> @@ -13,6 +13,7 @@
> #include "i915_sysfs.h"
> #include "intel_gt.h"
> #include "intel_gt_sysfs.h"
> +#include "intel_gt_sysfs_pm.h"
> #include "intel_gt_types.h"
> #include "intel_rc6.h"
>
> @@ -54,6 +55,11 @@ struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
> return kobj_to_gt(kobj);
> }
>
> +static struct kobject *gt_get_parent_obj(struct intel_gt *gt)
> +{
> + return >->i915->drm.primary->kdev->kobj;
> +}
> +
> static ssize_t id_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -107,6 +113,17 @@ void intel_gt_sysfs_register(struct intel_gt *gt)
> struct kobject *dir;
> char name[80];
>
> + /*
> + * We need to make things right with the
> + * ABI compatibility. The files were originally
> + * generated under the parent directory.
> + *
> + * We generate the files only for gt 0
> + * to avoid duplicates.
> + */
> + if (gt_is_root(gt))
> + intel_gt_sysfs_pm_init(gt, gt_get_parent_obj(gt));
> +
> snprintf(name, sizeof(name), "gt%d", gt->info.id);
>
> dir = intel_gt_create_kobj(gt, gt->i915->sysfs_gt, name);
> @@ -115,4 +132,6 @@ void intel_gt_sysfs_register(struct intel_gt *gt)
> "failed to initialize %s sysfs root\n", name);
> return;
> }
> +
> + intel_gt_sysfs_pm_init(gt, dir);
> }
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> new file mode 100644
> index 000000000000..27d93a36894a
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +#include <drm/drm_device.h>
> +#include <linux/sysfs.h>
> +#include <linux/printk.h>
> +
> +#include "i915_drv.h"
> +#include "i915_sysfs.h"
> +#include "intel_gt.h"
> +#include "intel_gt_regs.h"
> +#include "intel_gt_sysfs.h"
> +#include "intel_gt_sysfs_pm.h"
> +#include "intel_rc6.h"
> +
> +#ifdef CONFIG_PM
> +static s64
> +sysfs_gt_attribute_r_func(struct device *dev, struct device_attribute *attr,
> + s64 (func)(struct intel_gt *gt))
> +{
> + struct intel_gt *gt;
> + s64 ret = 0;
> +
> + if (!is_object_gt(&dev->kobj)) {
> + int i, num_gt = 0;
> + struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> +
> + for_each_gt(gt, i915, i) {
> + ret += func(gt);
> + num_gt++;
> + }
> +
> + ret /= num_gt;
> + } else {
> + gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> + ret = func(gt);
> + }
> +
> + return ret;
> +}
Return value is always converted to u32 or int, maybe we could use just
int ?
> +
> +static u32 get_residency(struct intel_gt *gt, i915_reg_t reg)
> +{
> + intel_wakeref_t wakeref;
> + u64 res = 0;
> +
> + with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> + res = intel_rc6_residency_us(>->rc6, reg);
> +
> + return DIV_ROUND_CLOSEST_ULL(res, 1000);
> +}
> +
> +static ssize_t rc6_enable_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buff)
> +{
> + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> + u8 mask = 0;
> +
> + if (HAS_RC6(gt->i915))
> + mask |= BIT(0);
> + if (HAS_RC6p(gt->i915))
> + mask |= BIT(1);
> + if (HAS_RC6pp(gt->i915))
> + mask |= BIT(2);
> +
> + return sysfs_emit(buff, "%x\n", mask);
> +}
> +
> +static s64 __rc6_residency_ms_show(struct intel_gt *gt)
> +{
> + return get_residency(gt, GEN6_GT_GFX_RC6);
> +}
> +
> +static ssize_t rc6_residency_ms_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buff)
> +{
> + s64 rc6_residency = sysfs_gt_attribute_r_func(dev, attr,
> + __rc6_residency_ms_show);
> +
> + return sysfs_emit(buff, "%u\n", (u32) rc6_residency);
Here and below (where applicable) variable should be just u32, no need
to conversion in sysfs_emit.
> +}
> +
> +static s64 __rc6p_residency_ms_show(struct intel_gt *gt)
> +{
> + return get_residency(gt, GEN6_GT_GFX_RC6p);
> +}
> +
> +static ssize_t rc6p_residency_ms_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buff)
> +{
> + s64 rc6p_residency = sysfs_gt_attribute_r_func(dev, attr,
> + __rc6p_residency_ms_show);
> +
> + return sysfs_emit(buff, "%u\n", (u32) rc6p_residency);
> +}
> +
> +static s64 __rc6pp_residency_ms_show(struct intel_gt *gt)
> +{
> + return get_residency(gt, GEN6_GT_GFX_RC6pp);
> +}
> +
> +static ssize_t rc6pp_residency_ms_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buff)
> +{
> + s64 rc6pp_residency = sysfs_gt_attribute_r_func(dev, attr,
> + __rc6pp_residency_ms_show);
> +
> + return sysfs_emit(buff, "%u\n", (u32) rc6pp_residency);
> +}
> +
> +static s64 __media_rc6_residency_ms_show(struct intel_gt *gt)
> +{
> + return get_residency(gt, VLV_GT_MEDIA_RC6);
> +}
> +
> +static ssize_t media_rc6_residency_ms_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buff)
> +{
> + s64 rc6_residency = sysfs_gt_attribute_r_func(dev, attr,
> + __media_rc6_residency_ms_show);
> +
> + return sysfs_emit(buff, "%u\n", (u32) rc6_residency);
> +}
> +
> +static DEVICE_ATTR_RO(rc6_enable);
> +static DEVICE_ATTR_RO(rc6_residency_ms);
> +static DEVICE_ATTR_RO(rc6p_residency_ms);
> +static DEVICE_ATTR_RO(rc6pp_residency_ms);
> +static DEVICE_ATTR_RO(media_rc6_residency_ms);
> +
> +static struct attribute *rc6_attrs[] = {
> + &dev_attr_rc6_enable.attr,
> + &dev_attr_rc6_residency_ms.attr,
> + NULL
> +};
> +
> +static struct attribute *rc6p_attrs[] = {
> + &dev_attr_rc6p_residency_ms.attr,
> + &dev_attr_rc6pp_residency_ms.attr,
> + NULL
> +};
> +
> +static struct attribute *media_rc6_attrs[] = {
> + &dev_attr_media_rc6_residency_ms.attr,
> + NULL
> +};
> +
> +static const struct attribute_group rc6_attr_group[] = {
> + { .attrs = rc6_attrs, },
> + { .name = power_group_name, .attrs = rc6_attrs, },
> +};
> +
> +static const struct attribute_group rc6p_attr_group[] = {
> + { .attrs = rc6p_attrs, },
> + { .name = power_group_name, .attrs = rc6p_attrs, },
> +};
> +
> +static const struct attribute_group media_rc6_attr_group[] = {
> + { .attrs = media_rc6_attrs, },
> + { .name = power_group_name, .attrs = media_rc6_attrs, },
> +};
> +
> +static int __intel_gt_sysfs_create_group(struct kobject *kobj,
> + const struct attribute_group *grp)
> +{
> + return is_object_gt(kobj) ?
> + sysfs_create_group(kobj, &grp[0]) :
> + sysfs_merge_group(kobj, &grp[1]);
> +}
Merging handling "gt/gt#/*" and "power/*" attributes into the same
helpers seems unnatural to me, in many functions we have two branches
based on value of is_object_gt, with the most hacky
intel_gt_sysfs_get_drvdata.
Splitting handling these attributes would allow to drop fragile
is_object_gt helper and make functions more straightforward, probably at
the cost of few lines more. On the other side I am not sure if it is
worth to change everything to just address code purity concerns :)
So with variable type adjustement:
Reviewed-by: Andrzej Hajda <andrzej.hajda at intel.com>
Regards
Andrzej
Regards
Andrzej
> +
> +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj)
> +{
> + int ret;
> +
> + if (!HAS_RC6(gt->i915))
> + return;
> +
> + ret = __intel_gt_sysfs_create_group(kobj, rc6_attr_group);
> + if (ret)
> + drm_warn(>->i915->drm,
> + "failed to create gt%u RC6 sysfs files\n",
> + gt->info.id);
> +
> + /*
> + * cannot use the is_visible() attribute because
> + * the upper object inherits from the parent group.
> + */
> + if (HAS_RC6p(gt->i915)) {
> + ret = __intel_gt_sysfs_create_group(kobj, rc6p_attr_group);
> + if (ret)
> + drm_warn(>->i915->drm,
> + "failed to create gt%u RC6p sysfs files\n",
> + gt->info.id);
> + }
> +
> + if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) {
> + ret = __intel_gt_sysfs_create_group(kobj, media_rc6_attr_group);
> + if (ret)
> + drm_warn(>->i915->drm,
> + "failed to create media %u RC6 sysfs files\n",
> + gt->info.id);
> + }
> +}
> +#else
> +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj)
> +{
> +}
> +#endif /* CONFIG_PM */
> +
> +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj)
> +{
> + intel_sysfs_rc6_init(gt, kobj);
> +}
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h
> new file mode 100644
> index 000000000000..f567105a4a89
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +#ifndef __SYSFS_GT_PM_H__
> +#define __SYSFS_GT_PM_H__
> +
> +#include <linux/kobject.h>
> +
> +#include "intel_gt_types.h"
> +
> +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj);
> +
> +#endif /* SYSFS_RC6_H */
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 3643efde52ca..b08a959e5ccc 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -45,107 +45,6 @@ struct drm_i915_private *kdev_minor_to_i915(struct device *kdev)
> return to_i915(minor->dev);
> }
>
> -#ifdef CONFIG_PM
> -static u32 calc_residency(struct drm_i915_private *dev_priv,
> - i915_reg_t reg)
> -{
> - intel_wakeref_t wakeref;
> - u64 res = 0;
> -
> - with_intel_runtime_pm(&dev_priv->runtime_pm, wakeref)
> - res = intel_rc6_residency_us(&to_gt(dev_priv)->rc6, reg);
> -
> - return DIV_ROUND_CLOSEST_ULL(res, 1000);
> -}
> -
> -static ssize_t rc6_enable_show(struct device *kdev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> - unsigned int mask;
> -
> - mask = 0;
> - if (HAS_RC6(dev_priv))
> - mask |= BIT(0);
> - if (HAS_RC6p(dev_priv))
> - mask |= BIT(1);
> - if (HAS_RC6pp(dev_priv))
> - mask |= BIT(2);
> -
> - return sysfs_emit(buf, "%x\n", mask);
> -}
> -
> -static ssize_t rc6_residency_ms_show(struct device *kdev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> - u32 rc6_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6);
> - return sysfs_emit(buf, "%u\n", rc6_residency);
> -}
> -
> -static ssize_t rc6p_residency_ms_show(struct device *kdev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> - u32 rc6p_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6p);
> - return sysfs_emit(buf, "%u\n", rc6p_residency);
> -}
> -
> -static ssize_t rc6pp_residency_ms_show(struct device *kdev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> - u32 rc6pp_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6pp);
> - return sysfs_emit(buf, "%u\n", rc6pp_residency);
> -}
> -
> -static ssize_t media_rc6_residency_ms_show(struct device *kdev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> - u32 rc6_residency = calc_residency(dev_priv, VLV_GT_MEDIA_RC6);
> - return sysfs_emit(buf, "%u\n", rc6_residency);
> -}
> -
> -static DEVICE_ATTR_RO(rc6_enable);
> -static DEVICE_ATTR_RO(rc6_residency_ms);
> -static DEVICE_ATTR_RO(rc6p_residency_ms);
> -static DEVICE_ATTR_RO(rc6pp_residency_ms);
> -static DEVICE_ATTR_RO(media_rc6_residency_ms);
> -
> -static struct attribute *rc6_attrs[] = {
> - &dev_attr_rc6_enable.attr,
> - &dev_attr_rc6_residency_ms.attr,
> - NULL
> -};
> -
> -static const struct attribute_group rc6_attr_group = {
> - .name = power_group_name,
> - .attrs = rc6_attrs
> -};
> -
> -static struct attribute *rc6p_attrs[] = {
> - &dev_attr_rc6p_residency_ms.attr,
> - &dev_attr_rc6pp_residency_ms.attr,
> - NULL
> -};
> -
> -static const struct attribute_group rc6p_attr_group = {
> - .name = power_group_name,
> - .attrs = rc6p_attrs
> -};
> -
> -static struct attribute *media_rc6_attrs[] = {
> - &dev_attr_media_rc6_residency_ms.attr,
> - NULL
> -};
> -
> -static const struct attribute_group media_rc6_attr_group = {
> - .name = power_group_name,
> - .attrs = media_rc6_attrs
> -};
> -#endif
> -
> static int l3_access_valid(struct drm_i915_private *i915, loff_t offset)
> {
> if (!HAS_L3_DPF(i915))
> @@ -497,29 +396,6 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
> struct device *kdev = dev_priv->drm.primary->kdev;
> int ret;
>
> -#ifdef CONFIG_PM
> - if (HAS_RC6(dev_priv)) {
> - ret = sysfs_merge_group(&kdev->kobj,
> - &rc6_attr_group);
> - if (ret)
> - drm_err(&dev_priv->drm,
> - "RC6 residency sysfs setup failed\n");
> - }
> - if (HAS_RC6p(dev_priv)) {
> - ret = sysfs_merge_group(&kdev->kobj,
> - &rc6p_attr_group);
> - if (ret)
> - drm_err(&dev_priv->drm,
> - "RC6p residency sysfs setup failed\n");
> - }
> - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> - ret = sysfs_merge_group(&kdev->kobj,
> - &media_rc6_attr_group);
> - if (ret)
> - drm_err(&dev_priv->drm,
> - "Media RC6 residency sysfs setup failed\n");
> - }
> -#endif
> if (HAS_L3_DPF(dev_priv)) {
> ret = device_create_bin_file(kdev, &dpf_attrs);
> if (ret)
> @@ -565,8 +441,4 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
> sysfs_remove_files(&kdev->kobj, gen6_attrs);
> device_remove_bin_file(kdev, &dpf_attrs_1);
> device_remove_bin_file(kdev, &dpf_attrs);
> -#ifdef CONFIG_PM
> - sysfs_unmerge_group(&kdev->kobj, &rc6_attr_group);
> - sysfs_unmerge_group(&kdev->kobj, &rc6p_attr_group);
> -#endif
> }
More information about the Intel-gfx
mailing list