| Message ID | 20190328173519.1856-1-radhakrishna.sripada@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] drm/i915/icl: Fix clockgating issue when using scalers | expand |
On Thu, Mar 28, 2019 at 10:35:19AM -0700, Radhakrishna Sripada wrote: > Fixes the clock-gating issue when pipe scaling is enabled. > (Lineage #2006604312) > > V2: Fix typo in headline(Chris) > Handle the non double buffered nature of the register(Ville) > V3: Fix checkpatch warning. BAT failure for V2 on gen3 looks unrelated. > V4: Split the icl and skl wa's(Ville) > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Aditya Swarup <aditya.swarup@intel.com> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 48 ++++++++++++++++++++-------- > 1 file changed, 34 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 8576a7f799f2..c3ca9cfd36fe 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -466,6 +466,7 @@ static const struct intel_limit intel_limits_bxt = { > .p2 = { .p2_slow = 1, .p2_fast = 20 }, > }; > > +/* WA Display #0827: Gen9:all */ > static void > skl_wa_clkgate(struct drm_i915_private *dev_priv, int pipe, bool enable) > { > @@ -478,6 +479,17 @@ skl_wa_clkgate(struct drm_i915_private *dev_priv, int pipe, bool enable) > ~(DUPS1_GATING_DIS | DUPS2_GATING_DIS)); > } > > +/* Wa_2006604312:icl */ > +static void > +icl_wa_clkgate(struct drm_i915_private *dev_priv, int pipe, bool enable) These functions names (including the skl one) are rather too generic and don't describe which clock gating we're disabling. But fixing that is probably material for another patch. > +{ > + if (enable) > + I915_WRITE(CLKGATE_DIS_PSL(pipe), DPFR_GATING_DIS); > + else > + I915_WRITE(CLKGATE_DIS_PSL(pipe), > + I915_READ(CLKGATE_DIS_PSL(pipe)) & ~DPFR_GATING_DIS); No RMW for enable but RMW for disable? Seems rather inconsistent. > +} > + > static bool > needs_modeset(const struct drm_crtc_state *state) > { > @@ -5481,14 +5493,18 @@ static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_s > return !old_crtc_state->ips_enabled; > } > > -static bool needs_nv12_wa(struct drm_i915_private *dev_priv, > - const struct intel_crtc_state *crtc_state) > +static bool skl_needs_clk_wa(struct drm_i915_private *dev_priv, > + const struct intel_crtc_state *crtc_state) Still mixing our workarounds more than I'd like. I suggest keeping the two entirely separate. > { > - if (!crtc_state->nv12_planes) > - return false; > - > /* WA Display #0827: Gen9:all */ > - if (IS_GEN(dev_priv, 9) && !IS_GEMINILAKE(dev_priv)) > + if (!!crtc_state->nv12_planes && IS_GEN(dev_priv, 9) && > + !IS_GEMINILAKE(dev_priv)) > + return true; > + > + /* > + * Wa_2006604312:icl > + */ > + if (IS_ICELAKE(dev_priv) && crtc_state->pch_pfit.enabled) > return true; > > return false; > @@ -5527,10 +5543,12 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) > intel_post_enable_primary(&crtc->base, pipe_config); > } > > - /* Display WA 827 */ > - if (needs_nv12_wa(dev_priv, old_crtc_state) && > - !needs_nv12_wa(dev_priv, pipe_config)) { > - skl_wa_clkgate(dev_priv, crtc->pipe, false); > + if (skl_needs_clk_wa(dev_priv, old_crtc_state) && > + !skl_needs_clk_wa(dev_priv, pipe_config)) { > + if (IS_ICELAKE(dev_priv)) > + icl_wa_clkgate(dev_priv, crtc->pipe, false); > + else > + skl_wa_clkgate(dev_priv, crtc->pipe, false); > } > } > > @@ -5566,10 +5584,12 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, > intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false); > } > > - /* Display WA 827 */ > - if (!needs_nv12_wa(dev_priv, old_crtc_state) && > - needs_nv12_wa(dev_priv, pipe_config)) { > - skl_wa_clkgate(dev_priv, crtc->pipe, true); > + if (!skl_needs_clk_wa(dev_priv, old_crtc_state) && > + skl_needs_clk_wa(dev_priv, pipe_config)) { > + if (IS_ICELAKE(dev_priv)) > + icl_wa_clkgate(dev_priv, crtc->pipe, true); > + else > + skl_wa_clkgate(dev_priv, crtc->pipe, true); > } > > /* > -- > 2.20.0.rc2.7.g965798d1f299
On Fri, 2019-03-29 at 20:39 +0200, Ville Syrjälä wrote: > On Thu, Mar 28, 2019 at 10:35:19AM -0700, Radhakrishna Sripada wrote: > > Fixes the clock-gating issue when pipe scaling is enabled. > > (Lineage #2006604312) > > > > V2: Fix typo in headline(Chris) > > Handle the non double buffered nature of the register(Ville) > > V3: Fix checkpatch warning. BAT failure for V2 on gen3 looks > > unrelated. > > V4: Split the icl and skl wa's(Ville) > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Aditya Swarup <aditya.swarup@intel.com> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com > > > > > --- > > drivers/gpu/drm/i915/intel_display.c | 48 ++++++++++++++++++++-- > > ------ > > 1 file changed, 34 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 8576a7f799f2..c3ca9cfd36fe 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -466,6 +466,7 @@ static const struct intel_limit > > intel_limits_bxt = { > > .p2 = { .p2_slow = 1, .p2_fast = 20 }, > > }; > > > > +/* WA Display #0827: Gen9:all */ > > static void > > skl_wa_clkgate(struct drm_i915_private *dev_priv, int pipe, bool > > enable) > > { > > @@ -478,6 +479,17 @@ skl_wa_clkgate(struct drm_i915_private > > *dev_priv, int pipe, bool enable) > > ~(DUPS1_GATING_DIS | DUPS2_GATING_DIS)); > > } > > > > +/* Wa_2006604312:icl */ > > +static void > > +icl_wa_clkgate(struct drm_i915_private *dev_priv, int pipe, bool > > enable) > > These functions names (including the skl one) are rather too generic > and don't describe which clock gating we're disabling. But fixing > that is probably material for another patch. Sure let me spin out another patch for this. > > > +{ > > + if (enable) > > + I915_WRITE(CLKGATE_DIS_PSL(pipe), DPFR_GATING_DIS); > > + else > > + I915_WRITE(CLKGATE_DIS_PSL(pipe), > > + I915_READ(CLKGATE_DIS_PSL(pipe)) & > > ~DPFR_GATING_DIS); > > No RMW for enable but RMW for disable? Seems rather inconsistent. Sure looks inconsistent. Let me fix it next rev. > > > +} > > + > > static bool > > needs_modeset(const struct drm_crtc_state *state) > > { > > @@ -5481,14 +5493,18 @@ static bool > > hsw_post_update_enable_ips(const struct intel_crtc_state > > *old_crtc_s > > return !old_crtc_state->ips_enabled; > > } > > > > -static bool needs_nv12_wa(struct drm_i915_private *dev_priv, > > - const struct intel_crtc_state *crtc_state) > > +static bool skl_needs_clk_wa(struct drm_i915_private *dev_priv, > > + const struct intel_crtc_state *crtc_state) > > Still mixing our workarounds more than I'd like. I suggest keeping > the two entirely separate. Sure let me separate it completely. Avoids duplicating checks as well. Thanks, Radhakrishna(RK) Sripada > > > { > > - if (!crtc_state->nv12_planes) > > - return false; > > - > > /* WA Display #0827: Gen9:all */ > > - if (IS_GEN(dev_priv, 9) && !IS_GEMINILAKE(dev_priv)) > > + if (!!crtc_state->nv12_planes && IS_GEN(dev_priv, 9) && > > + !IS_GEMINILAKE(dev_priv)) > > + return true; > > + > > + /* > > + * Wa_2006604312:icl > > + */ > > + if (IS_ICELAKE(dev_priv) && crtc_state->pch_pfit.enabled) > > return true; > > > > return false; > > @@ -5527,10 +5543,12 @@ static void intel_post_plane_update(struct > > intel_crtc_state *old_crtc_state) > > intel_post_enable_primary(&crtc->base, > > pipe_config); > > } > > > > - /* Display WA 827 */ > > - if (needs_nv12_wa(dev_priv, old_crtc_state) && > > - !needs_nv12_wa(dev_priv, pipe_config)) { > > - skl_wa_clkgate(dev_priv, crtc->pipe, false); > > + if (skl_needs_clk_wa(dev_priv, old_crtc_state) && > > + !skl_needs_clk_wa(dev_priv, pipe_config)) { > > + if (IS_ICELAKE(dev_priv)) > > + icl_wa_clkgate(dev_priv, crtc->pipe, false); > > + else > > + skl_wa_clkgate(dev_priv, crtc->pipe, false); > > } > > } > > > > @@ -5566,10 +5584,12 @@ static void intel_pre_plane_update(struct > > intel_crtc_state *old_crtc_state, > > intel_set_cpu_fifo_underrun_reporting(dev_priv, > > crtc->pipe, false); > > } > > > > - /* Display WA 827 */ > > - if (!needs_nv12_wa(dev_priv, old_crtc_state) && > > - needs_nv12_wa(dev_priv, pipe_config)) { > > - skl_wa_clkgate(dev_priv, crtc->pipe, true); > > + if (!skl_needs_clk_wa(dev_priv, old_crtc_state) && > > + skl_needs_clk_wa(dev_priv, pipe_config)) { > > + if (IS_ICELAKE(dev_priv)) > > + icl_wa_clkgate(dev_priv, crtc->pipe, true); > > + else > > + skl_wa_clkgate(dev_priv, crtc->pipe, true); > > } > > > > /* > > -- > > 2.20.0.rc2.7.g965798d1f299 > >
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 8576a7f799f2..c3ca9cfd36fe 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -466,6 +466,7 @@ static const struct intel_limit intel_limits_bxt = { .p2 = { .p2_slow = 1, .p2_fast = 20 }, }; +/* WA Display #0827: Gen9:all */ static void skl_wa_clkgate(struct drm_i915_private *dev_priv, int pipe, bool enable) { @@ -478,6 +479,17 @@ skl_wa_clkgate(struct drm_i915_private *dev_priv, int pipe, bool enable) ~(DUPS1_GATING_DIS | DUPS2_GATING_DIS)); } +/* Wa_2006604312:icl */ +static void +icl_wa_clkgate(struct drm_i915_private *dev_priv, int pipe, bool enable) +{ + if (enable) + I915_WRITE(CLKGATE_DIS_PSL(pipe), DPFR_GATING_DIS); + else + I915_WRITE(CLKGATE_DIS_PSL(pipe), + I915_READ(CLKGATE_DIS_PSL(pipe)) & ~DPFR_GATING_DIS); +} + static bool needs_modeset(const struct drm_crtc_state *state) { @@ -5481,14 +5493,18 @@ static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_s return !old_crtc_state->ips_enabled; } -static bool needs_nv12_wa(struct drm_i915_private *dev_priv, - const struct intel_crtc_state *crtc_state) +static bool skl_needs_clk_wa(struct drm_i915_private *dev_priv, + const struct intel_crtc_state *crtc_state) { - if (!crtc_state->nv12_planes) - return false; - /* WA Display #0827: Gen9:all */ - if (IS_GEN(dev_priv, 9) && !IS_GEMINILAKE(dev_priv)) + if (!!crtc_state->nv12_planes && IS_GEN(dev_priv, 9) && + !IS_GEMINILAKE(dev_priv)) + return true; + + /* + * Wa_2006604312:icl + */ + if (IS_ICELAKE(dev_priv) && crtc_state->pch_pfit.enabled) return true; return false; @@ -5527,10 +5543,12 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) intel_post_enable_primary(&crtc->base, pipe_config); } - /* Display WA 827 */ - if (needs_nv12_wa(dev_priv, old_crtc_state) && - !needs_nv12_wa(dev_priv, pipe_config)) { - skl_wa_clkgate(dev_priv, crtc->pipe, false); + if (skl_needs_clk_wa(dev_priv, old_crtc_state) && + !skl_needs_clk_wa(dev_priv, pipe_config)) { + if (IS_ICELAKE(dev_priv)) + icl_wa_clkgate(dev_priv, crtc->pipe, false); + else + skl_wa_clkgate(dev_priv, crtc->pipe, false); } } @@ -5566,10 +5584,12 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false); } - /* Display WA 827 */ - if (!needs_nv12_wa(dev_priv, old_crtc_state) && - needs_nv12_wa(dev_priv, pipe_config)) { - skl_wa_clkgate(dev_priv, crtc->pipe, true); + if (!skl_needs_clk_wa(dev_priv, old_crtc_state) && + skl_needs_clk_wa(dev_priv, pipe_config)) { + if (IS_ICELAKE(dev_priv)) + icl_wa_clkgate(dev_priv, crtc->pipe, true); + else + skl_wa_clkgate(dev_priv, crtc->pipe, true); } /*
Fixes the clock-gating issue when pipe scaling is enabled. (Lineage #2006604312) V2: Fix typo in headline(Chris) Handle the non double buffered nature of the register(Ville) V3: Fix checkpatch warning. BAT failure for V2 on gen3 looks unrelated. V4: Split the icl and skl wa's(Ville) Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Aditya Swarup <aditya.swarup@intel.com> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 48 ++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 14 deletions(-)