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

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

Hi Tvrtko,

> > The previous power management files are kept in their original
> > root directory to avoid breaking the ABI. They point to the tile
> > '0' and a warning message is printed whenever accessed to. The
> > deprecated interface needs for the CONFIG_SYSFS_DEPRECATED_V2
> > flag in order to be generated.
> 
> CONFIG_SYSFS_DEPRECATED_V2 idea was abandoned, no? This patch at least does
> not appear to contain it so please update the commit message to reflect
> current state.
> 
> Adding Joonas to help address the question of how strict are userspace
> requirements for sysfs additions. Personally sysadmin use sounds fine to me,
> although it needs to be mentioned/documented as Matt requested, but I fear
> it may not be enough to upstream. Is Level0 at the stage where we can
> upstream for it I am also not sure.

no need... it's a leftover in the commit message. The deprecated
was removed a year ago, maybe. Thanks!

[...]

> > +		pr_devel_ratelimited(DEPRECATED
> > +			"%s (pid %d) is trying to access deprecated %s "
> > +			"sysfs control, please use gt/gt<n>/%s instead\n",
> 
> Maybe reword "is trying to access" to "is accesing" to remove any doubt
> whether something is failing or not?

OK

[...]

> > +	if (sysfs_create_file(dir, &dev_attr_id.attr))
> > +		drm_err(&gt->i915->drm,
> > +			"failed to create sysfs %s info files\n", name);
> 
> These drm_err could maybe be warn or notice, to reflect the fact driver is
> most likely completely functional? But only maybe since I haven't checked
> what we do for other sysfs failures. If rest are drm_err then I guess
> consistency trumps it.

yes, I agree with you and indeed they whould be treated as
warnings because the driver probes correctly if sysfs fails.

I will change this to drm_warn and fixx all the others. Thanks!

[...]

> > +static inline bool is_object_gt(struct kobject *kobj)
> > +{
> > +	bool b = !strncmp(kobj->name, "gt", 2);
> > +
> > +	GEM_BUG_ON(b && !isdigit(kobj->name[2]));
> 
> 1)
> Not sure what is the point of this GEM_BUG_ON? Checking strncmp works feels
> like an overkill.
> 
> 2)
> With the recent trend of "static inline considered harmful" perhaps consider
> moving it out of line.

OK

[...]

> > +static const struct attribute_group rc6_attr_group[] = {
> > +	{ .name = power_group_name, .attrs = rc6_attrs },
> > +	{ .attrs = rc6_attrs }
> > +};
> > +
> > +static const struct attribute_group rc6p_attr_group[] = {
> > +	{ .name = power_group_name, .attrs = rc6p_attrs },
> > +	{ .attrs = rc6p_attrs }
> > +};
> > +
> > +static const struct attribute_group media_rc6_attr_group[] = {
> > +	{ .name = power_group_name, .attrs = media_rc6_attrs },
> > +	{ .attrs = media_rc6_attrs }
> > +};
> > +
> > +static int __intel_gt_sysfs_create_group(struct kobject *kobj,
> > +					 const struct attribute_group *grp)
> > +{
> > +	int i = is_object_gt(kobj);
> 
> Is_object_gt returns a bool so can I be pedantic? :) Maybe just omit the
> local and solve it like that.

'i' is used also as an array index here, and callse merge/create
depending on what kind of object kobj is.

Maybe it's a bit of an ugly trick, but perhaps to mark the fact I
can do it like

	i = !!is_object_gt(kobj);

> > +
> > +	return i ? sysfs_create_group(kobj, &grp[i]) :
> > +		   sysfs_merge_group(kobj, &grp[i]);
> > +}
> > +
> > +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj)
> > +{
> > +	int ret;
> > +
> > +	if (!HAS_RC6(gt->i915))
> > +		return;
> > +
> > +	ret = __intel_gt_sysfs_create_group(kobj, rc6_attr_group);
> > +	if (ret)
> > +		drm_err(&gt->i915->drm,
> > +			"failed to create gt%u RC6 sysfs files\n", gt->info.id);
> > +
> > +	if (HAS_RC6p(gt->i915)) {
> > +		ret = __intel_gt_sysfs_create_group(kobj, rc6p_attr_group);
> > +		if (ret)
> > +			drm_err(&gt->i915->drm,
> > +				"failed to create gt%u RC6p sysfs files\n",
> > +				gt->info.id);
> > +	}
> > +
> > +	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) {
> > +		ret = __intel_gt_sysfs_create_group(kobj, media_rc6_attr_group);
> > +		if (ret)
> > +			drm_err(&gt->i915->drm,
> > +				"failed to create media %u RC6 sysfs files\n",
> > +				gt->info.id);
> > +	}
> > +}

[...]

> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -8480,6 +8480,7 @@ enum {
> >   #define   GEN6_AGGRESSIVE_TURBO			(0 << 15)
> >   #define   GEN9_SW_REQ_UNSLICE_RATIO_SHIFT	23
> >   #define   GEN9_IGNORE_SLICE_RATIO		(0 << 0)
> > +#define   GEN12_SW_REQ_UNSLICE_RATIO_SHIFT	23
> 
> Stray something?

I don't know how this ended up here... scary!

> Regards,
> 
> Tvrtko

Thanks a lot for looking again into this!

Andi