Re: [PATCH] drm/i915: Transform WaInPlaceDecompressionHang to a simple reg write

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@intel.com>
> >>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>>> Signed-off-by: Oscar Mateo <oscar.mateo@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?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx