Re: [Intel-xe] [PATCH v2 2/2] drm/xe: Add base performance and vram frequency sysfs attributes

[Riana Tauro <riana.tauro@intel.com>]

Hi Suja

Could you add the folder structure in commit message

On 9/26/2023 11:01 PM, Rodrigo Vivi wrote:
> On Tue, Sep 26, 2023 at 08:11:20PM +0530, Sujaritha Sundaresan wrote:
>> This patch adds base performace attributes as well as vram frequency
>> attributes.
> 
%/performace/performance
> Is vram freq tied to the gt or tied to the tile?
> 
>>
>> v2: Create a separate performance directory for attributes.
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> ---
>>   drivers/gpu/drm/xe/Makefile           |   1 +
>>   drivers/gpu/drm/xe/xe_gt_freq_sysfs.c | 218 ++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_gt_freq_sysfs.h |  16 ++
>>   drivers/gpu/drm/xe/xe_gt_sysfs.c      |   3 +
>>   drivers/gpu/drm/xe/xe_pcode_api.h     |  19 +++
>>   5 files changed, 257 insertions(+)
>>   create mode 100644 drivers/gpu/drm/xe/xe_gt_freq_sysfs.c
>>   create mode 100644 drivers/gpu/drm/xe/xe_gt_freq_sysfs.h
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index b1681d1416eb..bdd9922d3db5 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -60,6 +60,7 @@ xe-y += xe_bb.o \
>>   	xe_gt.o \
>>   	xe_gt_clock.o \
>>   	xe_gt_debugfs.o \
>> +	xe_gt_freq_sysfs.o \
>>   	xe_gt_idle_sysfs.o \
>>   	xe_gt_mcr.o \
>>   	xe_gt_pagefault.o \
>> diff --git a/drivers/gpu/drm/xe/xe_gt_freq_sysfs.c b/drivers/gpu/drm/xe/xe_gt_freq_sysfs.c
There are few attributes under tile, few under gt.
Should this have a common name instead of gt prefix?
>> new file mode 100644
>> index 000000000000..6017d09eeca2
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_gt_freq_sysfs.c
>> @@ -0,0 +1,218 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#include <drm/drm_managed.h>
>> +
>> +#include <regs/xe_gt_regs.h>
>> +#include "xe_device.h"
>> +#include "xe_gt.h"
>> +#include "xe_gt_freq_sysfs.h"
>> +#include "xe_gt_sysfs.h"
>> +#include "xe_pcode.h"
>> +#include "xe_pcode_api.h"
>> +
>> +#define GT_FREQUENCY_MULTIPLIER	50
>> +
>> +#define U8_8_VAL_MASK           0xffff
>> +#define U8_8_SCALE_TO_VALUE     "0.00390625"
>> +
>> +static ssize_t freq_factor_scale_show(struct device *dev,
>> +				      struct device_attribute *attr,
>> +				      char *buff)
>> +{
>> +	return sysfs_emit(buff, "%s\n", U8_8_SCALE_TO_VALUE);
>> +}
>> +
>> +static ssize_t base_freq_factor_show(struct device *dev,
>> +				     struct device_attribute *attr,
>> +				     char *buff)
>> +{
>> +	struct kobject *kobj = &dev->kobj;
>> +	struct xe_gt *gt = kobj_to_gt(kobj);
>> +	u32 val;
>> +	int err;
>> +
>> +	err = xe_gt_pcode_read(gt, PVC_PCODE_QOS_MULTIPLIER_GET,
>> +				PCODE_MBOX_DOMAIN_CHIPLET,
>> +				PCODE_MBOX_DOMAIN_BASE, &val);
>> +	if (err)
>> +		return err;
>> +
>> +	val &= U8_8_VAL_MASK;
>> +
>> +	return sysfs_emit(buff, "%u\n", val);
>> +}
>> +
>> +static ssize_t base_freq_factor_store(struct device *dev,
>> +				      struct device_attribute *attr,
>> +				      const char *buff, size_t count)
>> +{
>> +	struct kobject *kobj = &dev->kobj;
>> +	struct xe_gt *gt = kobj_to_gt(kobj);
>> +	u32 val;
>> +	int err;
>> +
>> +	err = kstrtou32(buff, 0, &val);
>> +	if (err)
>> +		return err;
>> +
>> +	if (val > U8_8_VAL_MASK)
>> +		return -EINVAL;
>> +
>> +	err = xe_gt_pcode_write(gt, PVC_PCODE_QOS_MULTIPLIER_SET,
>> +				PCODE_MBOX_DOMAIN_CHIPLET,
>> +				PCODE_MBOX_DOMAIN_BASE, val);
>> +	if (err)
>> +		return err;
>> +
>> +	return count;
>> +}
>> +
>> +static DEVICE_ATTR_RW(base_freq_factor);
>> +static struct device_attribute dev_attr_base_freq_factor_scale =
>> +	__ATTR(base_freq_factor.scale, 0444, freq_factor_scale_show, NULL);
>> +
>> +
>> +static ssize_t base_freq_rp0_show(struct device *dev, struct device_attribute *attr,
>> +				  char *buff)
>> +{
>> +	struct kobject *kobj = &dev->kobj;
>> +	struct xe_gt *gt = kobj_to_gt(kobj);
>> +	u32 val;
>> +	int err;
>> +
>> +	err = xe_gt_pcode_read(gt, XEHP_PCODE_FREQUENCY_CONFIG,
>> +				PCODE_MBOX_FC_SC_READ_FUSED_P0,
>> +				PCODE_MBOX_DOMAIN_BASE, &val);
>> +	if (err)
>> +		return err;
>> +
>> +	/* data_out - Fused P0 for domain ID in units of 50 MHz */
>> +	val *= GT_FREQUENCY_MULTIPLIER;
>> +
>> +	return sysfs_emit(buff, "%u\n", val);
>> +}
>> +static DEVICE_ATTR_RO(base_freq_rp0);
>> +
>> +static ssize_t base_freq_rpn_show(struct device *dev, struct device_attribute *attr,
>> +				  char *buff)
>> +{
>> +	struct kobject *kobj = &dev->kobj;
>> +	struct xe_gt *gt = kobj_to_gt(kobj);
>> +	u32 val;
>> +	int err;
>> +
>> +	err = xe_gt_pcode_read(gt, XEHP_PCODE_FREQUENCY_CONFIG,
>> +				PCODE_MBOX_FC_SC_READ_FUSED_PN,
>> +				PCODE_MBOX_DOMAIN_BASE, &val);
>> +	if (err)
>> +		return err;
>> +
>> +	/* data_out - Fused Pn for domain ID in units of 50 MHz */
>> +	val *= GT_FREQUENCY_MULTIPLIER;
>> +
>> +	return sysfs_emit(buff, "%u\n", val);
>> +}
>> +static DEVICE_ATTR_RO(base_freq_rpn);
>> +
>> +static const struct attribute *perf_power_attrs[] = {
>> +	&dev_attr_base_freq_factor.attr,
>> +	&dev_attr_base_freq_factor_scale.attr,
>> +	&dev_attr_base_freq_rp0.attr,
>> +	&dev_attr_base_freq_rpn.attr,
>> +	NULL
>> +};
>> +
>> +static ssize_t freq_vram_rp0_show(struct device *dev, struct device_attribute *attr,
>> +				  char *buff)
>> +{
>> +	struct kobject *kobj = &dev->kobj;
>> +	struct xe_gt *gt = kobj_to_gt(kobj);
>> +	u32 val;
>> +	int err;
>> +
>> +	err = xe_gt_pcode_read(gt, XEHP_PCODE_FREQUENCY_CONFIG,
>> +				PCODE_MBOX_FC_SC_READ_FUSED_P0,
>> +				PCODE_MBOX_DOMAIN_HBM, &val);
>> +	if (err)
>> +		return err;
>> +
>> +	/* data_out - Fused P0 for domain ID in units of 50 MHz */
>> +	val *= GT_FREQUENCY_MULTIPLIER;
>> +
>> +	return sysfs_emit(buff, "%u\n", val);
>> +}
>> +static DEVICE_ATTR_RO(freq_vram_rp0);
>> +
>> +static ssize_t freq_vram_rpn_show(struct device *dev, struct device_attribute *attr,
>> +				  char *buff)
>> +{
>> +	struct kobject *kobj = &dev->kobj;
>> +	struct xe_gt *gt = kobj_to_gt(kobj);
>> +	u32 val;
>> +	int err;
>> +
>> +	err = xe_gt_pcode_read(gt, XEHP_PCODE_FREQUENCY_CONFIG,
>> +				PCODE_MBOX_FC_SC_READ_FUSED_PN,
>> +				PCODE_MBOX_DOMAIN_HBM, &val);
>> +	if (err)
>> +		return err;
>> +
>> +	/* data_out - Fused P0 for domain ID in units of 50 MHz */
>> +	val *= GT_FREQUENCY_MULTIPLIER;
>> +
>> +	return sysfs_emit(buff, "%u\n", val);
>> +}
>> +static DEVICE_ATTR_RO(freq_vram_rpn);
>> +
>> +static const struct attribute *vram_freq_attrs[] = {
>> +	&dev_attr_freq_vram_rp0.attr,
>> +	&dev_attr_freq_vram_rpn.attr,
>> +	NULL
>> +};
>> +
>> +static void gt_freq_sysfs_fini(struct drm_device *drm, void *arg)
>> +{
>> +	struct kobject *kobj = arg;
>> +
>> +	sysfs_remove_files(kobj, perf_power_attrs);
>> +	kobject_put(kobj);
>> +}
>> +
>> +void xe_gt_freq_sysfs_init(struct xe_gt *gt)
>> +{
>> +	struct xe_tile *tile = gt_to_tile(gt);
>> +	struct xe_device *xe = gt_to_xe(gt);
>> +	struct kobject *kobj;
>> +	int err;
>> +
>> +	kobj = kobject_create_and_add("performance", gt->sysfs);
>> +	if (!kobj) {
>> +		drm_warn(&xe->drm, "%s failed, err: %d\n", __func__, -ENOMEM);
>> +		return;
>> +	}
>> +
>> +	err = sysfs_create_files(kobj, perf_power_attrs);
>> +	if (err) {
>> +		kobject_put(kobj);
>> +		drm_warn(&xe->drm, "failed to register performance power sysfs, err: %d\n", err);
>> +		return;
>> +	}
>> +
>> +	if (xe->info.platform == XE_PVC) {
>> +		err = sysfs_create_files(tile->sysfs, vram_freq_attrs);
>> +		if (err) {
>> +			kobject_put(kobj);
>> +			drm_warn(&xe->drm, "failed to register vram freq sysfs, err: %d\n", err);
>> +			return;
>> +		}
>> +
>> +	}
I'm seeing PVC prefix for perf_power_attrs above but you have added 
vram_freq_attrs under pvc. Shouldn't it be the other way?
>> +
>> +	err = drmm_add_action_or_reset(&xe->drm, gt_freq_sysfs_fini, kobj);
>> +	if (err)
>> +		drm_warn(&xe->drm, "%s: drmm_add_action_or_reset failed, err: %d\n",
>> +			 __func__, err);
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_gt_freq_sysfs.h b/drivers/gpu/drm/xe/xe_gt_freq_sysfs.h
>> new file mode 100644
>> index 000000000000..7b76c4670632
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_gt_freq_sysfs.h
>> @@ -0,0 +1,16 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_GT_FREQ_SYSFS_H_
>> +#define _XE_GT_FREQ_SYSFS_H_
>> +
>> +#include <drm/drm_managed.h>
>> +
>> +#include "xe_device.h"
>> +#include "xe_gt.h"
>> +
>> +void xe_gt_freq_sysfs_init(struct xe_gt *gt);
>> +
>> +#endif /* _XE_GT_FREQ_SYSFS_H_ */
>> diff --git a/drivers/gpu/drm/xe/xe_gt_sysfs.c b/drivers/gpu/drm/xe/xe_gt_sysfs.c
>> index c69d2e8a0fe1..3b6316ce10ed 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sysfs.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_sysfs.c
>> @@ -11,6 +11,7 @@
>>   #include <drm/drm_managed.h>
>>   
>>   #include "xe_gt.h"
>> +#include "xe_gt_freq_sysfs.h"
>>   
>>   static void xe_gt_sysfs_kobj_release(struct kobject *kobj)
>>   {
>> @@ -52,6 +53,8 @@ void xe_gt_sysfs_init(struct xe_gt *gt)
>>   
>>   	gt->sysfs = &kg->base;
>>   
>> +	xe_gt_freq_sysfs_init(gt);
>> +
>>   	err = drmm_add_action_or_reset(&xe->drm, gt_sysfs_fini, gt);
>>   	if (err) {
>>   		drm_warn(&xe->drm, "%s: drmm_add_action_or_reset failed, err: %d\n",
>> diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h b/drivers/gpu/drm/xe/xe_pcode_api.h
>> index 837ff7c71280..da4114bfaa7a 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode_api.h
>> +++ b/drivers/gpu/drm/xe/xe_pcode_api.h
>> @@ -30,6 +30,25 @@
>>   #define   PCODE_READ_MIN_FREQ_TABLE	0x9
>>   #define   PCODE_FREQ_RING_RATIO_SHIFT	16
>>   
>> +#define   XEHP_PCODE_FREQUENCY_CONFIG			0x6e	/* xehp, pvc */
>> +/* XEHP_PCODE_FREQUENCY_CONFIG sub-commands (param1) */
>> +#define     PCODE_MBOX_FC_SC_READ_FUSED_P0		0x0
>> +#define     PCODE_MBOX_FC_SC_READ_FUSED_PN		0x1
>> +/* PCODE_MBOX_DOMAIN_* - mailbox domain IDs */
>> +/* XEHP_PCODE_FREQUENCY_CONFIG param2 */
>> +#define     PCODE_MBOX_DOMAIN_NONE			0x0
>> +#define     PCODE_MBOX_DOMAIN_GT			0x1
>> +#define     PCODE_MBOX_DOMAIN_HBM			0x2
>> +#define     PCODE_MBOX_DOMAIN_MEDIAFF			0x3
>> +#define     PCODE_MBOX_DOMAIN_MEDIA_SAMPLER		0x4
>> +#define     PCODE_MBOX_DOMAIN_SYSTOLIC_ARRAY		0x5
>> +#define     PCODE_MBOX_DOMAIN_CHIPLET			0x6
>> +#define     PCODE_MBOX_DOMAIN_BASE_CHIPLET_LINK		0x7
>> +#define     PCODE_MBOX_DOMAIN_BASE			0x8
>> +#define   PVC_PCODE_QOS_MULTIPLIER_SET			0x67
Add only the mailboxes that are being used.

Thanks
Riana Tauro
>> +/* See PCODE_MBOX_DOMAIN_* - mailbox domain IDs - param1 and 2 */
>> +#define   PVC_PCODE_QOS_MULTIPLIER_GET			0x66
>> +
>>   /* PCODE Init */
>>   #define   DGFX_PCODE_STATUS		0x7E
>>   #define     DGFX_GET_INIT_STATUS	0x0
>> -- 
>> 2.25.1
>>