On Thu, 2025-01-23 at 12:52 -0300, Gustavo Sousa wrote:
> 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.
Yeah, thanks for the clarification.
Reviewed-by: Luca Coelho <luciano.coelho@xxxxxxxxx>
--
Cheers,
Luca.
