Re: [Intel-gfx] [PATCH v5 4/7] drm/i915/gt: create per-tile sysfs interface

Andi Shyti
2022-03-13T21:30:11Z

> > > > > +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