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

Hmm. Except that won't actually work since .cpu_transcoder won't have
been populated yet for the later pipes. So we can't use that in
.compute_config(). So either we do this after .compute_config()
is done for everyone or we just calculate the cpu_transcoder on the
spot (I mean it's just a trivial port==A->EDP else->pipe so no big deal).

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

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