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