On Wed, Sep 25, 2019 at 04:59:01PM +0200, Maarten Lankhorst wrote: > This can all be done from the intel_update_crtc function. Split out the > pipe update into a separate function, just like is done for the planes. > Pull in all the changes done during fastset as well. It makes no sense > for it to still exist as a separate function. > > Changes since v1: > - Inline intel_update_pipe_config() > > Signed-off-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 197 ++++++++----------- > 1 file changed, 86 insertions(+), 111 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index b77574cda648..5a9d06af9f29 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -136,8 +136,6 @@ static void vlv_prepare_pll(struct intel_crtc *crtc, > const struct intel_crtc_state *pipe_config); > static void chv_prepare_pll(struct intel_crtc *crtc, > const struct intel_crtc_state *pipe_config); > -static void intel_begin_crtc_commit(struct intel_atomic_state *, struct > intel_crtc *); > -static void intel_finish_crtc_commit(struct intel_atomic_state *, struct > intel_crtc *); > static void intel_crtc_init_scalers(struct intel_crtc *crtc, > struct intel_crtc_state *crtc_state); > static void skylake_pfit_enable(const struct intel_crtc_state *crtc_state); > @@ -4409,45 +4407,6 @@ static void icl_set_pipe_chicken(struct intel_crtc > *crtc) > I915_WRITE(PIPE_CHICKEN(pipe), tmp); > } > > -static void intel_update_pipe_config(const struct intel_crtc_state > *old_crtc_state, > - const struct intel_crtc_state > *new_crtc_state) > -{ > - struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc); > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > - > - /* drm_atomic_helper_update_legacy_modeset_state might not be called. */ > - crtc->base.mode = new_crtc_state->uapi.mode; > - > - /* > - * Update pipe size and adjust fitter if needed: the reason for this is > - * that in compute_mode_changes we check the native mode (not the pfit > - * mode) to see if we can flip rather than do a full mode set. In the > - * fastboot case, we'll flip, but if we don't update the pipesrc and > - * pfit state, we'll end up with a big fb scanned out into the wrong > - * sized surface. > - */ > - > - I915_WRITE(PIPESRC(crtc->pipe), > - ((new_crtc_state->pipe_src_w - 1) << 16) | > - (new_crtc_state->pipe_src_h - 1)); > - > - /* on skylake this is done by detaching scalers */ > - if (INTEL_GEN(dev_priv) >= 9) { > - skl_detach_scalers(new_crtc_state); > - > - if (new_crtc_state->pch_pfit.enabled) > - skylake_pfit_enable(new_crtc_state); > - } else if (HAS_PCH_SPLIT(dev_priv)) { > - if (new_crtc_state->pch_pfit.enabled) > - ironlake_pfit_enable(new_crtc_state); > - else if (old_crtc_state->pch_pfit.enabled) > - ironlake_pfit_disable(old_crtc_state); > - } > - > - if (INTEL_GEN(dev_priv) >= 11) > - icl_set_pipe_chicken(crtc); > -} > - > static void intel_fdi_normal_train(struct intel_crtc *crtc) > { > struct drm_device *dev = crtc->base.dev; > @@ -13739,13 +13698,88 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc > *crtc) > return crtc->base.funcs->get_vblank_counter(&crtc->base); > } > > +void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc, > + struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + > + if (!IS_GEN(dev_priv, 2)) > + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, > true); > + > + if (crtc_state->has_pch_encoder) { > + enum pipe pch_transcoder = > + intel_crtc_pch_transcoder(crtc); > + > + intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, > true); > + } > +} > + > +static void commit_pipe_config(struct intel_atomic_state *state, > + struct intel_crtc_state *old_crtc_state, > + struct intel_crtc_state *new_crtc_state) > +{ > + struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc); > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + bool modeset = needs_modeset(new_crtc_state); > + > + if (!modeset) { > + if (new_crtc_state->uapi.color_mgmt_changed || > + new_crtc_state->update_pipe) > + intel_color_commit(new_crtc_state); > + > + if (INTEL_GEN(dev_priv) >= 9) > + skl_detach_scalers(new_crtc_state); > + > + if (new_crtc_state->update_pipe) { > + /* drm_atomic_helper_update_legacy_modeset_state might > not be called. */ > + crtc->base.mode = new_crtc_state->uapi.mode; > + > + /* > + * Update pipe size and adjust fitter if needed: the > reason for this is > + * that in compute_mode_changes we check the native > mode (not the pfit > + * mode) to see if we can flip rather than do a full > mode set. In the > + * fastboot case, we'll flip, but if we don't update > the pipesrc and > + * pfit state, we'll end up with a big fb scanned out > into the wrong > + * sized surface. > + */ > + > + I915_WRITE(PIPESRC(crtc->pipe), > + ((new_crtc_state->pipe_src_w - 1) << 16) | > + (new_crtc_state->pipe_src_h - 1)); > + > + if (INTEL_GEN(dev_priv) >= 9) { > + /* > + * enable pipe scaler here if needed, if > + * it's disabled, that's done by > + * skl_detach_scalers() above. > + */ > + skylake_pfit_enable(new_crtc_state); > + } else if (HAS_PCH_SPLIT(dev_priv)) { > + if (!new_crtc_state->pch_pfit.enabled) > + ironlake_pfit_disable(old_crtc_state); > + else > + ironlake_pfit_enable(new_crtc_state); > + } > + > + if (INTEL_GEN(dev_priv) >= 11) > + icl_set_pipe_chicken(crtc); > + } > + > + if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) > + bdw_set_pipemisc(new_crtc_state); > + } > + > + if (dev_priv->display.atomic_update_watermarks) > + dev_priv->display.atomic_update_watermarks(state, > + new_crtc_state);
Feels like bunch of pointless nesting. We could just do if (!modeset) commit_pipe_thing(); if (wm) update_wm(); in the caller, or something. > +} > + > static void intel_update_crtc(struct intel_crtc *crtc, > struct intel_atomic_state *state, > struct intel_crtc_state *old_crtc_state, > struct intel_crtc_state *new_crtc_state) > { > - struct drm_device *dev = state->base.dev; > - struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > bool modeset = needs_modeset(new_crtc_state); > struct intel_plane_state *new_plane_state = > intel_atomic_get_new_plane_state(state, > @@ -13769,14 +13803,21 @@ static void intel_update_crtc(struct intel_crtc > *crtc, > else if (new_plane_state) > intel_fbc_enable(crtc, new_crtc_state, new_plane_state); > > - intel_begin_crtc_commit(state, crtc); > + /* Perform vblank evasion around commit operation */ > + intel_pipe_update_start(new_crtc_state); > + > + commit_pipe_config(state, old_crtc_state, new_crtc_state); > > if (INTEL_GEN(dev_priv) >= 9) > skl_update_planes_on_crtc(state, crtc); > else > i9xx_update_planes_on_crtc(state, crtc); > > - intel_finish_crtc_commit(state, crtc); > + intel_pipe_update_end(new_crtc_state); > + > + if (new_crtc_state->update_pipe && !modeset && > + old_crtc_state->hw.mode.private_flags & I915_MODE_FLAG_INHERITED) > + intel_crtc_arm_fifo_underrun(crtc, new_crtc_state); > } > > static void intel_old_crtc_state_disables(struct intel_atomic_state *state, > @@ -14573,72 +14614,6 @@ skl_max_scale(const struct intel_crtc_state > *crtc_state, > return max_scale; > } > > -static void intel_begin_crtc_commit(struct intel_atomic_state *state, > - struct intel_crtc *crtc) > -{ > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > - struct intel_crtc_state *old_crtc_state = > - intel_atomic_get_old_crtc_state(state, crtc); > - struct intel_crtc_state *new_crtc_state = > - intel_atomic_get_new_crtc_state(state, crtc); > - bool modeset = needs_modeset(new_crtc_state); > - > - /* Perform vblank evasion around commit operation */ > - intel_pipe_update_start(new_crtc_state); > - > - if (modeset) > - goto out; > - > - if (new_crtc_state->uapi.color_mgmt_changed || > - new_crtc_state->update_pipe) > - intel_color_commit(new_crtc_state); > - > - if (new_crtc_state->update_pipe) > - intel_update_pipe_config(old_crtc_state, new_crtc_state); > - else if (INTEL_GEN(dev_priv) >= 9) > - skl_detach_scalers(new_crtc_state); > - > - if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) > - bdw_set_pipemisc(new_crtc_state); > - > -out: > - if (dev_priv->display.atomic_update_watermarks) > - dev_priv->display.atomic_update_watermarks(state, > - new_crtc_state); > -} > - > -void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc, > - struct intel_crtc_state *crtc_state) > -{ > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > - > - if (!IS_GEN(dev_priv, 2)) > - intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, > true); > - > - if (crtc_state->has_pch_encoder) { > - enum pipe pch_transcoder = > - intel_crtc_pch_transcoder(crtc); > - > - intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, > true); > - } > -} > - > -static void intel_finish_crtc_commit(struct intel_atomic_state *state, > - struct intel_crtc *crtc) > -{ > - struct intel_crtc_state *old_crtc_state = > - intel_atomic_get_old_crtc_state(state, crtc); > - struct intel_crtc_state *new_crtc_state = > - intel_atomic_get_new_crtc_state(state, crtc); > - > - intel_pipe_update_end(new_crtc_state); > - > - if (new_crtc_state->update_pipe && > - !needs_modeset(new_crtc_state) && > - old_crtc_state->hw.mode.private_flags & I915_MODE_FLAG_INHERITED) > - intel_crtc_arm_fifo_underrun(crtc, new_crtc_state); > -} > - > /** > * intel_plane_destroy - destroy a plane > * @plane: plane to destroy > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx