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

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

Quoting Luca Coelho (2025-01-22 07:19:35-03:00)
>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@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?

Yeah. I thought about that when implementing this, but ended up going
with a separate implementation.

If you look at how the current series is done, there is a one-way
dependency between intel_dmc_wl_debugfs and intel_dmc_wl - the latter
depends on the former. I just didn't want to make this a circular
dependency, since the implementation is rather simple anyway...

Let me know if that convinced you :-)

>
>
>> 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?
>

In the same spirit as with my answer above... I think of this much as an
implementation detail that would be better off not exposed in headers.

--
Gustavo Sousa

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