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

Sundaresan, Sujaritha
2022-03-14T01:32:14Z

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