On Thu, 2019-12-05 at 12:38 +0200, Ville Syrjälä wrote:
> On Wed, Dec 04, 2019 at 12:55:10PM -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
> >
> > 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>
> > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 86 +++++++---------
> > ----
> > 1 file changed, 30 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..a9f5aaf8df9b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14393,77 +14393,51 @@ 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 it wasn't active no need to check the special
> > cases */
>
> This comment seems a bit redundant.
Okay, removing it.
>
> > + 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_mode(old_crtc_state))
> > + continue;
> >
> > - if (old_crtc_state->hw.active)
> > - intel_old_crtc_state_disables(state,
> > - old_crtc_
> > state,
> > - new_crtc_
> > state,
> > - crtc);
> > - }
> > + if (is_trans_port_sync_master(old_crtc_state))
> > + continue;
>
> I'd still like to see is_trans_port_sync_slave() so we could
> eliminate
> the double check here.
Thinking again, we only need
if (is_trans_port_sync_master(old_crtc_state))
continue;
here, no is_trans_port_sync_master() required.
Adding the is_trans_port_sync_slave() would leave use with
if (!is_trans_port_sync_master(old_crtc_state))
continue;
or with a indented block.
Fixing it...
>
> > +
> > + 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);
> > }
>
> The only concern left is the new ordering when we have multiple
> sets of genlocked pipes. Assuming the hardware is sane I don't
> think anything *should* break. But who knows.
>
> Apart from that:
> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
Thanks, will send a new version with the small changes above and keep
your rvb.
> > }
> >
> > --
> > 2.24.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
