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

[Matthew Brost]

On Wed, Aug 07, 2024 at 08:59:34AM -0500, Lucas De Marchi wrote:
> 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.
> 

I don't know enough about how compilers work these days but would the
atomic increment compile out without debugfs. If the answer is yes, then
agree we can drop this Kconfig.

Also does debugfs havea Kconfig, did a quick search and not seeing one.

Matt

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