On Fri, 17 Jan 2025, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> wrote:
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.h b/drivers/gpu/drm/i915/display/intel_dmc_wl.h
> index 5488fbdf29b8..d11b0ab50b3c 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.h
> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.h
> @@ -11,6 +11,7 @@
> #include <linux/refcount.h>
>
> #include "i915_reg_defs.h"
> +#include "intel_dmc_wl_debugfs.h"
>
> struct intel_display;
>
> @@ -27,6 +28,7 @@ struct intel_dmc_wl {
> */
> u32 dc_state;
> struct delayed_work work;
> + struct intel_dmc_wl_dbg dbg;
> };
>
With intel_de.h including intel_dmc_wl.h, we'll have almost all of
display include intel_dmc_wl_debugfs.h, and getting the definition of
struct intel_dmc_wl_dbg, really for no good reason.
I really like to flip this around. You need to have a *good reason* to
expose stuff to the entire display driver all of a sudden. Instead of
requiring a good reason to hide stuff.
BR,
Jani.
> void intel_dmc_wl_init(struct intel_display *display);
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.c b/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.c
> new file mode 100644
> index 000000000000..41e59d775fe5
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.c
> @@ -0,0 +1,251 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright (C) 2025 Intel Corporation
> + */
> +
> +#include <linux/debugfs.h>
> +
> +#include <drm/drm_device.h>
> +#include <drm/drm_managed.h>
> +#include <drm/drm_print.h>
> +
> +#include "intel_display_core.h"
> +#include "intel_dmc_wl_debugfs.h"
> +
> +#define DEBUGFS_UNTRACKED_BUFFER_SIZE_MAX 65536
> +
> +/*
> + * DOC: DMC wakelock debugfs
> + *
> + * The DMC wakelock code needs to keep track of register offsets that need the
> + * wakelock for proper access. If one of the necessary offsets are missed, then
> + * the failure in asserting the wakelock is very likely to cause problems down
> + * the road.
> + *
> + * A miss could happen for at least two different reasons:
> + *
> + * - We might have forgotten to add the offset (or range) to the relevant
> + * tables tracked by the driver in the first place.
> + *
> + * - Or updates to either the DMC firmware or the display IP that require new
> + * offsets to be tracked and we fail to realize that.
> + *
> + * To help capture these cases, we provide the intel_dmc_wl/ debugfs directory,
> + * which exports a buffer of untracked register offsets.
> + *
> + * Untracked offsets
> + * -----------------
> + *
> + * This is a buffer that records every register offset that went through the
> + * DMC wakelock check and was deemed not needing the wakelock for MMIO access.
> + *
> + * To activate the logging of offsets into such a buffer, one can do::
> + *
> + * # echo -1 > /sys/kernel/debug/dri/(...)/intel_dmc_wl/untracked_size
> + *
> + * This will create a buffer with the maximum number of entries allowed
> + * (DEBUGFS_UNTRACKED_BUFFER_SIZE_MAX). A positive value can be used instead to
> + * define a different size:
> + *
> + * # echo 1024 > /sys/kernel/debug/dri/(...)/intel_dmc_wl/untracked_size
> + *
> + * Every write to untracked_size will cause the buffer to be reset.
> + *
> + * It is also possible to read untracked_size in order to get the current
> + * value.
> + *
> + * After enabled, the buffer starts getting filled with offsets as MMIOs are
> + * performed by the driver.
> + *
> + * In order to view the content of the buffer, one can do::
> + *
> + * # cat /sys/kernel/debug/dri/(...)/intel_dmc_wl/untracked
> + * 0x000c4000
> + * 0x0016fe50
> + * 0x000c7200
> + * 0x000c7204
> + * 0x00045230
> + * 0x00046440
> + * 0x00045234
> + * 0x0016fa48
> + * 0x0016fa40
> + * 0x0016fa5c
> + * (...)
> + *
> + * The order of those offsets does not reflect the order the checks were done
> + * (some recently seen offsets are skipped to save space).
> + *
> + * Once done with it, the logging can be disabled with::
> + *
> + * # echo 0 > /sys/kernel/debug/dri/(...)/intel_dmc_wl/untracked_size
> + */
> +
> +static int untracked_size_get(void *data, u64 *val)
> +{
> + struct intel_display *display = data;
> + struct intel_dmc_wl_dbg *dbg = &display->wl.dbg;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dbg->lock, flags);
> + *val = dbg->untracked.size;
> + spin_unlock_irqrestore(&dbg->lock, flags);
> +
> + return 0;
> +}
> +
> +static int untracked_size_set(void *data, u64 val)
> +{
> + struct intel_display *display = data;
> + struct intel_dmc_wl_dbg *dbg = &display->wl.dbg;
> + s64 new_size;
> + u32 *old_offsets;
> + u32 *new_offsets;
> + unsigned long flags;
> +
> + new_size = (s64)val;
> +
> + if (new_size == -1) {
> + new_size = DEBUGFS_UNTRACKED_BUFFER_SIZE_MAX;
> + } else if (new_size < 0) {
> + drm_err(display->drm,
> + "%lld is invalid for untracked_size, the only negative value allowed is -1\n",
> + new_size);
> + return -EINVAL;
> + } else if (new_size > DEBUGFS_UNTRACKED_BUFFER_SIZE_MAX) {
> + drm_err(display->drm,
> + "%lld too big for untracked_size, maximum allowed value is %d\n",
> + new_size,
> + DEBUGFS_UNTRACKED_BUFFER_SIZE_MAX);
> + return -EINVAL;
> + }
> +
> + if (new_size == 0) {
> + new_offsets = NULL;
> + } else {
> + new_offsets = drmm_kmalloc_array(display->drm, new_size, sizeof(*new_offsets),
> + GFP_KERNEL);
> +
> + if (!new_offsets)
> + return -ENOMEM;
> + }
> +
> + spin_lock_irqsave(&dbg->lock, flags);
> + old_offsets = dbg->untracked.offsets;
> + dbg->untracked.offsets = new_offsets;
> + dbg->untracked.size = new_size;
> + dbg->untracked.head = 0;
> + dbg->untracked.len = 0;
> + dbg->untracked.overflow = false;
> + spin_unlock_irqrestore(&dbg->lock, flags);
> +
> + if (old_offsets)
> + drmm_kfree(display->drm, old_offsets);
> +
> + return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE_SIGNED(untracked_size_fops,
> + untracked_size_get,
> + untracked_size_set,
> + "%lld\n");
> +
> +static int untracked_show(struct seq_file *m, void *data)
> +{
> + struct intel_display *display = m->private;
> + struct intel_dmc_wl_dbg *dbg = &display->wl.dbg;
> + unsigned long flags;
> + size_t remaining;
> + size_t i;
> +
> + spin_lock_irqsave(&dbg->lock, flags);
> +
> + remaining = dbg->untracked.len;
> + i = dbg->untracked.head;
> +
> + while (remaining--) {
> + if (i == 0)
> + i = dbg->untracked.size;
> +
> + seq_printf(m, "0x%08x\n", dbg->untracked.offsets[--i]);
> + }
> +
> + spin_unlock_irqrestore(&dbg->lock, flags);
> +
> + return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(untracked);
> +
> +void intel_dmc_wl_debugfs_init(struct intel_display *display)
> +{
> + struct intel_dmc_wl_dbg *dbg = &display->wl.dbg;
> +
> + spin_lock_init(&dbg->lock);
> +}
> +
> +void intel_dmc_wl_debugfs_register(struct intel_display *display)
> +{
> + struct dentry *dir;
> +
> + if (!HAS_DMC_WAKELOCK(display))
> + return;
> +
> + dir = debugfs_create_dir("intel_dmc_wl", display->drm->debugfs_root);
> + if (IS_ERR(dir))
> + return;
> +
> + debugfs_create_file("untracked_size", 0644, dir, display,
> + &untracked_size_fops);
> + debugfs_create_file("untracked", 0644, dir, display,
> + &untracked_fops);
> +}
> +
> +static bool untracked_has_recent_offset(struct intel_display *display, u32 offset)
> +{
> + struct intel_dmc_wl_dbg *dbg = &display->wl.dbg;
> + int look_back = 32;
> + size_t i;
> +
> + if (look_back > dbg->untracked.len)
> + look_back = dbg->untracked.len;
> +
> + i = dbg->untracked.head;
> +
> + while (look_back--) {
> + if (i == 0)
> + i = dbg->untracked.size;
> +
> + if (dbg->untracked.offsets[--i] == offset)
> + return true;
> + }
> +
> + return false;
> +}
> +
> +void intel_dmc_wl_debugfs_log_untracked(struct intel_display *display, u32 offset)
> +{
> + struct intel_dmc_wl_dbg *dbg = &display->wl.dbg;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dbg->lock, flags);
> +
> + if (!dbg->untracked.size)
> + goto out_unlock;
> +
> + /* Save some space by not repeating recent offsets. */
> + if (untracked_has_recent_offset(display, offset))
> + goto out_unlock;
> +
> + dbg->untracked.offsets[dbg->untracked.head] = offset;
> + dbg->untracked.head = (dbg->untracked.head + 1) % dbg->untracked.size;
> + if (dbg->untracked.len < dbg->untracked.size)
> + dbg->untracked.len++;
> +
> + if (dbg->untracked.len == dbg->untracked.size && !dbg->untracked.overflow) {
> + dbg->untracked.overflow = true;
> + drm_warn(display->drm, "Overflow detected in DMC wakelock debugfs untracked offsets\n");
> + }
> +
> +out_unlock:
> + spin_unlock_irqrestore(&dbg->lock, flags);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.h b/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.h
> new file mode 100644
> index 000000000000..9437c324966f
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2025 Intel Corporation
> + */
> +
> +#ifndef __INTEL_DMC_WL_DEBUGFS_H__
> +#define __INTEL_DMC_WL_DEBUGFS_H__
> +
> +#include <linux/types.h>
> +#include <linux/spinlock.h>
> +
> +struct intel_display;
> +
> +struct intel_dmc_wl_dbg {
> + spinlock_t lock; /* protects everything below */
> + struct {
> + u32 *offsets;
> + size_t head;
> + size_t len;
> + size_t size;
> + bool overflow;
> + } untracked;
> +};
> +
> +void intel_dmc_wl_debugfs_init(struct intel_display *display);
> +void intel_dmc_wl_debugfs_register(struct intel_display *display);
> +void intel_dmc_wl_debugfs_log_untracked(struct intel_display *display, u32 offset);
> +
> +#endif /* __INTEL_DMC_WL_DEBUGFS_H__ */
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 81f63258a7e1..f03fbdbcb1a4 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -221,6 +221,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
> i915-display/intel_display_wa.o \
> i915-display/intel_dkl_phy.o \
> i915-display/intel_dmc.o \
> + i915-display/intel_dmc_wl_debugfs.o \
> i915-display/intel_dp.o \
> i915-display/intel_dp_aux.o \
> i915-display/intel_dp_aux_backlight.o \
--
Jani Nikula, Intel
