From ad457191015a197b3daf1a259ed8e7f70bf68bb9 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Jos=C3=A9=20Roberto=20de=20Souza?= Date: Thu, 5 Dec 2019 13:03:50 -0800 Subject: [PATCH] drm/i915/display: Refactor intel_commit_modeset_disables() MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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. v2: Not using crtc->active as it is deprecated v3: Removing is_trans_port_sync_mode() check, just check for is_trans_port_sync_master() is enough v4: Adding and using is_trans_port_sync_slave(), otherwise non-port sync pipes will be disabled in the first loop, what is not wrong but is not what patch description promises Cc: Lucas De Marchi Cc: Manasi Navare Cc: Matt Roper Cc: Maarten Lankhorst Cc: Ville Syrjälä Reviewed-by: Ville Syrjälä (v2) Signed-off-by: José Roberto de Souza Reviewed-by: Matt Roper Link: https://patchwork.freedesktop.org/patch/msgid/20191205210350.96795-3-jose.souza@intel.com --- drivers/gpu/drm/i915/display/intel_display.c | 88 +++++++------------- 1 file changed, 32 insertions(+), 56 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index cd8a1715c0c9..a35ec8e027c7 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -561,6 +561,12 @@ is_trans_port_sync_master(const struct intel_crtc_state *crtc_state) crtc_state->sync_mode_slaves_mask); } +static bool +is_trans_port_sync_slave(const struct intel_crtc_state *crtc_state) +{ + return crtc_state->master_transcoder != INVALID_TRANSCODER; +} + /* * Platform specific helpers to calculate the port PLL loopback- (clock.m), * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast @@ -14393,77 +14399,47 @@ 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(state, slave_crtc); - 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(state, crtc); - 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; + u32 handled = 0; 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) { + /* 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)) continue; + if (!old_crtc_state->hw.active) + continue; + /* In case of Transcoder port Sync master slave CRTCs can be * assigned in any order and we need to make sure that * slave CRTCs are disabled first and then master CRTC since * Slave vblanks are masked till Master Vblanks. */ - if (is_trans_port_sync_mode(old_crtc_state)) { - if (is_trans_port_sync_master(old_crtc_state)) - intel_trans_port_sync_modeset_disables(state, - crtc, - old_crtc_state, - new_crtc_state); - else - continue; - } else { - intel_pre_plane_update(state, crtc); + if (!is_trans_port_sync_slave(old_crtc_state)) + continue; - if (old_crtc_state->hw.active) - intel_old_crtc_state_disables(state, - old_crtc_state, - new_crtc_state, - crtc); - } + intel_pre_plane_update(state, crtc); + intel_old_crtc_state_disables(state, old_crtc_state, + new_crtc_state, crtc); + handled |= BIT(crtc->pipe); + } + + /* 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) || + (handled & BIT(crtc->pipe))) + continue; + + intel_pre_plane_update(state, crtc); + if (old_crtc_state->hw.active) + intel_old_crtc_state_disables(state, old_crtc_state, + new_crtc_state, crtc); } } -- 2.30.2