| Message ID | 20191205210350.96795-3-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/3] drm/i915/display: Do not check for the ddb allocations of turned off pipes | expand |
On Thu, Dec 05, 2019 at 01:03:50PM -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. > > 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 <lucas.demarchi@intel.com> > Cc: Manasi Navare <manasi.d.navare@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> (v2) > Signed-off-by: José Roberto de Souza <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 68575457d40e..821ba8053f9d 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; Is it intentional that we skip the intel_pre_plane_update() in this case for the slaves, but not the masters? Should this test be moved farther down below the pre_plane_update call? Matt > + > /* 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.24.0 >
On Fri, 2019-12-06 at 14:22 -0800, Matt Roper wrote: > On Thu, Dec 05, 2019 at 01:03:50PM -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. > > > > 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 <lucas.demarchi@intel.com> > > Cc: Manasi Navare <manasi.d.navare@intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> (v2) > > Signed-off-by: José Roberto de Souza <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 68575457d40e..821ba8053f9d 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; > > Is it intentional that we skip the intel_pre_plane_update() in this > case > for the slaves, but not the masters? Should this test be moved > farther > down below the pre_plane_update call? If it was inative it is not a port sync slave, for the pipes going from disabled to enabled in port sync slave mode the pre_plane_update() will be executed in the second loop. This is more of an optimization to not even check if is port sync slave and in future if is MST slave. I had a comment here but Ville asked to remove it. https://patchwork.freedesktop.org/patch/343736/?series=70462&rev=2 > > > Matt > > > + > > /* 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.24.0 > >
On Fri, Dec 06, 2019 at 02:35:38PM -0800, Souza, Jose wrote: > On Fri, 2019-12-06 at 14:22 -0800, Matt Roper wrote: > > On Thu, Dec 05, 2019 at 01:03:50PM -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. > > > > > > 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 <lucas.demarchi@intel.com> > > > Cc: Manasi Navare <manasi.d.navare@intel.com> > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> (v2) > > > Signed-off-by: José Roberto de Souza <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 68575457d40e..821ba8053f9d 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; > > > > Is it intentional that we skip the intel_pre_plane_update() in this > > case > > for the slaves, but not the masters? Should this test be moved > > farther > > down below the pre_plane_update call? > > If it was inative it is not a port sync slave, for the pipes going from > disabled to enabled in port sync slave mode the pre_plane_update() will > be executed in the second loop. > > This is more of an optimization to not even check if is port sync slave > and in future if is MST slave. > > I had a comment here but Ville asked to remove it. > https://patchwork.freedesktop.org/patch/343736/?series=70462&rev=2 Makes sense, thanks for clarifying. Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > > > > Matt > > > > > + > > > /* 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.24.0 > > >
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 68575457d40e..821ba8053f9d 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); } }
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 <lucas.demarchi@intel.com> Cc: Manasi Navare <manasi.d.navare@intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> (v2) Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 88 +++++++------------- 1 file changed, 32 insertions(+), 56 deletions(-)