Re: [v6] drm/xe: Add vram frequency sysfs attributes

["Sundaresan, Sujaritha" <sujaritha.sundaresan@intel.com>]

On 12/22/2023 7:34 PM, Gupta, Anshuman wrote:
>
>> -----Original Message-----
>> From: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
>> Sent: Friday, December 22, 2023 4:37 PM
>> To: intel-xe@lists.freedesktop.org
>> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Gupta, Anshuman
>> <anshuman.gupta@intel.com>; Sundaresan, Sujaritha
>> <sujaritha.sundaresan@intel.com>
>> Subject: [v6] drm/xe: Add vram frequency sysfs attributes
>>
>> Add vram frequency sysfs attributes under the below hierarchy;
>>
>> /device/tile#/memory/freq0
>> 			|-max_freq
>> 			|-min_freq
>>
>> v2: Drop "vram" from attribute names (Rodrigo)
>>
>> v3: Add documentation for new sysfs (Riana)
>>      Drop prefix from XEHP_PCODE_FREQUENCY_CONFIG (Riana)
>>
>> v4: Create sysfs under tile#/freq0 after removal of
>>      physical_memsize attrbute
>>
>> v5: Revert back to creating sysfs under tile#/memory/freq0
>>      Remove definition of GT_FREQUENCY_MULTIPLIER (Rodrigo)
>>
>> v6: Rename attributes to max/min_freq (Anshuman)
>>      Fix review comments (Rodrigo)
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_pcode_api.h  |  7 +++
>> drivers/gpu/drm/xe/xe_tile_sysfs.c | 86
>> ++++++++++++++++++++++++++++++
>>   2 files changed, 93 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h
>> b/drivers/gpu/drm/xe/xe_pcode_api.h
>> index 5935cfe30204..f153ce96f69a 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode_api.h
>> +++ b/drivers/gpu/drm/xe/xe_pcode_api.h
>> @@ -42,6 +42,13 @@
>>   #define	    POWER_SETUP_I1_SHIFT		6	/* 10.6 fixed
>> point format */
>>   #define	    POWER_SETUP_I1_DATA_MASK
>> 	REG_GENMASK(15, 0)
>>
>> +#define   PCODE_FREQUENCY_CONFIG		0x6e
>> +/* Frequency Config Sub Commands (param1) */
>> +#define     PCODE_MBOX_FC_SC_READ_FUSED_P0	0x0
>> +#define     PCODE_MBOX_FC_SC_READ_FUSED_PN	0x1
>> +/* Domain IDs (param2) */
>> +#define     PCODE_MBOX_DOMAIN_HBM		0x2
>> +
>>   struct pcode_err_decode {
>>   	int errno;
>>   	const char *str;
>> diff --git a/drivers/gpu/drm/xe/xe_tile_sysfs.c
>> b/drivers/gpu/drm/xe/xe_tile_sysfs.c
>> index 0f8d3e7fce46..cdc9dbbc97b0 100644
>> --- a/drivers/gpu/drm/xe/xe_tile_sysfs.c
>> +++ b/drivers/gpu/drm/xe/xe_tile_sysfs.c
>> @@ -7,9 +7,21 @@
>>   #include <linux/sysfs.h>
>>   #include <drm/drm_managed.h>
>>
>> +#include "xe_gt_types.h"
>> +#include "xe_pcode.h"
>> +#include "xe_pcode_api.h"
>>   #include "xe_tile.h"
>>   #include "xe_tile_sysfs.h"
>>
>> +/**
>> + * DOC: Xe Tile sysfs
>> + *
>> + * Provides sysfs entries for frequency in tile
>> + *
>> + * device/tile#/memory/freq0/max_freq - Maximum Frequency, not a
>> configuration and read-only.
> Let's increase verbosity of doc something explaining it is a fixed fuse point not a configuration.
Sure
>> + * device/tile#/memory/freq0/min_freq - Minimum Frequency, not a
>> configuration and read-only.
>> + */
>> +
>>   static void xe_tile_sysfs_kobj_release(struct kobject *kobj)  {
>>   	kfree(kobj);
>> @@ -20,6 +32,65 @@ static const struct kobj_type xe_tile_sysfs_kobj_type =
>> {
>>   	.sysfs_ops = &kobj_sysfs_ops,
>>   };
>>
>> +static ssize_t max_freq_show(struct device *kdev, struct device_attribute
>> *attr,
>> +			     char *buf)
>> +{
>> +	struct kobject *kobj = &kdev->kobj;
>> +	struct xe_tile *tile = kobj_to_tile(kobj->parent);
>> +	struct xe_gt *gt = tile->primary_gt;
>> +	u32 val, mbox;
>> +	int err;
>> +
>> +	mbox = REG_FIELD_PREP(PCODE_MB_COMMAND,
>> PCODE_FREQUENCY_CONFIG)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM1,
>> PCODE_MBOX_FC_SC_READ_FUSED_P0)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM2,
>> PCODE_MBOX_DOMAIN_HBM);
>> +
>> +	err = xe_pcode_read(gt, mbox, &val, NULL);
>> +	if (err)
>> +		return err;
>> +
>> +	/* data_out - Fused P0 for domain ID in units of 50 MHz */
>> +	val *= 50;
>> +
>> +	return sysfs_emit(buf, "%u\n", val);
>> +}
>> +static DEVICE_ATTR_RO(max_freq);
>> +
>> +static ssize_t min_freq_show(struct device *kdev, struct device_attribute
>> *attr,
>> +			     char *buf)
>> +{
>> +	struct kobject *kobj = &kdev->kobj;
>> +	struct xe_tile *tile = kobj_to_tile(kobj->parent);
> If you are missing to create a kobject for freq0 , then this should be
> kobj->parent->parent.

I don't think a kobject is needed for freq0, since it we are only using 
attribute_group for it.

Similar to throttle_reasons.

>> +	struct xe_gt *gt = tile->primary_gt;
>> +	u32 val, mbox;
>> +	int err;
>> +
>> +	mbox = REG_FIELD_PREP(PCODE_MB_COMMAND,
>> PCODE_FREQUENCY_CONFIG)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM1,
>> PCODE_MBOX_FC_SC_READ_FUSED_PN)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM2,
>> PCODE_MBOX_DOMAIN_HBM);
>> +
>> +	err = xe_pcode_read(gt, mbox, &val, NULL);
>> +	if (err)
>> +		return err;
>> +
>> +	/* data_out - Fused Pn for domain ID in units of 50 MHz */
>> +	val *= 50;
>> +
>> +	return sysfs_emit(buf, "%u\n", val);
>> +}
>> +static DEVICE_ATTR_RO(min_freq);
>> +
>> +static struct attribute *freq_attrs[] = {
>> +	&dev_attr_max_freq.attr,
>> +	&dev_attr_min_freq.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group freq_group_attrs = {
>> +	.name = "freq0",
>> +	.attrs = freq_attrs,
>> +};
>> +
>>   static void tile_sysfs_fini(struct drm_device *drm, void *arg)  {
>>   	struct xe_tile *tile = arg;
>> @@ -32,6 +103,7 @@ void xe_tile_sysfs_init(struct xe_tile *tile)
>>   	struct xe_device *xe = tile_to_xe(tile);
>>   	struct device *dev = xe->drm.dev;
>>   	struct kobj_tile *kt;
>> +	struct kobject *kobj;
>>   	int err;
>>
>>   	kt = kzalloc(sizeof(*kt), GFP_KERNEL); @@ -50,6 +122,20 @@ void
>> xe_tile_sysfs_init(struct xe_tile *tile)
>>
>>   	tile->sysfs = &kt->base;
>>
>> +	if (xe->info.platform == XE_PVC) {
>> +		kobj = kobject_create_and_add("memory", tile->sysfs);
> How freq0 is getting added, I am unable to see the freq0 kobject as per the path  "device/tile#/memory/freq0/"
freq0 is being added as an attribute group. It is similar to the 
throttle_reasons implementation.
>> +		if (!kobj)
>> +			drm_warn(&xe->drm, "failed to add memory
>> directory, err: %d\n", -ENOMEM);
>> +	}
>> +
>> +	if (kobj && xe->info.platform == XE_PVC) {
>> +		err = sysfs_create_group(kobj, &freq_group_attrs);
>> +		if (err) {
>> +			drm_warn(&xe->drm, "failed to register vram freq
>> sysfs, err: %d\n", err);
>> +			return;
>> +		}
>> +	}
> Don't we need sysfs cleanup and kobject_put in tile_sysfs_fini() ?
> Have you made sure kmemleak won't complain here on memory leak ?
>
> Thanks,
> Anshuman Gupta.

I have already checked for mem leaks during the cleanup. The kobject_put 
is not needed.

Thanks,

Suja

>> +
>>   	err = drmm_add_action_or_reset(&xe->drm, tile_sysfs_fini, tile);
>>   	if (err)
>>   		drm_warn(&xe->drm, "%s: drmm_add_action_or_reset failed,
>> err: %d\n",
>> --
>> 2.25.1