[v3,1/6] drm/i915/display/icl: Save Master transcoder in slave's crtc_state for Transcoder Port Sync

Commit Message

Navare, Manasi Sept. 22, 2019, 5:08 p.m. UTC

In case of tiled displays when the two tiles are sent across two CRTCs
over two separate DP SST connectors, we need a mechanism to synchronize
the two CRTCs and their corresponding transcoders.
So use the master-slave mode where there is one master corresponding
to last horizontal and vertical tile that needs to be genlocked with
all other slave tiles.
This patch identifies saves the master transcoder in all the slave
CRTC states. This is needed to select the master CRTC/transcoder
while configuring transcoder port sync for the corresponding slaves.

v4:
* Rebase
v3:
* Use master_tramscoder instead of master_crtc for valid
HW state readouts (Ville)
v2:
* Move this to intel_mode_set_pipe_config(Jani N, Ville)
* Use slave_bitmask to save associated slaves in master crtc state (Ville)

Cc: Daniel Vetter <[e-mail]>
Cc: Ville Syrjälä <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Matt Roper <[e-mail]>
Signed-off-by: Manasi Navare <[e-mail]>
Reviewed-by: Maarten Lankhorst <[e-mail]>
---
 drivers/gpu/drm/i915/display/intel_display.c  | 123 ++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_display.h  |   3 +
 .../drm/i915/display/intel_display_types.h    |   6 +
 3 files changed, 132 insertions(+)

Comments

Ville Syrjala Sept. 30, 2019, 2:14 p.m. UTC | #1

On Sun, Sep 22, 2019 at 10:08:02AM -0700, Manasi Navare wrote:
> In case of tiled displays when the two tiles are sent across two CRTCs
> over two separate DP SST connectors, we need a mechanism to synchronize
> the two CRTCs and their corresponding transcoders.
> So use the master-slave mode where there is one master corresponding
> to last horizontal and vertical tile that needs to be genlocked with
> all other slave tiles.
> This patch identifies saves the master transcoder in all the slave
> CRTC states. This is needed to select the master CRTC/transcoder
> while configuring transcoder port sync for the corresponding slaves.
> 
> v4:
> * Rebase
> v3:
> * Use master_tramscoder instead of master_crtc for valid
> HW state readouts (Ville)
> v2:
> * Move this to intel_mode_set_pipe_config(Jani N, Ville)
> * Use slave_bitmask to save associated slaves in master crtc state (Ville)
> 
> Cc: Daniel Vetter <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Matt Roper <[e-mail]>
> Signed-off-by: Manasi Navare <[e-mail]>
> Reviewed-by: Maarten Lankhorst <[e-mail]>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  | 123 ++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.h  |   3 +
>  .../drm/i915/display/intel_display_types.h    |   6 +
>  3 files changed, 132 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c05ba6af6226..4ff375d5852d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -521,6 +521,24 @@ needs_modeset(const struct intel_crtc_state *state)
>  	return drm_atomic_crtc_needs_modeset(&state->base);
>  }
>  
> +bool
> +is_trans_port_sync_mode(struct drm_i915_private *dev_priv,

Redundant function parameter. Can be derived if needed.

> +			const struct intel_crtc_state *state)

'crtc_state'

> +{
> +	return (INTEL_GEN(dev_priv) >= 11 &&

I don't think we need a gen check at all. The state should not have
master/slaves set if the feature is not supported.

> +		(state->master_transcoder != INVALID_TRANSCODER ||
> +		 state->sync_mode_slaves_mask));
> +}
> +
> +static bool
> +is_trans_port_sync_master(struct drm_i915_private *dev_priv,
> +			  const struct intel_crtc_state *state)
> +{
> +	return (INTEL_GEN(dev_priv) >= 11 &&
> +		(state->master_transcoder == INVALID_TRANSCODER &&
> +		 state->sync_mode_slaves_mask));
> +}
> +
>  /*
>   * Platform specific helpers to calculate the port PLL loopback- (clock.m),
>   * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
> @@ -11773,6 +11791,91 @@ 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 drm_crtc *crtc,

intel_ types all over please.

Also don't need all three funciton arguments. Either just
crtc_state or state+crtc will do.

> +				   struct intel_crtc_state *crtc_state,
> +				   struct drm_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> +	struct drm_connector *master_connector, *connector;
> +	struct drm_connector_state *connector_state;
> +	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;
> +
> +	/*
> +	 * 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.
> +	 */
> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
> +		if (connector_state->crtc != crtc)
> +			continue;
> +		if (!connector->has_tile)
> +			continue;
> +		if (crtc_state->base.mode.hdisplay != connector->tile_h_size ||
> +		    crtc_state->base.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)
> +			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,
> +									   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;
> +			}
> +		}
> +		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;
> +		}
> +
> +		master_crtc_state = drm_atomic_get_crtc_state(state,
> +							      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->base.crtc->base.id,
> +			      master_pipe_config->sync_mode_slaves_mask);
> +	}
> +
> +	return 0;
> +}
> +
>  static int intel_crtc_atomic_check(struct drm_crtc *_crtc,
>  				   struct drm_crtc_state *_crtc_state)
>  {
> @@ -12276,6 +12379,12 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	if (IS_G4X(dev_priv) ||
>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		saved_state->wm = crtc_state->wm;
> +	/* Save the slave bitmask which gets filled for master crtc state during
> +	 * slave atomic check call.
> +	 */

Wrong comment format.

> +	if (is_trans_port_sync_master(dev_priv, crtc_state))
> +		saved_state->sync_mode_slaves_mask =
> +			crtc_state->sync_mode_slaves_mask;
>  
>  	/* Keep base drm_crtc_state intact, only clear our extended struct */
>  	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> @@ -12369,6 +12478,15 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  	drm_mode_set_crtcinfo(&pipe_config->base.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(crtc, pipe_config, state);
> +	if (ret) {
> +		DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
> +			      ret);
> +		return ret;
> +	}
> +
>  	/* Pass our mode to the connectors and the CRTC to give them a chance to
>  	 * adjust it according to limitations or connector properties, and also
>  	 * a chance to reject the mode entirely.
> @@ -12882,6 +13000,11 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  	PIPE_CONF_CHECK_INFOFRAME(hdmi);
>  	PIPE_CONF_CHECK_INFOFRAME(drm);
>  
> +	if (INTEL_GEN(dev_priv) >= 11) {

Just do the check uncoditionally.

> +		PIPE_CONF_CHECK_I(sync_mode_slaves_mask);
> +		PIPE_CONF_CHECK_I(master_transcoder);
> +	}
> +
>  #undef PIPE_CONF_CHECK_X
>  #undef PIPE_CONF_CHECK_I
>  #undef PIPE_CONF_CHECK_BOOL
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index b1ae0e59c715..1623face436b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -93,6 +93,7 @@ enum pipe {
>  #define pipe_name(p) ((p) + 'A')
>  
>  enum transcoder {
> +	INVALID_TRANSCODER = -1,
>  	/*
>  	 * The following transcoders have a 1:1 transcoder -> pipe mapping,
>  	 * keep their values fixed: the code assumes that TRANSCODER_A=0, the
> @@ -453,6 +454,8 @@ enum drm_mode_status
>  intel_mode_valid_max_plane_size(struct drm_i915_private *dev_priv,
>  				const struct drm_display_mode *mode);
>  enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port);
> +bool is_trans_port_sync_mode(struct drm_i915_private *i915,
> +			     const struct intel_crtc_state *state);
>  
>  void intel_plane_destroy(struct drm_plane *plane);
>  void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index d5cc4b810d9e..17ff34ca298b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -991,6 +991,12 @@ struct intel_crtc_state {
>  
>  	/* Forward Error correction State */
>  	bool fec_enable;
> +
> +	/* Pointer to master transcoder in case of tiled displays */
> +	enum transcoder master_transcoder;
> +
> +	/* Bitmask to indicate slaves attached */
> +	u8 sync_mode_slaves_mask;
>  };
>  
>  struct intel_crtc {
> -- 
> 2.19.1

Lucas De Marchi Sept. 30, 2019, 6:37 p.m. UTC | #2

On Sun, Sep 22, 2019 at 10:08:02AM -0700, Manasi Navare wrote:
>In case of tiled displays when the two tiles are sent across two CRTCs
>over two separate DP SST connectors, we need a mechanism to synchronize
>the two CRTCs and their corresponding transcoders.
>So use the master-slave mode where there is one master corresponding
>to last horizontal and vertical tile that needs to be genlocked with
>all other slave tiles.
>This patch identifies saves the master transcoder in all the slave
>CRTC states. This is needed to select the master CRTC/transcoder
>while configuring transcoder port sync for the corresponding slaves.
>
>v4:
>* Rebase
>v3:
>* Use master_tramscoder instead of master_crtc for valid
>HW state readouts (Ville)
>v2:
>* Move this to intel_mode_set_pipe_config(Jani N, Ville)
>* Use slave_bitmask to save associated slaves in master crtc state (Ville)
>
>Cc: Daniel Vetter <[e-mail]>
>Cc: Ville Syrjälä <[e-mail]>
>Cc: Maarten Lankhorst <[e-mail]>
>Cc: Matt Roper <[e-mail]>
>Signed-off-by: Manasi Navare <[e-mail]>
>Reviewed-by: Maarten Lankhorst <[e-mail]>
>---
> drivers/gpu/drm/i915/display/intel_display.c  | 123 ++++++++++++++++++
> drivers/gpu/drm/i915/display/intel_display.h  |   3 +
> .../drm/i915/display/intel_display_types.h    |   6 +
> 3 files changed, 132 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>index c05ba6af6226..4ff375d5852d 100644
>--- a/drivers/gpu/drm/i915/display/intel_display.c
>+++ b/drivers/gpu/drm/i915/display/intel_display.c
>@@ -521,6 +521,24 @@ needs_modeset(const struct intel_crtc_state *state)
> 	return drm_atomic_crtc_needs_modeset(&state->base);
> }
>
>+bool
>+is_trans_port_sync_mode(struct drm_i915_private *dev_priv,
>+			const struct intel_crtc_state *state)

on TGL we now also need a master transcoder for DP-MST. I'm wondering if
we couldn't reuse the same mechanism so we would dissociate a little bit
the port_sync_mode from saving or searching for a master transcoder
in crtc_state.

>@@ -12369,6 +12478,15 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> 	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
> 			      CRTC_STEREO_DOUBLE);
>
>+	/* Set the crtc_state defaults for trans_port_sync */
>+	pipe_config->master_transcoder = INVALID_TRANSCODER;

could we get away with the INVALID_TRANSCODER by simply making
pipe_config->master_transcoder = pipe_config->cpu_transcoder?

then we can always make sure it's assigned to something valid
and use it in the cases it makes sense (port sync mode and dp-mst).

Lucas De Marchi

Ville Syrjala Oct. 1, 2019, 12:17 p.m. UTC | #3

On Mon, Sep 30, 2019 at 11:37:41AM -0700, Lucas De Marchi wrote:
> On Sun, Sep 22, 2019 at 10:08:02AM -0700, Manasi Navare wrote:
> >In case of tiled displays when the two tiles are sent across two CRTCs
> >over two separate DP SST connectors, we need a mechanism to synchronize
> >the two CRTCs and their corresponding transcoders.
> >So use the master-slave mode where there is one master corresponding
> >to last horizontal and vertical tile that needs to be genlocked with
> >all other slave tiles.
> >This patch identifies saves the master transcoder in all the slave
> >CRTC states. This is needed to select the master CRTC/transcoder
> >while configuring transcoder port sync for the corresponding slaves.
> >
> >v4:
> >* Rebase
> >v3:
> >* Use master_tramscoder instead of master_crtc for valid
> >HW state readouts (Ville)
> >v2:
> >* Move this to intel_mode_set_pipe_config(Jani N, Ville)
> >* Use slave_bitmask to save associated slaves in master crtc state (Ville)
> >
> >Cc: Daniel Vetter <[e-mail]>
> >Cc: Ville Syrjälä <[e-mail]>
> >Cc: Maarten Lankhorst <[e-mail]>
> >Cc: Matt Roper <[e-mail]>
> >Signed-off-by: Manasi Navare <[e-mail]>
> >Reviewed-by: Maarten Lankhorst <[e-mail]>
> >---
> > drivers/gpu/drm/i915/display/intel_display.c  | 123 ++++++++++++++++++
> > drivers/gpu/drm/i915/display/intel_display.h  |   3 +
> > .../drm/i915/display/intel_display_types.h    |   6 +
> > 3 files changed, 132 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >index c05ba6af6226..4ff375d5852d 100644
> >--- a/drivers/gpu/drm/i915/display/intel_display.c
> >+++ b/drivers/gpu/drm/i915/display/intel_display.c
> >@@ -521,6 +521,24 @@ needs_modeset(const struct intel_crtc_state *state)
> > 	return drm_atomic_crtc_needs_modeset(&state->base);
> > }
> >
> >+bool
> >+is_trans_port_sync_mode(struct drm_i915_private *dev_priv,
> >+			const struct intel_crtc_state *state)
> 
> on TGL we now also need a master transcoder for DP-MST. I'm wondering if
> we couldn't reuse the same mechanism so we would dissociate a little bit
> the port_sync_mode from saving or searching for a master transcoder
> in crtc_state.

I think we want to track them separately to make the state checker etc.
robust. But I do think we should likely use the same logic for picking
all masters, and I think that logic should probably just be
"lowest numbered pipe is the master".

> 
> >@@ -12369,6 +12478,15 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > 	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
> > 			      CRTC_STEREO_DOUBLE);
> >
> >+	/* Set the crtc_state defaults for trans_port_sync */
> >+	pipe_config->master_transcoder = INVALID_TRANSCODER;
> 
> could we get away with the INVALID_TRANSCODER by simply making
> pipe_config->master_transcoder = pipe_config->cpu_transcoder?

That would degrade the state checker I think. Ie. we could
accidentally program the transcoder to be its own master and wouldn't
even notice. We would also need to add extra logic to check
for this when programming things and that would just result in
more wtfs when reading the code.

> 
> then we can always make sure it's assigned to something valid
> and use it in the cases it makes sense (port sync mode and dp-mst).
> 
> Lucas De Marchi
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Navare, Manasi Oct. 7, 2019, 3:43 a.m. UTC | #4

On Mon, Sep 30, 2019 at 05:14:15PM +0300, Ville Syrjälä wrote:
> On Sun, Sep 22, 2019 at 10:08:02AM -0700, Manasi Navare wrote:
> > In case of tiled displays when the two tiles are sent across two CRTCs
> > over two separate DP SST connectors, we need a mechanism to synchronize
> > the two CRTCs and their corresponding transcoders.
> > So use the master-slave mode where there is one master corresponding
> > to last horizontal and vertical tile that needs to be genlocked with
> > all other slave tiles.
> > This patch identifies saves the master transcoder in all the slave
> > CRTC states. This is needed to select the master CRTC/transcoder
> > while configuring transcoder port sync for the corresponding slaves.
> > 
> > v4:
> > * Rebase
> > v3:
> > * Use master_tramscoder instead of master_crtc for valid
> > HW state readouts (Ville)
> > v2:
> > * Move this to intel_mode_set_pipe_config(Jani N, Ville)
> > * Use slave_bitmask to save associated slaves in master crtc state (Ville)
> > 
> > Cc: Daniel Vetter <[e-mail]>
> > Cc: Ville Syrjälä <[e-mail]>
> > Cc: Maarten Lankhorst <[e-mail]>
> > Cc: Matt Roper <[e-mail]>
> > Signed-off-by: Manasi Navare <[e-mail]>
> > Reviewed-by: Maarten Lankhorst <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c  | 123 ++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_display.h  |   3 +
> >  .../drm/i915/display/intel_display_types.h    |   6 +
> >  3 files changed, 132 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index c05ba6af6226..4ff375d5852d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -521,6 +521,24 @@ needs_modeset(const struct intel_crtc_state *state)
> >  	return drm_atomic_crtc_needs_modeset(&state->base);
> >  }
> >  
> > +bool
> > +is_trans_port_sync_mode(struct drm_i915_private *dev_priv,
> 
> Redundant function parameter. Can be derived if needed.

Ok

> 
> > +			const struct intel_crtc_state *state)
> 
> 'crtc_state'

Ok


> 
> > +{
> > +	return (INTEL_GEN(dev_priv) >= 11 &&
> 
> I don't think we need a gen check at all. The state should not have
> master/slaves set if the feature is not supported.

Ok will remove this here and for master as well

> 
> > +		(state->master_transcoder != INVALID_TRANSCODER ||
> > +		 state->sync_mode_slaves_mask));
> > +}
> > +
> > +static bool
> > +is_trans_port_sync_master(struct drm_i915_private *dev_priv,
> > +			  const struct intel_crtc_state *state)
> > +{
> > +	return (INTEL_GEN(dev_priv) >= 11 &&
> > +		(state->master_transcoder == INVALID_TRANSCODER &&
> > +		 state->sync_mode_slaves_mask));
> > +}
> > +
> >  /*
> >   * Platform specific helpers to calculate the port PLL loopback- (clock.m),
> >   * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
> > @@ -11773,6 +11791,91 @@ 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 drm_crtc *crtc,
> 
> intel_ types all over please.

Yes i think this patch was written we changed everything to intel_ states, will change it all
to intel states

But do we stll keep drm_atomic_state or change that to intel_atomic_state as well?

> 
> Also don't need all three funciton arguments. Either just
> crtc_state or state+crtc will do.

Hmm, but I do need the atomic_state, crtc_state and also the crtc in the modeset


> 
> > +				   struct intel_crtc_state *crtc_state,
> > +				   struct drm_atomic_state *state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > +	struct drm_connector *master_connector, *connector;
> > +	struct drm_connector_state *connector_state;
> > +	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;
> > +
> > +	/*
> > +	 * 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.
> > +	 */
> > +	for_each_new_connector_in_state(state, connector, connector_state, i) {
> > +		if (connector_state->crtc != crtc)
> > +			continue;
> > +		if (!connector->has_tile)
> > +			continue;
> > +		if (crtc_state->base.mode.hdisplay != connector->tile_h_size ||
> > +		    crtc_state->base.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)
> > +			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,
> > +									   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;
> > +			}
> > +		}
> > +		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;
> > +		}
> > +
> > +		master_crtc_state = drm_atomic_get_crtc_state(state,
> > +							      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->base.crtc->base.id,
> > +			      master_pipe_config->sync_mode_slaves_mask);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int intel_crtc_atomic_check(struct drm_crtc *_crtc,
> >  				   struct drm_crtc_state *_crtc_state)
> >  {
> > @@ -12276,6 +12379,12 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> >  	if (IS_G4X(dev_priv) ||
> >  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >  		saved_state->wm = crtc_state->wm;
> > +	/* Save the slave bitmask which gets filled for master crtc state during
> > +	 * slave atomic check call.
> > +	 */
> 
> Wrong comment format.

What is wrong with the format?

> 
> > +	if (is_trans_port_sync_master(dev_priv, crtc_state))
> > +		saved_state->sync_mode_slaves_mask =
> > +			crtc_state->sync_mode_slaves_mask;
> >  
> >  	/* Keep base drm_crtc_state intact, only clear our extended struct */
> >  	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> > @@ -12369,6 +12478,15 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> >  	drm_mode_set_crtcinfo(&pipe_config->base.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(crtc, pipe_config, state);
> > +	if (ret) {
> > +		DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
> > +			      ret);
> > +		return ret;
> > +	}
> > +
> >  	/* Pass our mode to the connectors and the CRTC to give them a chance to
> >  	 * adjust it according to limitations or connector properties, and also
> >  	 * a chance to reject the mode entirely.
> > @@ -12882,6 +13000,11 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> >  	PIPE_CONF_CHECK_INFOFRAME(hdmi);
> >  	PIPE_CONF_CHECK_INFOFRAME(drm);
> >  
> > +	if (INTEL_GEN(dev_priv) >= 11) {
> 
> Just do the check uncoditionally.

Ok makes sens, will remove the gen check

Regards
Manasi

> 
> > +		PIPE_CONF_CHECK_I(sync_mode_slaves_mask);
> > +		PIPE_CONF_CHECK_I(master_transcoder);
> > +	}
> > +
> >  #undef PIPE_CONF_CHECK_X
> >  #undef PIPE_CONF_CHECK_I
> >  #undef PIPE_CONF_CHECK_BOOL
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> > index b1ae0e59c715..1623face436b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -93,6 +93,7 @@ enum pipe {
> >  #define pipe_name(p) ((p) + 'A')
> >  
> >  enum transcoder {
> > +	INVALID_TRANSCODER = -1,
> >  	/*
> >  	 * The following transcoders have a 1:1 transcoder -> pipe mapping,
> >  	 * keep their values fixed: the code assumes that TRANSCODER_A=0, the
> > @@ -453,6 +454,8 @@ enum drm_mode_status
> >  intel_mode_valid_max_plane_size(struct drm_i915_private *dev_priv,
> >  				const struct drm_display_mode *mode);
> >  enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port);
> > +bool is_trans_port_sync_mode(struct drm_i915_private *i915,
> > +			     const struct intel_crtc_state *state);
> >  
> >  void intel_plane_destroy(struct drm_plane *plane);
> >  void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index d5cc4b810d9e..17ff34ca298b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -991,6 +991,12 @@ struct intel_crtc_state {
> >  
> >  	/* Forward Error correction State */
> >  	bool fec_enable;
> > +
> > +	/* Pointer to master transcoder in case of tiled displays */
> > +	enum transcoder master_transcoder;
> > +
> > +	/* Bitmask to indicate slaves attached */
> > +	u8 sync_mode_slaves_mask;
> >  };
> >  
> >  struct intel_crtc {
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel

Ville Syrjala Oct. 9, 2019, 6:01 p.m. UTC | #5

On Sun, Oct 06, 2019 at 08:43:31PM -0700, Manasi Navare wrote:
> On Mon, Sep 30, 2019 at 05:14:15PM +0300, Ville Syrjälä wrote:
> > On Sun, Sep 22, 2019 at 10:08:02AM -0700, Manasi Navare wrote:
> > > In case of tiled displays when the two tiles are sent across two CRTCs
> > > over two separate DP SST connectors, we need a mechanism to synchronize
> > > the two CRTCs and their corresponding transcoders.
> > > So use the master-slave mode where there is one master corresponding
> > > to last horizontal and vertical tile that needs to be genlocked with
> > > all other slave tiles.
> > > This patch identifies saves the master transcoder in all the slave
> > > CRTC states. This is needed to select the master CRTC/transcoder
> > > while configuring transcoder port sync for the corresponding slaves.
> > > 
> > > v4:
> > > * Rebase
> > > v3:
> > > * Use master_tramscoder instead of master_crtc for valid
> > > HW state readouts (Ville)
> > > v2:
> > > * Move this to intel_mode_set_pipe_config(Jani N, Ville)
> > > * Use slave_bitmask to save associated slaves in master crtc state (Ville)
> > > 
> > > Cc: Daniel Vetter <[e-mail]>
> > > Cc: Ville Syrjälä <[e-mail]>
> > > Cc: Maarten Lankhorst <[e-mail]>
> > > Cc: Matt Roper <[e-mail]>
> > > Signed-off-by: Manasi Navare <[e-mail]>
> > > Reviewed-by: Maarten Lankhorst <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c  | 123 ++++++++++++++++++
> > >  drivers/gpu/drm/i915/display/intel_display.h  |   3 +
> > >  .../drm/i915/display/intel_display_types.h    |   6 +
> > >  3 files changed, 132 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index c05ba6af6226..4ff375d5852d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -521,6 +521,24 @@ needs_modeset(const struct intel_crtc_state *state)
> > >  	return drm_atomic_crtc_needs_modeset(&state->base);
> > >  }
> > >  
> > > +bool
> > > +is_trans_port_sync_mode(struct drm_i915_private *dev_priv,
> > 
> > Redundant function parameter. Can be derived if needed.
> 
> Ok
> 
> > 
> > > +			const struct intel_crtc_state *state)
> > 
> > 'crtc_state'
> 
> Ok
> 
> 
> > 
> > > +{
> > > +	return (INTEL_GEN(dev_priv) >= 11 &&
> > 
> > I don't think we need a gen check at all. The state should not have
> > master/slaves set if the feature is not supported.
> 
> Ok will remove this here and for master as well
> 
> > 
> > > +		(state->master_transcoder != INVALID_TRANSCODER ||
> > > +		 state->sync_mode_slaves_mask));
> > > +}
> > > +
> > > +static bool
> > > +is_trans_port_sync_master(struct drm_i915_private *dev_priv,
> > > +			  const struct intel_crtc_state *state)
> > > +{
> > > +	return (INTEL_GEN(dev_priv) >= 11 &&
> > > +		(state->master_transcoder == INVALID_TRANSCODER &&
> > > +		 state->sync_mode_slaves_mask));
> > > +}
> > > +
> > >  /*
> > >   * Platform specific helpers to calculate the port PLL loopback- (clock.m),
> > >   * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
> > > @@ -11773,6 +11791,91 @@ 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 drm_crtc *crtc,
> > 
> > intel_ types all over please.
> 
> Yes i think this patch was written we changed everything to intel_ states, will change it all
> to intel states
> 
> But do we stll keep drm_atomic_state or change that to intel_atomic_state as well?
> 
> > 
> > Also don't need all three funciton arguments. Either just
> > crtc_state or state+crtc will do.
> 
> Hmm, but I do need the atomic_state, crtc_state and also the crtc in the modeset

One can always derive the others.

Option 1:
foo(crtc_state)
{
	crtc = crtc_state->crtc;
	state = crtc_state->state;
	...
}

Option 2:
foo(state, crtc)
{
	crtc_state = get_crtc_state(state, crtc);
	...
}

IMO life is more pleasant when you don't have to think about
every function needing to be passed a different set of redundant
information.

> 
> 
> > 
> > > +				   struct intel_crtc_state *crtc_state,
> > > +				   struct drm_atomic_state *state)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > > +	struct drm_connector *master_connector, *connector;
> > > +	struct drm_connector_state *connector_state;
> > > +	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;
> > > +
> > > +	/*
> > > +	 * 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.
> > > +	 */
> > > +	for_each_new_connector_in_state(state, connector, connector_state, i) {
> > > +		if (connector_state->crtc != crtc)
> > > +			continue;
> > > +		if (!connector->has_tile)
> > > +			continue;
> > > +		if (crtc_state->base.mode.hdisplay != connector->tile_h_size ||
> > > +		    crtc_state->base.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)
> > > +			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,
> > > +									   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;
> > > +			}
> > > +		}
> > > +		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;
> > > +		}
> > > +
> > > +		master_crtc_state = drm_atomic_get_crtc_state(state,
> > > +							      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->base.crtc->base.id,
> > > +			      master_pipe_config->sync_mode_slaves_mask);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int intel_crtc_atomic_check(struct drm_crtc *_crtc,
> > >  				   struct drm_crtc_state *_crtc_state)
> > >  {
> > > @@ -12276,6 +12379,12 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> > >  	if (IS_G4X(dev_priv) ||
> > >  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > >  		saved_state->wm = crtc_state->wm;
> > > +	/* Save the slave bitmask which gets filled for master crtc state during
> > > +	 * slave atomic check call.
> > > +	 */
> > 
> > Wrong comment format.
> 
> What is wrong with the format?

Should look like:

/*
 * foo
 * bar
 */

not 

/* foo
 * bar
 */

Hmm. Looks like checkpatch doesn't complain about that anymore,
so maybe some people have finally come to accept either format.

> 
> > 
> > > +	if (is_trans_port_sync_master(dev_priv, crtc_state))
> > > +		saved_state->sync_mode_slaves_mask =
> > > +			crtc_state->sync_mode_slaves_mask;
> > >  
> > >  	/* Keep base drm_crtc_state intact, only clear our extended struct */
> > >  	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> > > @@ -12369,6 +12478,15 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > >  	drm_mode_set_crtcinfo(&pipe_config->base.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(crtc, pipe_config, state);
> > > +	if (ret) {
> > > +		DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
> > > +			      ret);
> > > +		return ret;
> > > +	}
> > > +
> > >  	/* Pass our mode to the connectors and the CRTC to give them a chance to
> > >  	 * adjust it according to limitations or connector properties, and also
> > >  	 * a chance to reject the mode entirely.
> > > @@ -12882,6 +13000,11 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> > >  	PIPE_CONF_CHECK_INFOFRAME(hdmi);
> > >  	PIPE_CONF_CHECK_INFOFRAME(drm);
> > >  
> > > +	if (INTEL_GEN(dev_priv) >= 11) {
> > 
> > Just do the check uncoditionally.
> 
> Ok makes sens, will remove the gen check
> 
> Regards
> Manasi
> 
> > 
> > > +		PIPE_CONF_CHECK_I(sync_mode_slaves_mask);
> > > +		PIPE_CONF_CHECK_I(master_transcoder);
> > > +	}
> > > +
> > >  #undef PIPE_CONF_CHECK_X
> > >  #undef PIPE_CONF_CHECK_I
> > >  #undef PIPE_CONF_CHECK_BOOL
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> > > index b1ae0e59c715..1623face436b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > > @@ -93,6 +93,7 @@ enum pipe {
> > >  #define pipe_name(p) ((p) + 'A')
> > >  
> > >  enum transcoder {
> > > +	INVALID_TRANSCODER = -1,
> > >  	/*
> > >  	 * The following transcoders have a 1:1 transcoder -> pipe mapping,
> > >  	 * keep their values fixed: the code assumes that TRANSCODER_A=0, the
> > > @@ -453,6 +454,8 @@ enum drm_mode_status
> > >  intel_mode_valid_max_plane_size(struct drm_i915_private *dev_priv,
> > >  				const struct drm_display_mode *mode);
> > >  enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port);
> > > +bool is_trans_port_sync_mode(struct drm_i915_private *i915,
> > > +			     const struct intel_crtc_state *state);
> > >  
> > >  void intel_plane_destroy(struct drm_plane *plane);
> > >  void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index d5cc4b810d9e..17ff34ca298b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -991,6 +991,12 @@ struct intel_crtc_state {
> > >  
> > >  	/* Forward Error correction State */
> > >  	bool fec_enable;
> > > +
> > > +	/* Pointer to master transcoder in case of tiled displays */
> > > +	enum transcoder master_transcoder;
> > > +
> > > +	/* Bitmask to indicate slaves attached */
> > > +	u8 sync_mode_slaves_mask;
> > >  };
> > >  
> > >  struct intel_crtc {
> > > -- 
> > > 2.19.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index c05ba6af6226..4ff375d5852d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -521,6 +521,24 @@  needs_modeset(const struct intel_crtc_state *state)
 	return drm_atomic_crtc_needs_modeset(&state->base);
 }
 
+bool
+is_trans_port_sync_mode(struct drm_i915_private *dev_priv,
+			const struct intel_crtc_state *state)
+{
+	return (INTEL_GEN(dev_priv) >= 11 &&
+		(state->master_transcoder != INVALID_TRANSCODER ||
+		 state->sync_mode_slaves_mask));
+}
+
+static bool
+is_trans_port_sync_master(struct drm_i915_private *dev_priv,
+			  const struct intel_crtc_state *state)
+{
+	return (INTEL_GEN(dev_priv) >= 11 &&
+		(state->master_transcoder == INVALID_TRANSCODER &&
+		 state->sync_mode_slaves_mask));
+}
+
 /*
  * Platform specific helpers to calculate the port PLL loopback- (clock.m),
  * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
@@ -11773,6 +11791,91 @@  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 drm_crtc *crtc,
+				   struct intel_crtc_state *crtc_state,
+				   struct drm_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
+	struct drm_connector *master_connector, *connector;
+	struct drm_connector_state *connector_state;
+	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;
+
+	/*
+	 * 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.
+	 */
+	for_each_new_connector_in_state(state, connector, connector_state, i) {
+		if (connector_state->crtc != crtc)
+			continue;
+		if (!connector->has_tile)
+			continue;
+		if (crtc_state->base.mode.hdisplay != connector->tile_h_size ||
+		    crtc_state->base.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)
+			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,
+									   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;
+			}
+		}
+		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;
+		}
+
+		master_crtc_state = drm_atomic_get_crtc_state(state,
+							      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->base.crtc->base.id,
+			      master_pipe_config->sync_mode_slaves_mask);
+	}
+
+	return 0;
+}
+
 static int intel_crtc_atomic_check(struct drm_crtc *_crtc,
 				   struct drm_crtc_state *_crtc_state)
 {
@@ -12276,6 +12379,12 @@  clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	if (IS_G4X(dev_priv) ||
 	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		saved_state->wm = crtc_state->wm;
+	/* Save the slave bitmask which gets filled for master crtc state during
+	 * slave atomic check call.
+	 */
+	if (is_trans_port_sync_master(dev_priv, crtc_state))
+		saved_state->sync_mode_slaves_mask =
+			crtc_state->sync_mode_slaves_mask;
 
 	/* Keep base drm_crtc_state intact, only clear our extended struct */
 	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
@@ -12369,6 +12478,15 @@  intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
 	drm_mode_set_crtcinfo(&pipe_config->base.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(crtc, pipe_config, state);
+	if (ret) {
+		DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
+			      ret);
+		return ret;
+	}
+
 	/* Pass our mode to the connectors and the CRTC to give them a chance to
 	 * adjust it according to limitations or connector properties, and also
 	 * a chance to reject the mode entirely.
@@ -12882,6 +13000,11 @@  intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 	PIPE_CONF_CHECK_INFOFRAME(hdmi);
 	PIPE_CONF_CHECK_INFOFRAME(drm);
 
+	if (INTEL_GEN(dev_priv) >= 11) {
+		PIPE_CONF_CHECK_I(sync_mode_slaves_mask);
+		PIPE_CONF_CHECK_I(master_transcoder);
+	}
+
 #undef PIPE_CONF_CHECK_X
 #undef PIPE_CONF_CHECK_I
 #undef PIPE_CONF_CHECK_BOOL
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index b1ae0e59c715..1623face436b 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -93,6 +93,7 @@  enum pipe {
 #define pipe_name(p) ((p) + 'A')
 
 enum transcoder {
+	INVALID_TRANSCODER = -1,
 	/*
 	 * The following transcoders have a 1:1 transcoder -> pipe mapping,
 	 * keep their values fixed: the code assumes that TRANSCODER_A=0, the
@@ -453,6 +454,8 @@  enum drm_mode_status
 intel_mode_valid_max_plane_size(struct drm_i915_private *dev_priv,
 				const struct drm_display_mode *mode);
 enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port);
+bool is_trans_port_sync_mode(struct drm_i915_private *i915,
+			     const struct intel_crtc_state *state);
 
 void intel_plane_destroy(struct drm_plane *plane);
 void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index d5cc4b810d9e..17ff34ca298b 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -991,6 +991,12 @@  struct intel_crtc_state {
 
 	/* Forward Error correction State */
 	bool fec_enable;
+
+	/* Pointer to master transcoder in case of tiled displays */
+	enum transcoder master_transcoder;
+
+	/* Bitmask to indicate slaves attached */
+	u8 sync_mode_slaves_mask;
 };
 
 struct intel_crtc {