| Message ID | 20190925145901.3943-4-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915: Prepare to split crtc state in uapi and hw state | expand |
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.lankhorst@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
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.lankhorst@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. 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
Op 25-09-2019 om 23:42 schreef Matt Roper: > 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.lankhorst@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. Plus > that will let us just do a atomic_update_watermarks just sets linetime on skl+. :) Per plane watermarks are done in skl_write_plane_wm, called from skl_program_plane. > > /* > * 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. Yeah can drop even more with a check for update_pipe. > >> +} >> + >> 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
Op 25-09-2019 om 23:42 schreef Matt Roper: > 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.lankhorst@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. Yeah, this line probably predates atomic, with the old fastboot hack in the setcrtc call. Good to get rid of it. :) >> + >> + 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. 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
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.lankhorst@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
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); +} + 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
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.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 197 ++++++++----------- 1 file changed, 86 insertions(+), 111 deletions(-)