| Message ID | 1475257729-11283-14-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 30, 2016 at 06:48:48PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Simply replace the linear search with the kernel's binary > search implementation. There is only six registers currently > in that table so this may not be that interesting. It adds a > function call so hopefully remains performance neutral for now. > > v2: No need for manual conversion to bool for return. > (Joonas Lahtinen) > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_uncore.c | 26 +++++++++++++++++++++----- > 1 file changed, 21 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 10b21516a7de..70a7fef79846 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -689,14 +689,30 @@ static void intel_shadow_table_check(void) > #endif > } > > +static int mmio_reg_cmp(const void *key, const void *elt) > +{ > + u32 offset = (u32)(unsigned long)key; > + i915_reg_t *reg = (i915_reg_t *)elt; > + > + if (offset < i915_mmio_reg_offset(*reg)) > + return -1; > + else if (offset > i915_mmio_reg_offset(*reg)) > + return 1; > + else > + return 0; There's no issue with using return offset - i915_mmio_reg_offset(*reg) here. > +} > + > static bool is_gen8_shadowed(u32 offset) > { > - int i; > - for (i = 0; i < ARRAY_SIZE(gen8_shadowed_regs); i++) > - if (offset == gen8_shadowed_regs[i].reg) > - return true; > + i915_reg_t *reg; > > - return false; > + reg = bsearch((void *)(unsigned long)offset, > + (const void *)gen8_shadowed_regs, > + ARRAY_SIZE(gen8_shadowed_regs), > + sizeof(i915_reg_t), > + mmio_reg_cmp); > + > + return reg; Or just return bseearch() ? (Probably like this for easing future patches.) -Chris
On pe, 2016-09-30 at 19:08 +0100, Chris Wilson wrote: > On Fri, Sep 30, 2016 at 06:48:48PM +0100, Tvrtko Ursulin wrote: > > +static int mmio_reg_cmp(const void *key, const void *elt) > > +{ > > + u32 offset = (u32)(unsigned long)key; > > + i915_reg_t *reg = (i915_reg_t *)elt; > > + > > + if (offset < i915_mmio_reg_offset(*reg)) > > + return -1; > > + else if (offset > i915_mmio_reg_offset(*reg)) > > + return 1; > > + else > > + return 0; > > There's no issue with using > > return offset - i915_mmio_reg_offset(*reg) > > here. > Why not? > > + reg = bsearch((void *)(unsigned long)offset, > > + (const void *)gen8_shadowed_regs, > > + ARRAY_SIZE(gen8_shadowed_regs), > > + sizeof(i915_reg_t), > > + mmio_reg_cmp); > > + > > + return reg; > > Or just return bseearch() ? (Probably like this for easing future > patches.) Suggested that too, but obviously he has something in mind. Regards, Joonas
On 03/10/2016 09:05, Joonas Lahtinen wrote: > On pe, 2016-09-30 at 19:08 +0100, Chris Wilson wrote: >> On Fri, Sep 30, 2016 at 06:48:48PM +0100, Tvrtko Ursulin wrote: >>> +static int mmio_reg_cmp(const void *key, const void *elt) >>> +{ >>> + u32 offset = (u32)(unsigned long)key; >>> + i915_reg_t *reg = (i915_reg_t *)elt; >>> + >>> + if (offset < i915_mmio_reg_offset(*reg)) >>> + return -1; >>> + else if (offset > i915_mmio_reg_offset(*reg)) >>> + return 1; >>> + else >>> + return 0; >> There's no issue with using >> >> return offset - i915_mmio_reg_offset(*reg) >> >> here. >> > Why not? > >>> + reg = bsearch((void *)(unsigned long)offset, >>> + (const void *)gen8_shadowed_regs, >>> + ARRAY_SIZE(gen8_shadowed_regs), >>> + sizeof(i915_reg_t), >>> + mmio_reg_cmp); >>> + >>> + return reg; >> Or just return bseearch() ? (Probably like this for easing future >> patches.) > Suggested that too, but obviously he has something in mind. It becomes a direct return in a following patch. It is just a virtue of me wanting to split out the series a lot for easier review, and then not picking up all relevant places to tidy when acting on review feedback. But this one as I said happens in a following patch. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 10b21516a7de..70a7fef79846 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -689,14 +689,30 @@ static void intel_shadow_table_check(void) #endif } +static int mmio_reg_cmp(const void *key, const void *elt) +{ + u32 offset = (u32)(unsigned long)key; + i915_reg_t *reg = (i915_reg_t *)elt; + + if (offset < i915_mmio_reg_offset(*reg)) + return -1; + else if (offset > i915_mmio_reg_offset(*reg)) + return 1; + else + return 0; +} + static bool is_gen8_shadowed(u32 offset) { - int i; - for (i = 0; i < ARRAY_SIZE(gen8_shadowed_regs); i++) - if (offset == gen8_shadowed_regs[i].reg) - return true; + i915_reg_t *reg; - return false; + reg = bsearch((void *)(unsigned long)offset, + (const void *)gen8_shadowed_regs, + ARRAY_SIZE(gen8_shadowed_regs), + sizeof(i915_reg_t), + mmio_reg_cmp); + + return reg; } #define __gen8_reg_write_fw_domains(offset) \