Re: [Intel-gfx] [PATCH 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present

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

On Wed, Dec 11, 2019 at 01:14:24PM -0800, Manasi Navare wrote:
> Add an extra check before making master slave assignments for tiled
> displays to make sure we make these assignments only if all tiled
> connectors are present. If not then initialize the state to defaults
> so it does a normal non tiled modeset without transcoder port sync.
> 
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 28 ++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 7263eaa66cda..c0a2dab3fe67 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12048,6 +12048,12 @@ static bool c8_planes_changed(const struct intel_crtc_state *new_crtc_state)
>  	return !old_crtc_state->c8_planes != !new_crtc_state->c8_planes;
>  }
>  
> +static void initialize_trans_port_sync_mode_state(struct intel_crtc_state *crtc_state)
> +{
> +	crtc_state->master_transcoder = INVALID_TRANSCODER;
> +	crtc_state->sync_mode_slaves_mask = 0;
> +}
> +
>  static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_crtc *crtc = crtc_state->uapi.crtc;
> @@ -12059,11 +12065,22 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
>  	struct drm_crtc *master_crtc = NULL;
>  	struct drm_crtc_state *master_crtc_state;
>  	struct intel_crtc_state *master_pipe_config;
> -	int i, tile_group_id;
> +	int i, tile_group_id = 0, num_tiled_conns = 0;
>  
>  	if (INTEL_GEN(dev_priv) < 11)
>  		return 0;
>  
> +	/* If all tiles not present do not make master slave assignments
> +	 * Here we assume all tiles belong to the same tile group for now.
> +	 */
> +	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> +		if (connector->has_tile) {
> +			if (!tile_group_id)
> +				tile_group_id = connector->tile_group->id;

Isn't 0 a valid tile group id?

> +			num_tiled_conns++;
> +		}

This whole thing looks confused. Should it not just look for the same
tile group as what the current connector belongs to?

> +	}
> +
>  	/*
>  	 * In case of tiled displays there could be one or more slaves but there is
>  	 * only one master. Lets make the CRTC used by the connector corresponding
> @@ -12077,8 +12094,15 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
>  		if (!connector->has_tile)
>  			continue;
>  		if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> -		    crtc_state->hw.mode.vdisplay != connector->tile_v_size)
> +		    crtc_state->hw.mode.vdisplay != connector->tile_v_size) {
> +			initialize_trans_port_sync_mode_state(crtc_state);
>  			return 0;
> +		}
> +		if (connector->tile_group->id == tile_group_id &&
> +		    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> +			initialize_trans_port_sync_mode_state(crtc_state);
> +			return 0;
> +		}
>  		if (connector->tile_h_loc == connector->num_h_tile - 1 &&
>  		    connector->tile_v_loc == connector->num_v_tile - 1)
>  			continue;

This whole thing seems kinda overly complicated. I suggest it should
just blindly go through all connectors of the same tile group and pick
the lowest transcoder as the master, which is the logic Jose is using
for MST. Except I guess we have to special case the EDP transcoder
for port sync since it can't be a slave. So a simple numeric comparison
won't quite do like used for MST.

And then we should probably move this thing to the encoder
.compute_config(). I suppose it should look in the end something like:

compute_config() {
	...
	crtc_state->master = compute_master_transcoder();
	crtc_state->slaves = 0;
	if (master_transcoder == cpu_transcoder)
		crtc_state->master = INVALID;
		crtc_state->slave = compute_slave_transcoders();
	}
}

That keeps it very readable and avoids the confusing stuff of
comptue_config() for one pipe randomly mutating the states of
the other pipes.

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