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

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

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(&gt->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