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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Oct 08, 2025 at 10:37:13AM -0700, Matt Roper wrote:
> On Wed, Oct 08, 2025 at 10:22:42AM -0700, Matt Atwood wrote:
> > On Wed, Oct 08, 2025 at 09:53:34AM -0700, Matt Roper wrote:
> > > On Tue, Oct 07, 2025 at 02:23:36PM -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.
> > > 
> > > It looks like this is a new revision of this patch from a couple years
> > > ago, right?
> > > 
> > >         https://lore.kernel.org/all/20231129205122.3464299-1-matthew.s.atwood@intel.com/
> > > 
> > > Even though it's been a long time, it would still be a good idea to
> > > include a patch changelog so that it's clear what's been changed and
> > > what review feedback was/wasn't incorporated.
> > Sorry, I will include it if theres another version
> > > 
> > > I'm also wondering if we should be thinking about moving i915 to use
> > > 'struct regmap_range' and existing functions like regmap_reg_in_ranges()
> > > rather than maintaining our own i915-specific versions of the logic.
> > > regmap in general does a bunch of other stuff that isn't relevant to
> > > i915, but it seems like we might be able to re-use the type definitions
> > > and basic lookups to avoid reinventing the wheel.
> > This is doable but just requires a rewrite of the current implementation
> > as it's not a 1:1 conversion.
> 
> The idea is that we'd eliminate 'struct i915_range' and related
> functions and just use the regmap types and functions instead.  It looks
> like the main difference is that the regmap lists are size-based, while
> our lists use a sentinel to mark the end of the table.
> 
> Although I did just notice that even the basic types and helpers for
> regmap rely on CONFIG_REGMAP, so that might be an argument against
> switching over since we'd need to add an extra kconfig dependency, and
> most of what it brings in isn't useful to us.  But probably more
> something for Rodrigo and the other maintainers to weigh in on.

Cc: all other maintainers.

I could easily be convinced either way.

I like the idea of reusing something existing and this helper and struct
does fit to our needs.
I don't mind having to include another config dependency here.
The part that is not good is to bring a lot more than we need :/

Perhaps the really right thing to do there would be to split regmap
into a generic map part and the support to the other different bus stuff.
Then we start using the generic part.

> 
> 
> Matt
> 
> > > 
> > > > 
> > > > 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 |  1 +
> > > >  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            | 45 ++++++++-------------
> > > >  drivers/gpu/drm/i915/intel_uncore.c         |  1 +
> > > >  drivers/gpu/drm/i915/intel_uncore.h         |  6 ---
> > > >  7 files changed, 57 insertions(+), 34 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..a3c08bde9b2e 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"
> > > > 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..c5484b16e28a
> > > > --- /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 reg_in_i915_range_table(u32 addr, const struct i915_range *table)
> > > 
> > > I think some of the feedback about function naming from the previous
> > > version was overlooked.  If we have a file i915_foo.c, then the general
> > > expectation is that the non-static function names should be
> > > i915_foo_*().  In this case, it means that functions you expose here
> > > should probably start with an "i915_mmio_range_" prefix.  So maybe
> > > something like "i915_mmio_range_table_contains()" would be a better
> > > choice.
> > Ah, the initial feedback I got from Rodrigo was that the original
> > function name could give the impression a function was more generic then
> > it actually was. The name I chose her was getting the struct name
> > (i915_range) into the function name. I can easily change the name
> > depending on what the community wants. 
> > 
> > MattA
> > > 
> > > Our existing code isn't entirely consistent about following this rule
> > > (especially for i915 which has a lot of historical baggage), but we try
> > > to follow it when writing new code.

My bad on that. But yeap, let's try to be a bit consistent now and not
get inspired in the bad examples. The file-name is a component name
and should be used as prefix on the rest. My bad, sorry.

Thanks,
Rodrigo.

> > > 
> > > 
> > > Matt
> > > 
> > > > +{
> > > > +	while (table->start || table->end) {
> > > > +		if (addr >= table->start && addr <= table->end)
> > > > +			return true;
> > > > +
> > > > +		table++;
> > > > +	}
> > > > +
> > > > +	return false;
> > > > +}
> > > > diff --git a/drivers/gpu/drm/i915/i915_mmio_range.h b/drivers/gpu/drm/i915/i915_mmio_range.h
> > > > new file mode 100644
> > > > index 000000000000..c3c477a8a0c1
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/i915/i915_mmio_range.h
> > > > @@ -0,0 +1,19 @@
> > > > +/* SPDX-License-Identifier: MIT */
> > > > +/*
> > > > + * Copyright © 2025 Intel Corporation
> > > > + */
> > > > +
> > > > +#ifndef __I915_MMIO_RANGE_H__
> > > > +#define __I915_MMIO_RANGE_H__
> > > > +
> > > > +#include <linux/types.h>
> > > > +
> > > > +/* Other register ranges (e.g., shadow tables, MCR tables, etc.) */
> > > > +struct i915_range {
> > > > +	u32 start;
> > > > +	u32 end;
> > > > +};
> > > > +
> > > > +bool reg_in_i915_range_table(u32 addr, const struct i915_range *table);
> > > > +
> > > > +#endif /* __I915_MMIO_RANGE_H__ */
> > > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > > > index 1658f1246c6f..b319398d7df1 100644
> > > > --- a/drivers/gpu/drm/i915/i915_perf.c
> > > > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > > > @@ -219,6 +219,7 @@
> > > >  #include "i915_perf.h"
> > > >  #include "i915_perf_oa_regs.h"
> > > >  #include "i915_reg.h"
> > > > +#include "i915_mmio_range.h"
> > > >  
> > > >  /* HW requires this to be a power of two, between 128k and 16M, though driver
> > > >   * is currently generally designed assuming the largest 16M size is used such
> > > > @@ -4320,18 +4321,6 @@ static bool gen8_is_valid_flex_addr(struct i915_perf *perf, u32 addr)
> > > >  	return false;
> > > >  }
> > > >  
> > > > -static bool reg_in_range_table(u32 addr, const struct i915_range *table)
> > > > -{
> > > > -	while (table->start || table->end) {
> > > > -		if (addr >= table->start && addr <= table->end)
> > > > -			return true;
> > > > -
> > > > -		table++;
> > > > -	}
> > > > -
> > > > -	return false;
> > > > -}
> > > > -
> > > >  #define REG_EQUAL(addr, mmio) \
> > > >  	((addr) == i915_mmio_reg_offset(mmio))
> > > >  
> > > > @@ -4421,61 +4410,61 @@ static const struct i915_range mtl_oa_mux_regs[] = {
> > > >  
> > > >  static bool gen7_is_valid_b_counter_addr(struct i915_perf *perf, u32 addr)
> > > >  {
> > > > -	return reg_in_range_table(addr, gen7_oa_b_counters);
> > > > +	return reg_in_i915_range_table(addr, gen7_oa_b_counters);
> > > >  }
> > > >  
> > > >  static bool gen8_is_valid_mux_addr(struct i915_perf *perf, u32 addr)
> > > >  {
> > > > -	return reg_in_range_table(addr, gen7_oa_mux_regs) ||
> > > > -		reg_in_range_table(addr, gen8_oa_mux_regs);
> > > > +	return reg_in_i915_range_table(addr, gen7_oa_mux_regs) ||
> > > > +		reg_in_i915_range_table(addr, gen8_oa_mux_regs);
> > > >  }
> > > >  
> > > >  static bool gen11_is_valid_mux_addr(struct i915_perf *perf, u32 addr)
> > > >  {
> > > > -	return reg_in_range_table(addr, gen7_oa_mux_regs) ||
> > > > -		reg_in_range_table(addr, gen8_oa_mux_regs) ||
> > > > -		reg_in_range_table(addr, gen11_oa_mux_regs);
> > > > +	return reg_in_i915_range_table(addr, gen7_oa_mux_regs) ||
> > > > +		reg_in_i915_range_table(addr, gen8_oa_mux_regs) ||
> > > > +		reg_in_i915_range_table(addr, gen11_oa_mux_regs);
> > > >  }
> > > >  
> > > >  static bool hsw_is_valid_mux_addr(struct i915_perf *perf, u32 addr)
> > > >  {
> > > > -	return reg_in_range_table(addr, gen7_oa_mux_regs) ||
> > > > -		reg_in_range_table(addr, hsw_oa_mux_regs);
> > > > +	return reg_in_i915_range_table(addr, gen7_oa_mux_regs) ||
> > > > +		reg_in_i915_range_table(addr, hsw_oa_mux_regs);
> > > >  }
> > > >  
> > > >  static bool chv_is_valid_mux_addr(struct i915_perf *perf, u32 addr)
> > > >  {
> > > > -	return reg_in_range_table(addr, gen7_oa_mux_regs) ||
> > > > -		reg_in_range_table(addr, chv_oa_mux_regs);
> > > > +	return reg_in_i915_range_table(addr, gen7_oa_mux_regs) ||
> > > > +		reg_in_i915_range_table(addr, chv_oa_mux_regs);
> > > >  }
> > > >  
> > > >  static bool gen12_is_valid_b_counter_addr(struct i915_perf *perf, u32 addr)
> > > >  {
> > > > -	return reg_in_range_table(addr, gen12_oa_b_counters);
> > > > +	return reg_in_i915_range_table(addr, gen12_oa_b_counters);
> > > >  }
> > > >  
> > > >  static bool mtl_is_valid_oam_b_counter_addr(struct i915_perf *perf, u32 addr)
> > > >  {
> > > >  	if (HAS_OAM(perf->i915) &&
> > > >  	    GRAPHICS_VER_FULL(perf->i915) >= IP_VER(12, 70))
> > > > -		return reg_in_range_table(addr, mtl_oam_b_counters);
> > > > +		return reg_in_i915_range_table(addr, mtl_oam_b_counters);
> > > >  
> > > >  	return false;
> > > >  }
> > > >  
> > > >  static bool xehp_is_valid_b_counter_addr(struct i915_perf *perf, u32 addr)
> > > >  {
> > > > -	return reg_in_range_table(addr, xehp_oa_b_counters) ||
> > > > -		reg_in_range_table(addr, gen12_oa_b_counters) ||
> > > > +	return reg_in_i915_range_table(addr, xehp_oa_b_counters) ||
> > > > +		reg_in_i915_range_table(addr, gen12_oa_b_counters) ||
> > > >  		mtl_is_valid_oam_b_counter_addr(perf, addr);
> > > >  }
> > > >  
> > > >  static bool gen12_is_valid_mux_addr(struct i915_perf *perf, u32 addr)
> > > >  {
> > > >  	if (GRAPHICS_VER_FULL(perf->i915) >= IP_VER(12, 70))
> > > > -		return reg_in_range_table(addr, mtl_oa_mux_regs);
> > > > +		return reg_in_i915_range_table(addr, mtl_oa_mux_regs);
> > > >  	else
> > > > -		return reg_in_range_table(addr, gen12_oa_mux_regs);
> > > > +		return reg_in_i915_range_table(addr, gen12_oa_mux_regs);
> > > >  }
> > > >  
> > > >  static u32 mask_reg_value(u32 reg, u32 val)
> > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > > index 8cb59f8d1f4c..aea81e41d6dd 100644
> > > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > > @@ -35,6 +35,7 @@
> > > >  #include "i915_reg.h"
> > > >  #include "i915_vgpu.h"
> > > >  #include "i915_wait_util.h"
> > > > +#include "i915_mmio_range.h"
> > > >  #include "intel_uncore_trace.h"
> > > >  
> > > >  #define FORCEWAKE_ACK_TIMEOUT_MS 50
> > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> > > > index 6048b99b96cb..6df624afab30 100644
> > > > --- a/drivers/gpu/drm/i915/intel_uncore.h
> > > > +++ b/drivers/gpu/drm/i915/intel_uncore.h
> > > > @@ -123,12 +123,6 @@ struct intel_forcewake_range {
> > > >  	enum forcewake_domains domains;
> > > >  };
> > > >  
> > > > -/* Other register ranges (e.g., shadow tables, MCR tables, etc.) */
> > > > -struct i915_range {
> > > > -	u32 start;
> > > > -	u32 end;
> > > > -};
> > > > -
> > > >  struct intel_uncore {
> > > >  	void __iomem *regs;
> > > >  
> > > > -- 
> > > > 2.51.0
> > > > 
> > > 
> > > -- 
> > > Matt Roper
> > > Graphics Software Engineer
> > > Linux GPU Platform Enablement
> > > Intel Corporation
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation