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

Sundaresan, Sujaritha
2022-02-17T17:07:25Z

On 2/17/2022 7:45 AM, Andi Shyti wrote:
> Hi,
>
> I forgot to add some note to this patch...
>
> [...]
>
>> +static ssize_t throttle_reason_status_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;
>> + bool status = !!intel_rps_read_throttle_reason_status(rps);
> why are these boolean? Can't we send whatever we read from the
> register?
Didn't want to report out the register mask value since the sysfs is
supposed to a 0/1 status.
>
> [...]
>
>> +#define GT0_PERF_LIMIT_REASONS _MMIO(0x1381A8)
>> +#define GT0_PERF_LIMIT_REASONS_MASK 0x00000de3
> This mask is really weird! Sujaritha, can you please explain it?
>
> It looks something like this,
>
> REG_GENMASK(11, 6) | REG_GENMASK(2, 0)
>
> But I don't know if it improves any readability, in any case, the
> mask is not clear.

This is meant to be an overall status flag as a one stop shop check for
any kind of throttling.

>
>> +#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)
> I hope I got these right. Sujaritha, can you please check?
>
> Andi

Yes these are correct.

Thanks,

Suja