[PATCH v2 0/4] Enable Transcoder Port Sync feature for Tiled displays

[PATCH v2 0/4] Enable Transcoder Port Sync feature for Tiled displays

[Manasi Navare]

In case of tiled displays, each tile is sent across a separate CRTC and a
separate port/connector connected to the monitor. In this case we need to make
sure that the timings across these transcoders/ports are synchronized else
the two tile displays can be off.

Transcoder Port Sync is a transcoder level feature. This mode forces
two or more transcoders to be in sync with one transcoder master
and one or more transcoder slaves. In the case of DP/eDP, the master
is unaware that it is operating in Port Sync mode, only the slave is aware
that it is operating in this mode. Hence in this configuration, we need
to do a mdoeset on slaves first and configure the port sync in slave
transcoder.


Manasi Navare (4):
  drm/i915/icl: Assign Master slave crtc links for Transcoder Port Sync
  drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across
    separate ports
  drm/i915/dp: Trigger modeset if master_crtc or slaves bitmask changes
  drm/i915/dp: Enable master-slaves in trans port sync mode in correct
    order

 drivers/gpu/drm/i915/intel_ddi.c     |   3 +-
 drivers/gpu/drm/i915/intel_display.c | 274 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |   6 +
 3 files changed, 282 insertions(+), 1 deletion(-)

-- 
2.19.1

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

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

[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 <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 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.
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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Srivatsa, Anusha]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>Manasi Navare
>Sent: Tuesday, April 23, 2019 8:49 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>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 <daniel.vetter@ffwll.ch>
>Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>Cc: Matt Roper <matthew.d.roper@intel.com>
>Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
>---
> 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
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Manasi Navare]

On Fri, May 03, 2019 at 05:09:20PM -0700, Srivatsa, Anusha wrote:
> 
> 
> >-----Original Message-----
> >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> >Manasi Navare
> >Sent: Tuesday, April 23, 2019 8:49 AM
> >To: intel-gfx@lists.freedesktop.org
> >Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >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 <daniel.vetter@ffwll.ch>
> >Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >Cc: Matt Roper <matthew.d.roper@intel.com>
> >Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> >---
> > 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
> >Intel-gfx@lists.freedesktop.org
> >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Manasi Navare]

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 <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

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 <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  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 {


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

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

[Ville Syrjälä]

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 <daniel.vetter@ffwll.ch>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  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.

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

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

[Manasi Navare]

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 <daniel.vetter@ffwll.ch>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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 <daniel.vetter@ffwll.ch>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  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?

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

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

[Manasi Navare]

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 <daniel.vetter@ffwll.ch>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > >  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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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 <daniel.vetter@ffwll.ch>
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > ---
> > > > > >  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.

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

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

[Manasi Navare]

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 <daniel.vetter@ffwll.ch>
> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > > ---
> > > > > > >  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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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 <daniel.vetter@ffwll.ch>
> > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > > > ---
> > > > > > > >  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.

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

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

[Manasi Navare]

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 <daniel.vetter@ffwll.ch>
> > > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > > > > ---
> > > > > > > > >  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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Manasi Navare]

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 <daniel.vetter@ffwll.ch>
> > > > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > > > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > > > > > ---
> > > > > > > > > >  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
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/4] drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports

[Manasi Navare]

In case of tiled displays where different tiles are displayed across
different ports, we need to synchronize the transcoders involved.
This patch implements the transcoder port sync feature for
synchronizing one master transcoder with one or more slave
transcoders. This is only enbaled in slave transcoder
and the master transcoder is unaware that it is operating
in this mode.
This has been tested with tiled display connected to ICL.

v2:
* Do not use RMW, just write to the register in commit (Jani N)

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 92dea2231499..81e8cb9fe221 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4020,6 +4020,61 @@ static void icl_set_pipe_chicken(struct intel_crtc *crtc)
 	I915_WRITE(PIPE_CHICKEN(pipe), tmp);
 }
 
+static void icl_set_transcoder_port_sync(struct intel_atomic_state *old_intel_state,
+					 const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	struct intel_crtc_state *master_crtc_state = NULL;
+	enum transcoder master_transcoder;
+	u32 trans_ddi_func_ctl2_val;
+	u8 master_select;
+
+	/*
+	 * Port Sync Mode cannot be enabled for DP MST
+	 */
+	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST))
+		return;
+
+	/*
+	 * Configure the master select and enable Transcoder Port Sync for
+	 * Slave CRTCs transcoder.
+	 */
+	if (!crtc_state->master_crtc)
+		return;
+
+	master_crtc_state = intel_atomic_get_new_crtc_state(old_intel_state,
+							    crtc_state->master_crtc);
+	if (WARN_ON(!master_crtc_state))
+		return;
+
+	master_transcoder = master_crtc_state->cpu_transcoder;
+	switch (master_transcoder) {
+	case TRANSCODER_A:
+		master_select = 1;
+		break;
+	case TRANSCODER_B:
+		master_select = 2;
+		break;
+	case TRANSCODER_C:
+		master_select = 3;
+		break;
+	case TRANSCODER_EDP:
+	default:
+		master_select = 0;
+		break;
+	}
+	/* Set the master select bits for Tranascoder Port Sync */
+	trans_ddi_func_ctl2_val = (PORT_SYNC_MODE_MASTER_SELECT(master_select) &
+				   PORT_SYNC_MODE_MASTER_SELECT_MASK) <<
+		PORT_SYNC_MODE_MASTER_SELECT_SHIFT;
+	/* Enable Transcoder Port Sync */
+	trans_ddi_func_ctl2_val |= PORT_SYNC_MODE_ENABLE;
+
+	I915_WRITE(TRANS_DDI_FUNC_CTL2(crtc_state->cpu_transcoder),
+		   trans_ddi_func_ctl2_val);
+}
+
 static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_state,
 				     const struct intel_crtc_state *new_crtc_state)
 {
@@ -5971,6 +6026,9 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
 	if (!transcoder_is_dsi(cpu_transcoder))
 		intel_set_pipe_timings(pipe_config);
 
+	if (INTEL_GEN(dev_priv) >= 11)
+		icl_set_transcoder_port_sync(old_intel_state, pipe_config);
+
 	intel_set_pipe_src_size(pipe_config);
 
 	if (cpu_transcoder != TRANSCODER_EDP &&
-- 
2.19.1

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

Re: [PATCH v2 2/4] drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports

[Srivatsa, Anusha]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>Manasi Navare
>Sent: Tuesday, April 23, 2019 8:49 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Nikula, Jani <jani.nikula@intel.com>; Vetter, Daniel
><daniel.vetter@intel.com>
>Subject: [Intel-gfx] [PATCH v2 2/4] drm/i915/icl: Enable TRANSCODER PORT SYNC
>for tiled displays across separate ports
>
>In case of tiled displays where different tiles are displayed across different ports,
>we need to synchronize the transcoders involved.
>This patch implements the transcoder port sync feature for synchronizing one
>master transcoder with one or more slave transcoders. This is only enbaled in
								   ^^^typo

Anusha 	
>slave transcoder and the master transcoder is unaware that it is operating in this
>mode.
>This has been tested with tiled display connected to ICL.
>
>v2:
>* Do not use RMW, just write to the register in commit (Jani N)
>
>Cc: Daniel Vetter <daniel.vetter@intel.com>
>Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>Cc: Matt Roper <matthew.d.roper@intel.com>
>Cc: Jani Nikula <jani.nikula@intel.com>
>Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
>---
> drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/intel_display.c
>b/drivers/gpu/drm/i915/intel_display.c
>index 92dea2231499..81e8cb9fe221 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -4020,6 +4020,61 @@ static void icl_set_pipe_chicken(struct intel_crtc
>*crtc)
> 	I915_WRITE(PIPE_CHICKEN(pipe), tmp);
> }
>
>+static void icl_set_transcoder_port_sync(struct intel_atomic_state
>*old_intel_state,
>+					 const struct intel_crtc_state
>*crtc_state) {
>+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>+	struct intel_crtc_state *master_crtc_state = NULL;
>+	enum transcoder master_transcoder;
>+	u32 trans_ddi_func_ctl2_val;
>+	u8 master_select;
>+
>+	/*
>+	 * Port Sync Mode cannot be enabled for DP MST
>+	 */
>+	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST))
>+		return;
>+
>+	/*
>+	 * Configure the master select and enable Transcoder Port Sync for
>+	 * Slave CRTCs transcoder.
>+	 */
>+	if (!crtc_state->master_crtc)
>+		return;
>+
>+	master_crtc_state = intel_atomic_get_new_crtc_state(old_intel_state,
>+							    crtc_state-
>>master_crtc);
>+	if (WARN_ON(!master_crtc_state))
>+		return;
>+
>+	master_transcoder = master_crtc_state->cpu_transcoder;
>+	switch (master_transcoder) {
>+	case TRANSCODER_A:
>+		master_select = 1;
>+		break;
>+	case TRANSCODER_B:
>+		master_select = 2;
>+		break;
>+	case TRANSCODER_C:
>+		master_select = 3;
>+		break;
>+	case TRANSCODER_EDP:
>+	default:
>+		master_select = 0;
>+		break;
>+	}
>+	/* Set the master select bits for Tranascoder Port Sync */
>+	trans_ddi_func_ctl2_val =
>(PORT_SYNC_MODE_MASTER_SELECT(master_select) &
>+				   PORT_SYNC_MODE_MASTER_SELECT_MASK)
><<
>+		PORT_SYNC_MODE_MASTER_SELECT_SHIFT;
>+	/* Enable Transcoder Port Sync */
>+	trans_ddi_func_ctl2_val |= PORT_SYNC_MODE_ENABLE;
>+
>+	I915_WRITE(TRANS_DDI_FUNC_CTL2(crtc_state->cpu_transcoder),
>+		   trans_ddi_func_ctl2_val);
>+}
>+
> static void intel_update_pipe_config(const struct intel_crtc_state
>*old_crtc_state,
> 				     const struct intel_crtc_state
>*new_crtc_state)  { @@ -5971,6 +6026,9 @@ static void
>haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> 	if (!transcoder_is_dsi(cpu_transcoder))
> 		intel_set_pipe_timings(pipe_config);
>
>+	if (INTEL_GEN(dev_priv) >= 11)
>+		icl_set_transcoder_port_sync(old_intel_state, pipe_config);
>+
> 	intel_set_pipe_src_size(pipe_config);
>
> 	if (cpu_transcoder != TRANSCODER_EDP &&
>--
>2.19.1
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/4] drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports

[Manasi Navare]

On Fri, May 03, 2019 at 05:13:47PM -0700, Srivatsa, Anusha wrote:
> 
> 
> >-----Original Message-----
> >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> >Manasi Navare
> >Sent: Tuesday, April 23, 2019 8:49 AM
> >To: intel-gfx@lists.freedesktop.org
> >Cc: Nikula, Jani <jani.nikula@intel.com>; Vetter, Daniel
> ><daniel.vetter@intel.com>
> >Subject: [Intel-gfx] [PATCH v2 2/4] drm/i915/icl: Enable TRANSCODER PORT SYNC
> >for tiled displays across separate ports
> >
> >In case of tiled displays where different tiles are displayed across different ports,
> >we need to synchronize the transcoders involved.
> >This patch implements the transcoder port sync feature for synchronizing one
> >master transcoder with one or more slave transcoders. This is only enbaled in
> 								   ^^^typo
> 

Thanks for the catch, will fix this in the next revision.
Does the actual port sync bit and master select setting part in the register look good?

Manasi


> Anusha 	
> >slave transcoder and the master transcoder is unaware that it is operating in this
> >mode.
> >This has been tested with tiled display connected to ICL.
> >
> >v2:
> >* Do not use RMW, just write to the register in commit (Jani N)
> >
> >Cc: Daniel Vetter <daniel.vetter@intel.com>
> >Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >Cc: Matt Roper <matthew.d.roper@intel.com>
> >Cc: Jani Nikula <jani.nikula@intel.com>
> >Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> >---
> > drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++
> > 1 file changed, 58 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_display.c
> >b/drivers/gpu/drm/i915/intel_display.c
> >index 92dea2231499..81e8cb9fe221 100644
> >--- a/drivers/gpu/drm/i915/intel_display.c
> >+++ b/drivers/gpu/drm/i915/intel_display.c
> >@@ -4020,6 +4020,61 @@ static void icl_set_pipe_chicken(struct intel_crtc
> >*crtc)
> > 	I915_WRITE(PIPE_CHICKEN(pipe), tmp);
> > }
> >
> >+static void icl_set_transcoder_port_sync(struct intel_atomic_state
> >*old_intel_state,
> >+					 const struct intel_crtc_state
> >*crtc_state) {
> >+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >+	struct intel_crtc_state *master_crtc_state = NULL;
> >+	enum transcoder master_transcoder;
> >+	u32 trans_ddi_func_ctl2_val;
> >+	u8 master_select;
> >+
> >+	/*
> >+	 * Port Sync Mode cannot be enabled for DP MST
> >+	 */
> >+	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST))
> >+		return;
> >+
> >+	/*
> >+	 * Configure the master select and enable Transcoder Port Sync for
> >+	 * Slave CRTCs transcoder.
> >+	 */
> >+	if (!crtc_state->master_crtc)
> >+		return;
> >+
> >+	master_crtc_state = intel_atomic_get_new_crtc_state(old_intel_state,
> >+							    crtc_state-
> >>master_crtc);
> >+	if (WARN_ON(!master_crtc_state))
> >+		return;
> >+
> >+	master_transcoder = master_crtc_state->cpu_transcoder;
> >+	switch (master_transcoder) {
> >+	case TRANSCODER_A:
> >+		master_select = 1;
> >+		break;
> >+	case TRANSCODER_B:
> >+		master_select = 2;
> >+		break;
> >+	case TRANSCODER_C:
> >+		master_select = 3;
> >+		break;
> >+	case TRANSCODER_EDP:
> >+	default:
> >+		master_select = 0;
> >+		break;
> >+	}
> >+	/* Set the master select bits for Tranascoder Port Sync */
> >+	trans_ddi_func_ctl2_val =
> >(PORT_SYNC_MODE_MASTER_SELECT(master_select) &
> >+				   PORT_SYNC_MODE_MASTER_SELECT_MASK)
> ><<
> >+		PORT_SYNC_MODE_MASTER_SELECT_SHIFT;
> >+	/* Enable Transcoder Port Sync */
> >+	trans_ddi_func_ctl2_val |= PORT_SYNC_MODE_ENABLE;
> >+
> >+	I915_WRITE(TRANS_DDI_FUNC_CTL2(crtc_state->cpu_transcoder),
> >+		   trans_ddi_func_ctl2_val);
> >+}
> >+
> > static void intel_update_pipe_config(const struct intel_crtc_state
> >*old_crtc_state,
> > 				     const struct intel_crtc_state
> >*new_crtc_state)  { @@ -5971,6 +6026,9 @@ static void
> >haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> > 	if (!transcoder_is_dsi(cpu_transcoder))
> > 		intel_set_pipe_timings(pipe_config);
> >
> >+	if (INTEL_GEN(dev_priv) >= 11)
> >+		icl_set_transcoder_port_sync(old_intel_state, pipe_config);
> >+
> > 	intel_set_pipe_src_size(pipe_config);
> >
> > 	if (cpu_transcoder != TRANSCODER_EDP &&
> >--
> >2.19.1
> >
> >_______________________________________________
> >Intel-gfx mailing list
> >Intel-gfx@lists.freedesktop.org
> >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/4] drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports

[Maarten Lankhorst]

Op 23-04-2019 om 17:48 schreef Manasi Navare:
> In case of tiled displays where different tiles are displayed across
> different ports, we need to synchronize the transcoders involved.
> This patch implements the transcoder port sync feature for
> synchronizing one master transcoder with one or more slave
> transcoders. This is only enbaled in slave transcoder
> and the master transcoder is unaware that it is operating
> in this mode.
> This has been tested with tiled display connected to ICL.
>
> v2:
> * Do not use RMW, just write to the register in commit (Jani N)
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 92dea2231499..81e8cb9fe221 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4020,6 +4020,61 @@ static void icl_set_pipe_chicken(struct intel_crtc *crtc)
>  	I915_WRITE(PIPE_CHICKEN(pipe), tmp);
>  }
>  
> +static void icl_set_transcoder_port_sync(struct intel_atomic_state *old_intel_state,
> +					 const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct intel_crtc_state *master_crtc_state = NULL;
> +	enum transcoder master_transcoder;
> +	u32 trans_ddi_func_ctl2_val;
> +	u8 master_select;
> +
> +	/*
> +	 * Port Sync Mode cannot be enabled for DP MST
> +	 */
> +	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST))
> +		return;
Surely we should reject this case before we get here in 1/4? This should be a WARN_ON below the !master_crtc check at most.
> +	/*
> +	 * Configure the master select and enable Transcoder Port Sync for
> +	 * Slave CRTCs transcoder.
> +	 */
> +	if (!crtc_state->master_crtc)
> +		return;
> +
> +	master_crtc_state = intel_atomic_get_new_crtc_state(old_intel_state,
> +							    crtc_state->master_crtc);
> +	if (WARN_ON(!master_crtc_state))
> +		return;
> +
> +	master_transcoder = master_crtc_state->cpu_transcoder;
> +	switch (master_transcoder) {
> +	case TRANSCODER_A:
> +		master_select = 1;
> +		break;
> +	case TRANSCODER_B:
> +		master_select = 2;
> +		break;
> +	case TRANSCODER_C:
> +		master_select = 3;
> +		break;
> +	case TRANSCODER_EDP:
> +	default:
> +		master_select = 0;
> +		break;
> +	}
> +	/* Set the master select bits for Tranascoder Port Sync */
> +	trans_ddi_func_ctl2_val = (PORT_SYNC_MODE_MASTER_SELECT(master_select) &
> +				   PORT_SYNC_MODE_MASTER_SELECT_MASK) <<
> +		PORT_SYNC_MODE_MASTER_SELECT_SHIFT;
> +	/* Enable Transcoder Port Sync */
> +	trans_ddi_func_ctl2_val |= PORT_SYNC_MODE_ENABLE;
> +
> +	I915_WRITE(TRANS_DDI_FUNC_CTL2(crtc_state->cpu_transcoder),
> +		   trans_ddi_func_ctl2_val);
> +}
> +
>  static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_state,
>  				     const struct intel_crtc_state *new_crtc_state)
>  {
> @@ -5971,6 +6026,9 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
>  	if (!transcoder_is_dsi(cpu_transcoder))
>  		intel_set_pipe_timings(pipe_config);
>  
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		icl_set_transcoder_port_sync(old_intel_state, pipe_config);
> +
>  	intel_set_pipe_src_size(pipe_config);
>  
>  	if (cpu_transcoder != TRANSCODER_EDP &&


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

[PATCH v2 3/4] drm/i915/dp: Trigger modeset if master_crtc or slaves bitmask changes

[Manasi Navare]

Add the comparison between the current state and new_crtc_state for
newly added master_crtc pointer and slave bitmask so that
if any of those change then the curernt master-slave links have
changed and we need to reconfigure the transcoder port sync register
and hence trigger a full modeset.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 81e8cb9fe221..4bd23e61c6fd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12524,6 +12524,11 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 	PIPE_CONF_CHECK_INFOFRAME(spd);
 	PIPE_CONF_CHECK_INFOFRAME(hdmi);
 
+	if (INTEL_GEN(dev_priv) >= 11) {
+		PIPE_CONF_CHECK_I(trans_port_sync_slaves);
+		PIPE_CONF_CHECK_P(master_crtc);
+	}
+
 #undef PIPE_CONF_CHECK_X
 #undef PIPE_CONF_CHECK_I
 #undef PIPE_CONF_CHECK_BOOL
-- 
2.19.1

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

Re: [PATCH v2 3/4] drm/i915/dp: Trigger modeset if master_crtc or slaves bitmask changes

[Srivatsa, Anusha]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>Manasi Navare
>Sent: Tuesday, April 23, 2019 8:49 AM
>To: intel-gfx@lists.freedesktop.org
>Subject: [Intel-gfx] [PATCH v2 3/4] drm/i915/dp: Trigger modeset if master_crtc
>or slaves bitmask changes
>
>Add the comparison between the current state and new_crtc_state for newly
>added master_crtc pointer and slave bitmask so that if any of those change then
>the curernt master-slave links have changed and we need to reconfigure the
>transcoder port sync register and hence trigger a full modeset.
>
>Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>Cc: Matt Roper <matthew.d.roper@intel.com>
>Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Looks good.

Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>

>---
> drivers/gpu/drm/i915/intel_display.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/intel_display.c
>b/drivers/gpu/drm/i915/intel_display.c
>index 81e8cb9fe221..4bd23e61c6fd 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -12524,6 +12524,11 @@ intel_pipe_config_compare(struct
>drm_i915_private *dev_priv,
> 	PIPE_CONF_CHECK_INFOFRAME(spd);
> 	PIPE_CONF_CHECK_INFOFRAME(hdmi);
>
>+	if (INTEL_GEN(dev_priv) >= 11) {
>+		PIPE_CONF_CHECK_I(trans_port_sync_slaves);
>+		PIPE_CONF_CHECK_P(master_crtc);
>+	}
>+
> #undef PIPE_CONF_CHECK_X
> #undef PIPE_CONF_CHECK_I
> #undef PIPE_CONF_CHECK_BOOL
>--
>2.19.1
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/4] drm/i915/dp: Trigger modeset if master_crtc or slaves bitmask changes

[Maarten Lankhorst]

Op 23-04-2019 om 17:49 schreef Manasi Navare:
> Add the comparison between the current state and new_crtc_state for
> newly added master_crtc pointer and slave bitmask so that
> if any of those change then the curernt master-slave links have
> changed and we need to reconfigure the transcoder port sync register
> and hence trigger a full modeset.
>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 81e8cb9fe221..4bd23e61c6fd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12524,6 +12524,11 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  	PIPE_CONF_CHECK_INFOFRAME(spd);
>  	PIPE_CONF_CHECK_INFOFRAME(hdmi);
>  
> +	if (INTEL_GEN(dev_priv) >= 11) {
> +		PIPE_CONF_CHECK_I(trans_port_sync_slaves);
> +		PIPE_CONF_CHECK_P(master_crtc);
> +	}
> +
>  #undef PIPE_CONF_CHECK_X
>  #undef PIPE_CONF_CHECK_I
>  #undef PIPE_CONF_CHECK_BOOL

Should probably be merged with patch 1/4

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

Re: [PATCH v2 3/4] drm/i915/dp: Trigger modeset if master_crtc or slaves bitmask changes

[Manasi Navare]

On Wed, Jun 12, 2019 at 10:32:04AM +0200, Maarten Lankhorst wrote:
> Op 23-04-2019 om 17:49 schreef Manasi Navare:
> > Add the comparison between the current state and new_crtc_state for
> > newly added master_crtc pointer and slave bitmask so that
> > if any of those change then the curernt master-slave links have
> > changed and we need to reconfigure the transcoder port sync register
> > and hence trigger a full modeset.
> >
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 81e8cb9fe221..4bd23e61c6fd 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12524,6 +12524,11 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> >  	PIPE_CONF_CHECK_INFOFRAME(spd);
> >  	PIPE_CONF_CHECK_INFOFRAME(hdmi);
> >  
> > +	if (INTEL_GEN(dev_priv) >= 11) {
> > +		PIPE_CONF_CHECK_I(trans_port_sync_slaves);
> > +		PIPE_CONF_CHECK_P(master_crtc);
> > +	}
> > +
> >  #undef PIPE_CONF_CHECK_X
> >  #undef PIPE_CONF_CHECK_I
> >  #undef PIPE_CONF_CHECK_BOOL
> 
> Should probably be merged with patch 1/4

Yes in my next revision will combine this into patch 1, also need another patch for
HW state readout, that should be a separate patch though right?

Manasi
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 4/4] drm/i915/dp: Enable master-slaves in trans port sync mode in correct order

[Manasi Navare]

As per the display enable sequence, we need to follow the enable sequence
for slaves first with DP_TP_CTL set to Idle and configure the transcoder
port sync register to select the corersponding master, then follow the
enable sequence for master leaving DP_TP_CTL to idle.
At this point the transcoder port sync mode is configured and enabled
and the Vblanks of both ports are synchronized so then set DP_TP_CTL
for the slave and master to Normal and do post crtc enable updates.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     |   3 +-
 drivers/gpu/drm/i915/intel_display.c | 122 +++++++++++++++++++++++++++
 2 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 24f9106efcc6..0b326b2c274d 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3120,7 +3120,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 					      true);
 	intel_dp_sink_set_fec_ready(intel_dp, crtc_state);
 	intel_dp_start_link_train(intel_dp);
-	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
+	if ((port != PORT_A || INTEL_GEN(dev_priv) >= 9) &&
+	    INTEL_GEN(dev_priv) < 11)
 		intel_dp_stop_link_train(intel_dp);
 
 	intel_ddi_enable_fec(encoder, crtc_state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4bd23e61c6fd..bd00549a9e00 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13488,6 +13488,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state)
 		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 			bool vbl_wait = false;
 			unsigned int cmask = drm_crtc_mask(crtc);
+			bool modeset = needs_modeset(new_crtc_state);
 
 			intel_crtc = to_intel_crtc(crtc);
 			cstate = to_intel_crtc_state(new_crtc_state);
@@ -13496,6 +13497,12 @@ static void skl_update_crtcs(struct drm_atomic_state *state)
 			if (updated & cmask || !cstate->base.active)
 				continue;
 
+			if (modeset && INTEL_GEN(dev_priv) >= 11) {
+				DRM_DEBUG_KMS("Pushing the Tiled CRTC %d %s that needs update to separate loop\n",
+					      intel_crtc->base.base.id, intel_crtc->base.name);
+				continue;
+			}
+
 			if (skl_ddb_allocation_overlaps(&cstate->wm.skl.ddb,
 							entries,
 							INTEL_INFO(dev_priv)->num_pipes, i))
@@ -13526,6 +13533,121 @@ static void skl_update_crtcs(struct drm_atomic_state *state)
 		}
 	} while (progress);
 
+	/* Separate loop for updating Slave CRTCs that need modeset.
+	 * We need to loop through all slaves of tiled display first and
+	 * follow enable sequence with DP_TP_CTL left Idle until the master
+	 * is ready.
+	 */
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		bool modeset = needs_modeset(new_crtc_state);
+		struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
+
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!pipe_config->base.active)
+			continue;
+		if (!modeset)
+			continue;
+		if (!pipe_config->master_crtc)
+			continue;
+
+		update_scanline_offset(pipe_config);
+		dev_priv->display.crtc_enable(pipe_config, state);
+	}
+	/* Now do the display enable sequence for master CRTC */
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		bool modeset = needs_modeset(new_crtc_state);
+		struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
+
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!pipe_config->base.active)
+			continue;
+		if (!modeset)
+			continue;
+		if (pipe_config->master_crtc)
+			continue;
+
+		update_scanline_offset(pipe_config);
+		dev_priv->display.crtc_enable(pipe_config, state);
+	}
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		struct drm_connector_state *conn_state;
+		struct drm_connector *conn;
+		bool modeset = needs_modeset(new_crtc_state);
+		struct intel_dp *intel_dp;
+
+		intel_crtc = to_intel_crtc(crtc);
+		cstate = to_intel_crtc_state(new_crtc_state);
+
+		if (!cstate->base.active)
+			continue;
+		if (!modeset)
+			continue;
+		if (!cstate->master_crtc)
+			continue;
+
+		for_each_new_connector_in_state(state, conn, conn_state, i) {
+			if (conn_state->crtc == crtc)
+				break;
+		}
+		intel_dp = enc_to_intel_dp(&intel_attached_encoder(conn)->base);
+		intel_dp_stop_link_train(intel_dp);
+	}
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		struct drm_connector_state *conn_state;
+		struct drm_connector *conn;
+		bool modeset = needs_modeset(new_crtc_state);
+		struct intel_dp *intel_dp;
+
+		intel_crtc = to_intel_crtc(crtc);
+		cstate = to_intel_crtc_state(new_crtc_state);
+
+		if (!cstate->base.active)
+			continue;
+		if (!modeset)
+			continue;
+		if (cstate->master_crtc)
+			continue;
+
+		/* Wait for 200us before setting the master DP_TP_TCL to Normal */
+		usleep_range(200, 400);
+		for_each_new_connector_in_state(state, conn, conn_state, i) {
+			if (conn_state->crtc == crtc)
+				break;
+		}
+		intel_dp = enc_to_intel_dp(&intel_attached_encoder(conn)->base);
+		intel_dp_stop_link_train(intel_dp);
+	}
+	/* Now do the post crtc enable for all master and slaves */
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		bool modeset = needs_modeset(new_crtc_state);
+		struct intel_plane_state *new_plane_state;
+		struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
+
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!pipe_config->base.active)
+			continue;
+		if (!modeset)
+			continue;
+
+		new_plane_state =
+			intel_atomic_get_new_plane_state(to_intel_atomic_state(state),
+							 to_intel_plane(crtc->primary));
+		if (pipe_config->update_pipe && !pipe_config->enable_fbc)
+			intel_fbc_disable(intel_crtc);
+		else if (new_plane_state)
+			intel_fbc_enable(intel_crtc, pipe_config, new_plane_state);
+
+		intel_begin_crtc_commit(to_intel_atomic_state(state), intel_crtc);
+		if (INTEL_GEN(dev_priv) >= 9)
+			skl_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
+		else
+			i9xx_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
+
+		intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
+	}
 	/* If 2nd DBuf slice is no more required disable it */
 	if (INTEL_GEN(dev_priv) >= 11 && required_slices < hw_enabled_slices)
 		icl_dbuf_slices_update(dev_priv, required_slices);
-- 
2.19.1

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

Re: [PATCH v2 4/4] drm/i915/dp: Enable master-slaves in trans port sync mode in correct order

[Maarten Lankhorst]

Op 23-04-2019 om 17:49 schreef Manasi Navare:
> As per the display enable sequence, we need to follow the enable sequence
> for slaves first with DP_TP_CTL set to Idle and configure the transcoder
> port sync register to select the corersponding master, then follow the
> enable sequence for master leaving DP_TP_CTL to idle.
> At this point the transcoder port sync mode is configured and enabled
> and the Vblanks of both ports are synchronized so then set DP_TP_CTL
> for the slave and master to Normal and do post crtc enable updates.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |   3 +-
>  drivers/gpu/drm/i915/intel_display.c | 122 +++++++++++++++++++++++++++
>  2 files changed, 124 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 24f9106efcc6..0b326b2c274d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3120,7 +3120,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>  					      true);
>  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state);
>  	intel_dp_start_link_train(intel_dp);
> -	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> +	if ((port != PORT_A || INTEL_GEN(dev_priv) >= 9) &&
> +	    INTEL_GEN(dev_priv) < 11)
&& !master_crtc instead of gen check? Would be more clear what it 's for, and only changes link training in sync mode.
>  		intel_dp_stop_link_train(intel_dp);
>  
>  	intel_ddi_enable_fec(encoder, crtc_state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4bd23e61c6fd..bd00549a9e00 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13488,6 +13488,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state)
>  		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>  			bool vbl_wait = false;
>  			unsigned int cmask = drm_crtc_mask(crtc);
> +			bool modeset = needs_modeset(new_crtc_state);
>  
>  			intel_crtc = to_intel_crtc(crtc);
>  			cstate = to_intel_crtc_state(new_crtc_state);
> @@ -13496,6 +13497,12 @@ static void skl_update_crtcs(struct drm_atomic_state *state)
>  			if (updated & cmask || !cstate->base.active)
>  				continue;
>  
> +			if (modeset && INTEL_GEN(dev_priv) >= 11) {
> +				DRM_DEBUG_KMS("Pushing the Tiled CRTC %d %s that needs update to separate loop\n",
> +					      intel_crtc->base.base.id, intel_crtc->base.name);
> +				continue;
> +			}
Only do this for synced crtc's?
> +
>  			if (skl_ddb_allocation_overlaps(&cstate->wm.skl.ddb,
>  							entries,
>  							INTEL_INFO(dev_priv)->num_pipes, i))
> @@ -13526,6 +13533,121 @@ static void skl_update_crtcs(struct drm_atomic_state *state)
>  		}
>  	} while (progress);
>  
> +	/* Separate loop for updating Slave CRTCs that need modeset.
> +	 * We need to loop through all slaves of tiled display first and
> +	 * follow enable sequence with DP_TP_CTL left Idle until the master
> +	 * is ready.
> +	 */
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		bool modeset = needs_modeset(new_crtc_state);
> +		struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
> +
> +		intel_crtc = to_intel_crtc(crtc);
> +
> +		if (!pipe_config->base.active)
> +			continue;
> +		if (!modeset)
> +			continue;
> +		if (!pipe_config->master_crtc)
> +			continue;
> +
> +		update_scanline_offset(pipe_config);
> +		dev_priv->display.crtc_enable(pipe_config, state);
> +	}
> +	/* Now do the display enable sequence for master CRTC */
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		bool modeset = needs_modeset(new_crtc_state);
> +		struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
> +
> +		intel_crtc = to_intel_crtc(crtc);
> +
> +		if (!pipe_config->base.active)
> +			continue;
> +		if (!modeset)
> +			continue;
> +		if (pipe_config->master_crtc)
> +			continue;
> +
> +		update_scanline_offset(pipe_config);
> +		dev_priv->display.crtc_enable(pipe_config, state);
> +	}
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		struct drm_connector_state *conn_state;
> +		struct drm_connector *conn;
> +		bool modeset = needs_modeset(new_crtc_state);
> +		struct intel_dp *intel_dp;
> +
> +		intel_crtc = to_intel_crtc(crtc);
> +		cstate = to_intel_crtc_state(new_crtc_state);
> +
> +		if (!cstate->base.active)
> +			continue;
> +		if (!modeset)
> +			continue;
> +		if (!cstate->master_crtc)
> +			continue;
> +
> +		for_each_new_connector_in_state(state, conn, conn_state, i) {
> +			if (conn_state->crtc == crtc)
> +				break;
> +		}
> +		intel_dp = enc_to_intel_dp(&intel_attached_encoder(conn)->base);
> +		intel_dp_stop_link_train(intel_dp);
> +	}
Why is this a separate pass?
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		struct drm_connector_state *conn_state;
> +		struct drm_connector *conn;
> +		bool modeset = needs_modeset(new_crtc_state);
> +		struct intel_dp *intel_dp;
> +
> +		intel_crtc = to_intel_crtc(crtc);
> +		cstate = to_intel_crtc_state(new_crtc_state);
> +
> +		if (!cstate->base.active)
> +			continue;
> +		if (!modeset)
> +			continue;
> +		if (cstate->master_crtc)
> +			continue;
> +
> +		/* Wait for 200us before setting the master DP_TP_TCL to Normal */
> +		usleep_range(200, 400);
> +		for_each_new_connector_in_state(state, conn, conn_state, i) {
> +			if (conn_state->crtc == crtc)
> +				break;
> +		}
> +		intel_dp = enc_to_intel_dp(&intel_attached_encoder(conn)->base);
> +		intel_dp_stop_link_train(intel_dp);
> +	}
> +	/* Now do the post crtc enable for all master and slaves */
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		bool modeset = needs_modeset(new_crtc_state);
> +		struct intel_plane_state *new_plane_state;
> +		struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
> +
> +		intel_crtc = to_intel_crtc(crtc);
> +
> +		if (!pipe_config->base.active)
> +			continue;
> +		if (!modeset)
> +			continue;
> +
> +		new_plane_state =
> +			intel_atomic_get_new_plane_state(to_intel_atomic_state(state),
> +							 to_intel_plane(crtc->primary));
> +		if (pipe_config->update_pipe && !pipe_config->enable_fbc)
> +			intel_fbc_disable(intel_crtc);
> +		else if (new_plane_state)
> +			intel_fbc_enable(intel_crtc, pipe_config, new_plane_state);
> +
> +		intel_begin_crtc_commit(to_intel_atomic_state(state), intel_crtc);
> +		if (INTEL_GEN(dev_priv) >= 9)
> +			skl_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
> +		else
> +			i9xx_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
This is called from skl_update_crtcs, don't need the <gen9 fallback code.
> +		intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
> +	}
>  	/* If 2nd DBuf slice is no more required disable it */
>  	if (INTEL_GEN(dev_priv) >= 11 && required_slices < hw_enabled_slices)
>  		icl_dbuf_slices_update(dev_priv, required_slices);

Better yet, make an icl_update_crtcs that handles the sync mode, and remove all the gen11 stuff from skl_update_crtcs.

~Maarten

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

Re: [PATCH v2 4/4] drm/i915/dp: Enable master-slaves in trans port sync mode in correct order

[Manasi Navare]

Thanks a lot Maarten for your review comments.
Somehow none of my email-clients caught Maarten's review on this, so capturing and
responding to his review comments from Patchwork

On Tue, Apr 23, 2019 at 08:49:01AM -0700, Manasi Navare wrote:
> As per the display enable sequence, we need to follow the enable sequence
> for slaves first with DP_TP_CTL set to Idle and configure the transcoder
> port sync register to select the corersponding master, then follow the
> enable sequence for master leaving DP_TP_CTL to idle.
> At this point the transcoder port sync mode is configured and enabled
> and the Vblanks of both ports are synchronized so then set DP_TP_CTL
> for the slave and master to Normal and do post crtc enable updates.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |   3 +-
>  drivers/gpu/drm/i915/intel_display.c | 122 +++++++++++++++++++++++++++
>  2 files changed, 124 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 24f9106efcc6..0b326b2c274d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3120,7 +3120,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>  					      true);
>  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state);
>  	intel_dp_start_link_train(intel_dp);
> -	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> +	if ((port != PORT_A || INTEL_GEN(dev_priv) >= 9) &&
> +	    INTEL_GEN(dev_priv) < 11)
>  		intel_dp_stop_link_train(intel_dp);

[Maarten's comment]: && !master_crtc instead of gen check? Would be more clear what it 's for, and only changes link training in sync mode.

->It is gen based check because we want to call the intel_dp_stop_link_train() as is for older gens whereas for Gen 11 it gets called
separately in crtc_enable() hook for all crtcs (ganged and non ganged).
It was Ville's feedback that its better to call stop_dp_link_train() in crtc_enable() for all crtcs.

But now that I rethink, we shouldnt change the enable sequence for crtcs not in sync mode and only do this for crtcs
in sync mode. 
So If we have to avoid calling it only for the trans_port_sync mode and CRTCs involved in this mode then we would need to add a check
&& !master_crtc && !trans_port_sync_slaves

Ville, Maarten, any thoughts?

>  
>  	intel_ddi_enable_fec(encoder, crtc_state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4bd23e61c6fd..bd00549a9e00 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13488,6 +13488,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state)
>  		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>  			bool vbl_wait = false;
>  			unsigned int cmask = drm_crtc_mask(crtc);
> +			bool modeset = needs_modeset(new_crtc_state);
>  
>  			intel_crtc = to_intel_crtc(crtc);
>  			cstate = to_intel_crtc_state(new_crtc_state);
> @@ -13496,6 +13497,12 @@ static void skl_update_crtcs(struct drm_atomic_state *state)
>  			if (updated & cmask || !cstate->base.active)
>  				continue;

[Maarten's comment]: Only do this for synced crtc's?

-> I think this goes back to Ville's suggestion of following this for all CRTCs, but that means we deviate
from th bspec sequence for non synced crtcs too.

Should we do thsi only for synced crtcs? Again use the condition master_crtc || trans_port_sync_slave then do this?

>  
> +			if (modeset && INTEL_GEN(dev_priv) >= 11) {
> +				DRM_DEBUG_KMS("Pushing the Tiled CRTC %d %s that needs update to separate loop\n",
> +					      intel_crtc->base.base.id, intel_crtc->base.name);
> +				continue;
> +			}
> +
>  			if (skl_ddb_allocation_overlaps(&cstate->wm.skl.ddb,
>  							entries,
>  							INTEL_INFO(dev_priv)->num_pipes, i))
> @@ -13526,6 +13533,121 @@ static void skl_update_crtcs(struct drm_atomic_state *state)
>  		}
>  	} while (progress);
>  
> +	/* Separate loop for updating Slave CRTCs that need modeset.
> +	 * We need to loop through all slaves of tiled display first and
> +	 * follow enable sequence with DP_TP_CTL left Idle until the master
> +	 * is ready.
> +	 */
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		bool modeset = needs_modeset(new_crtc_state);
> +		struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
> +
> +		intel_crtc = to_intel_crtc(crtc);
> +
> +		if (!pipe_config->base.active)
> +			continue;
> +		if (!modeset)
> +			continue;
> +		if (!pipe_config->master_crtc)
> +			continue;
> +
> +		update_scanline_offset(pipe_config);
> +		dev_priv->display.crtc_enable(pipe_config, state);
> +	}
> +	/* Now do the display enable sequence for master CRTC */
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		bool modeset = needs_modeset(new_crtc_state);
> +		struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
> +
> +		intel_crtc = to_intel_crtc(crtc);
> +
> +		if (!pipe_config->base.active)
> +			continue;
> +		if (!modeset)
> +			continue;
> +		if (pipe_config->master_crtc)
> +			continue;
> +
> +		update_scanline_offset(pipe_config);
> +		dev_priv->display.crtc_enable(pipe_config, state);
> +	}
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		struct drm_connector_state *conn_state;
> +		struct drm_connector *conn;
> +		bool modeset = needs_modeset(new_crtc_state);
> +		struct intel_dp *intel_dp;
> +
> +		intel_crtc = to_intel_crtc(crtc);
> +		cstate = to_intel_crtc_state(new_crtc_state);
> +
> +		if (!cstate->base.active)
> +			continue;
> +		if (!modeset)
> +			continue;
> +		if (!cstate->master_crtc)
> +			continue;
> +
> +		for_each_new_connector_in_state(state, conn, conn_state, i) {
> +			if (conn_state->crtc == crtc)
> +				break;
> +		}
> +		intel_dp = enc_to_intel_dp(&intel_attached_encoder(conn)->base);
> +		intel_dp_stop_link_train(intel_dp);
> +	}

[Maarten's comment]: Why is this a separate pass?

-> This is a separate pass because for the synced mode we cannot call stop_link_train() which
sets DP_TP_CTL to Normal on master before the slave, so we call stop_link_train() for all slaves first
and then for the master.

> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		struct drm_connector_state *conn_state;
> +		struct drm_connector *conn;
> +		bool modeset = needs_modeset(new_crtc_state);
> +		struct intel_dp *intel_dp;
> +
> +		intel_crtc = to_intel_crtc(crtc);
> +		cstate = to_intel_crtc_state(new_crtc_state);
> +
> +		if (!cstate->base.active)
> +			continue;
> +		if (!modeset)
> +			continue;
> +		if (cstate->master_crtc)
> +			continue;
> +
> +		/* Wait for 200us before setting the master DP_TP_TCL to Normal */
> +		usleep_range(200, 400);
> +		for_each_new_connector_in_state(state, conn, conn_state, i) {
> +			if (conn_state->crtc == crtc)
> +				break;
> +		}
> +		intel_dp = enc_to_intel_dp(&intel_attached_encoder(conn)->base);
> +		intel_dp_stop_link_train(intel_dp);
> +	}
> +	/* Now do the post crtc enable for all master and slaves */
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		bool modeset = needs_modeset(new_crtc_state);
> +		struct intel_plane_state *new_plane_state;
> +		struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
> +
> +		intel_crtc = to_intel_crtc(crtc);
> +
> +		if (!pipe_config->base.active)
> +			continue;
> +		if (!modeset)
> +			continue;
> +
> +		new_plane_state =
> +			intel_atomic_get_new_plane_state(to_intel_atomic_state(state),
> +							 to_intel_plane(crtc->primary));
> +		if (pipe_config->update_pipe && !pipe_config->enable_fbc)
> +			intel_fbc_disable(intel_crtc);
> +		else if (new_plane_state)
> +			intel_fbc_enable(intel_crtc, pipe_config, new_plane_state);
> +
> +		intel_begin_crtc_commit(to_intel_atomic_state(state), intel_crtc);
> +		if (INTEL_GEN(dev_priv) >= 9)
> +			skl_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
> +		else
> +			i9xx_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);

[Maarten's comments]: This is called from skl_update_crtcs, don't need the <gen9 fallback code.

-> Yup, will remove the fallback code

> +
> +		intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
> +	}
>  	/* If 2nd DBuf slice is no more required disable it */
>  	if (INTEL_GEN(dev_priv) >= 11 && required_slices < hw_enabled_slices)
>  		icl_dbuf_slices_update(dev_priv, required_slices);

[Maarten's comment]: Better yet, make an icl_update_crtcs that handles the sync mode, and remove all the gen11 stuff from skl_update_crtcs.

~Maarten

-> Yes that makes sense, I will create a new hook for icl_update_crtcs() and add all trans port sync code there then we can get rid
of the GEN checks.
Now the only open is if we should split into 4 loops for all crtcs that need modeset or should we do that only for
crtcs in the sync mode.

Manasi

> -- 
> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for Enable Transcoder Port Sync feature for Tiled displays

[Patchwork]

== Series Details ==

Series: Enable Transcoder Port Sync feature for Tiled displays
URL   : https://patchwork.freedesktop.org/series/59837/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
3288bd9171c1 drm/i915/icl: Assign Master slave crtc links for Transcoder Port Sync
-:109: WARNING:TYPO_SPELLING: 'bitmast' may be misspelled - perhaps 'bitmask'?
#109: FILE: drivers/gpu/drm/i915/intel_display.c:11390:
+		DRM_DEBUG_KMS("Master CRTC = %d added for Slave CRTC = %d\n, slave bitmast = %d",

total: 0 errors, 1 warnings, 0 checks, 119 lines checked
40e43568332f drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports
8be6467cdfa9 drm/i915/dp: Trigger modeset if master_crtc or slaves bitmask changes
2d882b36c3ec drm/i915/dp: Enable master-slaves in trans port sync mode in correct order

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

✗ Fi.CI.BAT: failure for Enable Transcoder Port Sync feature for Tiled displays

[Patchwork]

== Series Details ==

Series: Enable Transcoder Port Sync feature for Tiled displays
URL   : https://patchwork.freedesktop.org/series/59837/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5971 -> Patchwork_12855
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12855 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12855, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/59837/revisions/1/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12855:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-kbl-r:           [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_5971/fi-kbl-r/igt@gem_exec_suspend@basic-s3.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-kbl-r/igt@gem_exec_suspend@basic-s3.html
    - fi-skl-6600u:       [PASS][3] -> [DMESG-WARN][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_5971/fi-skl-6600u/igt@gem_exec_suspend@basic-s3.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-skl-6600u/igt@gem_exec_suspend@basic-s3.html

  * igt@runner@aborted:
    - fi-bxt-j4205:       NOTRUN -> [FAIL][5]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-bxt-j4205/igt@runner@aborted.html
    - fi-whl-u:           NOTRUN -> [FAIL][6]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-whl-u/igt@runner@aborted.html
    - fi-apl-guc:         NOTRUN -> [FAIL][7]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-apl-guc/igt@runner@aborted.html

  
Known issues
------------

  Here are the changes found in Patchwork_12855 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-skl-iommu:       [PASS][8] -> [INCOMPLETE][9] ([fdo#104108] / [fdo#107773])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_5971/fi-skl-iommu/igt@gem_exec_suspend@basic-s3.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-skl-iommu/igt@gem_exec_suspend@basic-s3.html

  * igt@kms_busy@basic-flip-b:
    - fi-icl-y:           [PASS][10] -> [INCOMPLETE][11] ([fdo#107713])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_5971/fi-icl-y/igt@kms_busy@basic-flip-b.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-icl-y/igt@kms_busy@basic-flip-b.html

  * igt@kms_pipe_crc_basic@read-crc-pipe-a:
    - fi-byt-clapper:     [PASS][12] -> [FAIL][13] ([fdo#103191])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_5971/fi-byt-clapper/igt@kms_pipe_crc_basic@read-crc-pipe-a.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-byt-clapper/igt@kms_pipe_crc_basic@read-crc-pipe-a.html

  * igt@runner@aborted:
    - fi-cfl-8109u:       NOTRUN -> [FAIL][14] ([k.org#202107] / [k.org#202109])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-cfl-8109u/igt@runner@aborted.html
    - fi-kbl-7567u:       NOTRUN -> [FAIL][15] ([fdo#108903] / [fdo#108904] / [fdo#108905])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-kbl-7567u/igt@runner@aborted.html
    - fi-kbl-x1275:       NOTRUN -> [FAIL][16] ([fdo#108903] / [fdo#108904] / [fdo#108905])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-kbl-x1275/igt@runner@aborted.html
    - fi-skl-6600u:       NOTRUN -> [FAIL][17] ([fdo#104108])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-skl-6600u/igt@runner@aborted.html
    - fi-skl-lmem:        NOTRUN -> [FAIL][18] ([fdo#104108])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-skl-lmem/igt@runner@aborted.html
    - fi-kbl-r:           NOTRUN -> [FAIL][19] ([fdo#109383])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-kbl-r/igt@runner@aborted.html
    - fi-skl-6770hq:      NOTRUN -> [FAIL][20] ([fdo#104108])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-skl-6770hq/igt@runner@aborted.html
    - fi-skl-gvtdvm:      NOTRUN -> [FAIL][21] ([fdo#104108])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-skl-gvtdvm/igt@runner@aborted.html

  
#### Warnings ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-whl-u:           [FAIL][22] ([fdo#103375]) -> [DMESG-FAIL][23] ([fdo#103375])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_5971/fi-whl-u/igt@gem_exec_suspend@basic-s3.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-whl-u/igt@gem_exec_suspend@basic-s3.html

  * igt@runner@aborted:
    - fi-kbl-7500u:       [FAIL][24] ([fdo#103841]) -> [FAIL][25] ([fdo#108903] / [fdo#108904] / [fdo#108905])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_5971/fi-kbl-7500u/igt@runner@aborted.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/fi-kbl-7500u/igt@runner@aborted.html

  
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#103841]: https://bugs.freedesktop.org/show_bug.cgi?id=103841
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#108903]: https://bugs.freedesktop.org/show_bug.cgi?id=108903
  [fdo#108904]: https://bugs.freedesktop.org/show_bug.cgi?id=108904
  [fdo#108905]: https://bugs.freedesktop.org/show_bug.cgi?id=108905
  [fdo#109383]: https://bugs.freedesktop.org/show_bug.cgi?id=109383
  [k.org#202107]: https://bugzilla.kernel.org/show_bug.cgi?id=202107
  [k.org#202109]: https://bugzilla.kernel.org/show_bug.cgi?id=202109


Participating hosts (50 -> 33)
------------------------------

  Missing    (17): fi-ilk-m540 fi-bxt-dsi fi-hsw-4200u fi-skl-guc fi-byt-squawks fi-icl-u2 fi-skl-6260u fi-bwr-2160 fi-cfl-8700k fi-ctg-p8600 fi-bsw-cyan fi-cfl-guc fi-ivb-3770 fi-icl-u3 fi-bdw-samus fi-icl-dsi fi-skl-6700k2 


Build changes
-------------

  * Linux: CI_DRM_5971 -> Patchwork_12855

  CI_DRM_5971: e91b848a66e8672c48ea65d082b260f13f2c86b9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4959: 504367d33b787de2ba8e007a5b620cfd6f0b3074 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12855: 2d882b36c3ecc0e0b17fd6bb8a9a56bd33b4ed98 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

2d882b36c3ec drm/i915/dp: Enable master-slaves in trans port sync mode in correct order
8be6467cdfa9 drm/i915/dp: Trigger modeset if master_crtc or slaves bitmask changes
40e43568332f drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports
3288bd9171c1 drm/i915/icl: Assign Master slave crtc links for Transcoder Port Sync

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12855/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx