| Message ID | 20191127190556.1574-6-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Cleanups around pre/post plane update | expand |
On Wed, 2019-11-27 at 21:05 +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > We have the active_planes bitmask now so use it to properly > determine when some planes are visible for the gen2 underrun > workaround. > > This let's us almost eliminate intel_post_enable_primary(). > The manual underrun checks we can simply move into > intel_atomic_commit_tail() since they loop over all the pipes > already. No point in repeating the checks multiple times when > there are multiple pipes in the commit. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 155 +++++++++------ > ---- > 1 file changed, 70 insertions(+), 85 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 72655b5b1365..5368f3ab70af 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -5908,37 +5908,6 @@ static void > intel_crtc_dpms_overlay_disable(struct intel_crtc *intel_crtc) > */ > } > > -/** > - * intel_post_enable_primary - Perform operations after enabling > primary plane > - * @crtc: the CRTC whose primary plane was just enabled > - * @new_crtc_state: the enabling state > - * > - * Performs potentially sleeping operations that must be done after > the primary > - * plane is enabled, such as updating FBC and IPS. Note that this > may be > - * called due to an explicit primary plane update, or due to an > implicit > - * re-enable that is caused when a sprite plane is updated to no > longer > - * completely hide the primary plane. > - */ > -static void > -intel_post_enable_primary(struct intel_crtc *crtc) > -{ > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > - enum pipe pipe = crtc->pipe; > - > - /* > - * Gen2 reports pipe underruns whenever all planes are > disabled. > - * So don't enable underrun reporting before at least some > planes > - * are enabled. > - * FIXME: Need to fix the logic to work when we turn off all > planes > - * but leave the pipe running. > - */ > - if (IS_GEN(dev_priv, 2)) > - intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, > true); > - > - /* Underruns don't always raise interrupts, so check manually. > */ > - intel_check_cpu_fifo_underruns(dev_priv); > - intel_check_pch_fifo_underruns(dev_priv); > -} > > /* FIXME get rid of this and use pre_plane_update */ > static void > @@ -6059,6 +6028,20 @@ static bool needs_scalerclk_wa(const struct > intel_crtc_state *crtc_state) > return false; > } > > +static bool planes_enabling(const struct intel_crtc_state > *old_crtc_state, > + const struct intel_crtc_state > *new_crtc_state) > +{ > + return (!old_crtc_state->active_planes || > needs_modeset(new_crtc_state)) && > + new_crtc_state->active_planes; > +} > + > +static bool planes_disabling(const struct intel_crtc_state > *old_crtc_state, > + const struct intel_crtc_state > *new_crtc_state) > +{ > + return old_crtc_state->active_planes && > + (!new_crtc_state->active_planes || > needs_modeset(new_crtc_state)); > +} > + > static void intel_post_plane_update(struct intel_atomic_state > *state, > struct intel_crtc *crtc) > { > @@ -6068,10 +6051,9 @@ static void intel_post_plane_update(struct > intel_atomic_state *state, > intel_atomic_get_old_crtc_state(state, crtc); > const struct intel_crtc_state *new_crtc_state = > intel_atomic_get_new_crtc_state(state, crtc); > - const struct intel_plane_state *old_primary_state = > - intel_atomic_get_old_plane_state(state, primary); > const struct intel_plane_state *new_primary_state = > intel_atomic_get_new_plane_state(state, primary); > + enum pipe pipe = crtc->pipe; > > intel_frontbuffer_flip(dev_priv, new_crtc_state->fb_bits); > > @@ -6081,22 +6063,16 @@ static void intel_post_plane_update(struct > intel_atomic_state *state, > if (hsw_post_update_enable_ips(old_crtc_state, new_crtc_state)) > hsw_enable_ips(new_crtc_state); > > - if (new_primary_state) { > + if (new_primary_state) > intel_fbc_post_update(crtc); > > - if (new_primary_state->uapi.visible && > - (needs_modeset(new_crtc_state) || > - !old_primary_state->uapi.visible)) > - intel_post_enable_primary(crtc); > - } > - > if (needs_nv12_wa(old_crtc_state) && > !needs_nv12_wa(new_crtc_state)) > - skl_wa_827(dev_priv, crtc->pipe, false); > + skl_wa_827(dev_priv, pipe, false); Nitpick: could be left as it was(s/crtc->pipe/pipe) > > if (needs_scalerclk_wa(old_crtc_state) && > !needs_scalerclk_wa(new_crtc_state)) > - icl_wa_scalerclkgating(dev_priv, crtc->pipe, false); > + icl_wa_scalerclkgating(dev_priv, pipe, false); > } > > static void intel_pre_plane_update(struct intel_atomic_state *state, > @@ -6108,35 +6084,25 @@ static void intel_pre_plane_update(struct > intel_atomic_state *state, > intel_atomic_get_old_crtc_state(state, crtc); > const struct intel_crtc_state *new_crtc_state = > intel_atomic_get_new_crtc_state(state, crtc); > - const struct intel_plane_state *old_primary_state = > - intel_atomic_get_old_plane_state(state, primary); > const struct intel_plane_state *new_primary_state = > intel_atomic_get_new_plane_state(state, primary); > - bool modeset = needs_modeset(new_crtc_state); > + enum pipe pipe = crtc->pipe; > > if (hsw_pre_update_disable_ips(old_crtc_state, new_crtc_state)) > hsw_disable_ips(old_crtc_state); > > - if (new_primary_state) { > + if (new_primary_state) > intel_fbc_pre_update(crtc, new_crtc_state, > new_primary_state); > - /* > - * Gen2 reports pipe underruns whenever all planes are > disabled. > - * So disable underrun reporting before all the planes > get disabled. > - */ > - if (IS_GEN(dev_priv, 2) && old_primary_state- > >uapi.visible && > - (modeset || !new_primary_state->uapi.visible)) > - intel_set_cpu_fifo_underrun_reporting(dev_priv, > crtc->pipe, false); > - } > > /* Display WA 827 */ > if (!needs_nv12_wa(old_crtc_state) && > needs_nv12_wa(new_crtc_state)) > - skl_wa_827(dev_priv, crtc->pipe, true); > + skl_wa_827(dev_priv, pipe, true); > > /* Wa_2006604312:icl */ > if (!needs_scalerclk_wa(old_crtc_state) && > needs_scalerclk_wa(new_crtc_state)) > - icl_wa_scalerclkgating(dev_priv, crtc->pipe, true); > + icl_wa_scalerclkgating(dev_priv, pipe, true); > > /* > * Vblank time updates from the shadow to live plane control > register > @@ -6149,7 +6115,7 @@ static void intel_pre_plane_update(struct > intel_atomic_state *state, > */ > if (HAS_GMCH(dev_priv) && old_crtc_state->hw.active && > new_crtc_state->disable_cxsr && > intel_set_memory_cxsr(dev_priv, false)) > - intel_wait_for_vblank(dev_priv, crtc->pipe); > + intel_wait_for_vblank(dev_priv, pipe); > > /* > * IVB workaround: must disable low power watermarks for at > least > @@ -6160,33 +6126,43 @@ static void intel_pre_plane_update(struct > intel_atomic_state *state, > */ > if (old_crtc_state->hw.active && > new_crtc_state->disable_lp_wm && > ilk_disable_lp_wm(dev_priv)) > - intel_wait_for_vblank(dev_priv, crtc->pipe); > + intel_wait_for_vblank(dev_priv, pipe); > > /* > - * If we're doing a modeset, we're done. No need to do any > pre-vblank > - * watermark programming here. > + * If we're doing a modeset we don't need to do any > + * pre-vblank watermark programming here. > */ > - if (needs_modeset(new_crtc_state)) > - return; > + if (!needs_modeset(new_crtc_state)) { > + /* > + * For platforms that support atomic watermarks, > program the > + * 'intermediate' watermarks immediately. On pre-gen9 > platforms, these > + * will be the intermediate values that are safe for > both pre- and > + * post- vblank; when vblank happens, the 'active' > values will be set > + * to the final 'target' values and we'll do this again > to get the > + * optimal watermarks. For gen9+ platforms, the values > we program here > + * will be the final target values which will get > automatically latched > + * at vblank time; no further programming will be > necessary. > + * > + * If a platform hasn't been transitioned to atomic > watermarks yet, > + * we'll continue to update watermarks the old way, if > flags tell > + * us to. > + */ A few lines are now over 80 characters but I know you did not wanted to change it. Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > + if (dev_priv->display.initial_watermarks) > + dev_priv->display.initial_watermarks(state, > crtc); > + else if (new_crtc_state->update_wm_pre) > + intel_update_watermarks(crtc); > + } > > /* > - * For platforms that support atomic watermarks, program the > - * 'intermediate' watermarks immediately. On pre-gen9 > platforms, these > - * will be the intermediate values that are safe for both pre- > and > - * post- vblank; when vblank happens, the 'active' values will > be set > - * to the final 'target' values and we'll do this again to get > the > - * optimal watermarks. For gen9+ platforms, the values we > program here > - * will be the final target values which will get automatically > latched > - * at vblank time; no further programming will be necessary. > + * Gen2 reports pipe underruns whenever all planes are > disabled. > + * So disable underrun reporting before all the planes get > disabled. > * > - * If a platform hasn't been transitioned to atomic watermarks > yet, > - * we'll continue to update watermarks the old way, if flags > tell > - * us to. > + * We do this after .initial_watermarks() so that we have a > + * chance of catching underruns with the intermediate > watermarks > + * vs. the old plane configuration. > */ > - if (dev_priv->display.initial_watermarks) > - dev_priv->display.initial_watermarks(state, crtc); > - else if (new_crtc_state->update_wm_pre) > - intel_update_watermarks(crtc); > + if (IS_GEN(dev_priv, 2) && planes_disabling(old_crtc_state, > new_crtc_state)) > + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, > false); > } > > static void intel_crtc_disable_planes(struct intel_atomic_state > *state, > @@ -14423,13 +14399,6 @@ static void > intel_old_crtc_state_disables(struct intel_atomic_state *state, > intel_fbc_disable(crtc); > intel_disable_shared_dpll(old_crtc_state); > > - /* > - * Underruns don't always raise interrupts, > - * so check manually. > - */ > - intel_check_cpu_fifo_underruns(dev_priv); > - intel_check_pch_fifo_underruns(dev_priv); > - > /* FIXME unify this for all platforms */ > if (!new_crtc_state->hw.active && > !HAS_GMCH(dev_priv) && > @@ -14882,7 +14851,19 @@ static void intel_atomic_commit_tail(struct > intel_atomic_state *state) > * > * TODO: Move this (and other cleanup) to an async worker > eventually. > */ > - for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, > i) { > + for_each_oldnew_intel_crtc_in_state(state, crtc, > old_crtc_state, > + new_crtc_state, i) { > + /* > + * Gen2 reports pipe underruns whenever all planes are > disabled. > + * So re-enable underrun reporting after some planes > get enabled. > + * > + * We do this before .optimize_watermarks() so that we > have a > + * chance of catching underruns with the intermediate > watermarks > + * vs. the new plane configuration. > + */ > + if (IS_GEN(dev_priv, 2) && > planes_enabling(old_crtc_state, new_crtc_state)) > + intel_set_cpu_fifo_underrun_reporting(dev_priv, > crtc->pipe, true); > + > if (dev_priv->display.optimize_watermarks) > dev_priv->display.optimize_watermarks(state, > crtc); > } > @@ -14896,6 +14877,10 @@ static void intel_atomic_commit_tail(struct > intel_atomic_state *state) > intel_modeset_verify_crtc(crtc, state, old_crtc_state, > new_crtc_state); > } > > + /* Underruns don't always raise interrupts, so check manually > */ > + intel_check_cpu_fifo_underruns(dev_priv); > + intel_check_pch_fifo_underruns(dev_priv); > + > if (state->modeset) > intel_verify_planes(state); >
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 72655b5b1365..5368f3ab70af 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -5908,37 +5908,6 @@ static void intel_crtc_dpms_overlay_disable(struct intel_crtc *intel_crtc) */ } -/** - * intel_post_enable_primary - Perform operations after enabling primary plane - * @crtc: the CRTC whose primary plane was just enabled - * @new_crtc_state: the enabling state - * - * Performs potentially sleeping operations that must be done after the primary - * plane is enabled, such as updating FBC and IPS. Note that this may be - * called due to an explicit primary plane update, or due to an implicit - * re-enable that is caused when a sprite plane is updated to no longer - * completely hide the primary plane. - */ -static void -intel_post_enable_primary(struct intel_crtc *crtc) -{ - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); - enum pipe pipe = crtc->pipe; - - /* - * Gen2 reports pipe underruns whenever all planes are disabled. - * So don't enable underrun reporting before at least some planes - * are enabled. - * FIXME: Need to fix the logic to work when we turn off all planes - * but leave the pipe running. - */ - if (IS_GEN(dev_priv, 2)) - intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); - - /* Underruns don't always raise interrupts, so check manually. */ - intel_check_cpu_fifo_underruns(dev_priv); - intel_check_pch_fifo_underruns(dev_priv); -} /* FIXME get rid of this and use pre_plane_update */ static void @@ -6059,6 +6028,20 @@ static bool needs_scalerclk_wa(const struct intel_crtc_state *crtc_state) return false; } +static bool planes_enabling(const struct intel_crtc_state *old_crtc_state, + const struct intel_crtc_state *new_crtc_state) +{ + return (!old_crtc_state->active_planes || needs_modeset(new_crtc_state)) && + new_crtc_state->active_planes; +} + +static bool planes_disabling(const struct intel_crtc_state *old_crtc_state, + const struct intel_crtc_state *new_crtc_state) +{ + return old_crtc_state->active_planes && + (!new_crtc_state->active_planes || needs_modeset(new_crtc_state)); +} + static void intel_post_plane_update(struct intel_atomic_state *state, struct intel_crtc *crtc) { @@ -6068,10 +6051,9 @@ static void intel_post_plane_update(struct intel_atomic_state *state, intel_atomic_get_old_crtc_state(state, crtc); const struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); - const struct intel_plane_state *old_primary_state = - intel_atomic_get_old_plane_state(state, primary); const struct intel_plane_state *new_primary_state = intel_atomic_get_new_plane_state(state, primary); + enum pipe pipe = crtc->pipe; intel_frontbuffer_flip(dev_priv, new_crtc_state->fb_bits); @@ -6081,22 +6063,16 @@ static void intel_post_plane_update(struct intel_atomic_state *state, if (hsw_post_update_enable_ips(old_crtc_state, new_crtc_state)) hsw_enable_ips(new_crtc_state); - if (new_primary_state) { + if (new_primary_state) intel_fbc_post_update(crtc); - if (new_primary_state->uapi.visible && - (needs_modeset(new_crtc_state) || - !old_primary_state->uapi.visible)) - intel_post_enable_primary(crtc); - } - if (needs_nv12_wa(old_crtc_state) && !needs_nv12_wa(new_crtc_state)) - skl_wa_827(dev_priv, crtc->pipe, false); + skl_wa_827(dev_priv, pipe, false); if (needs_scalerclk_wa(old_crtc_state) && !needs_scalerclk_wa(new_crtc_state)) - icl_wa_scalerclkgating(dev_priv, crtc->pipe, false); + icl_wa_scalerclkgating(dev_priv, pipe, false); } static void intel_pre_plane_update(struct intel_atomic_state *state, @@ -6108,35 +6084,25 @@ static void intel_pre_plane_update(struct intel_atomic_state *state, intel_atomic_get_old_crtc_state(state, crtc); const struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); - const struct intel_plane_state *old_primary_state = - intel_atomic_get_old_plane_state(state, primary); const struct intel_plane_state *new_primary_state = intel_atomic_get_new_plane_state(state, primary); - bool modeset = needs_modeset(new_crtc_state); + enum pipe pipe = crtc->pipe; if (hsw_pre_update_disable_ips(old_crtc_state, new_crtc_state)) hsw_disable_ips(old_crtc_state); - if (new_primary_state) { + if (new_primary_state) intel_fbc_pre_update(crtc, new_crtc_state, new_primary_state); - /* - * Gen2 reports pipe underruns whenever all planes are disabled. - * So disable underrun reporting before all the planes get disabled. - */ - if (IS_GEN(dev_priv, 2) && old_primary_state->uapi.visible && - (modeset || !new_primary_state->uapi.visible)) - intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false); - } /* Display WA 827 */ if (!needs_nv12_wa(old_crtc_state) && needs_nv12_wa(new_crtc_state)) - skl_wa_827(dev_priv, crtc->pipe, true); + skl_wa_827(dev_priv, pipe, true); /* Wa_2006604312:icl */ if (!needs_scalerclk_wa(old_crtc_state) && needs_scalerclk_wa(new_crtc_state)) - icl_wa_scalerclkgating(dev_priv, crtc->pipe, true); + icl_wa_scalerclkgating(dev_priv, pipe, true); /* * Vblank time updates from the shadow to live plane control register @@ -6149,7 +6115,7 @@ static void intel_pre_plane_update(struct intel_atomic_state *state, */ if (HAS_GMCH(dev_priv) && old_crtc_state->hw.active && new_crtc_state->disable_cxsr && intel_set_memory_cxsr(dev_priv, false)) - intel_wait_for_vblank(dev_priv, crtc->pipe); + intel_wait_for_vblank(dev_priv, pipe); /* * IVB workaround: must disable low power watermarks for at least @@ -6160,33 +6126,43 @@ static void intel_pre_plane_update(struct intel_atomic_state *state, */ if (old_crtc_state->hw.active && new_crtc_state->disable_lp_wm && ilk_disable_lp_wm(dev_priv)) - intel_wait_for_vblank(dev_priv, crtc->pipe); + intel_wait_for_vblank(dev_priv, pipe); /* - * If we're doing a modeset, we're done. No need to do any pre-vblank - * watermark programming here. + * If we're doing a modeset we don't need to do any + * pre-vblank watermark programming here. */ - if (needs_modeset(new_crtc_state)) - return; + if (!needs_modeset(new_crtc_state)) { + /* + * For platforms that support atomic watermarks, program the + * 'intermediate' watermarks immediately. On pre-gen9 platforms, these + * will be the intermediate values that are safe for both pre- and + * post- vblank; when vblank happens, the 'active' values will be set + * to the final 'target' values and we'll do this again to get the + * optimal watermarks. For gen9+ platforms, the values we program here + * will be the final target values which will get automatically latched + * at vblank time; no further programming will be necessary. + * + * If a platform hasn't been transitioned to atomic watermarks yet, + * we'll continue to update watermarks the old way, if flags tell + * us to. + */ + if (dev_priv->display.initial_watermarks) + dev_priv->display.initial_watermarks(state, crtc); + else if (new_crtc_state->update_wm_pre) + intel_update_watermarks(crtc); + } /* - * For platforms that support atomic watermarks, program the - * 'intermediate' watermarks immediately. On pre-gen9 platforms, these - * will be the intermediate values that are safe for both pre- and - * post- vblank; when vblank happens, the 'active' values will be set - * to the final 'target' values and we'll do this again to get the - * optimal watermarks. For gen9+ platforms, the values we program here - * will be the final target values which will get automatically latched - * at vblank time; no further programming will be necessary. + * Gen2 reports pipe underruns whenever all planes are disabled. + * So disable underrun reporting before all the planes get disabled. * - * If a platform hasn't been transitioned to atomic watermarks yet, - * we'll continue to update watermarks the old way, if flags tell - * us to. + * We do this after .initial_watermarks() so that we have a + * chance of catching underruns with the intermediate watermarks + * vs. the old plane configuration. */ - if (dev_priv->display.initial_watermarks) - dev_priv->display.initial_watermarks(state, crtc); - else if (new_crtc_state->update_wm_pre) - intel_update_watermarks(crtc); + if (IS_GEN(dev_priv, 2) && planes_disabling(old_crtc_state, new_crtc_state)) + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); } static void intel_crtc_disable_planes(struct intel_atomic_state *state, @@ -14423,13 +14399,6 @@ static void intel_old_crtc_state_disables(struct intel_atomic_state *state, intel_fbc_disable(crtc); intel_disable_shared_dpll(old_crtc_state); - /* - * Underruns don't always raise interrupts, - * so check manually. - */ - intel_check_cpu_fifo_underruns(dev_priv); - intel_check_pch_fifo_underruns(dev_priv); - /* FIXME unify this for all platforms */ if (!new_crtc_state->hw.active && !HAS_GMCH(dev_priv) && @@ -14882,7 +14851,19 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state) * * TODO: Move this (and other cleanup) to an async worker eventually. */ - for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, + new_crtc_state, i) { + /* + * Gen2 reports pipe underruns whenever all planes are disabled. + * So re-enable underrun reporting after some planes get enabled. + * + * We do this before .optimize_watermarks() so that we have a + * chance of catching underruns with the intermediate watermarks + * vs. the new plane configuration. + */ + if (IS_GEN(dev_priv, 2) && planes_enabling(old_crtc_state, new_crtc_state)) + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true); + if (dev_priv->display.optimize_watermarks) dev_priv->display.optimize_watermarks(state, crtc); } @@ -14896,6 +14877,10 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state) intel_modeset_verify_crtc(crtc, state, old_crtc_state, new_crtc_state); } + /* Underruns don't always raise interrupts, so check manually */ + intel_check_cpu_fifo_underruns(dev_priv); + intel_check_pch_fifo_underruns(dev_priv); + if (state->modeset) intel_verify_planes(state);