Thanks a lot Maarten for your review comments.
Somehow none of my email-clients caught Maarten's review on this, so capturing and
responding to his review comments from Patchwork
On Tue, Apr 23, 2019 at 08:49:01AM -0700, Manasi Navare wrote:
> 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.
>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 3 +-
> drivers/gpu/drm/i915/intel_display.c | 122 +++++++++++++++++++++++++++
> 2 files changed, 124 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 24f9106efcc6..0b326b2c274d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3120,7 +3120,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) &&
> + INTEL_GEN(dev_priv) < 11)
> intel_dp_stop_link_train(intel_dp);
[Maarten's comment]: && !master_crtc instead of gen check? Would be more clear what it 's for, and only changes link training in sync mode.
->It is gen based check because we want to call the intel_dp_stop_link_train() as is for older gens whereas for Gen 11 it gets called
separately in crtc_enable() hook for all crtcs (ganged and non ganged).
It was Ville's feedback that its better to call stop_dp_link_train() in crtc_enable() for all crtcs.
But now that I rethink, we shouldnt change the enable sequence for crtcs not in sync mode and only do this for crtcs
in sync mode.
So If we have to avoid calling it only for the trans_port_sync mode and CRTCs involved in this mode then we would need to add a check
&& !master_crtc && !trans_port_sync_slaves
Ville, Maarten, any thoughts?
>
> intel_ddi_enable_fec(encoder, crtc_state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4bd23e61c6fd..bd00549a9e00 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13488,6 +13488,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state)
> 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);
> @@ -13496,6 +13497,12 @@ static void skl_update_crtcs(struct drm_atomic_state *state)
> if (updated & cmask || !cstate->base.active)
> continue;
[Maarten's comment]: Only do this for synced crtc's?
-> I think this goes back to Ville's suggestion of following this for all CRTCs, but that means we deviate
from th bspec sequence for non synced crtcs too.
Should we do thsi only for synced crtcs? Again use the condition master_crtc || trans_port_sync_slave then do this?
>
> + if (modeset && INTEL_GEN(dev_priv) >= 11) {
> + DRM_DEBUG_KMS("Pushing the Tiled 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))
> @@ -13526,6 +13533,121 @@ static void skl_update_crtcs(struct drm_atomic_state *state)
> }
> } while (progress);
>
> + /* Separate loop for updating Slave CRTCs that need modeset.
> + * We need to loop through all slaves of tiled display first and
> + * follow enable sequence with DP_TP_CTL left Idle until the master
> + * is ready.
> + */
> + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> + bool modeset = needs_modeset(new_crtc_state);
> + struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
> +
> + intel_crtc = to_intel_crtc(crtc);
> +
> + if (!pipe_config->base.active)
> + continue;
> + if (!modeset)
> + continue;
> + if (!pipe_config->master_crtc)
> + continue;
> +
> + update_scanline_offset(pipe_config);
> + dev_priv->display.crtc_enable(pipe_config, state);
> + }
> + /* Now do the display enable sequence for master CRTC */
> + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> + bool modeset = needs_modeset(new_crtc_state);
> + struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
> +
> + intel_crtc = to_intel_crtc(crtc);
> +
> + if (!pipe_config->base.active)
> + continue;
> + if (!modeset)
> + continue;
> + if (pipe_config->master_crtc)
> + continue;
> +
> + update_scanline_offset(pipe_config);
> + dev_priv->display.crtc_enable(pipe_config, state);
> + }
> + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> + struct drm_connector_state *conn_state;
> + struct drm_connector *conn;
> + bool modeset = needs_modeset(new_crtc_state);
> + struct intel_dp *intel_dp;
> +
> + intel_crtc = to_intel_crtc(crtc);
> + cstate = to_intel_crtc_state(new_crtc_state);
> +
> + if (!cstate->base.active)
> + continue;
> + if (!modeset)
> + continue;
> + if (!cstate->master_crtc)
> + continue;
> +
> + for_each_new_connector_in_state(state, conn, conn_state, i) {
> + if (conn_state->crtc == crtc)
> + break;
> + }
> + intel_dp = enc_to_intel_dp(&intel_attached_encoder(conn)->base);
> + intel_dp_stop_link_train(intel_dp);
> + }
[Maarten's comment]: Why is this a separate pass?
-> This is a separate pass because for the synced mode we cannot call stop_link_train() which
sets DP_TP_CTL to Normal on master before the slave, so we call stop_link_train() for all slaves first
and then for the master.
> + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> + struct drm_connector_state *conn_state;
> + struct drm_connector *conn;
> + bool modeset = needs_modeset(new_crtc_state);
> + struct intel_dp *intel_dp;
> +
> + intel_crtc = to_intel_crtc(crtc);
> + cstate = to_intel_crtc_state(new_crtc_state);
> +
> + if (!cstate->base.active)
> + continue;
> + if (!modeset)
> + continue;
> + if (cstate->master_crtc)
> + continue;
> +
> + /* Wait for 200us before setting the master DP_TP_TCL to Normal */
> + usleep_range(200, 400);
> + for_each_new_connector_in_state(state, conn, conn_state, i) {
> + if (conn_state->crtc == crtc)
> + break;
> + }
> + intel_dp = enc_to_intel_dp(&intel_attached_encoder(conn)->base);
> + intel_dp_stop_link_train(intel_dp);
> + }
> + /* Now do the post crtc enable for all master and slaves */
> + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> + bool modeset = needs_modeset(new_crtc_state);
> + struct intel_plane_state *new_plane_state;
> + struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
> +
> + intel_crtc = to_intel_crtc(crtc);
> +
> + if (!pipe_config->base.active)
> + continue;
> + if (!modeset)
> + continue;
> +
> + 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);
> + if (INTEL_GEN(dev_priv) >= 9)
> + skl_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
> + else
> + i9xx_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
[Maarten's comments]: This is called from skl_update_crtcs, don't need the <gen9 fallback code.
-> Yup, will remove the fallback code
> +
> + intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
> + }
> /* If 2nd DBuf slice is no more required disable it */
> if (INTEL_GEN(dev_priv) >= 11 && required_slices < hw_enabled_slices)
> icl_dbuf_slices_update(dev_priv, required_slices);
[Maarten's comment]: Better yet, make an icl_update_crtcs that handles the sync mode, and remove all the gen11 stuff from skl_update_crtcs.
~Maarten
-> Yes that makes sense, I will create a new hook for icl_update_crtcs() and add all trans port sync code there then we can get rid
of the GEN checks.
Now the only open is if we should split into 4 loops for all crtcs that need modeset or should we do that only for
crtcs in the sync mode.
Manasi
> --
> 2.19.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
