[Intel-gfx] [PATCH v2] drm/i915/gt: make a gt sysfs group and move power management files

[Tvrtko Ursulin]

On 11/02/2020 21:06, Andi Shyti wrote:
> Hi Tvrtko,
> 
>>> +void intel_gt_sysfs_register(struct intel_gt *gt)
>>> +{
>>> +	struct kobject *parent = kobject_get(&gt->i915->drm.primary->kdev->kobj);
>>
>> Does this needs a kobject_put to balance out somewhere?
> 
> Yes, I forgot the two kobject_put that are needed.
> 
>>> +	int ret;
>>> +
>>> +	gt->kobj = kobject_create_and_add("gt", parent);
>>> +	if (!gt->kobj) {
>>> +		pr_err("failed to initialize sysfs file\n");
>>> +		return;
>>> +	}
>>> +
>>> +	dev_set_drvdata(kobj_to_dev(gt->kobj), gt);
>>> +
>>> +	ret = sysfs_create_files(gt->kobj, gt_attrs);
>>> +	if (ret)
>>> +		pr_err("failed to create sysfs gt info files\n");
>>
>> I can't remember which log helper takes in the device and prefixes that
>> string but I think it could be useful here, since the error is otherwise
>> eaten.
> 
> pr_* is used a lot in gt/. Playing with the dev pointer I
> can use "dev_err(dev,...)"
> 
>>> +void intel_gt_sysfs_unregister(struct intel_gt *gt)
>>> +{
>>> +	if (!gt->kobj)
>>> +		return;
>>> +
>>> +	intel_gt_sysfs_pm_remove(gt, gt->kobj);
>>> +	sysfs_remove_files(gt->kobj, gt_attrs);
>>
>> Why is this asymmetrical to creation?
> 
> Because in V1 gt_attrs and whatever was created in sysfs_gt_pm
> was in the same group, but it desn't matter.
> 
>> I mean you call intel_gt_sysfs_pm_init
>> two times with different roots, so why not intel_gt_sysfs_pm_remove also
>> twice with different roots?
> 
> Because I forgot them in the cleanups :)

Next version incoming soon? :)

>>> +	/*
>>> +	 * 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 (strcmp(kobj->name, "gt")) {
>>> +		pr_warn("the interface is obsolete, use gt/\n");
>>
>> I think the message will need to be a bit more verbose since it is intended
>> for users. I don't have any suggestions straight away apart from googling to
>> find similar examples from the past and other subsystems.
> 
> Yes, I couldn't come up with a better message in 80 characters,
> and I can use dev_warn instead of pr_warn.

Maybe we even need to log this once. Well we may need to log the 
offending process name/pid. Not sure if we can manage once on top of 
that.. sounds too hard. So maybe just name/pid.

> 
>>> +static ssize_t rc6_enable_show(struct device *dev,
>>> +			       struct device_attribute *attr,
>>> +			       char *buff)
>>> +{
>>> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
>>
>> The rest of code is unchanged apart from this same line in all show/store
>> vfuncs?
> 
> yes, more or less.

Cool, just so I know what I don't have to cross-reference too much.

Regards,

Tvrtko