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

[Andi Shyti <andi.shyti@linux.intel.com>]

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