Re: [Intel-gfx] [Patch v3 1/2] drm/i915: Move reg_in_range_table

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Nov 30, 2023 at 03:00:59PM -0500, Rodrigo Vivi wrote:
> On Wed, Nov 29, 2023 at 12:51:21PM -0800, Matt Atwood wrote:
> > Function reg_in_range_table is useful in more places, move function to
> > intel_uncore.h to make available to more places.
> > 
> > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_perf.c    | 13 +------------
> >  drivers/gpu/drm/i915/intel_uncore.h | 12 ++++++++++++
> >  2 files changed, 13 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 7b1c8de2f9cb3..5e5dc73621379 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 "intel_uncore.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
> > @@ -4324,18 +4325,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))
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> > index f419c311a0dea..1e85eaec1fc5a 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.h
> > +++ b/drivers/gpu/drm/i915/intel_uncore.h
> > @@ -496,6 +496,18 @@ static inline int intel_uncore_write_and_verify(struct intel_uncore *uncore,
> >  	return (reg_val & mask) != expected_val ? -EINVAL : 0;
> >  }
> >  
> > +static inline bool reg_in_range_table(u32 addr, const struct i915_range *table)
> 
> exported functions should carry the name of the component that is exposing it.
> 
> something like intel_uncore_reg_in_range()
> 
> but also, based on your second patch, this doesn't seem to be the right thing to do.
> 
> although I hate the i915_utils.h, that sounds like a better place for a function like this.

If the function leaves intel_uncore, we should probably move i915_range
to wherever it ends up as well since that's defined in this header at
the moment.  Today intel_uncore is where pretty much all of the general
register handling is, even though "uncore" is a bit of a misnomer for
most of what we're actually doing.

This function probably doesn't need to be an inline either.  Just a
normal function in a .c would probably be best.


Matt

> 
> or on a second thought... maybe just duplicate the logic that is inside this
> function if only one extra call... or maybe duplicate this function along
> the other table definition instead of having the table in one place and using
> the helper from another place.
> 
> > +{
> > +	while (table->start || table->end) {
> > +		if (addr >= table->start && addr <= table->end)
> > +			return true;
> > +
> > +		table++;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static inline void __iomem *intel_uncore_regs(struct intel_uncore *uncore)
> >  {
> >  	return uncore->regs;
> > -- 
> > 2.40.1
> > 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation