Re: [Intel-gfx] [PATCH 7/8] drm/i915/gt: Expose per-gt RPS defaults in sysfs

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 10 May 2022 03:58:13 -0700, Andi Shyti wrote:
>
> Hi Ashutosh,

Hi Andi,

> > > +static ssize_t
> > > +default_min_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > > +{
> > > +	struct intel_gt *gt = kobj_to_gt(kobj->parent);
> > > +
> > > +	return sysfs_emit(buf, "%d\n", gt->rps_defaults.min_freq);
>
> I guess this is %u.

Fixed in v2.

> > > +}
> > > +
> > > +static struct kobj_attribute default_min_freq_mhz =
> > > +__ATTR(rps_min_freq_mhz, 0444, default_min_freq_mhz_show, NULL);
> > > +
> > > +static ssize_t
> > > +default_max_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > > +{
> > > +	struct intel_gt *gt = kobj_to_gt(kobj->parent);
> > > +
> > > +	return sysfs_emit(buf, "%d\n", gt->rps_defaults.max_freq);
> > > +}
> > > +
> > > +static struct kobj_attribute default_max_freq_mhz =
> > > +__ATTR(rps_max_freq_mhz, 0444, default_max_freq_mhz_show, NULL);
> > > +
> > > +static ssize_t
> > > +default_boost_freq_mhz_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > > +{
> > > +	struct intel_gt *gt = kobj_to_gt(kobj->parent);
> > > +
> > > +	return sysfs_emit(buf, "%d\n", gt->rps_defaults.boost_freq);
> > > +}
> > > +
> > > +static struct kobj_attribute default_boost_freq_mhz =
> > > +__ATTR(rps_boost_freq_mhz, 0444, default_boost_freq_mhz_show, NULL);
> > > +
> > > +static const struct attribute * const rps_defaults_attrs[] = {
> > > +	&default_min_freq_mhz.attr,
> > > +	&default_max_freq_mhz.attr,
> > > +	&default_boost_freq_mhz.attr,
> > > +	NULL
> > > +};
>
> Do you think this in the default group of kobj_gt_type like the
> gt_id?

gt_id I think fits well in the category of a "default" attribute for a
gt. I am not sure if these attributes similarly qualify as "default"
attributes (for the .defaults sysfs), what do you think? That is why I have
followed what is done in the engine sysfs which also does not use default
attributes. But we can revisit based on your comments.

>
> [...]
>
> > > +struct intel_rps_defaults {
> > > +	u32 min_freq;
> > > +	u32 max_freq;
> > > +	u32 boost_freq;
> > > +};
> > > +
> > >   enum intel_submission_method {
> > >		INTEL_SUBMISSION_RING,
> > >		INTEL_SUBMISSION_ELSP,
> > > @@ -227,6 +233,10 @@ struct intel_gt {
> > >		/* gt/gtN sysfs */
> > >		struct kobject sysfs_gt;
> > > +
> > > +	/* sysfs defaults per gt */
> > > +	struct intel_rps_defaults rps_defaults;
>
> more of a matter of taste, but this looks natural to me to be in
> rps rather then in the gt.

In v2 I have changed the name of this from 'struct intel_rps_defaults' to
'struct gt_defaults'. So the idea is to have a central place in the gt
where any "module" (RPS, PM, ...) can store their defaults which are then
exposed via sysfs files in gt/gtN/.defaults directory. There are multiple
ways of doing this but this is what I've done in this series, basically to
keep things simple.

Thanks.
--
Ashutosh