[Intel-gfx] [PATCH v3 6/8] drm/i915/display/icl: Enable master-slaves in trans port sync mode in correct order
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Tue Jul 30 10:53:30 UTC 2019
Op 24-06-2019 om 23:08 schreef Manasi Navare:
> As per the display enable sequence, we need to follow the enable sequence
> for slaves first with DP_TP_CTL set to Idle and configure the transcoder
> port sync register to select the corersponding master, then follow the
> enable sequence for master leaving DP_TP_CTL to idle.
> At this point the transcoder port sync mode is configured and enabled
> and the Vblanks of both ports are synchronized so then set DP_TP_CTL
> for the slave and master to Normal and do post crtc enable updates.
>
> v2:
> * Create a icl_update_crtcs hook (Maarten, Danvet)
> * This sequence only for CRTCs in trans port sync mode (Maarten)
>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Matt Roper <matthew.d.roper at intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_ddi.c | 3 +-
> drivers/gpu/drm/i915/display/intel_display.c | 217 ++++++++++++++++++-
> drivers/gpu/drm/i915/display/intel_display.h | 4 +
> 3 files changed, 221 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 7925a176f900..bceb7e4b1877 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3154,7 +3154,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> true);
> intel_dp_sink_set_fec_ready(intel_dp, crtc_state);
> intel_dp_start_link_train(intel_dp);
> - if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> + if ((port != PORT_A || INTEL_GEN(dev_priv) >= 9) &&
> + !is_trans_port_sync_mode(crtc_state))
> intel_dp_stop_link_train(intel_dp);
>
> intel_ddi_enable_fec(encoder, crtc_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 7156b1b4c6c5..f88d3a929e36 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -520,6 +520,26 @@ needs_modeset(const struct drm_crtc_state *state)
> return drm_atomic_crtc_needs_modeset(state);
> }
>
> +bool
> +is_trans_port_sync_mode(const struct intel_crtc_state *state)
> +{
> + return (state->master_transcoder != INVALID_TRANSCODER ||
> + state->sync_mode_slaves_mask);
> +}
> +
> +static bool
> +is_trans_port_sync_slave(const struct intel_crtc_state *state)
> +{
> + return state->master_transcoder != INVALID_TRANSCODER;
> +}
> +
> +static bool
> +is_trans_port_sync_master(const struct intel_crtc_state *state)
> +{
> + return (state->master_transcoder == INVALID_TRANSCODER &&
> + state->sync_mode_slaves_mask);
> +}
> +
> /*
> * Platform specific helpers to calculate the port PLL loopback- (clock.m),
> * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
> @@ -13944,9 +13964,200 @@ static void skl_commit_modeset_enables(struct drm_atomic_state *state)
> progress = true;
> }
> } while (progress);
> +}
>
> +static void icl_commit_modeset_enables(struct drm_atomic_state *state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(state->dev);
> + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> + struct drm_crtc *crtc;
> + struct intel_crtc *intel_crtc;
> + struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> + struct intel_crtc_state *cstate;
> + unsigned int updated = 0;
> + bool progress;
> + enum pipe pipe;
> + int i;
> + u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
> + u8 required_slices = intel_state->wm_results.ddb.enabled_slices;
> + struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
Add old_entries as well, merge master + slave
> +
> + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i)
> + /* ignore allocations for crtc's that have been turned off. */
> + if (new_crtc_state->active)
> + entries[i] = to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
Can be changed to: if (new_crtc_state->active && !needs_modeset(new_crtc_state)) ?
Small refinement to the algorithm in general, I dislike the current handling.
What I would do:
- Make a new_entries array as well, that contains (for the master crtc, during modeset only) master_ddb.begin + slave_ddb.end,
and nothing for the slave ddb in that case, the ddb allocation for all other cases including master/slave non-modeset should be the default wm.skl.ddb.
Use it for comparing instead of the one from crtc_state. This way we know we can always enable both at the same time.
- Ignore the slave crtc when needs_modeset() is true, called from master instead.
- If a modeset happens on master crtc, do the special enabling dance on both in a separate function.
- Also ensure that the slave crtc is marked as updated, and update both entries to correct values in the entries again.
You should be able to get the slave crtc with intel_get_crtc_for_pipe(dev_priv, intel_crtc->pipe + 1);
With these changes you should be able to get rid of the icl special handling, it's confined to a single function for enabling,
only called when needs_modeset() && is_trans_port_sync_master is true, without the problem of overlapping allocations.
> + /* If 2nd DBuf slice required, enable it here */
> + if (required_slices > hw_enabled_slices)
> + icl_dbuf_slices_update(dev_priv, required_slices);
> +
> + /*
> + * Whenever the number of active pipes changes, we need to make sure we
> + * update the pipes in the right order so that their ddb allocations
> + * never overlap with eachother inbetween CRTC updates. Otherwise we'll
> + * cause pipe underruns and other bad stuff.
> + */
> + do {
> + progress = false;
> +
> + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> + bool vbl_wait = false;
> + unsigned int cmask = drm_crtc_mask(crtc);
> + bool modeset = needs_modeset(new_crtc_state);
> +
> + intel_crtc = to_intel_crtc(crtc);
> + cstate = to_intel_crtc_state(new_crtc_state);
> + pipe = intel_crtc->pipe;
> +
> + if (updated & cmask || !cstate->base.active)
> + continue;
> +
> + if (modeset && is_trans_port_sync_mode(cstate)) {
> + DRM_DEBUG_KMS("Pushing the Sync Mode CRTC %d %s that needs update to separate loop\n",
> + intel_crtc->base.base.id, intel_crtc->base.name);
> + continue;
> + }
> +
> + if (skl_ddb_allocation_overlaps(&cstate->wm.skl.ddb,
> + entries,
> + INTEL_INFO(dev_priv)->num_pipes, i))
> + continue;
> +
> + updated |= cmask;
> + entries[i] = cstate->wm.skl.ddb;
> +
> + /*
> + * If this is an already active pipe, it's DDB changed,
> + * and this isn't the last pipe that needs updating
> + * then we need to wait for a vblank to pass for the
> + * new ddb allocation to take effect.
> + */
> + if (!skl_ddb_entry_equal(&cstate->wm.skl.ddb,
> + &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb) &&
> + !new_crtc_state->active_changed &&
> + intel_state->wm_results.dirty_pipes != updated)
> + vbl_wait = true;
> +
> + intel_update_crtc(crtc, state, old_crtc_state,
> + new_crtc_state);
> +
> + if (vbl_wait)
> + intel_wait_for_vblank(dev_priv, pipe);
> +
> + progress = true;
> + }
> + } while (progress);
> + /* Set Slave's DP_TP_CTL to Normal */
> + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> + int j;
> + struct drm_connector_state *conn_state;
> + struct drm_connector *conn;
> + bool modeset = needs_modeset(new_crtc_state);
> + struct intel_dp *intel_dp;
> + struct intel_crtc_state *pipe_config =
> + to_intel_crtc_state(new_crtc_state);
> +
> + intel_crtc = to_intel_crtc(crtc);
> +
> + if (!pipe_config->base.active || !modeset ||
> + !is_trans_port_sync_slave(pipe_config))
> + continue;
> +
> + for_each_new_connector_in_state(state, conn, conn_state, j) {
> + if (conn_state->crtc == crtc)
> + break;
> + }
> + intel_dp = enc_to_intel_dp(&intel_attached_encoder(conn)->base);
enc_to_intel_dp(conn_state->best_encoder). :)
Make a small helper here, stop_link_training_for_crtc, call it for slave_crtc, then usleep, then master_crtc in the function described above?
> +
> + new_plane_state =
> + intel_atomic_get_new_plane_state(to_intel_atomic_state(state),
> + to_intel_plane(crtc->primary));
> + if (pipe_config->update_pipe && !pipe_config->enable_fbc)
> + intel_fbc_disable(intel_crtc);
> + else if (new_plane_state)
> + intel_fbc_enable(intel_crtc, pipe_config, new_plane_state);
> +
> + intel_begin_crtc_commit(to_intel_atomic_state(state), intel_crtc);
> + skl_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
> + intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
> + }
intel_commit_planes()
More information about the Intel-gfx
mailing list