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

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

[Sujaritha Sundaresan]

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.
+ * 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);
+	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);
+		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;
+		}
+	}
+
 	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

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

[Gupta, Anshuman]

> -----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.
> + * 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.
> +	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/"
> +		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. 
> +
>  	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

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

[Sundaresan, Sujaritha]

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

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

[Gupta, Anshuman]

> -----Original Message-----
> From: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
> Sent: Tuesday, December 26, 2023 10:02 AM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
> xe@lists.freedesktop.org
> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Subject: Re: [v6] drm/xe: Add vram frequency sysfs attributes
> 
> 
> 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.
Sure,  thanks for explanation.
> 
> >> +	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 ?
Check the kobject_release() , this will only get call , when all ref count of that kobj is being put.
Here you are creating a object and it will never be released.
Also  we need to call  the sysfs_remove_group() as well in fini function ?

Thanks,
Anshuman.

> >
> > 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

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

[Sundaresan, Sujaritha]

On 12/26/2023 7:58 PM, Gupta, Anshuman wrote:
>
>> -----Original Message-----
>> From: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
>> Sent: Tuesday, December 26, 2023 10:02 AM
>> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
>> xe@lists.freedesktop.org
>> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>> Subject: Re: [v6] drm/xe: Add vram frequency sysfs attributes
>>
>>
>> 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.
> Sure,  thanks for explanation.
>>>> +	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 ?
> Check the kobject_release() , this will only get call , when all ref count of that kobj is being put.
> Here you are creating a object and it will never be released.
> Also  we need to call  the sysfs_remove_group() as well in fini function ?
>
> Thanks,
> Anshuman.

This is a bit of unique case. In this file we have two issues that keeps 
us from cleaning up like others.

One, we need to cleanup the base tile directory here. And second, we are 
creating the kobject only for PVC. If we add the "memory" kobject 
cleanup to fini, we will be defining tile using kobj. This is causing an 
error on unload, despite adding platform conditions on fini.

After testing multiple iterations of the fini function, this was the 
cleanest way with no errors that worked across platforms.

If needed, the only way to accommodate the kobject_put(kobj)  and the 
sysfs_remove_group is to move the vram sysfs creation to a separate 
file, similar to throttle_reasons and gt_freq.

Thanks,

Suja

>
>>> 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

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

[Gupta, Anshuman]

> -----Original Message-----
> From: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
> Sent: Wednesday, December 27, 2023 3:01 PM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
> xe@lists.freedesktop.org
> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Subject: Re: [v6] drm/xe: Add vram frequency sysfs attributes
> 
> 
> On 12/26/2023 7:58 PM, Gupta, Anshuman wrote:
> >
> >> -----Original Message-----
> >> From: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
> >> Sent: Tuesday, December 26, 2023 10:02 AM
> >> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
> >> xe@lists.freedesktop.org
> >> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> >> Subject: Re: [v6] drm/xe: Add vram frequency sysfs attributes
> >>
> >>
> >> 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.
> > Sure,  thanks for explanation.
> >>>> +	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 ?
> > Check the kobject_release() , this will only get call , when all ref count of that
> kobj is being put.
> > Here you are creating a object and it will never be released.
> > Also  we need to call  the sysfs_remove_group() as well in fini function ?
> >
> > Thanks,
> > Anshuman.
> 
> This is a bit of unique case. In this file we have two issues that keeps us from
> cleaning up like others.
Current patch looks broken to me, this will leak the kobject on module unload.
Please check kmemleak after module unload.
> 
> One, we need to cleanup the base tile directory here. And second, we are
> creating the kobject only for PVC. If we add the "memory" kobject cleanup to
> fini, we will be defining tile using kobj. This is causing an error on unload,
> despite adding platform conditions on fini.
You may try by changing the type of void *arg to "memory" kob to fini function ?
> 
> After testing multiple iterations of the fini function, this was the cleanest way
> with no errors that worked across platforms.
> 
> If needed, the only way to accommodate the kobject_put(kobj)  and the
> sysfs_remove_group is to move the vram sysfs creation to a separate file,
> similar to throttle_reasons and gt_freq.
I think it is needed for a functional working patch, if it can not be handled by using
"void *arg" in fini function.

Thanks,
Anshuman Gupta.


> 
> Thanks,
> 
> Suja
> 
> >
> >>> 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

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

[Sundaresan, Sujaritha]

On 12/27/2023 4:26 PM, Gupta, Anshuman wrote:
>
>> -----Original Message-----
>> From: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
>> Sent: Wednesday, December 27, 2023 3:01 PM
>> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
>> xe@lists.freedesktop.org
>> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>> Subject: Re: [v6] drm/xe: Add vram frequency sysfs attributes
>>
>>
>> On 12/26/2023 7:58 PM, Gupta, Anshuman wrote:
>>>> -----Original Message-----
>>>> From: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
>>>> Sent: Tuesday, December 26, 2023 10:02 AM
>>>> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
>>>> xe@lists.freedesktop.org
>>>> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>>>> Subject: Re: [v6] drm/xe: Add vram frequency sysfs attributes
>>>>
>>>>
>>>> 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.
>>> Sure,  thanks for explanation.
>>>>>> +	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 ?
>>> Check the kobject_release() , this will only get call , when all ref count of that
>> kobj is being put.
>>> Here you are creating a object and it will never be released.
>>> Also  we need to call  the sysfs_remove_group() as well in fini function ?
>>>
>>> Thanks,
>>> Anshuman.
>> This is a bit of unique case. In this file we have two issues that keeps us from
>> cleaning up like others.
> Current patch looks broken to me, this will leak the kobject on module unload.
> Please check kmemleak after module unload.
>> One, we need to cleanup the base tile directory here. And second, we are
>> creating the kobject only for PVC. If we add the "memory" kobject cleanup to
>> fini, we will be defining tile using kobj. This is causing an error on unload,
>> despite adding platform conditions on fini.
> You may try by changing the type of void *arg to "memory" kob to fini function ?
>> After testing multiple iterations of the fini function, this was the cleanest way
>> with no errors that worked across platforms.
>>
>> If needed, the only way to accommodate the kobject_put(kobj)  and the
>> sysfs_remove_group is to move the vram sysfs creation to a separate file,
>> similar to throttle_reasons and gt_freq.
> I think it is needed for a functional working patch, if it can not be handled by using
> "void *arg" in fini function.
>
> Thanks,
> Anshuman Gupta.

Since this is bit contentious even though the patch is functional, I 
will move these sysfs out of xe_tile_sysfs.c to it own new file.

This should clear up everything.

Thanks,

Suja

>
>
>> Thanks,
>>
>> Suja
>>
>>>>> 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

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

[Sundaresan, Sujaritha]

On 12/27/2023 5:48 PM, Sundaresan, Sujaritha wrote:
>
> On 12/27/2023 4:26 PM, Gupta, Anshuman wrote:
>>
>>> -----Original Message-----
>>> From: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
>>> Sent: Wednesday, December 27, 2023 3:01 PM
>>> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
>>> xe@lists.freedesktop.org
>>> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>>> Subject: Re: [v6] drm/xe: Add vram frequency sysfs attributes
>>>
>>>
>>> On 12/26/2023 7:58 PM, Gupta, Anshuman wrote:
>>>>> -----Original Message-----
>>>>> From: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
>>>>> Sent: Tuesday, December 26, 2023 10:02 AM
>>>>> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
>>>>> xe@lists.freedesktop.org
>>>>> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>>>>> Subject: Re: [v6] drm/xe: Add vram frequency sysfs attributes
>>>>>
>>>>>
>>>>> 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.
>>>> Sure,  thanks for explanation.
>>>>>>> +    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 ?
>>>> Check the kobject_release() , this will only get call , when all 
>>>> ref count of that
>>> kobj is being put.
>>>> Here you are creating a object and it will never be released.
>>>> Also  we need to call  the sysfs_remove_group() as well in fini 
>>>> function ?
>>>>
>>>> Thanks,
>>>> Anshuman.
>>> This is a bit of unique case. In this file we have two issues that 
>>> keeps us from
>>> cleaning up like others.
>> Current patch looks broken to me, this will leak the kobject on 
>> module unload.
>> Please check kmemleak after module unload.
>>> One, we need to cleanup the base tile directory here. And second, we 
>>> are
>>> creating the kobject only for PVC. If we add the "memory" kobject 
>>> cleanup to
>>> fini, we will be defining tile using kobj. This is causing an error 
>>> on unload,
>>> despite adding platform conditions on fini.
>> You may try by changing the type of void *arg to "memory" kob to fini 
>> function ?
>>> After testing multiple iterations of the fini function, this was the 
>>> cleanest way
>>> with no errors that worked across platforms.
>>>
>>> If needed, the only way to accommodate the kobject_put(kobj) and the
>>> sysfs_remove_group is to move the vram sysfs creation to a separate 
>>> file,
>>> similar to throttle_reasons and gt_freq.
>> I think it is needed for a functional working patch, if it can not be 
>> handled by using
>> "void *arg" in fini function.
>>
>> Thanks,
>> Anshuman Gupta.
>
> Since this is bit contentious even though the patch is functional, I 
> will move these sysfs out of xe_tile_sysfs.c to it own new file.
>
> This should clear up everything.
>
> Thanks,
>
> Suja

Hi Anshuman,

I've moved everything to a new file called xe_vram_freq.c/h . All 
kmemleaks and unload/reload checks are clean.

Let me know if the naming works.

The documentation added is as below, let me know if any tweaks are needed;

/**
* DOC: Xe VRAM freq
*
* Provides sysfs entries for vram frequency in tile
*
* device/tile#/memory/freq0/max_freq - This is maximum frequency. This 
value is read-only as it
*         is the fixed fuse point P0. It is not the system
*         configuration frequency.
* device/tile#/memory/freq0/min_freq - This is minimum frequency. This 
value is read-only as it
*         is the fixed fuse point PN. It is not the system
*          configuration frequency.
*/

Thanks,

Suja

>
>>
>>
>>> Thanks,
>>>
>>> Suja
>>>
>>>>>> 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

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

[Riana Tauro]

Hi Suja

On 12/28/2023 3:34 PM, Sundaresan, Sujaritha wrote:
> 
> On 12/27/2023 5:48 PM, Sundaresan, Sujaritha wrote:
>>
>> On 12/27/2023 4:26 PM, Gupta, Anshuman wrote:
>>>
>>>> -----Original Message-----
>>>> From: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
>>>> Sent: Wednesday, December 27, 2023 3:01 PM
>>>> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
>>>> xe@lists.freedesktop.org
>>>> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>>>> Subject: Re: [v6] drm/xe: Add vram frequency sysfs attributes
>>>>
>>>>
>>>> On 12/26/2023 7:58 PM, Gupta, Anshuman wrote:
>>>>>> -----Original Message-----
>>>>>> From: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
>>>>>> Sent: Tuesday, December 26, 2023 10:02 AM
>>>>>> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
>>>>>> xe@lists.freedesktop.org
>>>>>> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>>>>>> Subject: Re: [v6] drm/xe: Add vram frequency sysfs attributes
>>>>>>
>>>>>>
>>>>>> 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.
>>>>> Sure,  thanks for explanation.
>>>>>>>> +    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 ?
>>>>> Check the kobject_release() , this will only get call , when all 
>>>>> ref count of that
>>>> kobj is being put.
>>>>> Here you are creating a object and it will never be released.
>>>>> Also  we need to call  the sysfs_remove_group() as well in fini 
>>>>> function ?
>>>>>
>>>>> Thanks,
>>>>> Anshuman.
>>>> This is a bit of unique case. In this file we have two issues that 
>>>> keeps us from
>>>> cleaning up like others.
>>> Current patch looks broken to me, this will leak the kobject on 
>>> module unload.
>>> Please check kmemleak after module unload.
>>>> One, we need to cleanup the base tile directory here. And second, we 
>>>> are
>>>> creating the kobject only for PVC. If we add the "memory" kobject 
>>>> cleanup to
>>>> fini, we will be defining tile using kobj. This is causing an error 
>>>> on unload,
>>>> despite adding platform conditions on fini.
>>> You may try by changing the type of void *arg to "memory" kob to fini 
>>> function ?
>>>> After testing multiple iterations of the fini function, this was the 
>>>> cleanest way
>>>> with no errors that worked across platforms.
>>>>
>>>> If needed, the only way to accommodate the kobject_put(kobj) and the
>>>> sysfs_remove_group is to move the vram sysfs creation to a separate 
>>>> file,
>>>> similar to throttle_reasons and gt_freq.
>>> I think it is needed for a functional working patch, if it can not be 
>>> handled by using
>>> "void *arg" in fini function.
I think you can try something similar to the below file
xe_hw_engine_class_sysfs.c

Has different functions for different hierarchy of kobjects.

Thanks
Riana
>>>
>>> Thanks,
>>> Anshuman Gupta.
>>
>> Since this is bit contentious even though the patch is functional, I 
>> will move these sysfs out of xe_tile_sysfs.c to it own new file.
>>
>> This should clear up everything.
>>
>> Thanks,
>>
>> Suja
> 
> Hi Anshuman,
> 
> I've moved everything to a new file called xe_vram_freq.c/h . All 
> kmemleaks and unload/reload checks are clean.
> 
> Let me know if the naming works.
> 
> The documentation added is as below, let me know if any tweaks are needed;
> 
> /**
> * DOC: Xe VRAM freq
> *
> * Provides sysfs entries for vram frequency in tile
> *
> * device/tile#/memory/freq0/max_freq - This is maximum frequency. This 
> value is read-only as it
> *         is the fixed fuse point P0. It is not the system
> *         configuration frequency.
> * device/tile#/memory/freq0/min_freq - This is minimum frequency. This 
> value is read-only as it
> *         is the fixed fuse point PN. It is not the system
> *          configuration frequency.
> */
> 
> Thanks,
> 
> Suja
> 
>>
>>>
>>>
>>>> Thanks,
>>>>
>>>> Suja
>>>>
>>>>>>> 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