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"...
>
>> +
>> +/*
>> + * 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
>>
