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