[Intel-gfx] [PATCH 1/7] drm/i915/display: Refactor intel_commit_modeset_disables()
Matt Roper
matthew.d.roper at intel.com
Tue Nov 26 22:49:47 UTC 2019
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_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.
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.
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
More information about the Intel-gfx
mailing list