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

["Sundaresan, Sujaritha" <sujaritha.sundaresan@xxxxxxxxx>]

On 3/13/2022 5:38 PM, Andi Shyti wrote:
> 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()?
>
> [...]
Sure since it is kind of a gt helper, makes sense to have gt prefix.
> > > +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?
>
> [...]
Yes we can make this bool as well.
> > > +#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