Re: [Intel-gfx] [PATCH v2 3/3] drm/i915/display: Refactor intel_commit_modeset_disables()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Dec 04, 2019 at 12:55:10PM -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
> 
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 86 +++++++-------------
>  1 file changed, 30 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..a9f5aaf8df9b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14393,77 +14393,51 @@ 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 it wasn't active no need to check the special cases */

This comment seems a bit redundant.

> +		if (!old_crtc_state->hw.active)
> +			continue;
> +
>  		/* 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_mode(old_crtc_state))
> +			continue;
>  
> -			if (old_crtc_state->hw.active)
> -				intel_old_crtc_state_disables(state,
> -							      old_crtc_state,
> -							      new_crtc_state,
> -							      crtc);
> -		}
> +		if (is_trans_port_sync_master(old_crtc_state))
> +			continue;

I'd still like to see is_trans_port_sync_slave() so we could eliminate
the double check here.

> +
> +		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);
>  	}

The only concern left is the new ordering when we have multiple
sets of genlocked pipes. Assuming the hardware is sane I don't
think anything *should* break. But who knows.

Apart from that:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
> -- 
> 2.24.0

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx