On Fri, Apr 5, 2019 at 1:43 PM Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
>
> Quoting Lucas De Marchi (2019-04-05 21:39:46)
> > On Fri, Apr 5, 2019 at 1:24 PM Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > Currently i915_reset.c mixes calls to intel_uncore, pci and our old
> > > style I915_READ mmio interfaces. Cast aside the old implicit macros,
> > > and harmonise on using uncore throughout.
> > >
> > > add/remove: 1/1 grow/shrink: 0/4 up/down: 65/-207 (-142)
> > > Function old new delta
> > > rmw_register - 65 +65
> > > gen8_reset_engines 945 942 -3
> > > g4x_do_reset 407 376 -31
> > > intel_gpu_reset 545 509 -36
> > > clear_register 63 - -63
> > > i915_clear_error_registers 461 387 -74
> > >
> > > A little bit of pointer dancing elimination works wonders.
> > >
> > > v2: Roll up the helpers into intel_uncore for general use
> > >
> > > With the helpers gcc was a little more eager to inline:
> > > add/remove: 0/1 grow/shrink: 1/3 up/down: 99/-133 (-34)
> > > Function old new delta
> > > i915_clear_error_registers 461 560 +99
> > > gen8_reset_engines 945 942 -3
> > > g4x_do_reset 407 376 -31
> > > intel_gpu_reset 545 509 -36
> > > clear_register 63 - -63
> > > Total: Before=1544400, After=1544366, chg -0.00%
> > >
> > > Win some, lose some, gcc is gcc.
> > >
> > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/i915_reset.c | 122 ++++++++++++++++------------
> > > drivers/gpu/drm/i915/intel_uncore.h | 23 +++++-
> > > 2 files changed, 89 insertions(+), 56 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> > > index d44dc8422e8c..ac168de6a66e 100644
> > > --- a/drivers/gpu/drm/i915/i915_reset.c
> > > +++ b/drivers/gpu/drm/i915/i915_reset.c
> > > @@ -18,6 +18,26 @@
> > > /* XXX How to handle concurrent GGTT updates using tiling registers? */
> > > #define RESET_UNDER_STOP_MACHINE 0
> > >
> > > +static void rmw_set(struct intel_uncore *uncore, i915_reg_t reg, u32 set)
> > > +{
> > > + intel_uncore_rmw(uncore, reg, 0, set);
> > > +}
> > > +
> > > +static void rmw_clear(struct intel_uncore *uncore, i915_reg_t reg, u32 clr)
> > > +{
> > > + intel_uncore_rmw(uncore, reg, clr, 0);
> > > +}
> > > +
> > > +static void rwm_set_fw(struct intel_uncore *uncore, i915_reg_t reg, u32 set)
> >
> > rwm?!?
>
> read, write, then modify. It's when you have that senior moment and
> enter a room forgetting why.
:)
I think the rwm_* functions are a nice addition. I know people say
"rmw is considered harmful", but is a real necessity in some places.
And the alternative is to roll our own in each part of the code. I
remember converting
icl_mg_pll_write() to use a rmw function, but then that got killed
under this argument.
Any chance to promote this to i915_drv.h (the place those functions
are today, but could be another smaller one, too) so we can use them
in more places?
thanks
Lucas De Marchi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
