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

[Andrzej Hajda <andrzej.hajda@intel.com>]

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