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

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 09 Oct 2025, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Thu, Oct 09, 2025 at 03:08:28PM +0200, Andi Shyti wrote:
>> On Wed, Oct 08, 2025 at 05:34:39PM -0400, Rodrigo Vivi wrote:
>> > 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.
>> 
>> It's true that they are similar (regmap_reg_in_ranges() is
>> basically a copy paste), but regmap and mmio are two different
>> things (although conceptually similar in some cases). Working to
>> expose regmap_range so that we can use it as mmio_range looks to
>> me a bit of an overkill.
>
> fair enough. Let's go then with this i915-only approach here, but
> renaming the functions and structs.

Agreed.

I'll note that there's also include/linux/range.h, which could be
expanded to our use case, but it deals with u64 offsets.

BR,
Jani.

-- 
Jani Nikula, Intel