[PATCH v2 1/2] drm/xe/gt: Add APIs for printing stats over debugfs
Lucas De Marchi
lucas.demarchi at intel.com
Wed Aug 7 17:45:50 UTC 2024
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.
>>
>
>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
}
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