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@xxxxxxxxx>
> > ---
> > 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
