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

[Nirmoy Das <nirmoy.das@intel.com>]

On 8/9/2024 6:35 AM, Matthew Brost wrote:
> 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/

I will make it const.


I think Lucas also wanted to add a build time check for id but that 
turns out complicated as I have to move this func to

the header file as static inline. Accessing gt members in a header file 
doesn't work without adding xe_gt.h that again

ends with recursive include. So decided to keep this as v2


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

Thanks, I will rebase and resnd.

Nirmoy

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