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@xxxxxxxxx>
> ---
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.
