[Intel-gfx] [PATCH v2] drm/i915: Convert i915_reset.c over to using uncore mmio

[Lucas De Marchi]

On Fri, Apr 5, 2019 at 1:43 PM Chris Wilson <chris at chris-wilson.co.uk> wrote:
>
> Quoting Lucas De Marchi (2019-04-05 21:39:46)
> > On Fri, Apr 5, 2019 at 1:24 PM Chris Wilson <chris at chris-wilson.co.uk> 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 at chris-wilson.co.uk>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > ---
> > >  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