On Wed, Sep 25, 2019 at 02:42:12PM -0700, Matt Roper wrote: > 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)); > > intel_set_pipe_src_size(new_crtc_state); > > to be more future-proof in case the register changes in the future. > > > + > > + 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); > > It might make sense to move the watermark update out to the caller. The > watermark registers themselves are plane-based, so dealing with them in > the commit_pipe_config function is already slightly non-intuitive.
This one actually just does the linetime wm on skl+. We should probably just move that into some other place so we could get rid of this hook on those platforms. > Plus > that will let us just do a > > /* > * During modesets pipe configuration was programmed as the > * CRTC was enabled. > */ > if (!modeset) > return; > > and drop a level of nesting for the rest of this function. > > > > +} > > + > > 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); > > It might be worth adding a comment to this block. I always have to go > dig back through git history to remind myself we this is needed. Maybe > something like: > > /* > * We usually enable FIFO underrun interrupts as part of the > * CRTC enable sequence during modesets. But when we inherit a > * valid pipe configuration from the BIOS we need to take care > * of enabling them on the CRTC's first fastset. > */ > > > Matt > > > } > > > > 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 > > -- > Matt Roper > Graphics Software Engineer > VTT-OSGC Platform Enablement > Intel Corporation > (916) 356-2795 > _______________________________________________ > 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