[Intel-xe] ✗ CI.Patch_applied: failure for Add some performance and frequency sysfs nodes

[Intel-xe] ✗ CI.Patch_applied: failure for Add some performance and frequency sysfs nodes

[Patchwork]

== Series Details ==

Series: Add some performance and frequency sysfs nodes
URL   : https://patchwork.freedesktop.org/series/123164/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-xe-next' with base: ===
Base commit: e4a038763 fixup! drm/xe: Add min/max cap for engine scheduler properties
=== git am output follows ===
error: patch failed: drivers/gpu/drm/xe/xe_guc_pc.c:925
error: drivers/gpu/drm/xe/xe_guc_pc.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch
Applying: drm/xe: Add a couple of pcode helpers
Applying: drm/xe: Add base performance sysfs attributes
Patch failed at 0002 drm/xe: Add base performance sysfs attributes
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

[Intel-xe] [PATCH 0/3] Add some performance and frequency sysfs nodes

[Sujaritha Sundaresan]

Adding base performace and vram frequency attributes to sysfs.

Sujaritha Sundaresan (3):
  drm/xe: Add a couple of pcode helpers
  drm/xe: Add base performance sysfs attributes
  drm/xe: Sysfs entries to query vram fused min, max frequency

 drivers/gpu/drm/xe/xe_guc_pc.c    | 170 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_pcode.c     |  28 +++++
 drivers/gpu/drm/xe/xe_pcode.h     |   3 +
 drivers/gpu/drm/xe/xe_pcode_api.h |  19 ++++
 4 files changed, 220 insertions(+)

-- 
2.25.1

[Intel-xe] [PATCH 1/3] drm/xe: Add a couple of pcode helpers

[Sujaritha Sundaresan]

Some pcode commands take additional sub-commands and parameters. Add a
couple of helpers to help formatting these commands to improve code
readability.

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
---
 drivers/gpu/drm/xe/xe_pcode.c | 28 ++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_pcode.h |  3 +++
 2 files changed, 31 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
index 7f1bf2297f51..e45169f47500 100644
--- a/drivers/gpu/drm/xe/xe_pcode.c
+++ b/drivers/gpu/drm/xe/xe_pcode.c
@@ -104,6 +104,34 @@ int xe_pcode_read(struct xe_gt *gt, u32 mbox, u32 *val, u32 *val1)
 	return err;
 }
 
+int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 *val)
+{
+	u32 mbox;
+	int err;
+
+	mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
+		| REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
+		| REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
+
+	err = xe_pcode_read(gt, mbox, val, NULL);
+
+	return err;
+}
+
+int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 val)
+{
+	u32 mbox;
+	int err;
+
+	mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
+		| REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
+		| REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
+
+	err = xe_pcode_write(gt, mbox, val);
+
+	return err;
+}
+
 static int xe_pcode_try_request(struct xe_gt *gt, u32 mbox,
 				u32 request, u32 reply_mask, u32 reply,
 				u32 *status, bool atomic, int timeout_us)
diff --git a/drivers/gpu/drm/xe/xe_pcode.h b/drivers/gpu/drm/xe/xe_pcode.h
index 3b4aa8c1a3ba..8d4103afd7e0 100644
--- a/drivers/gpu/drm/xe/xe_pcode.h
+++ b/drivers/gpu/drm/xe/xe_pcode.h
@@ -19,6 +19,9 @@ int xe_pcode_write_timeout(struct xe_gt *gt, u32 mbox, u32 val,
 #define xe_pcode_write(gt, mbox, val) \
 	xe_pcode_write_timeout(gt, mbox, val, 1)
 
+int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 *val);
+int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 val);
+
 int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
 		     u32 reply_mask, u32 reply, int timeout_ms);
 
-- 
2.25.1

[Intel-xe] [PATCH 2/3] drm/xe: Add base performance sysfs attributes

[Sujaritha Sundaresan]

Add the following attribute to Xe sysfs
- base_freq_factor
- base_freq_factor.scale
- base_freq_rp0
- base_freq_rpn

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
---
 drivers/gpu/drm/xe/xe_guc_pc.c    | 116 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_pcode_api.h |  19 +++++
 2 files changed, 135 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
index c03bb58e7049..6a139d1dbf66 100644
--- a/drivers/gpu/drm/xe/xe_guc_pc.c
+++ b/drivers/gpu/drm/xe/xe_guc_pc.c
@@ -20,6 +20,7 @@
 #include "xe_map.h"
 #include "xe_mmio.h"
 #include "xe_pcode.h"
+#include "xe_pcode_api.h"
 
 #define MCHBAR_MIRROR_BASE_SNB	0x140000
 
@@ -37,6 +38,9 @@
 #define GT_FREQUENCY_MULTIPLIER	50
 #define GEN9_FREQ_SCALER	3
 
+#define U8_8_VAL_MASK           0xffff
+#define U8_8_SCALE_TO_VALUE     "0.00390625"
+
 /**
  * DOC: GuC Power Conservation (PC)
  *
@@ -642,6 +646,112 @@ static const struct attribute *pc_attrs[] = {
 	NULL
 };
 
+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_pcode_read_p(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_pcode_write_p(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_pcode_read_p(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_pcode_read_p(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 *pvc_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 void mtl_init_fused_rp_values(struct xe_guc_pc *pc)
 {
 	struct xe_gt *gt = pc_to_gt(pc);
@@ -925,6 +1035,12 @@ int xe_guc_pc_init(struct xe_guc_pc *pc)
 	if (err)
 		return err;
 
+	if (xe->info.platform == XE_PVC) {
+		err = sysfs_create_files(gt->sysfs, pvc_perf_power_attrs);
+		if (err)
+			return err;
+	}
+
 	err = drmm_add_action_or_reset(&xe->drm, pc_fini, pc);
 	if (err)
 		return err;
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
+/* 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

[Intel-xe] [PATCH 3/3, v2] drm/xe: Sysfs entries to query vram fused min, max frequency

[Sujaritha Sundaresan]

Add sysfs enteries for vram rp0 and vram rpn frequencies

v2: Fix review comments (Anshuman)

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
---
 drivers/gpu/drm/xe/xe_guc_pc.c | 54 ++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
index 6a139d1dbf66..5084da83896b 100644
--- a/drivers/gpu/drm/xe/xe_guc_pc.c
+++ b/drivers/gpu/drm/xe/xe_guc_pc.c
@@ -752,6 +752,54 @@ static const struct attribute *pvc_perf_power_attrs[] = {
 	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_pcode_read_p(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_pcode_read_p(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 mtl_init_fused_rp_values(struct xe_guc_pc *pc)
 {
 	struct xe_gt *gt = pc_to_gt(pc);
@@ -1041,6 +1089,12 @@ int xe_guc_pc_init(struct xe_guc_pc *pc)
 			return err;
 	}
 
+	if (xe->info.platform == XE_PVC) {
+		err = sysfs_create_files(tile->sysfs, vram_freq_attrs);
+		if (err)
+			return err;
+	}
+
 	err = drmm_add_action_or_reset(&xe->drm, pc_fini, pc);
 	if (err)
 		return err;
-- 
2.25.1

Re: [Intel-xe] [PATCH 1/3] drm/xe: Add a couple of pcode helpers

[Rodrigo Vivi]

On Fri, Sep 01, 2023 at 05:45:43PM +0530, Sujaritha Sundaresan wrote:
> Some pcode commands take additional sub-commands and parameters. Add a
> couple of helpers to help formatting these commands to improve code
> readability.
> 
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pcode.c | 28 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_pcode.h |  3 +++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> index 7f1bf2297f51..e45169f47500 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.c
> +++ b/drivers/gpu/drm/xe/xe_pcode.c
> @@ -104,6 +104,34 @@ int xe_pcode_read(struct xe_gt *gt, u32 mbox, u32 *val, u32 *val1)
>  	return err;
>  }
>  

a doc would be required...

> +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 *val)

a better name would be nice....

> +{
> +	u32 mbox;
> +	int err;
> +
> +	mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
> +		| REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
> +		| REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
> +
> +	err = xe_pcode_read(gt, mbox, val, NULL);

but why not simply modifying the existent one to accept 2 params?

int xe_pcode_read(struct xe_gt *gt, u32 mbox_param1, u32 mbox_param2,
    		  u32 *val, u32 *val1)

and the equivalent write...

oh, and while doing that, could you please add the missing documentation
to these 2 functions?

Thanks,
Rodrigo.

> +
> +	return err;
> +}
> +
> +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 val)
> +{
> +	u32 mbox;
> +	int err;
> +
> +	mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
> +		| REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
> +		| REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
> +
> +	err = xe_pcode_write(gt, mbox, val);
> +
> +	return err;
> +}
> +
>  static int xe_pcode_try_request(struct xe_gt *gt, u32 mbox,
>  				u32 request, u32 reply_mask, u32 reply,
>  				u32 *status, bool atomic, int timeout_us)
> diff --git a/drivers/gpu/drm/xe/xe_pcode.h b/drivers/gpu/drm/xe/xe_pcode.h
> index 3b4aa8c1a3ba..8d4103afd7e0 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.h
> +++ b/drivers/gpu/drm/xe/xe_pcode.h
> @@ -19,6 +19,9 @@ int xe_pcode_write_timeout(struct xe_gt *gt, u32 mbox, u32 val,
>  #define xe_pcode_write(gt, mbox, val) \
>  	xe_pcode_write_timeout(gt, mbox, val, 1)
>  
> +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 *val);
> +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 val);
> +
>  int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
>  		     u32 reply_mask, u32 reply, int timeout_ms);
>  
> -- 
> 2.25.1
> 

Re: [Intel-xe] [PATCH 2/3] drm/xe: Add base performance sysfs attributes

[Rodrigo Vivi]

On Fri, Sep 01, 2023 at 05:45:44PM +0530, Sujaritha Sundaresan wrote:
> Add the following attribute to Xe sysfs
> - base_freq_factor
> - base_freq_factor.scale
> - base_freq_rp0
> - base_freq_rpn
> 
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc_pc.c    | 116 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_pcode_api.h |  19 +++++
>  2 files changed, 135 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index c03bb58e7049..6a139d1dbf66 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c

again, this has nothing to do with guc_pc.
We clearly need a new component to handle the frequency.

> @@ -20,6 +20,7 @@
>  #include "xe_map.h"
>  #include "xe_mmio.h"
>  #include "xe_pcode.h"
> +#include "xe_pcode_api.h"
>  
>  #define MCHBAR_MIRROR_BASE_SNB	0x140000
>  
> @@ -37,6 +38,9 @@
>  #define GT_FREQUENCY_MULTIPLIER	50
>  #define GEN9_FREQ_SCALER	3
>  
> +#define U8_8_VAL_MASK           0xffff
> +#define U8_8_SCALE_TO_VALUE     "0.00390625"
> +
>  /**
>   * DOC: GuC Power Conservation (PC)
>   *
> @@ -642,6 +646,112 @@ static const struct attribute *pc_attrs[] = {
>  	NULL
>  };
>  
> +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_pcode_read_p(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_pcode_write_p(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_pcode_read_p(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_pcode_read_p(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 *pvc_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 void mtl_init_fused_rp_values(struct xe_guc_pc *pc)
>  {
>  	struct xe_gt *gt = pc_to_gt(pc);
> @@ -925,6 +1035,12 @@ int xe_guc_pc_init(struct xe_guc_pc *pc)
>  	if (err)
>  		return err;
>  
> +	if (xe->info.platform == XE_PVC) {
> +		err = sysfs_create_files(gt->sysfs, pvc_perf_power_attrs);
> +		if (err)
> +			return err;
> +	}
> +
>  	err = drmm_add_action_or_reset(&xe->drm, pc_fini, pc);
>  	if (err)
>  		return err;
> 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
> +/* 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
> 

Re: [Intel-xe] [PATCH 1/3] drm/xe: Add a couple of pcode helpers

[Sundaresan, Sujaritha]

On 9/2/2023 2:04 AM, Rodrigo Vivi wrote:
> On Fri, Sep 01, 2023 at 05:45:43PM +0530, Sujaritha Sundaresan wrote:
>> Some pcode commands take additional sub-commands and parameters. Add a
>> couple of helpers to help formatting these commands to improve code
>> readability.
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_pcode.c | 28 ++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_pcode.h |  3 +++
>>   2 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
>> index 7f1bf2297f51..e45169f47500 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode.c
>> +++ b/drivers/gpu/drm/xe/xe_pcode.c
>> @@ -104,6 +104,34 @@ int xe_pcode_read(struct xe_gt *gt, u32 mbox, u32 *val, u32 *val1)
>>   	return err;
>>   }
>>   
> a doc would be required...
>
>> +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 *val)
> a better name would be nice....
>
>> +{
>> +	u32 mbox;
>> +	int err;
>> +
>> +	mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
>> +
>> +	err = xe_pcode_read(gt, mbox, val, NULL);
> but why not simply modifying the existent one to accept 2 params?
>
> int xe_pcode_read(struct xe_gt *gt, u32 mbox_param1, u32 mbox_param2,
>      		  u32 *val, u32 *val1)
>
> and the equivalent write...
>
> oh, and while doing that, could you please add the missing documentation
> to these 2 functions?
>
> Thanks,
> Rodrigo.

Sure that would work. Will add the docs as well.

Thanks,

Suja

>
>> +
>> +	return err;
>> +}
>> +
>> +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 val)
>> +{
>> +	u32 mbox;
>> +	int err;
>> +
>> +	mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
>> +
>> +	err = xe_pcode_write(gt, mbox, val);
>> +
>> +	return err;
>> +}
>> +
>>   static int xe_pcode_try_request(struct xe_gt *gt, u32 mbox,
>>   				u32 request, u32 reply_mask, u32 reply,
>>   				u32 *status, bool atomic, int timeout_us)
>> diff --git a/drivers/gpu/drm/xe/xe_pcode.h b/drivers/gpu/drm/xe/xe_pcode.h
>> index 3b4aa8c1a3ba..8d4103afd7e0 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode.h
>> +++ b/drivers/gpu/drm/xe/xe_pcode.h
>> @@ -19,6 +19,9 @@ int xe_pcode_write_timeout(struct xe_gt *gt, u32 mbox, u32 val,
>>   #define xe_pcode_write(gt, mbox, val) \
>>   	xe_pcode_write_timeout(gt, mbox, val, 1)
>>   
>> +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 *val);
>> +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, u32 val);
>> +
>>   int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
>>   		     u32 reply_mask, u32 reply, int timeout_ms);
>>   
>> -- 
>> 2.25.1
>>

Re: [Intel-xe] [PATCH 2/3] drm/xe: Add base performance sysfs attributes

[Sundaresan, Sujaritha]

On 9/2/2023 2:05 AM, Rodrigo Vivi wrote:
> On Fri, Sep 01, 2023 at 05:45:44PM +0530, Sujaritha Sundaresan wrote:
>> Add the following attribute to Xe sysfs
>> - base_freq_factor
>> - base_freq_factor.scale
>> - base_freq_rp0
>> - base_freq_rpn
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guc_pc.c    | 116 ++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_pcode_api.h |  19 +++++
>>   2 files changed, 135 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
>> index c03bb58e7049..6a139d1dbf66 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> again, this has nothing to do with guc_pc.
> We clearly need a new component to handle the frequency.

Right. will look to define a new xe_freq component

Thanks,

Suja

>
>> @@ -20,6 +20,7 @@
>>   #include "xe_map.h"
>>   #include "xe_mmio.h"
>>   #include "xe_pcode.h"
>> +#include "xe_pcode_api.h"
>>   
>>   #define MCHBAR_MIRROR_BASE_SNB	0x140000
>>   
>> @@ -37,6 +38,9 @@
>>   #define GT_FREQUENCY_MULTIPLIER	50
>>   #define GEN9_FREQ_SCALER	3
>>   
>> +#define U8_8_VAL_MASK           0xffff
>> +#define U8_8_SCALE_TO_VALUE     "0.00390625"
>> +
>>   /**
>>    * DOC: GuC Power Conservation (PC)
>>    *
>> @@ -642,6 +646,112 @@ static const struct attribute *pc_attrs[] = {
>>   	NULL
>>   };
>>   
>> +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_pcode_read_p(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_pcode_write_p(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_pcode_read_p(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_pcode_read_p(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 *pvc_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 void mtl_init_fused_rp_values(struct xe_guc_pc *pc)
>>   {
>>   	struct xe_gt *gt = pc_to_gt(pc);
>> @@ -925,6 +1035,12 @@ int xe_guc_pc_init(struct xe_guc_pc *pc)
>>   	if (err)
>>   		return err;
>>   
>> +	if (xe->info.platform == XE_PVC) {
>> +		err = sysfs_create_files(gt->sysfs, pvc_perf_power_attrs);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>>   	err = drmm_add_action_or_reset(&xe->drm, pc_fini, pc);
>>   	if (err)
>>   		return err;
>> 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
>> +/* 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
>>

Re: [Intel-xe] [PATCH 1/3] drm/xe: Add a couple of pcode helpers

[Sundaresan, Sujaritha]

On 9/3/2023 7:00 PM, Sundaresan, Sujaritha wrote:
>
> On 9/2/2023 2:04 AM, Rodrigo Vivi wrote:
>> On Fri, Sep 01, 2023 at 05:45:43PM +0530, Sujaritha Sundaresan wrote:
>>> Some pcode commands take additional sub-commands and parameters. Add a
>>> couple of helpers to help formatting these commands to improve code
>>> readability.
>>>
>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_pcode.c | 28 ++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/xe/xe_pcode.h |  3 +++
>>>   2 files changed, 31 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_pcode.c 
>>> b/drivers/gpu/drm/xe/xe_pcode.c
>>> index 7f1bf2297f51..e45169f47500 100644
>>> --- a/drivers/gpu/drm/xe/xe_pcode.c
>>> +++ b/drivers/gpu/drm/xe/xe_pcode.c
>>> @@ -104,6 +104,34 @@ int xe_pcode_read(struct xe_gt *gt, u32 mbox, 
>>> u32 *val, u32 *val1)
>>>       return err;
>>>   }
>> a doc would be required...
>>
>>> +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, 
>>> u32 *val)
>> a better name would be nice....
>>
>>> +{
>>> +    u32 mbox;
>>> +    int err;
>>> +
>>> +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
>>> +
>>> +    err = xe_pcode_read(gt, mbox, val, NULL);
>> but why not simply modifying the existent one to accept 2 params?
>>
>> int xe_pcode_read(struct xe_gt *gt, u32 mbox_param1, u32 mbox_param2,
>>                u32 *val, u32 *val1)
>>
>> and the equivalent write...
>>
>> oh, and while doing that, could you please add the missing documentation
>> to these 2 functions?
>>
>> Thanks,
>> Rodrigo.
>
> Sure that would work. Will add the docs as well.
>
> Thanks,
>
> Suja

Hi Rodrigo,

Another question,

I can change the existing pcode_read function, but would it be better to 
have a separate new write equivalent ?

>
>>
>>> +
>>> +    return err;
>>> +}
>>> +
>>> +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, 
>>> u32 val)
>>> +{
>>> +    u32 mbox;
>>> +    int err;
>>> +
>>> +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
>>> +
>>> +    err = xe_pcode_write(gt, mbox, val);
>>> +
>>> +    return err;
>>> +}
>>> +
>>>   static int xe_pcode_try_request(struct xe_gt *gt, u32 mbox,
>>>                   u32 request, u32 reply_mask, u32 reply,
>>>                   u32 *status, bool atomic, int timeout_us)
>>> diff --git a/drivers/gpu/drm/xe/xe_pcode.h 
>>> b/drivers/gpu/drm/xe/xe_pcode.h
>>> index 3b4aa8c1a3ba..8d4103afd7e0 100644
>>> --- a/drivers/gpu/drm/xe/xe_pcode.h
>>> +++ b/drivers/gpu/drm/xe/xe_pcode.h
>>> @@ -19,6 +19,9 @@ int xe_pcode_write_timeout(struct xe_gt *gt, u32 
>>> mbox, u32 val,
>>>   #define xe_pcode_write(gt, mbox, val) \
>>>       xe_pcode_write_timeout(gt, mbox, val, 1)
>>>   +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, 
>>> u32 *val);
>>> +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32 p2, 
>>> u32 val);
>>> +
>>>   int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
>>>                u32 reply_mask, u32 reply, int timeout_ms);
>>>   --
>>> 2.25.1
>>>

Re: [Intel-xe] [PATCH 2/3] drm/xe: Add base performance sysfs attributes

[Sundaresan, Sujaritha]

On 9/3/2023 7:06 PM, Sundaresan, Sujaritha wrote:
>
> On 9/2/2023 2:05 AM, Rodrigo Vivi wrote:
>> On Fri, Sep 01, 2023 at 05:45:44PM +0530, Sujaritha Sundaresan wrote:
>>> Add the following attribute to Xe sysfs
>>> - base_freq_factor
>>> - base_freq_factor.scale
>>> - base_freq_rp0
>>> - base_freq_rpn
>>>
>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_guc_pc.c    | 116 
>>> ++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/xe/xe_pcode_api.h |  19 +++++
>>>   2 files changed, 135 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c 
>>> b/drivers/gpu/drm/xe/xe_guc_pc.c
>>> index c03bb58e7049..6a139d1dbf66 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>> again, this has nothing to do with guc_pc.
>> We clearly need a new component to handle the frequency.
>
> Right. will look to define a new xe_freq component
>
> Thanks,
>
> Suja

Hi Rodrigo,

I've moved a lot of the frequency attributes I'm adding to a new file

with a new xe_freq component.

Does that work for this ?

Thanks,

Suja

>
>
>>
>>> @@ -20,6 +20,7 @@
>>>   #include "xe_map.h"
>>>   #include "xe_mmio.h"
>>>   #include "xe_pcode.h"
>>> +#include "xe_pcode_api.h"
>>>     #define MCHBAR_MIRROR_BASE_SNB    0x140000
>>>   @@ -37,6 +38,9 @@
>>>   #define GT_FREQUENCY_MULTIPLIER    50
>>>   #define GEN9_FREQ_SCALER    3
>>>   +#define U8_8_VAL_MASK           0xffff
>>> +#define U8_8_SCALE_TO_VALUE     "0.00390625"
>>> +
>>>   /**
>>>    * DOC: GuC Power Conservation (PC)
>>>    *
>>> @@ -642,6 +646,112 @@ static const struct attribute *pc_attrs[] = {
>>>       NULL
>>>   };
>>>   +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_pcode_read_p(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_pcode_write_p(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_pcode_read_p(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_pcode_read_p(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 *pvc_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 void mtl_init_fused_rp_values(struct xe_guc_pc *pc)
>>>   {
>>>       struct xe_gt *gt = pc_to_gt(pc);
>>> @@ -925,6 +1035,12 @@ int xe_guc_pc_init(struct xe_guc_pc *pc)
>>>       if (err)
>>>           return err;
>>>   +    if (xe->info.platform == XE_PVC) {
>>> +        err = sysfs_create_files(gt->sysfs, pvc_perf_power_attrs);
>>> +        if (err)
>>> +            return err;
>>> +    }
>>> +
>>>       err = drmm_add_action_or_reset(&xe->drm, pc_fini, pc);
>>>       if (err)
>>>           return err;
>>> 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
>>> +/* 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
>>>

Re: [Intel-xe] [PATCH 1/3] drm/xe: Add a couple of pcode helpers

[Rodrigo Vivi]

On Tue, Sep 12, 2023 at 10:38:50AM +0530, Sundaresan, Sujaritha wrote:
> 
> On 9/3/2023 7:00 PM, Sundaresan, Sujaritha wrote:
> > 
> > On 9/2/2023 2:04 AM, Rodrigo Vivi wrote:
> > > On Fri, Sep 01, 2023 at 05:45:43PM +0530, Sujaritha Sundaresan wrote:
> > > > Some pcode commands take additional sub-commands and parameters. Add a
> > > > couple of helpers to help formatting these commands to improve code
> > > > readability.
> > > > 
> > > > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/xe/xe_pcode.c | 28 ++++++++++++++++++++++++++++
> > > >   drivers/gpu/drm/xe/xe_pcode.h |  3 +++
> > > >   2 files changed, 31 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_pcode.c
> > > > b/drivers/gpu/drm/xe/xe_pcode.c
> > > > index 7f1bf2297f51..e45169f47500 100644
> > > > --- a/drivers/gpu/drm/xe/xe_pcode.c
> > > > +++ b/drivers/gpu/drm/xe/xe_pcode.c
> > > > @@ -104,6 +104,34 @@ int xe_pcode_read(struct xe_gt *gt, u32
> > > > mbox, u32 *val, u32 *val1)
> > > >       return err;
> > > >   }
> > > a doc would be required...
> > > 
> > > > +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
> > > > p2, u32 *val)
> > > a better name would be nice....
> > > 
> > > > +{
> > > > +    u32 mbox;
> > > > +    int err;
> > > > +
> > > > +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
> > > > +        | REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
> > > > +        | REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
> > > > +
> > > > +    err = xe_pcode_read(gt, mbox, val, NULL);
> > > but why not simply modifying the existent one to accept 2 params?
> > > 
> > > int xe_pcode_read(struct xe_gt *gt, u32 mbox_param1, u32 mbox_param2,
> > >                u32 *val, u32 *val1)
> > > 
> > > and the equivalent write...
> > > 
> > > oh, and while doing that, could you please add the missing documentation
> > > to these 2 functions?
> > > 
> > > Thanks,
> > > Rodrigo.
> > 
> > Sure that would work. Will add the docs as well.
> > 
> > Thanks,
> > 
> > Suja
> 
> Hi Rodrigo,
> 
> Another question,
> 
> I can change the existing pcode_read function, but would it be better to
> have a separate new write equivalent ?

I wonder if we should do s/xe_pcode_write_timeout(/xe_pcode_write(

where timeout is still an argument but it can be null.
And then we merge with your options here and make a single write fn.

> 
> > 
> > > 
> > > > +
> > > > +    return err;
> > > > +}
> > > > +
> > > > +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
> > > > p2, u32 val)
> > > > +{
> > > > +    u32 mbox;
> > > > +    int err;
> > > > +
> > > > +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
> > > > +        | REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
> > > > +        | REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
> > > > +
> > > > +    err = xe_pcode_write(gt, mbox, val);
> > > > +
> > > > +    return err;
> > > > +}
> > > > +
> > > >   static int xe_pcode_try_request(struct xe_gt *gt, u32 mbox,
> > > >                   u32 request, u32 reply_mask, u32 reply,
> > > >                   u32 *status, bool atomic, int timeout_us)
> > > > diff --git a/drivers/gpu/drm/xe/xe_pcode.h
> > > > b/drivers/gpu/drm/xe/xe_pcode.h
> > > > index 3b4aa8c1a3ba..8d4103afd7e0 100644
> > > > --- a/drivers/gpu/drm/xe/xe_pcode.h
> > > > +++ b/drivers/gpu/drm/xe/xe_pcode.h
> > > > @@ -19,6 +19,9 @@ int xe_pcode_write_timeout(struct xe_gt *gt,
> > > > u32 mbox, u32 val,
> > > >   #define xe_pcode_write(gt, mbox, val) \
> > > >       xe_pcode_write_timeout(gt, mbox, val, 1)
> > > >   +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
> > > > p2, u32 *val);
> > > > +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
> > > > p2, u32 val);
> > > > +
> > > >   int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
> > > >                u32 reply_mask, u32 reply, int timeout_ms);
> > > >   --
> > > > 2.25.1
> > > > 

Re: [Intel-xe] [PATCH 2/3] drm/xe: Add base performance sysfs attributes

[Rodrigo Vivi]

On Tue, Sep 12, 2023 at 02:18:55PM +0530, Sundaresan, Sujaritha wrote:
> 
> On 9/3/2023 7:06 PM, Sundaresan, Sujaritha wrote:
> > 
> > On 9/2/2023 2:05 AM, Rodrigo Vivi wrote:
> > > On Fri, Sep 01, 2023 at 05:45:44PM +0530, Sujaritha Sundaresan wrote:
> > > > Add the following attribute to Xe sysfs
> > > > - base_freq_factor
> > > > - base_freq_factor.scale
> > > > - base_freq_rp0
> > > > - base_freq_rpn
> > > > 
> > > > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/xe/xe_guc_pc.c    | 116
> > > > ++++++++++++++++++++++++++++++
> > > >   drivers/gpu/drm/xe/xe_pcode_api.h |  19 +++++
> > > >   2 files changed, 135 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c
> > > > b/drivers/gpu/drm/xe/xe_guc_pc.c
> > > > index c03bb58e7049..6a139d1dbf66 100644
> > > > --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> > > > +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> > > again, this has nothing to do with guc_pc.
> > > We clearly need a new component to handle the frequency.
> > 
> > Right. will look to define a new xe_freq component
> > 
> > Thanks,
> > 
> > Suja
> 
> Hi Rodrigo,
> 
> I've moved a lot of the frequency attributes I'm adding to a new file
> 
> with a new xe_freq component.
> 
> Does that work for this ?

I think so, but I might be missing something.
Is this already in the mailing list or some branch that I could peak?

> 
> Thanks,
> 
> Suja
> 
> > 
> > 
> > > 
> > > > @@ -20,6 +20,7 @@
> > > >   #include "xe_map.h"
> > > >   #include "xe_mmio.h"
> > > >   #include "xe_pcode.h"
> > > > +#include "xe_pcode_api.h"
> > > >     #define MCHBAR_MIRROR_BASE_SNB    0x140000
> > > >   @@ -37,6 +38,9 @@
> > > >   #define GT_FREQUENCY_MULTIPLIER    50
> > > >   #define GEN9_FREQ_SCALER    3
> > > >   +#define U8_8_VAL_MASK           0xffff
> > > > +#define U8_8_SCALE_TO_VALUE     "0.00390625"
> > > > +
> > > >   /**
> > > >    * DOC: GuC Power Conservation (PC)
> > > >    *
> > > > @@ -642,6 +646,112 @@ static const struct attribute *pc_attrs[] = {
> > > >       NULL
> > > >   };
> > > >   +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_pcode_read_p(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_pcode_write_p(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_pcode_read_p(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_pcode_read_p(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 *pvc_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 void mtl_init_fused_rp_values(struct xe_guc_pc *pc)
> > > >   {
> > > >       struct xe_gt *gt = pc_to_gt(pc);
> > > > @@ -925,6 +1035,12 @@ int xe_guc_pc_init(struct xe_guc_pc *pc)
> > > >       if (err)
> > > >           return err;
> > > >   +    if (xe->info.platform == XE_PVC) {
> > > > +        err = sysfs_create_files(gt->sysfs, pvc_perf_power_attrs);
> > > > +        if (err)
> > > > +            return err;
> > > > +    }
> > > > +
> > > >       err = drmm_add_action_or_reset(&xe->drm, pc_fini, pc);
> > > >       if (err)
> > > >           return err;
> > > > 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
> > > > +/* 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
> > > > 

Re: [Intel-xe] [PATCH 2/3] drm/xe: Add base performance sysfs attributes

[Sundaresan, Sujaritha]

On 9/12/2023 8:04 PM, Rodrigo Vivi wrote:
> On Tue, Sep 12, 2023 at 02:18:55PM +0530, Sundaresan, Sujaritha wrote:
>> On 9/3/2023 7:06 PM, Sundaresan, Sujaritha wrote:
>>> On 9/2/2023 2:05 AM, Rodrigo Vivi wrote:
>>>> On Fri, Sep 01, 2023 at 05:45:44PM +0530, Sujaritha Sundaresan wrote:
>>>>> Add the following attribute to Xe sysfs
>>>>> - base_freq_factor
>>>>> - base_freq_factor.scale
>>>>> - base_freq_rp0
>>>>> - base_freq_rpn
>>>>>
>>>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/xe/xe_guc_pc.c    | 116
>>>>> ++++++++++++++++++++++++++++++
>>>>>    drivers/gpu/drm/xe/xe_pcode_api.h |  19 +++++
>>>>>    2 files changed, 135 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c
>>>>> b/drivers/gpu/drm/xe/xe_guc_pc.c
>>>>> index c03bb58e7049..6a139d1dbf66 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>>>> again, this has nothing to do with guc_pc.
>>>> We clearly need a new component to handle the frequency.
>>> Right. will look to define a new xe_freq component
>>>
>>> Thanks,
>>>
>>> Suja
>> Hi Rodrigo,
>>
>> I've moved a lot of the frequency attributes I'm adding to a new file
>>
>> with a new xe_freq component.
>>
>> Does that work for this ?
> I think so, but I might be missing something.
> Is this already in the mailing list or some branch that I could peak?
>
>> Thanks,
>>
>> Suja

Sure I can share the branch. There is a throttle reasons patch on the 
ML, which is similar in concept.

That's the direction I went with for the sysfs in this patch as well.

Thanks,

Suja

>>
>>>
>>>>> @@ -20,6 +20,7 @@
>>>>>    #include "xe_map.h"
>>>>>    #include "xe_mmio.h"
>>>>>    #include "xe_pcode.h"
>>>>> +#include "xe_pcode_api.h"
>>>>>      #define MCHBAR_MIRROR_BASE_SNB    0x140000
>>>>>    @@ -37,6 +38,9 @@
>>>>>    #define GT_FREQUENCY_MULTIPLIER    50
>>>>>    #define GEN9_FREQ_SCALER    3
>>>>>    +#define U8_8_VAL_MASK           0xffff
>>>>> +#define U8_8_SCALE_TO_VALUE     "0.00390625"
>>>>> +
>>>>>    /**
>>>>>     * DOC: GuC Power Conservation (PC)
>>>>>     *
>>>>> @@ -642,6 +646,112 @@ static const struct attribute *pc_attrs[] = {
>>>>>        NULL
>>>>>    };
>>>>>    +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_pcode_read_p(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_pcode_write_p(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_pcode_read_p(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_pcode_read_p(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 *pvc_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 void mtl_init_fused_rp_values(struct xe_guc_pc *pc)
>>>>>    {
>>>>>        struct xe_gt *gt = pc_to_gt(pc);
>>>>> @@ -925,6 +1035,12 @@ int xe_guc_pc_init(struct xe_guc_pc *pc)
>>>>>        if (err)
>>>>>            return err;
>>>>>    +    if (xe->info.platform == XE_PVC) {
>>>>> +        err = sysfs_create_files(gt->sysfs, pvc_perf_power_attrs);
>>>>> +        if (err)
>>>>> +            return err;
>>>>> +    }
>>>>> +
>>>>>        err = drmm_add_action_or_reset(&xe->drm, pc_fini, pc);
>>>>>        if (err)
>>>>>            return err;
>>>>> 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
>>>>> +/* 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
>>>>>

Re: [Intel-xe] [PATCH 1/3] drm/xe: Add a couple of pcode helpers

[Sundaresan, Sujaritha]

On 9/12/2023 8:03 PM, Rodrigo Vivi wrote:
> On Tue, Sep 12, 2023 at 10:38:50AM +0530, Sundaresan, Sujaritha wrote:
>> On 9/3/2023 7:00 PM, Sundaresan, Sujaritha wrote:
>>> On 9/2/2023 2:04 AM, Rodrigo Vivi wrote:
>>>> On Fri, Sep 01, 2023 at 05:45:43PM +0530, Sujaritha Sundaresan wrote:
>>>>> Some pcode commands take additional sub-commands and parameters. Add a
>>>>> couple of helpers to help formatting these commands to improve code
>>>>> readability.
>>>>>
>>>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/xe/xe_pcode.c | 28 ++++++++++++++++++++++++++++
>>>>>    drivers/gpu/drm/xe/xe_pcode.h |  3 +++
>>>>>    2 files changed, 31 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_pcode.c
>>>>> b/drivers/gpu/drm/xe/xe_pcode.c
>>>>> index 7f1bf2297f51..e45169f47500 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_pcode.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_pcode.c
>>>>> @@ -104,6 +104,34 @@ int xe_pcode_read(struct xe_gt *gt, u32
>>>>> mbox, u32 *val, u32 *val1)
>>>>>        return err;
>>>>>    }
>>>> a doc would be required...
>>>>
>>>>> +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
>>>>> p2, u32 *val)
>>>> a better name would be nice....
>>>>
>>>>> +{
>>>>> +    u32 mbox;
>>>>> +    int err;
>>>>> +
>>>>> +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
>>>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
>>>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
>>>>> +
>>>>> +    err = xe_pcode_read(gt, mbox, val, NULL);
>>>> but why not simply modifying the existent one to accept 2 params?
>>>>
>>>> int xe_pcode_read(struct xe_gt *gt, u32 mbox_param1, u32 mbox_param2,
>>>>                 u32 *val, u32 *val1)
>>>>
>>>> and the equivalent write...
>>>>
>>>> oh, and while doing that, could you please add the missing documentation
>>>> to these 2 functions?
>>>>
>>>> Thanks,
>>>> Rodrigo.
>>> Sure that would work. Will add the docs as well.
>>>
>>> Thanks,
>>>
>>> Suja
>> Hi Rodrigo,
>>
>> Another question,
>>
>> I can change the existing pcode_read function, but would it be better to
>> have a separate new write equivalent ?
> I wonder if we should do s/xe_pcode_write_timeout(/xe_pcode_write(
>
> where timeout is still an argument but it can be null.
> And then we merge with your options here and make a single write fn.

I'll see if this works out.

Thanks,

Suja

>
>>>>> +
>>>>> +    return err;
>>>>> +}
>>>>> +
>>>>> +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
>>>>> p2, u32 val)
>>>>> +{
>>>>> +    u32 mbox;
>>>>> +    int err;
>>>>> +
>>>>> +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
>>>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
>>>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
>>>>> +
>>>>> +    err = xe_pcode_write(gt, mbox, val);
>>>>> +
>>>>> +    return err;
>>>>> +}
>>>>> +
>>>>>    static int xe_pcode_try_request(struct xe_gt *gt, u32 mbox,
>>>>>                    u32 request, u32 reply_mask, u32 reply,
>>>>>                    u32 *status, bool atomic, int timeout_us)
>>>>> diff --git a/drivers/gpu/drm/xe/xe_pcode.h
>>>>> b/drivers/gpu/drm/xe/xe_pcode.h
>>>>> index 3b4aa8c1a3ba..8d4103afd7e0 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_pcode.h
>>>>> +++ b/drivers/gpu/drm/xe/xe_pcode.h
>>>>> @@ -19,6 +19,9 @@ int xe_pcode_write_timeout(struct xe_gt *gt,
>>>>> u32 mbox, u32 val,
>>>>>    #define xe_pcode_write(gt, mbox, val) \
>>>>>        xe_pcode_write_timeout(gt, mbox, val, 1)
>>>>>    +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
>>>>> p2, u32 *val);
>>>>> +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
>>>>> p2, u32 val);
>>>>> +
>>>>>    int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
>>>>>                 u32 reply_mask, u32 reply, int timeout_ms);
>>>>>    --
>>>>> 2.25.1
>>>>>

Re: [Intel-xe] [PATCH 1/3] drm/xe: Add a couple of pcode helpers

[Sundaresan, Sujaritha]

On 9/13/2023 9:33 AM, Sundaresan, Sujaritha wrote:
>
> On 9/12/2023 8:03 PM, Rodrigo Vivi wrote:
>> On Tue, Sep 12, 2023 at 10:38:50AM +0530, Sundaresan, Sujaritha wrote:
>>> On 9/3/2023 7:00 PM, Sundaresan, Sujaritha wrote:
>>>> On 9/2/2023 2:04 AM, Rodrigo Vivi wrote:
>>>>> On Fri, Sep 01, 2023 at 05:45:43PM +0530, Sujaritha Sundaresan wrote:
>>>>>> Some pcode commands take additional sub-commands and parameters. 
>>>>>> Add a
>>>>>> couple of helpers to help formatting these commands to improve code
>>>>>> readability.
>>>>>>
>>>>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/xe/xe_pcode.c | 28 ++++++++++++++++++++++++++++
>>>>>>    drivers/gpu/drm/xe/xe_pcode.h |  3 +++
>>>>>>    2 files changed, 31 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/xe/xe_pcode.c
>>>>>> b/drivers/gpu/drm/xe/xe_pcode.c
>>>>>> index 7f1bf2297f51..e45169f47500 100644
>>>>>> --- a/drivers/gpu/drm/xe/xe_pcode.c
>>>>>> +++ b/drivers/gpu/drm/xe/xe_pcode.c
>>>>>> @@ -104,6 +104,34 @@ int xe_pcode_read(struct xe_gt *gt, u32
>>>>>> mbox, u32 *val, u32 *val1)
>>>>>>        return err;
>>>>>>    }
>>>>> a doc would be required...
>>>>>
>>>>>> +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
>>>>>> p2, u32 *val)
>>>>> a better name would be nice....
>>>>>
>>>>>> +{
>>>>>> +    u32 mbox;
>>>>>> +    int err;
>>>>>> +
>>>>>> +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
>>>>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
>>>>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
>>>>>> +
>>>>>> +    err = xe_pcode_read(gt, mbox, val, NULL);
>>>>> but why not simply modifying the existent one to accept 2 params?
>>>>>
>>>>> int xe_pcode_read(struct xe_gt *gt, u32 mbox_param1, u32 mbox_param2,
>>>>>                 u32 *val, u32 *val1)
>>>>>
>>>>> and the equivalent write...
>>>>>
>>>>> oh, and while doing that, could you please add the missing 
>>>>> documentation
>>>>> to these 2 functions?
>>>>>
>>>>> Thanks,
>>>>> Rodrigo.
>>>> Sure that would work. Will add the docs as well.
>>>>
>>>> Thanks,
>>>>
>>>> Suja
>>> Hi Rodrigo,
>>>
>>> Another question,
>>>
>>> I can change the existing pcode_read function, but would it be 
>>> better to
>>> have a separate new write equivalent ?
>> I wonder if we should do s/xe_pcode_write_timeout(/xe_pcode_write(
>>
>> where timeout is still an argument but it can be null.
>> And then we merge with your options here and make a single write fn.
>
> I'll see if this works out.
>
> Thanks,
>
> Suja
>
Hi Rodrigo,

As I am making the final fixes, looks like it would be better to have 
separate new read/write functions.

With changing the existing read/write functions, we are having to change 
the pcode_mailbox_rw function.

That is causing issues with other existing functions. Shall I keep the 
new functions defined separately ?

Thanks,

Suja

>>
>>>>>> +
>>>>>> +    return err;
>>>>>> +}
>>>>>> +
>>>>>> +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
>>>>>> p2, u32 val)
>>>>>> +{
>>>>>> +    u32 mbox;
>>>>>> +    int err;
>>>>>> +
>>>>>> +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
>>>>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
>>>>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
>>>>>> +
>>>>>> +    err = xe_pcode_write(gt, mbox, val);
>>>>>> +
>>>>>> +    return err;
>>>>>> +}
>>>>>> +
>>>>>>    static int xe_pcode_try_request(struct xe_gt *gt, u32 mbox,
>>>>>>                    u32 request, u32 reply_mask, u32 reply,
>>>>>>                    u32 *status, bool atomic, int timeout_us)
>>>>>> diff --git a/drivers/gpu/drm/xe/xe_pcode.h
>>>>>> b/drivers/gpu/drm/xe/xe_pcode.h
>>>>>> index 3b4aa8c1a3ba..8d4103afd7e0 100644
>>>>>> --- a/drivers/gpu/drm/xe/xe_pcode.h
>>>>>> +++ b/drivers/gpu/drm/xe/xe_pcode.h
>>>>>> @@ -19,6 +19,9 @@ int xe_pcode_write_timeout(struct xe_gt *gt,
>>>>>> u32 mbox, u32 val,
>>>>>>    #define xe_pcode_write(gt, mbox, val) \
>>>>>>        xe_pcode_write_timeout(gt, mbox, val, 1)
>>>>>>    +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
>>>>>> p2, u32 *val);
>>>>>> +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
>>>>>> p2, u32 val);
>>>>>> +
>>>>>>    int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
>>>>>>                 u32 reply_mask, u32 reply, int timeout_ms);
>>>>>>    --
>>>>>> 2.25.1
>>>>>>

Re: [Intel-xe] [PATCH 1/3] drm/xe: Add a couple of pcode helpers

[Rodrigo Vivi]

On Mon, Sep 25, 2023 at 01:43:07PM +0530, Sundaresan, Sujaritha wrote:
> 
> On 9/13/2023 9:33 AM, Sundaresan, Sujaritha wrote:
> > 
> > On 9/12/2023 8:03 PM, Rodrigo Vivi wrote:
> > > On Tue, Sep 12, 2023 at 10:38:50AM +0530, Sundaresan, Sujaritha wrote:
> > > > On 9/3/2023 7:00 PM, Sundaresan, Sujaritha wrote:
> > > > > On 9/2/2023 2:04 AM, Rodrigo Vivi wrote:
> > > > > > On Fri, Sep 01, 2023 at 05:45:43PM +0530, Sujaritha Sundaresan wrote:
> > > > > > > Some pcode commands take additional sub-commands and
> > > > > > > parameters. Add a
> > > > > > > couple of helpers to help formatting these commands to improve code
> > > > > > > readability.
> > > > > > > 
> > > > > > > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> > > > > > > ---
> > > > > > >    drivers/gpu/drm/xe/xe_pcode.c | 28 ++++++++++++++++++++++++++++
> > > > > > >    drivers/gpu/drm/xe/xe_pcode.h |  3 +++
> > > > > > >    2 files changed, 31 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_pcode.c
> > > > > > > b/drivers/gpu/drm/xe/xe_pcode.c
> > > > > > > index 7f1bf2297f51..e45169f47500 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_pcode.c
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_pcode.c
> > > > > > > @@ -104,6 +104,34 @@ int xe_pcode_read(struct xe_gt *gt, u32
> > > > > > > mbox, u32 *val, u32 *val1)
> > > > > > >        return err;
> > > > > > >    }
> > > > > > a doc would be required...
> > > > > > 
> > > > > > > +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
> > > > > > > p2, u32 *val)
> > > > > > a better name would be nice....
> > > > > > 
> > > > > > > +{
> > > > > > > +    u32 mbox;
> > > > > > > +    int err;
> > > > > > > +
> > > > > > > +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
> > > > > > > +        | REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
> > > > > > > +        | REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
> > > > > > > +
> > > > > > > +    err = xe_pcode_read(gt, mbox, val, NULL);
> > > > > > but why not simply modifying the existent one to accept 2 params?
> > > > > > 
> > > > > > int xe_pcode_read(struct xe_gt *gt, u32 mbox_param1, u32 mbox_param2,
> > > > > >                 u32 *val, u32 *val1)
> > > > > > 
> > > > > > and the equivalent write...
> > > > > > 
> > > > > > oh, and while doing that, could you please add the
> > > > > > missing documentation
> > > > > > to these 2 functions?
> > > > > > 
> > > > > > Thanks,
> > > > > > Rodrigo.
> > > > > Sure that would work. Will add the docs as well.
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > Suja
> > > > Hi Rodrigo,
> > > > 
> > > > Another question,
> > > > 
> > > > I can change the existing pcode_read function, but would it be
> > > > better to
> > > > have a separate new write equivalent ?
> > > I wonder if we should do s/xe_pcode_write_timeout(/xe_pcode_write(
> > > 
> > > where timeout is still an argument but it can be null.
> > > And then we merge with your options here and make a single write fn.
> > 
> > I'll see if this works out.
> > 
> > Thanks,
> > 
> > Suja
> > 
> Hi Rodrigo,
> 
> As I am making the final fixes, looks like it would be better to have
> separate new read/write functions.
> 
> With changing the existing read/write functions, we are having to change the
> pcode_mailbox_rw function.
> 
> That is causing issues with other existing functions. Shall I keep the new
> functions defined separately ?

I'm really sorry for having missed this here.
I still believe that there are ways of combining them, even if we have
to change the internal rw function. But if that gets too complicated or too
ugly fine, let's have something alternative, but with a better naming

> 
> Thanks,
> 
> Suja
> 
> > > 
> > > > > > > +
> > > > > > > +    return err;
> > > > > > > +}
> > > > > > > +
> > > > > > > +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
> > > > > > > p2, u32 val)
> > > > > > > +{
> > > > > > > +    u32 mbox;
> > > > > > > +    int err;
> > > > > > > +
> > > > > > > +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
> > > > > > > +        | REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
> > > > > > > +        | REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
> > > > > > > +
> > > > > > > +    err = xe_pcode_write(gt, mbox, val);
> > > > > > > +
> > > > > > > +    return err;
> > > > > > > +}
> > > > > > > +
> > > > > > >    static int xe_pcode_try_request(struct xe_gt *gt, u32 mbox,
> > > > > > >                    u32 request, u32 reply_mask, u32 reply,
> > > > > > >                    u32 *status, bool atomic, int timeout_us)
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_pcode.h
> > > > > > > b/drivers/gpu/drm/xe/xe_pcode.h
> > > > > > > index 3b4aa8c1a3ba..8d4103afd7e0 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_pcode.h
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_pcode.h
> > > > > > > @@ -19,6 +19,9 @@ int xe_pcode_write_timeout(struct xe_gt *gt,
> > > > > > > u32 mbox, u32 val,
> > > > > > >    #define xe_pcode_write(gt, mbox, val) \
> > > > > > >        xe_pcode_write_timeout(gt, mbox, val, 1)
> > > > > > >    +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
> > > > > > > p2, u32 *val);
> > > > > > > +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
> > > > > > > p2, u32 val);
> > > > > > > +
> > > > > > >    int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
> > > > > > >                 u32 reply_mask, u32 reply, int timeout_ms);
> > > > > > >    --
> > > > > > > 2.25.1
> > > > > > >