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@xxxxxxxxx>
> > > 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>
> > > Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> (v2)
> > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxx>
>
> >
> >
> > 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
> > >
--
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
