On 23.01.2025 13:41, Gustavo Sousa wrote:
> Quoting Vivekanandan, Balasubramani (2025-01-23 13:11:03-03:00)
> >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@xxxxxxxxx>
> >> ---
> >> 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
>
> I think size doesn't really have to mean number of bytes, but, in this
> case, I agree, that "buffer size" could be easily thought that way.
>
> However, couldn't "buffer count" be somewhat ambiguous? One might read
> as the number of buffers and then only later after reading the code that
> it would refer to the number of entries in a single buffer.
>
> Between the two options, I think I would still go with "buffer size"...
I was thinking something like DEBUGFS_UNTRACKED_OFFSET_COUNT
Regards,
Bala
>
> >
> >> +
> >> +/*
> >> + * 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.
>
> Well, this is already supported. The use of -1 is just a convenience,
> and the user is not required to use it.
>
> The current interface doesn't provide a way of letting the user know the
> maximum value and I think it would be a bit overkill adding another
> debugfs file just for that purpose.
>
> As such, I think -1 is a useful way of letting the user use the maximum
> size, specially when she doesn't have the up-to-date documentation in
> handy to know what value that is. That's certainly better than having
> the user finding the maximum via trial and error.
>
> >
> >> + *
> >> + * 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
>
> This is very local to this function and I guess the variable name
> already convey it's meaning?
>
> >
> >> + 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.
>
> This code does get called in interrupt context. It is called by
> intel_dmc_wl_get(), which is called for most of display MMIO access
> functions, and that happens both in user and interrupt context.
>
> >
> >> +
> >> + 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.
>
> Ah, right. If overflow==false, then the length equals head, if
> overflow==true, then length equals size.
>
> Maybe the inconvenience is that we will need to calculate the length
> every time we need it. But maybe that's not too bad...
>
> Thanks! I'll remove the "len" member in v2.
>
> --
> Gustavo Sousa
>
> >
> >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
> >>
