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

[Sundaresan, Sujaritha]

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