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

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

On Thu, Aug 08, 2024 at 11:19:14AM +0200, 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.
> 
> v3: s/CONFIG_DRM_XE_STATS/CONFIG_DEBUG_FS(Lucas)
> v2: add missing docs
>     Add boundary checks for stats id and other improvements (Michal)
>     Fix build when CONFIG_DRM_XE_STATS is disabled(Matt)
> 

This will need to be rebased on Lucas's series [1].

[1] https://patchwork.freedesktop.org/series/137053/

> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> 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/Makefile        |  2 ++
>  drivers/gpu/drm/xe/xe_gt_debugfs.c |  2 ++
>  drivers/gpu/drm/xe/xe_gt_stats.c   | 48 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_gt_stats.h   | 32 ++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_gt_types.h   |  9 ++++++
>  5 files changed, 93 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/Makefile b/drivers/gpu/drm/xe/Makefile
> index 1ff9602a52f6..390e5652cd89 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -287,6 +287,8 @@ ifeq ($(CONFIG_DEBUG_FS),y)
>  		i915-display/intel_pipe_crc.o
>  endif
>  
> +xe-$(CONFIG_DEBUG_FS) += xe_gt_stats.o
> +
>  obj-$(CONFIG_DRM_XE) += xe.o
>  obj-$(CONFIG_DRM_XE_KUNIT_TEST) += tests/
>  
> diff --git a/drivers/gpu/drm/xe/xe_gt_debugfs.c b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> index 5e7fd937917a..5125d76ccfac 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,7 @@ 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},
> +	{"stats", .show = xe_gt_debugfs_simple_show, .data = xe_gt_stats_print_info},
>  };
>  
>  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..74ed9342af91
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_gt_stats.c
> @@ -0,0 +1,48 @@
> +// 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"
> +
> +/**
> + * 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.
> + */
> +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;
> +}
> 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..b97363e99ba7
> --- /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_DEBUG_FS
> +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);

I think Lucas wants this to be s/enum xe_gt_stats_id id/const enum xe_gt_stats_id id/

> +#else
> +static inline int xe_gt_stats_print_info(struct xe_gt *gt, struct drm_printer *p)
> +{
> +	return 0;
> +}

You don't need this as the only caller will compile out without
CONFIG_DEBUG_FS after Lucas's series.

With the nits fixed:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> +
> +static inline void xe_gt_stats_incr(struct xe_gt *gt, enum xe_gt_stats_id id,
> +				    int incr)
> +{
> +}
> +
> +#endif
> +#endif
> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
> index 631928258d71..8e46c0969bc7 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_DEBUG_FS)
> +	/** @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
>