| Message ID | 20231129205122.3464299-1-matthew.s.atwood@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] drm/i915: Move reg_in_range_table | expand |
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. 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 >
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 > >
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) +{ + 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;
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(-)