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

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Aug 07, 2024 at 04:57:17PM +0000, Matthew Brost wrote:
> 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.
> 

Oh, found it... CONFIG_DEBUG_FS

So do you mean protect the atomic inc by CONFIG_DEBUG_FS too? That could work.

Matt

> Matt
> 
> > 
> > > 
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Cc: Sai Gowtham Ch <sai.gowtham.ch@intel.com>
> > > Signed-off-by: Nirmoy Das <nirmoy.das@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
> > >