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

[Manasi Navare <manasi.d.navare@intel.com>]

On Fri, Dec 20, 2019 at 03:59:41PM +0200, Ville Syrjälä wrote:
> 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?
>

No we wont treat multiple tile grp ids as a single display but we need to support
multiple pile grp ids in the state so 2 pairs of master and slave.
 
> >  	 */
> > -	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.
>

Hmm i can add that to intel_crtc_prepare_cleared_state(), but since we compute master crtc
and its crtc_state during slave's compute_Config, we cannot clear the state if its master.
 
> > +		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.
>

This is how we had decided to do the master slave asignments and adding other crtc
corresponding to last tile (master) to state by iteratin through all connectors.
This was sugested on IRC by Danvet and Maarten when I was finalizing the design in the code
for master slave assignments.

so basically for the master compute config, it just returns and this port sync sonfig compute 
happens for the slave, if master slaves all not present so all tiles not present then we reset the 
asignments and return.

And thats one of the reasons why i need the reset here since the corner cases check happens here like
if all tiles are present if hdisplay vdisplay matches tile size, if not then we should reset the master slave crtc states.

 
> > +		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.
>

Yea but if all tiles are not present meaning num_tiled_conns not equal to num tiles then
we should not make master slave assignments, here the crtcs would not be synced and just operate
as an individual crtc not synced crtc. Like after we disconnect 1 connector, there is only 1 tiled conn connected
and in that case it still displays the lower non tiled mode but we should not make master slave asignments.

May be all these checks for hdisplay , vdisplay, num conns == num_tiles can be moved to intel_atomic_sync_mode_check at the beginning?

Manasi
 
> >  
> >  	/* 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