Re: [Intel-gfx] [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface

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

Hi Andrzej,

> > Now tiles have their own sysfs interfaces under the gt/
> > directory. Because RC6 is a property that can be configured on a
> > tile basis, then each tile should have its own interface
> > 
> > The new sysfs structure will have a similar layout for the 4 tile
> > case:
> > 
> > /sys/.../card0
> >           ├── gt
> >           │   ├── gt0
> >           │   │   ├── id
> >           │   │   ├── rc6_enable
> >           │   │   ├── rc6_residency_ms
> >           .   .   .
> >           .   .   .
> >           .   .
> >           │   └── gtN
> >           │       ├── id
> >           │       ├── rc6_enable
> >           │       ├── rc6_residency_ms
> >           │       .
> >           │       .
> >           │
> >           └── power/                -+
> >                ├── rc6_enable        |    Original interface
> >                ├── rc6_residency_ms  +->  kept as existing ABI;
> >                .                     |    it multiplexes over
> >                .                     |    the GTs
> >                                     -+
> > 
> > The existing interfaces have been kept in their original location
> > to preserve the existing ABI. They act on all the GTs: when
> > reading they provide the average value from all the GTs.
> 
> If ABI should be kept forever, depreciation msg should be removed from
> intel_gt_sysfs_get_drvdata.

yes... to be removed!

> > +#ifdef CONFIG_PM
> > +static s64
> > +sysfs_gt_attribute_r_func(struct device *dev, struct device_attribute *attr,
> > +			  s64 (func)(struct intel_gt *gt))
> > +{
> > +	struct intel_gt *gt;
> > +	s64 ret = 0;
> > +
> > +	if (!is_object_gt(&dev->kobj)) {
> > +		int i, num_gt = 0;
> > +		struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> > +
> > +		for_each_gt(gt, i915, i) {
> > +			ret += func(gt);
> > +			num_gt++;
> > +		}
> > +
> > +		ret /= num_gt;
> > +	} else {
> > +		gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> > +		ret = func(gt);
> > +	}
> > +
> > +	return ret;
> > +}
> 
> Return value is always converted to u32 or int, maybe we could use just int
> ?

there is one case when the value to be returned is an 'int' and
that is the "int intel_gpu_freq()". That's why I supposed that
s64 was the right size. But I don't see how it can be negative so
that I will make it u32. Perhaps we need to change intel_gpu_freq
to be u32.

(Didn't want to take it too far, but I was also thinking that in
the future we might need to return error values)

> > +static ssize_t rc6_residency_ms_show(struct device *dev,
> > +				     struct device_attribute *attr,
> > +				     char *buff)
> > +{
> > +	s64 rc6_residency = sysfs_gt_attribute_r_func(dev, attr,
> > +						      __rc6_residency_ms_show);
> > +
> > +	return sysfs_emit(buff, "%u\n", (u32) rc6_residency);
> 
> Here and below (where applicable) variable should be just u32, no need to
> conversion in sysfs_emit.

yep! same comment as above.

> > +static int __intel_gt_sysfs_create_group(struct kobject *kobj,
> > +					 const struct attribute_group *grp)
> > +{
> > +	return is_object_gt(kobj) ?
> > +	       sysfs_create_group(kobj, &grp[0]) :
> > +	       sysfs_merge_group(kobj, &grp[1]);
> > +}
> 
> Merging handling "gt/gt#/*" and "power/*" attributes into the same helpers
> seems unnatural to me, in many functions we have two branches based on value
> of is_object_gt, with the most hacky intel_gt_sysfs_get_drvdata.
> Splitting handling these attributes would allow to drop fragile is_object_gt
> helper and make functions more straightforward, probably at the cost of few
> lines more. On the other side I am not sure if it is worth to change
> everything to just address code purity concerns :)

I the amount of duplicated code would be too high and there have
been already complaints some times in the past (e.g. we have had
same discussion in the case of the debugfs files).

But it's true that is quite hard to find the correct balance
between duplicated code and compact code.

> So with variable type adjustement:
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Thanks!