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

Michal Wajdeczko michal.wajdeczko at intel.com
Tue Aug 6 21:45:49 UTC 2024 


On 06.08.2024 22:08, Nirmoy Das wrote:
> Looks like send this bit prematurely:
> 
> On 8/6/2024 9:45 PM, 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.
>>
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko 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   | 30 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_gt_stats.h   | 31 ++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_gt_types.h   |  7 +++++++
>>   6 files changed, 85 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..cc69d748e8f6
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_gt_stats.c
>> @@ -0,0 +1,30 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2024 Intel Corporation
>> + */
>> +
>> +#include <linux/atomic.h>
>> +
>> +#include "xe_gt.h"
>> +#include "xe_gt_stats.h"
>> +
>> +#ifdef CONFIG_DRM_XE_STATS

doc is also missing here

>> +void xe_gt_stats_incr(struct xe_gt *gt, enum xe_gt_counter_stats id,
>> int incr)
>> +{

what if id = STATS_NUM ?

>> +    atomic_add(incr, &gt->stats.counters[id]);
>> +}
>> +
>> +static const char *const stat_description[] = {
>> +};
>> +

and here

>> +int xe_gt_stats_print_info(struct xe_gt *gt, struct drm_printer *p)
>> +{
>> +    enum xe_gt_counter_stats id;
>> +
>> +    for (id = 0; id < XE_GT_COUNTER_STATS_NUM; ++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..e83155ab1fdb
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_gt_stats.h
>> @@ -0,0 +1,31 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2024 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_GT_STATS_H_
>> +#define _XE_GT_STATS_H_
>> +
>> +#include <drm/drm_print.h>
>> +
>> +struct xe_gt;
>> +//struct drm_printer;
> 
> Forgot to remove this.

actually this forward decl should stay (instead of the full #include above)

> 
> 
>> +
>> +enum xe_gt_counter_stats {

nit: this file is "xe_gt_stats" so type name should follow it:

	enum xe_gt_stats_id {

same with enumerator names:

	XE_GT_STATS_ID_TLB_INVAL,

>> +    XE_GT_COUNTER_STATS_NUM,

hmm, if you really want to count stat ids using this last enumerator
then make sure that functions that accepts such enum will not accept
this NUM as index and maybe name it slightly differently than regular IDs:

	__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_counter_stats
>> stat, int incr);
>> +#else
>> +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_counter_stats
>> stat, int incr)
>> +{
>> +}
>> +
>> +#endif /* CONFIG_DRM_XE_STATS */
>> +#endif /* _XE_GT_STATS_H_ */

IIRC we don't use comments on closing guards

>> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h
>> b/drivers/gpu/drm/xe/xe_gt_types.h
>> index 631928258d71..0ded57b56628 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,12 @@ struct xe_gt {
>>           u8 has_indirect_ring_state:1;
>>       } info;
>>   +#if IS_ENABLED(CONFIG_DRM_XE_STATS)
>> +    struct {
>> +        atomic_t counters[XE_GT_COUNTER_STATS_NUM];
>> +    } stats;
>> +#endif
> 
> I forgot to  add doc here. I will wait for reviews and resend with docs.
> 
> 
> Regards,
> 
> Nirmoy
> 
>> +
>>       /**
>>        * @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

More information about the Intel-xe mailing list