[Intel-xe] [PATCH 1/1] [RFC]drm/xe: Adding throttle reasons sysfs attributes

[Thomas Hellström]

Hi,

Some style comments:

On 5/30/23 12:34, Sujaritha Sundaresan wrote:
> Adding throttle reasons sysfs attributes.

Please use imperative language as per the linux patch submission 
guidelines: "Add throttle..."

Also please add a message explaining why / for what these sysfs 
attributes are needed.


>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
> ---
>   drivers/gpu/drm/xe/regs/xe_gt_regs.h | 12 +++++
>   drivers/gpu/drm/xe/xe_guc_pc.c       | 67 ++++++++++++++++++++++++++++
>   2 files changed, 79 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> index e64e5c0733d6..e7fe8a3e348e 100644
> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> @@ -374,4 +374,16 @@
>   #define XEHPC_BCS5_BCS6_INTR_MASK		XE_REG(0x190118)
>   #define XEHPC_BCS7_BCS8_INTR_MASK		XE_REG(0x19011c)
>   
> +#define GT0_PERF_LIMIT_REASONS			XE_REG(0x1381a8)
> +#define   GT0_PERF_LIMIT_REASONS_MASK		0xde3
> +#define   PROCHOT_MASK				REG_BIT(0)
> +#define   THERMAL_LIMIT_MASK			REG_BIT(1)
> +#define   RATL_MASK				REG_BIT(5)
> +#define   VR_THERMALERT_MASK			REG_BIT(6)
> +#define   VR_TDC_MASK				REG_BIT(7)
> +#define   POWER_LIMIT_4_MASK			REG_BIT(8)
> +#define   POWER_LIMIT_1_MASK			REG_BIT(10)
> +#define   POWER_LIMIT_2_MASK			REG_BIT(11)
> +#define MTL_MEDIA_PERF_LIMIT_REASONS		XE_REG(0x138030)
> +

Documentation, please.


>   #endif
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index e799faa1c6b8..363332881e4d 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> @@ -638,6 +638,73 @@ static const struct attribute *pc_attrs[] = {
>   	NULL
>   };
>   
> +static inline
> +xe_reg_t xe_gt_perf_limit_reasons_reg(struct xe_gt *gt)
> +{
> +        if (xe_gt_is_media_type(gt))
> +                return MTL_MEDIA_PERF_LIMIT_REASONS;
> +
> +        return GT0_PERF_LIMIT_REASONS;
> +}
> +

Only use "static inline" in headers. Otherwise let the compiler decide 
what to inline.

> +struct xe_gt_bool_throttle_attr {
> +	struct attribute attr;
> +	ssize_t (*show)(struct device *dev, struct device_attribute *attr,
> +			char *buf);
> +	xe_reg_t (*reg32)(struct xe_gt *gt);
> +	u32 mask;
> +};
> +

Struct needs kerneldoc.


> +bool xe_mmio_mask_read(struct xe_gt *gt,
> +			xe_reg_t reg32, u32 mask)
> +{
> +	return xe_mmio_read32(gt, reg32) & mask;
> +}
> +

Extern function needs kerneldoc, but does it need to be extern?

> +static ssize_t throttle_reason_bool_show(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buff)
> +{
> +	struct kobject *kobj = &dev->kobj;
> +	struct xe_gt *gt = kobj_to_gt(kobj);
> +	struct xe_gt_bool_throttle_attr *t_attr =
> +			(struct xe_gt_bool_throttle_attr *) attr;
> +	bool val = xe_mmio_mask_read(&gt->rps, t_attr->reg32(gt), t_attr->mask);
> +
> +	return sysfs_emit(buff, "%u\n", val);
> +}
> +
> +#define XE_GT_BOOL_ATTR_RO(sysfs_func__, mask__) \
> +struct xe_gt_bool_throttle_attr attr_##sysfs_func__ = { \
> +	.attr = { .name = __stringify(sysfs_func__), .mode = 0444 }, \
> +	.show = throttle_reason_bool_show, \
> +	.reg32 = xe_gt_perf_limit_reasons_reg, \
> +	.mask = mask__, \
> +}
> +
> +static XE_GT_BOOL_ATTR_RO(throttle_reason_status, GT0_PERF_LIMIT_REASONS_MASK);
> +static XE_GT_BOOL_ATTR_RO(throttle_reason_pl1, POWER_LIMIT_1_MASK);
> +static XE_GT_BOOL_ATTR_RO(throttle_reason_pl2, POWER_LIMIT_2_MASK);
> +static XE_GT_BOOL_ATTR_RO(throttle_reason_pl4, POWER_LIMIT_4_MASK);
> +static XE_GT_BOOL_ATTR_RO(throttle_reason_thermal, THERMAL_LIMIT_MASK);
> +static XE_GT_BOOL_ATTR_RO(throttle_reason_prochot, PROCHOT_MASK);
> +static XE_GT_BOOL_ATTR_RO(throttle_reason_ratl, RATL_MASK);
> +static XE_GT_BOOL_ATTR_RO(throttle_reason_vr_thermalert, VR_THERMALERT_MASK);
> +static XE_GT_BOOL_ATTR_RO(throttle_reason_vr_tdc, VR_TDC_MASK);
> +
> +static const struct attribute *throttle_reason_attrs[] = {
> +	&dev_attr_throttle_reason_status.attr,
> +	&dev_attr_throttle_reason_pl1.attr,
> +	&dev_attr_throttle_reason_pl2.attr,
> +	&dev_attr_throttle_reason_pl4.attr,
> +	&dev_attr_throttle_reason_thermal.attr,
> +	&dev_attr_throttle_reason_prochot.attr,
> +	&dev_attr_throttle_reason_ratl.attr,
> +	&dev_attr_throttle_reason_vr_thermalert.attr,
> +	&dev_attr_throttle_reason_vr_tdc.attr,
> +	NULL
> +};
> +
>   static void mtl_init_fused_rp_values(struct xe_guc_pc *pc)
>   {
>   	struct xe_gt *gt = pc_to_gt(pc);