[Intel-gfx] [PATCH] drm/i915: Transform WaInPlaceDecompressionHang to a simple reg write

[Daniel Vetter]

On Thu, Sep 07, 2017 at 02:54:53PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 06, 2017 at 02:51:10PM -0700, Oscar Mateo wrote:
> > 
> > 
> > On 09/06/2017 02:43 PM, Chris Wilson wrote:
> > > Quoting Oscar Mateo (2017-09-06 22:27:47)
> > >>
> > >> On 09/06/2017 02:19 PM, Chris Wilson wrote:
> > >>> Quoting Oscar Mateo (2017-09-06 22:12:11)
> > >>>> Afaict, GEN9_GAMT_ECO_REG_RW_IA does not live in the context, so writing
> > >>>> it on every context creation is overkill (and wrong).
> > >>>>
> > >>>> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> > >>>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > >>>> Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
> > >>>> ---
> > >>>>    drivers/gpu/drm/i915/intel_engine_cs.c | 25 +++++++++++++++----------
> > >>>>    1 file changed, 15 insertions(+), 10 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > >>>> index 23812ec..9f01a5c 100644
> > >>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > >>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > >>>> @@ -985,8 +985,9 @@ static int skl_init_workarounds(struct intel_engine_cs *engine)
> > >>>>    
> > >>>>           /* WaInPlaceDecompressionHang:skl */
> > >>>>           if (IS_SKL_REVID(dev_priv, SKL_REVID_H0, REVID_FOREVER))
> > >>>> -               WA_SET_BIT(GEN9_GAMT_ECO_REG_RW_IA,
> > >>>> -                          GAMT_ECO_ENABLE_IN_PLACE_DECOMPRESS);
> > >>> Anything using a precalculated RMW value for a ctx register is indeed
> > >>> fishy. Whilst you are checking this register, can you check whether the
> > >>> other users of WA_SET_BIT/WA_CLR_BIT are indeed context bound?
> > >>> -Chris
> > >> Sure, I'll try to go through all of them (but I'd like to clarify first
> > >> if I should also be moving those I find to xxx_init_clock_gating).
> > > The short answer is probably not, init_clock_gating we expect to be
> > > targetting display w/a. There's not always a clear divide between GT and
> > > display, but we keep on muttering that we should keep them them as
> > > cleanly separated as possible so that we know where to look when
> > > different IP blocks are updated. (And yes the name is one of those
> > > things that we keep on waiting for someone else to fix.)
> > > -Chris
> > 
> > It's not only the name, there is even a comment saying non-context, 
> > non-WABB GT workarounds go here:
> > 
> > /**
> >   * intel_init_clock_gating_hooks - setup the clock gating hooks
> >   * @dev_priv: device private
> >   *
> >   * Setup the hooks that configure which clocks of a given platform can be
> >   * gated and also apply various GT and display specific workarounds for 
> > these
> >   * platforms. Note that some GT specific workarounds are applied separately
> >   * when GPU contexts or batchbuffers start their execution.
> >   */
> 
> IIRC one problem at least used to be that we called .init_clock_gating()
> after we'd already touched the GT. So it was probabably called too late.
> Not sure what the order is these days.
> 
> And I have a feeling that some platforms had a few registers that are
> clobbered by a GPU reset but aren't saved in the context. So those we'd
> need to reconfigure somewhere else becase .init_clock_gating() isn't
> called on GPU reset (except on pre-g4x where also the display gets
> clobbered). I have a feeling these were some L3 related things on IVB
> perhaps?

Those should all be put in the engine setup. And yes we got this wrong a
few times, where a gpu reset broke of fixed mesa (depending upon which way
round the w/a vs reset defaults was).

Maybe we need a "how to apply w/a writes and where" doc page somewhere
that we use to whack every wa patch with in review? This comes up for
every platform a few times ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch