From: Andi Shyti <andi.shyti@linux.intel.com> To: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Abdiel Janulgue <abdiel.janulgue@gmail.com>, Andi Shyti <andi.shyti@linux.intel.com>, Tvrtko Ursulin <tvrtko.ursulin@intel.com>, Intel GFX <intel-gfx@lists.freedesktop.org>, Lucas De Marchi <lucas.demarchi@intel.com>, DRI Devel <dri-devel@lists.freedesktop.org>, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>, Matthew Auld <matthew.auld@intel.com>, Andi Shyti <andi@etezian.org>, Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> Subject: Re: [PATCH v4 2/2] drm/i915/gt: make a gt sysfs group and move power management files Date: Tue, 18 Jan 2022 02:00:55 +0200 [thread overview] Message-ID: <YeYDN/GG/C3/6mE0@intel.intel> (raw) In-Reply-To: <d3eb3ece-3f15-7c2c-dd72-57000835bd65@intel.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Andi Shyti <andi.shyti@linux.intel.com> To: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Intel GFX <intel-gfx@lists.freedesktop.org>, Lucas De Marchi <lucas.demarchi@intel.com>, DRI Devel <dri-devel@lists.freedesktop.org>, Matthew Auld <matthew.auld@intel.com> Subject: Re: [Intel-gfx] [PATCH v4 2/2] drm/i915/gt: make a gt sysfs group and move power management files Date: Tue, 18 Jan 2022 02:00:55 +0200 [thread overview] Message-ID: <YeYDN/GG/C3/6mE0@intel.intel> (raw) In-Reply-To: <d3eb3ece-3f15-7c2c-dd72-57000835bd65@intel.com> 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
next prev parent reply other threads:[~2022-01-18 0:01 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-01-17 19:32 [PATCH v4 0/2] Introduce multitile support Andi Shyti 2022-01-17 19:32 ` [Intel-gfx] " Andi Shyti 2022-01-17 19:32 ` [PATCH v4 1/2] drm/i915: Prepare for multiple GTs Andi Shyti 2022-01-17 19:32 ` [Intel-gfx] " Andi Shyti 2022-01-17 22:24 ` Michal Wajdeczko 2022-01-17 22:24 ` Michal Wajdeczko 2022-01-17 23:12 ` Andi Shyti 2022-01-17 23:12 ` [Intel-gfx] " Andi Shyti 2022-01-17 19:32 ` [PATCH v4 2/2] drm/i915/gt: make a gt sysfs group and move power management files Andi Shyti 2022-01-17 19:32 ` [Intel-gfx] " Andi Shyti 2022-01-17 22:49 ` Michal Wajdeczko 2022-01-17 22:49 ` [Intel-gfx] " Michal Wajdeczko 2022-01-18 0:00 ` Andi Shyti [this message] 2022-01-18 0:00 ` Andi Shyti 2022-01-18 11:16 ` Tvrtko Ursulin 2022-01-18 12:49 ` Andi Shyti 2022-01-18 12:49 ` Andi Shyti 2022-01-17 19:49 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Introduce multitile support Patchwork 2022-01-17 19:50 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork 2022-01-17 20:16 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 2022-01-17 21:32 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YeYDN/GG/C3/6mE0@intel.intel \ --to=andi.shyti@linux.intel.com \ --cc=abdiel.janulgue@gmail.com \ --cc=andi@etezian.org \ --cc=daniele.ceraolospurio@intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=lucas.demarchi@intel.com \ --cc=matthew.auld@intel.com \ --cc=michal.wajdeczko@intel.com \ --cc=sujaritha.sundaresan@intel.com \ --cc=tvrtko.ursulin@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.