On Tue, Nov 26, 2019 at 02:49:47PM -0800, Matt Roper wrote:
On Tue, Nov 26, 2019 at 02:03:08PM -0800, Souza, Jose wrote:On Tue, 2019-11-26 at 21:40 +0200, Ville Syrjälä wrote: > 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? With just one loop we were using "old_crtc_state->hw.active" but as we should not modify the computed state in this phase and intel_old_crtc_state_disables() sets crtc->active = false, using it instead.I don't think we want to be using intel_crtc->active for anything new if we can help it. We have to keep that field around right now to support some of our ancient platforms that still don't have atomic support yet, but we don't want to be using it anywhere new we don't already have to for legacy reasons.
humm, good to know. Maybe this deserves a comment like this in struct intel_crtc.active?
You're only looking at the active flag here, not modifying it, so it should be fine to use old_crtc_state->active for the pre-modeset status of the CRTC (and new_crtc_state->active anywhere you need the post-modeset status). If you're just trying to keep track of which ones you've already disabled in this function when you come back around for your second loop, it would be better to just maintain a bitmask of CRTCs you turned off in the first pass as a local variable in this function so you know what to skip on the second pass.
yep, that's a great suggestion. Lucas De Marchi
Matt> > > 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()? We don't. > > > > > - 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)... Thoughts Manasi? > > > } > > } > > > > -- > > 2.24.0-- Matt Roper Graphics Software Engineer VTT-OSGC Platform Enablement Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
