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

[Gustavo Sousa <gustavo.sousa@intel.com>]

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

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?

>
>[...]
>> +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.

--
Gustavo Sousa

>
>[...]
>
>--
>Cheers,
>Luca.
>