[Intel-gfx] [PATCH 1/7] drm/i915/display: Refactor intel_commit_modeset_disables()
Lucas De Marchi
lucas.demarchi at intel.com
Wed Nov 27 18:49:18 UTC 2019
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 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.
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 at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list