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