[Intel-gfx] [PATCH] drm/i915: Implement ReadHitWriteOnlyDisable.

[Rafael Antognolli]

On Wed, Nov 01, 2017 at 02:11:05PM -0700, Rodrigo Vivi wrote:
> On Wed, Nov 01, 2017 at 04:32:35PM +0000, Rafael Antognolli wrote:
> > The workaround for this is described as:
> > 
> > "if RenderSurfaceState.Num_Multisamples > 1, disable RCC clock gating if
> > RenderSurfaceState.Num_Multisamples == 1, set 0x7010[14] = 1"
> > 
> > So it looks like the userspace should be responsible for setting these,
> > based on the number of multisamples dependency. However, the register
> > that controls RCC clock gating is not a context register, and cannot be
> > set by userspace.
> > 
> > Since we would end up setting one or another based on the number of
> > multisamples anyway, it seems harmless to just set both all the time.
> > 
> > This change (specially the GEN10_READ_HIT_WRITEONLY_DISABLE bit)
> 
> I wonder if we shouldn't stay only with this bit. For me it looks like
> one or another.

I would say we need at least the GEN10_READ_HIT_WRITEONLY_DISABLE bit, and then
we can decide if we are adding the other one or not. Within the same
program (I think sometimes even within a single draw call), it's
possible that we have some surfaces that are multisampled while others
are not, so in that case we would probably have to set both anyway.

However, I don't have a test case that proves that the workaround for
the multisampled case is required...

> > improves CNL stability by avoiding some of the hangs seen in the
> > platform.
> 
> But this is what matters. If this is the safest option for us
> let's do it.
> 
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antognolli at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h        | 2 ++
> >  drivers/gpu/drm/i915/intel_engine_cs.c | 5 +++++
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 8c775e96b4e4..d9a65cebefaa 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3837,6 +3837,7 @@ enum {
> >   */
> >  #define SLICE_UNIT_LEVEL_CLKGATE	_MMIO(0x94d4)
> >  #define  SARBUNIT_CLKGATE_DIS		(1 << 5)
> > +#define  RCCUNIT_CLKGATE_DIS		(1 << 7)
> >  
> >  /*
> >   * Display engine regs
> > @@ -7016,6 +7017,7 @@ enum {
> >  #define GEN7_COMMON_SLICE_CHICKEN1		_MMIO(0x7010)
> >  # define GEN7_CSC1_RHWO_OPT_DISABLE_IN_RCC	((1<<10) | (1<<26))
> >  # define GEN9_RHWO_OPTIMIZATION_DISABLE		(1<<14)
> > +# define GEN10_READ_HIT_WRITEONLY_DISABLE	(1<<14)
> 
> I don't believe you need to redefine this.
> It is same as GEN9_RHWO_OPTIMIZATION_DISABLE.
> 
> RCC Read Hit Write Only Optimization Disabled, SKL+ o spec.

Oh, I haven't noticed that! Will fix it in the next version...

> >  #define COMMON_SLICE_CHICKEN2			_MMIO(0x7014)
> >  # define GEN9_PBE_COMPRESSED_HASH_SELECTION	(1<<13)
> >  # define GEN9_DISABLE_GATHER_AT_SET_SHADER_COMMON_SLICE (1<<12)
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index f31f2d6384c3..0d8e25a4623a 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -1320,6 +1320,11 @@ static int cnl_init_workarounds(struct intel_engine_cs *engine)
> >  	WA_SET_FIELD_MASKED(GEN8_CS_CHICKEN1, GEN9_PREEMPT_GPGPU_LEVEL_MASK,
> >  			    GEN9_PREEMPT_GPGPU_COMMAND_LEVEL);
> >  
> > +	/* ReadHitWriteOnlyDisable: cnl */
> 
> I was going to complain about the name but I saw on bspec it is really
> ReadHitWriteOnlyDisable while on wa_database it is WaReadHitWriteOnlyDisable
> 
> I would tend to prefer the second one, but with the first one the search on Bspec works
> and search on wa_database also works... while second one it would be found on BSpec.
> So let it be: ReadHitWriteOnlyDisable
> 
> Thanks,
> Rodrigo.
> 
> > +	WA_SET_BIT_MASKED(GEN7_COMMON_SLICE_CHICKEN1,
> > +			  GEN10_READ_HIT_WRITEONLY_DISABLE);
> > +	WA_SET_BIT_MASKED(SLICE_UNIT_LEVEL_CLKGATE, RCCUNIT_CLKGATE_DIS);
> > +
> 
> >  	/* WaEnablePreemptionGranularityControlByUMD:cnl */
> >  	I915_WRITE(GEN7_FF_SLICE_CS_CHICKEN1,
> >  		   _MASKED_BIT_ENABLE(GEN9_FFSC_PERCTX_PREEMPT_CTRL));
> > -- 
> > 2.13.6
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx