[PATCH 1/2] drm/i915/icl: Assign genlock CRTC pointer in all slave CRTC states for tiled displays

[PATCH 1/2] drm/i915/icl: Assign genlock CRTC pointer in all slave CRTC states for tiled displays

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

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 | 84 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  3 +
 2 files changed, 87 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8ff7aa8cb3cf..9980a4ed8c9c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11281,6 +11281,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
 	return 0;
 }
 
+static int icl_add_genlock_crtcs(struct drm_device *dev,
+				 struct drm_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_connector *genlock_connector, *connector;
+	struct drm_connector_state *connector_state;
+	struct drm_connector_list_iter conn_iter;
+	struct drm_crtc *genlock_crtc = NULL;
+	struct drm_crtc_state *genlock_crtc_state;
+	struct intel_crtc_state *slave_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)
+			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;
+		slave_crtc_state = to_intel_crtc_state(
+			drm_atomic_get_new_crtc_state(state,
+						      connector_state->crtc));
+		slave_crtc_state->genlock_crtc = NULL;
+		tile_group_id = connector->tile_group->id;
+		drm_connector_list_iter_begin(dev, &conn_iter);
+		drm_for_each_connector_iter(genlock_connector, &conn_iter) {
+			struct drm_connector_state *genlock_conn_state = NULL;
+
+			if (!genlock_connector->has_tile)
+				continue;
+			if (genlock_connector->tile_h_loc != genlock_connector->num_h_tile - 1 ||
+			    genlock_connector->tile_v_loc != genlock_connector->num_v_tile - 1)
+				continue;
+			if (genlock_connector->tile_group->id != tile_group_id)
+				continue;
+
+			genlock_conn_state = drm_atomic_get_connector_state(state,
+									    genlock_connector);
+			if (IS_ERR(genlock_conn_state)) {
+				drm_connector_list_iter_end(&conn_iter);
+				return PTR_ERR(genlock_conn_state);
+			}
+			if (genlock_conn_state->crtc) {
+				genlock_crtc = genlock_conn_state->crtc;
+				break;
+			}
+		}
+		drm_connector_list_iter_end(&conn_iter);
+
+		if (!genlock_crtc) {
+			DRM_DEBUG_KMS("Could not add Genlock CRTC for Slave CRTC %d\n",
+				      connector_state->crtc->base.id);
+			return -EINVAL;
+		}
+
+		genlock_crtc_state = drm_atomic_get_crtc_state(state,
+							       genlock_crtc);
+		if (IS_ERR(genlock_crtc_state))
+			return PTR_ERR(genlock_crtc_state);
+
+		slave_crtc_state->genlock_crtc = to_intel_crtc(genlock_crtc);
+		DRM_DEBUG_KMS("Genlock CRTC = %d added for Slave CRTC = %d\n",
+			      genlock_crtc->base.id,
+			      slave_crtc_state->base.crtc->base.id);
+	}
+
+	return 0;
+}
+
 static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 				   struct drm_crtc_state *crtc_state)
 {
@@ -13098,6 +13178,10 @@ static int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		return ret;
 
+	ret = icl_add_genlock_crtcs(dev, state);
+	if (ret)
+		return ret;
+
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) {
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc_state);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4d7ae579fc92..b41728946b73 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1074,6 +1074,9 @@ struct intel_crtc_state {
 
 	/* Forward Error correction State */
 	bool fec_enable;
+
+	/* Pointer to master/genlock crtc in case of tiled displays */
+	struct intel_crtc *genlock_crtc;
 };
 
 struct intel_crtc {
-- 
2.19.1

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

[PATCH 2/2] 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.

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 | 59 ++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9980a4ed8c9c..16b46a3cb3bd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4009,6 +4009,62 @@ 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 *genlock_crtc_state = NULL;
+	enum transcoder genlock_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->genlock_crtc)
+		return;
+
+	genlock_crtc_state = intel_atomic_get_new_crtc_state(old_intel_state,
+							     crtc_state->genlock_crtc);
+	if (WARN_ON(!genlock_crtc_state))
+		return;
+
+	genlock_transcoder = genlock_crtc_state->cpu_transcoder;
+	switch (genlock_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 = I915_READ(TRANS_DDI_FUNC_CTL2(crtc_state->cpu_transcoder));
+	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)
 {
@@ -5960,6 +6016,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 2/2] drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports

[Jani Nikula]

On Thu, 21 Mar 2019, Manasi Navare <manasi.d.navare@intel.com> wrote:
> 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.
>
> 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 | 59 ++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9980a4ed8c9c..16b46a3cb3bd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4009,6 +4009,62 @@ 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 *genlock_crtc_state = NULL;
> +	enum transcoder genlock_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->genlock_crtc)
> +		return;
> +
> +	genlock_crtc_state = intel_atomic_get_new_crtc_state(old_intel_state,
> +							     crtc_state->genlock_crtc);
> +	if (WARN_ON(!genlock_crtc_state))
> +		return;
> +
> +	genlock_transcoder = genlock_crtc_state->cpu_transcoder;
> +	switch (genlock_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 = I915_READ(TRANS_DDI_FUNC_CTL2(crtc_state->cpu_transcoder));
> +	trans_ddi_func_ctl2_val |= (PORT_SYNC_MODE_MASTER_SELECT(master_select) &
> +				    PORT_SYNC_MODE_MASTER_SELECT_MASK) <<
> +		PORT_SYNC_MODE_MASTER_SELECT_SHIFT;

This doesn't do what you think it does. ITYM,

	val = I915_READ();
        val &= ~FOO_MASK;
        val |= FOO_BAR;

Please actually use just "val" for the variable, the long name just
makes this all harder to read.

BR,
Jani.


> +	/* 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)
>  {
> @@ -5960,6 +6016,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 &&

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Fri, Mar 22, 2019 at 11:34:25AM +0200, Jani Nikula wrote:
> On Thu, 21 Mar 2019, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > 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.
> >
> > 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 | 59 ++++++++++++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 9980a4ed8c9c..16b46a3cb3bd 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4009,6 +4009,62 @@ 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 *genlock_crtc_state = NULL;
> > +	enum transcoder genlock_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->genlock_crtc)
> > +		return;
> > +
> > +	genlock_crtc_state = intel_atomic_get_new_crtc_state(old_intel_state,
> > +							     crtc_state->genlock_crtc);
> > +	if (WARN_ON(!genlock_crtc_state))
> > +		return;
> > +
> > +	genlock_transcoder = genlock_crtc_state->cpu_transcoder;
> > +	switch (genlock_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 = I915_READ(TRANS_DDI_FUNC_CTL2(crtc_state->cpu_transcoder));
> > +	trans_ddi_func_ctl2_val |= (PORT_SYNC_MODE_MASTER_SELECT(master_select) &
> > +				    PORT_SYNC_MODE_MASTER_SELECT_MASK) <<
> > +		PORT_SYNC_MODE_MASTER_SELECT_SHIFT;
> 
> This doesn't do what you think it does. ITYM,
> 
> 	val = I915_READ();
>         val &= ~FOO_MASK;
>         val |= FOO_BAR;

Also we alreayd have a place where we write this registers. Is there
some magic requirement why these bits can't be set there along with
eveyrthing else?

> 
> Please actually use just "val" for the variable, the long name just
> makes this all harder to read.
> 
> BR,
> Jani.
> 
> 
> > +	/* 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)
> >  {
> > @@ -5960,6 +6016,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 &&
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

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

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

[Manasi Navare]

On Fri, Mar 22, 2019 at 03:16:03PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 22, 2019 at 11:34:25AM +0200, Jani Nikula wrote:
> > On Thu, 21 Mar 2019, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > > 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.
> > >
> > > 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 | 59 ++++++++++++++++++++++++++++
> > >  1 file changed, 59 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 9980a4ed8c9c..16b46a3cb3bd 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -4009,6 +4009,62 @@ 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 *genlock_crtc_state = NULL;
> > > +	enum transcoder genlock_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->genlock_crtc)
> > > +		return;
> > > +
> > > +	genlock_crtc_state = intel_atomic_get_new_crtc_state(old_intel_state,
> > > +							     crtc_state->genlock_crtc);
> > > +	if (WARN_ON(!genlock_crtc_state))
> > > +		return;
> > > +
> > > +	genlock_transcoder = genlock_crtc_state->cpu_transcoder;
> > > +	switch (genlock_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 = I915_READ(TRANS_DDI_FUNC_CTL2(crtc_state->cpu_transcoder));
> > > +	trans_ddi_func_ctl2_val |= (PORT_SYNC_MODE_MASTER_SELECT(master_select) &
> > > +				    PORT_SYNC_MODE_MASTER_SELECT_MASK) <<
> > > +		PORT_SYNC_MODE_MASTER_SELECT_SHIFT;
> > 
> > This doesn't do what you think it does. ITYM,
> > 
> > 	val = I915_READ();
> >         val &= ~FOO_MASK;
> >         val |= FOO_BAR;
> 
> Also we alreayd have a place where we write this registers. Is there
> some magic requirement why these bits can't be set there along with
> eveyrthing else?

We only write the bits of TRANS_DDI_FUNC_CTL currently but these bits
are written to TRANS_DDI_FUNC_CTL2 and need to be written before enabling
the transcoder.
Thats why I created this separate function here to set the bits in TRANS_DDI_FUNC_CTL2

Manasi

> 
> > 
> > Please actually use just "val" for the variable, the long name just
> > makes this all harder to read.
> > 
> > BR,
> > Jani.
> > 
> > 
> > > +	/* 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)
> > >  {
> > > @@ -5960,6 +6016,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 &&
> > 
> > -- 
> > Jani Nikula, Intel Open Source Graphics Center
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Fri, Mar 22, 2019 at 10:58:01AM -0700, Manasi Navare wrote:
> On Fri, Mar 22, 2019 at 03:16:03PM +0200, Ville Syrjälä wrote:
> > On Fri, Mar 22, 2019 at 11:34:25AM +0200, Jani Nikula wrote:
> > > On Thu, 21 Mar 2019, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > > > 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.
> > > >
> > > > 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 | 59 ++++++++++++++++++++++++++++
> > > >  1 file changed, 59 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 9980a4ed8c9c..16b46a3cb3bd 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -4009,6 +4009,62 @@ 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 *genlock_crtc_state = NULL;
> > > > +	enum transcoder genlock_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->genlock_crtc)
> > > > +		return;
> > > > +
> > > > +	genlock_crtc_state = intel_atomic_get_new_crtc_state(old_intel_state,
> > > > +							     crtc_state->genlock_crtc);
> > > > +	if (WARN_ON(!genlock_crtc_state))
> > > > +		return;
> > > > +
> > > > +	genlock_transcoder = genlock_crtc_state->cpu_transcoder;
> > > > +	switch (genlock_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 = I915_READ(TRANS_DDI_FUNC_CTL2(crtc_state->cpu_transcoder));
> > > > +	trans_ddi_func_ctl2_val |= (PORT_SYNC_MODE_MASTER_SELECT(master_select) &
> > > > +				    PORT_SYNC_MODE_MASTER_SELECT_MASK) <<
> > > > +		PORT_SYNC_MODE_MASTER_SELECT_SHIFT;
> > > 
> > > This doesn't do what you think it does. ITYM,
> > > 
> > > 	val = I915_READ();
> > >         val &= ~FOO_MASK;
> > >         val |= FOO_BAR;
> > 
> > Also we alreayd have a place where we write this registers. Is there
> > some magic requirement why these bits can't be set there along with
> > eveyrthing else?
> 
> We only write the bits of TRANS_DDI_FUNC_CTL currently but these bits
> are written to TRANS_DDI_FUNC_CTL2 and need to be written before enabling
> the transcoder.
> Thats why I created this separate function here to set the bits in TRANS_DDI_FUNC_CTL2

In that case there is no point in doing a rmw.

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

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

[Manasi Navare]

On Fri, Mar 22, 2019 at 08:09:50PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 22, 2019 at 10:58:01AM -0700, Manasi Navare wrote:
> > On Fri, Mar 22, 2019 at 03:16:03PM +0200, Ville Syrjälä wrote:
> > > On Fri, Mar 22, 2019 at 11:34:25AM +0200, Jani Nikula wrote:
> > > > On Thu, 21 Mar 2019, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > > > > 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.
> > > > >
> > > > > 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 | 59 ++++++++++++++++++++++++++++
> > > > >  1 file changed, 59 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > index 9980a4ed8c9c..16b46a3cb3bd 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -4009,6 +4009,62 @@ 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 *genlock_crtc_state = NULL;
> > > > > +	enum transcoder genlock_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->genlock_crtc)
> > > > > +		return;
> > > > > +
> > > > > +	genlock_crtc_state = intel_atomic_get_new_crtc_state(old_intel_state,
> > > > > +							     crtc_state->genlock_crtc);
> > > > > +	if (WARN_ON(!genlock_crtc_state))
> > > > > +		return;
> > > > > +
> > > > > +	genlock_transcoder = genlock_crtc_state->cpu_transcoder;
> > > > > +	switch (genlock_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 = I915_READ(TRANS_DDI_FUNC_CTL2(crtc_state->cpu_transcoder));
> > > > > +	trans_ddi_func_ctl2_val |= (PORT_SYNC_MODE_MASTER_SELECT(master_select) &
> > > > > +				    PORT_SYNC_MODE_MASTER_SELECT_MASK) <<
> > > > > +		PORT_SYNC_MODE_MASTER_SELECT_SHIFT;
> > > > 
> > > > This doesn't do what you think it does. ITYM,
> > > > 
> > > > 	val = I915_READ();
> > > >         val &= ~FOO_MASK;
> > > >         val |= FOO_BAR;
> > > 
> > > Also we alreayd have a place where we write this registers. Is there
> > > some magic requirement why these bits can't be set there along with
> > > eveyrthing else?
> > 
> > We only write the bits of TRANS_DDI_FUNC_CTL currently but these bits
> > are written to TRANS_DDI_FUNC_CTL2 and need to be written before enabling
> > the transcoder.
> > Thats why I created this separate function here to set the bits in TRANS_DDI_FUNC_CTL2
> 
> In that case there is no point in doing a rmw.

But isnt it always a good idea to do rmw? I mean what if the master select was set to something else
earlier?

Also could you look at the patch 1 of this series that assigns the genlock crtc pointer?

Manasi

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

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

[Ville Syrjälä]

On Fri, Mar 22, 2019 at 11:44:21AM -0700, Manasi Navare wrote:
> On Fri, Mar 22, 2019 at 08:09:50PM +0200, Ville Syrjälä wrote:
> > On Fri, Mar 22, 2019 at 10:58:01AM -0700, Manasi Navare wrote:
> > > On Fri, Mar 22, 2019 at 03:16:03PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Mar 22, 2019 at 11:34:25AM +0200, Jani Nikula wrote:
> > > > > On Thu, 21 Mar 2019, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > > > > > 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.
> > > > > >
> > > > > > 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 | 59 ++++++++++++++++++++++++++++
> > > > > >  1 file changed, 59 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > > index 9980a4ed8c9c..16b46a3cb3bd 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > @@ -4009,6 +4009,62 @@ 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 *genlock_crtc_state = NULL;
> > > > > > +	enum transcoder genlock_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->genlock_crtc)
> > > > > > +		return;
> > > > > > +
> > > > > > +	genlock_crtc_state = intel_atomic_get_new_crtc_state(old_intel_state,
> > > > > > +							     crtc_state->genlock_crtc);
> > > > > > +	if (WARN_ON(!genlock_crtc_state))
> > > > > > +		return;
> > > > > > +
> > > > > > +	genlock_transcoder = genlock_crtc_state->cpu_transcoder;
> > > > > > +	switch (genlock_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 = I915_READ(TRANS_DDI_FUNC_CTL2(crtc_state->cpu_transcoder));
> > > > > > +	trans_ddi_func_ctl2_val |= (PORT_SYNC_MODE_MASTER_SELECT(master_select) &
> > > > > > +				    PORT_SYNC_MODE_MASTER_SELECT_MASK) <<
> > > > > > +		PORT_SYNC_MODE_MASTER_SELECT_SHIFT;
> > > > > 
> > > > > This doesn't do what you think it does. ITYM,
> > > > > 
> > > > > 	val = I915_READ();
> > > > >         val &= ~FOO_MASK;
> > > > >         val |= FOO_BAR;
> > > > 
> > > > Also we alreayd have a place where we write this registers. Is there
> > > > some magic requirement why these bits can't be set there along with
> > > > eveyrthing else?
> > > 
> > > We only write the bits of TRANS_DDI_FUNC_CTL currently but these bits
> > > are written to TRANS_DDI_FUNC_CTL2 and need to be written before enabling
> > > the transcoder.
> > > Thats why I created this separate function here to set the bits in TRANS_DDI_FUNC_CTL2
> > 
> > In that case there is no point in doing a rmw.
> 
> But isnt it always a good idea to do rmw? I mean what if the master select was set to something else
> earlier?

RMW is the root of many evils. It should be avoided unless there is
a really compelling reason to use it.

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

RMW considered harmful (was: Re: [PATCH 2/2] drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports)

[Jani Nikula]

On Fri, 22 Mar 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Mar 22, 2019 at 11:44:21AM -0700, Manasi Navare wrote:
>> On Fri, Mar 22, 2019 at 08:09:50PM +0200, Ville Syrjälä wrote:
>> > In that case there is no point in doing a rmw.
>> 
>> But isnt it always a good idea to do rmw? I mean what if the master
>> select was set to something else earlier?
>
> RMW is the root of many evils. It should be avoided unless there is a
> really compelling reason to use it.

Hear, hear!

We have the software state that we want to write to the hardware. If we
use RMW to do this, it might all work by coincidence due to the old
values in the registers, or it might just as well break by coincidence
due to some garbage in the registers.

In most cases, there should only be one place that writes a particular
display register during modeset. Sometimes this isn't possible, and RMW
is required.

Some registers also have reserved bits potentially used by the hardware
that must not be changed, and RMW is required. These are documented in
bspec.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: RMW considered harmful (was: Re: [PATCH 2/2] drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports)

[Manasi Navare]

On Fri, Mar 22, 2019 at 09:28:01PM +0200, Jani Nikula wrote:
> On Fri, 22 Mar 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Mar 22, 2019 at 11:44:21AM -0700, Manasi Navare wrote:
> >> On Fri, Mar 22, 2019 at 08:09:50PM +0200, Ville Syrjälä wrote:
> >> > In that case there is no point in doing a rmw.
> >> 
> >> But isnt it always a good idea to do rmw? I mean what if the master
> >> select was set to something else earlier?
> >
> > RMW is the root of many evils. It should be avoided unless there is a
> > really compelling reason to use it.
> 
> Hear, hear!
> 
> We have the software state that we want to write to the hardware. If we
> use RMW to do this, it might all work by coincidence due to the old
> values in the registers, or it might just as well break by coincidence
> due to some garbage in the registers.
> 
> In most cases, there should only be one place that writes a particular
> display register during modeset. Sometimes this isn't possible, and RMW
> is required.
> 
> Some registers also have reserved bits potentially used by the hardware
> that must not be changed, and RMW is required. These are documented in
> bspec.
> 
> BR,
> Jani.
>

Thanks for the explanation. It does make sense now that we are doing a full
modeset, we should just be then writing the value directly?
The only concern I have is that say DSI code sets this somewhere els ein the modeset path,
then we would need to modify this to do RMW or always make sure DSI also
uses the same function for writing to this reg.
What do you suggest doing now?

Manasi
 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: RMW considered harmful (was: Re: [PATCH 2/2] drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports)

[Jani Nikula]

On Fri, 22 Mar 2019, Manasi Navare <manasi.d.navare@intel.com> wrote:
> On Fri, Mar 22, 2019 at 09:28:01PM +0200, Jani Nikula wrote:
>> On Fri, 22 Mar 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Fri, Mar 22, 2019 at 11:44:21AM -0700, Manasi Navare wrote:
>> >> On Fri, Mar 22, 2019 at 08:09:50PM +0200, Ville Syrjälä wrote:
>> >> > In that case there is no point in doing a rmw.
>> >> 
>> >> But isnt it always a good idea to do rmw? I mean what if the master
>> >> select was set to something else earlier?
>> >
>> > RMW is the root of many evils. It should be avoided unless there is a
>> > really compelling reason to use it.
>> 
>> Hear, hear!
>> 
>> We have the software state that we want to write to the hardware. If we
>> use RMW to do this, it might all work by coincidence due to the old
>> values in the registers, or it might just as well break by coincidence
>> due to some garbage in the registers.
>> 
>> In most cases, there should only be one place that writes a particular
>> display register during modeset. Sometimes this isn't possible, and RMW
>> is required.
>> 
>> Some registers also have reserved bits potentially used by the hardware
>> that must not be changed, and RMW is required. These are documented in
>> bspec.
>> 
>> BR,
>> Jani.
>>
>
> Thanks for the explanation. It does make sense now that we are doing a
> full modeset, we should just be then writing the value directly?  The
> only concern I have is that say DSI code sets this somewhere els ein
> the modeset path, then we would need to modify this to do RMW or
> always make sure DSI also uses the same function for writing to this
> reg.  What do you suggest doing now?

I think all encoders in a tile group are always of the same type.

If the tile grouping in your patch is based purely on EDID, we may need
to enforce this. Surely genlock only works on encoders of the same type?

In any case DSI (at least currently) does not use tile groups, and will
never be mixed up in non-DSI tile groups. The DSI transcoders are
separate from other transcoders, so we're not writing the same registers
here.

---

Looking at the code, I am wondering if this should be pushed to encoder
hooks instead of adding into crtc enable.

BR,
Jani.



-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: RMW considered harmful (was: Re: [PATCH 2/2] drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports)

[Manasi Navare]

On Thu, Mar 28, 2019 at 11:18:56AM +0200, Jani Nikula wrote:
> On Fri, 22 Mar 2019, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > On Fri, Mar 22, 2019 at 09:28:01PM +0200, Jani Nikula wrote:
> >> On Fri, 22 Mar 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> > On Fri, Mar 22, 2019 at 11:44:21AM -0700, Manasi Navare wrote:
> >> >> On Fri, Mar 22, 2019 at 08:09:50PM +0200, Ville Syrjälä wrote:
> >> >> > In that case there is no point in doing a rmw.
> >> >> 
> >> >> But isnt it always a good idea to do rmw? I mean what if the master
> >> >> select was set to something else earlier?
> >> >
> >> > RMW is the root of many evils. It should be avoided unless there is a
> >> > really compelling reason to use it.
> >> 
> >> Hear, hear!
> >> 
> >> We have the software state that we want to write to the hardware. If we
> >> use RMW to do this, it might all work by coincidence due to the old
> >> values in the registers, or it might just as well break by coincidence
> >> due to some garbage in the registers.
> >> 
> >> In most cases, there should only be one place that writes a particular
> >> display register during modeset. Sometimes this isn't possible, and RMW
> >> is required.
> >> 
> >> Some registers also have reserved bits potentially used by the hardware
> >> that must not be changed, and RMW is required. These are documented in
> >> bspec.
> >> 
> >> BR,
> >> Jani.
> >>
> >
> > Thanks for the explanation. It does make sense now that we are doing a
> > full modeset, we should just be then writing the value directly?  The
> > only concern I have is that say DSI code sets this somewhere els ein
> > the modeset path, then we would need to modify this to do RMW or
> > always make sure DSI also uses the same function for writing to this
> > reg.  What do you suggest doing now?
> 
> I think all encoders in a tile group are always of the same type.

Yes all the encoders in  tile group are always same type.

> 
> If the tile grouping in your patch is based purely on EDID, we may need
> to enforce this. Surely genlock only works on encoders of the same type?
>

So all the slaves and their master will always be of same type and yes it is
based on the EDID tile block parsing.
But just to double sure I think when i assign the master slave pointers, I should
check that the connector type is the same.
 
> In any case DSI (at least currently) does not use tile groups, and will
> never be mixed up in non-DSI tile groups. The DSI transcoders are
> separate from other transcoders, so we're not writing the same registers
> here.
> 
> ---
> 
> Looking at the code, I am wondering if this should be pushed to encoder
> hooks instead of adding into crtc enable.

As per the Bspec sequence, this needs to happen before enabling the TRANS_DDI_FUNC_CTL
and after the link training, so I put in the crtc_enable hook, which encoder hooks
are you suggesting adding this?

Regards
Manasi
> 
> BR,
> Jani.
> 
> 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: RMW considered harmful (was: Re: [PATCH 2/2] drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports)

[Jani Nikula]

On Thu, 28 Mar 2019, Manasi Navare <manasi.d.navare@intel.com> wrote:
> On Thu, Mar 28, 2019 at 11:18:56AM +0200, Jani Nikula wrote:
>> On Fri, 22 Mar 2019, Manasi Navare <manasi.d.navare@intel.com> wrote:
>> > On Fri, Mar 22, 2019 at 09:28:01PM +0200, Jani Nikula wrote:
>> >> On Fri, 22 Mar 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> >> > On Fri, Mar 22, 2019 at 11:44:21AM -0700, Manasi Navare wrote:
>> >> >> On Fri, Mar 22, 2019 at 08:09:50PM +0200, Ville Syrjälä wrote:
>> >> >> > In that case there is no point in doing a rmw.
>> >> >> 
>> >> >> But isnt it always a good idea to do rmw? I mean what if the master
>> >> >> select was set to something else earlier?
>> >> >
>> >> > RMW is the root of many evils. It should be avoided unless there is a
>> >> > really compelling reason to use it.
>> >> 
>> >> Hear, hear!
>> >> 
>> >> We have the software state that we want to write to the hardware. If we
>> >> use RMW to do this, it might all work by coincidence due to the old
>> >> values in the registers, or it might just as well break by coincidence
>> >> due to some garbage in the registers.
>> >> 
>> >> In most cases, there should only be one place that writes a particular
>> >> display register during modeset. Sometimes this isn't possible, and RMW
>> >> is required.
>> >> 
>> >> Some registers also have reserved bits potentially used by the hardware
>> >> that must not be changed, and RMW is required. These are documented in
>> >> bspec.
>> >> 
>> >> BR,
>> >> Jani.
>> >>
>> >
>> > Thanks for the explanation. It does make sense now that we are doing a
>> > full modeset, we should just be then writing the value directly?  The
>> > only concern I have is that say DSI code sets this somewhere els ein
>> > the modeset path, then we would need to modify this to do RMW or
>> > always make sure DSI also uses the same function for writing to this
>> > reg.  What do you suggest doing now?
>> 
>> I think all encoders in a tile group are always of the same type.
>
> Yes all the encoders in  tile group are always same type.
>
>> 
>> If the tile grouping in your patch is based purely on EDID, we may need
>> to enforce this. Surely genlock only works on encoders of the same type?
>>
>
> So all the slaves and their master will always be of same type and yes it is
> based on the EDID tile block parsing.
> But just to double sure I think when i assign the master slave pointers, I should
> check that the connector type is the same.
>  
>> In any case DSI (at least currently) does not use tile groups, and will
>> never be mixed up in non-DSI tile groups. The DSI transcoders are
>> separate from other transcoders, so we're not writing the same registers
>> here.
>> 
>> ---
>> 
>> Looking at the code, I am wondering if this should be pushed to encoder
>> hooks instead of adding into crtc enable.
>
> As per the Bspec sequence, this needs to happen before enabling the
> TRANS_DDI_FUNC_CTL and after the link training, so I put in the
> crtc_enable hook, which encoder hooks are you suggesting adding this?

Maybe go with what you have now first, this can be pushed to encoders
later if needed. (I hope I don't regret this. ;)

BR,
Jani.



>
> Regards
> Manasi
>> 
>> BR,
>> Jani.
>> 
>> 
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Manasi Navare]

On Fri, Mar 22, 2019 at 11:34:25AM +0200, Jani Nikula wrote:
> On Thu, 21 Mar 2019, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > 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.
> >
> > 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 | 59 ++++++++++++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 9980a4ed8c9c..16b46a3cb3bd 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4009,6 +4009,62 @@ 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 *genlock_crtc_state = NULL;
> > +	enum transcoder genlock_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->genlock_crtc)
> > +		return;
> > +
> > +	genlock_crtc_state = intel_atomic_get_new_crtc_state(old_intel_state,
> > +							     crtc_state->genlock_crtc);
> > +	if (WARN_ON(!genlock_crtc_state))
> > +		return;
> > +
> > +	genlock_transcoder = genlock_crtc_state->cpu_transcoder;
> > +	switch (genlock_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 = I915_READ(TRANS_DDI_FUNC_CTL2(crtc_state->cpu_transcoder));
> > +	trans_ddi_func_ctl2_val |= (PORT_SYNC_MODE_MASTER_SELECT(master_select) &
> > +				    PORT_SYNC_MODE_MASTER_SELECT_MASK) <<
> > +		PORT_SYNC_MODE_MASTER_SELECT_SHIFT;
> 
> This doesn't do what you think it does. ITYM,
> 
> 	val = I915_READ();
>         val &= ~FOO_MASK;
>         val |= FOO_BAR;

Ah, yes I should use the mask to clear the value and then OR the new value.
Will make this change, thanks for pointing this out.

> 
> Please actually use just "val" for the variable, the long name just
> makes this all harder to read.

Okay will change the variable name to val.

Regards
Manasi
> 
> BR,
> Jani.
> 
> 
> > +	/* 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)
> >  {
> > @@ -5960,6 +6016,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 &&
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915/icl: Assign genlock CRTC pointer in all slave CRTC states for tiled displays

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/icl: Assign genlock CRTC pointer in all slave CRTC states for tiled displays
URL   : https://patchwork.freedesktop.org/series/58393/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
d15c48b8741b drm/i915/icl: Assign genlock CRTC pointer in all slave CRTC states for tiled displays
-:64: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#64: FILE: drivers/gpu/drm/i915/intel_display.c:11314:
+		slave_crtc_state = to_intel_crtc_state(

total: 0 errors, 0 warnings, 1 checks, 105 lines checked
fc2128ba4096 drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/icl: Assign genlock CRTC pointer in all slave CRTC states for tiled displays

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/icl: Assign genlock CRTC pointer in all slave CRTC states for tiled displays
URL   : https://patchwork.freedesktop.org/series/58393/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5792 -> Patchwork_12567
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@userptr:
    - fi-kbl-8809g:       PASS -> DMESG-WARN [fdo#108965]

  * igt@gem_ctx_create@basic-files:
    - fi-gdg-551:         NOTRUN -> SKIP [fdo#109271] +106

  * igt@gem_exec_basic@readonly-bsd2:
    - fi-pnv-d510:        NOTRUN -> SKIP [fdo#109271] +76

  * igt@i915_selftest@live_uncore:
    - fi-skl-gvtdvm:      PASS -> DMESG-FAIL [fdo#110210]

  * igt@kms_busy@basic-flip-c:
    - fi-gdg-551:         NOTRUN -> SKIP [fdo#109271] / [fdo#109278]
    - fi-pnv-d510:        NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_psr@primary_mmap_gtt:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +27

  * igt@kms_psr@primary_page_flip:
    - fi-apl-guc:         NOTRUN -> SKIP [fdo#109271] +50

  * igt@prime_vgem@basic-fence-flip:
    - fi-gdg-551:         NOTRUN -> DMESG-FAIL [fdo#103182]

  
#### Possible fixes ####

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u3:          FAIL [fdo#103167] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#110210]: https://bugs.freedesktop.org/show_bug.cgi?id=110210


Participating hosts (40 -> 35)
------------------------------

  Additional (3): fi-gdg-551 fi-apl-guc fi-pnv-d510 
  Missing    (8): fi-ilk-m540 fi-hsw-4200u fi-byt-j1900 fi-byt-squawks fi-bsw-cyan fi-snb-2520m fi-ctg-p8600 fi-bdw-samus 


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

    * Linux: CI_DRM_5792 -> Patchwork_12567

  CI_DRM_5792: 109336adf97713e8d6f693a56f22f334b9f64ce9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4897: e12d69496a6bef09ac6c0f792b8d60a65cf5e0bf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12567: fc2128ba409640d2bebb3ba503fa3b81d2f80c90 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

fc2128ba4096 drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports
d15c48b8741b drm/i915/icl: Assign genlock CRTC pointer in all slave CRTC states for tiled displays

== Logs ==

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

✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915/icl: Assign genlock CRTC pointer in all slave CRTC states for tiled displays

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/icl: Assign genlock CRTC pointer in all slave CRTC states for tiled displays
URL   : https://patchwork.freedesktop.org/series/58393/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5792_full -> Patchwork_12567_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_schedule@preempt-other-chain-blt:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] +35

  * igt@gem_pread@stolen-uncached:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] +62

  * igt@gem_wait@write-wait-bsd1:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109276] +7

  * igt@i915_pm_backlight@fade_with_suspend:
    - shard-iclb:         NOTRUN -> FAIL [fdo#107847]

  * igt@i915_pm_rpm@i2c:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#109982]

  * igt@i915_pm_rps@reset:
    - shard-iclb:         NOTRUN -> FAIL [fdo#108059] +1

  * igt@kms_atomic_transition@3x-modeset-transitions-fencing:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109278] +2

  * igt@kms_busy@extended-modeset-hang-newfb-render-a:
    - shard-apl:          PASS -> DMESG-WARN [fdo#110222]

  * igt@kms_busy@extended-modeset-hang-newfb-render-d:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +4

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
    - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#110222]

  * igt@kms_busy@extended-modeset-hang-oldfb-render-e:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +6

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-a:
    - shard-glk:          PASS -> DMESG-WARN [fdo#110222]

  * igt@kms_chamelium@hdmi-crc-single:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109284]

  * igt@kms_cursor_legacy@2x-long-flip-vs-cursor-legacy:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109274] +7

  * igt@kms_dp_dsc@basic-dsc-enable-dp:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109349]

  * igt@kms_force_connector_basic@force-edid:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109285]

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-render:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@fbc-stridechange:
    - shard-iclb:         PASS -> FAIL [fdo#105682] / [fdo#108040]

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-blt:
    - shard-iclb:         PASS -> FAIL [fdo#109247] +22

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-scndscrn-cur-indfb-move:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109280] +16

  * igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-render:
    - shard-iclb:         PASS -> FAIL [fdo#105682] / [fdo#109247]

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-indfb-draw-blt:
    - shard-iclb:         NOTRUN -> FAIL [fdo#109247] +1

  * igt@kms_pipe_b_c_ivb@from-pipe-c-to-b-with-3-lanes:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109289]

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108]

  * igt@kms_plane_alpha_blend@pipe-a-alpha-transparant-fb:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_plane_alpha_blend@pipe-b-alpha-basic:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145] / [fdo#108590]

  * igt@kms_plane_alpha_blend@pipe-b-alpha-opaque-fb:
    - shard-glk:          PASS -> FAIL [fdo#108145] +1

  * igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb:
    - shard-apl:          PASS -> FAIL [fdo#108145]

  * igt@kms_plane_alpha_blend@pipe-c-constant-alpha-max:
    - shard-skl:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_plane_scaling@pipe-c-scaler-with-rotation:
    - shard-glk:          PASS -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_psr@psr2_dpms:
    - shard-iclb:         PASS -> SKIP [fdo#109441] +2

  * igt@kms_psr@psr2_primary_page_flip:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109441]

  * igt@kms_psr@sprite_render:
    - shard-iclb:         PASS -> FAIL [fdo#107383] / [fdo#110215] +1

  * igt@kms_setmode@basic:
    - shard-kbl:          PASS -> FAIL [fdo#99912]

  * igt@kms_universal_plane@disable-primary-vs-flip-pipe-d:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +4

  * igt@perf_pmu@semaphore-wait-vcs1:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] +60

  * igt@prime_nv_pcopy@test1_micro:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109291] +1

  * igt@prime_vgem@fence-read-hang:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109295]

  * igt@v3d_get_bo_offset@get-bad-handle:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109315]

  
#### Possible fixes ####

  * igt@gem_busy@close-race:
    - shard-apl:          FAIL -> PASS

  * igt@gem_create@create-clear:
    - shard-snb:          INCOMPLETE [fdo#105411] -> PASS

  * igt@gem_exec_schedule@wide-vebox:
    - shard-iclb:         FAIL [fdo#109633] -> PASS

  * igt@gem_ppgtt@blt-vs-render-ctx0:
    - shard-iclb:         INCOMPLETE [fdo#109801] -> PASS +1

  * igt@gem_softpin@noreloc-s3:
    - shard-skl:          INCOMPLETE [fdo#104108] / [fdo#107773] -> PASS

  * igt@gem_tiled_pread_pwrite:
    - shard-iclb:         TIMEOUT [fdo#109673] -> PASS

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions:
    - shard-skl:          FAIL [fdo#102670] -> PASS

  * igt@kms_fbcon_fbt@fbc:
    - shard-iclb:         DMESG-WARN [fdo#109593] -> PASS

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-iclb:         FAIL [fdo#105363] -> PASS

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-skl:          INCOMPLETE [fdo#109507] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw:
    - shard-iclb:         FAIL [fdo#103167] -> PASS +1

  * igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-pwrite:
    - shard-iclb:         FAIL [fdo#105682] / [fdo#109247] -> PASS +2

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-cur-indfb-draw-mmap-wc:
    - shard-iclb:         FAIL [fdo#109247] -> PASS +19

  * {igt@kms_plane@pixel-format-pipe-c-planes-source-clamping}:
    - shard-iclb:         INCOMPLETE [fdo#110036 ] -> PASS

  * igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min:
    - shard-skl:          FAIL [fdo#108145] -> PASS

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          FAIL [fdo#107815] -> PASS +1

  * igt@kms_psr@psr2_sprite_blt:
    - shard-iclb:         SKIP [fdo#109441] -> PASS +3

  * igt@kms_psr@sprite_mmap_gtt:
    - shard-iclb:         FAIL [fdo#107383] / [fdo#110215] -> PASS +3

  * igt@kms_setmode@basic:
    - shard-glk:          FAIL [fdo#99912] -> PASS

  * igt@perf@oa-exponents:
    - shard-glk:          FAIL [fdo#105483] -> PASS

  
#### Warnings ####

  * igt@gem_tiled_swapping@non-threaded:
    - shard-iclb:         INCOMPLETE [fdo#110197] -> FAIL [fdo#108686]

  * igt@i915_pm_rpm@dpms-mode-unset-non-lpsp:
    - shard-skl:          SKIP [fdo#109271] -> INCOMPLETE [fdo#107807]

  * igt@i915_selftest@live_contexts:
    - shard-iclb:         INCOMPLETE [fdo#108569] -> DMESG-FAIL [fdo#108569]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102670]: https://bugs.freedesktop.org/show_bug.cgi?id=102670
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#105483]: https://bugs.freedesktop.org/show_bug.cgi?id=105483
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#107383]: https://bugs.freedesktop.org/show_bug.cgi?id=107383
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
  [fdo#107847]: https://bugs.freedesktop.org/show_bug.cgi?id=107847
  [fdo#108040]: https://bugs.freedesktop.org/show_bug.cgi?id=108040
  [fdo#108059]: https://bugs.freedesktop.org/show_bug.cgi?id=108059
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109247]: https://bugs.freedesktop.org/show_bug.cgi?id=109247
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109507]: https://bugs.freedesktop.org/show_bug.cgi?id=109507
  [fdo#109593]: https://bugs.freedesktop.org/show_bug.cgi?id=109593
  [fdo#109633]: https://bugs.freedesktop.org/show_bug.cgi?id=109633
  [fdo#109673]: https://bugs.freedesktop.org/show_bug.cgi?id=109673
  [fdo#109801]: https://bugs.freedesktop.org/show_bug.cgi?id=109801
  [fdo#109982]: https://bugs.freedesktop.org/show_bug.cgi?id=109982
  [fdo#110036 ]: https://bugs.freedesktop.org/show_bug.cgi?id=110036 
  [fdo#110197]: https://bugs.freedesktop.org/show_bug.cgi?id=110197
  [fdo#110215]: https://bugs.freedesktop.org/show_bug.cgi?id=110215
  [fdo#110222]: https://bugs.freedesktop.org/show_bug.cgi?id=110222
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 9)
------------------------------

  Missing    (1): shard-hsw 


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

    * Linux: CI_DRM_5792 -> Patchwork_12567

  CI_DRM_5792: 109336adf97713e8d6f693a56f22f334b9f64ce9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4897: e12d69496a6bef09ac6c0f792b8d60a65cf5e0bf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12567: fc2128ba409640d2bebb3ba503fa3b81d2f80c90 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH 1/2] drm/i915/icl: Assign genlock CRTC pointer in all slave CRTC states for tiled displays

[Jani Nikula]

On Thu, 21 Mar 2019, Manasi Navare <manasi.d.navare@intel.com> 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.
>
> 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 | 84 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +
>  2 files changed, 87 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8ff7aa8cb3cf..9980a4ed8c9c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11281,6 +11281,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> +static int icl_add_genlock_crtcs(struct drm_device *dev,
> +				 struct drm_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_connector *genlock_connector, *connector;
> +	struct drm_connector_state *connector_state;
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_crtc *genlock_crtc = NULL;
> +	struct drm_crtc_state *genlock_crtc_state;
> +	struct intel_crtc_state *slave_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)
> +			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;
> +		slave_crtc_state = to_intel_crtc_state(
> +			drm_atomic_get_new_crtc_state(state,
> +						      connector_state->crtc));
> +		slave_crtc_state->genlock_crtc = NULL;
> +		tile_group_id = connector->tile_group->id;
> +		drm_connector_list_iter_begin(dev, &conn_iter);
> +		drm_for_each_connector_iter(genlock_connector, &conn_iter) {
> +			struct drm_connector_state *genlock_conn_state = NULL;
> +
> +			if (!genlock_connector->has_tile)
> +				continue;
> +			if (genlock_connector->tile_h_loc != genlock_connector->num_h_tile - 1 ||
> +			    genlock_connector->tile_v_loc != genlock_connector->num_v_tile - 1)
> +				continue;
> +			if (genlock_connector->tile_group->id != tile_group_id)
> +				continue;
> +
> +			genlock_conn_state = drm_atomic_get_connector_state(state,
> +									    genlock_connector);
> +			if (IS_ERR(genlock_conn_state)) {
> +				drm_connector_list_iter_end(&conn_iter);
> +				return PTR_ERR(genlock_conn_state);
> +			}
> +			if (genlock_conn_state->crtc) {
> +				genlock_crtc = genlock_conn_state->crtc;
> +				break;
> +			}
> +		}
> +		drm_connector_list_iter_end(&conn_iter);

The above loop would benefit from being abstracted to a separate
function. "find genlock master based on tile info"

I wonder if it would work to have each relevant encoder ->compute_config
hook look for its genlock master instead of adding another top level
loop.

BR,
Jani.


> +
> +		if (!genlock_crtc) {
> +			DRM_DEBUG_KMS("Could not add Genlock CRTC for Slave CRTC %d\n",
> +				      connector_state->crtc->base.id);
> +			return -EINVAL;
> +		}
> +
> +		genlock_crtc_state = drm_atomic_get_crtc_state(state,
> +							       genlock_crtc);
> +		if (IS_ERR(genlock_crtc_state))
> +			return PTR_ERR(genlock_crtc_state);
> +
> +		slave_crtc_state->genlock_crtc = to_intel_crtc(genlock_crtc);
> +		DRM_DEBUG_KMS("Genlock CRTC = %d added for Slave CRTC = %d\n",
> +			      genlock_crtc->base.id,
> +			      slave_crtc_state->base.crtc->base.id);
> +	}
> +
> +	return 0;
> +}
> +
>  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  				   struct drm_crtc_state *crtc_state)
>  {
> @@ -13098,6 +13178,10 @@ static int intel_atomic_check(struct drm_device *dev,
>  	if (ret)
>  		return ret;
>  
> +	ret = icl_add_genlock_crtcs(dev, state);
> +	if (ret)
> +		return ret;
> +
>  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) {
>  		struct intel_crtc_state *pipe_config =
>  			to_intel_crtc_state(crtc_state);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4d7ae579fc92..b41728946b73 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1074,6 +1074,9 @@ struct intel_crtc_state {
>  
>  	/* Forward Error correction State */
>  	bool fec_enable;
> +
> +	/* Pointer to master/genlock crtc in case of tiled displays */
> +	struct intel_crtc *genlock_crtc;
>  };
>  
>  struct intel_crtc {

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/icl: Assign genlock CRTC pointer in all slave CRTC states for tiled displays

[Manasi Navare]

On Thu, Mar 28, 2019 at 11:32:18AM +0200, Jani Nikula wrote:
> On Thu, 21 Mar 2019, Manasi Navare <manasi.d.navare@intel.com> 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.
> >
> > 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 | 84 ++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 +
> >  2 files changed, 87 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 8ff7aa8cb3cf..9980a4ed8c9c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11281,6 +11281,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
> >  	return 0;
> >  }
> >  
> > +static int icl_add_genlock_crtcs(struct drm_device *dev,
> > +				 struct drm_atomic_state *state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_connector *genlock_connector, *connector;
> > +	struct drm_connector_state *connector_state;
> > +	struct drm_connector_list_iter conn_iter;
> > +	struct drm_crtc *genlock_crtc = NULL;
> > +	struct drm_crtc_state *genlock_crtc_state;
> > +	struct intel_crtc_state *slave_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)
> > +			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;
> > +		slave_crtc_state = to_intel_crtc_state(
> > +			drm_atomic_get_new_crtc_state(state,
> > +						      connector_state->crtc));
> > +		slave_crtc_state->genlock_crtc = NULL;
> > +		tile_group_id = connector->tile_group->id;
> > +		drm_connector_list_iter_begin(dev, &conn_iter);
> > +		drm_for_each_connector_iter(genlock_connector, &conn_iter) {
> > +			struct drm_connector_state *genlock_conn_state = NULL;
> > +
> > +			if (!genlock_connector->has_tile)
> > +				continue;
> > +			if (genlock_connector->tile_h_loc != genlock_connector->num_h_tile - 1 ||
> > +			    genlock_connector->tile_v_loc != genlock_connector->num_v_tile - 1)
> > +				continue;
> > +			if (genlock_connector->tile_group->id != tile_group_id)
> > +				continue;
> > +
> > +			genlock_conn_state = drm_atomic_get_connector_state(state,
> > +									    genlock_connector);
> > +			if (IS_ERR(genlock_conn_state)) {
> > +				drm_connector_list_iter_end(&conn_iter);
> > +				return PTR_ERR(genlock_conn_state);
> > +			}
> > +			if (genlock_conn_state->crtc) {
> > +				genlock_crtc = genlock_conn_state->crtc;
> > +				break;
> > +			}
> > +		}
> > +		drm_connector_list_iter_end(&conn_iter);
> 
> The above loop would benefit from being abstracted to a separate
> function. "find genlock master based on tile info"
> 
> I wonder if it would work to have each relevant encoder ->compute_config
> hook look for its genlock master instead of adding another top level
> loop.

So are you suggesting adding this directly inside say compute_config() hook
for DP encoder so inside intel_dp_compute_config(), or would it be better to add
just the inner find_genlock_master() function in the existing for_each_connector() in intel_modeset_pipe_config()
before calling into the compute_config encoder hook?

Regards
Manasi
> 
> BR,
> Jani.
> 
> 
> > +
> > +		if (!genlock_crtc) {
> > +			DRM_DEBUG_KMS("Could not add Genlock CRTC for Slave CRTC %d\n",
> > +				      connector_state->crtc->base.id);
> > +			return -EINVAL;
> > +		}
> > +
> > +		genlock_crtc_state = drm_atomic_get_crtc_state(state,
> > +							       genlock_crtc);
> > +		if (IS_ERR(genlock_crtc_state))
> > +			return PTR_ERR(genlock_crtc_state);
> > +
> > +		slave_crtc_state->genlock_crtc = to_intel_crtc(genlock_crtc);
> > +		DRM_DEBUG_KMS("Genlock CRTC = %d added for Slave CRTC = %d\n",
> > +			      genlock_crtc->base.id,
> > +			      slave_crtc_state->base.crtc->base.id);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> >  				   struct drm_crtc_state *crtc_state)
> >  {
> > @@ -13098,6 +13178,10 @@ static int intel_atomic_check(struct drm_device *dev,
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = icl_add_genlock_crtcs(dev, state);
> > +	if (ret)
> > +		return ret;
> > +
> >  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) {
> >  		struct intel_crtc_state *pipe_config =
> >  			to_intel_crtc_state(crtc_state);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 4d7ae579fc92..b41728946b73 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1074,6 +1074,9 @@ struct intel_crtc_state {
> >  
> >  	/* Forward Error correction State */
> >  	bool fec_enable;
> > +
> > +	/* Pointer to master/genlock crtc in case of tiled displays */
> > +	struct intel_crtc *genlock_crtc;
> >  };
> >  
> >  struct intel_crtc {
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx