[v2,1/4] drm/i915/icl: Assign Master slave crtc links for Transcoder Port Sync

Commit Message

Navare, Manasi April 23, 2019, 3:48 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 CRTC pointer in all the slave
CRTC states. This pointer is needed to select the master CRTC/transcoder
while configuring transcoder port sync for the corresponding slaves.

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]>
---
 drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  6 ++
 2 files changed, 95 insertions(+)

Comments

Srivatsa, Anusha May 4, 2019, 12:09 a.m. UTC | #1

>-----Original Message-----
>From: Intel-gfx [mailto:[e-mail]] On Behalf Of
>Manasi Navare
>Sent: Tuesday, April 23, 2019 8:49 AM
>To: [e-mail]
>Cc: Daniel Vetter <[e-mail]>
>Subject: [Intel-gfx] [PATCH v2 1/4] drm/i915/icl: Assign Master slave crtc links for
>Transcoder Port Sync
>
>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 CRTC pointer in all the slave CRTC states.
>This pointer is needed to select the master CRTC/transcoder while configuring
>transcoder port sync for the corresponding slaves.
>
>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]>
>---
> drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_drv.h     |  6 ++
> 2 files changed, 95 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/intel_display.c
>b/drivers/gpu/drm/i915/intel_display.c
>index b276345779e6..92dea2231499 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -11316,6 +11316,86 @@ static int icl_check_nv12_planes(struct
>intel_crtc_state *crtc_state)
> 	return 0;
> }
>
>+static int icl_add_genlock_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;
>+	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 (connector->tile_h_loc == connector->num_h_tile - 1 &&
>+		    connector->tile_v_loc == connector->num_v_tile - 1)
>+			continue;
>+		crtc_state->master_crtc = NULL;
>+		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)

Will this ever be true? With the checks above (for slaves) we have iterated over all slaves and potentially left with only master/last tile right?

Anusha 

>+				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 add 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);
>+
>+		crtc_state->master_crtc = to_intel_crtc(master_crtc);
>+		to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves
>|=
>+			BIT(to_intel_crtc(crtc)->pipe);
>+		DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC =
>%d\n, slave bitmast = %d",
>+			      master_crtc->base.id,
>+			      crtc_state->base.crtc->base.id,
>+			      to_intel_crtc_state(master_crtc_state)-
>>trans_port_sync_slaves);
>+	}
>+
>+	return 0;
>+}
>+
> static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> 				   struct drm_crtc_state *crtc_state)  { @@ -
>11795,6 +11875,9 @@ 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;
>+	if (INTEL_GEN(dev_priv) >= 11)
>+		saved_state->trans_port_sync_slaves =
>+			crtc_state->trans_port_sync_slaves;
>
> 	/* Keep base drm_crtc_state intact, only clear our extended struct */
> 	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base)); @@ -11888,6
>+11971,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> 	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
> 			      CRTC_STEREO_DOUBLE);
>
>+	ret = icl_add_genlock_crtcs(crtc, pipe_config, state);
>+	if (ret) {
>+		DRM_DEBUG_KMS("\n8K Debug: Cannot assign genlock crtcs");
>+		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.
>diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>index a38b9cff5cd0..8ae9cb662e28 100644
>--- a/drivers/gpu/drm/i915/intel_drv.h
>+++ b/drivers/gpu/drm/i915/intel_drv.h
>@@ -1082,6 +1082,12 @@ struct intel_crtc_state {
>
> 	/* Forward Error correction State */
> 	bool fec_enable;
>+
>+	/* Pointer to master crtc in case of tiled displays */
>+	struct intel_crtc *master_crtc;
>+
>+	/* Bitmask to indicate slaves attached */
>+	u8 trans_port_sync_slaves;
> };
>
> struct intel_crtc {
>--
>2.19.1
>
>_______________________________________________
>Intel-gfx mailing list
>[e-mail]
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Navare, Manasi May 22, 2019, 9:40 p.m. UTC | #2

On Fri, May 03, 2019 at 05:09:20PM -0700, Srivatsa, Anusha wrote:
> 
> 
> >-----Original Message-----
> >From: Intel-gfx [mailto:[e-mail]] On Behalf Of
> >Manasi Navare
> >Sent: Tuesday, April 23, 2019 8:49 AM
> >To: [e-mail]
> >Cc: Daniel Vetter <[e-mail]>
> >Subject: [Intel-gfx] [PATCH v2 1/4] drm/i915/icl: Assign Master slave crtc links for
> >Transcoder Port Sync
> >
> >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 CRTC pointer in all the slave CRTC states.
> >This pointer is needed to select the master CRTC/transcoder while configuring
> >transcoder port sync for the corresponding slaves.
> >
> >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]>
> >---
> > drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_drv.h     |  6 ++
> > 2 files changed, 95 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_display.c
> >b/drivers/gpu/drm/i915/intel_display.c
> >index b276345779e6..92dea2231499 100644
> >--- a/drivers/gpu/drm/i915/intel_display.c
> >+++ b/drivers/gpu/drm/i915/intel_display.c
> >@@ -11316,6 +11316,86 @@ static int icl_check_nv12_planes(struct
> >intel_crtc_state *crtc_state)
> > 	return 0;
> > }
> >
> >+static int icl_add_genlock_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;
> >+	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 (connector->tile_h_loc == connector->num_h_tile - 1 &&
> >+		    connector->tile_v_loc == connector->num_v_tile - 1)
> >+			continue;
> >+		crtc_state->master_crtc = NULL;
> >+		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)
> 
> Will this ever be true? With the checks above (for slaves) we have iterated over all slaves and potentially left with only master/last tile right?
> 
> Anusha 

Actually its the other way around. So like the comment says the last tile is the master and all others are slaves,
so here in for_each_connector(), we skip the master and call drm_for_each_connector_iter() for each slave to find its
corresponding master. So if its not the last tile then we continue until we find the last tile which becomes the master.
Makes sense?

Manasi

> 
> >+				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 add 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);
> >+
> >+		crtc_state->master_crtc = to_intel_crtc(master_crtc);
> >+		to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves
> >|=
> >+			BIT(to_intel_crtc(crtc)->pipe);
> >+		DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC =
> >%d\n, slave bitmast = %d",
> >+			      master_crtc->base.id,
> >+			      crtc_state->base.crtc->base.id,
> >+			      to_intel_crtc_state(master_crtc_state)-
> >>trans_port_sync_slaves);
> >+	}
> >+
> >+	return 0;
> >+}
> >+
> > static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> > 				   struct drm_crtc_state *crtc_state)  { @@ -
> >11795,6 +11875,9 @@ 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;
> >+	if (INTEL_GEN(dev_priv) >= 11)
> >+		saved_state->trans_port_sync_slaves =
> >+			crtc_state->trans_port_sync_slaves;
> >
> > 	/* Keep base drm_crtc_state intact, only clear our extended struct */
> > 	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base)); @@ -11888,6
> >+11971,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> > 	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
> > 			      CRTC_STEREO_DOUBLE);
> >
> >+	ret = icl_add_genlock_crtcs(crtc, pipe_config, state);
> >+	if (ret) {
> >+		DRM_DEBUG_KMS("\n8K Debug: Cannot assign genlock crtcs");
> >+		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.
> >diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >index a38b9cff5cd0..8ae9cb662e28 100644
> >--- a/drivers/gpu/drm/i915/intel_drv.h
> >+++ b/drivers/gpu/drm/i915/intel_drv.h
> >@@ -1082,6 +1082,12 @@ struct intel_crtc_state {
> >
> > 	/* Forward Error correction State */
> > 	bool fec_enable;
> >+
> >+	/* Pointer to master crtc in case of tiled displays */
> >+	struct intel_crtc *master_crtc;
> >+
> >+	/* Bitmask to indicate slaves attached */
> >+	u8 trans_port_sync_slaves;
> > };
> >
> > struct intel_crtc {
> >--
> >2.19.1
> >
> >_______________________________________________
> >Intel-gfx mailing list
> >[e-mail]
> >https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Navare, Manasi June 11, 2019, 11:28 p.m. UTC | #3

Need some suggestions on what could be stored in slave_crtc_state to point to the master,
See my comments/questions inline

On Tue, Apr 23, 2019 at 08:48:58AM -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 CRTC pointer in all the slave
> CRTC states. This pointer is needed to select the master CRTC/transcoder
> while configuring transcoder port sync for the corresponding slaves.
> 
> 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]>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  6 ++
>  2 files changed, 95 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b276345779e6..92dea2231499 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11316,6 +11316,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> +static int icl_add_genlock_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;
> +	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 (connector->tile_h_loc == connector->num_h_tile - 1 &&
> +		    connector->tile_v_loc == connector->num_v_tile - 1)
> +			continue;
> +		crtc_state->master_crtc = NULL;
> +		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 add 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);
> +
> +		crtc_state->master_crtc = to_intel_crtc(master_crtc);

This stores a pointer to master_crtc which is used to obtain the master_crtc_state in crtc_enable
and obtain cpu_transcoder from that to configure master_select in TRANS PORT SYNC register
However, during the HW state readout we will read this register back for the slave to obtain the
master transcoder and I dont really have a clear path to find the master crtc from master transcoder
in order to do pipe_config_compare() in verify_crtc_state() which is currently causing the Pipe mismatch errors

Any suggestions on how this can be fixed? This assignment happens very early on before modeset_pipe_config, 
so I dont believe I can use master_crtc_state->cpu_transcoder to obtain master transcoder.

Manasi

> +		to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves |=
> +			BIT(to_intel_crtc(crtc)->pipe);
> +		DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC = %d\n, slave bitmast = %d",
> +			      master_crtc->base.id,
> +			      crtc_state->base.crtc->base.id,
> +			      to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves);
> +	}
> +
> +	return 0;
> +}
> +
>  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  				   struct drm_crtc_state *crtc_state)
>  {
> @@ -11795,6 +11875,9 @@ 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;
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		saved_state->trans_port_sync_slaves =
> +			crtc_state->trans_port_sync_slaves;
>  
>  	/* Keep base drm_crtc_state intact, only clear our extended struct */
>  	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> @@ -11888,6 +11971,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
>  			      CRTC_STEREO_DOUBLE);
>  
> +	ret = icl_add_genlock_crtcs(crtc, pipe_config, state);
> +	if (ret) {
> +		DRM_DEBUG_KMS("\n8K Debug: Cannot assign genlock crtcs");
> +		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.
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a38b9cff5cd0..8ae9cb662e28 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1082,6 +1082,12 @@ struct intel_crtc_state {
>  
>  	/* Forward Error correction State */
>  	bool fec_enable;
> +
> +	/* Pointer to master crtc in case of tiled displays */
> +	struct intel_crtc *master_crtc;
> +
> +	/* Bitmask to indicate slaves attached */
> +	u8 trans_port_sync_slaves;
>  };
>  
>  struct intel_crtc {
> -- 
> 2.19.1
>

Maarten Lankhorst June 12, 2019, 9:39 a.m. UTC | #4

Op 23-04-2019 om 17:48 schreef Manasi Navare:
> 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 CRTC pointer in all the slave
> CRTC states. This pointer is needed to select the master CRTC/transcoder
> while configuring transcoder port sync for the corresponding slaves.
>
> 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]>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  6 ++
>  2 files changed, 95 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b276345779e6..92dea2231499 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11316,6 +11316,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> +static int icl_add_genlock_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;
> +	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 (connector->tile_h_loc == connector->num_h_tile - 1 &&
> +		    connector->tile_v_loc == connector->num_v_tile - 1)
> +			continue;
> +		crtc_state->master_crtc = NULL;
> +		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 add 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);
> +
> +		crtc_state->master_crtc = to_intel_crtc(master_crtc);
> +		to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves |=
> +			BIT(to_intel_crtc(crtc)->pipe);
> +		DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC = %d\n, slave bitmast = %d",
> +			      master_crtc->base.id,
> +			      crtc_state->base.crtc->base.id,
> +			      to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves);
> +	}
> +
> +	return 0;
> +}
> +
>  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  				   struct drm_crtc_state *crtc_state)
>  {
> @@ -11795,6 +11875,9 @@ 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;
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		saved_state->trans_port_sync_slaves =
> +			crtc_state->trans_port_sync_slaves;
>  
>  	/* Keep base drm_crtc_state intact, only clear our extended struct */
>  	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> @@ -11888,6 +11971,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
>  			      CRTC_STEREO_DOUBLE);
>  
> +	ret = icl_add_genlock_crtcs(crtc, pipe_config, state);
> +	if (ret) {
> +		DRM_DEBUG_KMS("\n8K Debug: Cannot assign genlock crtcs");
> +		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.

I thgink we should extend on this to make master/slave mode work by doing what matt roper was working on, in collaboration with Ville?

crtc->state points to drm_crtc_state, which is the uapi state, but also contains a shadow struct intel_crtc_state, which is the actual hw state. It has its own drm_crtc_state object for the real state, which gets copied after check.

This should then also be done for intel_plane_state, which will allow us to correctly handle this mode.

> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a38b9cff5cd0..8ae9cb662e28 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1082,6 +1082,12 @@ struct intel_crtc_state {
>  
>  	/* Forward Error correction State */
>  	bool fec_enable;
> +
> +	/* Pointer to master crtc in case of tiled displays */
> +	struct intel_crtc *master_crtc;
> +
> +	/* Bitmask to indicate slaves attached */
> +	u8 trans_port_sync_slaves;
>  };
>  
>  struct intel_crtc {

Ville Syrjälä June 12, 2019, 7:04 p.m. UTC | #5

On Wed, Jun 12, 2019 at 11:39:03AM +0200, Maarten Lankhorst wrote:
> Op 23-04-2019 om 17:48 schreef Manasi Navare:
> > 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 CRTC pointer in all the slave
> > CRTC states. This pointer is needed to select the master CRTC/transcoder
> > while configuring transcoder port sync for the corresponding slaves.
> >
> > 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]>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |  6 ++
> >  2 files changed, 95 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b276345779e6..92dea2231499 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11316,6 +11316,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
> >  	return 0;
> >  }
> >  
> > +static int icl_add_genlock_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;
> > +	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 (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > +		    connector->tile_v_loc == connector->num_v_tile - 1)
> > +			continue;
> > +		crtc_state->master_crtc = NULL;
> > +		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 add 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);
> > +
> > +		crtc_state->master_crtc = to_intel_crtc(master_crtc);
> > +		to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves |=
> > +			BIT(to_intel_crtc(crtc)->pipe);
> > +		DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC = %d\n, slave bitmast = %d",
> > +			      master_crtc->base.id,
> > +			      crtc_state->base.crtc->base.id,
> > +			      to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> >  				   struct drm_crtc_state *crtc_state)
> >  {
> > @@ -11795,6 +11875,9 @@ 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;
> > +	if (INTEL_GEN(dev_priv) >= 11)
> > +		saved_state->trans_port_sync_slaves =
> > +			crtc_state->trans_port_sync_slaves;
> >  
> >  	/* Keep base drm_crtc_state intact, only clear our extended struct */
> >  	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> > @@ -11888,6 +11971,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> >  	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
> >  			      CRTC_STEREO_DOUBLE);
> >  
> > +	ret = icl_add_genlock_crtcs(crtc, pipe_config, state);
> > +	if (ret) {
> > +		DRM_DEBUG_KMS("\n8K Debug: Cannot assign genlock crtcs");
> > +		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.
> 
> I thgink we should extend on this to make master/slave mode work by doing what matt roper was working on, in collaboration with Ville?
> 
> crtc->state points to drm_crtc_state, which is the uapi state, but also contains a shadow struct intel_crtc_state, which is the actual hw state. It has its own drm_crtc_state object for the real state, which gets copied after check.
> 
> This should then also be done for intel_plane_state, which will allow us to correctly handle this mode.

I don't think there's any linkage between this and the 2-pipe-1-port
cases other than we might want to handle tiled displays that way one
day to make life easier for userspace. But that has other issues such
as we'd have to generate a new EDID for the whole display from the EDIDs
of the tiles.

Also we maybe want port sync for 100% independent displays too. So IMO
no point in trying to shoehorn both things into the same bucket.

Navare, Manasi June 12, 2019, 7:11 p.m. UTC | #6

On Wed, Jun 12, 2019 at 10:04:26PM +0300, Ville Syrjälä wrote:
> On Wed, Jun 12, 2019 at 11:39:03AM +0200, Maarten Lankhorst wrote:
> > Op 23-04-2019 om 17:48 schreef Manasi Navare:
> > > 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 CRTC pointer in all the slave
> > > CRTC states. This pointer is needed to select the master CRTC/transcoder
> > > while configuring transcoder port sync for the corresponding slaves.
> > >
> > > 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]>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h     |  6 ++
> > >  2 files changed, 95 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index b276345779e6..92dea2231499 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -11316,6 +11316,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
> > >  	return 0;
> > >  }
> > >  
> > > +static int icl_add_genlock_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;
> > > +	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 (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > > +		    connector->tile_v_loc == connector->num_v_tile - 1)
> > > +			continue;
> > > +		crtc_state->master_crtc = NULL;
> > > +		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 add 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);
> > > +
> > > +		crtc_state->master_crtc = to_intel_crtc(master_crtc);
> > > +		to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves |=
> > > +			BIT(to_intel_crtc(crtc)->pipe);
> > > +		DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC = %d\n, slave bitmast = %d",
> > > +			      master_crtc->base.id,
> > > +			      crtc_state->base.crtc->base.id,
> > > +			      to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> > >  				   struct drm_crtc_state *crtc_state)
> > >  {
> > > @@ -11795,6 +11875,9 @@ 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;
> > > +	if (INTEL_GEN(dev_priv) >= 11)
> > > +		saved_state->trans_port_sync_slaves =
> > > +			crtc_state->trans_port_sync_slaves;
> > >  
> > >  	/* Keep base drm_crtc_state intact, only clear our extended struct */
> > >  	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> > > @@ -11888,6 +11971,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> > >  	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
> > >  			      CRTC_STEREO_DOUBLE);
> > >  
> > > +	ret = icl_add_genlock_crtcs(crtc, pipe_config, state);
> > > +	if (ret) {
> > > +		DRM_DEBUG_KMS("\n8K Debug: Cannot assign genlock crtcs");
> > > +		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.
> > 
> > I thgink we should extend on this to make master/slave mode work by doing what matt roper was working on, in collaboration with Ville?
> > 
> > crtc->state points to drm_crtc_state, which is the uapi state, but also contains a shadow struct intel_crtc_state, which is the actual hw state. It has its own drm_crtc_state object for the real state, which gets copied after check.
> > 
> > This should then also be done for intel_plane_state, which will allow us to correctly handle this mode.
> 
> I don't think there's any linkage between this and the 2-pipe-1-port
> cases other than we might want to handle tiled displays that way one
> day to make life easier for userspace. But that has other issues such
> as we'd have to generate a new EDID for the whole display from the EDIDs
> of the tiles.
> 
> Also we maybe want port sync for 100% independent displays too. So IMO
> no point in trying to shoehorn both things into the same bucket.

I agree, also the tiled display case is simple in terms of the states since its 2 connectors and 2 crtcs exposed to the userspace
so the userspace will create two states and they are used as is for the two crtcs, the only difference is that we need to program
one as a slave and the other as master and find a way to store a pointer to master from the slave since we need to use that
to configure and enable transcoder port sync while enabling slave CRTC.

So on those lines, if i store the master transcoder directly in the slave crtc state then trans = 0 is valid for trans A then how
do I identify which crtc is slave?
Currently I check for !master_crtc and that tells me that it is not slave.

Manasi

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

Ville Syrjälä June 12, 2019, 7:30 p.m. UTC | #7

On Wed, Jun 12, 2019 at 12:11:02PM -0700, Manasi Navare wrote:
> On Wed, Jun 12, 2019 at 10:04:26PM +0300, Ville Syrjälä wrote:
> > On Wed, Jun 12, 2019 at 11:39:03AM +0200, Maarten Lankhorst wrote:
> > > Op 23-04-2019 om 17:48 schreef Manasi Navare:
> > > > 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 CRTC pointer in all the slave
> > > > CRTC states. This pointer is needed to select the master CRTC/transcoder
> > > > while configuring transcoder port sync for the corresponding slaves.
> > > >
> > > > 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]>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_drv.h     |  6 ++
> > > >  2 files changed, 95 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index b276345779e6..92dea2231499 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -11316,6 +11316,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static int icl_add_genlock_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;
> > > > +	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 (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > > > +		    connector->tile_v_loc == connector->num_v_tile - 1)
> > > > +			continue;
> > > > +		crtc_state->master_crtc = NULL;
> > > > +		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 add 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);
> > > > +
> > > > +		crtc_state->master_crtc = to_intel_crtc(master_crtc);
> > > > +		to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves |=
> > > > +			BIT(to_intel_crtc(crtc)->pipe);
> > > > +		DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC = %d\n, slave bitmast = %d",
> > > > +			      master_crtc->base.id,
> > > > +			      crtc_state->base.crtc->base.id,
> > > > +			      to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves);
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> > > >  				   struct drm_crtc_state *crtc_state)
> > > >  {
> > > > @@ -11795,6 +11875,9 @@ 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;
> > > > +	if (INTEL_GEN(dev_priv) >= 11)
> > > > +		saved_state->trans_port_sync_slaves =
> > > > +			crtc_state->trans_port_sync_slaves;
> > > >  
> > > >  	/* Keep base drm_crtc_state intact, only clear our extended struct */
> > > >  	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> > > > @@ -11888,6 +11971,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> > > >  	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
> > > >  			      CRTC_STEREO_DOUBLE);
> > > >  
> > > > +	ret = icl_add_genlock_crtcs(crtc, pipe_config, state);
> > > > +	if (ret) {
> > > > +		DRM_DEBUG_KMS("\n8K Debug: Cannot assign genlock crtcs");
> > > > +		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.
> > > 
> > > I thgink we should extend on this to make master/slave mode work by doing what matt roper was working on, in collaboration with Ville?
> > > 
> > > crtc->state points to drm_crtc_state, which is the uapi state, but also contains a shadow struct intel_crtc_state, which is the actual hw state. It has its own drm_crtc_state object for the real state, which gets copied after check.
> > > 
> > > This should then also be done for intel_plane_state, which will allow us to correctly handle this mode.
> > 
> > I don't think there's any linkage between this and the 2-pipe-1-port
> > cases other than we might want to handle tiled displays that way one
> > day to make life easier for userspace. But that has other issues such
> > as we'd have to generate a new EDID for the whole display from the EDIDs
> > of the tiles.
> > 
> > Also we maybe want port sync for 100% independent displays too. So IMO
> > no point in trying to shoehorn both things into the same bucket.
> 
> I agree, also the tiled display case is simple in terms of the states since its 2 connectors and 2 crtcs exposed to the userspace
> so the userspace will create two states and they are used as is for the two crtcs, the only difference is that we need to program
> one as a slave and the other as master and find a way to store a pointer to master from the slave since we need to use that
> to configure and enable transcoder port sync while enabling slave CRTC.
> 
> So on those lines, if i store the master transcoder directly in the slave crtc state then trans = 0 is valid for trans A then how
> do I identify which crtc is slave?
> Currently I check for !master_crtc and that tells me that it is not slave.

INVALID_TRANSCODDER or something?

Navare, Manasi June 12, 2019, 8:59 p.m. UTC | #8

On Wed, Jun 12, 2019 at 10:30:55PM +0300, Ville Syrjälä wrote:
> On Wed, Jun 12, 2019 at 12:11:02PM -0700, Manasi Navare wrote:
> > On Wed, Jun 12, 2019 at 10:04:26PM +0300, Ville Syrjälä wrote:
> > > On Wed, Jun 12, 2019 at 11:39:03AM +0200, Maarten Lankhorst wrote:
> > > > Op 23-04-2019 om 17:48 schreef Manasi Navare:
> > > > > 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 CRTC pointer in all the slave
> > > > > CRTC states. This pointer is needed to select the master CRTC/transcoder
> > > > > while configuring transcoder port sync for the corresponding slaves.
> > > > >
> > > > > 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]>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++++++++++++++++
> > > > >  drivers/gpu/drm/i915/intel_drv.h     |  6 ++
> > > > >  2 files changed, 95 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > index b276345779e6..92dea2231499 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -11316,6 +11316,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > +static int icl_add_genlock_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;
> > > > > +	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 (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > > > > +		    connector->tile_v_loc == connector->num_v_tile - 1)
> > > > > +			continue;
> > > > > +		crtc_state->master_crtc = NULL;
> > > > > +		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 add 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);
> > > > > +
> > > > > +		crtc_state->master_crtc = to_intel_crtc(master_crtc);
> > > > > +		to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves |=
> > > > > +			BIT(to_intel_crtc(crtc)->pipe);
> > > > > +		DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC = %d\n, slave bitmast = %d",
> > > > > +			      master_crtc->base.id,
> > > > > +			      crtc_state->base.crtc->base.id,
> > > > > +			      to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves);
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> > > > >  				   struct drm_crtc_state *crtc_state)
> > > > >  {
> > > > > @@ -11795,6 +11875,9 @@ 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;
> > > > > +	if (INTEL_GEN(dev_priv) >= 11)
> > > > > +		saved_state->trans_port_sync_slaves =
> > > > > +			crtc_state->trans_port_sync_slaves;
> > > > >  
> > > > >  	/* Keep base drm_crtc_state intact, only clear our extended struct */
> > > > >  	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> > > > > @@ -11888,6 +11971,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> > > > >  	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
> > > > >  			      CRTC_STEREO_DOUBLE);
> > > > >  
> > > > > +	ret = icl_add_genlock_crtcs(crtc, pipe_config, state);
> > > > > +	if (ret) {
> > > > > +		DRM_DEBUG_KMS("\n8K Debug: Cannot assign genlock crtcs");
> > > > > +		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.
> > > > 
> > > > I thgink we should extend on this to make master/slave mode work by doing what matt roper was working on, in collaboration with Ville?
> > > > 
> > > > crtc->state points to drm_crtc_state, which is the uapi state, but also contains a shadow struct intel_crtc_state, which is the actual hw state. It has its own drm_crtc_state object for the real state, which gets copied after check.
> > > > 
> > > > This should then also be done for intel_plane_state, which will allow us to correctly handle this mode.
> > > 
> > > I don't think there's any linkage between this and the 2-pipe-1-port
> > > cases other than we might want to handle tiled displays that way one
> > > day to make life easier for userspace. But that has other issues such
> > > as we'd have to generate a new EDID for the whole display from the EDIDs
> > > of the tiles.
> > > 
> > > Also we maybe want port sync for 100% independent displays too. So IMO
> > > no point in trying to shoehorn both things into the same bucket.
> > 
> > I agree, also the tiled display case is simple in terms of the states since its 2 connectors and 2 crtcs exposed to the userspace
> > so the userspace will create two states and they are used as is for the two crtcs, the only difference is that we need to program
> > one as a slave and the other as master and find a way to store a pointer to master from the slave since we need to use that
> > to configure and enable transcoder port sync while enabling slave CRTC.
> > 
> > So on those lines, if i store the master transcoder directly in the slave crtc state then trans = 0 is valid for trans A then how
> > do I identify which crtc is slave?
> > Currently I check for !master_crtc and that tells me that it is not slave.
> 
> INVALID_TRANSCODDER or something?

Actually I just realized that in the hw_state_readout after i get the transcoder which would be same as pipe,
can I just use the intel_get_crtc_for_pipe() function to get the intel_crtc* which should match the master_crtc saved
in slave crtc_state, wha do you think?

Manasi

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

Ville Syrjälä June 13, 2019, 9:10 a.m. UTC | #9

On Wed, Jun 12, 2019 at 01:59:52PM -0700, Manasi Navare wrote:
> On Wed, Jun 12, 2019 at 10:30:55PM +0300, Ville Syrjälä wrote:
> > On Wed, Jun 12, 2019 at 12:11:02PM -0700, Manasi Navare wrote:
> > > On Wed, Jun 12, 2019 at 10:04:26PM +0300, Ville Syrjälä wrote:
> > > > On Wed, Jun 12, 2019 at 11:39:03AM +0200, Maarten Lankhorst wrote:
> > > > > Op 23-04-2019 om 17:48 schreef Manasi Navare:
> > > > > > 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 CRTC pointer in all the slave
> > > > > > CRTC states. This pointer is needed to select the master CRTC/transcoder
> > > > > > while configuring transcoder port sync for the corresponding slaves.
> > > > > >
> > > > > > 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]>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++++++++++++++++
> > > > > >  drivers/gpu/drm/i915/intel_drv.h     |  6 ++
> > > > > >  2 files changed, 95 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > > index b276345779e6..92dea2231499 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > @@ -11316,6 +11316,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
> > > > > >  	return 0;
> > > > > >  }
> > > > > >  
> > > > > > +static int icl_add_genlock_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;
> > > > > > +	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 (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > > > > > +		    connector->tile_v_loc == connector->num_v_tile - 1)
> > > > > > +			continue;
> > > > > > +		crtc_state->master_crtc = NULL;
> > > > > > +		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 add 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);
> > > > > > +
> > > > > > +		crtc_state->master_crtc = to_intel_crtc(master_crtc);
> > > > > > +		to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves |=
> > > > > > +			BIT(to_intel_crtc(crtc)->pipe);
> > > > > > +		DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC = %d\n, slave bitmast = %d",
> > > > > > +			      master_crtc->base.id,
> > > > > > +			      crtc_state->base.crtc->base.id,
> > > > > > +			      to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves);
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > >  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> > > > > >  				   struct drm_crtc_state *crtc_state)
> > > > > >  {
> > > > > > @@ -11795,6 +11875,9 @@ 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;
> > > > > > +	if (INTEL_GEN(dev_priv) >= 11)
> > > > > > +		saved_state->trans_port_sync_slaves =
> > > > > > +			crtc_state->trans_port_sync_slaves;
> > > > > >  
> > > > > >  	/* Keep base drm_crtc_state intact, only clear our extended struct */
> > > > > >  	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> > > > > > @@ -11888,6 +11971,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> > > > > >  	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
> > > > > >  			      CRTC_STEREO_DOUBLE);
> > > > > >  
> > > > > > +	ret = icl_add_genlock_crtcs(crtc, pipe_config, state);
> > > > > > +	if (ret) {
> > > > > > +		DRM_DEBUG_KMS("\n8K Debug: Cannot assign genlock crtcs");
> > > > > > +		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.
> > > > > 
> > > > > I thgink we should extend on this to make master/slave mode work by doing what matt roper was working on, in collaboration with Ville?
> > > > > 
> > > > > crtc->state points to drm_crtc_state, which is the uapi state, but also contains a shadow struct intel_crtc_state, which is the actual hw state. It has its own drm_crtc_state object for the real state, which gets copied after check.
> > > > > 
> > > > > This should then also be done for intel_plane_state, which will allow us to correctly handle this mode.
> > > > 
> > > > I don't think there's any linkage between this and the 2-pipe-1-port
> > > > cases other than we might want to handle tiled displays that way one
> > > > day to make life easier for userspace. But that has other issues such
> > > > as we'd have to generate a new EDID for the whole display from the EDIDs
> > > > of the tiles.
> > > > 
> > > > Also we maybe want port sync for 100% independent displays too. So IMO
> > > > no point in trying to shoehorn both things into the same bucket.
> > > 
> > > I agree, also the tiled display case is simple in terms of the states since its 2 connectors and 2 crtcs exposed to the userspace
> > > so the userspace will create two states and they are used as is for the two crtcs, the only difference is that we need to program
> > > one as a slave and the other as master and find a way to store a pointer to master from the slave since we need to use that
> > > to configure and enable transcoder port sync while enabling slave CRTC.
> > > 
> > > So on those lines, if i store the master transcoder directly in the slave crtc state then trans = 0 is valid for trans A then how
> > > do I identify which crtc is slave?
> > > Currently I check for !master_crtc and that tells me that it is not slave.
> > 
> > INVALID_TRANSCODDER or something?
> 
> Actually I just realized that in the hw_state_readout after i get the transcoder which would be same as pipe,
> can I just use the intel_get_crtc_for_pipe() function to get the intel_crtc* which should match the master_crtc saved
> in slave crtc_state, wha do you think?

Only if you ignore the fact that transcoder EDP can be the master.

Navare, Manasi June 13, 2019, 4:41 p.m. UTC | #10

On Thu, Jun 13, 2019 at 12:10:57PM +0300, Ville Syrjälä wrote:
> On Wed, Jun 12, 2019 at 01:59:52PM -0700, Manasi Navare wrote:
> > On Wed, Jun 12, 2019 at 10:30:55PM +0300, Ville Syrjälä wrote:
> > > On Wed, Jun 12, 2019 at 12:11:02PM -0700, Manasi Navare wrote:
> > > > On Wed, Jun 12, 2019 at 10:04:26PM +0300, Ville Syrjälä wrote:
> > > > > On Wed, Jun 12, 2019 at 11:39:03AM +0200, Maarten Lankhorst wrote:
> > > > > > Op 23-04-2019 om 17:48 schreef Manasi Navare:
> > > > > > > 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 CRTC pointer in all the slave
> > > > > > > CRTC states. This pointer is needed to select the master CRTC/transcoder
> > > > > > > while configuring transcoder port sync for the corresponding slaves.
> > > > > > >
> > > > > > > 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]>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++++++++++++++++
> > > > > > >  drivers/gpu/drm/i915/intel_drv.h     |  6 ++
> > > > > > >  2 files changed, 95 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > index b276345779e6..92dea2231499 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > @@ -11316,6 +11316,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
> > > > > > >  	return 0;
> > > > > > >  }
> > > > > > >  
> > > > > > > +static int icl_add_genlock_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;
> > > > > > > +	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 (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > > > > > > +		    connector->tile_v_loc == connector->num_v_tile - 1)
> > > > > > > +			continue;
> > > > > > > +		crtc_state->master_crtc = NULL;
> > > > > > > +		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 add 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);
> > > > > > > +
> > > > > > > +		crtc_state->master_crtc = to_intel_crtc(master_crtc);
> > > > > > > +		to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves |=
> > > > > > > +			BIT(to_intel_crtc(crtc)->pipe);
> > > > > > > +		DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC = %d\n, slave bitmast = %d",
> > > > > > > +			      master_crtc->base.id,
> > > > > > > +			      crtc_state->base.crtc->base.id,
> > > > > > > +			      to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves);
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > >  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> > > > > > >  				   struct drm_crtc_state *crtc_state)
> > > > > > >  {
> > > > > > > @@ -11795,6 +11875,9 @@ 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;
> > > > > > > +	if (INTEL_GEN(dev_priv) >= 11)
> > > > > > > +		saved_state->trans_port_sync_slaves =
> > > > > > > +			crtc_state->trans_port_sync_slaves;
> > > > > > >  
> > > > > > >  	/* Keep base drm_crtc_state intact, only clear our extended struct */
> > > > > > >  	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> > > > > > > @@ -11888,6 +11971,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> > > > > > >  	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
> > > > > > >  			      CRTC_STEREO_DOUBLE);
> > > > > > >  
> > > > > > > +	ret = icl_add_genlock_crtcs(crtc, pipe_config, state);
> > > > > > > +	if (ret) {
> > > > > > > +		DRM_DEBUG_KMS("\n8K Debug: Cannot assign genlock crtcs");
> > > > > > > +		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.
> > > > > > 
> > > > > > I thgink we should extend on this to make master/slave mode work by doing what matt roper was working on, in collaboration with Ville?
> > > > > > 
> > > > > > crtc->state points to drm_crtc_state, which is the uapi state, but also contains a shadow struct intel_crtc_state, which is the actual hw state. It has its own drm_crtc_state object for the real state, which gets copied after check.
> > > > > > 
> > > > > > This should then also be done for intel_plane_state, which will allow us to correctly handle this mode.
> > > > > 
> > > > > I don't think there's any linkage between this and the 2-pipe-1-port
> > > > > cases other than we might want to handle tiled displays that way one
> > > > > day to make life easier for userspace. But that has other issues such
> > > > > as we'd have to generate a new EDID for the whole display from the EDIDs
> > > > > of the tiles.
> > > > > 
> > > > > Also we maybe want port sync for 100% independent displays too. So IMO
> > > > > no point in trying to shoehorn both things into the same bucket.
> > > > 
> > > > I agree, also the tiled display case is simple in terms of the states since its 2 connectors and 2 crtcs exposed to the userspace
> > > > so the userspace will create two states and they are used as is for the two crtcs, the only difference is that we need to program
> > > > one as a slave and the other as master and find a way to store a pointer to master from the slave since we need to use that
> > > > to configure and enable transcoder port sync while enabling slave CRTC.
> > > > 
> > > > So on those lines, if i store the master transcoder directly in the slave crtc state then trans = 0 is valid for trans A then how
> > > > do I identify which crtc is slave?
> > > > Currently I check for !master_crtc and that tells me that it is not slave.
> > > 
> > > INVALID_TRANSCODDER or something?
> > 
> > Actually I just realized that in the hw_state_readout after i get the transcoder which would be same as pipe,
> > can I just use the intel_get_crtc_for_pipe() function to get the intel_crtc* which should match the master_crtc saved
> > in slave crtc_state, wha do you think?
> 
> Only if you ignore the fact that transcoder EDP can be the master.

Actually Transcoder EDP can never be the master so there should be a warning if TRANSCODER EDP gets set as a master in teh configuration time itself.

So ideally there are two options:
store master_pipe directly in slave crtc state and then use PIPE_INVALID as a check to find if its a slave or a master, that way the HW readout can be mapped to the corresponding pipe and compared
OR
store master_crtc like we are storing now and then use intel_get_crtc_from_pipe in HW state readout for the comparison

Which one would be a preferred way?

The other mismatch is in the trans_port_sync_slaves bitmask maintaining the list of slaves in the master_crtc_state, even for that in the HW state readout of the master CRTC there is no HW state/register that gives the associated slaves so nothing to compare against which creates the mismatch, how can this be solved?

As Matt suggested, may be we should do the pipe config compares differently for the ganged mode? Any ideas?

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

Ville Syrjälä June 13, 2019, 5:12 p.m. UTC | #11

On Thu, Jun 13, 2019 at 09:41:42AM -0700, Manasi Navare wrote:
> On Thu, Jun 13, 2019 at 12:10:57PM +0300, Ville Syrjälä wrote:
> > On Wed, Jun 12, 2019 at 01:59:52PM -0700, Manasi Navare wrote:
> > > On Wed, Jun 12, 2019 at 10:30:55PM +0300, Ville Syrjälä wrote:
> > > > On Wed, Jun 12, 2019 at 12:11:02PM -0700, Manasi Navare wrote:
> > > > > On Wed, Jun 12, 2019 at 10:04:26PM +0300, Ville Syrjälä wrote:
> > > > > > On Wed, Jun 12, 2019 at 11:39:03AM +0200, Maarten Lankhorst wrote:
> > > > > > > Op 23-04-2019 om 17:48 schreef Manasi Navare:
> > > > > > > > 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 CRTC pointer in all the slave
> > > > > > > > CRTC states. This pointer is needed to select the master CRTC/transcoder
> > > > > > > > while configuring transcoder port sync for the corresponding slaves.
> > > > > > > >
> > > > > > > > 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]>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++++++++++++++++
> > > > > > > >  drivers/gpu/drm/i915/intel_drv.h     |  6 ++
> > > > > > > >  2 files changed, 95 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > index b276345779e6..92dea2231499 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > @@ -11316,6 +11316,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
> > > > > > > >  	return 0;
> > > > > > > >  }
> > > > > > > >  
> > > > > > > > +static int icl_add_genlock_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;
> > > > > > > > +	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 (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > > > > > > > +		    connector->tile_v_loc == connector->num_v_tile - 1)
> > > > > > > > +			continue;
> > > > > > > > +		crtc_state->master_crtc = NULL;
> > > > > > > > +		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 add 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);
> > > > > > > > +
> > > > > > > > +		crtc_state->master_crtc = to_intel_crtc(master_crtc);
> > > > > > > > +		to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves |=
> > > > > > > > +			BIT(to_intel_crtc(crtc)->pipe);
> > > > > > > > +		DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC = %d\n, slave bitmast = %d",
> > > > > > > > +			      master_crtc->base.id,
> > > > > > > > +			      crtc_state->base.crtc->base.id,
> > > > > > > > +			      to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves);
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> > > > > > > >  				   struct drm_crtc_state *crtc_state)
> > > > > > > >  {
> > > > > > > > @@ -11795,6 +11875,9 @@ 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;
> > > > > > > > +	if (INTEL_GEN(dev_priv) >= 11)
> > > > > > > > +		saved_state->trans_port_sync_slaves =
> > > > > > > > +			crtc_state->trans_port_sync_slaves;
> > > > > > > >  
> > > > > > > >  	/* Keep base drm_crtc_state intact, only clear our extended struct */
> > > > > > > >  	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> > > > > > > > @@ -11888,6 +11971,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> > > > > > > >  	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
> > > > > > > >  			      CRTC_STEREO_DOUBLE);
> > > > > > > >  
> > > > > > > > +	ret = icl_add_genlock_crtcs(crtc, pipe_config, state);
> > > > > > > > +	if (ret) {
> > > > > > > > +		DRM_DEBUG_KMS("\n8K Debug: Cannot assign genlock crtcs");
> > > > > > > > +		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.
> > > > > > > 
> > > > > > > I thgink we should extend on this to make master/slave mode work by doing what matt roper was working on, in collaboration with Ville?
> > > > > > > 
> > > > > > > crtc->state points to drm_crtc_state, which is the uapi state, but also contains a shadow struct intel_crtc_state, which is the actual hw state. It has its own drm_crtc_state object for the real state, which gets copied after check.
> > > > > > > 
> > > > > > > This should then also be done for intel_plane_state, which will allow us to correctly handle this mode.
> > > > > > 
> > > > > > I don't think there's any linkage between this and the 2-pipe-1-port
> > > > > > cases other than we might want to handle tiled displays that way one
> > > > > > day to make life easier for userspace. But that has other issues such
> > > > > > as we'd have to generate a new EDID for the whole display from the EDIDs
> > > > > > of the tiles.
> > > > > > 
> > > > > > Also we maybe want port sync for 100% independent displays too. So IMO
> > > > > > no point in trying to shoehorn both things into the same bucket.
> > > > > 
> > > > > I agree, also the tiled display case is simple in terms of the states since its 2 connectors and 2 crtcs exposed to the userspace
> > > > > so the userspace will create two states and they are used as is for the two crtcs, the only difference is that we need to program
> > > > > one as a slave and the other as master and find a way to store a pointer to master from the slave since we need to use that
> > > > > to configure and enable transcoder port sync while enabling slave CRTC.
> > > > > 
> > > > > So on those lines, if i store the master transcoder directly in the slave crtc state then trans = 0 is valid for trans A then how
> > > > > do I identify which crtc is slave?
> > > > > Currently I check for !master_crtc and that tells me that it is not slave.
> > > > 
> > > > INVALID_TRANSCODDER or something?
> > > 
> > > Actually I just realized that in the hw_state_readout after i get the transcoder which would be same as pipe,
> > > can I just use the intel_get_crtc_for_pipe() function to get the intel_crtc* which should match the master_crtc saved
> > > in slave crtc_state, wha do you think?
> > 
> > Only if you ignore the fact that transcoder EDP can be the master.
> 
> Actually Transcoder EDP can never be the master so there should be a warning if TRANSCODER EDP gets set as a master in teh configuration time itself.

IIRC the docs say it can be master, but not slave.

Navare, Manasi June 13, 2019, 8:13 p.m. UTC | #12

On Thu, Jun 13, 2019 at 08:12:37PM +0300, Ville Syrjälä wrote:
> On Thu, Jun 13, 2019 at 09:41:42AM -0700, Manasi Navare wrote:
> > On Thu, Jun 13, 2019 at 12:10:57PM +0300, Ville Syrjälä wrote:
> > > On Wed, Jun 12, 2019 at 01:59:52PM -0700, Manasi Navare wrote:
> > > > On Wed, Jun 12, 2019 at 10:30:55PM +0300, Ville Syrjälä wrote:
> > > > > On Wed, Jun 12, 2019 at 12:11:02PM -0700, Manasi Navare wrote:
> > > > > > On Wed, Jun 12, 2019 at 10:04:26PM +0300, Ville Syrjälä wrote:
> > > > > > > On Wed, Jun 12, 2019 at 11:39:03AM +0200, Maarten Lankhorst wrote:
> > > > > > > > Op 23-04-2019 om 17:48 schreef Manasi Navare:
> > > > > > > > > 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 CRTC pointer in all the slave
> > > > > > > > > CRTC states. This pointer is needed to select the master CRTC/transcoder
> > > > > > > > > while configuring transcoder port sync for the corresponding slaves.
> > > > > > > > >
> > > > > > > > > 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]>
> > > > > > > > > ---
> > > > > > > > >  drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++++++++++++++++
> > > > > > > > >  drivers/gpu/drm/i915/intel_drv.h     |  6 ++
> > > > > > > > >  2 files changed, 95 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > > index b276345779e6..92dea2231499 100644
> > > > > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > > @@ -11316,6 +11316,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
> > > > > > > > >  	return 0;
> > > > > > > > >  }
> > > > > > > > >  
> > > > > > > > > +static int icl_add_genlock_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;
> > > > > > > > > +	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 (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > > > > > > > > +		    connector->tile_v_loc == connector->num_v_tile - 1)
> > > > > > > > > +			continue;
> > > > > > > > > +		crtc_state->master_crtc = NULL;
> > > > > > > > > +		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 add 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);
> > > > > > > > > +
> > > > > > > > > +		crtc_state->master_crtc = to_intel_crtc(master_crtc);
> > > > > > > > > +		to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves |=
> > > > > > > > > +			BIT(to_intel_crtc(crtc)->pipe);
> > > > > > > > > +		DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC = %d\n, slave bitmast = %d",
> > > > > > > > > +			      master_crtc->base.id,
> > > > > > > > > +			      crtc_state->base.crtc->base.id,
> > > > > > > > > +			      to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves);
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	return 0;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> > > > > > > > >  				   struct drm_crtc_state *crtc_state)
> > > > > > > > >  {
> > > > > > > > > @@ -11795,6 +11875,9 @@ 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;
> > > > > > > > > +	if (INTEL_GEN(dev_priv) >= 11)
> > > > > > > > > +		saved_state->trans_port_sync_slaves =
> > > > > > > > > +			crtc_state->trans_port_sync_slaves;
> > > > > > > > >  
> > > > > > > > >  	/* Keep base drm_crtc_state intact, only clear our extended struct */
> > > > > > > > >  	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> > > > > > > > > @@ -11888,6 +11971,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> > > > > > > > >  	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
> > > > > > > > >  			      CRTC_STEREO_DOUBLE);
> > > > > > > > >  
> > > > > > > > > +	ret = icl_add_genlock_crtcs(crtc, pipe_config, state);
> > > > > > > > > +	if (ret) {
> > > > > > > > > +		DRM_DEBUG_KMS("\n8K Debug: Cannot assign genlock crtcs");
> > > > > > > > > +		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.
> > > > > > > > 
> > > > > > > > I thgink we should extend on this to make master/slave mode work by doing what matt roper was working on, in collaboration with Ville?
> > > > > > > > 
> > > > > > > > crtc->state points to drm_crtc_state, which is the uapi state, but also contains a shadow struct intel_crtc_state, which is the actual hw state. It has its own drm_crtc_state object for the real state, which gets copied after check.
> > > > > > > > 
> > > > > > > > This should then also be done for intel_plane_state, which will allow us to correctly handle this mode.
> > > > > > > 
> > > > > > > I don't think there's any linkage between this and the 2-pipe-1-port
> > > > > > > cases other than we might want to handle tiled displays that way one
> > > > > > > day to make life easier for userspace. But that has other issues such
> > > > > > > as we'd have to generate a new EDID for the whole display from the EDIDs
> > > > > > > of the tiles.
> > > > > > > 
> > > > > > > Also we maybe want port sync for 100% independent displays too. So IMO
> > > > > > > no point in trying to shoehorn both things into the same bucket.
> > > > > > 
> > > > > > I agree, also the tiled display case is simple in terms of the states since its 2 connectors and 2 crtcs exposed to the userspace
> > > > > > so the userspace will create two states and they are used as is for the two crtcs, the only difference is that we need to program
> > > > > > one as a slave and the other as master and find a way to store a pointer to master from the slave since we need to use that
> > > > > > to configure and enable transcoder port sync while enabling slave CRTC.
> > > > > > 
> > > > > > So on those lines, if i store the master transcoder directly in the slave crtc state then trans = 0 is valid for trans A then how
> > > > > > do I identify which crtc is slave?
> > > > > > Currently I check for !master_crtc and that tells me that it is not slave.
> > > > > 
> > > > > INVALID_TRANSCODDER or something?
> > > > 
> > > > Actually I just realized that in the hw_state_readout after i get the transcoder which would be same as pipe,
> > > > can I just use the intel_get_crtc_for_pipe() function to get the intel_crtc* which should match the master_crtc saved
> > > > in slave crtc_state, wha do you think?
> > > 
> > > Only if you ignore the fact that transcoder EDP can be the master.
> > 
> > Actually Transcoder EDP can never be the master so there should be a warning if TRANSCODER EDP gets set as a master in teh configuration time itself.
> 
> IIRC the docs say it can be master, but not slave.

So its probably better to just store the master cpu transcoder directly in the slave crtc state. In that case, I will have to add TRANSCODER_INVALID = -1
to the enum transcoder just like we do for the PIPE_INVALID and then I can use TRANSCODER_INVALID to  decide whether its a slave or master.

Also for trans_port_sync_slaves bitmask, in the HW state readout for master CRTC, I will loop through all the active transcoders and find the pipe for the
transcoders that have port sync enable set in their TRANS_PORT_SYNC reg

Does this sound like a correct approach?

Manasi

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

Navare, Manasi June 21, 2019, 6:40 p.m. UTC | #13

As per the feedback, I tested the code with saving master_transcoder instead of master_crtc
and in the HW state readout I loop through all transcoders to fill the trans_port_sync_slaves bitmask.
This is working great and i dont see any pipe state mismatches now.

Thanks for all the feedback and I will be sending the new patch revision out later today.

Regards
Manasi

On Thu, Jun 13, 2019 at 01:13:28PM -0700, Manasi Navare wrote:
> On Thu, Jun 13, 2019 at 08:12:37PM +0300, Ville Syrjälä wrote:
> > On Thu, Jun 13, 2019 at 09:41:42AM -0700, Manasi Navare wrote:
> > > On Thu, Jun 13, 2019 at 12:10:57PM +0300, Ville Syrjälä wrote:
> > > > On Wed, Jun 12, 2019 at 01:59:52PM -0700, Manasi Navare wrote:
> > > > > On Wed, Jun 12, 2019 at 10:30:55PM +0300, Ville Syrjälä wrote:
> > > > > > On Wed, Jun 12, 2019 at 12:11:02PM -0700, Manasi Navare wrote:
> > > > > > > On Wed, Jun 12, 2019 at 10:04:26PM +0300, Ville Syrjälä wrote:
> > > > > > > > On Wed, Jun 12, 2019 at 11:39:03AM +0200, Maarten Lankhorst wrote:
> > > > > > > > > Op 23-04-2019 om 17:48 schreef Manasi Navare:
> > > > > > > > > > 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 CRTC pointer in all the slave
> > > > > > > > > > CRTC states. This pointer is needed to select the master CRTC/transcoder
> > > > > > > > > > while configuring transcoder port sync for the corresponding slaves.
> > > > > > > > > >
> > > > > > > > > > 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]>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++++++++++++++++
> > > > > > > > > >  drivers/gpu/drm/i915/intel_drv.h     |  6 ++
> > > > > > > > > >  2 files changed, 95 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > > > index b276345779e6..92dea2231499 100644
> > > > > > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > > > @@ -11316,6 +11316,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
> > > > > > > > > >  	return 0;
> > > > > > > > > >  }
> > > > > > > > > >  
> > > > > > > > > > +static int icl_add_genlock_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;
> > > > > > > > > > +	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 (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > > > > > > > > > +		    connector->tile_v_loc == connector->num_v_tile - 1)
> > > > > > > > > > +			continue;
> > > > > > > > > > +		crtc_state->master_crtc = NULL;
> > > > > > > > > > +		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 add 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);
> > > > > > > > > > +
> > > > > > > > > > +		crtc_state->master_crtc = to_intel_crtc(master_crtc);
> > > > > > > > > > +		to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves |=
> > > > > > > > > > +			BIT(to_intel_crtc(crtc)->pipe);
> > > > > > > > > > +		DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC = %d\n, slave bitmast = %d",
> > > > > > > > > > +			      master_crtc->base.id,
> > > > > > > > > > +			      crtc_state->base.crtc->base.id,
> > > > > > > > > > +			      to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves);
> > > > > > > > > > +	}
> > > > > > > > > > +
> > > > > > > > > > +	return 0;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > >  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> > > > > > > > > >  				   struct drm_crtc_state *crtc_state)
> > > > > > > > > >  {
> > > > > > > > > > @@ -11795,6 +11875,9 @@ 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;
> > > > > > > > > > +	if (INTEL_GEN(dev_priv) >= 11)
> > > > > > > > > > +		saved_state->trans_port_sync_slaves =
> > > > > > > > > > +			crtc_state->trans_port_sync_slaves;
> > > > > > > > > >  
> > > > > > > > > >  	/* Keep base drm_crtc_state intact, only clear our extended struct */
> > > > > > > > > >  	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> > > > > > > > > > @@ -11888,6 +11971,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> > > > > > > > > >  	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
> > > > > > > > > >  			      CRTC_STEREO_DOUBLE);
> > > > > > > > > >  
> > > > > > > > > > +	ret = icl_add_genlock_crtcs(crtc, pipe_config, state);
> > > > > > > > > > +	if (ret) {
> > > > > > > > > > +		DRM_DEBUG_KMS("\n8K Debug: Cannot assign genlock crtcs");
> > > > > > > > > > +		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.
> > > > > > > > > 
> > > > > > > > > I thgink we should extend on this to make master/slave mode work by doing what matt roper was working on, in collaboration with Ville?
> > > > > > > > > 
> > > > > > > > > crtc->state points to drm_crtc_state, which is the uapi state, but also contains a shadow struct intel_crtc_state, which is the actual hw state. It has its own drm_crtc_state object for the real state, which gets copied after check.
> > > > > > > > > 
> > > > > > > > > This should then also be done for intel_plane_state, which will allow us to correctly handle this mode.
> > > > > > > > 
> > > > > > > > I don't think there's any linkage between this and the 2-pipe-1-port
> > > > > > > > cases other than we might want to handle tiled displays that way one
> > > > > > > > day to make life easier for userspace. But that has other issues such
> > > > > > > > as we'd have to generate a new EDID for the whole display from the EDIDs
> > > > > > > > of the tiles.
> > > > > > > > 
> > > > > > > > Also we maybe want port sync for 100% independent displays too. So IMO
> > > > > > > > no point in trying to shoehorn both things into the same bucket.
> > > > > > > 
> > > > > > > I agree, also the tiled display case is simple in terms of the states since its 2 connectors and 2 crtcs exposed to the userspace
> > > > > > > so the userspace will create two states and they are used as is for the two crtcs, the only difference is that we need to program
> > > > > > > one as a slave and the other as master and find a way to store a pointer to master from the slave since we need to use that
> > > > > > > to configure and enable transcoder port sync while enabling slave CRTC.
> > > > > > > 
> > > > > > > So on those lines, if i store the master transcoder directly in the slave crtc state then trans = 0 is valid for trans A then how
> > > > > > > do I identify which crtc is slave?
> > > > > > > Currently I check for !master_crtc and that tells me that it is not slave.
> > > > > > 
> > > > > > INVALID_TRANSCODDER or something?
> > > > > 
> > > > > Actually I just realized that in the hw_state_readout after i get the transcoder which would be same as pipe,
> > > > > can I just use the intel_get_crtc_for_pipe() function to get the intel_crtc* which should match the master_crtc saved
> > > > > in slave crtc_state, wha do you think?
> > > > 
> > > > Only if you ignore the fact that transcoder EDP can be the master.
> > > 
> > > Actually Transcoder EDP can never be the master so there should be a warning if TRANSCODER EDP gets set as a master in teh configuration time itself.
> > 
> > IIRC the docs say it can be master, but not slave.
> 
> So its probably better to just store the master cpu transcoder directly in the slave crtc state. In that case, I will have to add TRANSCODER_INVALID = -1
> to the enum transcoder just like we do for the PIPE_INVALID and then I can use TRANSCODER_INVALID to  decide whether its a slave or master.
> 
> Also for trans_port_sync_slaves bitmask, in the HW state readout for master CRTC, I will loop through all the active transcoders and find the pipe for the
> transcoders that have port sync enable set in their TRANS_PORT_SYNC reg
> 
> Does this sound like a correct approach?
> 
> Manasi
> 
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b276345779e6..92dea2231499 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11316,6 +11316,86 @@  static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
 	return 0;
 }
 
+static int icl_add_genlock_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;
+	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 (connector->tile_h_loc == connector->num_h_tile - 1 &&
+		    connector->tile_v_loc == connector->num_v_tile - 1)
+			continue;
+		crtc_state->master_crtc = NULL;
+		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 add 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);
+
+		crtc_state->master_crtc = to_intel_crtc(master_crtc);
+		to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves |=
+			BIT(to_intel_crtc(crtc)->pipe);
+		DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC = %d\n, slave bitmast = %d",
+			      master_crtc->base.id,
+			      crtc_state->base.crtc->base.id,
+			      to_intel_crtc_state(master_crtc_state)->trans_port_sync_slaves);
+	}
+
+	return 0;
+}
+
 static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 				   struct drm_crtc_state *crtc_state)
 {
@@ -11795,6 +11875,9 @@  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;
+	if (INTEL_GEN(dev_priv) >= 11)
+		saved_state->trans_port_sync_slaves =
+			crtc_state->trans_port_sync_slaves;
 
 	/* Keep base drm_crtc_state intact, only clear our extended struct */
 	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
@@ -11888,6 +11971,12 @@  intel_modeset_pipe_config(struct drm_crtc *crtc,
 	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
 			      CRTC_STEREO_DOUBLE);
 
+	ret = icl_add_genlock_crtcs(crtc, pipe_config, state);
+	if (ret) {
+		DRM_DEBUG_KMS("\n8K Debug: Cannot assign genlock crtcs");
+		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.
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a38b9cff5cd0..8ae9cb662e28 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1082,6 +1082,12 @@  struct intel_crtc_state {
 
 	/* Forward Error correction State */
 	bool fec_enable;
+
+	/* Pointer to master crtc in case of tiled displays */
+	struct intel_crtc *master_crtc;
+
+	/* Bitmask to indicate slaves attached */
+	u8 trans_port_sync_slaves;
 };
 
 struct intel_crtc {