On Wed, 2020-01-15 at 21:08 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
> 
> The post-fastset "does anyone still need a full modeset?" for
> port sync looks busted. The outer loop bails out of a full modeset
> is still needed by the current crtc which, and then we skip forcing
> a full modeset on the related crtcs. That's totally the opposite
> of what we want.

Ops yeah, it is always doing a full modeset.

> 
> The MST path has the logic mostly the other way around so it
> looks correct. To fix the port sync case let's follow the MST
> logic for both. So, if the current crtc already needs a modeset
> we do nothing. otherwise we check if any of the related crtcs
> needs a modeset, and if so we force a full modeset for the
> current crtc.
> 
> And while at let's change the else if to a plain if to so
> we don't have needless coupling between the MST and port sync
> checks.
> 
> Cc: José Roberto de Souza <jose.so...@intel.com>
> Cc: Manasi Navare <manasi.d.nav...@intel.com>
> Fixes: 05a8e45136ca ("drm/i915/display: Use external dependency loop
> for port sync")
> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 43 ++++++++--------
> ----
>  1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index dd03987cc24f..b397816ce253 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14469,37 +14469,23 @@ static int intel_atomic_check_crtcs(struct
> intel_atomic_state *state)
>       return 0;
>  }
>  
> -static bool intel_cpu_transcoder_needs_modeset(struct
> intel_atomic_state *state,
> -                                            enum transcoder
> transcoder)
> +static bool intel_cpu_transcoders_need_modeset(struct
> intel_atomic_state *state,
> +                                            u8 transcoders)
>  {
> -     struct intel_crtc_state *new_crtc_state;
> +     const struct intel_crtc_state *new_crtc_state;
>       struct intel_crtc *crtc;
>       int i;
>  
> -     for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> i)
> -             if (new_crtc_state->cpu_transcoder == transcoder)
> -                     return needs_modeset(new_crtc_state);
> +     for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> i) {
> +             if (new_crtc_state->hw.enable &&

The intel_pipe_config_compare() will force a modeset for MST and port
sync but this hw.enable could skip modeset in future features when one
of the pipes are going from enabled to disabled.

Removing it:

Reviewed-by: José Roberto de Souza <jose.so...@intel.com>

> +                 transcoders & BIT(new_crtc_state->cpu_transcoder)
> &&
> +                 needs_modeset(new_crtc_state))
> +                     return true;
> +     }
>  
>       return false;
>  }
>  
> -static void
> -intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> -                        u8 transcoders)
> -{
> -     struct intel_crtc_state *new_crtc_state;
> -     struct intel_crtc *crtc;
> -     int i;
> -
> -     for_each_new_intel_crtc_in_state(state, crtc,
> -                                      new_crtc_state, i) {
> -             if (transcoders & BIT(new_crtc_state->cpu_transcoder))
> {
> -                     new_crtc_state->uapi.mode_changed = true;
> -                     new_crtc_state->update_pipe = false;
> -             }
> -     }
> -}
> -
>  static int
>  intel_modeset_all_tiles(struct intel_atomic_state *state, int
> tile_grp_id)
>  {
> @@ -14655,15 +14641,20 @@ static int intel_atomic_check(struct
> drm_device *dev,
>               if (intel_dp_mst_is_slave_trans(new_crtc_state)) {
>                       enum transcoder master = new_crtc_state-
> >mst_master_transcoder;
>  
> -                     if (intel_cpu_transcoder_needs_modeset(state,
> master)) {
> +                     if (intel_cpu_transcoders_need_modeset(state,
> BIT(master))) {
>                               new_crtc_state->uapi.mode_changed =
> true;
>                               new_crtc_state->update_pipe = false;
>                       }
> -             } else if (is_trans_port_sync_mode(new_crtc_state)) {
> +             }
> +
> +             if (is_trans_port_sync_mode(new_crtc_state)) {
>                       u8 trans = new_crtc_state-
> >sync_mode_slaves_mask |
>                                  BIT(new_crtc_state-
> >master_transcoder);
>  
> -                     intel_modeset_synced_crtcs(state, trans);
> +                     if (intel_cpu_transcoders_need_modeset(state,
> trans)) {
> +                             new_crtc_state->uapi.mode_changed =
> true;
> +                             new_crtc_state->update_pipe = false;
> +                     }
>               }
>       }
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to