Re: [Intel-gfx] [PATCH v5 7/7] drm/i915/gt: Adding new sysfs frequency attributes

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

Hi Michal,

[...]

> > +static ssize_t punit_req_freq_mhz_show(struct device *dev,
> > +				       struct device_attribute *attr,
> > +				       char *buff)
> > +{
> > +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> > +	struct intel_rps *rps = &gt->rps;
> > +	u32 preq = intel_rps_read_punit_req_frequency(rps);
> > +
> > +	return scnprintf(buff, PAGE_SIZE, "%d\n", preq);
> 
> %u since preq is u32
> 
> and use sysfs_emit (also in below show functions)

sure! I'll change them.

[...]

> >  static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj,
> >  				const struct attribute * const *attrs)
> >  {
> > @@ -493,4 +628,11 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj)
> >  	if (ret)
> >  		drm_warn(&gt->i915->drm,
> >  			 "failed to create gt%u RPS sysfs files", gt->info.id);
> > +
> > +	ret = sysfs_create_files(kobj, freq_attrs);
> > +	if (ret)
> > +		drm_warn(&gt->i915->drm,
> > +			 "failed to create gt%u throttle sysfs files",
> > +			 gt->info.id);
> 
> nit: would be nice to see %pe why it failed

[...]

I will add it to the other cases as well.

> > +static u32 __rps_read_mmio(struct intel_gt *gt, i915_reg_t reg32)
> 
> this doesn't look like "rps" helper, rather like "gt" so it should have
> different prefix and maybe even be exported by the gt or uncore ?
> 
> unless you wanted:
> 
> static u32 __rps_read_mmio(struct intel_rps *rps, i915_reg_t reg32)
> {
> 	struct intel_gt *gt = rps_to_gt(rps);
> 
> > +{
> > +	intel_wakeref_t wakeref;
> > +	u32 val;
> > +
> > +	with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> > +		val = intel_uncore_read(gt->uncore, reg32);
> > +
> > +	return val;
> > +}

Yes, you are right!

@Sujaritha: shall I move "__rps_read_mmio()" in intel_gt.c and
call it intel_gt_read_mmio()?

[...]

> > +u32 intel_rps_read_throttle_reason_vr_thermalert(struct intel_rps *rps)
> > +{
> > +	struct intel_gt *gt = rps_to_gt(rps);
> > +	u32 thermalert = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & VR_THERMALERT_MASK;
> > +
> > +	return thermalert;
> > +}
> 
> shouldn't we return bool by all of these functions as used/expected in
> show() counterparts ?

Suja?

[...]

> > +#define GT0_PERF_LIMIT_REASONS		_MMIO(0x1381A8)
> > +#define   GT0_PERF_LIMIT_REASONS_MASK	0x00000de3
> 
> this mask is different that other (FIELD_PREP/GET wont work) so maybe we
> should name it in special way ?

As far as I understood this is still a mask and used as such.
This mask is actually telling that there is some throttling going
on.

It looks weird because there are some unwanted bits in between
the interesting bits.

> > +#define   PROCHOT_MASK			BIT(1)
> > +#define   THERMAL_LIMIT_MASK		BIT(2)
> > +#define   RATL_MASK			BIT(6)
> > +#define   VR_THERMALERT_MASK		BIT(7)
> > +#define   VR_TDC_MASK			BIT(8)
> > +#define   POWER_LIMIT_4_MASK		BIT(9)
> > +#define   POWER_LIMIT_1_MASK		BIT(11)
> > +#define   POWER_LIMIT_2_MASK		BIT(12)
> 
> REG_BIT ?

yes!

Thanks, Michal!
Andi