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.demar...@intel.com>
> Cc: Manasi Navare <manasi.d.nav...@intel.com>
> Cc: Matt Roper <matthew.d.ro...@intel.com>
> Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrj...@linux.intel.com> (v2)
> Signed-off-by: José Roberto de Souza <jose.so...@intel.com>
> ---
>  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?


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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to