On 30.01.2025 14:00, Vivekanandan, Balasubramani wrote:
> On 17.01.2025 19:06, Gustavo Sousa wrote:
> > We already have a way of finding the set of untracked offsets for which
> > there has been one or more MMIO operations via the
> > "intel_dmc_wl/untracked" debugfs interface.
> >
> > However, in order to try adding one or more of those registers to the
> > set of tracked offsets, one would need to manually change the source
> > code and re-compile the driver.
> >
> > To make debugging easier, also add a "intel_dmc_wl/extra_ranges" debugfs
> > interface so that extra offsets to be tracked can be defined during
> > runtime, removing the need of re-compilation or even module reloading.
> >
> > With "intel_dmc_wl/untracked" and "intel_dmc_wl/extra_ranges", one could
> > even come up with a search algorithm to find missing offsets when
> > debugging a failing test case in a similar fashion to git-bisect. Such
> > an algorithm is subject for a future tool, probably implemented in
> > another repository (e.g. IGT).
> >
> > Signed-off-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx>
>
> Looks good to me.
>
> Regards,
> Bala
Forgot to add
Reviewed-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@xxxxxxxxx>
Regards,
Bala
>
> > ---
> > drivers/gpu/drm/i915/display/intel_dmc_wl.c | 7 +
> > .../drm/i915/display/intel_dmc_wl_debugfs.c | 254 +++++++++++++++++-
> > .../drm/i915/display/intel_dmc_wl_debugfs.h | 7 +
> > 3 files changed, 267 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > index 3686d4e90167..c9740250be73 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > @@ -276,6 +276,13 @@ static bool intel_dmc_wl_check_range(struct intel_display *display,
> > if (ranges && intel_dmc_wl_offset_in_ranges(offset, ranges))
> > return true;
> >
> > + /*
> > + * Call to check extra ranges from debugfs only as last resort to avoid
> > + * taking intel_dmc_wl_dbg's spinlock.
> > + */
> > + if (intel_dmc_wl_debugfs_offset_in_extra_ranges(display, offset))
> > + return true;
> > +
> > return false;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.c b/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.c
> > index 41e59d775fe5..1493d296ac98 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.c
> > @@ -3,6 +3,8 @@
> > * Copyright (C) 2025 Intel Corporation
> > */
> >
> > +#include <linux/kstrtox.h>
> > +#include <linux/ctype.h>
> > #include <linux/debugfs.h>
> >
> > #include <drm/drm_device.h>
> > @@ -13,6 +15,7 @@
> > #include "intel_dmc_wl_debugfs.h"
> >
> > #define DEBUGFS_UNTRACKED_BUFFER_SIZE_MAX 65536
> > +#define DEBUGFS_EXTRA_RANGES_MAX 255
> >
> > /*
> > * DOC: DMC wakelock debugfs
> > @@ -31,7 +34,8 @@
> > * 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.
> > + * which exports a buffer of untracked register offsets and also allows extra
> > + * register offsets to be tracked by the driver.
> > *
> > * Untracked offsets
> > * -----------------
> > @@ -78,6 +82,43 @@
> > * Once done with it, the logging can be disabled with::
> > *
> > * # echo 0 > /sys/kernel/debug/dri/(...)/intel_dmc_wl/untracked_size
> > + *
> > + * Tracking extra ranges
> > + * ---------------------
> > + *
> > + * After looking at a list of untracked offsets via intel_dmc_wl/untracked, it
> > + * is likely that one would want to check whether tracking a suspicious
> > + * set of register offsets (i.e. asserting the DMC wakelock for them) would
> > + * solve a bug.
> > + *
> > + * Instead of adding the offsets manually in the source code (which would
> > + * require re-compiling and reloading the module) the intel_dmc_wl/extra_ranges
> > + * debugfs interface allow defining extra ranges during runtime, which can
> > + * significantly speed up debugging time.
> > + *
> > + * Every write to intel_dmc_wl/untracked defines a new set of ranges to be
> > + * tracked. The input format is a whitespace-separated list of ranges and each
> > + * range is in the format <start_offset>..<end_offset>, with ..<end_offset>
> > + * being optional. Note that <end_offset> is inclusive.
> > + *
> > + * Examples::
> > + *
> > + * # echo 0x44400..0x4440c \ # Track a single range of 4 registers
> > + * > /sys/kernel/debug/dri/(...)/intel_dmc_wl/untracked_size
> > + *
> > + * # echo 0x44400..0x4440c 0x44410..0x4441c \ # Track 2 ranges
> > + * > /sys/kernel/debug/dri/(...)/intel_dmc_wl/untracked_size
> > + *
> > + * # echo 0x44400 0x44410..0x4441c \ # Track a single register and 1 range
> > + * > /sys/kernel/debug/dri/(...)/intel_dmc_wl/untracked_size
> > + *
> > + * # echo \ # Do not track extra ranges anymore
> > + * > /sys/kernel/debug/dri/(...)/intel_dmc_wl/untracked_size
> > + *
> > + * The new set of extra ranges take effect after the write operation, meaning
> > + * that the next MMIOs made by the driver for registers that match those
> > + * offsets will assert the wakelock (besides the offsets already hardcoded in
> > + * the driver).
> > */
> >
> > static int untracked_size_get(void *data, u64 *val)
> > @@ -176,6 +217,189 @@ static int untracked_show(struct seq_file *m, void *data)
> >
> > DEFINE_SHOW_ATTRIBUTE(untracked);
> >
> > +static int extra_ranges_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;
> > +
> > + spin_lock_irqsave(&dbg->lock, flags);
> > +
> > + if (!dbg->extra_ranges)
> > + goto out_unlock;
> > +
> > + for (int i = 0; dbg->extra_ranges[i].start; i++) {
> > + if (dbg->extra_ranges[i].end)
> > + seq_printf(m, "0x%08x..0x%08x\n",
> > + dbg->extra_ranges[i].start,
> > + dbg->extra_ranges[i].end);
> > + else
> > + seq_printf(m, "0x%08x\n", dbg->extra_ranges[i].start);
> > + }
> > +
> > +out_unlock:
> > + spin_unlock_irqrestore(&dbg->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Parse *p to match the pattern <start_offset>..<end_offset> and store the
> > + * offsets into *dest, if dest is not NULL.
> > + *
> > + * Leading whitespaces are ignored and ..<end_offset> is optional. Both offsets
> > + * are expected to be expressed in hexadecimal.
> > + *
> > + * The pointer *p is updated to point at the next character in the string for
> > + * parsing a new range.
> > + *
> > + * On success, 1 is returned if a valid range was found and 0 is returned if
> > + * there is no range left to parse. On error, a negative error number is
> > + * returned.
> > + */
> > +static int parse_single_extra_range(struct intel_display *display,
> > + char **p,
> > + struct intel_dmc_wl_dbg_extra_range *dest)
> > +{
> > + char c;
> > + char *s;
> > + char *range_substr;
> > + int err;
> > + u32 val;
> > +
> > + while (isspace(**p))
> > + ++*p;
> > +
> > + if (**p == '\0')
> > + return 0;
> > +
> > + range_substr = *p;
> > +
> > + /* s is the <start_offset> substr */
> > + s = *p;
> > + while (!isspace(**p) && **p != '.' && **p != '\0')
> > + ++*p;
> > + c = **p;
> > + **p = '\0';
> > + err = kstrtou32(s, 16, &val);
> > + **p = c;
> > + if (err)
> > + goto out_err;
> > +
> > + if (dest)
> > + dest->start = val;
> > +
> > + if (**p != '.') {
> > + /* only the "start offset" was passed */
> > + if (dest)
> > + dest->end = 0;
> > + return 1;
> > + }
> > +
> > + if (*(++*p) != '.') {
> > + err = -EINVAL;
> > + goto out_err;
> > + }
> > +
> > + /* s is the <end_offset> substr */
> > + s = ++*p;
> > + while (!isspace(**p) && **p != '\0')
> > + ++*p;
> > + c = **p;
> > + **p = '\0';
> > + err = kstrtou32(s, 16, &val);
> > + **p = c;
> > + if (err)
> > + goto out_err;
> > +
> > + if (dest)
> > + dest->end = val;
> > +
> > + return 1;
> > +
> > +out_err:
> > + while (!isspace(**p) && **p != '\0')
> > + ++*p;
> > + c = **p;
> > + **p = '\0';
> > + drm_err(display->drm, "invalid DMC Wakelock extra range: %s\n", range_substr);
> > + **p = c;
> > +
> > + return err;
> > +}
> > +
> > +static struct intel_dmc_wl_dbg_extra_range *
> > +parse_extra_ranges(struct intel_display *display, char *s)
> > +{
> > + struct intel_dmc_wl_dbg_extra_range *ranges;
> > + char *p;
> > + int num_ranges;
> > + int err;
> > +
> > + /* Do a first pass and validate everything. */
> > + p = s;
> > + num_ranges = 0;
> > + while ((err = parse_single_extra_range(display, &p, NULL)) > 0) {
> > + num_ranges++;
> > + if (num_ranges > DEBUGFS_EXTRA_RANGES_MAX) {
> > + drm_err(display->drm, "Too many DMC wakelock extra ranges, maximum is %d\n",
> > + DEBUGFS_EXTRA_RANGES_MAX);
> > + return ERR_PTR(-EINVAL);
> > + }
> > + }
> > +
> > + if (err < 0)
> > + return ERR_PTR(err);
> > +
> > + /* Now allocate and do a second pass storing the parsed ranges. */
> > + ranges = drmm_kmalloc_array(display->drm, num_ranges + 1, sizeof(*ranges), GFP_KERNEL);
> > + if (!ranges)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + p = s;
> > + num_ranges = 0;
> > + while (parse_single_extra_range(display, &p, &ranges[num_ranges]) > 0)
> > + num_ranges++;
> > +
> > + ranges[num_ranges].start = 0; /* Sentinel value. */
> > +
> > + return ranges;
> > +}
> > +
> > +static ssize_t extra_ranges_write(struct file *file,
> > + const char __user *ubuf,
> > + size_t len, loff_t *offp)
> > +{
> > + struct seq_file *m = file->private_data;
> > + struct intel_display *display = m->private;
> > + struct intel_dmc_wl_dbg *dbg = &display->wl.dbg;
> > + struct intel_dmc_wl_dbg_extra_range *old_extra_ranges;
> > + struct intel_dmc_wl_dbg_extra_range *new_extra_ranges;
> > + unsigned long flags;
> > + char *kbuf;
> > +
> > + kbuf = memdup_user_nul(ubuf, len);
> > + if (IS_ERR(kbuf))
> > + return PTR_ERR(kbuf);
> > +
> > + new_extra_ranges = parse_extra_ranges(display, kbuf);
> > + kfree(kbuf);
> > + if (IS_ERR(new_extra_ranges))
> > + return PTR_ERR(new_extra_ranges);
> > +
> > + spin_lock_irqsave(&dbg->lock, flags);
> > + old_extra_ranges = dbg->extra_ranges;
> > + dbg->extra_ranges = new_extra_ranges;
> > + spin_unlock_irqrestore(&dbg->lock, flags);
> > +
> > + if (old_extra_ranges)
> > + drmm_kfree(display->drm, old_extra_ranges);
> > +
> > + return len;
> > +}
> > +
> > +DEFINE_SHOW_STORE_ATTRIBUTE(extra_ranges);
> > +
> > void intel_dmc_wl_debugfs_init(struct intel_display *display)
> > {
> > struct intel_dmc_wl_dbg *dbg = &display->wl.dbg;
> > @@ -198,6 +422,8 @@ void intel_dmc_wl_debugfs_register(struct intel_display *display)
> > &untracked_size_fops);
> > debugfs_create_file("untracked", 0644, dir, display,
> > &untracked_fops);
> > + debugfs_create_file("extra_ranges", 0644, dir, display,
> > + &extra_ranges_fops);
> > }
> >
> > static bool untracked_has_recent_offset(struct intel_display *display, u32 offset)
> > @@ -249,3 +475,29 @@ void intel_dmc_wl_debugfs_log_untracked(struct intel_display *display, u32 offse
> > out_unlock:
> > spin_unlock_irqrestore(&dbg->lock, flags);
> > }
> > +
> > +bool intel_dmc_wl_debugfs_offset_in_extra_ranges(struct intel_display *display, u32 offset)
> > +{
> > + struct intel_dmc_wl_dbg *dbg = &display->wl.dbg;
> > + bool ret = false;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&dbg->lock, flags);
> > +
> > + if (!dbg->extra_ranges)
> > + goto out_unlock;
> > +
> > + for (int i = 0; dbg->extra_ranges[i].start; i++) {
> > + u32 end = dbg->extra_ranges[i].end ?: dbg->extra_ranges[i].start;
> > +
> > + if (dbg->extra_ranges[i].start <= offset && offset <= end) {
> > + ret = true;
> > + goto out_unlock;
> > + }
> > + }
> > +
> > +out_unlock:
> > + spin_unlock_irqrestore(&dbg->lock, flags);
> > +
> > + return ret;
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.h b/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.h
> > index 9437c324966f..ae61217a2789 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.h
> > @@ -11,6 +11,11 @@
> >
> > struct intel_display;
> >
> > +struct intel_dmc_wl_dbg_extra_range {
> > + u32 start;
> > + u32 end;
> > +};
> > +
> > struct intel_dmc_wl_dbg {
> > spinlock_t lock; /* protects everything below */
> > struct {
> > @@ -20,10 +25,12 @@ struct intel_dmc_wl_dbg {
> > size_t size;
> > bool overflow;
> > } untracked;
> > + struct intel_dmc_wl_dbg_extra_range *extra_ranges;
> > };
> >
> > 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);
> > +bool intel_dmc_wl_debugfs_offset_in_extra_ranges(struct intel_display *display, u32 offset);
> >
> > #endif /* __INTEL_DMC_WL_DEBUGFS_H__ */
> > --
> > 2.48.0
> >
