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/[email protected]/ > > > > 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. Matt > > > > > > > > Suggested-by: Rodrigo Vivi <[email protected]> > > > Signed-off-by: Matt Atwood <[email protected]> > > > --- > > > 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. > > > > > > 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
