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

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Jani Nikula (2025-01-27 06:47:58-03:00)
>On Fri, 17 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> 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.

Maybe make dbg a pointer and have only intel_dmc_wl.c knowing its guts?

Or do you have some other suggestion?

--
Gustavo Sousa

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