[PATCH v2 1/2] drm/xe/gt: Add APIs for printing stats over debugfs
Nirmoy Das
nirmoy.das at intel.com
Wed Aug 7 19:56:37 UTC 2024
On 8/7/2024 7:45 PM, Lucas De Marchi wrote:
> On Wed, Aug 07, 2024 at 04:57:17PM GMT, 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.
CONFIG_DEBUG_FS will work. It just didn't occur to me.
>>>
>>
>> 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.
>
> either this:
>
>
> static inline void xe_gt_stats_incr(struct xe_gt *gt, enum
> xe_gt_stats_id id, ...)
> {
> #ifdef CONFIG_DEBUG_FS
> atomic_add(...)
> #endif
> }
I will resend with this method.
Thanks,
Nirmoy
>
> or a dummy implementation separately would work.
>
> We can also use
>
> if (!IS_ENABLED(CONFIG_DEBUG_FS))
> return;
>
> Compiler would still constant fold and drop it. See
> Documentation/process/coding-style.rst - 21) Conditional Compilation
>
> But in this last case we wouldn't be able to remove the symbol from the
> gt struct, which may or may not be relevant since it's just a few bytes.
>
>>
>> Also does debugfs havea Kconfig, did a quick search and not seeing one.
>>
>
> see above. And we already check it in drivers/gpu/drm/xe/Makefile:
>
> ifeq ($(CONFIG_DEBUG_FS),y) xe-$(CONFIG_DRM_XE_DISPLAY) += \
> i915-display/intel_display_debugfs.o \
> i915-display/intel_display_debugfs_params.o \
> i915-display/intel_pipe_crc.o endif
> oddly we are not compiling out xe_gt_debugfs, which we probably should
> since otherwise we will just see a warning "Create GT directory failed"
>
> Lucas De Marchi
>
>> Matt
>>
>>>
>>> >
>>> > Cc: Matthew Brost <matthew.brost at intel.com>
>>> > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> > Cc: Sai Gowtham Ch <sai.gowtham.ch 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 | 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, >->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(>->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
>>> >
More information about the Intel-xe
mailing list