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

[Luca Coelho]

On Fri, 2025-01-17 at 19:06 -0300, 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 at intel.com>
> ---

This looks great overall!

A couple of comments below.


[...]
> +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 size < 32, you would check some values twice, right? Probably not a
real case scenario, and it wouldn't matter much, but it took me a bit
to wrap my head around this.

[...]
> +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");
> +	}

Couldn't it be useful to print overflows every time they occur? Maybe
convert overflow to a uint to track how many times it happened? (though
maybe this is a bit overkill).

[...]

--
Cheers,
Luca.