[Intel-gfx] [PATCH 1/7] drm/i915/display: Refactor intel_commit_modeset_disables()
Souza, Jose
jose.souza at intel.com
Tue Nov 26 23:03:10 UTC 2019
On Tue, 2019-11-26 at 14:49 -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 at intel.com>
> > > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > ---
> > > > 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_st
> > > > ate,
> > > > - new_slave_crtc_st
> > > > ate,
> > > > - 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_st
> > > > ate, 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.
>
> 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.
>
Thanks for the headsup, just used it because was easiest option.
Will fix it after get more comments in the other patches.
>
> 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_d
> > > > isables(
> > > > 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(s
> > > > tate,
> > > > - o
> > > > ld_crtc_
> > > > state,
> > > > - n
> > > > ew_crtc_
> > > > state,
> > > > - c
> > > > rtc);
> > > > - }
> > > > + 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
More information about the Intel-gfx
mailing list