Intel-XE Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/2] Implement stats layer to measure events
@ 2024-07-25 15:05 Nirmoy Das
  2024-07-25 15:05 ` [RFC PATCH 1/2] drm/xe: Implement APIs for measuring various events Nirmoy Das
  2024-07-25 15:05 ` [PATCH] drm/xe: Keep track of TLB inval events Nirmoy Das
  0 siblings, 2 replies; 7+ messages in thread
From: Nirmoy Das @ 2024-07-25 15:05 UTC (permalink / raw)
  To: intel-xe; +Cc: Nirmoy Das, Matthew Brost

Add basic stats layer to track and show stats of various events over
debugfs. Tagging it as RFC to gather inputs on how the final
APIs/implementation should look like.

Cc: Matthew Brost <matthew.brost@intel.com>
Nirmoy Das (2):
  drm/xe: Implement APIs for measuring various events
  drm/xe: Keep track of TLB inval events

 drivers/gpu/drm/xe/Kconfig.debug            |  11 ++
 drivers/gpu/drm/xe/Makefile                 |   1 +
 drivers/gpu/drm/xe/xe_gt_debugfs.c          |  11 ++
 drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c |  31 +++--
 drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h |   1 +
 drivers/gpu/drm/xe/xe_gt_types.h            |   4 +
 drivers/gpu/drm/xe/xe_stats.c               | 145 ++++++++++++++++++++
 drivers/gpu/drm/xe/xe_stats.h               |  66 +++++++++
 drivers/gpu/drm/xe/xe_uc_fw.c               |   2 +-
 9 files changed, 262 insertions(+), 10 deletions(-)
 create mode 100644 drivers/gpu/drm/xe/xe_stats.c
 create mode 100644 drivers/gpu/drm/xe/xe_stats.h

-- 
2.42.0


^ permalink raw reply	[flat|nested] 7+ messages in thread
* [RFC PATCH 1/2] drm/xe: Implement APIs for measuring various events
  2024-07-25 15:05 [RFC PATCH 0/2] Implement stats layer to measure events Nirmoy Das
@ 2024-07-25 15:05 ` Nirmoy Das
  2024-07-25 15:42   ` Matthew Brost
  2024-07-25 15:48   ` Michal Wajdeczko
  2024-07-25 15:05 ` [PATCH] drm/xe: Keep track of TLB inval events Nirmoy Das
  1 sibling, 2 replies; 7+ messages in thread
From: Nirmoy Das @ 2024-07-25 15:05 UTC (permalink / raw)
  To: intel-xe; +Cc: Nirmoy Das, Matthew Brost

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=a, Size: 6881 bytes --]

Introduces a set of APIs for tracking and managing events.
This currently only supports counters.

This can be conditionally enabled with the CONFIG_DRM_XE_STATS
config option.

Cc: Matthew Brost <matthew.brost@intel.com>
Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/xe/Kconfig.debug |  11 +++
 drivers/gpu/drm/xe/Makefile      |   1 +
 drivers/gpu/drm/xe/xe_stats.c    | 145 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_stats.h    |  66 ++++++++++++++
 4 files changed, 223 insertions(+)
 create mode 100644 drivers/gpu/drm/xe/xe_stats.c
 create mode 100644 drivers/gpu/drm/xe/xe_stats.h

diff --git a/drivers/gpu/drm/xe/Kconfig.debug b/drivers/gpu/drm/xe/Kconfig.debug
index bc177368af6c..4db6d707c1af 100644
--- a/drivers/gpu/drm/xe/Kconfig.debug
+++ b/drivers/gpu/drm/xe/Kconfig.debug
@@ -94,3 +94,14 @@ config DRM_XE_USERPTR_INVAL_INJECT
 
 	 Recomended for driver developers only.
 	 If in doubt, say "N".
+
+config DRM_XE_STATS
+	bool "Enable XE statistics"
+	default n
+	help
+	  Choose this option to enable support for collecting and
+	  displaying XE statistics as debugfs.
+
+	  Recommended for driver developers only.
+
+	  If in doubt, say "N".
diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index 1ff9602a52f6..c5d9ec22c6bf 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -117,6 +117,7 @@ xe-y += xe_bb.o \
 	xe_wopcm.o
 
 xe-$(CONFIG_HMM_MIRROR) += xe_hmm.o
+xe-$(CONFIG_DRM_XE_STATS) += xe_stats.o
 
 # graphics hardware monitoring (HWMON) support
 xe-$(CONFIG_HWMON) += xe_hwmon.o
diff --git a/drivers/gpu/drm/xe/xe_stats.c b/drivers/gpu/drm/xe/xe_stats.c
new file mode 100644
index 000000000000..1b8910900fd3
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_stats.c
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/slab.h>
+
+#include <drm/drm_device.h>
+#include <drm/drm_managed.h>
+#include <drm/drm_print.h>
+
+#include "xe_stats.h"
+
+#ifdef CONFIG_DRM_XE_STATS
+static struct xe_stats_entry *find_stats_entry(struct xe_stats *stats, const char *name)
+{
+	struct xe_stats_entry *stats_entry;
+
+	list_for_each_entry(stats_entry, &stats->entries, list) {
+		if (strcmp(stats_entry->name, name) == 0)
+			return stats_entry;
+	}
+
+	return NULL;
+}
+
+static int stats_show(struct seq_file *m, void *v)
+{
+	struct xe_stats_entry *entry = m->private;
+
+	if (entry && !entry->enabled)
+		return 0;
+
+	switch (entry->type) {
+	case XE_STATS_TYPE_COUNTER:
+		seq_printf(m, "%s: Counter value: %llu\n", entry->name,
+			   atomic64_read(&entry->data.counter.value));
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int stats_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, stats_show, inode->i_private);
+}
+
+static const struct file_operations stats_fops = {
+	.owner   = THIS_MODULE,
+	.open    = stats_open,
+	.read    = seq_read,
+	.release = single_release,
+};
+
+struct xe_stats *xe_stats_init(struct drm_device *drm, struct dentry *parent)
+{
+	struct xe_stats *stats;
+
+	stats = drmm_kzalloc(drm, sizeof(*stats), GFP_KERNEL);
+	if (!stats) {
+		drm_err(drm, "Failed to allocate memory for xe_stats\n");
+		return NULL;
+	}
+
+	stats->drm = drm;
+	INIT_LIST_HEAD(&stats->entries);
+
+	stats->debugfs_entry = debugfs_create_dir("stats", parent);
+	if (!stats->debugfs_entry) {
+		drm_err(drm, "Failed to create stats debugfs directory\n");
+		return NULL;
+	}
+
+	return stats;
+}
+
+void xe_stats_add_entry(struct xe_stats *stats, const char *name, enum xe_stats_type type)
+{
+	struct xe_stats_entry *entry;
+	struct dentry *debugfs_entry;
+
+	if (!stats || !name)
+		return;
+
+	entry = drmm_kzalloc(stats->drm, sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return;
+
+	entry->name = drmm_kstrdup(stats->drm, name, GFP_KERNEL);
+	if (!entry->name)
+		return;
+
+	entry->type = type;
+	entry->enabled = true;
+
+	switch (type) {
+	case XE_STATS_TYPE_COUNTER:
+		atomic64_set(&entry->data.counter.value, 0);
+		break;
+	default:
+		return;
+	}
+
+	list_add(&entry->list, &stats->entries);
+
+	debugfs_entry = debugfs_create_file(name, 0444, stats->debugfs_entry, entry, &stats_fops);
+	if (!debugfs_entry) {
+		drm_err(stats->drm, "Failed to create stats debugfs file for %s\n", name);
+		list_del(&entry->list);
+		return;
+	}
+
+	entry->dentry = debugfs_entry;
+}
+
+void xe_stats_increment_counter(struct xe_stats *stats, const char *name)
+{
+	struct xe_stats_entry *entry;
+
+	if (!stats || !name)
+		return;
+
+	entry = find_stats_entry(stats, name);
+	if (entry && entry->type == XE_STATS_TYPE_COUNTER && entry->enabled)
+		atomic64_inc(&entry->data.counter.value);
+}
+
+void xe_stats_decrement_counter(struct xe_stats *stats, const char *name)
+{
+	struct xe_stats_entry *entry;
+
+	if (!stats || !name)
+		return;
+
+	entry = find_stats_entry(stats, name);
+	if (entry && entry->type == XE_STATS_TYPE_COUNTER && entry->enabled)
+		atomic64_dec(&entry->data.counter.value);
+}
+
+#endif
diff --git a/drivers/gpu/drm/xe/xe_stats.h b/drivers/gpu/drm/xe/xe_stats.h
new file mode 100644
index 000000000000..dba79ae28714
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_stats.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+#ifndef _XE_STATS_H_
+#define _XE_STATS_H_
+
+#include <linux/atomic.h>
+#include <linux/debugfs.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+#include <drm/drm_print.h>
+
+struct drm_device;
+
+enum xe_stats_type {
+	XE_STATS_TYPE_COUNTER,
+};
+
+#ifdef CONFIG_DRM_XE_STATS
+struct xe_stats_entry {
+	const char *name;
+	enum xe_stats_type type;
+	bool enabled;
+	union {
+		struct {
+			atomic64_t value;
+		} counter;
+	} data;
+	struct list_head list;
+	struct dentry *dentry;
+};
+
+struct xe_stats {
+	struct dentry *debugfs_entry;
+	struct list_head entries;
+	struct drm_device *drm;
+};
+
+struct xe_stats *xe_stats_init(struct drm_device *drm, struct dentry *parent);
+void xe_stats_add_entry(struct xe_stats *stats, const char *name, enum xe_stats_type type);
+void xe_stats_increment_counter(struct xe_stats *c, const char *name);
+void xe_stats_decrement_counter(struct xe_stats *stats, const char *name);
+
+#else /* CONFIG_DRM_XE_STATS not defined */
+struct xe_stats;
+
+static inline struct xe_stats *
+xe_stats_init(struct drm_device *drm, struct dentry *parent)
+{
+	return NULL;
+}
+
+static inline void
+xe_stats_add_entry(struct xe_stats *stats, const char *name, enum xe_stats_type type) { }
+static inline void
+xe_stats_increment_counter(struct xe_stats *stats, const char *name) { }
+static inline void
+xe_stats_decrement_counter(struct xe_stats *stats, const char *name) { }
+
+#endif /* CONFIG_DRM_XE_STATS */
+
+#endif /* _XE_STATS_H_ */
+
-- 
2.42.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [PATCH] drm/xe: Keep track of TLB inval events
  2024-07-25 15:05 [RFC PATCH 0/2] Implement stats layer to measure events Nirmoy Das
  2024-07-25 15:05 ` [RFC PATCH 1/2] drm/xe: Implement APIs for measuring various events Nirmoy Das
@ 2024-07-25 15:05 ` Nirmoy Das
  1 sibling, 0 replies; 7+ messages in thread
From: Nirmoy Das @ 2024-07-25 15:05 UTC (permalink / raw)
  To: intel-xe; +Cc: Nirmoy Das

Use stats API to keep track of TLB invalidation events per GT.

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/xe/xe_gt_debugfs.c          | 10 +++++++
 drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c | 31 +++++++++++++++------
 drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h |  1 +
 drivers/gpu/drm/xe/xe_gt_types.h            |  4 +++
 drivers/gpu/drm/xe/xe_uc_fw.c               |  2 +-
 5 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_gt_debugfs.c b/drivers/gpu/drm/xe/xe_gt_debugfs.c
index 5e7fd937917a..28a9aa18d3bc 100644
--- a/drivers/gpu/drm/xe/xe_gt_debugfs.c
+++ b/drivers/gpu/drm/xe/xe_gt_debugfs.c
@@ -17,6 +17,7 @@
 #include "xe_gt_mcr.h"
 #include "xe_gt_sriov_pf_debugfs.h"
 #include "xe_gt_sriov_vf_debugfs.h"
+#include "xe_gt_tlb_invalidation.h"
 #include "xe_gt_topology.h"
 #include "xe_hw_engine.h"
 #include "xe_lrc.h"
@@ -27,6 +28,7 @@
 #include "xe_reg_sr.h"
 #include "xe_reg_whitelist.h"
 #include "xe_sriov.h"
+#include "xe_stats.h"
 #include "xe_uc_debugfs.h"
 #include "xe_wa.h"
 
@@ -288,6 +290,12 @@ static const struct drm_info_list debugfs_list[] = {
 	{"default_lrc_vecs", .show = xe_gt_debugfs_simple_show, .data = vecs_default_lrc},
 };
 
+static void xe_gt_debugfs_stats_register(struct xe_gt *gt, struct dentry *root)
+{
+	gt->stats = xe_stats_init(&gt_to_xe(gt)->drm, root);
+	xe_gt_tlb_invalidation_stats_init(gt);
+}
+
 void xe_gt_debugfs_register(struct xe_gt *gt)
 {
 	struct xe_device *xe = gt_to_xe(gt);
@@ -321,4 +329,6 @@ void xe_gt_debugfs_register(struct xe_gt *gt)
 		xe_gt_sriov_pf_debugfs_register(gt, root);
 	else if (IS_SRIOV_VF(xe))
 		xe_gt_sriov_vf_debugfs_register(gt, root);
+
+	xe_gt_debugfs_stats_register(gt, root);
 }
diff --git a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
index 87cb76a8718c..60f9b8bc4121 100644
--- a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
+++ b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
@@ -37,11 +37,13 @@ static long tlb_timeout_jiffies(struct xe_gt *gt)
 }
 
 static void
-__invalidation_fence_signal(struct xe_device *xe, struct xe_gt_tlb_invalidation_fence *fence)
+__invalidation_fence_signal(struct xe_gt *gt, struct xe_gt_tlb_invalidation_fence *fence)
 {
+	struct xe_device *xe = gt_to_xe(gt);
 	bool stack = test_bit(FENCE_STACK_BIT, &fence->base.flags);
 
 	trace_xe_gt_tlb_invalidation_fence_signal(xe, fence);
+	xe_stats_decrement_counter(gt->stats, "TLB_INVAL_INFLIGHT");
 	xe_gt_tlb_invalidation_fence_fini(fence);
 	dma_fence_signal(&fence->base);
 	if (!stack)
@@ -49,10 +51,10 @@ __invalidation_fence_signal(struct xe_device *xe, struct xe_gt_tlb_invalidation_
 }
 
 static void
-invalidation_fence_signal(struct xe_device *xe, struct xe_gt_tlb_invalidation_fence *fence)
+invalidation_fence_signal(struct xe_gt *gt, struct xe_gt_tlb_invalidation_fence *fence)
 {
 	list_del(&fence->link);
-	__invalidation_fence_signal(xe, fence);
+	__invalidation_fence_signal(gt, fence);
 }
 
 static void xe_gt_tlb_fence_timeout(struct work_struct *work)
@@ -76,7 +78,7 @@ static void xe_gt_tlb_fence_timeout(struct work_struct *work)
 			  fence->seqno, gt->tlb_invalidation.seqno_recv);
 
 		fence->base.error = -ETIME;
-		invalidation_fence_signal(xe, fence);
+		invalidation_fence_signal(gt, fence);
 	}
 	if (!list_empty(&gt->tlb_invalidation.pending_fences))
 		queue_delayed_work(system_wq,
@@ -140,7 +142,7 @@ void xe_gt_tlb_invalidation_reset(struct xe_gt *gt)
 
 	list_for_each_entry_safe(fence, next,
 				 &gt->tlb_invalidation.pending_fences, link)
-		invalidation_fence_signal(gt_to_xe(gt), fence);
+		invalidation_fence_signal(gt, fence);
 	spin_unlock_irq(&gt->tlb_invalidation.pending_lock);
 	mutex_unlock(&gt->uc.guc.ct.lock);
 }
@@ -183,6 +185,9 @@ static int send_tlb_invalidation(struct xe_guc *guc,
 	ret = xe_guc_ct_send_locked(&guc->ct, action, len,
 				    G2H_LEN_DW_TLB_INVALIDATE, 1);
 	if (!ret) {
+		xe_stats_increment_counter(gt->stats, "TLB_INVAL_INFLIGHT");
+		xe_stats_increment_counter(gt->stats, "TLB_INVAL_TOTAL_SENT");
+
 		spin_lock_irq(&gt->tlb_invalidation.pending_lock);
 		/*
 		 * We haven't actually published the TLB fence as per
@@ -191,7 +196,7 @@ static int send_tlb_invalidation(struct xe_guc *guc,
 		 * we can just go ahead and signal the fence here.
 		 */
 		if (tlb_invalidation_seqno_past(gt, seqno)) {
-			__invalidation_fence_signal(xe, fence);
+			__invalidation_fence_signal(gt, fence);
 		} else {
 			fence->invalidation_time = ktime_get();
 			list_add_tail(&fence->link,
@@ -204,7 +209,7 @@ static int send_tlb_invalidation(struct xe_guc *guc,
 		}
 		spin_unlock_irq(&gt->tlb_invalidation.pending_lock);
 	} else if (ret < 0) {
-		__invalidation_fence_signal(xe, fence);
+		__invalidation_fence_signal(gt, fence);
 	}
 	if (!ret) {
 		gt->tlb_invalidation.seqno = (gt->tlb_invalidation.seqno + 1) %
@@ -321,7 +326,7 @@ int xe_gt_tlb_invalidation_range(struct xe_gt *gt,
 
 	/* Execlists not supported */
 	if (gt_to_xe(gt)->info.force_execlist) {
-		__invalidation_fence_signal(xe, fence);
+		__invalidation_fence_signal(gt, fence);
 		return 0;
 	}
 
@@ -455,7 +460,7 @@ int xe_guc_tlb_invalidation_done_handler(struct xe_guc *guc, u32 *msg, u32 len)
 		if (!tlb_invalidation_seqno_past(gt, fence->seqno))
 			break;
 
-		invalidation_fence_signal(xe, fence);
+		invalidation_fence_signal(gt, fence);
 	}
 
 	if (!list_empty(&gt->tlb_invalidation.pending_fences))
@@ -525,3 +530,11 @@ void xe_gt_tlb_invalidation_fence_fini(struct xe_gt_tlb_invalidation_fence *fenc
 {
 	xe_pm_runtime_put(gt_to_xe(fence->gt));
 }
+
+void xe_gt_tlb_invalidation_stats_init(struct xe_gt *gt)
+{
+	xe_stats_add_entry(gt->stats, "TLB_INVAL_TOTAL_SENT",
+			   XE_STATS_TYPE_COUNTER);
+	xe_stats_add_entry(gt->stats, "TLB_INVAL_INFLIGHT",
+			   XE_STATS_TYPE_COUNTER);
+}
diff --git a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h
index a84065fa324c..62180379cc40 100644
--- a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h
+++ b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h
@@ -36,4 +36,5 @@ xe_gt_tlb_invalidation_fence_wait(struct xe_gt_tlb_invalidation_fence *fence)
 	dma_fence_wait(&fence->base, false);
 }
 
+void xe_gt_tlb_invalidation_stats_init(struct xe_gt *gt);
 #endif	/* _XE_GT_TLB_INVALIDATION_ */
diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
index 631928258d71..702b21ac611a 100644
--- a/drivers/gpu/drm/xe/xe_gt_types.h
+++ b/drivers/gpu/drm/xe/xe_gt_types.h
@@ -15,6 +15,7 @@
 #include "xe_oa.h"
 #include "xe_reg_sr_types.h"
 #include "xe_sa_types.h"
+#include "xe_stats.h"
 #include "xe_uc_types.h"
 
 struct xe_exec_queue_ops;
@@ -133,6 +134,9 @@ struct xe_gt {
 		u8 has_indirect_ring_state:1;
 	} info;
 
+	/* @stats: Track stats of various events, currently TLB inval */
+	struct xe_stats *stats;
+
 	/**
 	 * @mmio: mmio info for GT.  All GTs within a tile share the same
 	 * register space, but have their own copy of GSI registers at a
diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
index c1dcf58d25d7..0e51d853fa4a 100644
--- a/drivers/gpu/drm/xe/xe_uc_fw.c
+++ b/drivers/gpu/drm/xe/xe_uc_fw.c
@@ -258,7 +258,7 @@ uc_fw_override(struct xe_uc_fw *uc_fw)
 		path_override = xe_modparam.huc_firmware_path;
 		break;
 	case XE_UC_FW_TYPE_GSC:
-		path_override = xe_modparam.gsc_firmware_path;
+		path_override = "";
 		break;
 	default:
 		break;
-- 
2.42.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [RFC PATCH 1/2] drm/xe: Implement APIs for measuring various events
  2024-07-25 15:05 ` [RFC PATCH 1/2] drm/xe: Implement APIs for measuring various events Nirmoy Das
@ 2024-07-25 15:42   ` Matthew Brost
  2024-07-25 15:48   ` Michal Wajdeczko
  1 sibling, 0 replies; 7+ messages in thread
From: Matthew Brost @ 2024-07-25 15:42 UTC (permalink / raw)
  To: Nirmoy Das; +Cc: intel-xe

On Thu, Jul 25, 2024 at 05:05:44PM +0200, Nirmoy Das wrote:
> Introduces a set of APIs for tracking and managing events.
> This currently only supports counters.
> 
> This can be conditionally enabled with the CONFIG_DRM_XE_STATS
> config option.
> 

Idea is right but thinking the implementation is overly complicated.

Roughly what I think, in this example a GT stat.

struct xe_gt {
	...
#if IS_ENABLED(CONFIG_DRM_XE_STATS)
        struct {
                atomic_t value[XE_GT_STAT_COUNT];
        } stats;
#endif
	...
};

enum xe_gt_stat {
	XE_GT_STAT_FOO,
	XE_GT_STAT_COUNT,
};

void xe_gt_stats_incr(struct xe_gt *gt, enum xe_gt_stat xe_gt_stat, int incr)
{
        atomic_add(incr, &gt->stats.value[xe_gt_stat]);
}

static const char *stat_description[] = {
        "foo",
};

void xe_gt_stats_print_info(struct xe_gt *gt, struct drm_printer *p)
{
        enum xe_gt_stat xe_gt_stat;

        for (xe_gt_stat = 0; xe_gt_stat < XE_GT_STAT_COUNT; ++xe_gt_stat)
                drm_printf(p, "%s: %d\n", stat_description[xe_gt_stat],
                           atomic_read(&gt->stats.value[xe_gt_stat]));
}

With xe_gt_stats_incr used at the caller which wants to increment stat:
xe_gt_stats_incr(gt, XE_GT_STAT_FOO, 1);

Add a 'stats' GT debugfs entry (see xe_gt_debugfs.c) which calls
'xe_gt_stats_print_info'.

We can also add 'xe_device' stats too.

This is simplier as no need to configure stats, just add an enum + table
entry, and incr caller.

Also leverages existing debugfs code, honors existing layering, and no
need to malloc anything either.

Matt

> Cc: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>  drivers/gpu/drm/xe/Kconfig.debug |  11 +++
>  drivers/gpu/drm/xe/Makefile      |   1 +
>  drivers/gpu/drm/xe/xe_stats.c    | 145 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_stats.h    |  66 ++++++++++++++
>  4 files changed, 223 insertions(+)
>  create mode 100644 drivers/gpu/drm/xe/xe_stats.c
>  create mode 100644 drivers/gpu/drm/xe/xe_stats.h
> 
> diff --git a/drivers/gpu/drm/xe/Kconfig.debug b/drivers/gpu/drm/xe/Kconfig.debug
> index bc177368af6c..4db6d707c1af 100644
> --- a/drivers/gpu/drm/xe/Kconfig.debug
> +++ b/drivers/gpu/drm/xe/Kconfig.debug
> @@ -94,3 +94,14 @@ config DRM_XE_USERPTR_INVAL_INJECT
>  
>  	 Recomended for driver developers only.
>  	 If in doubt, say "N".
> +
> +config DRM_XE_STATS
> +	bool "Enable XE statistics"
> +	default n
> +	help
> +	  Choose this option to enable support for collecting and
> +	  displaying XE statistics as debugfs.
> +
> +	  Recommended for driver developers only.
> +
> +	  If in doubt, say "N".
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 1ff9602a52f6..c5d9ec22c6bf 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -117,6 +117,7 @@ xe-y += xe_bb.o \
>  	xe_wopcm.o
>  
>  xe-$(CONFIG_HMM_MIRROR) += xe_hmm.o
> +xe-$(CONFIG_DRM_XE_STATS) += xe_stats.o
>  
>  # graphics hardware monitoring (HWMON) support
>  xe-$(CONFIG_HWMON) += xe_hwmon.o
> diff --git a/drivers/gpu/drm/xe/xe_stats.c b/drivers/gpu/drm/xe/xe_stats.c
> new file mode 100644
> index 000000000000..1b8910900fd3
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_stats.c
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +#include <linux/slab.h>
> +
> +#include <drm/drm_device.h>
> +#include <drm/drm_managed.h>
> +#include <drm/drm_print.h>
> +
> +#include "xe_stats.h"
> +
> +#ifdef CONFIG_DRM_XE_STATS
> +static struct xe_stats_entry *find_stats_entry(struct xe_stats *stats, const char *name)
> +{
> +	struct xe_stats_entry *stats_entry;
> +
> +	list_for_each_entry(stats_entry, &stats->entries, list) {
> +		if (strcmp(stats_entry->name, name) == 0)
> +			return stats_entry;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int stats_show(struct seq_file *m, void *v)
> +{
> +	struct xe_stats_entry *entry = m->private;
> +
> +	if (entry && !entry->enabled)
> +		return 0;
> +
> +	switch (entry->type) {
> +	case XE_STATS_TYPE_COUNTER:
> +		seq_printf(m, "%s: Counter value: %llu\n", entry->name,
> +			   atomic64_read(&entry->data.counter.value));
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int stats_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, stats_show, inode->i_private);
> +}
> +
> +static const struct file_operations stats_fops = {
> +	.owner   = THIS_MODULE,
> +	.open    = stats_open,
> +	.read    = seq_read,
> +	.release = single_release,
> +};
> +
> +struct xe_stats *xe_stats_init(struct drm_device *drm, struct dentry *parent)
> +{
> +	struct xe_stats *stats;
> +
> +	stats = drmm_kzalloc(drm, sizeof(*stats), GFP_KERNEL);
> +	if (!stats) {
> +		drm_err(drm, "Failed to allocate memory for xe_stats\n");
> +		return NULL;
> +	}
> +
> +	stats->drm = drm;
> +	INIT_LIST_HEAD(&stats->entries);
> +
> +	stats->debugfs_entry = debugfs_create_dir("stats", parent);
> +	if (!stats->debugfs_entry) {
> +		drm_err(drm, "Failed to create stats debugfs directory\n");
> +		return NULL;
> +	}
> +
> +	return stats;
> +}
> +
> +void xe_stats_add_entry(struct xe_stats *stats, const char *name, enum xe_stats_type type)
> +{
> +	struct xe_stats_entry *entry;
> +	struct dentry *debugfs_entry;
> +
> +	if (!stats || !name)
> +		return;
> +
> +	entry = drmm_kzalloc(stats->drm, sizeof(*entry), GFP_KERNEL);
> +	if (!entry)
> +		return;
> +
> +	entry->name = drmm_kstrdup(stats->drm, name, GFP_KERNEL);
> +	if (!entry->name)
> +		return;
> +
> +	entry->type = type;
> +	entry->enabled = true;
> +
> +	switch (type) {
> +	case XE_STATS_TYPE_COUNTER:
> +		atomic64_set(&entry->data.counter.value, 0);
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	list_add(&entry->list, &stats->entries);
> +
> +	debugfs_entry = debugfs_create_file(name, 0444, stats->debugfs_entry, entry, &stats_fops);
> +	if (!debugfs_entry) {
> +		drm_err(stats->drm, "Failed to create stats debugfs file for %s\n", name);
> +		list_del(&entry->list);
> +		return;
> +	}
> +
> +	entry->dentry = debugfs_entry;
> +}
> +
> +void xe_stats_increment_counter(struct xe_stats *stats, const char *name)
> +{
> +	struct xe_stats_entry *entry;
> +
> +	if (!stats || !name)
> +		return;
> +
> +	entry = find_stats_entry(stats, name);
> +	if (entry && entry->type == XE_STATS_TYPE_COUNTER && entry->enabled)
> +		atomic64_inc(&entry->data.counter.value);
> +}
> +
> +void xe_stats_decrement_counter(struct xe_stats *stats, const char *name)
> +{
> +	struct xe_stats_entry *entry;
> +
> +	if (!stats || !name)
> +		return;
> +
> +	entry = find_stats_entry(stats, name);
> +	if (entry && entry->type == XE_STATS_TYPE_COUNTER && entry->enabled)
> +		atomic64_dec(&entry->data.counter.value);
> +}
> +
> +#endif
> diff --git a/drivers/gpu/drm/xe/xe_stats.h b/drivers/gpu/drm/xe/xe_stats.h
> new file mode 100644
> index 000000000000..dba79ae28714
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_stats.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +#ifndef _XE_STATS_H_
> +#define _XE_STATS_H_
> +
> +#include <linux/atomic.h>
> +#include <linux/debugfs.h>
> +#include <linux/list.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
> +#include <drm/drm_print.h>
> +
> +struct drm_device;
> +
> +enum xe_stats_type {
> +	XE_STATS_TYPE_COUNTER,
> +};
> +
> +#ifdef CONFIG_DRM_XE_STATS
> +struct xe_stats_entry {
> +	const char *name;
> +	enum xe_stats_type type;
> +	bool enabled;
> +	union {
> +		struct {
> +			atomic64_t value;
> +		} counter;
> +	} data;
> +	struct list_head list;
> +	struct dentry *dentry;
> +};
> +
> +struct xe_stats {
> +	struct dentry *debugfs_entry;
> +	struct list_head entries;
> +	struct drm_device *drm;
> +};
> +
> +struct xe_stats *xe_stats_init(struct drm_device *drm, struct dentry *parent);
> +void xe_stats_add_entry(struct xe_stats *stats, const char *name, enum xe_stats_type type);
> +void xe_stats_increment_counter(struct xe_stats *c, const char *name);
> +void xe_stats_decrement_counter(struct xe_stats *stats, const char *name);
> +
> +#else /* CONFIG_DRM_XE_STATS not defined */
> +struct xe_stats;
> +
> +static inline struct xe_stats *
> +xe_stats_init(struct drm_device *drm, struct dentry *parent)
> +{
> +	return NULL;
> +}
> +
> +static inline void
> +xe_stats_add_entry(struct xe_stats *stats, const char *name, enum xe_stats_type type) { }
> +static inline void
> +xe_stats_increment_counter(struct xe_stats *stats, const char *name) { }
> +static inline void
> +xe_stats_decrement_counter(struct xe_stats *stats, const char *name) { }
> +
> +#endif /* CONFIG_DRM_XE_STATS */
> +
> +#endif /* _XE_STATS_H_ */
> +
> -- 
> 2.42.0
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [RFC PATCH 1/2] drm/xe: Implement APIs for measuring various events
  2024-07-25 15:05 ` [RFC PATCH 1/2] drm/xe: Implement APIs for measuring various events Nirmoy Das
  2024-07-25 15:42   ` Matthew Brost
@ 2024-07-25 15:48   ` Michal Wajdeczko
  2024-07-25 16:27     ` Matthew Brost
  1 sibling, 1 reply; 7+ messages in thread
From: Michal Wajdeczko @ 2024-07-25 15:48 UTC (permalink / raw)
  To: Nirmoy Das, intel-xe; +Cc: Matthew Brost



On 25.07.2024 17:05, Nirmoy Das wrote:
> Introduces a set of APIs for tracking and managing events.
> This currently only supports counters.
> 
> This can be conditionally enabled with the CONFIG_DRM_XE_STATS
> config option.
> 
> Cc: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>  drivers/gpu/drm/xe/Kconfig.debug |  11 +++
>  drivers/gpu/drm/xe/Makefile      |   1 +
>  drivers/gpu/drm/xe/xe_stats.c    | 145 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_stats.h    |  66 ++++++++++++++
>  4 files changed, 223 insertions(+)
>  create mode 100644 drivers/gpu/drm/xe/xe_stats.c
>  create mode 100644 drivers/gpu/drm/xe/xe_stats.h
> 
> diff --git a/drivers/gpu/drm/xe/Kconfig.debug b/drivers/gpu/drm/xe/Kconfig.debug
> index bc177368af6c..4db6d707c1af 100644
> --- a/drivers/gpu/drm/xe/Kconfig.debug
> +++ b/drivers/gpu/drm/xe/Kconfig.debug
> @@ -94,3 +94,14 @@ config DRM_XE_USERPTR_INVAL_INJECT
>  
>  	 Recomended for driver developers only.
>  	 If in doubt, say "N".
> +
> +config DRM_XE_STATS
> +	bool "Enable XE statistics"
> +	default n
> +	help
> +	  Choose this option to enable support for collecting and
> +	  displaying XE statistics as debugfs.
> +
> +	  Recommended for driver developers only.
> +
> +	  If in doubt, say "N".
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 1ff9602a52f6..c5d9ec22c6bf 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -117,6 +117,7 @@ xe-y += xe_bb.o \
>  	xe_wopcm.o
>  
>  xe-$(CONFIG_HMM_MIRROR) += xe_hmm.o
> +xe-$(CONFIG_DRM_XE_STATS) += xe_stats.o
>  
>  # graphics hardware monitoring (HWMON) support
>  xe-$(CONFIG_HWMON) += xe_hwmon.o
> diff --git a/drivers/gpu/drm/xe/xe_stats.c b/drivers/gpu/drm/xe/xe_stats.c
> new file mode 100644
> index 000000000000..1b8910900fd3
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_stats.c
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +#include <linux/slab.h>
> +
> +#include <drm/drm_device.h>
> +#include <drm/drm_managed.h>
> +#include <drm/drm_print.h>
> +
> +#include "xe_stats.h"
> +
> +#ifdef CONFIG_DRM_XE_STATS
> +static struct xe_stats_entry *find_stats_entry(struct xe_stats *stats, const char *name)
> +{
> +	struct xe_stats_entry *stats_entry;
> +
> +	list_for_each_entry(stats_entry, &stats->entries, list) {
> +		if (strcmp(stats_entry->name, name) == 0)
> +			return stats_entry;

hmm, this seems to be very costly and likely will not scale well once we
would like to add more counters

can't we just use CONFIG_DRM_XE_STATS to hide different stats structures
provide nop variants of the helpers that would update them ?

> +	}
> +
> +	return NULL;
> +}
> +
> +static int stats_show(struct seq_file *m, void *v)
> +{
> +	struct xe_stats_entry *entry = m->private;
> +
> +	if (entry && !entry->enabled)
> +		return 0;
> +
> +	switch (entry->type) {
> +	case XE_STATS_TYPE_COUNTER:
> +		seq_printf(m, "%s: Counter value: %llu\n", entry->name,
> +			   atomic64_read(&entry->data.counter.value));
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int stats_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, stats_show, inode->i_private);
> +}
> +
> +static const struct file_operations stats_fops = {
> +	.owner   = THIS_MODULE,
> +	.open    = stats_open,
> +	.read    = seq_read,
> +	.release = single_release,
> +};
> +
> +struct xe_stats *xe_stats_init(struct drm_device *drm, struct dentry *parent)
> +{
> +	struct xe_stats *stats;
> +
> +	stats = drmm_kzalloc(drm, sizeof(*stats), GFP_KERNEL);
> +	if (!stats) {
> +		drm_err(drm, "Failed to allocate memory for xe_stats\n");
> +		return NULL;
> +	}
> +
> +	stats->drm = drm;
> +	INIT_LIST_HEAD(&stats->entries);
> +
> +	stats->debugfs_entry = debugfs_create_dir("stats", parent);
> +	if (!stats->debugfs_entry) {
> +		drm_err(drm, "Failed to create stats debugfs directory\n");
> +		return NULL;
> +	}
> +
> +	return stats;
> +}
> +
> +void xe_stats_add_entry(struct xe_stats *stats, const char *name, enum xe_stats_type type)
> +{
> +	struct xe_stats_entry *entry;
> +	struct dentry *debugfs_entry;
> +
> +	if (!stats || !name)
> +		return;
> +
> +	entry = drmm_kzalloc(stats->drm, sizeof(*entry), GFP_KERNEL);
> +	if (!entry)
> +		return;
> +
> +	entry->name = drmm_kstrdup(stats->drm, name, GFP_KERNEL);
> +	if (!entry->name)
> +		return;
> +
> +	entry->type = type;
> +	entry->enabled = true;
> +
> +	switch (type) {
> +	case XE_STATS_TYPE_COUNTER:
> +		atomic64_set(&entry->data.counter.value, 0);
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	list_add(&entry->list, &stats->entries);
> +
> +	debugfs_entry = debugfs_create_file(name, 0444, stats->debugfs_entry, entry, &stats_fops);
> +	if (!debugfs_entry) {
> +		drm_err(stats->drm, "Failed to create stats debugfs file for %s\n", name);
> +		list_del(&entry->list);
> +		return;
> +	}
> +
> +	entry->dentry = debugfs_entry;
> +}
> +
> +void xe_stats_increment_counter(struct xe_stats *stats, const char *name)
> +{
> +	struct xe_stats_entry *entry;
> +
> +	if (!stats || !name)
> +		return;
> +
> +	entry = find_stats_entry(stats, name);
> +	if (entry && entry->type == XE_STATS_TYPE_COUNTER && entry->enabled)
> +		atomic64_inc(&entry->data.counter.value);
> +}
> +
> +void xe_stats_decrement_counter(struct xe_stats *stats, const char *name)
> +{
> +	struct xe_stats_entry *entry;
> +
> +	if (!stats || !name)
> +		return;
> +
> +	entry = find_stats_entry(stats, name);
> +	if (entry && entry->type == XE_STATS_TYPE_COUNTER && entry->enabled)
> +		atomic64_dec(&entry->data.counter.value);
> +}
> +
> +#endif
> diff --git a/drivers/gpu/drm/xe/xe_stats.h b/drivers/gpu/drm/xe/xe_stats.h
> new file mode 100644
> index 000000000000..dba79ae28714
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_stats.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +#ifndef _XE_STATS_H_
> +#define _XE_STATS_H_
> +
> +#include <linux/atomic.h>
> +#include <linux/debugfs.h>
> +#include <linux/list.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
> +#include <drm/drm_print.h>
> +
> +struct drm_device;
> +
> +enum xe_stats_type {
> +	XE_STATS_TYPE_COUNTER,
> +};
> +
> +#ifdef CONFIG_DRM_XE_STATS
> +struct xe_stats_entry {
> +	const char *name;
> +	enum xe_stats_type type;
> +	bool enabled;
> +	union {
> +		struct {
> +			atomic64_t value;
> +		} counter;
> +	} data;
> +	struct list_head list;
> +	struct dentry *dentry;
> +};
> +
> +struct xe_stats {
> +	struct dentry *debugfs_entry;
> +	struct list_head entries;
> +	struct drm_device *drm;
> +};
> +
> +struct xe_stats *xe_stats_init(struct drm_device *drm, struct dentry *parent);
> +void xe_stats_add_entry(struct xe_stats *stats, const char *name, enum xe_stats_type type);
> +void xe_stats_increment_counter(struct xe_stats *c, const char *name);
> +void xe_stats_decrement_counter(struct xe_stats *stats, const char *name);
> +
> +#else /* CONFIG_DRM_XE_STATS not defined */
> +struct xe_stats;
> +
> +static inline struct xe_stats *
> +xe_stats_init(struct drm_device *drm, struct dentry *parent)
> +{
> +	return NULL;
> +}
> +
> +static inline void
> +xe_stats_add_entry(struct xe_stats *stats, const char *name, enum xe_stats_type type) { }
> +static inline void
> +xe_stats_increment_counter(struct xe_stats *stats, const char *name) { }
> +static inline void
> +xe_stats_decrement_counter(struct xe_stats *stats, const char *name) { }
> +
> +#endif /* CONFIG_DRM_XE_STATS */
> +
> +#endif /* _XE_STATS_H_ */
> +

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [RFC PATCH 1/2] drm/xe: Implement APIs for measuring various events
  2024-07-25 15:48   ` Michal Wajdeczko
@ 2024-07-25 16:27     ` Matthew Brost
  2024-07-25 16:41       ` Nirmoy Das
  0 siblings, 1 reply; 7+ messages in thread
From: Matthew Brost @ 2024-07-25 16:27 UTC (permalink / raw)
  To: Michal Wajdeczko; +Cc: Nirmoy Das, intel-xe

On Thu, Jul 25, 2024 at 05:48:34PM +0200, Michal Wajdeczko wrote:
> 
> 
> On 25.07.2024 17:05, Nirmoy Das wrote:
> > Introduces a set of APIs for tracking and managing events.
> > This currently only supports counters.
> > 
> > This can be conditionally enabled with the CONFIG_DRM_XE_STATS
> > config option.
> > 
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> > ---
> >  drivers/gpu/drm/xe/Kconfig.debug |  11 +++
> >  drivers/gpu/drm/xe/Makefile      |   1 +
> >  drivers/gpu/drm/xe/xe_stats.c    | 145 +++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/xe/xe_stats.h    |  66 ++++++++++++++
> >  4 files changed, 223 insertions(+)
> >  create mode 100644 drivers/gpu/drm/xe/xe_stats.c
> >  create mode 100644 drivers/gpu/drm/xe/xe_stats.h
> > 
> > diff --git a/drivers/gpu/drm/xe/Kconfig.debug b/drivers/gpu/drm/xe/Kconfig.debug
> > index bc177368af6c..4db6d707c1af 100644
> > --- a/drivers/gpu/drm/xe/Kconfig.debug
> > +++ b/drivers/gpu/drm/xe/Kconfig.debug
> > @@ -94,3 +94,14 @@ config DRM_XE_USERPTR_INVAL_INJECT
> >  
> >  	 Recomended for driver developers only.
> >  	 If in doubt, say "N".
> > +
> > +config DRM_XE_STATS
> > +	bool "Enable XE statistics"
> > +	default n
> > +	help
> > +	  Choose this option to enable support for collecting and
> > +	  displaying XE statistics as debugfs.
> > +
> > +	  Recommended for driver developers only.
> > +
> > +	  If in doubt, say "N".
> > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> > index 1ff9602a52f6..c5d9ec22c6bf 100644
> > --- a/drivers/gpu/drm/xe/Makefile
> > +++ b/drivers/gpu/drm/xe/Makefile
> > @@ -117,6 +117,7 @@ xe-y += xe_bb.o \
> >  	xe_wopcm.o
> >  
> >  xe-$(CONFIG_HMM_MIRROR) += xe_hmm.o
> > +xe-$(CONFIG_DRM_XE_STATS) += xe_stats.o
> >  
> >  # graphics hardware monitoring (HWMON) support
> >  xe-$(CONFIG_HWMON) += xe_hwmon.o
> > diff --git a/drivers/gpu/drm/xe/xe_stats.c b/drivers/gpu/drm/xe/xe_stats.c
> > new file mode 100644
> > index 000000000000..1b8910900fd3
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_stats.c
> > @@ -0,0 +1,145 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2024 Intel Corporation
> > + */
> > +
> > +#include <linux/debugfs.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/slab.h>
> > +
> > +#include <drm/drm_device.h>
> > +#include <drm/drm_managed.h>
> > +#include <drm/drm_print.h>
> > +
> > +#include "xe_stats.h"
> > +
> > +#ifdef CONFIG_DRM_XE_STATS
> > +static struct xe_stats_entry *find_stats_entry(struct xe_stats *stats, const char *name)
> > +{
> > +	struct xe_stats_entry *stats_entry;
> > +
> > +	list_for_each_entry(stats_entry, &stats->entries, list) {
> > +		if (strcmp(stats_entry->name, name) == 0)
> > +			return stats_entry;
> 
> hmm, this seems to be very costly and likely will not scale well once we
> would like to add more counters
> 

Agree this won't scale well. See my suggestion a reply to Nirmoy. Let me
know what you think.

Matt 

> can't we just use CONFIG_DRM_XE_STATS to hide different stats structures
> provide nop variants of the helpers that would update them ?
> 
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +static int stats_show(struct seq_file *m, void *v)
> > +{
> > +	struct xe_stats_entry *entry = m->private;
> > +
> > +	if (entry && !entry->enabled)
> > +		return 0;
> > +
> > +	switch (entry->type) {
> > +	case XE_STATS_TYPE_COUNTER:
> > +		seq_printf(m, "%s: Counter value: %llu\n", entry->name,
> > +			   atomic64_read(&entry->data.counter.value));
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int stats_open(struct inode *inode, struct file *file)
> > +{
> > +	return single_open(file, stats_show, inode->i_private);
> > +}
> > +
> > +static const struct file_operations stats_fops = {
> > +	.owner   = THIS_MODULE,
> > +	.open    = stats_open,
> > +	.read    = seq_read,
> > +	.release = single_release,
> > +};
> > +
> > +struct xe_stats *xe_stats_init(struct drm_device *drm, struct dentry *parent)
> > +{
> > +	struct xe_stats *stats;
> > +
> > +	stats = drmm_kzalloc(drm, sizeof(*stats), GFP_KERNEL);
> > +	if (!stats) {
> > +		drm_err(drm, "Failed to allocate memory for xe_stats\n");
> > +		return NULL;
> > +	}
> > +
> > +	stats->drm = drm;
> > +	INIT_LIST_HEAD(&stats->entries);
> > +
> > +	stats->debugfs_entry = debugfs_create_dir("stats", parent);
> > +	if (!stats->debugfs_entry) {
> > +		drm_err(drm, "Failed to create stats debugfs directory\n");
> > +		return NULL;
> > +	}
> > +
> > +	return stats;
> > +}
> > +
> > +void xe_stats_add_entry(struct xe_stats *stats, const char *name, enum xe_stats_type type)
> > +{
> > +	struct xe_stats_entry *entry;
> > +	struct dentry *debugfs_entry;
> > +
> > +	if (!stats || !name)
> > +		return;
> > +
> > +	entry = drmm_kzalloc(stats->drm, sizeof(*entry), GFP_KERNEL);
> > +	if (!entry)
> > +		return;
> > +
> > +	entry->name = drmm_kstrdup(stats->drm, name, GFP_KERNEL);
> > +	if (!entry->name)
> > +		return;
> > +
> > +	entry->type = type;
> > +	entry->enabled = true;
> > +
> > +	switch (type) {
> > +	case XE_STATS_TYPE_COUNTER:
> > +		atomic64_set(&entry->data.counter.value, 0);
> > +		break;
> > +	default:
> > +		return;
> > +	}
> > +
> > +	list_add(&entry->list, &stats->entries);
> > +
> > +	debugfs_entry = debugfs_create_file(name, 0444, stats->debugfs_entry, entry, &stats_fops);
> > +	if (!debugfs_entry) {
> > +		drm_err(stats->drm, "Failed to create stats debugfs file for %s\n", name);
> > +		list_del(&entry->list);
> > +		return;
> > +	}
> > +
> > +	entry->dentry = debugfs_entry;
> > +}
> > +
> > +void xe_stats_increment_counter(struct xe_stats *stats, const char *name)
> > +{
> > +	struct xe_stats_entry *entry;
> > +
> > +	if (!stats || !name)
> > +		return;
> > +
> > +	entry = find_stats_entry(stats, name);
> > +	if (entry && entry->type == XE_STATS_TYPE_COUNTER && entry->enabled)
> > +		atomic64_inc(&entry->data.counter.value);
> > +}
> > +
> > +void xe_stats_decrement_counter(struct xe_stats *stats, const char *name)
> > +{
> > +	struct xe_stats_entry *entry;
> > +
> > +	if (!stats || !name)
> > +		return;
> > +
> > +	entry = find_stats_entry(stats, name);
> > +	if (entry && entry->type == XE_STATS_TYPE_COUNTER && entry->enabled)
> > +		atomic64_dec(&entry->data.counter.value);
> > +}
> > +
> > +#endif
> > diff --git a/drivers/gpu/drm/xe/xe_stats.h b/drivers/gpu/drm/xe/xe_stats.h
> > new file mode 100644
> > index 000000000000..dba79ae28714
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_stats.h
> > @@ -0,0 +1,66 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2024 Intel Corporation
> > + */
> > +#ifndef _XE_STATS_H_
> > +#define _XE_STATS_H_
> > +
> > +#include <linux/atomic.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/list.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/types.h>
> > +
> > +#include <drm/drm_print.h>
> > +
> > +struct drm_device;
> > +
> > +enum xe_stats_type {
> > +	XE_STATS_TYPE_COUNTER,
> > +};
> > +
> > +#ifdef CONFIG_DRM_XE_STATS
> > +struct xe_stats_entry {
> > +	const char *name;
> > +	enum xe_stats_type type;
> > +	bool enabled;
> > +	union {
> > +		struct {
> > +			atomic64_t value;
> > +		} counter;
> > +	} data;
> > +	struct list_head list;
> > +	struct dentry *dentry;
> > +};
> > +
> > +struct xe_stats {
> > +	struct dentry *debugfs_entry;
> > +	struct list_head entries;
> > +	struct drm_device *drm;
> > +};
> > +
> > +struct xe_stats *xe_stats_init(struct drm_device *drm, struct dentry *parent);
> > +void xe_stats_add_entry(struct xe_stats *stats, const char *name, enum xe_stats_type type);
> > +void xe_stats_increment_counter(struct xe_stats *c, const char *name);
> > +void xe_stats_decrement_counter(struct xe_stats *stats, const char *name);
> > +
> > +#else /* CONFIG_DRM_XE_STATS not defined */
> > +struct xe_stats;
> > +
> > +static inline struct xe_stats *
> > +xe_stats_init(struct drm_device *drm, struct dentry *parent)
> > +{
> > +	return NULL;
> > +}
> > +
> > +static inline void
> > +xe_stats_add_entry(struct xe_stats *stats, const char *name, enum xe_stats_type type) { }
> > +static inline void
> > +xe_stats_increment_counter(struct xe_stats *stats, const char *name) { }
> > +static inline void
> > +xe_stats_decrement_counter(struct xe_stats *stats, const char *name) { }
> > +
> > +#endif /* CONFIG_DRM_XE_STATS */
> > +
> > +#endif /* _XE_STATS_H_ */
> > +

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [RFC PATCH 1/2] drm/xe: Implement APIs for measuring various events
  2024-07-25 16:27     ` Matthew Brost
@ 2024-07-25 16:41       ` Nirmoy Das
  0 siblings, 0 replies; 7+ messages in thread
From: Nirmoy Das @ 2024-07-25 16:41 UTC (permalink / raw)
  To: Matthew Brost, Michal Wajdeczko; +Cc: intel-xe


On 7/25/2024 6:27 PM, Matthew Brost wrote:
> On Thu, Jul 25, 2024 at 05:48:34PM +0200, Michal Wajdeczko wrote:
>>
>> On 25.07.2024 17:05, Nirmoy Das wrote:
>>> Introduces a set of APIs for tracking and managing events.
>>> This currently only supports counters.
>>>
>>> This can be conditionally enabled with the CONFIG_DRM_XE_STATS
>>> config option.
>>>
>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/Kconfig.debug |  11 +++
>>>   drivers/gpu/drm/xe/Makefile      |   1 +
>>>   drivers/gpu/drm/xe/xe_stats.c    | 145 +++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/xe/xe_stats.h    |  66 ++++++++++++++
>>>   4 files changed, 223 insertions(+)
>>>   create mode 100644 drivers/gpu/drm/xe/xe_stats.c
>>>   create mode 100644 drivers/gpu/drm/xe/xe_stats.h
>>>
>>> diff --git a/drivers/gpu/drm/xe/Kconfig.debug b/drivers/gpu/drm/xe/Kconfig.debug
>>> index bc177368af6c..4db6d707c1af 100644
>>> --- a/drivers/gpu/drm/xe/Kconfig.debug
>>> +++ b/drivers/gpu/drm/xe/Kconfig.debug
>>> @@ -94,3 +94,14 @@ config DRM_XE_USERPTR_INVAL_INJECT
>>>   
>>>   	 Recomended for driver developers only.
>>>   	 If in doubt, say "N".
>>> +
>>> +config DRM_XE_STATS
>>> +	bool "Enable XE statistics"
>>> +	default n
>>> +	help
>>> +	  Choose this option to enable support for collecting and
>>> +	  displaying XE statistics as debugfs.
>>> +
>>> +	  Recommended for driver developers only.
>>> +
>>> +	  If in doubt, say "N".
>>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>>> index 1ff9602a52f6..c5d9ec22c6bf 100644
>>> --- a/drivers/gpu/drm/xe/Makefile
>>> +++ b/drivers/gpu/drm/xe/Makefile
>>> @@ -117,6 +117,7 @@ xe-y += xe_bb.o \
>>>   	xe_wopcm.o
>>>   
>>>   xe-$(CONFIG_HMM_MIRROR) += xe_hmm.o
>>> +xe-$(CONFIG_DRM_XE_STATS) += xe_stats.o
>>>   
>>>   # graphics hardware monitoring (HWMON) support
>>>   xe-$(CONFIG_HWMON) += xe_hwmon.o
>>> diff --git a/drivers/gpu/drm/xe/xe_stats.c b/drivers/gpu/drm/xe/xe_stats.c
>>> new file mode 100644
>>> index 000000000000..1b8910900fd3
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/xe/xe_stats.c
>>> @@ -0,0 +1,145 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2024 Intel Corporation
>>> + */
>>> +
>>> +#include <linux/debugfs.h>
>>> +#include <linux/seq_file.h>
>>> +#include <linux/slab.h>
>>> +
>>> +#include <drm/drm_device.h>
>>> +#include <drm/drm_managed.h>
>>> +#include <drm/drm_print.h>
>>> +
>>> +#include "xe_stats.h"
>>> +
>>> +#ifdef CONFIG_DRM_XE_STATS
>>> +static struct xe_stats_entry *find_stats_entry(struct xe_stats *stats, const char *name)
>>> +{
>>> +	struct xe_stats_entry *stats_entry;
>>> +
>>> +	list_for_each_entry(stats_entry, &stats->entries, list) {
>>> +		if (strcmp(stats_entry->name, name) == 0)
>>> +			return stats_entry;
>> hmm, this seems to be very costly and likely will not scale well once we
>> would like to add more counters

Both of you came to the same conclusion :) I have enough hints now. Let 
me spin up the next

simpler version next week.


Thanks,

Nirmoy


>>
> Agree this won't scale well. See my suggestion a reply to Nirmoy. Let me
> know what you think.
>
> Matt
>
>> can't we just use CONFIG_DRM_XE_STATS to hide different stats structures
>> provide nop variants of the helpers that would update them ?
>>
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>> +static int stats_show(struct seq_file *m, void *v)
>>> +{
>>> +	struct xe_stats_entry *entry = m->private;
>>> +
>>> +	if (entry && !entry->enabled)
>>> +		return 0;
>>> +
>>> +	switch (entry->type) {
>>> +	case XE_STATS_TYPE_COUNTER:
>>> +		seq_printf(m, "%s: Counter value: %llu\n", entry->name,
>>> +			   atomic64_read(&entry->data.counter.value));
>>> +		break;
>>> +	default:
>>> +		break;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int stats_open(struct inode *inode, struct file *file)
>>> +{
>>> +	return single_open(file, stats_show, inode->i_private);
>>> +}
>>> +
>>> +static const struct file_operations stats_fops = {
>>> +	.owner   = THIS_MODULE,
>>> +	.open    = stats_open,
>>> +	.read    = seq_read,
>>> +	.release = single_release,
>>> +};
>>> +
>>> +struct xe_stats *xe_stats_init(struct drm_device *drm, struct dentry *parent)
>>> +{
>>> +	struct xe_stats *stats;
>>> +
>>> +	stats = drmm_kzalloc(drm, sizeof(*stats), GFP_KERNEL);
>>> +	if (!stats) {
>>> +		drm_err(drm, "Failed to allocate memory for xe_stats\n");
>>> +		return NULL;
>>> +	}
>>> +
>>> +	stats->drm = drm;
>>> +	INIT_LIST_HEAD(&stats->entries);
>>> +
>>> +	stats->debugfs_entry = debugfs_create_dir("stats", parent);
>>> +	if (!stats->debugfs_entry) {
>>> +		drm_err(drm, "Failed to create stats debugfs directory\n");
>>> +		return NULL;
>>> +	}
>>> +
>>> +	return stats;
>>> +}
>>> +
>>> +void xe_stats_add_entry(struct xe_stats *stats, const char *name, enum xe_stats_type type)
>>> +{
>>> +	struct xe_stats_entry *entry;
>>> +	struct dentry *debugfs_entry;
>>> +
>>> +	if (!stats || !name)
>>> +		return;
>>> +
>>> +	entry = drmm_kzalloc(stats->drm, sizeof(*entry), GFP_KERNEL);
>>> +	if (!entry)
>>> +		return;
>>> +
>>> +	entry->name = drmm_kstrdup(stats->drm, name, GFP_KERNEL);
>>> +	if (!entry->name)
>>> +		return;
>>> +
>>> +	entry->type = type;
>>> +	entry->enabled = true;
>>> +
>>> +	switch (type) {
>>> +	case XE_STATS_TYPE_COUNTER:
>>> +		atomic64_set(&entry->data.counter.value, 0);
>>> +		break;
>>> +	default:
>>> +		return;
>>> +	}
>>> +
>>> +	list_add(&entry->list, &stats->entries);
>>> +
>>> +	debugfs_entry = debugfs_create_file(name, 0444, stats->debugfs_entry, entry, &stats_fops);
>>> +	if (!debugfs_entry) {
>>> +		drm_err(stats->drm, "Failed to create stats debugfs file for %s\n", name);
>>> +		list_del(&entry->list);
>>> +		return;
>>> +	}
>>> +
>>> +	entry->dentry = debugfs_entry;
>>> +}
>>> +
>>> +void xe_stats_increment_counter(struct xe_stats *stats, const char *name)
>>> +{
>>> +	struct xe_stats_entry *entry;
>>> +
>>> +	if (!stats || !name)
>>> +		return;
>>> +
>>> +	entry = find_stats_entry(stats, name);
>>> +	if (entry && entry->type == XE_STATS_TYPE_COUNTER && entry->enabled)
>>> +		atomic64_inc(&entry->data.counter.value);
>>> +}
>>> +
>>> +void xe_stats_decrement_counter(struct xe_stats *stats, const char *name)
>>> +{
>>> +	struct xe_stats_entry *entry;
>>> +
>>> +	if (!stats || !name)
>>> +		return;
>>> +
>>> +	entry = find_stats_entry(stats, name);
>>> +	if (entry && entry->type == XE_STATS_TYPE_COUNTER && entry->enabled)
>>> +		atomic64_dec(&entry->data.counter.value);
>>> +}
>>> +
>>> +#endif
>>> diff --git a/drivers/gpu/drm/xe/xe_stats.h b/drivers/gpu/drm/xe/xe_stats.h
>>> new file mode 100644
>>> index 000000000000..dba79ae28714
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/xe/xe_stats.h
>>> @@ -0,0 +1,66 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright © 2024 Intel Corporation
>>> + */
>>> +#ifndef _XE_STATS_H_
>>> +#define _XE_STATS_H_
>>> +
>>> +#include <linux/atomic.h>
>>> +#include <linux/debugfs.h>
>>> +#include <linux/list.h>
>>> +#include <linux/spinlock.h>
>>> +#include <linux/types.h>
>>> +
>>> +#include <drm/drm_print.h>
>>> +
>>> +struct drm_device;
>>> +
>>> +enum xe_stats_type {
>>> +	XE_STATS_TYPE_COUNTER,
>>> +};
>>> +
>>> +#ifdef CONFIG_DRM_XE_STATS
>>> +struct xe_stats_entry {
>>> +	const char *name;
>>> +	enum xe_stats_type type;
>>> +	bool enabled;
>>> +	union {
>>> +		struct {
>>> +			atomic64_t value;
>>> +		} counter;
>>> +	} data;
>>> +	struct list_head list;
>>> +	struct dentry *dentry;
>>> +};
>>> +
>>> +struct xe_stats {
>>> +	struct dentry *debugfs_entry;
>>> +	struct list_head entries;
>>> +	struct drm_device *drm;
>>> +};
>>> +
>>> +struct xe_stats *xe_stats_init(struct drm_device *drm, struct dentry *parent);
>>> +void xe_stats_add_entry(struct xe_stats *stats, const char *name, enum xe_stats_type type);
>>> +void xe_stats_increment_counter(struct xe_stats *c, const char *name);
>>> +void xe_stats_decrement_counter(struct xe_stats *stats, const char *name);
>>> +
>>> +#else /* CONFIG_DRM_XE_STATS not defined */
>>> +struct xe_stats;
>>> +
>>> +static inline struct xe_stats *
>>> +xe_stats_init(struct drm_device *drm, struct dentry *parent)
>>> +{
>>> +	return NULL;
>>> +}
>>> +
>>> +static inline void
>>> +xe_stats_add_entry(struct xe_stats *stats, const char *name, enum xe_stats_type type) { }
>>> +static inline void
>>> +xe_stats_increment_counter(struct xe_stats *stats, const char *name) { }
>>> +static inline void
>>> +xe_stats_decrement_counter(struct xe_stats *stats, const char *name) { }
>>> +
>>> +#endif /* CONFIG_DRM_XE_STATS */
>>> +
>>> +#endif /* _XE_STATS_H_ */
>>> +

^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-07-25 16:41 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-25 15:05 [RFC PATCH 0/2] Implement stats layer to measure events Nirmoy Das
2024-07-25 15:05 ` [RFC PATCH 1/2] drm/xe: Implement APIs for measuring various events Nirmoy Das
2024-07-25 15:42   ` Matthew Brost
2024-07-25 15:48   ` Michal Wajdeczko
2024-07-25 16:27     ` Matthew Brost
2024-07-25 16:41       ` Nirmoy Das
2024-07-25 15:05 ` [PATCH] drm/xe: Keep track of TLB inval events Nirmoy Das

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox