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

[Manasi Navare]

On Fri, Dec 13, 2019 at 10:28:49PM +0200, Ville Syrjälä wrote:
> 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 at linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare at 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?

I was making sure that we dont count the connectors that dont belong to the same tile grp id.
But yes I agree that the tile grp id can be assigned by current connector's tile grp id
and add num_conns only if they are of the same tile grp id.

> 
> > +	}
> > +
> >  	/*
> >  	 * 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.

But the logic that we are using is always make the connector corresponding to the
last tile the master and all other connectors as slaves since only 1 master (last tile)
and multiple slaves.
This was agreed upon with discussions with you and Danvet and Maarten almost 6 months back
and the entire enable/disable sequence is based on this.
I would rather keep the logic same for now since the master slave asisgnments which was
designed based on consensus during the initial patches and since its working well now.

The only thing that I wanted to add in this patch is reset the master_trans and slave_bitmask
properly if all tiles not present in order to handle the hotplug case correctly.

I will make changes for the tile grp id that you suggested for now and we can simplify the logic later
as enhncements/optimizations.

Sounds good?

Manasi

> 
> -- 
> Ville Syrjälä
> Intel