On Thu, 2025-01-23 at 11:41 -0300, Gustavo Sousa wrote:
> Quoting Luca Coelho (2025-01-22 06:06:00-03:00)
> > 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.
>
> Nope. If look_back is greater than the current number of offsets, it
> will be just reset to that count (see the "if (look_back >
> dbg->untracked.len)" above).
>
> Note that the number of valid elements in the circular buffer is tracked
> with untracked.len, while untracked.size is actually the capacity. Maybe
> I should have used that name for the member instead?
Yeah, maybe untracked.len and untracked.max_len? Dunno, it's also fine
as it is. I was blind focusing on the loop itself and didn't see the
if above it.
> > [...]
> > > +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).
>
> The warning is just to let the user know that the buffer doesn't have
> every untracked offset that has been seen since enabling the logging.
> The best thing to do in this case is to try a bigger size or try to
> reduce the execution to be tracked (maybe a smaller test case).
>
> I'm not sure how providing a count would be useful here.
It would at least make it easier to know how to size the buffer for the
next run. ;) But that's moot.
Reviewed-by: Luca Coelho <luciano.coelho@xxxxxxxxx>
--
Cheers,
Luca.
