Hi Michal,
/sys/.../card0 ├── gt │ ├── gt0 │ │ ├── id │ │ ├── rc6_enable │ │ ├── rc6_residency_ms │ │ ├── rps_act_freq_mhz │ │ ├── rps_boost_freq_mhz │ │ ├── rps_cur_freq_mhz │ │ ├── rps_max_freq_mhz │ │ ├── rps_min_freq_mhz │ │ ├── rps_RP0_freq_mhz │ │ ├── rps_RP1_freq_mhz │ │ └── rps_RPn_freq_mhz . . . . . . │ └── gt3
gtN ?
yep!
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index aa86ac33effc..5fd203c626fc 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -121,7 +121,9 @@ gt-y += \ gt/intel_timeline.o \ gt/intel_workarounds.o \ gt/shmem_utils.o \
- gt/sysfs_engines.o
- gt/sysfs_engines.o \
- gt/sysfs_gt.o \
- gt/sysfs_gt_pm.o
shouldn't these be named as
- gt/intel_gt_sysfs.o \
- gt/intel_gt_pm_sysfs.o
You are right with wanting a coherent prefix, but I kept the trend of starting with sysfs_gt*. We already have sysfs_engine.c.
And, because I wouldn't like to have part of it sysfs_gt* and part of it intel_gt_sysfs*, then we either rename all or we leave it as it is.
On the other hand if we are under i915/gt/... I don't expect it to be the sysfs of another system.
To be honest, I don't have a strong opinion on this. If you do, then I will change everything intel_gt_sysfs*.
[...]
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt.c @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: MIT +/*
- Copyright © 2020 Intel Corporation
2022 ?
Time flies... huh? :)
+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->info.id)
maybe we should have gt_is_root(gt) helper ?
yes, makes sense.
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);
- if (!dir) {
drm_warn(>->i915->drm,"failed to initialize %s sysfs root\n", name);return;- }
- if (sysfs_create_file(dir, &dev_attr_id.attr))
drm_warn(>->i915->drm,"failed to create sysfs %s info files\n", name);can't we use default_groups in kobj_type ?
yeah... I'll try that.
[...]
+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 scnprintf(buff, PAGE_SIZE, "%x\n", mask);
sysfs_emit ?
OK
[...]
- ret = __intel_gt_sysfs_create_group(kobj, rc6_attr_group);
- if (ret)
drm_err(>->i915->drm,"failed to create gt%u RC6 sysfs files\n", gt->info.id);- if (HAS_RC6p(gt->i915)) {
ret = __intel_gt_sysfs_create_group(kobj, rc6p_attr_group);if (ret)drm_err(>->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_err(>->i915->drm,"failed to create media %u RC6 sysfs files\n",gt->info.id);- }
did you consider using attribute_group.is_visible instead adding groups manually ?
I can try this, as well.
[...]
maybe this large but simple code movement should be done in a separate patch so we could then apply smaller and easier to review fixes ?
I can try to split it, even though most of it is basically a copy/paste.
~Michal
Thanks a lot for this review, as well!
Andi