Re: [PATCH 2/4] drm/i915/dmc_wl: Add debugfs for untracked offsets

["Vivekanandan, Balasubramani" <balasubramani.vivekanandan@intel.com>]

On 17.01.2025 19:06, Gustavo Sousa wrote:
> 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, let's introduce a debugfs interface for the
> DMC wakelock.
> 
> In this part, we export a buffer containing offsets of registers that
> were considered not needing the wakelock by our driver. In an upcoming
> change we will also allow defining an extra set of offset ranges to be
> tracked by our driver.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile                 |   1 +
>  .../drm/i915/display/intel_display_debugfs.c  |   2 +
>  drivers/gpu/drm/i915/display/intel_dmc_wl.c   |   5 +-
>  drivers/gpu/drm/i915/display/intel_dmc_wl.h   |   2 +
>  .../drm/i915/display/intel_dmc_wl_debugfs.c   | 251 ++++++++++++++++++
>  .../drm/i915/display/intel_dmc_wl_debugfs.h   |  29 ++
>  drivers/gpu/drm/xe/Makefile                   |   1 +
>  7 files changed, 290 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 3dda9f0eda82..ac1ab79de9c8 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -251,6 +251,7 @@ i915-y += \
>  	display/intel_display_wa.o \
>  	display/intel_dmc.o \
>  	display/intel_dmc_wl.o \
> +	display/intel_dmc_wl_debugfs.o \
>  	display/intel_dpio_phy.o \
>  	display/intel_dpll.o \
>  	display/intel_dpll_mgr.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index f1d76484025a..b032535f4830 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -26,6 +26,7 @@
>  #include "intel_display_power_well.h"
>  #include "intel_display_types.h"
>  #include "intel_dmc.h"
> +#include "intel_dmc_wl_debugfs.h"
>  #include "intel_dp.h"
>  #include "intel_dp_link_training.h"
>  #include "intel_dp_mst.h"
> @@ -883,6 +884,7 @@ void intel_display_debugfs_register(struct drm_i915_private *i915)
>  
>  	intel_bios_debugfs_register(display);
>  	intel_cdclk_debugfs_register(display);
> +	intel_dmc_wl_debugfs_register(display);
>  	intel_dmc_debugfs_register(display);
>  	intel_dp_test_debugfs_register(display);
>  	intel_fbc_debugfs_register(display);
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> index 330b43a72e08..3686d4e90167 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> @@ -338,6 +338,7 @@ void intel_dmc_wl_init(struct intel_display *display)
>  	spin_lock_init(&wl->lock);
>  	refcount_set(&wl->refcount,
>  		     display->params.enable_dmc_wl == ENABLE_DMC_WL_ALWAYS_LOCKED ? 1 : 0);
> +	intel_dmc_wl_debugfs_init(display);
>  }
>  
>  /* Must only be called as part of enabling dynamic DC states. */
> @@ -444,8 +445,10 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
>  	spin_lock_irqsave(&wl->lock, flags);
>  
>  	if (i915_mmio_reg_valid(reg) &&
> -	    !intel_dmc_wl_check_range(display, reg, wl->dc_state))
> +	    !intel_dmc_wl_check_range(display, reg, wl->dc_state)) {
> +		intel_dmc_wl_debugfs_log_untracked(display, i915_mmio_reg_offset(reg));
>  		goto out_unlock;
> +	}
>  
>  	if (!wl->enabled) {
>  		if (!refcount_inc_not_zero(&wl->refcount))
> 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;
>  };
>  
>  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
This macro is not actually the size but the count of offsets stored in
the buffer. This is used as the array size for drmm_kmalloc_array call.
It makes sense to rename this macro as count

> +
> +/*
> + * 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 knob is setting the count of offsets to be stored and not the size.
I think this should be renamed to indicate it as count.

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

For me passing negative number doesn't look intuitive. Thinking do we
really need the case of passing default buffer size. We can allow just 0
to disable and any positive number to enable with the buffer size set as
value passed.

> + *
> + * 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;
Define a macro for this magic number

> +	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);
As this code never gets called by an interrupt, we can use just the
spin_lock instead of spin_lock_irqsave. Same applies for all the places
where spin_lock/unlock_irqsave/irqrestore is used.

> +
> +	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;
There is no need of both len and size. head will always give the count
of entries in the buffer. During overflow, we are keeping a flag to
indicate a overflow, which indicates the we also have date in the buffer
above head till the end of buffer.

Regards,
Bala

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