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

Andrzej Hajda
2022-03-07T20:25:27Z

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