On Fri, Nov 22, 2019 at 04:54:53PM -0800, José Roberto de Souza wrote:
> Commit 9c722e17c1b9 ("drm/i915: Disable pipes in reverse order")
> reverted the order that pipes gets disabled because of TGL
> master/slave relationship between transcoders in MST mode.
>
> But as stated in a comment in skl_commit_modeset_enables() the
> enabling order is not always crescent, possibly causing previously
> selected slave transcoder being enabled before master so another
> approach will be needed to select a transcoder to master in MST mode.
> It will be similar to the approach taken in port sync.
>
> But instead of implement something like
> intel_trans_port_sync_modeset_disables() to MST lets simply it and
> iterate over all pipes 2 times, the first one disabling any slave and
> then disabling everything else.
> The MST bits will be added in another patch.
>
> Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 79 ++++++--------------
> 1 file changed, 22 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 53dc310a5f6d..1b1fbb6d8acc 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14443,53 +14443,16 @@ static void intel_old_crtc_state_disables(struct intel_atomic_state *state,
> dev_priv->display.initial_watermarks(state, crtc);
> }
>
> -static void intel_trans_port_sync_modeset_disables(struct intel_atomic_state *state,
> - struct intel_crtc *crtc,
> - struct intel_crtc_state *old_crtc_state,
> - struct intel_crtc_state *new_crtc_state)
> -{
> - struct intel_crtc *slave_crtc = intel_get_slave_crtc(new_crtc_state);
> - struct intel_crtc_state *new_slave_crtc_state =
> - intel_atomic_get_new_crtc_state(state, slave_crtc);
> - struct intel_crtc_state *old_slave_crtc_state =
> - intel_atomic_get_old_crtc_state(state, slave_crtc);
> -
> - WARN_ON(!slave_crtc || !new_slave_crtc_state ||
> - !old_slave_crtc_state);
> -
> - /* Disable Slave first */
> - intel_pre_plane_update(old_slave_crtc_state, new_slave_crtc_state);
> - if (old_slave_crtc_state->hw.active)
> - intel_old_crtc_state_disables(state,
> - old_slave_crtc_state,
> - new_slave_crtc_state,
> - slave_crtc);
> -
> - /* Disable Master */
> - intel_pre_plane_update(old_crtc_state, new_crtc_state);
> - if (old_crtc_state->hw.active)
> - intel_old_crtc_state_disables(state,
> - old_crtc_state,
> - new_crtc_state,
> - crtc);
> -}
> -
> static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> {
> struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> struct intel_crtc *crtc;
> int i;
>
> - /*
> - * Disable CRTC/pipes in reverse order because some features(MST in
> - * TGL+) requires master and slave relationship between pipes, so it
> - * should always pick the lowest pipe as master as it will be enabled
> - * first and disable in the reverse order so the master will be the
> - * last one to be disabled.
> - */
> - for_each_oldnew_intel_crtc_in_state_reverse(state, crtc, old_crtc_state,
> - new_crtc_state, i) {
> - if (!needs_modeset(new_crtc_state))
> + /* Only disable port sync slaves */
> + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> + new_crtc_state, i) {
> + if (!needs_modeset(new_crtc_state) || !crtc->active)
What's the deal with these crtc->active checks?
> continue;
>
> /* In case of Transcoder port Sync master slave CRTCs can be
> @@ -14497,23 +14460,25 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> * slave CRTCs are disabled first and then master CRTC since
> * Slave vblanks are masked till Master Vblanks.
> */
> - if (is_trans_port_sync_mode(new_crtc_state)) {
> - if (is_trans_port_sync_master(new_crtc_state))
> - intel_trans_port_sync_modeset_disables(state,
> - crtc,
> - old_crtc_state,
> - new_crtc_state);
> - else
> - continue;
> - } else {
> - intel_pre_plane_update(old_crtc_state, new_crtc_state);
> + if (!is_trans_port_sync_mode(new_crtc_state))
> + continue;
> + if (is_trans_port_sync_master(new_crtc_state))
> + continue;
We don't have is_trans_sync_slave()?
>
> - if (old_crtc_state->hw.active)
> - intel_old_crtc_state_disables(state,
> - old_crtc_state,
> - new_crtc_state,
> - crtc);
> - }
> + intel_pre_plane_update(old_crtc_state, new_crtc_state);
> + intel_old_crtc_state_disables(state, old_crtc_state,
> + new_crtc_state, crtc);
> + }
> +
> + /* Disable everything else left on */
> + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> + new_crtc_state, i) {
> + if (!needs_modeset(new_crtc_state) || !crtc->active)
> + continue;
> +
> + intel_pre_plane_update(old_crtc_state, new_crtc_state);
> + intel_old_crtc_state_disables(state, old_crtc_state,
> + new_crtc_state, crtc);
Pondering if there's any chance of some odd fail if we have two ports
running in port sync mode. That will now lead to
disable_slave(0)->disable_slave(1)->disable_master(0)->disable_master(1)...
> }
> }
>
> --
> 2.24.0
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
