[PATCH 3/4] drm/i915/dmc_wl: Add extra_ranges debugfs

[Luca Coelho]

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 at intel.com>
> ---

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.