[PATCH v2 1/2] drm/xe/gt: Add APIs for printing stats over debugfs

Wed Aug 7 13:59:34 UTC 2024

On Wed, Aug 07, 2024 at 01:07:58PM GMT, Nirmoy Das wrote:
>Add skeleton APIs for recording and printing various stats over
>debugfs. This currently only added counter types stats which is backed
>by atomic_t and wrapped with CONFIG_DRM_XE_STATS so this can be disabled
>on production system.
>
>v2: add missing docs
>    Add boundary checks for stats id and other improvements (Michal)
>    Fix build when CONFIG_DRM_XE_STATS is disabled(Matt)

I don't understand why we are even adding a config here.
This should just be compiled out if build is without debugfs.

>
>Cc: Matthew Brost <matthew.brost at intel.com>
>Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>Cc: Sai Gowtham Ch <sai.gowtham.ch at intel.com>
>Signed-off-by: Nirmoy Das <nirmoy.das at intel.com>
>---
> drivers/gpu/drm/xe/Kconfig.debug   | 11 +++++++
> drivers/gpu/drm/xe/Makefile        |  2 ++
> drivers/gpu/drm/xe/xe_gt_debugfs.c |  4 +++
> drivers/gpu/drm/xe/xe_gt_stats.c   | 51 ++++++++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_gt_stats.h   | 32 +++++++++++++++++++
> drivers/gpu/drm/xe/xe_gt_types.h   |  9 ++++++
> 6 files changed, 109 insertions(+)
> create mode 100644 drivers/gpu/drm/xe/xe_gt_stats.c
> create mode 100644 drivers/gpu/drm/xe/xe_gt_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..b0bbb248f644 100644
>--- a/drivers/gpu/drm/xe/Makefile
>+++ b/drivers/gpu/drm/xe/Makefile
>@@ -143,6 +143,8 @@ xe-$(CONFIG_PCI_IOV) += \
> 	xe_pci_sriov.o \
> 	xe_sriov_pf.o
>
>+xe-$(CONFIG_DRM_XE_STATS) += xe_gt_stats.o
>+
> # include helpers for tests even when XE is built-in
> ifdef CONFIG_DRM_XE_KUNIT_TEST
> xe-y += tests/xe_kunit_helpers.o
>diff --git a/drivers/gpu/drm/xe/xe_gt_debugfs.c b/drivers/gpu/drm/xe/xe_gt_debugfs.c
>index 5e7fd937917a..3765c04919dc 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_stats.h"
> #include "xe_gt_topology.h"
> #include "xe_hw_engine.h"
> #include "xe_lrc.h"
>@@ -286,6 +287,9 @@ static const struct drm_info_list debugfs_list[] = {
> 	{"default_lrc_bcs", .show = xe_gt_debugfs_simple_show, .data = bcs_default_lrc},
> 	{"default_lrc_vcs", .show = xe_gt_debugfs_simple_show, .data = vcs_default_lrc},
> 	{"default_lrc_vecs", .show = xe_gt_debugfs_simple_show, .data = vecs_default_lrc},
>+#if IS_ENABLED(CONFIG_DRM_XE_STATS)
>+	{"stats", .show = xe_gt_debugfs_simple_show, .data = xe_gt_stats_print_info},
>+#endif
> };
>
> void xe_gt_debugfs_register(struct xe_gt *gt)
>diff --git a/drivers/gpu/drm/xe/xe_gt_stats.c b/drivers/gpu/drm/xe/xe_gt_stats.c
>new file mode 100644
>index 000000000000..b13aee5488d5
>--- /dev/null
>+++ b/drivers/gpu/drm/xe/xe_gt_stats.c
>@@ -0,0 +1,51 @@
>+// SPDX-License-Identifier: MIT
>+/*
>+ * Copyright © 2024 Intel Corporation
>+ */
>+
>+#include <linux/atomic.h>
>+
>+#include <drm/drm_print.h>
>+
>+#include "xe_gt.h"
>+#include "xe_gt_stats.h"
>+
>+#ifdef CONFIG_DRM_XE_STATS
>+
>+/**
>+ * xe_gt_stats_incr - Increments the specified stats counter
>+ * @gt: graphics tile
>+ * @id: xe_gt_stats_id type id that needs to be incremented
>+ * @incr: value to be incremented with
>+ *
>+ * Increments the specified stats counter if that is within expected range.
>+ */
>+void xe_gt_stats_incr(struct xe_gt *gt, enum xe_gt_stats_id id, int incr)
>+{
>+	if (id >= __XE_GT_STATS_NUM_IDS)
>+		return;
>+
>+	atomic_add(incr, &gt->stats.counters[id]);
>+}
>+
>+static const char *const stat_description[__XE_GT_STATS_NUM_IDS] = {
>+};
>+
>+/**
>+ * xe_gt_stats_print_info - Print the GT stats
>+ * @gt: graphics tile
>+ * @p: drm_printer where it will be printed out.
>+ *
>+ * This prints out all the available GT stats.
>+ */
>+int xe_gt_stats_print_info(struct xe_gt *gt, struct drm_printer *p)
>+{
>+	enum xe_gt_stats_id id;
>+
>+	for (id = 0; id < __XE_GT_STATS_NUM_IDS; ++id)
>+		drm_printf(p, "%s: %d\n", stat_description[id],
>+			   atomic_read(&gt->stats.counters[id]));
>+
>+	return 0;
>+}
>+#endif
>diff --git a/drivers/gpu/drm/xe/xe_gt_stats.h b/drivers/gpu/drm/xe/xe_gt_stats.h
>new file mode 100644
>index 000000000000..04a1d61ce1fb
>--- /dev/null
>+++ b/drivers/gpu/drm/xe/xe_gt_stats.h
>@@ -0,0 +1,32 @@
>+/* SPDX-License-Identifier: MIT */
>+/*
>+ * Copyright © 2024 Intel Corporation
>+ */
>+
>+#ifndef _XE_GT_STATS_H_
>+#define _XE_GT_STATS_H_
>+
>+struct xe_gt;
>+struct drm_printer;
>+
>+enum xe_gt_stats_id {
>+	/* must be the last entry */
>+	__XE_GT_STATS_NUM_IDS,
>+};
>+
>+#ifdef CONFIG_DRM_XE_STATS
>+int xe_gt_stats_print_info(struct xe_gt *gt, struct drm_printer *p);
>+void xe_gt_stats_incr(struct xe_gt *gt, enum xe_gt_stats_id id, int incr);
>+#else
>+static inline int xe_gt_stats_print_info(struct xe_gt *gt, struct drm_printer *p)
>+{
>+	return 0;
>+}
>+
>+static inline void xe_gt_stats_incr(struct xe_gt *gt, enum xe_gt_stats_id id,
>+				    int incr)
>+{
>+}

Is there a reason not to force id to always be a compile-time constant?
The only use case added in patch 2 pass it as a constant.
This way the id is checked at build time, the penalty with debugfs is
just an atomic_add() and the penalty without debugfs is none.

I think this is largely different from e.g. CONFIG_SCHEDSTATS and may
not deserve a separate kernel config: just handle the no-debugfs case,
which is mostly implemented already by not building
drivers/gpu/drm/xe/xe_gt_debugfs.c, with the remaining part being only
the dummy implementations in the header.

Lucas De Marchi

>+
>+#endif
>+#endif
>diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
>index 631928258d71..4955612fb01d 100644
>--- a/drivers/gpu/drm/xe/xe_gt_types.h
>+++ b/drivers/gpu/drm/xe/xe_gt_types.h
>@@ -10,6 +10,7 @@
> #include "xe_gt_idle_types.h"
> #include "xe_gt_sriov_pf_types.h"
> #include "xe_gt_sriov_vf_types.h"
>+#include "xe_gt_stats.h"
> #include "xe_hw_engine_types.h"
> #include "xe_hw_fence_types.h"
> #include "xe_oa.h"
>@@ -133,6 +134,14 @@ struct xe_gt {
> 		u8 has_indirect_ring_state:1;
> 	} info;
>
>+#if IS_ENABLED(CONFIG_DRM_XE_STATS)
>+	/** @stats: GT stats */
>+	struct {
>+		/** @stats.counters: counters for various GT stats */
>+		atomic_t counters[__XE_GT_STATS_NUM_IDS];
>+	} stats;
>+#endif
>+
> 	/**
> 	 * @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
>-- 
>2.42.0
>