| Message ID | 20170928195436.32272-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 28, 2017 at 07:54:36PM +0000, Imre Deak wrote: > On GLK and CNL enabling a pipe with its pipe scaler enabled will result > in a FIFO underrun. This happens only once after driver loading or > system/runtime resume, more specifically after power well 1 gets > enabled; subsequent modesets seem to be free of underruns. The BSpec > workaround for this is to disable the pipe scaler clock gating for the > duration of modeset. Based on my tests disabling clock gating must be > done before enabling pipe scaling and we can re-enable it after the pipe > is enabled and one vblank has passed. Oh! Great! I had this Wa in my list here, but I was without access to HDSES and was postponing it... But now that you mention the bits it was easy to find on BSpec as well. > > For consistency I also checked if plane scaling would cause the same > problem, but that doesn't seem to trigger this problem. > > The patch is based on an earlier version from Ander. > > Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100302 > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++ > drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 82f36dd0cd94..40a3c045d9d0 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3811,6 +3811,14 @@ enum { > #define PWM2_GATING_DIS (1 << 14) > #define PWM1_GATING_DIS (1 << 13) > > +#define _CLKGATE_DIS_PSL_A 0x46520 > +#define _CLKGATE_DIS_PSL_B 0x46524 > +#define _CLKGATE_DIS_PSL_C 0x46528 > +#define DPF_GATING_DIS (1 << 10) On BSpec they also tells us to disable bits 8 and 9: "To disable Scaler clock gating, set bits 8, 9, and 10 of 0x46520 (Pipe A), 0x46524 (Pipe B), or 0x46528 (Pipe C)" > + > +#define CLKGATE_DIS_PSL(pipe) \ > + _MMIO_PIPE(pipe, _CLKGATE_DIS_PSL_A, _CLKGATE_DIS_PSL_B) > + > /* > * GEN10 clock gating regs > */ > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 026fa5460fe5..9d0b5a5596a5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5459,6 +5459,19 @@ static bool hsw_crtc_supports_ips(struct intel_crtc *crtc) > return HAS_IPS(to_i915(crtc->base.dev)) && crtc->pipe == PIPE_A; > } > > +static void glk_pipe_scaler_clock_gating_wa(struct drm_i915_private *dev_priv, > + enum pipe pipe, bool apply) > +{ > + u32 tmp = I915_READ(CLKGATE_DIS_PSL(pipe)); > + > + if (apply) > + tmp |= DPF_GATING_DIS; > + else > + tmp &= ~DPF_GATING_DIS; > + > + I915_WRITE(CLKGATE_DIS_PSL(pipe), tmp); > +} > + > static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, > struct drm_atomic_state *old_state) > { > @@ -5469,6 +5482,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, > enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder; > struct intel_atomic_state *old_intel_state = > to_intel_atomic_state(old_state); > + bool psl_clkgate_wa; > > if (WARN_ON(intel_crtc->active)) > return; > @@ -5522,6 +5536,12 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, > if (!transcoder_is_dsi(cpu_transcoder)) > intel_ddi_enable_pipe_clock(pipe_config); > > + /* WaDisableScalarClockGating: glk, cnl */ Please also add the BSpec reference Display WA #1180. > + psl_clkgate_wa = (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) && > + intel_crtc->config->pch_pfit.enabled; > + if (psl_clkgate_wa) > + glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true); > + Is this place enough? Wouldn't be better to start that on atomic commit before we set cdclock and mainly before we do a pre plane update? Also on spec they say: " Chance of underflows when the Pipe Scaler is enabled during a mode-set while the Planes are disabled. Workaround: Disable the Scaler clock gating when entering a mode-set routine. The Scaler clock gating should be re-enabled at the end of the mode-set sequence. " So I wonder if here is not already too late to disable the clock gatings. > if (INTEL_GEN(dev_priv) >= 9) > skylake_pfit_enable(intel_crtc); > else > @@ -5555,6 +5575,11 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, > > intel_encoders_enable(crtc, pipe_config, old_state); > > + if (psl_clkgate_wa) { > + intel_wait_for_vblank(dev_priv, pipe); > + glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, false); > + } > + > if (intel_crtc->config->has_pch_encoder) { > intel_wait_for_vblank(dev_priv, pipe); > intel_wait_for_vblank(dev_priv, pipe); > -- > 2.13.2 >
On Thu, Sep 28, 2017 at 01:18:30PM -0700, Rodrigo Vivi wrote: > On Thu, Sep 28, 2017 at 07:54:36PM +0000, Imre Deak wrote: > > On GLK and CNL enabling a pipe with its pipe scaler enabled will result > > in a FIFO underrun. This happens only once after driver loading or > > system/runtime resume, more specifically after power well 1 gets > > enabled; subsequent modesets seem to be free of underruns. The BSpec > > workaround for this is to disable the pipe scaler clock gating for the > > duration of modeset. Based on my tests disabling clock gating must be > > done before enabling pipe scaling and we can re-enable it after the pipe > > is enabled and one vblank has passed. > > Oh! Great! > I had this Wa in my list here, but I was without access to HDSES > and was postponing it... > > But now that you mention the bits it was easy to find on BSpec as well. > > > > > For consistency I also checked if plane scaling would cause the same > > problem, but that doesn't seem to trigger this problem. > > > > The patch is based on an earlier version from Ander. > > > > Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100302 > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++ > > drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++++++++++ > > 2 files changed, 33 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 82f36dd0cd94..40a3c045d9d0 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -3811,6 +3811,14 @@ enum { > > #define PWM2_GATING_DIS (1 << 14) > > #define PWM1_GATING_DIS (1 << 13) > > > > +#define _CLKGATE_DIS_PSL_A 0x46520 > > +#define _CLKGATE_DIS_PSL_B 0x46524 > > +#define _CLKGATE_DIS_PSL_C 0x46528 > > +#define DPF_GATING_DIS (1 << 10) > > On BSpec they also tells us to disable bits 8 and 9: > > "To disable Scaler clock gating, set bits 8, 9, and 10 of 0x46520 (Pipe A), 0x46524 (Pipe B), or 0x46528 (Pipe C)" Right, thanks for catching this. The original WA was only bit 10, and it does get rid of the problem. I haven't noticed this change, will update the patch based on that. > > > + > > +#define CLKGATE_DIS_PSL(pipe) \ > > + _MMIO_PIPE(pipe, _CLKGATE_DIS_PSL_A, _CLKGATE_DIS_PSL_B) > > + > > /* > > * GEN10 clock gating regs > > */ > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 026fa5460fe5..9d0b5a5596a5 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -5459,6 +5459,19 @@ static bool hsw_crtc_supports_ips(struct intel_crtc *crtc) > > return HAS_IPS(to_i915(crtc->base.dev)) && crtc->pipe == PIPE_A; > > } > > > > +static void glk_pipe_scaler_clock_gating_wa(struct drm_i915_private *dev_priv, > > + enum pipe pipe, bool apply) > > +{ > > + u32 tmp = I915_READ(CLKGATE_DIS_PSL(pipe)); > > + > > + if (apply) > > + tmp |= DPF_GATING_DIS; > > + else > > + tmp &= ~DPF_GATING_DIS; > > + > > + I915_WRITE(CLKGATE_DIS_PSL(pipe), tmp); > > +} > > + > > static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, > > struct drm_atomic_state *old_state) > > { > > @@ -5469,6 +5482,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, > > enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder; > > struct intel_atomic_state *old_intel_state = > > to_intel_atomic_state(old_state); > > + bool psl_clkgate_wa; > > > > if (WARN_ON(intel_crtc->active)) > > return; > > @@ -5522,6 +5536,12 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, > > if (!transcoder_is_dsi(cpu_transcoder)) > > intel_ddi_enable_pipe_clock(pipe_config); > > > > + /* WaDisableScalarClockGating: glk, cnl */ > > Please also add the BSpec reference Display WA #1180. Ok. > > + psl_clkgate_wa = (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) && > > + intel_crtc->config->pch_pfit.enabled; > > + if (psl_clkgate_wa) > > + glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true); > > + > > Is this place enough? Wouldn't be better to start that on atomic commit before we set cdclock > and mainly before we do a pre plane update? I'd prefer keeping it here leaving the WA's range narrow, unless it's proven that it's not enough. If CD clock would be a factor, then we'd have to force a CD clock disable/re-enable if the CD clock rate wouldn't change and that's not required by the WA. pre_plane_update is only called on the fastset path, and this WA is only needed during a full modeset (luckily b/c of the vblank wait). > > Also on spec they say: > " > Chance of underflows when the Pipe Scaler is enabled during a mode-set while the Planes are disabled. > Workaround: Disable the Scaler clock gating when entering a mode-set routine. > The Scaler clock gating should be re-enabled at the end of the mode-set sequence. > " > > So I wonder if here is not already too late to disable the clock gatings. Yea, the wording is a bit unclear, it doesn't either say for example when to re-enable the clock gating. I will ask for clarification, but I'd say it's a reasonable assumption that the pipe scaler clock gating doesn't have any effect until the pipe scaler is enabled and this is also what I see during my tests. So imo we can go with this until proven otherwise. > > > if (INTEL_GEN(dev_priv) >= 9) > > skylake_pfit_enable(intel_crtc); > > else > > @@ -5555,6 +5575,11 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, > > > > intel_encoders_enable(crtc, pipe_config, old_state); > > > > + if (psl_clkgate_wa) { > > + intel_wait_for_vblank(dev_priv, pipe); > > + glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, false); > > + } > > + > > if (intel_crtc->config->has_pch_encoder) { > > intel_wait_for_vblank(dev_priv, pipe); > > intel_wait_for_vblank(dev_priv, pipe); > > -- > > 2.13.2 > >
On Mon, Oct 02, 2017 at 08:55:28AM +0000, Patchwork wrote: > == Series Details == > > Series: drm/i915/glk, cnl: Implement WaDisableScalarClockGating (rev2) > URL : https://patchwork.freedesktop.org/series/31094/ > State : success Thanks for the review, pushed this to -dinq. > > == Summary == > > Series 31094v2 drm/i915/glk, cnl: Implement WaDisableScalarClockGating > https://patchwork.freedesktop.org/api/1.0/series/31094/revisions/2/mbox/ > > fi-bdw-5557u total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:458s > fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:471s > fi-blb-e6850 total:289 pass:224 dwarn:1 dfail:0 fail:0 skip:64 time:392s > fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:575s > fi-bwr-2160 total:289 pass:184 dwarn:0 dfail:0 fail:0 skip:105 time:289s > fi-bxt-dsi total:289 pass:259 dwarn:0 dfail:0 fail:0 skip:30 time:535s > fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:532s > fi-byt-j1900 total:289 pass:254 dwarn:1 dfail:0 fail:0 skip:34 time:542s > fi-byt-n2820 total:289 pass:250 dwarn:1 dfail:0 fail:0 skip:38 time:534s > fi-cfl-s total:289 pass:256 dwarn:1 dfail:0 fail:0 skip:32 time:564s > fi-cnl-y total:289 pass:261 dwarn:1 dfail:0 fail:0 skip:27 time:620s > fi-elk-e7500 total:289 pass:230 dwarn:0 dfail:0 fail:0 skip:59 time:435s > fi-glk-1 total:289 pass:257 dwarn:3 dfail:0 fail:0 skip:29 time:593s > fi-hsw-4770 total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:440s > fi-hsw-4770r total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:426s > fi-ilk-650 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:467s > fi-ivb-3520m total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:505s > fi-ivb-3770 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:479s > fi-kbl-7500u total:289 pass:264 dwarn:1 dfail:0 fail:0 skip:24 time:507s > fi-kbl-7560u total:289 pass:270 dwarn:0 dfail:0 fail:0 skip:19 time:583s > fi-kbl-7567u total:289 pass:265 dwarn:4 dfail:0 fail:0 skip:20 time:485s > fi-kbl-r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:589s > fi-pnv-d510 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:657s > fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:473s > fi-skl-6700k total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:543s > fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:591s > fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:473s > fi-snb-2520m total:289 pass:251 dwarn:0 dfail:0 fail:0 skip:38 time:584s > fi-snb-2600 total:289 pass:250 dwarn:0 dfail:0 fail:0 skip:39 time:433s > > b89a3b1a69f77be194ad4786906048a5e02b9975 drm-tip: 2017y-10m-02d-08h-07m-17s UTC integration manifest > 27ae0ed67a28 drm/i915/glk, cnl: Implement WaDisableScalarClockGating > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5867/
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 82f36dd0cd94..40a3c045d9d0 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3811,6 +3811,14 @@ enum { #define PWM2_GATING_DIS (1 << 14) #define PWM1_GATING_DIS (1 << 13) +#define _CLKGATE_DIS_PSL_A 0x46520 +#define _CLKGATE_DIS_PSL_B 0x46524 +#define _CLKGATE_DIS_PSL_C 0x46528 +#define DPF_GATING_DIS (1 << 10) + +#define CLKGATE_DIS_PSL(pipe) \ + _MMIO_PIPE(pipe, _CLKGATE_DIS_PSL_A, _CLKGATE_DIS_PSL_B) + /* * GEN10 clock gating regs */ diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 026fa5460fe5..9d0b5a5596a5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5459,6 +5459,19 @@ static bool hsw_crtc_supports_ips(struct intel_crtc *crtc) return HAS_IPS(to_i915(crtc->base.dev)) && crtc->pipe == PIPE_A; } +static void glk_pipe_scaler_clock_gating_wa(struct drm_i915_private *dev_priv, + enum pipe pipe, bool apply) +{ + u32 tmp = I915_READ(CLKGATE_DIS_PSL(pipe)); + + if (apply) + tmp |= DPF_GATING_DIS; + else + tmp &= ~DPF_GATING_DIS; + + I915_WRITE(CLKGATE_DIS_PSL(pipe), tmp); +} + static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, struct drm_atomic_state *old_state) { @@ -5469,6 +5482,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder; struct intel_atomic_state *old_intel_state = to_intel_atomic_state(old_state); + bool psl_clkgate_wa; if (WARN_ON(intel_crtc->active)) return; @@ -5522,6 +5536,12 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, if (!transcoder_is_dsi(cpu_transcoder)) intel_ddi_enable_pipe_clock(pipe_config); + /* WaDisableScalarClockGating: glk, cnl */ + psl_clkgate_wa = (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) && + intel_crtc->config->pch_pfit.enabled; + if (psl_clkgate_wa) + glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true); + if (INTEL_GEN(dev_priv) >= 9) skylake_pfit_enable(intel_crtc); else @@ -5555,6 +5575,11 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, intel_encoders_enable(crtc, pipe_config, old_state); + if (psl_clkgate_wa) { + intel_wait_for_vblank(dev_priv, pipe); + glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, false); + } + if (intel_crtc->config->has_pch_encoder) { intel_wait_for_vblank(dev_priv, pipe); intel_wait_for_vblank(dev_priv, pipe);
On GLK and CNL enabling a pipe with its pipe scaler enabled will result in a FIFO underrun. This happens only once after driver loading or system/runtime resume, more specifically after power well 1 gets enabled; subsequent modesets seem to be free of underruns. The BSpec workaround for this is to disable the pipe scaler clock gating for the duration of modeset. Based on my tests disabling clock gating must be done before enabling pipe scaling and we can re-enable it after the pipe is enabled and one vblank has passed. For consistency I also checked if plane scaling would cause the same problem, but that doesn't seem to trigger this problem. The patch is based on an earlier version from Ander. Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100302 Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++ drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+)