On Fri, 2025-01-17 at 19:06 -0300, 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>
> ---
Some comments below.
[...]
> 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
[...]
> +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;
> +}
This function is probably almost identical than the one used to check
the hard-coded ranges, isn't it? In that case, couldn't you just pass
the ranges array (in this case dbg->extra_ranges) to the same function?
> 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;
> +};
> +
Why do you need another struct for this?
[...]
--
Cheers,
Luca.
