[Intel-gfx] [PATCH 1/5] drm/i915: Fix post-fastset modeset check for port sync

[Souza, Jose]

On Wed, 2020-01-15 at 21:08 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala at 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.souza at intel.com>
> Cc: Manasi Navare <manasi.d.navare at intel.com>
> Fixes: 05a8e45136ca ("drm/i915/display: Use external dependency loop
> for port sync")
> Signed-off-by: Ville Syrjälä <ville.syrjala at 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.souza at 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;
> +			}
>  		}
>  	}
>