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/[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.
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 <[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. 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
