| Message ID | 1465483794-27623-1-git-send-email-tim.gore@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/06/2016 20:19, tim.gore@intel.com wrote: > From: Tim Gore <tim.gore@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@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 > return 0; > } > >
Tim Gore Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ > -----Original Message----- > From: Arun Siluvery [mailto:arun.siluvery@linux.intel.com] > Sent: Friday, June 10, 2016 7:30 AM > To: Gore, Tim; intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH v2] drm/i915/gen9: implement > WaConextSwitchWithConcurrentTLBInvalidate > > On 09/06/2016 20:19, tim.gore@intel.com wrote: > > From: Tim Gore <tim.gore@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@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? Tim > > return 0; > > } > > > >
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@linux.intel.com] >> Sent: Friday, June 10, 2016 7:30 AM >> To: Gore, Tim; intel-gfx@lists.freedesktop.org >> Subject: Re: [PATCH v2] drm/i915/gen9: implement >> WaConextSwitchWithConcurrentTLBInvalidate >> >> On 09/06/2016 20:19, tim.gore@intel.com wrote: >>> From: Tim Gore <tim.gore@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@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 > > Tim >>> return 0; >>> } >>> >>> > >
Tim Gore Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ > -----Original Message----- > From: Arun Siluvery [mailto:arun.siluvery@linux.intel.com] > Sent: Friday, June 10, 2016 9:38 AM > To: Gore, Tim; intel-gfx@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@linux.intel.com] > >> Sent: Friday, June 10, 2016 7:30 AM > >> To: Gore, Tim; intel-gfx@lists.freedesktop.org > >> Subject: Re: [PATCH v2] drm/i915/gen9: implement > >> WaConextSwitchWithConcurrentTLBInvalidate > >> > >> On 09/06/2016 20:19, tim.gore@intel.com wrote: > >>> From: Tim Gore <tim.gore@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@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; > >>> } > >>> > >>> > > > >
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)); + return 0; }