| Message ID | 20220217144158.21555-5-andi.shyti@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce multitile support | expand |
On 17.02.2022 15:41, Andi Shyti wrote: > Now that we have tiles we want each of them to have its own > interface. A directory "gt/" is created under "cardN/" that will > contain as many diroctories as the tiles. > > In the coming patches tile related interfaces will be added. For > now the sysfs gt structure simply has an id interface related > to the current tile count. > > The directory structure will follow this scheme: > > /sys/.../card0 > └── gt > ├── gt0 > │ └── id > : > : > └─- gtN > └── id > > 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@linux.intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Reviewed-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> > --- > drivers/gpu/drm/i915/Makefile | 1 + > drivers/gpu/drm/i915/gt/intel_gt.c | 2 + > drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 118 +++++++++++++++++++++++ > drivers/gpu/drm/i915/gt/intel_gt_sysfs.h | 34 +++++++ > drivers/gpu/drm/i915/i915_drv.h | 2 + > drivers/gpu/drm/i915/i915_sysfs.c | 12 ++- > drivers/gpu/drm/i915/i915_sysfs.h | 3 + > 7 files changed, 171 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs.c > create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs.h > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index 9d588d936e3d..277064b00afd 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -105,6 +105,7 @@ gt-y += \ > gt/intel_gt_pm_debugfs.o \ > gt/intel_gt_pm_irq.o \ > gt/intel_gt_requests.o \ > + gt/intel_gt_sysfs.o \ > gt/intel_gtt.o \ > gt/intel_llc.o \ > gt/intel_lrc.o \ > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c > index 8c64b81e9ec9..0f080bbad043 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c > @@ -26,6 +26,7 @@ > #include "intel_rc6.h" > #include "intel_renderstate.h" > #include "intel_rps.h" > +#include "intel_gt_sysfs.h" > #include "intel_uncore.h" > #include "shmem_utils.h" > > @@ -458,6 +459,7 @@ void intel_gt_driver_register(struct intel_gt *gt) > intel_rps_driver_register(>->rps); > > intel_gt_debugfs_register(gt); > + intel_gt_sysfs_register(gt); > } > > static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size) > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c > new file mode 100644 > index 000000000000..0206e9aa4867 > --- /dev/null > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c > @@ -0,0 +1,118 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2022 Intel Corporation > + */ > + > +#include <drm/drm_device.h> > +#include <linux/device.h> > +#include <linux/kobject.h> > +#include <linux/printk.h> > +#include <linux/sysfs.h> > + > +#include "i915_drv.h" > +#include "i915_sysfs.h" > +#include "intel_gt.h" > +#include "intel_gt_sysfs.h" > +#include "intel_gt_types.h" > +#include "intel_rc6.h" > + > +bool is_object_gt(struct kobject *kobj) > +{ > + return !strncmp(kobj->name, "gt", 2); > +} It looks quite fragile, at the moment I do not have better idea:) maybe after reviewing the rest of the patches. > + > +static struct intel_gt *kobj_to_gt(struct kobject *kobj) > +{ > + return container_of(kobj, struct kobj_gt, base)->gt; > +} > + > +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, > + const char *name) > +{ > + struct kobject *kobj = &dev->kobj; > + > + /* > + * We are interested at knowing from where the interface > + * has been called, whether it's called from gt/ or from > + * the parent directory. > + * From the interface position it depends also the value of > + * the private data. > + * If the interface is called from gt/ then private data is > + * of the "struct intel_gt *" type, otherwise it's * a > + * "struct drm_i915_private *" type. > + */ > + if (!is_object_gt(kobj)) { > + struct drm_i915_private *i915 = kdev_minor_to_i915(dev); > + > + pr_devel_ratelimited(DEPRECATED > + "%s (pid %d) is accessing deprecated %s " > + "sysfs control, please use gt/gt<n>/%s instead\n", > + current->comm, task_pid_nr(current), name, name); > + return to_gt(i915); > + } > + > + return kobj_to_gt(kobj); It took some time for me to understand what is going on here. We have dev argument which sometimes can point to "struct device", sometimes to "struct kobj_gt", but it's type suggests differently, quite ugly. I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in case of intel_engines_add_sysfs. This way abstractions would look better, hopefully. > +} > + > +static ssize_t id_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); > + > + return sysfs_emit(buf, "%u\n", gt->info.id); > +} > +static DEVICE_ATTR_RO(id); > + > +static struct attribute *id_attrs[] = { > + &dev_attr_id.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(id); > + > +static void kobj_gt_release(struct kobject *kobj) > +{ > + kfree(kobj); > +} > + > +static struct kobj_type kobj_gt_type = { > + .release = kobj_gt_release, > + .sysfs_ops = &kobj_sysfs_ops, > + .default_groups = id_groups, > +}; > + > +struct kobject * > +intel_gt_create_kobj(struct intel_gt *gt, struct kobject *dir, const char *name) > +{ > + struct kobj_gt *kg; > + > + kg = kzalloc(sizeof(*kg), GFP_KERNEL); > + if (!kg) > + return NULL; > + > + kobject_init(&kg->base, &kobj_gt_type); > + kg->gt = gt; > + > + /* xfer ownership to sysfs tree */ > + if (kobject_add(&kg->base, dir, "%s", name)) { > + kobject_put(&kg->base); > + return NULL; > + } > + > + return &kg->base; /* borrowed ref */ > +} > + > +void intel_gt_sysfs_register(struct intel_gt *gt) > +{ > + struct kobject *dir; > + char name[80]; > + > + 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; > + } > +} Squashing intel_gt_create_kobj into intel_gt_sysfs_register would simplify code and allows drop snprintf to local array. > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h > new file mode 100644 > index 000000000000..9471b26752cf > --- /dev/null > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h > @@ -0,0 +1,34 @@ > +/* SPDX-License-Identifier: MIT */ > +/* > + * Copyright © 2022 Intel Corporation > + */ > + > +#ifndef __SYSFS_GT_H__ > +#define __SYSFS_GT_H__ > + > +#include <linux/ctype.h> > +#include <linux/kobject.h> > + > +#include "i915_gem.h" /* GEM_BUG_ON() */ > + > +struct intel_gt; > + > +struct kobj_gt { > + struct kobject base; > + struct intel_gt *gt; > +}; > + > +bool is_object_gt(struct kobject *kobj); > + > +struct drm_i915_private *kobj_to_i915(struct kobject *kobj); > + > +struct kobject * > +intel_gt_create_kobj(struct intel_gt *gt, > + struct kobject *dir, > + const char *name); > + > +void intel_gt_sysfs_register(struct intel_gt *gt); > +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, > + const char *name); > + > +#endif /* SYSFS_GT_H */ > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 88a83cd81ddd..e8c729f2ae00 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -811,6 +811,8 @@ struct drm_i915_private { > #define I915_MAX_GT 4 > struct intel_gt *gt[I915_MAX_GT]; > > + struct kobject *sysfs_gt; > + > struct { > struct i915_gem_contexts { > spinlock_t lock; /* locks list */ > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c > index a4d1759375b9..3643efde52ca 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.c > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > @@ -39,7 +39,7 @@ > #include "i915_sysfs.h" > #include "intel_pm.h" > > -static inline struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) > +struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) > { > struct drm_minor *minor = dev_get_drvdata(kdev); > return to_i915(minor->dev); > @@ -487,6 +487,11 @@ static void i915_setup_error_capture(struct device *kdev) {} > static void i915_teardown_error_capture(struct device *kdev) {} > #endif > > +static struct kobject *i915_setup_gt_sysfs(struct kobject *parent) > +{ > + return kobject_create_and_add("gt", parent); > +} > + > void i915_setup_sysfs(struct drm_i915_private *dev_priv) > { > struct device *kdev = dev_priv->drm.primary->kdev; > @@ -538,6 +543,11 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) > if (ret) > drm_err(&dev_priv->drm, "RPS sysfs setup failed\n"); > > + dev_priv->sysfs_gt = i915_setup_gt_sysfs(&kdev->kobj); Why not directly kobject_create_and_add("gt", parent) ? up to you. Regards Andrzej > + if (!dev_priv->sysfs_gt) > + drm_warn(&dev_priv->drm, > + "failed to register GT sysfs directory\n"); > + > i915_setup_error_capture(kdev); > > intel_engines_add_sysfs(dev_priv); > diff --git a/drivers/gpu/drm/i915/i915_sysfs.h b/drivers/gpu/drm/i915/i915_sysfs.h > index 41afd4366416..243a17741e3f 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.h > +++ b/drivers/gpu/drm/i915/i915_sysfs.h > @@ -6,8 +6,11 @@ > #ifndef __I915_SYSFS_H__ > #define __I915_SYSFS_H__ > > +struct device; > struct drm_i915_private; > > +struct drm_i915_private *kdev_minor_to_i915(struct device *kdev); > + > void i915_setup_sysfs(struct drm_i915_private *i915); > void i915_teardown_sysfs(struct drm_i915_private *i915); >
Hi Andrzej, [...] > > +bool is_object_gt(struct kobject *kobj) > > +{ > > + return !strncmp(kobj->name, "gt", 2); > > +} > > It looks quite fragile, at the moment I do not have better idea:) maybe > after reviewing the rest of the patches. yeah... it's not pretty, I agree, but I couldn't come up with a better way of doing it. > > +static struct intel_gt *kobj_to_gt(struct kobject *kobj) > > +{ > > + return container_of(kobj, struct kobj_gt, base)->gt; > > +} > > + > > +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, > > + const char *name) > > +{ > > + struct kobject *kobj = &dev->kobj; > > + > > + /* > > + * We are interested at knowing from where the interface > > + * has been called, whether it's called from gt/ or from > > + * the parent directory. > > + * From the interface position it depends also the value of > > + * the private data. > > + * If the interface is called from gt/ then private data is > > + * of the "struct intel_gt *" type, otherwise it's * a > > + * "struct drm_i915_private *" type. > > + */ > > + if (!is_object_gt(kobj)) { > > + struct drm_i915_private *i915 = kdev_minor_to_i915(dev); > > + > > + pr_devel_ratelimited(DEPRECATED > > + "%s (pid %d) is accessing deprecated %s " > > + "sysfs control, please use gt/gt<n>/%s instead\n", > > + current->comm, task_pid_nr(current), name, name); > > + return to_gt(i915); > > + } > > + > > + return kobj_to_gt(kobj); > > It took some time for me to understand what is going on here. > We have dev argument which sometimes can point to "struct device", sometimes > to "struct kobj_gt", but it's type suggests differently, quite ugly. > I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in > case of intel_engines_add_sysfs. This way abstractions would look better, > hopefully. How would it help? The difference is that I'm adding twice different interfaces with the same name and different location (i.e. different object). The legacy intrefaces inherit the object from drm and I'm preserving that reference. While the new objects would derive from the previous and they are pretty much like intel_engines_add_sysfs(). [...] > > +struct kobject * > > +intel_gt_create_kobj(struct intel_gt *gt, struct kobject *dir, const char *name) > > +{ > > + struct kobj_gt *kg; > > + > > + kg = kzalloc(sizeof(*kg), GFP_KERNEL); > > + if (!kg) > > + return NULL; > > + > > + kobject_init(&kg->base, &kobj_gt_type); > > + kg->gt = gt; > > + > > + /* xfer ownership to sysfs tree */ > > + if (kobject_add(&kg->base, dir, "%s", name)) { > > + kobject_put(&kg->base); > > + return NULL; > > + } > > + > > + return &kg->base; /* borrowed ref */ > > +} > > + > > +void intel_gt_sysfs_register(struct intel_gt *gt) > > +{ > > + struct kobject *dir; > > + char name[80]; > > + > > + 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; > > + } > > +} > > Squashing intel_gt_create_kobj into intel_gt_sysfs_register would simplify > code and allows drop snprintf to local array. right! > > +static struct kobject *i915_setup_gt_sysfs(struct kobject *parent) > > +{ > > + return kobject_create_and_add("gt", parent); > > +} > > + > > void i915_setup_sysfs(struct drm_i915_private *dev_priv) > > { > > struct device *kdev = dev_priv->drm.primary->kdev; > > @@ -538,6 +543,11 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) > > if (ret) > > drm_err(&dev_priv->drm, "RPS sysfs setup failed\n"); > > + dev_priv->sysfs_gt = i915_setup_gt_sysfs(&kdev->kobj); > > Why not directly kobject_create_and_add("gt", parent) ? up to you. of course! [...] Thanks a lot for the review, Andi
On 07.03.2022 00:04, Andi Shyti wrote: > Hi Andrzej, > > [...] > >>> +bool is_object_gt(struct kobject *kobj) >>> +{ >>> + return !strncmp(kobj->name, "gt", 2); >>> +} >> It looks quite fragile, at the moment I do not have better idea:) maybe >> after reviewing the rest of the patches. > yeah... it's not pretty, I agree, but I couldn't come up with a > better way of doing it. > >>> +static struct intel_gt *kobj_to_gt(struct kobject *kobj) >>> +{ >>> + return container_of(kobj, struct kobj_gt, base)->gt; >>> +} >>> + >>> +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, >>> + const char *name) >>> +{ >>> + struct kobject *kobj = &dev->kobj; >>> + >>> + /* >>> + * We are interested at knowing from where the interface >>> + * has been called, whether it's called from gt/ or from >>> + * the parent directory. >>> + * From the interface position it depends also the value of >>> + * the private data. >>> + * If the interface is called from gt/ then private data is >>> + * of the "struct intel_gt *" type, otherwise it's * a >>> + * "struct drm_i915_private *" type. >>> + */ >>> + if (!is_object_gt(kobj)) { >>> + struct drm_i915_private *i915 = kdev_minor_to_i915(dev); >>> + >>> + pr_devel_ratelimited(DEPRECATED >>> + "%s (pid %d) is accessing deprecated %s " >>> + "sysfs control, please use gt/gt<n>/%s instead\n", >>> + current->comm, task_pid_nr(current), name, name); >>> + return to_gt(i915); >>> + } >>> + >>> + return kobj_to_gt(kobj); >> It took some time for me to understand what is going on here. >> We have dev argument which sometimes can point to "struct device", sometimes >> to "struct kobj_gt", but it's type suggests differently, quite ugly. >> I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in >> case of intel_engines_add_sysfs. This way abstractions would look better, >> hopefully. > How would it help? > > The difference is that I'm adding twice different interfaces with > the same name and different location (i.e. different object). The > legacy intrefaces inherit the object from drm and I'm preserving > that reference. > > While the new objects would derive from the previous and they are > pretty much like intel_engines_add_sysfs(). I was not clear on the issue. Here in case of 'id' attribute it is defined as device_attribute, but in kobj_type.sysfs_ops you assign formally incompatible &kobj_sysfs_ops. kobj_sysfs_ops expects kobj_attribute! Fortunately kobj_attribute is 'binary compatible' with device_attribute and kobj is at beginning of struct device as well, so it does not blow up, but I wouldn't say it is clean solution :) If you look at intel_engines_add_sysfs you can see that all attributes are defined as kobj_attribute. Regards Andrzej > > [...] > >>> +struct kobject * >>> +intel_gt_create_kobj(struct intel_gt *gt, struct kobject *dir, const char *name) >>> +{ >>> + struct kobj_gt *kg; >>> + >>> + kg = kzalloc(sizeof(*kg), GFP_KERNEL); >>> + if (!kg) >>> + return NULL; >>> + >>> + kobject_init(&kg->base, &kobj_gt_type); >>> + kg->gt = gt; >>> + >>> + /* xfer ownership to sysfs tree */ >>> + if (kobject_add(&kg->base, dir, "%s", name)) { >>> + kobject_put(&kg->base); >>> + return NULL; >>> + } >>> + >>> + return &kg->base; /* borrowed ref */ >>> +} >>> + >>> +void intel_gt_sysfs_register(struct intel_gt *gt) >>> +{ >>> + struct kobject *dir; >>> + char name[80]; >>> + >>> + 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; >>> + } >>> +} >> Squashing intel_gt_create_kobj into intel_gt_sysfs_register would simplify >> code and allows drop snprintf to local array. > right! > >>> +static struct kobject *i915_setup_gt_sysfs(struct kobject *parent) >>> +{ >>> + return kobject_create_and_add("gt", parent); >>> +} >>> + >>> void i915_setup_sysfs(struct drm_i915_private *dev_priv) >>> { >>> struct device *kdev = dev_priv->drm.primary->kdev; >>> @@ -538,6 +543,11 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) >>> if (ret) >>> drm_err(&dev_priv->drm, "RPS sysfs setup failed\n"); >>> + dev_priv->sysfs_gt = i915_setup_gt_sysfs(&kdev->kobj); >> Why not directly kobject_create_and_add("gt", parent) ? up to you. > of course! > > [...] > > Thanks a lot for the review, > Andi
Hi Andrzej, I'm sorry, but I'm not fully understanding, > > > > +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, > > > > + const char *name) > > > > +{ > > > > + struct kobject *kobj = &dev->kobj; > > > > + > > > > + /* > > > > + * We are interested at knowing from where the interface > > > > + * has been called, whether it's called from gt/ or from > > > > + * the parent directory. > > > > + * From the interface position it depends also the value of > > > > + * the private data. > > > > + * If the interface is called from gt/ then private data is > > > > + * of the "struct intel_gt *" type, otherwise it's * a > > > > + * "struct drm_i915_private *" type. > > > > + */ > > > > + if (!is_object_gt(kobj)) { > > > > + struct drm_i915_private *i915 = kdev_minor_to_i915(dev); > > > > + > > > > + pr_devel_ratelimited(DEPRECATED > > > > + "%s (pid %d) is accessing deprecated %s " > > > > + "sysfs control, please use gt/gt<n>/%s instead\n", > > > > + current->comm, task_pid_nr(current), name, name); > > > > + return to_gt(i915); > > > > + } > > > > + > > > > + return kobj_to_gt(kobj); > > > It took some time for me to understand what is going on here. > > > We have dev argument which sometimes can point to "struct device", sometimes > > > to "struct kobj_gt", but it's type suggests differently, quite ugly. > > > I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in > > > case of intel_engines_add_sysfs. This way abstractions would look better, > > > hopefully. > > How would it help? > > > > The difference is that I'm adding twice different interfaces with > > the same name and different location (i.e. different object). The > > legacy intrefaces inherit the object from drm and I'm preserving > > that reference. > > > > While the new objects would derive from the previous and they are > > pretty much like intel_engines_add_sysfs(). > > I was not clear on the issue. Here in case of 'id' attribute it is defined > as device_attribute, but in kobj_type.sysfs_ops you assign formally > incompatible &kobj_sysfs_ops. 'kobj_sysfs_ops' is of the type 'kobj_type'. > kobj_sysfs_ops expects kobj_attribute! Fortunately kobj_attribute is 'binary > compatible' with device_attribute and kobj is at beginning of struct device > as well, so it does not blow up, but I wouldn't say it is clean solution :) > If you look at intel_engines_add_sysfs you can see that all attributes are > defined as kobj_attribute. That's exactly the approach I use in the next patches for the power management files, I use "struct kobj_gt" wrapped around "struct kobject". But I'm using that only for the GT files. Are you, btw, suggesting to use this same approache also for the legacy files that for now have a pointer to the drm kobject? This way I would need to add more information, like the pointer to i915 and gt_id. This way I wouldn't need the files above that look hacky to you. Is this what you mean? Andi
> > > > > +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, > > > > > + const char *name) > > > > > +{ > > > > > + struct kobject *kobj = &dev->kobj; > > > > > + > > > > > + /* > > > > > + * We are interested at knowing from where the interface > > > > > + * has been called, whether it's called from gt/ or from > > > > > + * the parent directory. > > > > > + * From the interface position it depends also the value of > > > > > + * the private data. > > > > > + * If the interface is called from gt/ then private data is > > > > > + * of the "struct intel_gt *" type, otherwise it's * a > > > > > + * "struct drm_i915_private *" type. > > > > > + */ > > > > > + if (!is_object_gt(kobj)) { > > > > > + struct drm_i915_private *i915 = kdev_minor_to_i915(dev); > > > > > + > > > > > + pr_devel_ratelimited(DEPRECATED > > > > > + "%s (pid %d) is accessing deprecated %s " > > > > > + "sysfs control, please use gt/gt<n>/%s instead\n", > > > > > + current->comm, task_pid_nr(current), name, name); > > > > > + return to_gt(i915); > > > > > + } > > > > > + > > > > > + return kobj_to_gt(kobj); > > > > It took some time for me to understand what is going on here. > > > > We have dev argument which sometimes can point to "struct device", sometimes > > > > to "struct kobj_gt", but it's type suggests differently, quite ugly. > > > > I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in > > > > case of intel_engines_add_sysfs. This way abstractions would look better, > > > > hopefully. > > > How would it help? > > > > > > The difference is that I'm adding twice different interfaces with > > > the same name and different location (i.e. different object). The > > > legacy intrefaces inherit the object from drm and I'm preserving > > > that reference. > > > > > > While the new objects would derive from the previous and they are > > > pretty much like intel_engines_add_sysfs(). > > > > I was not clear on the issue. Here in case of 'id' attribute it is defined > > as device_attribute, but in kobj_type.sysfs_ops you assign formally > > incompatible &kobj_sysfs_ops. > > 'kobj_sysfs_ops' is of the type 'kobj_type'. > > > kobj_sysfs_ops expects kobj_attribute! Fortunately kobj_attribute is 'binary > > compatible' with device_attribute and kobj is at beginning of struct device > > as well, so it does not blow up, but I wouldn't say it is clean solution :) > > If you look at intel_engines_add_sysfs you can see that all attributes are > > defined as kobj_attribute. > > That's exactly the approach I use in the next patches for the > power management files, I use "struct kobj_gt" wrapped around > "struct kobject". But I'm using that only for the GT files. > > Are you, btw, suggesting to use this same approache also for the > legacy files that for now have a pointer to the drm kobject? This > way I would need to add more information, like the pointer to > i915 and gt_id. This way I wouldn't need the files above that > look hacky to you. Is this what you mean? Still this wouldn't solve it because I am merging the legacy interfaces to an existing kobject and creating new kobjects for the new interfaces that go under gt. I would need some other ugly hack to have things coming around. Unless I misunderstood you. Andi
On 13.03.2022 20:45, Andi Shyti wrote: > Hi Andrzej, > > I'm sorry, but I'm not fully understanding, > >>>>> +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, >>>>> + const char *name) >>>>> +{ >>>>> + struct kobject *kobj = &dev->kobj; >>>>> + >>>>> + /* >>>>> + * We are interested at knowing from where the interface >>>>> + * has been called, whether it's called from gt/ or from >>>>> + * the parent directory. >>>>> + * From the interface position it depends also the value of >>>>> + * the private data. >>>>> + * If the interface is called from gt/ then private data is >>>>> + * of the "struct intel_gt *" type, otherwise it's * a >>>>> + * "struct drm_i915_private *" type. >>>>> + */ >>>>> + if (!is_object_gt(kobj)) { >>>>> + struct drm_i915_private *i915 = kdev_minor_to_i915(dev); >>>>> + >>>>> + pr_devel_ratelimited(DEPRECATED >>>>> + "%s (pid %d) is accessing deprecated %s " >>>>> + "sysfs control, please use gt/gt<n>/%s instead\n", >>>>> + current->comm, task_pid_nr(current), name, name); >>>>> + return to_gt(i915); >>>>> + } >>>>> + >>>>> + return kobj_to_gt(kobj); >>>> It took some time for me to understand what is going on here. >>>> We have dev argument which sometimes can point to "struct device", sometimes >>>> to "struct kobj_gt", but it's type suggests differently, quite ugly. >>>> I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in >>>> case of intel_engines_add_sysfs. This way abstractions would look better, >>>> hopefully. >>> How would it help? >>> >>> The difference is that I'm adding twice different interfaces with >>> the same name and different location (i.e. different object). The >>> legacy intrefaces inherit the object from drm and I'm preserving >>> that reference. >>> >>> While the new objects would derive from the previous and they are >>> pretty much like intel_engines_add_sysfs(). >> I was not clear on the issue. Here in case of 'id' attribute it is defined >> as device_attribute, but in kobj_type.sysfs_ops you assign formally >> incompatible &kobj_sysfs_ops. > 'kobj_sysfs_ops' is of the type 'kobj_type'. Yes, but for example kobj_sysfs_ops.show points to function kobj_attr_show, and kobj_attr_show expects that it's attr argument is embedded in kobj_attribute[1], but this is not true in case of 'id' attribute - it is embedded in device_attribute. In short kobj_sysfs_ops should be used only with attrs embeded in kobj_attribute, unless I missed sth. [1]: https://elixir.bootlin.com/linux/latest/source/lib/kobject.c#L836 > >> kobj_sysfs_ops expects kobj_attribute! Fortunately kobj_attribute is 'binary >> compatible' with device_attribute and kobj is at beginning of struct device >> as well, so it does not blow up, but I wouldn't say it is clean solution :) >> If you look at intel_engines_add_sysfs you can see that all attributes are >> defined as kobj_attribute. > That's exactly the approach I use in the next patches for the > power management files, I use "struct kobj_gt" wrapped around > "struct kobject". But I'm using that only for the GT files. But attributes are still defined using DEVICE_ATTR* macros, ie they are embedded in device_attribute, so the problem is the same - you are using kobj_sysfs_ops with device_attribute. > > Are you, btw, suggesting to use this same approache also for the > legacy files that for now have a pointer to the drm kobject? This > way I would need to add more information, like the pointer to > i915 and gt_id. This way I wouldn't need the files above that > look hacky to you. Is this what you mean? Positive feedback is more difficult :) I am little bit lost in possible solutions, after grepping other drivers I have not good advice about proper handling of such situation, *beside splitting the interface*. For sure attrs used in device/power must be embedded in device_attribute. So if you do not want to split interface, then it implies GTs attrs must be also in device_attribute. Then maybe creating custom sysfs_ops would help??? I am not sure. Regards Andrzej > > Andi
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 9d588d936e3d..277064b00afd 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -105,6 +105,7 @@ gt-y += \ gt/intel_gt_pm_debugfs.o \ gt/intel_gt_pm_irq.o \ gt/intel_gt_requests.o \ + gt/intel_gt_sysfs.o \ gt/intel_gtt.o \ gt/intel_llc.o \ gt/intel_lrc.o \ diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index 8c64b81e9ec9..0f080bbad043 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -26,6 +26,7 @@ #include "intel_rc6.h" #include "intel_renderstate.h" #include "intel_rps.h" +#include "intel_gt_sysfs.h" #include "intel_uncore.h" #include "shmem_utils.h" @@ -458,6 +459,7 @@ void intel_gt_driver_register(struct intel_gt *gt) intel_rps_driver_register(>->rps); intel_gt_debugfs_register(gt); + intel_gt_sysfs_register(gt); } static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size) diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c new file mode 100644 index 000000000000..0206e9aa4867 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2022 Intel Corporation + */ + +#include <drm/drm_device.h> +#include <linux/device.h> +#include <linux/kobject.h> +#include <linux/printk.h> +#include <linux/sysfs.h> + +#include "i915_drv.h" +#include "i915_sysfs.h" +#include "intel_gt.h" +#include "intel_gt_sysfs.h" +#include "intel_gt_types.h" +#include "intel_rc6.h" + +bool is_object_gt(struct kobject *kobj) +{ + return !strncmp(kobj->name, "gt", 2); +} + +static struct intel_gt *kobj_to_gt(struct kobject *kobj) +{ + return container_of(kobj, struct kobj_gt, base)->gt; +} + +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, + const char *name) +{ + struct kobject *kobj = &dev->kobj; + + /* + * We are interested at knowing from where the interface + * has been called, whether it's called from gt/ or from + * the parent directory. + * From the interface position it depends also the value of + * the private data. + * If the interface is called from gt/ then private data is + * of the "struct intel_gt *" type, otherwise it's * a + * "struct drm_i915_private *" type. + */ + if (!is_object_gt(kobj)) { + struct drm_i915_private *i915 = kdev_minor_to_i915(dev); + + pr_devel_ratelimited(DEPRECATED + "%s (pid %d) is accessing deprecated %s " + "sysfs control, please use gt/gt<n>/%s instead\n", + current->comm, task_pid_nr(current), name, name); + return to_gt(i915); + } + + return kobj_to_gt(kobj); +} + +static ssize_t id_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + + return sysfs_emit(buf, "%u\n", gt->info.id); +} +static DEVICE_ATTR_RO(id); + +static struct attribute *id_attrs[] = { + &dev_attr_id.attr, + NULL, +}; +ATTRIBUTE_GROUPS(id); + +static void kobj_gt_release(struct kobject *kobj) +{ + kfree(kobj); +} + +static struct kobj_type kobj_gt_type = { + .release = kobj_gt_release, + .sysfs_ops = &kobj_sysfs_ops, + .default_groups = id_groups, +}; + +struct kobject * +intel_gt_create_kobj(struct intel_gt *gt, struct kobject *dir, const char *name) +{ + struct kobj_gt *kg; + + kg = kzalloc(sizeof(*kg), GFP_KERNEL); + if (!kg) + return NULL; + + kobject_init(&kg->base, &kobj_gt_type); + kg->gt = gt; + + /* xfer ownership to sysfs tree */ + if (kobject_add(&kg->base, dir, "%s", name)) { + kobject_put(&kg->base); + return NULL; + } + + return &kg->base; /* borrowed ref */ +} + +void intel_gt_sysfs_register(struct intel_gt *gt) +{ + struct kobject *dir; + char name[80]; + + 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; + } +} diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h new file mode 100644 index 000000000000..9471b26752cf --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h @@ -0,0 +1,34 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2022 Intel Corporation + */ + +#ifndef __SYSFS_GT_H__ +#define __SYSFS_GT_H__ + +#include <linux/ctype.h> +#include <linux/kobject.h> + +#include "i915_gem.h" /* GEM_BUG_ON() */ + +struct intel_gt; + +struct kobj_gt { + struct kobject base; + struct intel_gt *gt; +}; + +bool is_object_gt(struct kobject *kobj); + +struct drm_i915_private *kobj_to_i915(struct kobject *kobj); + +struct kobject * +intel_gt_create_kobj(struct intel_gt *gt, + struct kobject *dir, + const char *name); + +void intel_gt_sysfs_register(struct intel_gt *gt); +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, + const char *name); + +#endif /* SYSFS_GT_H */ diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 88a83cd81ddd..e8c729f2ae00 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -811,6 +811,8 @@ struct drm_i915_private { #define I915_MAX_GT 4 struct intel_gt *gt[I915_MAX_GT]; + struct kobject *sysfs_gt; + struct { struct i915_gem_contexts { spinlock_t lock; /* locks list */ diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index a4d1759375b9..3643efde52ca 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -39,7 +39,7 @@ #include "i915_sysfs.h" #include "intel_pm.h" -static inline struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) +struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) { struct drm_minor *minor = dev_get_drvdata(kdev); return to_i915(minor->dev); @@ -487,6 +487,11 @@ static void i915_setup_error_capture(struct device *kdev) {} static void i915_teardown_error_capture(struct device *kdev) {} #endif +static struct kobject *i915_setup_gt_sysfs(struct kobject *parent) +{ + return kobject_create_and_add("gt", parent); +} + void i915_setup_sysfs(struct drm_i915_private *dev_priv) { struct device *kdev = dev_priv->drm.primary->kdev; @@ -538,6 +543,11 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) if (ret) drm_err(&dev_priv->drm, "RPS sysfs setup failed\n"); + dev_priv->sysfs_gt = i915_setup_gt_sysfs(&kdev->kobj); + if (!dev_priv->sysfs_gt) + drm_warn(&dev_priv->drm, + "failed to register GT sysfs directory\n"); + i915_setup_error_capture(kdev); intel_engines_add_sysfs(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_sysfs.h b/drivers/gpu/drm/i915/i915_sysfs.h index 41afd4366416..243a17741e3f 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.h +++ b/drivers/gpu/drm/i915/i915_sysfs.h @@ -6,8 +6,11 @@ #ifndef __I915_SYSFS_H__ #define __I915_SYSFS_H__ +struct device; struct drm_i915_private; +struct drm_i915_private *kdev_minor_to_i915(struct device *kdev); + void i915_setup_sysfs(struct drm_i915_private *i915); void i915_teardown_sysfs(struct drm_i915_private *i915);
Now that we have tiles we want each of them to have its own interface. A directory "gt/" is created under "cardN/" that will contain as many diroctories as the tiles. In the coming patches tile related interfaces will be added. For now the sysfs gt structure simply has an id interface related to the current tile count. The directory structure will follow this scheme: /sys/.../card0 └── gt ├── gt0 │ └── id : : └─- gtN └── id 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@linux.intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> --- drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/gt/intel_gt.c | 2 + drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 118 +++++++++++++++++++++++ drivers/gpu/drm/i915/gt/intel_gt_sysfs.h | 34 +++++++ drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_sysfs.c | 12 ++- drivers/gpu/drm/i915/i915_sysfs.h | 3 + 7 files changed, 171 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs.c create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs.h