Re: [PATCH v3] drm/i915:move and rename reg_in_range_table

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 10, 2025 at 02:23:48PM -0700, Matt Atwood wrote:
> On Fri, Oct 10, 2025 at 04:07:55PM +0300, Ville Syrjälä wrote:
> > On Thu, Oct 09, 2025 at 02:52:08PM -0700, Matt Atwood wrote:
> > > reg_in_range_table is a useful function that is used in multiple places,
> > > and will be needed for WA_BB implementation later.
> > > 
> > > Let's move this function and i915_range struct to its own file, as we are
> > > trying to move away from i915_utils files.
> > > 
> > > v2: move functions to their own file
> > > v3: use correct naming convention
> > > 
> > > Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/Makefile                 |  1 +
> > >  drivers/gpu/drm/i915/gt/intel_workarounds.c   |  9 +--
> > >  drivers/gpu/drm/i915/i915_mmio_range.c        | 18 +++++
> > >  drivers/gpu/drm/i915/i915_mmio_range.h        | 19 ++++++
> > >  drivers/gpu/drm/i915/i915_perf.c              | 67 ++++++++-----------
> > >  drivers/gpu/drm/i915/intel_uncore.c           | 15 +++--
> > >  drivers/gpu/drm/i915/intel_uncore.h           |  8 +--
> > >  drivers/gpu/drm/i915/selftests/intel_uncore.c |  4 +-
> > >  8 files changed, 82 insertions(+), 59 deletions(-)
> > >  create mode 100644 drivers/gpu/drm/i915/i915_mmio_range.c
> > >  create mode 100644 drivers/gpu/drm/i915/i915_mmio_range.h
> > > 
> > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > > index 78a45a6681df..e5819c4320bf 100644
> > > --- a/drivers/gpu/drm/i915/Makefile
> > > +++ b/drivers/gpu/drm/i915/Makefile
> > > @@ -26,6 +26,7 @@ i915-y += \
> > >  	i915_ioctl.o \
> > >  	i915_irq.o \
> > >  	i915_mitigations.o \
> > > +	i915_mmio_range.o \
> > >  	i915_module.o \
> > >  	i915_params.o \
> > >  	i915_pci.o \
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > index 7d486dfa2fc1..ece88c612e27 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > @@ -5,6 +5,7 @@
> > >  
> > >  #include "i915_drv.h"
> > >  #include "i915_reg.h"
> > > +#include "i915_mmio_range.h"
> > >  #include "intel_context.h"
> > >  #include "intel_engine_pm.h"
> > >  #include "intel_engine_regs.h"
> > > @@ -2923,7 +2924,7 @@ void intel_engine_apply_workarounds(struct intel_engine_cs *engine)
> > >  	wa_list_apply(&engine->wa_list);
> > >  }
> > >  
> > > -static const struct i915_range mcr_ranges_gen8[] = {
> > > +static const struct i915_mmio_range mcr_ranges_gen8[] = {
> > >  	{ .start = 0x5500, .end = 0x55ff },
> > >  	{ .start = 0x7000, .end = 0x7fff },
> > >  	{ .start = 0x9400, .end = 0x97ff },
> > > @@ -2932,7 +2933,7 @@ static const struct i915_range mcr_ranges_gen8[] = {
> > >  	{},
> > >  };
> > >  
> > > -static const struct i915_range mcr_ranges_gen12[] = {
> > > +static const struct i915_mmio_range mcr_ranges_gen12[] = {
> > >  	{ .start =  0x8150, .end =  0x815f },
> > >  	{ .start =  0x9520, .end =  0x955f },
> > >  	{ .start =  0xb100, .end =  0xb3ff },
> > > @@ -2941,7 +2942,7 @@ static const struct i915_range mcr_ranges_gen12[] = {
> > >  	{},
> > >  };
> > >  
> > > -static const struct i915_range mcr_ranges_xehp[] = {
> > > +static const struct i915_mmio_range mcr_ranges_xehp[] = {
> > >  	{ .start =  0x4000, .end =  0x4aff },
> > >  	{ .start =  0x5200, .end =  0x52ff },
> > >  	{ .start =  0x5400, .end =  0x7fff },
> > > @@ -2960,7 +2961,7 @@ static const struct i915_range mcr_ranges_xehp[] = {
> > >  
> > >  static bool mcr_range(struct drm_i915_private *i915, u32 offset)
> > >  {
> > > -	const struct i915_range *mcr_ranges;
> > > +	const struct i915_mmio_range *mcr_ranges;
> > >  	int i;
> > >  
> > >  	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 55))
> > > diff --git a/drivers/gpu/drm/i915/i915_mmio_range.c b/drivers/gpu/drm/i915/i915_mmio_range.c
> > > new file mode 100644
> > > index 000000000000..724041e81aa7
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/i915_mmio_range.c
> > > @@ -0,0 +1,18 @@
> > > +// SPDX-License-Identifier: MIT
> > > +/*
> > > + * Copyright © 2025 Intel Corporation
> > > + */
> > > +
> > > +#include "i915_mmio_range.h"
> > > +
> > > +bool i915_mmio_range_table_contains(u32 addr, const struct i915_mmio_range *table)
> > > +{
> > > +	while (table->start || table->end) {
> > > +		if (addr >= table->start && addr <= table->end)
> > > +			return true;
> > > +
> > > +		table++;
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > 
> > Should perhaps follow up with:
> > - Convert intel_uncore BSEARCH() into __inline_bsearch() and double
> >   check things still get inlined
> > - Add a function to validate the table is sorted and call that for all
> >   the tables in some common init functions (ideally should be compile
> >   time checked but alas we have no constexpr functions in C)
> > - Use __inline_bsearch() here as well. Not sure if this itself would
> >   need to be inline to allow intel_uncore.c to use it for the shadow range
> >   checks (I suspect the concern there was about inlining the comparisons
> >   rather than avoiding a single bsearch() function call...)
> to be clear, you want this work in a follow up patch, not in the next
> revision?

Yeah, shouldn't mix all that in pne patch. Just some things that
stood out to me when I had a quick look at all the range checks.

-- 
Ville Syrjälä
Intel