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

Nirmoy Das nirmoy.das at intel.com
Wed Aug 7 08:19:00 UTC 2024 

On 8/6/2024 11:45 PM, Michal Wajdeczko wrote:
>
> 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


Will resend with docs for public functions.

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


Agreed, I should add boundary checks.

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


Ok, I think in that case I have to move the include to the .c file.

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


I will adjust this series with above suggestions.

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


I will recheck and remove if so.


Thanks,

Nirmoy

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