[Intel-gfx] [PATCH v2] drm/i915/gen9: implement WaConextSwitchWithConcurrentTLBInvalidate

[Gore, Tim]

Tim Gore 
Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ


> -----Original Message-----
> From: Arun Siluvery [mailto:arun.siluvery at linux.intel.com]
> Sent: Friday, June 10, 2016 9:38 AM
> To: Gore, Tim; intel-gfx at lists.freedesktop.org
> Subject: Re: [PATCH v2] drm/i915/gen9: implement
> WaConextSwitchWithConcurrentTLBInvalidate
> 
> On 10/06/2016 12:16, Gore, Tim wrote:
> >
> >
> > Tim Gore
> > Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon
> > SN3 1RJ
> >
> >
> >> -----Original Message-----
> >> From: Arun Siluvery [mailto:arun.siluvery at linux.intel.com]
> >> Sent: Friday, June 10, 2016 7:30 AM
> >> To: Gore, Tim; intel-gfx at lists.freedesktop.org
> >> Subject: Re: [PATCH v2] drm/i915/gen9: implement
> >> WaConextSwitchWithConcurrentTLBInvalidate
> >>
> >> On 09/06/2016 20:19, tim.gore at intel.com wrote:
> >>> From: Tim Gore <tim.gore at intel.com>
> >>>
> >>> This patch enables a workaround for a mid thread preemption issue
> >>> where a hardware timing problem can prevent the context restore from
> >>> happening, leading to a hang.
> >>>
> >>> v2: move to gen9_init_workarounds (Arun)
> >>>
> >>> Signed-off-by: Tim Gore <tim.gore at intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_reg.h         | 4 ++++
> >>>    drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +++
> >>>    2 files changed, 7 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >>> b/drivers/gpu/drm/i915/i915_reg.h index 81d1896..2a6fc62 100644
> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>> @@ -1810,6 +1810,10 @@ enum skl_disp_power_wells {
> >>>    #define   GEN9_IZ_HASHING_MASK(slice)			(0x3
> <<
> >> ((slice) * 2))
> >>>    #define   GEN9_IZ_HASHING(slice, val)			((val) <<
> >> ((slice) * 2))
> >>>
> >>> +/* chicken reg for WaConextSwitchWithConcurrentTLBInvalidate */
> >>> +#define GEN9_CSFE_CHICKEN1_RCS _MMIO(0x20D4)
> >>> +#define   GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE (1 << 2)
> >>> +
> >>>    /* WaClearTdlStateAckDirtyBits */
> >>>    #define GEN8_STATE_ACK		_MMIO(0x20F0)
> >>>    #define GEN9_STATE_ACK_SLICE1	_MMIO(0x20F8)
> >>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>> b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>> index cf8d0bf..7c756ac 100644
> >>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>> @@ -1022,6 +1022,9 @@ static int gen9_init_workarounds(struct
> >> intel_engine_cs *engine)
> >>>    	if (ret)
> >>>    		return ret;
> >>>
> >>> +	/* WaConextSwitchWithConcurrentTLBInvalidate:skl,bxt,kbl */
> >>> +	I915_WRITE(GEN9_CSFE_CHICKEN1_RCS,
> >>>
> >>
> +_MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE)
> >> );
> >>> +
> >> WA_SET_BIT_MASKED(GEN9_CSFE_CHICKEN1_RCS,
> >> GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE);
> >>
> >> Please correct the spelling.
> >> We should try to keep WA regs in some order although it is not true
> >> for some of the existing ones but we should try to follow this rule
> >> for the new ones; HW whitelist registers are normally kept at the end.
> >> I think the correct place for this one is at the beginning of this
> >> function to maintain increasing order.
> >>
> >> regards
> >> Arun
> >>
> >>
> > Which spelling do you want corrected?
> 
> WaConextSwitchWithConcurrentTLBInvalidate - two occurrences.
> 
> regards
> Arun
> 
As per my previous replies to Jani and you, WaConextSwitchWithConcurrentTLBInvalidate
is copied directly from the documentation (bug report etc) and also from another
implementation of this workaround. Actually, to be precise the spec has
"WAConextSwitchWithConcurrentTLBInvalidate" (capitalised WA), but the bug report
and other implementation use "WaConextSwitchWithConcurrentTLBInvalidate".
I am not really comfortable with using a different spelling here, even if it is "correct".
Seems like it could lead to confusion or at least trip up some script.

  Tim
> >
> >    Tim
> >>>    	return 0;
> >>>    }
> >>>
> >>>
> >
> >