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

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

On Thu, Dec 19, 2019 at 01:51:16PM -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.
> 
> v2:
> * Rename icl_add_sync_mode_crtcs
> * Move this function just before .compute_config hook
> * Check if DP before master slave assignments (Ville)
> 
> 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 | 162 +++++++++++--------
>  1 file changed, 99 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 00608d8cef50..9c1b1256be68 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12014,88 +12014,106 @@ 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 int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> +static void reset_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_compute_port_sync_crtc_state(struct drm_connector *connector,
> +					    struct intel_crtc_state *crtc_state,
> +					    int num_tiled_conns)
>  {
>  	struct drm_crtc *crtc = crtc_state->uapi.crtc;
>  	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> -	struct drm_connector *master_connector, *connector;
> -	struct drm_connector_state *connector_state;
> +	struct drm_connector *master_connector;
>  	struct drm_connector_list_iter conn_iter;
>  	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;
>  
>  	if (INTEL_GEN(dev_priv) < 11)
>  		return 0;
>  
> +	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP))
> +		return 0;
> +
>  	/*
>  	 * 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
>  	 * to the last horizonal and last vertical tile a master/genlock CRTC.
>  	 * All the other CRTCs corresponding to other tiles of the same Tile group
>  	 * are the slave CRTCs and hold a pointer to their genlock CRTC.
> +	 * If all tiles not present do not make master slave assignments.
> +	 *
> +	 * FIXME: Add support for multiple tile grp ids in the future when such
> +	 * panels are available.

What does that mean? Why would we treat mutliple groups as a single
tiled display?

>  	 */
> -	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> -		if (connector_state->crtc != crtc)
> -			continue;
> -		if (!connector->has_tile)
> +	if (!connector->has_tile ||
> +	    crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> +	    crtc_state->hw.mode.vdisplay != connector->tile_v_size ||
> +	    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> +		reset_port_sync_mode_state(crtc_state);

I really don't understand this back and forth of setting/clearing
the master/slave state. I think the way to make it not convoluted is
to just clear everything in intel_crtc_prepare_cleared_state() and
then just compute everything from scratch.

> +		return 0;
> +	}
> +	/* Last Horizontal and last vertical tile connector is a master
> +	 * Master Trans for a Master CRTC is always INVALID.
> +	 */
> +	if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> +	    connector->tile_v_loc == connector->num_v_tile - 1) {
> +		crtc_state->master_transcoder = INVALID_TRANSCODER;
> +		return 0;
> +	}
> +
> +	/* Loop through all connectors and configure the Slave crtc_state
> +	 * to point to the correct master.
> +	 */
> +	reset_port_sync_mode_state(crtc_state);
> +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> +	drm_for_each_connector_iter(master_connector, &conn_iter) {

Why are we all of a sudden iterating through all connectors? I think this
should just iterate the connectors already in the state. The only place
where we want to look at the full connector list is the modeset_all_tiles()
thing.

> +		struct drm_connector_state *master_conn_state = NULL;
> +
> +		if (!(master_connector->has_tile &&
> +		      master_connector->tile_group->id == connector->tile_group->id))
>  			continue;
> -		if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> -		    crtc_state->hw.mode.vdisplay != connector->tile_v_size)
> -			return 0;
> -		if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> -		    connector->tile_v_loc == connector->num_v_tile - 1)
> +		if (master_connector->tile_h_loc != master_connector->num_h_tile - 1 ||
> +		    master_connector->tile_v_loc != master_connector->num_v_tile - 1)
>  			continue;
> -		crtc_state->sync_mode_slaves_mask = 0;
> -		tile_group_id = connector->tile_group->id;
> -		drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> -		drm_for_each_connector_iter(master_connector, &conn_iter) {
> -			struct drm_connector_state *master_conn_state = NULL;
>  
> -			if (!master_connector->has_tile)
> -				continue;
> -			if (master_connector->tile_h_loc != master_connector->num_h_tile - 1 ||
> -			    master_connector->tile_v_loc != master_connector->num_v_tile - 1)
> -				continue;
> -			if (master_connector->tile_group->id != tile_group_id)
> -				continue;
> -
> -			master_conn_state = drm_atomic_get_connector_state(&state->base,
> -									   master_connector);
> -			if (IS_ERR(master_conn_state)) {
> -				drm_connector_list_iter_end(&conn_iter);
> -				return PTR_ERR(master_conn_state);
> -			}
> -			if (master_conn_state->crtc) {
> -				master_crtc = master_conn_state->crtc;
> -				break;
> -			}
> +		master_conn_state = drm_atomic_get_connector_state(&state->base,
> +								   master_connector);
> +		if (IS_ERR(master_conn_state)) {
> +			drm_connector_list_iter_end(&conn_iter);
> +			return PTR_ERR(master_conn_state);
>  		}
> -		drm_connector_list_iter_end(&conn_iter);
> -
> -		if (!master_crtc) {
> -			DRM_DEBUG_KMS("Could not find Master CRTC for Slave CRTC %d\n",
> -				      connector_state->crtc->base.id);
> -			return -EINVAL;
> +		if (master_conn_state->crtc) {
> +			master_crtc = master_conn_state->crtc;
> +			break;
>  		}
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
>  
> -		master_crtc_state = drm_atomic_get_crtc_state(&state->base,
> -							      master_crtc);
> -		if (IS_ERR(master_crtc_state))
> -			return PTR_ERR(master_crtc_state);
> -
> -		master_pipe_config = to_intel_crtc_state(master_crtc_state);
> -		crtc_state->master_transcoder = master_pipe_config->cpu_transcoder;
> -		master_pipe_config->sync_mode_slaves_mask |=
> -			BIT(crtc_state->cpu_transcoder);
> -		DRM_DEBUG_KMS("Master Transcoder = %s added for Slave CRTC = %d, slave transcoder bitmask = %d\n",
> -			      transcoder_name(crtc_state->master_transcoder),
> -			      crtc_state->uapi.crtc->base.id,
> -			      master_pipe_config->sync_mode_slaves_mask);
> +	if (!master_crtc) {
> +		DRM_DEBUG_KMS("Could not find Master CRTC for Slave CRTC %d\n",
> +			      crtc->base.id);
> +		return -EINVAL;
>  	}
>  
> +	master_crtc_state = drm_atomic_get_crtc_state(&state->base,
> +						      master_crtc);
> +	if (IS_ERR(master_crtc_state))
> +		return PTR_ERR(master_crtc_state);
> +
> +	master_pipe_config = to_intel_crtc_state(master_crtc_state);
> +	crtc_state->master_transcoder = master_pipe_config->cpu_transcoder;
> +	master_pipe_config->sync_mode_slaves_mask |=
> +		BIT(crtc_state->cpu_transcoder);
> +	DRM_DEBUG_KMS("Master Transcoder = %s added for Slave CRTC = %d, slave transcoder bitmask = %d\n",
> +		      transcoder_name(crtc_state->master_transcoder),
> +		      crtc->base.id,
> +		      master_pipe_config->sync_mode_slaves_mask);
> +
>  	return 0;
>  }
>  
> @@ -12660,7 +12678,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  	struct drm_connector *connector;
>  	struct drm_connector_state *connector_state;
>  	int base_bpp, ret;
> -	int i;
> +	int i, tile_group_id = -1, num_tiled_conns = 0;
>  	bool retry = true;
>  
>  	pipe_config->cpu_transcoder =
> @@ -12730,13 +12748,23 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  	drm_mode_set_crtcinfo(&pipe_config->hw.adjusted_mode,
>  			      CRTC_STEREO_DOUBLE);
>  
> -	/* Set the crtc_state defaults for trans_port_sync */
> -	pipe_config->master_transcoder = INVALID_TRANSCODER;
> -	ret = icl_add_sync_mode_crtcs(pipe_config);
> -	if (ret) {
> -		DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
> -			      ret);
> -		return ret;
> +
> +	/* Get tile_group_id of tiled connector */
> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
> +		if (connector_state->crtc == crtc &&
> +		    connector->has_tile) {
> +			tile_group_id = connector->tile_group->id;
> +			break;
> +		}
> +	}
> +
> +	/* Get total number of tiled connectors in state that belong to
> +	 * this tile group.
> +	 */
> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
> +		if (connector->has_tile &&
> +		    connector->tile_group->id == tile_group_id)
> +			num_tiled_conns++;
>  	}

I don't see why we would need this num_tiled_conns stuff at all. Just
iterate all connectors in the state that belong to the same group.

>  
>  	/* Pass our mode to the connectors and the CRTC to give them a chance to
> @@ -12747,6 +12775,14 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  		if (connector_state->crtc != crtc)
>  			continue;
>  
> +		ret = icl_compute_port_sync_crtc_state(connector, pipe_config,
> +						       num_tiled_conns);
> +		if (ret) {
> +			DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
> +				      ret);
> +			return ret;
> +		}
> +
>  		encoder = to_intel_encoder(connector_state->best_encoder);
>  		ret = encoder->compute_config(encoder, pipe_config,
>  					      connector_state);
> -- 
> 2.19.1

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