[Intel-gfx] [PATCH v2 rebased 01/11] drm: Add __drm_atomic_helper_crtc_state_reset() & co.

[Intel-gfx] [PATCH v2 rebased 01/11] drm: Add __drm_atomic_helper_crtc_state_reset() & co.

[José Roberto de Souza]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Annoyingly __drm_atomic_helper_crtc_reset() does two
totally separate things:
a) reset the state to defaults values
b) assign the crtc->state pointer

I just want a) without the b) so let's split out part
a) into __drm_atomic_helper_crtc_state_reset(). And
of course we'll do the same thing for planes and connectors.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_atomic_state_helper.c | 70 ++++++++++++++++++++---
 include/drm/drm_atomic_state_helper.h     |  6 ++
 2 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index d0a937fb0c56..a972068d58cf 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -57,6 +57,22 @@
  * for these functions.
  */
 
+/**
+ * __drm_atomic_helper_crtc_state_reset - reset the CRTC state
+ * @crtc_state: atomic CRTC state, must not be NULL
+ * @crtc: CRTC object, must not be NULL
+ *
+ * Initializes the newly allocated @crtc_state with default
+ * values. This is useful for drivers that subclass the CRTC state.
+ */
+void
+__drm_atomic_helper_crtc_state_reset(struct drm_crtc_state *crtc_state,
+				     struct drm_crtc *crtc)
+{
+	crtc_state->crtc = crtc;
+}
+EXPORT_SYMBOL(__drm_atomic_helper_crtc_state_reset);
+
 /**
  * __drm_atomic_helper_crtc_reset - reset state on CRTC
  * @crtc: drm CRTC
@@ -74,7 +90,7 @@ __drm_atomic_helper_crtc_reset(struct drm_crtc *crtc,
 			       struct drm_crtc_state *crtc_state)
 {
 	if (crtc_state)
-		crtc_state->crtc = crtc;
+		__drm_atomic_helper_crtc_state_reset(crtc_state, crtc);
 
 	crtc->state = crtc_state;
 }
@@ -212,23 +228,43 @@ void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
 EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
 
 /**
- * __drm_atomic_helper_plane_reset - resets planes state to default values
+ * __drm_atomic_helper_plane_state_reset - resets plane state to default values
+ * @plane_state: atomic plane state, must not be NULL
  * @plane: plane object, must not be NULL
- * @state: atomic plane state, must not be NULL
  *
- * Initializes plane state to default. This is useful for drivers that subclass
- * the plane state.
+ * Initializes the newly allocated @plane_state with default
+ * values. This is useful for drivers that subclass the CRTC state.
  */
-void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
-				     struct drm_plane_state *state)
+void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *state,
+					   struct drm_plane *plane)
 {
 	state->plane = plane;
 	state->rotation = DRM_MODE_ROTATE_0;
 
 	state->alpha = DRM_BLEND_ALPHA_OPAQUE;
 	state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
+}
+EXPORT_SYMBOL(__drm_atomic_helper_plane_state_reset);
 
-	plane->state = state;
+/**
+ * __drm_atomic_helper_plane_reset - reset state on plane
+ * @plane: drm plane
+ * @plane_state: plane state to assign
+ *
+ * Initializes the newly allocated @plane_state and assigns it to
+ * the &drm_crtc->state pointer of @plane, usually required when
+ * initializing the drivers or when called from the &drm_plane_funcs.reset
+ * hook.
+ *
+ * This is useful for drivers that subclass the plane state.
+ */
+void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
+				     struct drm_plane_state *plane_state)
+{
+	if (plane_state)
+		__drm_atomic_helper_plane_state_reset(plane_state, plane);
+
+	plane->state = plane_state;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_reset);
 
@@ -335,6 +371,22 @@ void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
 }
 EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
 
+/**
+ * __drm_atomic_helper_connector_state_reset - reset the connector state
+ * @conn__state: atomic connector state, must not be NULL
+ * @connector: connectotr object, must not be NULL
+ *
+ * Initializes the newly allocated @conn_state with default
+ * values. This is useful for drivers that subclass the connector state.
+ */
+void
+__drm_atomic_helper_connector_state_reset(struct drm_connector_state *conn_state,
+					  struct drm_connector *connector)
+{
+	conn_state->connector = connector;
+}
+EXPORT_SYMBOL(__drm_atomic_helper_connector_state_reset);
+
 /**
  * __drm_atomic_helper_connector_reset - reset state on connector
  * @connector: drm connector
@@ -352,7 +404,7 @@ __drm_atomic_helper_connector_reset(struct drm_connector *connector,
 				    struct drm_connector_state *conn_state)
 {
 	if (conn_state)
-		conn_state->connector = connector;
+		__drm_atomic_helper_connector_state_reset(conn_state, connector);
 
 	connector->state = conn_state;
 }
diff --git a/include/drm/drm_atomic_state_helper.h b/include/drm/drm_atomic_state_helper.h
index e4577cc11689..8171dea4cc22 100644
--- a/include/drm/drm_atomic_state_helper.h
+++ b/include/drm/drm_atomic_state_helper.h
@@ -37,6 +37,8 @@ struct drm_private_state;
 struct drm_modeset_acquire_ctx;
 struct drm_device;
 
+void __drm_atomic_helper_crtc_state_reset(struct drm_crtc_state *state,
+					  struct drm_crtc *crtc);
 void __drm_atomic_helper_crtc_reset(struct drm_crtc *crtc,
 				    struct drm_crtc_state *state);
 void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc);
@@ -48,6 +50,8 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state);
 void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
 					  struct drm_crtc_state *state);
 
+void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *state,
+					   struct drm_plane *plane);
 void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
 				     struct drm_plane_state *state);
 void drm_atomic_helper_plane_reset(struct drm_plane *plane);
@@ -59,6 +63,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state);
 void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
 					  struct drm_plane_state *state);
 
+void __drm_atomic_helper_connector_state_reset(struct drm_connector_state *conn_state,
+					       struct drm_connector *connector);
 void __drm_atomic_helper_connector_reset(struct drm_connector *connector,
 					 struct drm_connector_state *conn_state);
 void drm_atomic_helper_connector_reset(struct drm_connector *connector);
-- 
2.24.1

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

[Intel-gfx] [PATCH v2 rebased 02/11] drm/i915: s/intel_crtc/crtc/ in intel_crtc_init()

[José Roberto de Souza]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Let's get rid of the redundant intel_ prefix on our variables.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 32 ++++++++++----------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 65579594f13b..3c3ef6291eab 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -15790,14 +15790,14 @@ static const struct drm_crtc_funcs i8xx_crtc_funcs = {
 static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 {
 	const struct drm_crtc_funcs *funcs;
-	struct intel_crtc *intel_crtc;
+	struct intel_crtc *crtc;
 	struct intel_crtc_state *crtc_state = NULL;
 	struct intel_plane *primary = NULL;
 	struct intel_plane *cursor = NULL;
 	int sprite, ret;
 
-	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
-	if (!intel_crtc)
+	crtc = kzalloc(sizeof(*crtc), GFP_KERNEL);
+	if (!crtc)
 		return -ENOMEM;
 
 	crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
@@ -15805,15 +15805,15 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 		ret = -ENOMEM;
 		goto fail;
 	}
-	__drm_atomic_helper_crtc_reset(&intel_crtc->base, &crtc_state->uapi);
-	intel_crtc->config = crtc_state;
+	__drm_atomic_helper_crtc_reset(&crtc->base, &crtc_state->uapi);
+	crtc->config = crtc_state;
 
 	primary = intel_primary_plane_create(dev_priv, pipe);
 	if (IS_ERR(primary)) {
 		ret = PTR_ERR(primary);
 		goto fail;
 	}
-	intel_crtc->plane_ids_mask |= BIT(primary->id);
+	crtc->plane_ids_mask |= BIT(primary->id);
 
 	for_each_sprite(dev_priv, pipe, sprite) {
 		struct intel_plane *plane;
@@ -15823,7 +15823,7 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 			ret = PTR_ERR(plane);
 			goto fail;
 		}
-		intel_crtc->plane_ids_mask |= BIT(plane->id);
+		crtc->plane_ids_mask |= BIT(plane->id);
 	}
 
 	cursor = intel_cursor_plane_create(dev_priv, pipe);
@@ -15831,7 +15831,7 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 		ret = PTR_ERR(cursor);
 		goto fail;
 	}
-	intel_crtc->plane_ids_mask |= BIT(cursor->id);
+	crtc->plane_ids_mask |= BIT(cursor->id);
 
 	if (HAS_GMCH(dev_priv)) {
 		if (IS_CHERRYVIEW(dev_priv) ||
@@ -15852,32 +15852,32 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 			funcs = &ilk_crtc_funcs;
 	}
 
-	ret = drm_crtc_init_with_planes(&dev_priv->drm, &intel_crtc->base,
+	ret = drm_crtc_init_with_planes(&dev_priv->drm, &crtc->base,
 					&primary->base, &cursor->base,
 					funcs, "pipe %c", pipe_name(pipe));
 	if (ret)
 		goto fail;
 
-	intel_crtc->pipe = pipe;
+	crtc->pipe = pipe;
 
 	/* initialize shared scalers */
-	intel_crtc_init_scalers(intel_crtc, crtc_state);
+	intel_crtc_init_scalers(crtc, crtc_state);
 
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->pipe_to_crtc_mapping) ||
 	       dev_priv->pipe_to_crtc_mapping[pipe] != NULL);
-	dev_priv->pipe_to_crtc_mapping[pipe] = intel_crtc;
+	dev_priv->pipe_to_crtc_mapping[pipe] = crtc;
 
 	if (INTEL_GEN(dev_priv) < 9) {
 		enum i9xx_plane_id i9xx_plane = primary->i9xx_plane;
 
 		BUG_ON(i9xx_plane >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 		       dev_priv->plane_to_crtc_mapping[i9xx_plane] != NULL);
-		dev_priv->plane_to_crtc_mapping[i9xx_plane] = intel_crtc;
+		dev_priv->plane_to_crtc_mapping[i9xx_plane] = crtc;
 	}
 
-	intel_color_init(intel_crtc);
+	intel_color_init(crtc);
 
-	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
+	WARN_ON(drm_crtc_index(&crtc->base) != crtc->pipe);
 
 	return 0;
 
@@ -15887,7 +15887,7 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 	 * crtcs/planes already initialized.
 	 */
 	kfree(crtc_state);
-	kfree(intel_crtc);
+	kfree(crtc);
 
 	return ret;
 }
-- 
2.24.1

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

[Intel-gfx] [PATCH v2 rebased 03/11] drm/i915: Introduce intel_crtc_{alloc, free}()

[José Roberto de Souza]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We already have alloc/free helpers for planes, add the same for
crtcs. The main benefit is we get to move all the annoying state
initialization out of the main crtc_init() flow.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 74 ++++++++++----------
 1 file changed, 36 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 3c3ef6291eab..b6d364dd3e4c 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -164,8 +164,7 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_state *pipe_config);
 static void chv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_state *pipe_config);
-static void intel_crtc_init_scalers(struct intel_crtc *crtc,
-				    struct intel_crtc_state *crtc_state);
+static void intel_crtc_init_scalers(struct intel_crtc_state *crtc_state);
 static void skylake_pfit_enable(const struct intel_crtc_state *crtc_state);
 static void ironlake_pfit_disable(const struct intel_crtc_state *old_crtc_state);
 static void ironlake_pfit_enable(const struct intel_crtc_state *crtc_state);
@@ -10627,7 +10626,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 	u64 power_domain_mask;
 	bool active;
 
-	intel_crtc_init_scalers(crtc, pipe_config);
+	intel_crtc_init_scalers(pipe_config);
 
 	pipe_config->master_transcoder = INVALID_TRANSCODER;
 
@@ -15698,25 +15697,12 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv,
 	return ERR_PTR(ret);
 }
 
-static void intel_crtc_init_scalers(struct intel_crtc *crtc,
-				    struct intel_crtc_state *crtc_state)
+static void intel_crtc_init_scalers(struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc_scaler_state *scaler_state =
 		&crtc_state->scaler_state;
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	int i;
-
-	crtc->num_scalers = RUNTIME_INFO(dev_priv)->num_scalers[crtc->pipe];
-	if (!crtc->num_scalers)
-		return;
-
-	for (i = 0; i < crtc->num_scalers; i++) {
-		struct intel_scaler *scaler = &scaler_state->scalers[i];
-
-		scaler->in_use = 0;
-		scaler->mode = 0;
-	}
 
+	memset(scaler_state, 0, sizeof(*scaler_state));
 	scaler_state->scaler_id = -1;
 }
 
@@ -15787,27 +15773,49 @@ static const struct drm_crtc_funcs i8xx_crtc_funcs = {
 	.disable_vblank = i8xx_disable_vblank,
 };
 
-static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
+static struct intel_crtc *intel_crtc_alloc(void)
 {
-	const struct drm_crtc_funcs *funcs;
+	struct intel_crtc_state *crtc_state;
 	struct intel_crtc *crtc;
-	struct intel_crtc_state *crtc_state = NULL;
-	struct intel_plane *primary = NULL;
-	struct intel_plane *cursor = NULL;
-	int sprite, ret;
 
 	crtc = kzalloc(sizeof(*crtc), GFP_KERNEL);
 	if (!crtc)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
 	if (!crtc_state) {
-		ret = -ENOMEM;
-		goto fail;
+		kfree(crtc);
+		return ERR_PTR(-ENOMEM);
 	}
+
 	__drm_atomic_helper_crtc_reset(&crtc->base, &crtc_state->uapi);
+	intel_crtc_init_scalers(crtc_state);
+
 	crtc->config = crtc_state;
 
+	return crtc;
+}
+
+static void intel_crtc_free(struct intel_crtc *crtc)
+{
+	intel_crtc_destroy_state(&crtc->base, crtc->base.state);
+	kfree(crtc);
+}
+
+static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
+{
+	struct intel_plane *primary, *cursor;
+	const struct drm_crtc_funcs *funcs;
+	struct intel_crtc *crtc;
+	int sprite, ret;
+
+	crtc = intel_crtc_alloc();
+	if (IS_ERR(crtc))
+		return PTR_ERR(crtc);
+
+	crtc->pipe = pipe;
+	crtc->num_scalers = RUNTIME_INFO(dev_priv)->num_scalers[pipe];
+
 	primary = intel_primary_plane_create(dev_priv, pipe);
 	if (IS_ERR(primary)) {
 		ret = PTR_ERR(primary);
@@ -15858,11 +15866,6 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 	if (ret)
 		goto fail;
 
-	crtc->pipe = pipe;
-
-	/* initialize shared scalers */
-	intel_crtc_init_scalers(crtc, crtc_state);
-
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->pipe_to_crtc_mapping) ||
 	       dev_priv->pipe_to_crtc_mapping[pipe] != NULL);
 	dev_priv->pipe_to_crtc_mapping[pipe] = crtc;
@@ -15882,12 +15885,7 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 	return 0;
 
 fail:
-	/*
-	 * drm_mode_config_cleanup() will free up any
-	 * crtcs/planes already initialized.
-	 */
-	kfree(crtc_state);
-	kfree(crtc);
+	intel_crtc_free(crtc);
 
 	return ret;
 }
-- 
2.24.1

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

[Intel-gfx] [PATCH v2 rebased 04/11] drm/i915: Introduce intel_crtc_state_reset()

[José Roberto de Souza]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We have a few places where we want to reset a crtc state to its
default values. Let's add a helper for that. We'll need the new
__drm_atomic_helper_crtc_state_reset() helper for this to allow
us to just reset the state itself without clobbering the
crtc->state pointer.

And while at it let's zero out the whole thing, except a few
choice member which we'll mark as "invalid". And thanks to this
we can now nuke intel_crtc_init_scalers().

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 47 +++++++++-----------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index b6d364dd3e4c..b4e44d3cd275 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -164,7 +164,6 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_state *pipe_config);
 static void chv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_state *pipe_config);
-static void intel_crtc_init_scalers(struct intel_crtc_state *crtc_state);
 static void skylake_pfit_enable(const struct intel_crtc_state *crtc_state);
 static void ironlake_pfit_disable(const struct intel_crtc_state *old_crtc_state);
 static void ironlake_pfit_enable(const struct intel_crtc_state *crtc_state);
@@ -10626,8 +10625,6 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 	u64 power_domain_mask;
 	bool active;
 
-	intel_crtc_init_scalers(pipe_config);
-
 	pipe_config->master_transcoder = INVALID_TRANSCODER;
 
 	power_domain = POWER_DOMAIN_PIPE(crtc->pipe);
@@ -11675,6 +11672,20 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,
 					 &pipe_config->fdi_m_n);
 }
 
+static void intel_crtc_state_reset(struct intel_crtc_state *crtc_state,
+				   struct intel_crtc *crtc)
+{
+	memset(crtc_state, 0, sizeof(*crtc_state));
+
+	__drm_atomic_helper_crtc_state_reset(&crtc_state->uapi, &crtc->base);
+
+	crtc_state->cpu_transcoder = INVALID_TRANSCODER;
+	crtc_state->master_transcoder = INVALID_TRANSCODER;
+	crtc_state->hsw_workaround_pipe = INVALID_PIPE;
+	crtc_state->output_format = INTEL_OUTPUT_FORMAT_INVALID;
+	crtc_state->scaler_state.scaler_id = -1;
+}
+
 /* Returns the currently programmed mode of the given encoder. */
 struct drm_display_mode *
 intel_encoder_current_mode(struct intel_encoder *encoder)
@@ -11700,7 +11711,7 @@ intel_encoder_current_mode(struct intel_encoder *encoder)
 		return NULL;
 	}
 
-	crtc_state->uapi.crtc = &crtc->base;
+	intel_crtc_state_reset(crtc_state, crtc);
 
 	if (!dev_priv->display.get_pipe_config(crtc, crtc_state)) {
 		kfree(crtc_state);
@@ -13552,18 +13563,14 @@ verify_crtc_state(struct intel_crtc *crtc,
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_encoder *encoder;
-	struct intel_crtc_state *pipe_config;
-	struct drm_atomic_state *state;
+	struct intel_crtc_state *pipe_config = old_crtc_state;
+	struct drm_atomic_state *state = old_crtc_state->uapi.state;
 	bool active;
 
-	state = old_crtc_state->uapi.state;
 	__drm_atomic_helper_crtc_destroy_state(&old_crtc_state->uapi);
 	intel_crtc_free_hw_state(old_crtc_state);
-
-	pipe_config = old_crtc_state;
-	memset(pipe_config, 0, sizeof(*pipe_config));
-	pipe_config->uapi.crtc = &crtc->base;
-	pipe_config->uapi.state = state;
+	intel_crtc_state_reset(old_crtc_state, crtc);
+	old_crtc_state->uapi.state = state;
 
 	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.base.id, crtc->base.name);
 
@@ -15697,15 +15704,6 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv,
 	return ERR_PTR(ret);
 }
 
-static void intel_crtc_init_scalers(struct intel_crtc_state *crtc_state)
-{
-	struct intel_crtc_scaler_state *scaler_state =
-		&crtc_state->scaler_state;
-
-	memset(scaler_state, 0, sizeof(*scaler_state));
-	scaler_state->scaler_id = -1;
-}
-
 #define INTEL_CRTC_FUNCS \
 	.gamma_set = drm_atomic_helper_legacy_gamma_set, \
 	.set_config = drm_atomic_helper_set_config, \
@@ -15788,9 +15786,9 @@ static struct intel_crtc *intel_crtc_alloc(void)
 		return ERR_PTR(-ENOMEM);
 	}
 
-	__drm_atomic_helper_crtc_reset(&crtc->base, &crtc_state->uapi);
-	intel_crtc_init_scalers(crtc_state);
+	intel_crtc_state_reset(crtc_state, crtc);
 
+	crtc->base.state = &crtc_state->uapi;
 	crtc->config = crtc_state;
 
 	return crtc;
@@ -17411,8 +17409,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 
 		__drm_atomic_helper_crtc_destroy_state(&crtc_state->uapi);
 		intel_crtc_free_hw_state(crtc_state);
-		memset(crtc_state, 0, sizeof(*crtc_state));
-		__drm_atomic_helper_crtc_reset(&crtc->base, &crtc_state->uapi);
+		intel_crtc_state_reset(crtc_state, crtc);
 
 		crtc_state->hw.active = crtc_state->hw.enable =
 			dev_priv->display.get_pipe_config(crtc, crtc_state);
-- 
2.24.1

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

[Intel-gfx] [PATCH v2 rebased 05/11] drm/i915: Introduce intel_plane_state_reset()

[José Roberto de Souza]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

For the sake of symmetry with the crtc stuff let's add
a helper to reset the plane state to sane default values.
For the moment this only gets caller from the plane init.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_atomic_plane.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
index 42b3b3449d2e..9429b8e17270 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
@@ -41,6 +41,16 @@
 #include "intel_pm.h"
 #include "intel_sprite.h"
 
+static void intel_plane_state_reset(struct intel_plane_state *plane_state,
+				    struct intel_plane *plane)
+{
+	memset(plane_state, 0, sizeof(*plane_state));
+
+	__drm_atomic_helper_plane_state_reset(&plane_state->uapi, &plane->base);
+
+	plane_state->scaler_id = -1;
+}
+
 struct intel_plane *intel_plane_alloc(void)
 {
 	struct intel_plane_state *plane_state;
@@ -56,8 +66,9 @@ struct intel_plane *intel_plane_alloc(void)
 		return ERR_PTR(-ENOMEM);
 	}
 
-	__drm_atomic_helper_plane_reset(&plane->base, &plane_state->uapi);
-	plane_state->scaler_id = -1;
+	intel_plane_state_reset(plane_state, plane);
+
+	plane->base.state = &plane_state->uapi;
 
 	return plane;
 }
-- 
2.24.1

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

[Intel-gfx] [PATCH v2 rebased 06/11] drm/i915/display: Share intel_connector_needs_modeset()

[José Roberto de Souza]

intel_connector_needs_modeset() will be used outside of
intel_display.c in a future patch so it would only be necessary to
remove the state and add the prototype to the header file.

But while at it, I simplified the arguments and changed to intel
types and moved it to a better place intel_atomic.c.

That allowed us to convert the whole
intel_encoders_update_prepare/complete to intel type too.

No behavior changes intended here.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_atomic.c  | 32 ++++++++++++
 drivers/gpu/drm/i915/display/intel_atomic.h  |  3 ++
 drivers/gpu/drm/i915/display/intel_display.c | 53 ++++++--------------
 3 files changed, 51 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index fd0026fc3618..6e93a39a6fec 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -174,6 +174,38 @@ intel_digital_connector_duplicate_state(struct drm_connector *connector)
 	return &state->base;
 }
 
+/**
+ * intel_digital_connector_needs_modeset - check if connector needs a modeset
+ */
+bool
+intel_digital_connector_needs_modeset(struct intel_atomic_state *state,
+				      struct intel_connector *connector)
+{
+	struct intel_digital_connector_state *old_connector_state, *new_connector_state;
+	struct intel_crtc *old_crtc, *new_crtc;
+	struct intel_crtc_state *new_crtc_state;
+
+	old_connector_state = intel_atomic_get_old_connector_state(state,
+								   connector);
+	if (old_connector_state->base.crtc)
+		old_crtc = to_intel_crtc(old_connector_state->base.crtc);
+	else
+		old_crtc = NULL;
+
+	new_connector_state = intel_atomic_get_new_connector_state(state,
+								   connector);
+	if (new_connector_state->base.crtc) {
+		new_crtc = to_intel_crtc(new_connector_state->base.crtc);
+		new_crtc_state = intel_atomic_get_new_crtc_state(state, new_crtc);
+	} else {
+		new_crtc_state = NULL;
+		new_crtc = NULL;
+	}
+
+	return new_crtc != old_crtc ||
+	       (new_crtc && drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi));
+}
+
 /**
  * intel_crtc_duplicate_state - duplicate crtc state
  * @crtc: drm crtc
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
index 7b49623419ba..ba9cc29a5865 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.h
+++ b/drivers/gpu/drm/i915/display/intel_atomic.h
@@ -17,6 +17,7 @@ struct drm_device;
 struct drm_i915_private;
 struct drm_property;
 struct intel_atomic_state;
+struct intel_connector;
 struct intel_crtc;
 struct intel_crtc_state;
 
@@ -32,6 +33,8 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
 					 struct drm_atomic_state *state);
 struct drm_connector_state *
 intel_digital_connector_duplicate_state(struct drm_connector *connector);
+bool intel_digital_connector_needs_modeset(struct intel_atomic_state *state,
+					   struct intel_connector *connector);
 
 struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
 void intel_crtc_destroy_state(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index b4e44d3cd275..39b00a19d752 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6185,71 +6185,50 @@ intel_connector_primary_encoder(struct intel_connector *connector)
 	return encoder;
 }
 
-static bool
-intel_connector_needs_modeset(struct intel_atomic_state *state,
-			      const struct drm_connector_state *old_conn_state,
-			      const struct drm_connector_state *new_conn_state)
-{
-	struct intel_crtc *old_crtc = old_conn_state->crtc ?
-				      to_intel_crtc(old_conn_state->crtc) : NULL;
-	struct intel_crtc *new_crtc = new_conn_state->crtc ?
-				      to_intel_crtc(new_conn_state->crtc) : NULL;
-
-	return new_crtc != old_crtc ||
-	       (new_crtc &&
-		needs_modeset(intel_atomic_get_new_crtc_state(state, new_crtc)));
-}
-
 static void intel_encoders_update_prepare(struct intel_atomic_state *state)
 {
-	struct drm_connector_state *old_conn_state;
-	struct drm_connector_state *new_conn_state;
-	struct drm_connector *conn;
+	struct intel_digital_connector_state *new_connector_state;
+	struct intel_connector *connector;
 	int i;
 
-	for_each_oldnew_connector_in_state(&state->base, conn,
-					   old_conn_state, new_conn_state, i) {
+	for_each_new_intel_connector_in_state(state, connector,
+					      new_connector_state, i) {
 		struct intel_encoder *encoder;
 		struct intel_crtc *crtc;
 
-		if (!intel_connector_needs_modeset(state,
-						   old_conn_state,
-						   new_conn_state))
+		if (!intel_digital_connector_needs_modeset(state, connector))
 			continue;
 
-		encoder = intel_connector_primary_encoder(to_intel_connector(conn));
+		encoder = intel_connector_primary_encoder(connector);
 		if (!encoder->update_prepare)
 			continue;
 
-		crtc = new_conn_state->crtc ?
-			to_intel_crtc(new_conn_state->crtc) : NULL;
+		crtc = new_connector_state->base.crtc ?
+			to_intel_crtc(new_connector_state->base.crtc) : NULL;
 		encoder->update_prepare(state, encoder, crtc);
 	}
 }
 
 static void intel_encoders_update_complete(struct intel_atomic_state *state)
 {
-	struct drm_connector_state *old_conn_state;
-	struct drm_connector_state *new_conn_state;
-	struct drm_connector *conn;
+	struct intel_digital_connector_state *new_connector_state;
+	struct intel_connector *connector;
 	int i;
 
-	for_each_oldnew_connector_in_state(&state->base, conn,
-					   old_conn_state, new_conn_state, i) {
+	for_each_new_intel_connector_in_state(state, connector,
+					      new_connector_state, i) {
 		struct intel_encoder *encoder;
 		struct intel_crtc *crtc;
 
-		if (!intel_connector_needs_modeset(state,
-						   old_conn_state,
-						   new_conn_state))
+		if (!intel_digital_connector_needs_modeset(state, connector))
 			continue;
 
-		encoder = intel_connector_primary_encoder(to_intel_connector(conn));
+		encoder = intel_connector_primary_encoder(connector);
 		if (!encoder->update_complete)
 			continue;
 
-		crtc = new_conn_state->crtc ?
-			to_intel_crtc(new_conn_state->crtc) : NULL;
+		crtc = new_connector_state->base.crtc ?
+			to_intel_crtc(new_connector_state->base.crtc) : NULL;
 		encoder->update_complete(state, encoder, crtc);
 	}
 }
-- 
2.24.1

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

[Intel-gfx] [PATCH v2 rebased 07/11] drm/i915/tgl: Select master transcoder for MST stream

[José Roberto de Souza]

On TGL the blending of all the streams have moved from DDI to
transcoder, so now every transcoder working over the same MST port must
send its stream to a master transcoder and master will send to DDI
respecting the time slots.

So here adding all the CRTCs that shares the same MST stream if
needed and computing their state again, it will pick the lowest
pipe/transcoder among the ones in the same stream to be master.

Most of the time skl_commit_modeset_enables() enables pipes in a
crescent order but due DDB overlapping it might not happen, this
scenarios will be handled in the next patch.

v2:
- Using recently added intel_crtc_state_reset() to set
mst_master_transcoder to invalid transcoder for all non gen12 & MST
code paths
- Setting lowest pipe/transcoder as master, previously it was the
first one but setting a predictable one will help in future MST e
port sync integration
- Moving to intel type as much as we can

BSpec: 50493
BSpec: 49190
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_atomic.c   |  14 ++
 drivers/gpu/drm/i915/display/intel_atomic.h   |   3 +
 drivers/gpu/drm/i915/display/intel_ddi.c      |  14 +-
 drivers/gpu/drm/i915/display/intel_display.c  |  13 +-
 .../drm/i915/display/intel_display_types.h    |   3 +
 drivers/gpu/drm/i915/display/intel_dp_mst.c   | 139 ++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_dp_mst.h   |   5 +
 7 files changed, 178 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index 6e93a39a6fec..69a0430c4638 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -206,6 +206,20 @@ intel_digital_connector_needs_modeset(struct intel_atomic_state *state,
 	       (new_crtc && drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi));
 }
 
+struct intel_digital_connector_state *
+intel_atomic_get_digital_connector_state(struct intel_atomic_state *state,
+					 struct intel_connector *connector)
+{
+	struct drm_connector_state *connector_state;
+
+	connector_state = drm_atomic_get_connector_state(&state->base,
+							 &connector->base);
+	if (IS_ERR(connector_state))
+		return ERR_CAST(connector_state);
+
+	return to_intel_digital_connector_state(connector_state);
+}
+
 /**
  * intel_crtc_duplicate_state - duplicate crtc state
  * @crtc: drm crtc
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
index ba9cc29a5865..6e8638d83d28 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.h
+++ b/drivers/gpu/drm/i915/display/intel_atomic.h
@@ -35,6 +35,9 @@ struct drm_connector_state *
 intel_digital_connector_duplicate_state(struct drm_connector *connector);
 bool intel_digital_connector_needs_modeset(struct intel_atomic_state *state,
 					   struct intel_connector *connector);
+struct intel_digital_connector_state *
+intel_atomic_get_digital_connector_state(struct intel_atomic_state *state,
+					 struct intel_connector *connector);
 
 struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
 void intel_crtc_destroy_state(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 5b6f32517c75..6ee5230045eb 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -1903,8 +1903,13 @@ intel_ddi_transcoder_func_reg_val_get(const struct intel_crtc_state *crtc_state)
 		temp |= TRANS_DDI_MODE_SELECT_DP_MST;
 		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
 
-		if (INTEL_GEN(dev_priv) >= 12)
-			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(crtc_state->cpu_transcoder);
+		if (INTEL_GEN(dev_priv) >= 12) {
+			enum transcoder master;
+
+			master = crtc_state->mst_master_transcoder;
+			WARN_ON(master == INVALID_TRANSCODER);
+			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master);
+		}
 	} else {
 		temp |= TRANS_DDI_MODE_SELECT_DP_SST;
 		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
@@ -4377,6 +4382,11 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
 		pipe_config->output_types |= BIT(INTEL_OUTPUT_DP_MST);
 		pipe_config->lane_count =
 			((temp & DDI_PORT_WIDTH_MASK) >> DDI_PORT_WIDTH_SHIFT) + 1;
+
+		if (INTEL_GEN(dev_priv) >= 12)
+			pipe_config->mst_master_transcoder =
+					REG_FIELD_GET(TRANS_DDI_MST_TRANSPORT_SELECT_MASK, temp);
+
 		intel_dp_get_m_n(intel_crtc, pipe_config);
 		break;
 	default:
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 39b00a19d752..1cecce2f54f8 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -46,6 +46,7 @@
 #include "display/intel_crt.h"
 #include "display/intel_ddi.h"
 #include "display/intel_dp.h"
+#include "display/intel_dp_mst.h"
 #include "display/intel_dsi.h"
 #include "display/intel_dvo.h"
 #include "display/intel_gmbus.h"
@@ -11663,6 +11664,7 @@ static void intel_crtc_state_reset(struct intel_crtc_state *crtc_state,
 	crtc_state->hsw_workaround_pipe = INVALID_PIPE;
 	crtc_state->output_format = INTEL_OUTPUT_FORMAT_INVALID;
 	crtc_state->scaler_state.scaler_id = -1;
+	crtc_state->mst_master_transcoder = INVALID_TRANSCODER;
 }
 
 /* Returns the currently programmed mode of the given encoder. */
@@ -12510,6 +12512,9 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
 			      pipe_config->csc_mode, pipe_config->gamma_mode,
 			      pipe_config->gamma_enable, pipe_config->csc_enable);
 
+	DRM_DEBUG_KMS("MST master transcoder: %s\n",
+		      transcoder_name(pipe_config->mst_master_transcoder));
+
 dump_planes:
 	if (!state)
 		return;
@@ -12651,6 +12656,7 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
 	memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls,
 	       sizeof(saved_state->icl_port_dplls));
 	saved_state->crc_enabled = crtc_state->crc_enabled;
+	saved_state->mst_master_transcoder = INVALID_TRANSCODER;
 	if (IS_G4X(dev_priv) ||
 	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		saved_state->wm = crtc_state->wm;
@@ -13290,6 +13296,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 	PIPE_CONF_CHECK_I(dsc.dsc_split);
 	PIPE_CONF_CHECK_I(dsc.compressed_bpp);
 
+	PIPE_CONF_CHECK_I(mst_master_transcoder);
+
 #undef PIPE_CONF_CHECK_X
 #undef PIPE_CONF_CHECK_I
 #undef PIPE_CONF_CHECK_BOOL
@@ -14374,7 +14382,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
 	u32 handled = 0;
 	int i;
 
-	/* Only disable port sync slaves */
+	/* Only disable port sync and MST slaves */
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
 					    new_crtc_state, i) {
 		if (!needs_modeset(new_crtc_state))
@@ -14388,7 +14396,8 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
 		 * slave CRTCs are disabled first and then master CRTC since
 		 * Slave vblanks are masked till Master Vblanks.
 		 */
-		if (!is_trans_port_sync_slave(old_crtc_state))
+		if (!is_trans_port_sync_slave(old_crtc_state) &&
+		    !intel_dp_mst_is_slave_trans(old_crtc_state))
 			continue;
 
 		intel_pre_plane_update(state, crtc);
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 83ea04149b77..630a94892b7b 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1054,6 +1054,9 @@ struct intel_crtc_state {
 
 	/* Bitmask to indicate slaves attached */
 	u8 sync_mode_slaves_mask;
+
+	/* Only valid on TGL+ */
+	enum transcoder mst_master_transcoder;
 };
 
 struct intel_crtc {
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 926e49f449a6..12f5e799d91f 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -87,6 +87,49 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
 	return 0;
 }
 
+/*
+ * Iterate over all connectors and set mst_master_transcoder to the smallest
+ * transcoder id that shares the same MST connector.
+ */
+static int
+intel_dp_mst_master_trans_compute(struct intel_encoder *encoder,
+				  struct intel_crtc_state *crtc_state,
+				  struct drm_connector_state *connector_state)
+{
+	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
+	struct intel_connector *connector = to_intel_connector(connector_state->connector);
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_digital_connector_state *iter_connector_state;
+	struct intel_connector *iter_connector;
+	int i;
+
+	if (INTEL_GEN(dev_priv) < 12)
+		return 0;
+
+	crtc_state->mst_master_transcoder = I915_MAX_TRANSCODERS;
+
+	for_each_new_intel_connector_in_state(state, iter_connector,
+					      iter_connector_state, i) {
+		struct intel_crtc_state *iter_crtc_state;
+		struct intel_crtc *iter_crtc;
+
+		if (connector->mst_port != iter_connector->mst_port ||
+		    !iter_connector_state->base.crtc)
+			continue;
+
+		iter_crtc = to_intel_crtc(iter_connector_state->base.crtc);
+		iter_crtc_state = intel_atomic_get_new_crtc_state(state,
+								  iter_crtc);
+		if (!iter_crtc_state->uapi.active)
+			continue;
+
+		if (iter_crtc_state->cpu_transcoder < crtc_state->mst_master_transcoder)
+			crtc_state->mst_master_transcoder = iter_crtc_state->cpu_transcoder;
+	}
+
+	return 0;
+}
+
 static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
 				       struct intel_crtc_state *pipe_config,
 				       struct drm_connector_state *conn_state)
@@ -154,24 +197,88 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
 
 	intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
 
+	ret = intel_dp_mst_master_trans_compute(encoder, pipe_config,
+						conn_state);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * If one of the connectors in a MST stream needs a modeset, mark all CRTCs
+ * that have a connector in the same MST stream as mode changed,
+ * intel_modeset_pipe_config()+intel_crtc_check_fastset() will take to do a
+ * fastset when possible.
+ */
+static int
+intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector,
+				       struct intel_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	struct drm_connector_list_iter connector_list_iter;
+	struct intel_connector *connector_iter;
+
+	if (INTEL_GEN(dev_priv) < 12)
+		return  0;
+
+	if (!intel_digital_connector_needs_modeset(state, connector))
+		return 0;
+
+	drm_connector_list_iter_begin(&dev_priv->drm, &connector_list_iter);
+	for_each_intel_connector_iter(connector_iter, &connector_list_iter) {
+		struct intel_digital_connector_state *connector_iter_state;
+		struct intel_crtc_state *crtc_state;
+		struct intel_crtc *crtc;
+
+		if (connector_iter->mst_port != connector->mst_port)
+			continue;
+
+		connector_iter_state =
+			intel_atomic_get_digital_connector_state(state,
+								 connector_iter);
+		if (IS_ERR(connector_iter_state)) {
+			drm_connector_list_iter_end(&connector_list_iter);
+			return PTR_ERR(connector_iter_state);
+		}
+
+		if (!connector_iter_state->base.crtc)
+			continue;
+
+		crtc = to_intel_crtc(connector_iter_state->base.crtc);
+		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
+		if (IS_ERR(crtc_state)) {
+			drm_connector_list_iter_end(&connector_list_iter);
+			return PTR_ERR(crtc_state);
+		}
+
+		crtc_state->uapi.mode_changed = true;
+	}
+	drm_connector_list_iter_end(&connector_list_iter);
+
 	return 0;
 }
 
 static int
 intel_dp_mst_atomic_check(struct drm_connector *connector,
-			  struct drm_atomic_state *state)
+			  struct drm_atomic_state *_state)
 {
+	struct intel_atomic_state *state = to_intel_atomic_state(_state);
 	struct drm_connector_state *new_conn_state =
-		drm_atomic_get_new_connector_state(state, connector);
+		drm_atomic_get_new_connector_state(&state->base, connector);
 	struct drm_connector_state *old_conn_state =
-		drm_atomic_get_old_connector_state(state, connector);
+		drm_atomic_get_old_connector_state(&state->base, connector);
 	struct intel_connector *intel_connector =
 		to_intel_connector(connector);
 	struct drm_crtc *new_crtc = new_conn_state->crtc;
 	struct drm_dp_mst_topology_mgr *mgr;
 	int ret;
 
-	ret = intel_digital_connector_atomic_check(connector, state);
+	ret = intel_digital_connector_atomic_check(connector, &state->base);
+	if (ret)
+		return ret;
+
+	ret = intel_dp_mst_atomic_master_trans_check(intel_connector, state);
 	if (ret)
 		return ret;
 
@@ -182,12 +289,9 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
 	 * connector
 	 */
 	if (new_crtc) {
-		struct intel_atomic_state *intel_state =
-			to_intel_atomic_state(state);
 		struct intel_crtc *intel_crtc = to_intel_crtc(new_crtc);
 		struct intel_crtc_state *crtc_state =
-			intel_atomic_get_new_crtc_state(intel_state,
-							intel_crtc);
+			intel_atomic_get_new_crtc_state(state, intel_crtc);
 
 		if (!crtc_state ||
 		    !drm_atomic_crtc_needs_modeset(&crtc_state->uapi) ||
@@ -196,7 +300,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
 	}
 
 	mgr = &enc_to_mst(old_conn_state->best_encoder)->primary->dp.mst_mgr;
-	ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
+	ret = drm_dp_atomic_release_vcpi_slots(&state->base, mgr,
 					       intel_connector->port);
 
 	return ret;
@@ -241,6 +345,9 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
 	intel_dp->active_mst_links--;
 	last_mst_stream = intel_dp->active_mst_links == 0;
 
+	WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream &&
+		!intel_dp_mst_is_master_trans(old_crtc_state));
+
 	/*
 	 * From TGL spec: "If multi-stream slave transcoder: Configure
 	 * Transcoder Clock Select to direct no clock to the transcoder"
@@ -321,6 +428,9 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
 	intel_mst->connector = connector;
 	first_mst_stream = intel_dp->active_mst_links == 0;
 
+	WARN_ON(INTEL_GEN(dev_priv) >= 12 && first_mst_stream &&
+		!intel_dp_mst_is_master_trans(pipe_config));
+
 	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
 
 	if (first_mst_stream)
@@ -726,3 +836,14 @@ intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port)
 	drm_dp_mst_topology_mgr_destroy(&intel_dp->mst_mgr);
 	/* encoders will get killed by normal cleanup */
 }
+
+bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state)
+{
+	return crtc_state->mst_master_transcoder == crtc_state->cpu_transcoder;
+}
+
+bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
+{
+	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
+	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
+}
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
index f660ad80db04..e40767f78343 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
@@ -6,10 +6,15 @@
 #ifndef __INTEL_DP_MST_H__
 #define __INTEL_DP_MST_H__
 
+#include <stdbool.h>
+
 struct intel_digital_port;
+struct intel_crtc_state;
 
 int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
 void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
 int intel_dp_mst_encoder_active_links(struct intel_digital_port *intel_dig_port);
+bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
+bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
 
 #endif /* __INTEL_DP_MST_H__ */
-- 
2.24.1

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

[Intel-gfx] [PATCH v2 rebased 08/11] drm/i915/display: Always enables MST master pipe first

[José Roberto de Souza]

Due to DDB overlaps the pipe enabling sequence is not always crescent.
As the previous patch selects the smallest pipe/transcoder in the MST
stream to be master and it needs to be enabled first this changes
were needed to guarantee that.

So first lets enable all pipes that did not needed a fullmodeset so
it don't have any external dependency, this ones can overlap with
each other ddb allocations.

Then on the second loop it will enable all the pipes that needs a
modeset and don't depends on other pipes like MST master
pipe/transcoder.

Then finally all the pipes that needs a modeset and have dependency
on other pipes.

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: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 77 ++++++++++++++------
 1 file changed, 56 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 1cecce2f54f8..fa58b396e084 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14566,18 +14566,24 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 	/*
 	 * Whenever the number of active pipes changes, we need to make sure we
 	 * update the pipes in the right order so that their ddb allocations
-	 * never overlap with eachother inbetween CRTC updates. Otherwise we'll
+	 * never overlap with each other between CRTC updates. Otherwise we'll
 	 * cause pipe underruns and other bad stuff.
+	 *
+	 * So first lets enable all pipes that did not needed a fullmodeset so
+	 * it don't have any external dependency
 	 */
 	do {
 		progress = false;
 
-		for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
-			enum pipe pipe = crtc->pipe;
-			bool vbl_wait = false;
-			bool modeset = needs_modeset(new_crtc_state);
+		for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+						    new_crtc_state, i) {
+			bool vbl_wait;
+
+			if (updated & BIT(crtc->pipe) ||
+			    !new_crtc_state->hw.active)
+				continue;
 
-			if (updated & BIT(crtc->pipe) || !new_crtc_state->hw.active)
+			if (needs_modeset(new_crtc_state))
 				continue;
 
 			if (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
@@ -14585,7 +14591,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 							INTEL_NUM_PIPES(dev_priv), i))
 				continue;
 
-			updated |= BIT(pipe);
+			updated |= BIT(crtc->pipe);
 			entries[i] = new_crtc_state->wm.skl.ddb;
 
 			/*
@@ -14596,30 +14602,59 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 			 */
 			if (!skl_ddb_entry_equal(&new_crtc_state->wm.skl.ddb,
 						 &old_crtc_state->wm.skl.ddb) &&
-			    !modeset &&
 			    state->wm_results.dirty_pipes != updated)
 				vbl_wait = true;
 
-			if (modeset && is_trans_port_sync_mode(new_crtc_state)) {
-				if (is_trans_port_sync_master(new_crtc_state))
-					intel_update_trans_port_sync_crtcs(crtc,
-									   state,
-									   old_crtc_state,
-									   new_crtc_state);
-				else
-					continue;
-			} else {
-				intel_update_crtc(crtc, state, old_crtc_state,
-						  new_crtc_state);
-			}
+			intel_update_crtc(crtc, state, old_crtc_state,
+					  new_crtc_state);
 
 			if (vbl_wait)
-				intel_wait_for_vblank(dev_priv, pipe);
+				intel_wait_for_vblank(dev_priv, crtc->pipe);
 
 			progress = true;
 		}
 	} while (progress);
 
+	/*
+	 * Enabling all pipes that needs a modeset and do not depends on other
+	 * pipes
+	 */
+	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+					    new_crtc_state, i) {
+		if (updated & BIT(crtc->pipe) || !new_crtc_state->hw.active)
+			continue;
+
+		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
+		    is_trans_port_sync_slave(new_crtc_state))
+			continue;
+
+		updated |= BIT(crtc->pipe);
+
+		if (is_trans_port_sync_mode(new_crtc_state))
+			intel_update_trans_port_sync_crtcs(crtc, state,
+							   old_crtc_state,
+							   new_crtc_state);
+		else
+			intel_update_crtc(crtc, state, old_crtc_state,
+					  new_crtc_state);
+	}
+
+	/*
+	 * Finally enable all pipes that needs a modeset and depends on
+	 * other pipes, right now it is only MST slaves as both port sync slave
+	 * and master are enabled together
+	 */
+	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+					    new_crtc_state, i) {
+		if (updated & BIT(crtc->pipe) || !new_crtc_state->hw.active)
+			continue;
+
+		if (is_trans_port_sync_slave(new_crtc_state))
+			continue;
+
+		intel_update_crtc(crtc, state, old_crtc_state, new_crtc_state);
+	}
+
 	/* 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.24.1

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

[Intel-gfx] [PATCH v2 rebased 09/11] drm/i915/dp: Fix MST disable sequences

[José Roberto de Souza]

The disable sequence after wait for transcoder off was not correctly
implemented.
The MST disable sequence is basically the same for HSW, SKL, ICL and
TGL, with just minor changes for TGL.

So here calling a new MST function to do the MST sequences, most of
the steps just moved from the post disable hook.

With this last patch we finally fixed the hotplugs triggered by MST
sinks during the disable/enable sequence, those were causing source
to try to do a link training while it was not ready causing CPU pipe
FIFO underrrus on TGL.

v2: Only unsetting TGL_TRANS_DDI_PORT_MASK for TGL on the post
disable sequence

BSpec: 4231
BSpec: 4163
BSpec: 22243
BSpec: 49190
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c     | 33 ++++++---
 drivers/gpu/drm/i915/display/intel_display.c |  2 +
 drivers/gpu/drm/i915/display/intel_dp_mst.c  | 70 ++++++++++++++++----
 drivers/gpu/drm/i915/display/intel_dp_mst.h  |  1 +
 4 files changed, 84 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 6ee5230045eb..7c844dc1b24b 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -34,6 +34,7 @@
 #include "intel_ddi.h"
 #include "intel_display_types.h"
 #include "intel_dp.h"
+#include "intel_dp_mst.h"
 #include "intel_dp_link_training.h"
 #include "intel_dpio_phy.h"
 #include "intel_dsi.h"
@@ -1953,17 +1954,19 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
-	i915_reg_t reg = TRANS_DDI_FUNC_CTL(cpu_transcoder);
-	u32 val = I915_READ(reg);
+	u32 val;
+
+	val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
+	val &= ~TRANS_DDI_FUNC_ENABLE;
 
 	if (INTEL_GEN(dev_priv) >= 12) {
-		val &= ~(TRANS_DDI_FUNC_ENABLE | TGL_TRANS_DDI_PORT_MASK |
-			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
+		if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) ||
+		    intel_dp_mst_is_slave_trans(crtc_state))
+			val &= ~TGL_TRANS_DDI_PORT_MASK;
 	} else {
-		val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_PORT_MASK |
-			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
+		val &= ~TRANS_DDI_PORT_MASK;
 	}
-	I915_WRITE(reg, val);
+	I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val);
 
 	if (dev_priv->quirks & QUIRK_INCREASE_DDI_DISABLED_TIME &&
 	    intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
@@ -3812,8 +3815,20 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder,
 	 */
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
 
-	if (INTEL_GEN(dev_priv) < 12 && !is_mst)
-		intel_ddi_disable_pipe_clock(old_crtc_state);
+	if (INTEL_GEN(dev_priv) >= 12) {
+		if (is_mst) {
+			enum transcoder cpu_transcoder;
+			u32 val;
+
+			cpu_transcoder = old_crtc_state->cpu_transcoder;
+			val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
+			val &= ~TGL_TRANS_DDI_PORT_MASK;
+			I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val);
+		}
+	} else {
+		if (!is_mst)
+			intel_ddi_disable_pipe_clock(old_crtc_state);
+	}
 
 	intel_disable_ddi_buf(encoder, old_crtc_state);
 
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index fa58b396e084..092412b10d7c 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6731,6 +6731,8 @@ static void haswell_crtc_disable(struct intel_atomic_state *state,
 	if (!transcoder_is_dsi(cpu_transcoder))
 		intel_disable_pipe(old_crtc_state);
 
+	intel_dp_mst_post_trans_disabled(old_crtc_state);
+
 	if (INTEL_GEN(dev_priv) >= 11)
 		icl_disable_transcoder_port_sync(old_crtc_state);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 12f5e799d91f..6d32d4feb351 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -330,6 +330,57 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder,
 					  old_crtc_state, old_conn_state);
 }
 
+static void
+dp_mst_post_trans_disabled(struct intel_encoder *encoder,
+			   const struct intel_crtc_state *old_crtc_state,
+			   const struct drm_connector_state *old_conn_state)
+{
+	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
+	struct intel_digital_port *intel_dig_port = intel_mst->primary;
+	struct intel_dp *intel_dp = &intel_dig_port->dp;
+	struct intel_connector *connector =
+		to_intel_connector(old_conn_state->connector);
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	u32 val;
+
+	drm_dp_update_payload_part2(&intel_dp->mst_mgr);
+
+	val = I915_READ(TRANS_DDI_FUNC_CTL(old_crtc_state->cpu_transcoder));
+	val &= ~TRANS_DDI_DP_VC_PAYLOAD_ALLOC;
+	I915_WRITE(TRANS_DDI_FUNC_CTL(old_crtc_state->cpu_transcoder), val);
+
+	if (intel_de_wait_for_set(dev_priv, intel_dp->regs.dp_tp_status,
+				  DP_TP_STATUS_ACT_SENT, 1))
+		DRM_ERROR("Timed out waiting for ACT sent when disabling\n");
+
+	drm_dp_check_act_status(&intel_dp->mst_mgr);
+
+	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector->port);
+}
+
+void
+intel_dp_mst_post_trans_disabled(const struct intel_crtc_state *old_crtc_state)
+{
+	struct drm_atomic_state *state = old_crtc_state->uapi.state;
+	struct drm_connector_state *old_conn_state;
+	struct drm_connector *conn;
+	int i;
+
+	if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST))
+		return;
+
+	for_each_old_connector_in_state(state, conn, old_conn_state, i) {
+		struct intel_encoder *encoder;
+
+		if (old_conn_state->crtc != old_crtc_state->uapi.crtc)
+			continue;
+
+		encoder = to_intel_encoder(old_conn_state->best_encoder);
+		dp_mst_post_trans_disabled(encoder, old_crtc_state,
+					   old_conn_state);
+	}
+}
+
 static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
 				      const struct intel_crtc_state *old_crtc_state,
 				      const struct drm_connector_state *old_conn_state)
@@ -348,6 +399,12 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
 	WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream &&
 		!intel_dp_mst_is_master_trans(old_crtc_state));
 
+	/*
+	 * Power down mst path before disabling the port, otherwise we end
+	 * up getting interrupts from the sink upon detecting link loss.
+	 */
+	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port,
+				     false);
 	/*
 	 * From TGL spec: "If multi-stream slave transcoder: Configure
 	 * Transcoder Clock Select to direct no clock to the transcoder"
@@ -358,19 +415,6 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
 	if (INTEL_GEN(dev_priv) < 12 || !last_mst_stream)
 		intel_ddi_disable_pipe_clock(old_crtc_state);
 
-	/* this can fail */
-	drm_dp_check_act_status(&intel_dp->mst_mgr);
-	/* and this can also fail */
-	drm_dp_update_payload_part2(&intel_dp->mst_mgr);
-
-	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector->port);
-
-	/*
-	 * Power down mst path before disabling the port, otherwise we end
-	 * up getting interrupts from the sink upon detecting link loss.
-	 */
-	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port,
-				     false);
 
 	intel_mst->connector = NULL;
 	if (last_mst_stream)
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
index e40767f78343..87f32fab90fc 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
@@ -16,5 +16,6 @@ void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
 int intel_dp_mst_encoder_active_links(struct intel_digital_port *intel_dig_port);
 bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
 bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
+void intel_dp_mst_post_trans_disabled(const struct intel_crtc_state *old_crtc_state);
 
 #endif /* __INTEL_DP_MST_H__ */
-- 
2.24.1

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

[Intel-gfx] [PATCH v2 rebased 10/11] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[José Roberto de Souza]

Check if fastset is allowed by external dependencies like other pipes
and transcoders.

Right now it only forces a fullmodeset when the MST master transcoder
did not changed but the pipe of the master transcoder needs a
fullmodeset so all slaves also needs to do a fullmodeset.
But it will probably be need for port sync as well.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 41 ++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 092412b10d7c..0c24d7dfa152 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13930,11 +13930,52 @@ static int calc_watermark_data(struct intel_atomic_state *state)
 	return 0;
 }
 
+/**
+ * Check if fastset is allowed by external dependencies like other pipes and
+ * transcoders.
+ *
+ * Right now it only forces a fullmodeset when the MST master transcoder did
+ * not changed but the pipe of the master transcoder needs a fullmodeset so
+ * all slaves also needs to do a fullmodeset.
+ */
+static bool
+intel_crtc_check_external_dependencies_fastset(const struct intel_crtc_state *old_crtc_state,
+					       struct intel_crtc_state *new_crtc_state)
+{
+	struct intel_atomic_state *state = to_intel_atomic_state(new_crtc_state->uapi.state);
+	struct drm_i915_private *dev_priv = to_i915(new_crtc_state->uapi.crtc->dev);
+	struct intel_crtc_state *new_crtc_state_iter;
+	struct intel_crtc *crtc_iter;
+	int i;
+
+	if (INTEL_GEN(dev_priv) < 12)
+		return true;
+
+	if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST) ||
+	    intel_dp_mst_is_master_trans(new_crtc_state))
+		return true;
+
+	for_each_new_intel_crtc_in_state(state, crtc_iter, new_crtc_state_iter, i) {
+		if (new_crtc_state_iter->cpu_transcoder !=
+		    new_crtc_state->mst_master_transcoder)
+			continue;
+
+		return !needs_modeset(new_crtc_state_iter);
+	}
+
+	DRM_ERROR("Master MST transcoder of pipe not found\n");
+	return false;
+}
+
 static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_state,
 				     struct intel_crtc_state *new_crtc_state)
 {
 	if (!intel_pipe_config_compare(old_crtc_state, new_crtc_state, true))
 		return;
+	if (!intel_crtc_check_external_dependencies_fastset(old_crtc_state,
+							    new_crtc_state))
+		return;
+
 
 	new_crtc_state->uapi.mode_changed = false;
 	new_crtc_state->update_pipe = true;
-- 
2.24.1

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

[Intel-gfx] [PATCH v2 rebased 11/11] drm/i915/display: Add comment to a function that probably can be removed

[José Roberto de Souza]

This function is only called from port sync and it is identical to
what will be executed again in intel_update_crtc() over port sync
pipes.
If it is really necessary it at least deserves a better name and a
comment, leaving it to people working on port sync.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 0c24d7dfa152..cee80f50ce37 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14505,6 +14505,10 @@ static void intel_set_dp_tp_ctl_normal(struct intel_crtc *crtc,
 	intel_dp_stop_link_train(intel_dp);
 }
 
+/*
+ * TODO: This is only called from port sync and it is identical to what will be
+ * executed again in intel_update_crtc() over port sync pipes
+ */
 static void intel_post_crtc_enable_updates(struct intel_crtc *crtc,
 					   struct intel_atomic_state *state)
 {
-- 
2.24.1

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

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v2,rebased,01/11] drm: Add __drm_atomic_helper_crtc_state_reset() & co.

[Patchwork]

== Series Details ==

Series: series starting with [v2,rebased,01/11] drm: Add __drm_atomic_helper_crtc_state_reset() & co.
URL   : https://patchwork.freedesktop.org/series/70775/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7545 -> Patchwork_15698
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15698/index.html

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_suspend@basic-s0:
    - fi-skl-lmem:        [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-skl-lmem/igt@gem_exec_suspend@basic-s0.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15698/fi-skl-lmem/igt@gem_exec_suspend@basic-s0.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-lmem:        [PASS][3] -> [DMESG-WARN][4] ([i915#592])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-skl-lmem/igt@i915_pm_rpm@module-reload.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15698/fi-skl-lmem/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live_gem_contexts:
    - fi-hsw-peppy:       [PASS][5] -> [DMESG-FAIL][6] ([i915#722])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-hsw-peppy/igt@i915_selftest@live_gem_contexts.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15698/fi-hsw-peppy/igt@i915_selftest@live_gem_contexts.html

  * igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size:
    - fi-skl-6700k2:      [PASS][7] -> [FAIL][8] ([i915#697]) +7 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-skl-6700k2/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15698/fi-skl-6700k2/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html

  
#### Possible fixes ####

  * igt@gem_exec_parallel@basic:
    - {fi-tgl-guc}:       [INCOMPLETE][9] ([i915#476]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-tgl-guc/igt@gem_exec_parallel@basic.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15698/fi-tgl-guc/igt@gem_exec_parallel@basic.html

  * igt@i915_selftest@live_blt:
    - fi-ivb-3770:        [DMESG-FAIL][11] ([i915#725]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-ivb-3770/igt@i915_selftest@live_blt.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15698/fi-ivb-3770/igt@i915_selftest@live_blt.html

  
#### Warnings ####

  * igt@i915_module_load@reload:
    - fi-icl-u2:          [DMESG-WARN][13] ([i915#109] / [i915#289]) -> [DMESG-WARN][14] ([i915#289])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-icl-u2/igt@i915_module_load@reload.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15698/fi-icl-u2/igt@i915_module_load@reload.html

  * igt@i915_selftest@live_blt:
    - fi-hsw-4770:        [DMESG-FAIL][15] ([i915#725]) -> [DMESG-FAIL][16] ([i915#553] / [i915#725])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-hsw-4770/igt@i915_selftest@live_blt.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15698/fi-hsw-4770/igt@i915_selftest@live_blt.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][17] ([fdo#111096] / [i915#323]) -> [FAIL][18] ([fdo#111407])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15698/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_flip@basic-flip-vs-modeset:
    - fi-kbl-x1275:       [DMESG-WARN][19] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][20] ([i915#62] / [i915#92]) +4 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-modeset.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15698/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-modeset.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-kbl-x1275:       [DMESG-WARN][21] ([i915#62] / [i915#92]) -> [DMESG-WARN][22] ([i915#62] / [i915#92] / [i915#95]) +6 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-kbl-x1275/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15698/fi-kbl-x1275/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

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

  [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
  [fdo#111407]: https://bugs.freedesktop.org/show_bug.cgi?id=111407
  [fdo#111593]: https://bugs.freedesktop.org/show_bug.cgi?id=111593
  [i915#109]: https://gitlab.freedesktop.org/drm/intel/issues/109
  [i915#289]: https://gitlab.freedesktop.org/drm/intel/issues/289
  [i915#323]: https://gitlab.freedesktop.org/drm/intel/issues/323
  [i915#476]: https://gitlab.freedesktop.org/drm/intel/issues/476
  [i915#553]: https://gitlab.freedesktop.org/drm/intel/issues/553
  [i915#592]: https://gitlab.freedesktop.org/drm/intel/issues/592
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#697]: https://gitlab.freedesktop.org/drm/intel/issues/697
  [i915#722]: https://gitlab.freedesktop.org/drm/intel/issues/722
  [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725
  [i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (52 -> 45)
------------------------------

  Missing    (7): fi-hsw-4770r fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7545 -> Patchwork_15698

  CI-20190529: 20190529
  CI_DRM_7545: b1b808dff985c3c2050b20771050453589a60ca3 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5346: 466b0e6cbcbaccff012b484d1fd7676364b37b93 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_15698: 9ce3848ce1da398b379b9f0d5bf510f165477546 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

9ce3848ce1da drm/i915/display: Add comment to a function that probably can be removed
9a502ffecff0 drm/i915/display: Check if pipe fastset is allowed by external dependencies
4fd1645b537e drm/i915/dp: Fix MST disable sequences
4660a66d3618 drm/i915/display: Always enables MST master pipe first
7d2b979eb74b drm/i915/tgl: Select master transcoder for MST stream
fd3979e2bdfb drm/i915/display: Share intel_connector_needs_modeset()
956214292dd1 drm/i915: Introduce intel_plane_state_reset()
0222e03a8c5d drm/i915: Introduce intel_crtc_state_reset()
f0a7c707f4d4 drm/i915: Introduce intel_crtc_{alloc, free}()
b4dc3af1b862 drm/i915: s/intel_crtc/crtc/ in intel_crtc_init()
d3b79f9dd591 drm: Add __drm_atomic_helper_crtc_state_reset() & co.

== Logs ==

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

Re: [Intel-gfx] [PATCH v2 rebased 06/11] drm/i915/display: Share intel_connector_needs_modeset()

[Ville Syrjälä]

On Wed, Dec 11, 2019 at 10:45:21AM -0800, José Roberto de Souza wrote:
> intel_connector_needs_modeset() will be used outside of
> intel_display.c in a future patch so it would only be necessary to
> remove the state and add the prototype to the header file.
> 
> But while at it, I simplified the arguments and changed to intel
> types and moved it to a better place intel_atomic.c.
> 
> That allowed us to convert the whole
> intel_encoders_update_prepare/complete to intel type too.
> 
> No behavior changes intended here.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic.c  | 32 ++++++++++++
>  drivers/gpu/drm/i915/display/intel_atomic.h  |  3 ++
>  drivers/gpu/drm/i915/display/intel_display.c | 53 ++++++--------------
>  3 files changed, 51 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index fd0026fc3618..6e93a39a6fec 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -174,6 +174,38 @@ intel_digital_connector_duplicate_state(struct drm_connector *connector)
>  	return &state->base;
>  }
>  
> +/**
> + * intel_digital_connector_needs_modeset - check if connector needs a modeset
> + */
> +bool
> +intel_digital_connector_needs_modeset(struct intel_atomic_state *state,

Why "digital"? Oh because intel_atomic_get_old_connector_state() return
a ditgital_connector_state. A bit surprising.

I suggest using just drm_connector_state here to keep this function
totally generic.

> +				      struct intel_connector *connector)
> +{
> +	struct intel_digital_connector_state *old_connector_state, *new_connector_state;
> +	struct intel_crtc *old_crtc, *new_crtc;
> +	struct intel_crtc_state *new_crtc_state;
> +
> +	old_connector_state = intel_atomic_get_old_connector_state(state,
> +								   connector);

Could be done when declaring the variable. Dunno which is prettier
though.

> +	if (old_connector_state->base.crtc)
> +		old_crtc = to_intel_crtc(old_connector_state->base.crtc);
> +	else
> +		old_crtc = NULL;

Simple
old_crtc = to_intel_crtc(old_connector_state->base.crtc); 
will do. Can be done when declaring the variable as well.

> +
> +	new_connector_state = intel_atomic_get_new_connector_state(state,
> +								   connector);
> +	if (new_connector_state->base.crtc) {
> +		new_crtc = to_intel_crtc(new_connector_state->base.crtc);

ditto.

> +		new_crtc_state = intel_atomic_get_new_crtc_state(state, new_crtc);

Then this just becomes
if (new_crtc)
	new_crtc_state = ...;

Or maybe
new_crtc_state = new_crtc ? get : NULL;
but that could be a bit ugly.


> +	} else {
> +		new_crtc_state = NULL;
> +		new_crtc = NULL;
> +	}
> +
> +	return new_crtc != old_crtc ||
> +	       (new_crtc && drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi));

Hmm. In fact this function could be one of those special cases where we
might even want to use all drm_ types internally since we don't actually
need anything else.

> +}
> +
>  /**
>   * intel_crtc_duplicate_state - duplicate crtc state
>   * @crtc: drm crtc
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
> index 7b49623419ba..ba9cc29a5865 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> @@ -17,6 +17,7 @@ struct drm_device;
>  struct drm_i915_private;
>  struct drm_property;
>  struct intel_atomic_state;
> +struct intel_connector;
>  struct intel_crtc;
>  struct intel_crtc_state;
>  
> @@ -32,6 +33,8 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
>  					 struct drm_atomic_state *state);
>  struct drm_connector_state *
>  intel_digital_connector_duplicate_state(struct drm_connector *connector);
> +bool intel_digital_connector_needs_modeset(struct intel_atomic_state *state,
> +					   struct intel_connector *connector);
>  
>  struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
>  void intel_crtc_destroy_state(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index b4e44d3cd275..39b00a19d752 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6185,71 +6185,50 @@ intel_connector_primary_encoder(struct intel_connector *connector)
>  	return encoder;
>  }
>  
> -static bool
> -intel_connector_needs_modeset(struct intel_atomic_state *state,
> -			      const struct drm_connector_state *old_conn_state,
> -			      const struct drm_connector_state *new_conn_state)
> -{
> -	struct intel_crtc *old_crtc = old_conn_state->crtc ?
> -				      to_intel_crtc(old_conn_state->crtc) : NULL;
> -	struct intel_crtc *new_crtc = new_conn_state->crtc ?
> -				      to_intel_crtc(new_conn_state->crtc) : NULL;
> -
> -	return new_crtc != old_crtc ||
> -	       (new_crtc &&
> -		needs_modeset(intel_atomic_get_new_crtc_state(state, new_crtc)));
> -}
> -
>  static void intel_encoders_update_prepare(struct intel_atomic_state *state)
>  {
> -	struct drm_connector_state *old_conn_state;
> -	struct drm_connector_state *new_conn_state;
> -	struct drm_connector *conn;
> +	struct intel_digital_connector_state *new_connector_state;
> +	struct intel_connector *connector;
>  	int i;
>  
> -	for_each_oldnew_connector_in_state(&state->base, conn,
> -					   old_conn_state, new_conn_state, i) {
> +	for_each_new_intel_connector_in_state(state, connector,
> +					      new_connector_state, i) {
>  		struct intel_encoder *encoder;
>  		struct intel_crtc *crtc;
>  
> -		if (!intel_connector_needs_modeset(state,
> -						   old_conn_state,
> -						   new_conn_state))
> +		if (!intel_digital_connector_needs_modeset(state, connector))
>  			continue;
>  
> -		encoder = intel_connector_primary_encoder(to_intel_connector(conn));
> +		encoder = intel_connector_primary_encoder(connector);
>  		if (!encoder->update_prepare)
>  			continue;
>  
> -		crtc = new_conn_state->crtc ?
> -			to_intel_crtc(new_conn_state->crtc) : NULL;
> +		crtc = new_connector_state->base.crtc ?
> +			to_intel_crtc(new_connector_state->base.crtc) : NULL;
>  		encoder->update_prepare(state, encoder, crtc);
>  	}
>  }
>  
>  static void intel_encoders_update_complete(struct intel_atomic_state *state)
>  {
> -	struct drm_connector_state *old_conn_state;
> -	struct drm_connector_state *new_conn_state;
> -	struct drm_connector *conn;
> +	struct intel_digital_connector_state *new_connector_state;
> +	struct intel_connector *connector;
>  	int i;
>  
> -	for_each_oldnew_connector_in_state(&state->base, conn,
> -					   old_conn_state, new_conn_state, i) {
> +	for_each_new_intel_connector_in_state(state, connector,
> +					      new_connector_state, i) {
>  		struct intel_encoder *encoder;
>  		struct intel_crtc *crtc;
>  
> -		if (!intel_connector_needs_modeset(state,
> -						   old_conn_state,
> -						   new_conn_state))
> +		if (!intel_digital_connector_needs_modeset(state, connector))
>  			continue;
>  
> -		encoder = intel_connector_primary_encoder(to_intel_connector(conn));
> +		encoder = intel_connector_primary_encoder(connector);
>  		if (!encoder->update_complete)
>  			continue;
>  
> -		crtc = new_conn_state->crtc ?
> -			to_intel_crtc(new_conn_state->crtc) : NULL;
> +		crtc = new_connector_state->base.crtc ?
> +			to_intel_crtc(new_connector_state->base.crtc) : NULL;
>  		encoder->update_complete(state, encoder, crtc);
>  	}
>  }
> -- 
> 2.24.1

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

Re: [Intel-gfx] [PATCH v2 rebased 07/11] drm/i915/tgl: Select master transcoder for MST stream

[Ville Syrjälä]

On Wed, Dec 11, 2019 at 10:45:22AM -0800, José Roberto de Souza wrote:
> On TGL the blending of all the streams have moved from DDI to
> transcoder, so now every transcoder working over the same MST port must
> send its stream to a master transcoder and master will send to DDI
> respecting the time slots.
> 
> So here adding all the CRTCs that shares the same MST stream if
> needed and computing their state again, it will pick the lowest
> pipe/transcoder among the ones in the same stream to be master.
> 
> Most of the time skl_commit_modeset_enables() enables pipes in a
> crescent order but due DDB overlapping it might not happen, this
> scenarios will be handled in the next patch.
> 
> v2:
> - Using recently added intel_crtc_state_reset() to set
> mst_master_transcoder to invalid transcoder for all non gen12 & MST
> code paths
> - Setting lowest pipe/transcoder as master, previously it was the
> first one but setting a predictable one will help in future MST e
> port sync integration
> - Moving to intel type as much as we can
> 
> BSpec: 50493
> BSpec: 49190
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic.c   |  14 ++
>  drivers/gpu/drm/i915/display/intel_atomic.h   |   3 +
>  drivers/gpu/drm/i915/display/intel_ddi.c      |  14 +-
>  drivers/gpu/drm/i915/display/intel_display.c  |  13 +-
>  .../drm/i915/display/intel_display_types.h    |   3 +
>  drivers/gpu/drm/i915/display/intel_dp_mst.c   | 139 ++++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_dp_mst.h   |   5 +
>  7 files changed, 178 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index 6e93a39a6fec..69a0430c4638 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -206,6 +206,20 @@ intel_digital_connector_needs_modeset(struct intel_atomic_state *state,
>  	       (new_crtc && drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi));
>  }
>  
> +struct intel_digital_connector_state *
> +intel_atomic_get_digital_connector_state(struct intel_atomic_state *state,
> +					 struct intel_connector *connector)
> +{
> +	struct drm_connector_state *connector_state;
> +
> +	connector_state = drm_atomic_get_connector_state(&state->base,
> +							 &connector->base);
> +	if (IS_ERR(connector_state))
> +		return ERR_CAST(connector_state);
> +
> +	return to_intel_digital_connector_state(connector_state);
> +}
> +
>  /**
>   * intel_crtc_duplicate_state - duplicate crtc state
>   * @crtc: drm crtc
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
> index ba9cc29a5865..6e8638d83d28 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> @@ -35,6 +35,9 @@ struct drm_connector_state *
>  intel_digital_connector_duplicate_state(struct drm_connector *connector);
>  bool intel_digital_connector_needs_modeset(struct intel_atomic_state *state,
>  					   struct intel_connector *connector);
> +struct intel_digital_connector_state *
> +intel_atomic_get_digital_connector_state(struct intel_atomic_state *state,
> +					 struct intel_connector *connector);
>  
>  struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
>  void intel_crtc_destroy_state(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 5b6f32517c75..6ee5230045eb 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1903,8 +1903,13 @@ intel_ddi_transcoder_func_reg_val_get(const struct intel_crtc_state *crtc_state)
>  		temp |= TRANS_DDI_MODE_SELECT_DP_MST;
>  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
>  
> -		if (INTEL_GEN(dev_priv) >= 12)
> -			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(crtc_state->cpu_transcoder);
> +		if (INTEL_GEN(dev_priv) >= 12) {
> +			enum transcoder master;
> +
> +			master = crtc_state->mst_master_transcoder;
> +			WARN_ON(master == INVALID_TRANSCODER);
> +			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master);
> +		}
>  	} else {
>  		temp |= TRANS_DDI_MODE_SELECT_DP_SST;
>  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> @@ -4377,6 +4382,11 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>  		pipe_config->output_types |= BIT(INTEL_OUTPUT_DP_MST);
>  		pipe_config->lane_count =
>  			((temp & DDI_PORT_WIDTH_MASK) >> DDI_PORT_WIDTH_SHIFT) + 1;
> +
> +		if (INTEL_GEN(dev_priv) >= 12)
> +			pipe_config->mst_master_transcoder =
> +					REG_FIELD_GET(TRANS_DDI_MST_TRANSPORT_SELECT_MASK, temp);
> +
>  		intel_dp_get_m_n(intel_crtc, pipe_config);
>  		break;
>  	default:
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 39b00a19d752..1cecce2f54f8 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -46,6 +46,7 @@
>  #include "display/intel_crt.h"
>  #include "display/intel_ddi.h"
>  #include "display/intel_dp.h"
> +#include "display/intel_dp_mst.h"
>  #include "display/intel_dsi.h"
>  #include "display/intel_dvo.h"
>  #include "display/intel_gmbus.h"
> @@ -11663,6 +11664,7 @@ static void intel_crtc_state_reset(struct intel_crtc_state *crtc_state,
>  	crtc_state->hsw_workaround_pipe = INVALID_PIPE;
>  	crtc_state->output_format = INTEL_OUTPUT_FORMAT_INVALID;
>  	crtc_state->scaler_state.scaler_id = -1;
> +	crtc_state->mst_master_transcoder = INVALID_TRANSCODER;
>  }
>  
>  /* Returns the currently programmed mode of the given encoder. */
> @@ -12510,6 +12512,9 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
>  			      pipe_config->csc_mode, pipe_config->gamma_mode,
>  			      pipe_config->gamma_enable, pipe_config->csc_enable);
>  
> +	DRM_DEBUG_KMS("MST master transcoder: %s\n",
> +		      transcoder_name(pipe_config->mst_master_transcoder));
> +
>  dump_planes:
>  	if (!state)
>  		return;
> @@ -12651,6 +12656,7 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
>  	memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls,
>  	       sizeof(saved_state->icl_port_dplls));
>  	saved_state->crc_enabled = crtc_state->crc_enabled;
> +	saved_state->mst_master_transcoder = INVALID_TRANSCODER;
>  	if (IS_G4X(dev_priv) ||
>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		saved_state->wm = crtc_state->wm;
> @@ -13290,6 +13296,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  	PIPE_CONF_CHECK_I(dsc.dsc_split);
>  	PIPE_CONF_CHECK_I(dsc.compressed_bpp);
>  
> +	PIPE_CONF_CHECK_I(mst_master_transcoder);
> +
>  #undef PIPE_CONF_CHECK_X
>  #undef PIPE_CONF_CHECK_I
>  #undef PIPE_CONF_CHECK_BOOL
> @@ -14374,7 +14382,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
>  	u32 handled = 0;
>  	int i;
>  
> -	/* Only disable port sync slaves */
> +	/* Only disable port sync and MST slaves */
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
>  		if (!needs_modeset(new_crtc_state))
> @@ -14388,7 +14396,8 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
>  		 * slave CRTCs are disabled first and then master CRTC since
>  		 * Slave vblanks are masked till Master Vblanks.
>  		 */
> -		if (!is_trans_port_sync_slave(old_crtc_state))
> +		if (!is_trans_port_sync_slave(old_crtc_state) &&
> +		    !intel_dp_mst_is_slave_trans(old_crtc_state))
>  			continue;
>  
>  		intel_pre_plane_update(state, crtc);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 83ea04149b77..630a94892b7b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1054,6 +1054,9 @@ struct intel_crtc_state {
>  
>  	/* Bitmask to indicate slaves attached */
>  	u8 sync_mode_slaves_mask;
> +
> +	/* Only valid on TGL+ */
> +	enum transcoder mst_master_transcoder;
>  };
>  
>  struct intel_crtc {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 926e49f449a6..12f5e799d91f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -87,6 +87,49 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
>  	return 0;
>  }
>  
> +/*
> + * Iterate over all connectors and set mst_master_transcoder to the smallest
> + * transcoder id that shares the same MST connector.
> + */
> +static int
> +intel_dp_mst_master_trans_compute(struct intel_encoder *encoder,
> +				  struct intel_crtc_state *crtc_state,
> +				  struct drm_connector_state *connector_state)
> +{
> +	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
> +	struct intel_connector *connector = to_intel_connector(connector_state->connector);
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_digital_connector_state *iter_connector_state;
> +	struct intel_connector *iter_connector;
> +	int i;
> +
> +	if (INTEL_GEN(dev_priv) < 12)
> +		return 0;
> +
> +	crtc_state->mst_master_transcoder = I915_MAX_TRANSCODERS;
> +
> +	for_each_new_intel_connector_in_state(state, iter_connector,
> +					      iter_connector_state, i) {
> +		struct intel_crtc_state *iter_crtc_state;
> +		struct intel_crtc *iter_crtc;
> +
> +		if (connector->mst_port != iter_connector->mst_port ||
> +		    !iter_connector_state->base.crtc)
> +			continue;
> +
> +		iter_crtc = to_intel_crtc(iter_connector_state->base.crtc);
> +		iter_crtc_state = intel_atomic_get_new_crtc_state(state,
> +								  iter_crtc);
> +		if (!iter_crtc_state->uapi.active)
> +			continue;
> +
> +		if (iter_crtc_state->cpu_transcoder < crtc_state->mst_master_transcoder)
> +			crtc_state->mst_master_transcoder = iter_crtc_state->cpu_transcoder;
> +	}
> +
> +	return 0;
> +}

This looks like it can be a pure function that returns the
master transcoder.

> +
>  static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  				       struct intel_crtc_state *pipe_config,
>  				       struct drm_connector_state *conn_state)
> @@ -154,24 +197,88 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  
>  	intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
>  
> +	ret = intel_dp_mst_master_trans_compute(encoder, pipe_config,
> +						conn_state);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +/*
> + * If one of the connectors in a MST stream needs a modeset, mark all CRTCs
> + * that have a connector in the same MST stream as mode changed,
> + * intel_modeset_pipe_config()+intel_crtc_check_fastset() will take to do a
> + * fastset when possible.
> + */
> +static int
> +intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector,
> +				       struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct drm_connector_list_iter connector_list_iter;
> +	struct intel_connector *connector_iter;
> +
> +	if (INTEL_GEN(dev_priv) < 12)
> +		return  0;
> +
> +	if (!intel_digital_connector_needs_modeset(state, connector))
> +		return 0;
> +
> +	drm_connector_list_iter_begin(&dev_priv->drm, &connector_list_iter);
> +	for_each_intel_connector_iter(connector_iter, &connector_list_iter) {
> +		struct intel_digital_connector_state *connector_iter_state;
> +		struct intel_crtc_state *crtc_state;
> +		struct intel_crtc *crtc;
> +
> +		if (connector_iter->mst_port != connector->mst_port)
> +			continue;

Do we need to exclude the connector itself? I guess not really?

> +
> +		connector_iter_state =
> +			intel_atomic_get_digital_connector_state(state,
> +								 connector_iter);
> +		if (IS_ERR(connector_iter_state)) {
> +			drm_connector_list_iter_end(&connector_list_iter);
> +			return PTR_ERR(connector_iter_state);
> +		}
> +
> +		if (!connector_iter_state->base.crtc)
> +			continue;
> +
> +		crtc = to_intel_crtc(connector_iter_state->base.crtc);
> +		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> +		if (IS_ERR(crtc_state)) {
> +			drm_connector_list_iter_end(&connector_list_iter);
> +			return PTR_ERR(crtc_state);
> +		}
> +
> +		crtc_state->uapi.mode_changed = true;
> +	}
> +	drm_connector_list_iter_end(&connector_list_iter);
> +
>  	return 0;
>  }
>  
>  static int
>  intel_dp_mst_atomic_check(struct drm_connector *connector,
> -			  struct drm_atomic_state *state)
> +			  struct drm_atomic_state *_state)
>  {
> +	struct intel_atomic_state *state = to_intel_atomic_state(_state);
>  	struct drm_connector_state *new_conn_state =
> -		drm_atomic_get_new_connector_state(state, connector);
> +		drm_atomic_get_new_connector_state(&state->base, connector);
>  	struct drm_connector_state *old_conn_state =
> -		drm_atomic_get_old_connector_state(state, connector);
> +		drm_atomic_get_old_connector_state(&state->base, connector);
>  	struct intel_connector *intel_connector =
>  		to_intel_connector(connector);
>  	struct drm_crtc *new_crtc = new_conn_state->crtc;
>  	struct drm_dp_mst_topology_mgr *mgr;
>  	int ret;
>  
> -	ret = intel_digital_connector_atomic_check(connector, state);
> +	ret = intel_digital_connector_atomic_check(connector, &state->base);
> +	if (ret)
> +		return ret;
> +
> +	ret = intel_dp_mst_atomic_master_trans_check(intel_connector, state);
>  	if (ret)
>  		return ret;
>  
> @@ -182,12 +289,9 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
>  	 * connector
>  	 */
>  	if (new_crtc) {
> -		struct intel_atomic_state *intel_state =
> -			to_intel_atomic_state(state);
>  		struct intel_crtc *intel_crtc = to_intel_crtc(new_crtc);
>  		struct intel_crtc_state *crtc_state =
> -			intel_atomic_get_new_crtc_state(intel_state,
> -							intel_crtc);
> +			intel_atomic_get_new_crtc_state(state, intel_crtc);
>  
>  		if (!crtc_state ||
>  		    !drm_atomic_crtc_needs_modeset(&crtc_state->uapi) ||
> @@ -196,7 +300,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
>  	}
>  
>  	mgr = &enc_to_mst(old_conn_state->best_encoder)->primary->dp.mst_mgr;
> -	ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
> +	ret = drm_dp_atomic_release_vcpi_slots(&state->base, mgr,
>  					       intel_connector->port);
>  
>  	return ret;
> @@ -241,6 +345,9 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
>  	intel_dp->active_mst_links--;
>  	last_mst_stream = intel_dp->active_mst_links == 0;
>  
> +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream &&
> +		!intel_dp_mst_is_master_trans(old_crtc_state));
> +
>  	/*
>  	 * From TGL spec: "If multi-stream slave transcoder: Configure
>  	 * Transcoder Clock Select to direct no clock to the transcoder"
> @@ -321,6 +428,9 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>  	intel_mst->connector = connector;
>  	first_mst_stream = intel_dp->active_mst_links == 0;
>  
> +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && first_mst_stream &&
> +		!intel_dp_mst_is_master_trans(pipe_config));
> +
>  	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>  
>  	if (first_mst_stream)
> @@ -726,3 +836,14 @@ intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port)
>  	drm_dp_mst_topology_mgr_destroy(&intel_dp->mst_mgr);
>  	/* encoders will get killed by normal cleanup */
>  }
> +
> +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state)
> +{
> +	return crtc_state->mst_master_transcoder == crtc_state->cpu_transcoder;
> +}
> +
> +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
> +{
> +	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
> +	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> index f660ad80db04..e40767f78343 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> @@ -6,10 +6,15 @@
>  #ifndef __INTEL_DP_MST_H__
>  #define __INTEL_DP_MST_H__
>  
> +#include <stdbool.h>

That doesn't seem like something you want to include in kernel code.

> +
>  struct intel_digital_port;
> +struct intel_crtc_state;
>  
>  int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
>  void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
>  int intel_dp_mst_encoder_active_links(struct intel_digital_port *intel_dig_port);
> +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
> +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
>  
>  #endif /* __INTEL_DP_MST_H__ */
> -- 
> 2.24.1

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

Re: [Intel-gfx] [PATCH v2 rebased 08/11] drm/i915/display: Always enables MST master pipe first

[Ville Syrjälä]

On Wed, Dec 11, 2019 at 10:45:23AM -0800, José Roberto de Souza wrote:
> Due to DDB overlaps the pipe enabling sequence is not always crescent.
> As the previous patch selects the smallest pipe/transcoder in the MST
> stream to be master and it needs to be enabled first this changes
> were needed to guarantee that.
> 
> So first lets enable all pipes that did not needed a fullmodeset so
> it don't have any external dependency, this ones can overlap with
> each other ddb allocations.
> 
> Then on the second loop it will enable all the pipes that needs a
> modeset and don't depends on other pipes like MST master
> pipe/transcoder.
> 
> Then finally all the pipes that needs a modeset and have dependency
> on other pipes.
> 
> 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: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 77 ++++++++++++++------
>  1 file changed, 56 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 1cecce2f54f8..fa58b396e084 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14566,18 +14566,24 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  	/*
>  	 * Whenever the number of active pipes changes, we need to make sure we
>  	 * update the pipes in the right order so that their ddb allocations
> -	 * never overlap with eachother inbetween CRTC updates. Otherwise we'll
> +	 * never overlap with each other between CRTC updates. Otherwise we'll
>  	 * cause pipe underruns and other bad stuff.
> +	 *
> +	 * So first lets enable all pipes that did not needed a fullmodeset so
> +	 * it don't have any external dependency
>  	 */
>  	do {
>  		progress = false;
>  
> -		for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> -			enum pipe pipe = crtc->pipe;
> -			bool vbl_wait = false;
> -			bool modeset = needs_modeset(new_crtc_state);
> +		for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +						    new_crtc_state, i) {
> +			bool vbl_wait;
> +
> +			if (updated & BIT(crtc->pipe) ||
> +			    !new_crtc_state->hw.active)
> +				continue;
>  
> -			if (updated & BIT(crtc->pipe) || !new_crtc_state->hw.active)
> +			if (needs_modeset(new_crtc_state))
>  				continue;
>  
>  			if (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> @@ -14585,7 +14591,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  							INTEL_NUM_PIPES(dev_priv), i))
>  				continue;
>  
> -			updated |= BIT(pipe);
> +			updated |= BIT(crtc->pipe);
>  			entries[i] = new_crtc_state->wm.skl.ddb;
>  
>  			/*
> @@ -14596,30 +14602,59 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  			 */
>  			if (!skl_ddb_entry_equal(&new_crtc_state->wm.skl.ddb,
>  						 &old_crtc_state->wm.skl.ddb) &&
> -			    !modeset &&
>  			    state->wm_results.dirty_pipes != updated)

I have a feeling this part is already broken. However I just pushed the
patch to change this to a local dirty_pipes mask. I think what we could
now do on top is split that into eg. update_pipes+modeset_pipes
bitmasks.

Then the first loop just does its thing until update_pipes is
empty. And this check here we can replace with just something like:

if (!ddb_equal() &&
    (update_pipes | modeset_pipes) != 0)

And throw out all modeset checks here because we've already
encoded that in update_pipes vs. modeset_pipes.

>  				vbl_wait = true;
>  
> -			if (modeset && is_trans_port_sync_mode(new_crtc_state)) {
> -				if (is_trans_port_sync_master(new_crtc_state))
> -					intel_update_trans_port_sync_crtcs(crtc,
> -									   state,
> -									   old_crtc_state,
> -									   new_crtc_state);
> -				else
> -					continue;
> -			} else {
> -				intel_update_crtc(crtc, state, old_crtc_state,
> -						  new_crtc_state);
> -			}
> +			intel_update_crtc(crtc, state, old_crtc_state,
> +					  new_crtc_state);
>  
>  			if (vbl_wait)
> -				intel_wait_for_vblank(dev_priv, pipe);
> +				intel_wait_for_vblank(dev_priv, crtc->pipe);
>  
>  			progress = true;
>  		}
>  	} while (progress);
>  
> +	/*
> +	 * Enabling all pipes that needs a modeset and do not depends on other
> +	 * pipes
> +	 */
> +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +					    new_crtc_state, i) {
> +		if (updated & BIT(crtc->pipe) || !new_crtc_state->hw.active)
> +			continue;
> +
> +		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> +		    is_trans_port_sync_slave(new_crtc_state))
> +			continue;
> +
> +		updated |= BIT(crtc->pipe);

I think we should update entries[] still in these lopps + WARN_ON(ddb_overlaps).

> +
> +		if (is_trans_port_sync_mode(new_crtc_state))
> +			intel_update_trans_port_sync_crtcs(crtc, state,
> +							   old_crtc_state,
> +							   new_crtc_state);
> +		else
> +			intel_update_crtc(crtc, state, old_crtc_state,
> +					  new_crtc_state);
> +	}
> +
> +	/*
> +	 * Finally enable all pipes that needs a modeset and depends on
> +	 * other pipes, right now it is only MST slaves as both port sync slave
> +	 * and master are enabled together
> +	 */
> +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +					    new_crtc_state, i) {
> +		if (updated & BIT(crtc->pipe) || !new_crtc_state->hw.active)
> +			continue;
> +
> +		if (is_trans_port_sync_slave(new_crtc_state))
> +			continue;
> +
> +		intel_update_crtc(crtc, state, old_crtc_state, new_crtc_state);
> +	}
> +
>  	/* 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.24.1

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

Re: [Intel-gfx] [PATCH v2 rebased 10/11] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Ville Syrjälä]

On Wed, Dec 11, 2019 at 10:45:25AM -0800, José Roberto de Souza wrote:
> Check if fastset is allowed by external dependencies like other pipes
> and transcoders.
> 
> Right now it only forces a fullmodeset when the MST master transcoder
> did not changed but the pipe of the master transcoder needs a
> fullmodeset so all slaves also needs to do a fullmodeset.
> But it will probably be need for port sync as well.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 41 ++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 092412b10d7c..0c24d7dfa152 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13930,11 +13930,52 @@ static int calc_watermark_data(struct intel_atomic_state *state)
>  	return 0;
>  }
>  
> +/**
> + * Check if fastset is allowed by external dependencies like other pipes and
> + * transcoders.
> + *
> + * Right now it only forces a fullmodeset when the MST master transcoder did
> + * not changed but the pipe of the master transcoder needs a fullmodeset so
> + * all slaves also needs to do a fullmodeset.
> + */
> +static bool
> +intel_crtc_check_external_dependencies_fastset(const struct intel_crtc_state *old_crtc_state,
> +					       struct intel_crtc_state *new_crtc_state)
> +{
> +	struct intel_atomic_state *state = to_intel_atomic_state(new_crtc_state->uapi.state);
> +	struct drm_i915_private *dev_priv = to_i915(new_crtc_state->uapi.crtc->dev);
> +	struct intel_crtc_state *new_crtc_state_iter;
> +	struct intel_crtc *crtc_iter;
> +	int i;
> +
> +	if (INTEL_GEN(dev_priv) < 12)
> +		return true;
> +
> +	if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST) ||
> +	    intel_dp_mst_is_master_trans(new_crtc_state))
> +		return true;
> +
> +	for_each_new_intel_crtc_in_state(state, crtc_iter, new_crtc_state_iter, i) {
> +		if (new_crtc_state_iter->cpu_transcoder !=
> +		    new_crtc_state->mst_master_transcoder)
> +			continue;
> +
> +		return !needs_modeset(new_crtc_state_iter);
> +	}
> +
> +	DRM_ERROR("Master MST transcoder of pipe not found\n");
> +	return false;
> +}
> +
>  static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_state,
>  				     struct intel_crtc_state *new_crtc_state)
>  {
>  	if (!intel_pipe_config_compare(old_crtc_state, new_crtc_state, true))
>  		return;
> +	if (!intel_crtc_check_external_dependencies_fastset(old_crtc_state,
> +							    new_crtc_state))
> +		return;

I don't think this will work. We've not yet .compute_config()'d
everything so the master assignments are still up in the air.

I think we need the logic higher up in intel_atomic_check():

for_each_crtc()
	compute_config()
	if (can_fastset())
		needs_modeset=false
		update_pipe=true;
	}
}

for_each_crtc()
	if (mst_slave && master.needs_modeset() {
		needs_modeset=true;
		update_pipe=false;
	}

for_each_crtc()
	if (update_pipe)
		copy_over_old_state();

	
+ we should probably rename/split update_pipes or add some
lkind of needs_fastset() wrapper to make this less confusing.


> +
>  
>  	new_crtc_state->uapi.mode_changed = false;
>  	new_crtc_state->update_pipe = true;
> -- 
> 2.24.1

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

Re: [Intel-gfx] [PATCH v2 rebased 10/11] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Manasi Navare]

On Thu, Dec 12, 2019 at 11:28:30PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 11, 2019 at 10:45:25AM -0800, José Roberto de Souza wrote:
> > Check if fastset is allowed by external dependencies like other pipes
> > and transcoders.
> > 
> > Right now it only forces a fullmodeset when the MST master transcoder
> > did not changed but the pipe of the master transcoder needs a
> > fullmodeset so all slaves also needs to do a fullmodeset.
> > But it will probably be need for port sync as well.

But in case of port sync, now I have this function in intel_atomic_check()
that will force a full modeset on all the connectors with has_tile if one of
the tiled connectors needs a full modeset.
So we wouldnt need to worry about fastset happening for one of the tiles in port sync.

This patch:
https://patchwork.freedesktop.org/patch/344766/?series=70788&rev=1

Manasi

> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 41 ++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 092412b10d7c..0c24d7dfa152 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -13930,11 +13930,52 @@ static int calc_watermark_data(struct intel_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > +/**
> > + * Check if fastset is allowed by external dependencies like other pipes and
> > + * transcoders.
> > + *
> > + * Right now it only forces a fullmodeset when the MST master transcoder did
> > + * not changed but the pipe of the master transcoder needs a fullmodeset so
> > + * all slaves also needs to do a fullmodeset.
> > + */
> > +static bool
> > +intel_crtc_check_external_dependencies_fastset(const struct intel_crtc_state *old_crtc_state,
> > +					       struct intel_crtc_state *new_crtc_state)
> > +{
> > +	struct intel_atomic_state *state = to_intel_atomic_state(new_crtc_state->uapi.state);
> > +	struct drm_i915_private *dev_priv = to_i915(new_crtc_state->uapi.crtc->dev);
> > +	struct intel_crtc_state *new_crtc_state_iter;
> > +	struct intel_crtc *crtc_iter;
> > +	int i;
> > +
> > +	if (INTEL_GEN(dev_priv) < 12)
> > +		return true;
> > +
> > +	if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST) ||
> > +	    intel_dp_mst_is_master_trans(new_crtc_state))
> > +		return true;
> > +
> > +	for_each_new_intel_crtc_in_state(state, crtc_iter, new_crtc_state_iter, i) {
> > +		if (new_crtc_state_iter->cpu_transcoder !=
> > +		    new_crtc_state->mst_master_transcoder)
> > +			continue;
> > +
> > +		return !needs_modeset(new_crtc_state_iter);
> > +	}
> > +
> > +	DRM_ERROR("Master MST transcoder of pipe not found\n");
> > +	return false;
> > +}
> > +
> >  static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_state,
> >  				     struct intel_crtc_state *new_crtc_state)
> >  {
> >  	if (!intel_pipe_config_compare(old_crtc_state, new_crtc_state, true))
> >  		return;
> > +	if (!intel_crtc_check_external_dependencies_fastset(old_crtc_state,
> > +							    new_crtc_state))
> > +		return;
> 
> I don't think this will work. We've not yet .compute_config()'d
> everything so the master assignments are still up in the air.
> 
> I think we need the logic higher up in intel_atomic_check():
> 
> for_each_crtc()
> 	compute_config()
> 	if (can_fastset())
> 		needs_modeset=false
> 		update_pipe=true;
> 	}
> }
> 
> for_each_crtc()
> 	if (mst_slave && master.needs_modeset() {
> 		needs_modeset=true;
> 		update_pipe=false;
> 	}
> 
> for_each_crtc()
> 	if (update_pipe)
> 		copy_over_old_state();
> 
> 	
> + we should probably rename/split update_pipes or add some
> lkind of needs_fastset() wrapper to make this less confusing.
> 
> 
> > +
> >  
> >  	new_crtc_state->uapi.mode_changed = false;
> >  	new_crtc_state->update_pipe = true;
> > -- 
> > 2.24.1
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 rebased 07/11] drm/i915/tgl: Select master transcoder for MST stream

[Ville Syrjälä]

On Thu, Dec 12, 2019 at 10:44:29PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 11, 2019 at 10:45:22AM -0800, José Roberto de Souza wrote:
> > On TGL the blending of all the streams have moved from DDI to
> > transcoder, so now every transcoder working over the same MST port must
> > send its stream to a master transcoder and master will send to DDI
> > respecting the time slots.
> > 
> > So here adding all the CRTCs that shares the same MST stream if
> > needed and computing their state again, it will pick the lowest
> > pipe/transcoder among the ones in the same stream to be master.
> > 
> > Most of the time skl_commit_modeset_enables() enables pipes in a
> > crescent order but due DDB overlapping it might not happen, this
> > scenarios will be handled in the next patch.
> > 
> > v2:
> > - Using recently added intel_crtc_state_reset() to set
> > mst_master_transcoder to invalid transcoder for all non gen12 & MST
> > code paths
> > - Setting lowest pipe/transcoder as master, previously it was the
> > first one but setting a predictable one will help in future MST e
> > port sync integration
> > - Moving to intel type as much as we can
> > 
> > BSpec: 50493
> > BSpec: 49190
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_atomic.c   |  14 ++
> >  drivers/gpu/drm/i915/display/intel_atomic.h   |   3 +
> >  drivers/gpu/drm/i915/display/intel_ddi.c      |  14 +-
> >  drivers/gpu/drm/i915/display/intel_display.c  |  13 +-
> >  .../drm/i915/display/intel_display_types.h    |   3 +
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c   | 139 ++++++++++++++++--
> >  drivers/gpu/drm/i915/display/intel_dp_mst.h   |   5 +
> >  7 files changed, 178 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index 6e93a39a6fec..69a0430c4638 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -206,6 +206,20 @@ intel_digital_connector_needs_modeset(struct intel_atomic_state *state,
> >  	       (new_crtc && drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi));
> >  }
> >  
> > +struct intel_digital_connector_state *
> > +intel_atomic_get_digital_connector_state(struct intel_atomic_state *state,
> > +					 struct intel_connector *connector)
> > +{
> > +	struct drm_connector_state *connector_state;
> > +
> > +	connector_state = drm_atomic_get_connector_state(&state->base,
> > +							 &connector->base);
> > +	if (IS_ERR(connector_state))
> > +		return ERR_CAST(connector_state);
> > +
> > +	return to_intel_digital_connector_state(connector_state);
> > +}
> > +
> >  /**
> >   * intel_crtc_duplicate_state - duplicate crtc state
> >   * @crtc: drm crtc
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
> > index ba9cc29a5865..6e8638d83d28 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> > @@ -35,6 +35,9 @@ struct drm_connector_state *
> >  intel_digital_connector_duplicate_state(struct drm_connector *connector);
> >  bool intel_digital_connector_needs_modeset(struct intel_atomic_state *state,
> >  					   struct intel_connector *connector);
> > +struct intel_digital_connector_state *
> > +intel_atomic_get_digital_connector_state(struct intel_atomic_state *state,
> > +					 struct intel_connector *connector);
> >  
> >  struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
> >  void intel_crtc_destroy_state(struct drm_crtc *crtc,
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 5b6f32517c75..6ee5230045eb 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -1903,8 +1903,13 @@ intel_ddi_transcoder_func_reg_val_get(const struct intel_crtc_state *crtc_state)
> >  		temp |= TRANS_DDI_MODE_SELECT_DP_MST;
> >  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> >  
> > -		if (INTEL_GEN(dev_priv) >= 12)
> > -			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(crtc_state->cpu_transcoder);
> > +		if (INTEL_GEN(dev_priv) >= 12) {
> > +			enum transcoder master;
> > +
> > +			master = crtc_state->mst_master_transcoder;
> > +			WARN_ON(master == INVALID_TRANSCODER);
> > +			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master);
> > +		}
> >  	} else {
> >  		temp |= TRANS_DDI_MODE_SELECT_DP_SST;
> >  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> > @@ -4377,6 +4382,11 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
> >  		pipe_config->output_types |= BIT(INTEL_OUTPUT_DP_MST);
> >  		pipe_config->lane_count =
> >  			((temp & DDI_PORT_WIDTH_MASK) >> DDI_PORT_WIDTH_SHIFT) + 1;
> > +
> > +		if (INTEL_GEN(dev_priv) >= 12)
> > +			pipe_config->mst_master_transcoder =
> > +					REG_FIELD_GET(TRANS_DDI_MST_TRANSPORT_SELECT_MASK, temp);
> > +
> >  		intel_dp_get_m_n(intel_crtc, pipe_config);
> >  		break;
> >  	default:
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 39b00a19d752..1cecce2f54f8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -46,6 +46,7 @@
> >  #include "display/intel_crt.h"
> >  #include "display/intel_ddi.h"
> >  #include "display/intel_dp.h"
> > +#include "display/intel_dp_mst.h"
> >  #include "display/intel_dsi.h"
> >  #include "display/intel_dvo.h"
> >  #include "display/intel_gmbus.h"
> > @@ -11663,6 +11664,7 @@ static void intel_crtc_state_reset(struct intel_crtc_state *crtc_state,
> >  	crtc_state->hsw_workaround_pipe = INVALID_PIPE;
> >  	crtc_state->output_format = INTEL_OUTPUT_FORMAT_INVALID;
> >  	crtc_state->scaler_state.scaler_id = -1;
> > +	crtc_state->mst_master_transcoder = INVALID_TRANSCODER;
> >  }
> >  
> >  /* Returns the currently programmed mode of the given encoder. */
> > @@ -12510,6 +12512,9 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
> >  			      pipe_config->csc_mode, pipe_config->gamma_mode,
> >  			      pipe_config->gamma_enable, pipe_config->csc_enable);
> >  
> > +	DRM_DEBUG_KMS("MST master transcoder: %s\n",
> > +		      transcoder_name(pipe_config->mst_master_transcoder));
> > +
> >  dump_planes:
> >  	if (!state)
> >  		return;
> > @@ -12651,6 +12656,7 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
> >  	memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls,
> >  	       sizeof(saved_state->icl_port_dplls));
> >  	saved_state->crc_enabled = crtc_state->crc_enabled;
> > +	saved_state->mst_master_transcoder = INVALID_TRANSCODER;
> >  	if (IS_G4X(dev_priv) ||
> >  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >  		saved_state->wm = crtc_state->wm;
> > @@ -13290,6 +13296,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> >  	PIPE_CONF_CHECK_I(dsc.dsc_split);
> >  	PIPE_CONF_CHECK_I(dsc.compressed_bpp);
> >  
> > +	PIPE_CONF_CHECK_I(mst_master_transcoder);
> > +
> >  #undef PIPE_CONF_CHECK_X
> >  #undef PIPE_CONF_CHECK_I
> >  #undef PIPE_CONF_CHECK_BOOL
> > @@ -14374,7 +14382,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> >  	u32 handled = 0;
> >  	int i;
> >  
> > -	/* Only disable port sync slaves */
> > +	/* Only disable port sync and MST slaves */
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> >  					    new_crtc_state, i) {
> >  		if (!needs_modeset(new_crtc_state))
> > @@ -14388,7 +14396,8 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> >  		 * slave CRTCs are disabled first and then master CRTC since
> >  		 * Slave vblanks are masked till Master Vblanks.
> >  		 */
> > -		if (!is_trans_port_sync_slave(old_crtc_state))
> > +		if (!is_trans_port_sync_slave(old_crtc_state) &&
> > +		    !intel_dp_mst_is_slave_trans(old_crtc_state))
> >  			continue;
> >  
> >  		intel_pre_plane_update(state, crtc);
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 83ea04149b77..630a94892b7b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1054,6 +1054,9 @@ struct intel_crtc_state {
> >  
> >  	/* Bitmask to indicate slaves attached */
> >  	u8 sync_mode_slaves_mask;
> > +
> > +	/* Only valid on TGL+ */
> > +	enum transcoder mst_master_transcoder;
> >  };
> >  
> >  struct intel_crtc {
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 926e49f449a6..12f5e799d91f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -87,6 +87,49 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Iterate over all connectors and set mst_master_transcoder to the smallest
> > + * transcoder id that shares the same MST connector.
> > + */
> > +static int
> > +intel_dp_mst_master_trans_compute(struct intel_encoder *encoder,
> > +				  struct intel_crtc_state *crtc_state,
> > +				  struct drm_connector_state *connector_state)
> > +{
> > +	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
> > +	struct intel_connector *connector = to_intel_connector(connector_state->connector);
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > +	struct intel_digital_connector_state *iter_connector_state;
> > +	struct intel_connector *iter_connector;
> > +	int i;
> > +
> > +	if (INTEL_GEN(dev_priv) < 12)
> > +		return 0;
> > +
> > +	crtc_state->mst_master_transcoder = I915_MAX_TRANSCODERS;
> > +
> > +	for_each_new_intel_connector_in_state(state, iter_connector,
> > +					      iter_connector_state, i) {
> > +		struct intel_crtc_state *iter_crtc_state;
> > +		struct intel_crtc *iter_crtc;
> > +
> > +		if (connector->mst_port != iter_connector->mst_port ||
> > +		    !iter_connector_state->base.crtc)
> > +			continue;
> > +
> > +		iter_crtc = to_intel_crtc(iter_connector_state->base.crtc);
> > +		iter_crtc_state = intel_atomic_get_new_crtc_state(state,
> > +								  iter_crtc);
> > +		if (!iter_crtc_state->uapi.active)
> > +			continue;
> > +
> > +		if (iter_crtc_state->cpu_transcoder < crtc_state->mst_master_transcoder)
> > +			crtc_state->mst_master_transcoder = iter_crtc_state->cpu_transcoder;

Was just reading Manasi's port sync stuff when I realized this won't
work. We haven't necessarily compute cpu_transcoder for the other
pipes yet. So I suggest we extract the code to compute the cpu_transcoder
from intel_ddi_connector_get_hw_state() and use it everywhere, including
here.

> > +	}
> > +
> > +	return 0;
> > +}
> 
> This looks like it can be a pure function that returns the
> master transcoder.
> 
> > +
> >  static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  				       struct intel_crtc_state *pipe_config,
> >  				       struct drm_connector_state *conn_state)
> > @@ -154,24 +197,88 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  
> >  	intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
> >  
> > +	ret = intel_dp_mst_master_trans_compute(encoder, pipe_config,
> > +						conn_state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * If one of the connectors in a MST stream needs a modeset, mark all CRTCs
> > + * that have a connector in the same MST stream as mode changed,
> > + * intel_modeset_pipe_config()+intel_crtc_check_fastset() will take to do a
> > + * fastset when possible.
> > + */
> > +static int
> > +intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector,
> > +				       struct intel_atomic_state *state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	struct drm_connector_list_iter connector_list_iter;
> > +	struct intel_connector *connector_iter;
> > +
> > +	if (INTEL_GEN(dev_priv) < 12)
> > +		return  0;
> > +
> > +	if (!intel_digital_connector_needs_modeset(state, connector))
> > +		return 0;
> > +
> > +	drm_connector_list_iter_begin(&dev_priv->drm, &connector_list_iter);
> > +	for_each_intel_connector_iter(connector_iter, &connector_list_iter) {
> > +		struct intel_digital_connector_state *connector_iter_state;
> > +		struct intel_crtc_state *crtc_state;
> > +		struct intel_crtc *crtc;
> > +
> > +		if (connector_iter->mst_port != connector->mst_port)
> > +			continue;
> 
> Do we need to exclude the connector itself? I guess not really?
> 
> > +
> > +		connector_iter_state =
> > +			intel_atomic_get_digital_connector_state(state,
> > +								 connector_iter);
> > +		if (IS_ERR(connector_iter_state)) {
> > +			drm_connector_list_iter_end(&connector_list_iter);
> > +			return PTR_ERR(connector_iter_state);
> > +		}
> > +
> > +		if (!connector_iter_state->base.crtc)
> > +			continue;
> > +
> > +		crtc = to_intel_crtc(connector_iter_state->base.crtc);
> > +		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> > +		if (IS_ERR(crtc_state)) {
> > +			drm_connector_list_iter_end(&connector_list_iter);
> > +			return PTR_ERR(crtc_state);
> > +		}
> > +
> > +		crtc_state->uapi.mode_changed = true;
> > +	}
> > +	drm_connector_list_iter_end(&connector_list_iter);
> > +
> >  	return 0;
> >  }
> >  
> >  static int
> >  intel_dp_mst_atomic_check(struct drm_connector *connector,
> > -			  struct drm_atomic_state *state)
> > +			  struct drm_atomic_state *_state)
> >  {
> > +	struct intel_atomic_state *state = to_intel_atomic_state(_state);
> >  	struct drm_connector_state *new_conn_state =
> > -		drm_atomic_get_new_connector_state(state, connector);
> > +		drm_atomic_get_new_connector_state(&state->base, connector);
> >  	struct drm_connector_state *old_conn_state =
> > -		drm_atomic_get_old_connector_state(state, connector);
> > +		drm_atomic_get_old_connector_state(&state->base, connector);
> >  	struct intel_connector *intel_connector =
> >  		to_intel_connector(connector);
> >  	struct drm_crtc *new_crtc = new_conn_state->crtc;
> >  	struct drm_dp_mst_topology_mgr *mgr;
> >  	int ret;
> >  
> > -	ret = intel_digital_connector_atomic_check(connector, state);
> > +	ret = intel_digital_connector_atomic_check(connector, &state->base);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = intel_dp_mst_atomic_master_trans_check(intel_connector, state);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -182,12 +289,9 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
> >  	 * connector
> >  	 */
> >  	if (new_crtc) {
> > -		struct intel_atomic_state *intel_state =
> > -			to_intel_atomic_state(state);
> >  		struct intel_crtc *intel_crtc = to_intel_crtc(new_crtc);
> >  		struct intel_crtc_state *crtc_state =
> > -			intel_atomic_get_new_crtc_state(intel_state,
> > -							intel_crtc);
> > +			intel_atomic_get_new_crtc_state(state, intel_crtc);
> >  
> >  		if (!crtc_state ||
> >  		    !drm_atomic_crtc_needs_modeset(&crtc_state->uapi) ||
> > @@ -196,7 +300,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
> >  	}
> >  
> >  	mgr = &enc_to_mst(old_conn_state->best_encoder)->primary->dp.mst_mgr;
> > -	ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
> > +	ret = drm_dp_atomic_release_vcpi_slots(&state->base, mgr,
> >  					       intel_connector->port);
> >  
> >  	return ret;
> > @@ -241,6 +345,9 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
> >  	intel_dp->active_mst_links--;
> >  	last_mst_stream = intel_dp->active_mst_links == 0;
> >  
> > +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream &&
> > +		!intel_dp_mst_is_master_trans(old_crtc_state));
> > +
> >  	/*
> >  	 * From TGL spec: "If multi-stream slave transcoder: Configure
> >  	 * Transcoder Clock Select to direct no clock to the transcoder"
> > @@ -321,6 +428,9 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> >  	intel_mst->connector = connector;
> >  	first_mst_stream = intel_dp->active_mst_links == 0;
> >  
> > +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && first_mst_stream &&
> > +		!intel_dp_mst_is_master_trans(pipe_config));
> > +
> >  	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
> >  
> >  	if (first_mst_stream)
> > @@ -726,3 +836,14 @@ intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port)
> >  	drm_dp_mst_topology_mgr_destroy(&intel_dp->mst_mgr);
> >  	/* encoders will get killed by normal cleanup */
> >  }
> > +
> > +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state)
> > +{
> > +	return crtc_state->mst_master_transcoder == crtc_state->cpu_transcoder;
> > +}
> > +
> > +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
> > +{
> > +	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
> > +	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > index f660ad80db04..e40767f78343 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > @@ -6,10 +6,15 @@
> >  #ifndef __INTEL_DP_MST_H__
> >  #define __INTEL_DP_MST_H__
> >  
> > +#include <stdbool.h>
> 
> That doesn't seem like something you want to include in kernel code.
> 
> > +
> >  struct intel_digital_port;
> > +struct intel_crtc_state;
> >  
> >  int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
> >  void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
> >  int intel_dp_mst_encoder_active_links(struct intel_digital_port *intel_dig_port);
> > +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
> > +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
> >  
> >  #endif /* __INTEL_DP_MST_H__ */
> > -- 
> > 2.24.1
> 
> -- 
> Ville Syrjälä
> Intel

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

Re: [Intel-gfx] [PATCH v2 rebased 06/11] drm/i915/display: Share intel_connector_needs_modeset()

[Lucas De Marchi]

On Thu, Dec 12, 2019 at 05:52:49PM +0200, Ville Syrjälä wrote:
>On Wed, Dec 11, 2019 at 10:45:21AM -0800, José Roberto de Souza wrote:
>> intel_connector_needs_modeset() will be used outside of
>> intel_display.c in a future patch so it would only be necessary to
>> remove the state and add the prototype to the header file.
>>
>> But while at it, I simplified the arguments and changed to intel
>> types and moved it to a better place intel_atomic.c.
>>
>> That allowed us to convert the whole
>> intel_encoders_update_prepare/complete to intel type too.
>>
>> No behavior changes intended here.
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_atomic.c  | 32 ++++++++++++
>>  drivers/gpu/drm/i915/display/intel_atomic.h  |  3 ++
>>  drivers/gpu/drm/i915/display/intel_display.c | 53 ++++++--------------
>>  3 files changed, 51 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
>> index fd0026fc3618..6e93a39a6fec 100644
>> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
>> @@ -174,6 +174,38 @@ intel_digital_connector_duplicate_state(struct drm_connector *connector)
>>  	return &state->base;
>>  }
>>
>> +/**
>> + * intel_digital_connector_needs_modeset - check if connector needs a modeset
>> + */
>> +bool
>> +intel_digital_connector_needs_modeset(struct intel_atomic_state *state,
>
>Why "digital"? Oh because intel_atomic_get_old_connector_state() return
>a ditgital_connector_state. A bit surprising.
>
>I suggest using just drm_connector_state here to keep this function
>totally generic.
>
>> +				      struct intel_connector *connector)
>> +{
>> +	struct intel_digital_connector_state *old_connector_state, *new_connector_state;
>> +	struct intel_crtc *old_crtc, *new_crtc;
>> +	struct intel_crtc_state *new_crtc_state;
>> +
>> +	old_connector_state = intel_atomic_get_old_connector_state(state,
>> +								   connector);
>
>Could be done when declaring the variable. Dunno which is prettier
>though.
>
>> +	if (old_connector_state->base.crtc)
>> +		old_crtc = to_intel_crtc(old_connector_state->base.crtc);
>> +	else
>> +		old_crtc = NULL;
>
>Simple
>old_crtc = to_intel_crtc(old_connector_state->base.crtc);
>will do. Can be done when declaring the variable as well.
>
>> +
>> +	new_connector_state = intel_atomic_get_new_connector_state(state,
>> +								   connector);
>> +	if (new_connector_state->base.crtc) {
>> +		new_crtc = to_intel_crtc(new_connector_state->base.crtc);
>
>ditto.
>
>> +		new_crtc_state = intel_atomic_get_new_crtc_state(state, new_crtc);
>
>Then this just becomes
>if (new_crtc)
>	new_crtc_state = ...;
>
>Or maybe
>new_crtc_state = new_crtc ? get : NULL;
>but that could be a bit ugly.
>
>
>> +	} else {
>> +		new_crtc_state = NULL;
>> +		new_crtc = NULL;
>> +	}
>> +
>> +	return new_crtc != old_crtc ||
>> +	       (new_crtc && drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi));
>
>Hmm. In fact this function could be one of those special cases where we
>might even want to use all drm_ types internally since we don't actually
>need anything else.

so... do you mean to bring intel_connector_needs_modeset() as is?

Lucas De Marchi

>
>> +}
>> +
>>  /**
>>   * intel_crtc_duplicate_state - duplicate crtc state
>>   * @crtc: drm crtc
>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
>> index 7b49623419ba..ba9cc29a5865 100644
>> --- a/drivers/gpu/drm/i915/display/intel_atomic.h
>> +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
>> @@ -17,6 +17,7 @@ struct drm_device;
>>  struct drm_i915_private;
>>  struct drm_property;
>>  struct intel_atomic_state;
>> +struct intel_connector;
>>  struct intel_crtc;
>>  struct intel_crtc_state;
>>
>> @@ -32,6 +33,8 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
>>  					 struct drm_atomic_state *state);
>>  struct drm_connector_state *
>>  intel_digital_connector_duplicate_state(struct drm_connector *connector);
>> +bool intel_digital_connector_needs_modeset(struct intel_atomic_state *state,
>> +					   struct intel_connector *connector);
>>
>>  struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
>>  void intel_crtc_destroy_state(struct drm_crtc *crtc,
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index b4e44d3cd275..39b00a19d752 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -6185,71 +6185,50 @@ intel_connector_primary_encoder(struct intel_connector *connector)
>>  	return encoder;
>>  }
>>
>> -static bool
>> -intel_connector_needs_modeset(struct intel_atomic_state *state,
>> -			      const struct drm_connector_state *old_conn_state,
>> -			      const struct drm_connector_state *new_conn_state)
>> -{
>> -	struct intel_crtc *old_crtc = old_conn_state->crtc ?
>> -				      to_intel_crtc(old_conn_state->crtc) : NULL;
>> -	struct intel_crtc *new_crtc = new_conn_state->crtc ?
>> -				      to_intel_crtc(new_conn_state->crtc) : NULL;
>> -
>> -	return new_crtc != old_crtc ||
>> -	       (new_crtc &&
>> -		needs_modeset(intel_atomic_get_new_crtc_state(state, new_crtc)));
>> -}
>> -
>>  static void intel_encoders_update_prepare(struct intel_atomic_state *state)
>>  {
>> -	struct drm_connector_state *old_conn_state;
>> -	struct drm_connector_state *new_conn_state;
>> -	struct drm_connector *conn;
>> +	struct intel_digital_connector_state *new_connector_state;
>> +	struct intel_connector *connector;
>>  	int i;
>>
>> -	for_each_oldnew_connector_in_state(&state->base, conn,
>> -					   old_conn_state, new_conn_state, i) {
>> +	for_each_new_intel_connector_in_state(state, connector,
>> +					      new_connector_state, i) {
>>  		struct intel_encoder *encoder;
>>  		struct intel_crtc *crtc;
>>
>> -		if (!intel_connector_needs_modeset(state,
>> -						   old_conn_state,
>> -						   new_conn_state))
>> +		if (!intel_digital_connector_needs_modeset(state, connector))
>>  			continue;
>>
>> -		encoder = intel_connector_primary_encoder(to_intel_connector(conn));
>> +		encoder = intel_connector_primary_encoder(connector);
>>  		if (!encoder->update_prepare)
>>  			continue;
>>
>> -		crtc = new_conn_state->crtc ?
>> -			to_intel_crtc(new_conn_state->crtc) : NULL;
>> +		crtc = new_connector_state->base.crtc ?
>> +			to_intel_crtc(new_connector_state->base.crtc) : NULL;
>>  		encoder->update_prepare(state, encoder, crtc);
>>  	}
>>  }
>>
>>  static void intel_encoders_update_complete(struct intel_atomic_state *state)
>>  {
>> -	struct drm_connector_state *old_conn_state;
>> -	struct drm_connector_state *new_conn_state;
>> -	struct drm_connector *conn;
>> +	struct intel_digital_connector_state *new_connector_state;
>> +	struct intel_connector *connector;
>>  	int i;
>>
>> -	for_each_oldnew_connector_in_state(&state->base, conn,
>> -					   old_conn_state, new_conn_state, i) {
>> +	for_each_new_intel_connector_in_state(state, connector,
>> +					      new_connector_state, i) {
>>  		struct intel_encoder *encoder;
>>  		struct intel_crtc *crtc;
>>
>> -		if (!intel_connector_needs_modeset(state,
>> -						   old_conn_state,
>> -						   new_conn_state))
>> +		if (!intel_digital_connector_needs_modeset(state, connector))
>>  			continue;
>>
>> -		encoder = intel_connector_primary_encoder(to_intel_connector(conn));
>> +		encoder = intel_connector_primary_encoder(connector);
>>  		if (!encoder->update_complete)
>>  			continue;
>>
>> -		crtc = new_conn_state->crtc ?
>> -			to_intel_crtc(new_conn_state->crtc) : NULL;
>> +		crtc = new_connector_state->base.crtc ?
>> +			to_intel_crtc(new_connector_state->base.crtc) : NULL;
>>  		encoder->update_complete(state, encoder, crtc);
>>  	}
>>  }
>> --
>> 2.24.1
>
>-- 
>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

Re: [Intel-gfx] [PATCH v2 rebased 06/11] drm/i915/display: Share intel_connector_needs_modeset()

[Ville Syrjälä]

On Fri, Dec 13, 2019 at 04:14:55PM -0800, Lucas De Marchi wrote:
> On Thu, Dec 12, 2019 at 05:52:49PM +0200, Ville Syrjälä wrote:
> >On Wed, Dec 11, 2019 at 10:45:21AM -0800, José Roberto de Souza wrote:
> >> intel_connector_needs_modeset() will be used outside of
> >> intel_display.c in a future patch so it would only be necessary to
> >> remove the state and add the prototype to the header file.
> >>
> >> But while at it, I simplified the arguments and changed to intel
> >> types and moved it to a better place intel_atomic.c.
> >>
> >> That allowed us to convert the whole
> >> intel_encoders_update_prepare/complete to intel type too.
> >>
> >> No behavior changes intended here.
> >>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_atomic.c  | 32 ++++++++++++
> >>  drivers/gpu/drm/i915/display/intel_atomic.h  |  3 ++
> >>  drivers/gpu/drm/i915/display/intel_display.c | 53 ++++++--------------
> >>  3 files changed, 51 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> >> index fd0026fc3618..6e93a39a6fec 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> >> @@ -174,6 +174,38 @@ intel_digital_connector_duplicate_state(struct drm_connector *connector)
> >>  	return &state->base;
> >>  }
> >>
> >> +/**
> >> + * intel_digital_connector_needs_modeset - check if connector needs a modeset
> >> + */
> >> +bool
> >> +intel_digital_connector_needs_modeset(struct intel_atomic_state *state,
> >
> >Why "digital"? Oh because intel_atomic_get_old_connector_state() return
> >a ditgital_connector_state. A bit surprising.
> >
> >I suggest using just drm_connector_state here to keep this function
> >totally generic.
> >
> >> +				      struct intel_connector *connector)
> >> +{
> >> +	struct intel_digital_connector_state *old_connector_state, *new_connector_state;
> >> +	struct intel_crtc *old_crtc, *new_crtc;
> >> +	struct intel_crtc_state *new_crtc_state;
> >> +
> >> +	old_connector_state = intel_atomic_get_old_connector_state(state,
> >> +								   connector);
> >
> >Could be done when declaring the variable. Dunno which is prettier
> >though.
> >
> >> +	if (old_connector_state->base.crtc)
> >> +		old_crtc = to_intel_crtc(old_connector_state->base.crtc);
> >> +	else
> >> +		old_crtc = NULL;
> >
> >Simple
> >old_crtc = to_intel_crtc(old_connector_state->base.crtc);
> >will do. Can be done when declaring the variable as well.
> >
> >> +
> >> +	new_connector_state = intel_atomic_get_new_connector_state(state,
> >> +								   connector);
> >> +	if (new_connector_state->base.crtc) {
> >> +		new_crtc = to_intel_crtc(new_connector_state->base.crtc);
> >
> >ditto.
> >
> >> +		new_crtc_state = intel_atomic_get_new_crtc_state(state, new_crtc);
> >
> >Then this just becomes
> >if (new_crtc)
> >	new_crtc_state = ...;
> >
> >Or maybe
> >new_crtc_state = new_crtc ? get : NULL;
> >but that could be a bit ugly.
> >
> >
> >> +	} else {
> >> +		new_crtc_state = NULL;
> >> +		new_crtc = NULL;
> >> +	}
> >> +
> >> +	return new_crtc != old_crtc ||
> >> +	       (new_crtc && drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi));
> >
> >Hmm. In fact this function could be one of those special cases where we
> >might even want to use all drm_ types internally since we don't actually
> >need anything else.
> 
> so... do you mean to bring intel_connector_needs_modeset() as is?

Maybe... Yeah, looks more useful as is.

> >> -static bool
> >> -intel_connector_needs_modeset(struct intel_atomic_state *state,
> >> -			      const struct drm_connector_state *old_conn_state,
> >> -			      const struct drm_connector_state *new_conn_state)
> >> -{
> >> -	struct intel_crtc *old_crtc = old_conn_state->crtc ?
> >> -				      to_intel_crtc(old_conn_state->crtc) : NULL;
> >> -	struct intel_crtc *new_crtc = new_conn_state->crtc ?
> >> -				      to_intel_crtc(new_conn_state->crtc) : NULL;

You could toss out those ternary operators while at it. They're not needed.

> >> -
> >> -	return new_crtc != old_crtc ||
> >> -	       (new_crtc &&
> >> -		needs_modeset(intel_atomic_get_new_crtc_state(state, new_crtc)));
> >> -}
> >> -

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

Re: [Intel-gfx] [PATCH v2 rebased 06/11] drm/i915/display: Share intel_connector_needs_modeset()

[Souza, Jose]

On Thu, 2019-12-12 at 17:52 +0200, Ville Syrjälä wrote:
> On Wed, Dec 11, 2019 at 10:45:21AM -0800, José Roberto de Souza
> wrote:
> > intel_connector_needs_modeset() will be used outside of
> > intel_display.c in a future patch so it would only be necessary to
> > remove the state and add the prototype to the header file.
> > 
> > But while at it, I simplified the arguments and changed to intel
> > types and moved it to a better place intel_atomic.c.
> > 
> > That allowed us to convert the whole
> > intel_encoders_update_prepare/complete to intel type too.
> > 
> > No behavior changes intended here.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_atomic.c  | 32 ++++++++++++
> >  drivers/gpu/drm/i915/display/intel_atomic.h  |  3 ++
> >  drivers/gpu/drm/i915/display/intel_display.c | 53 ++++++--------
> > ------
> >  3 files changed, 51 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index fd0026fc3618..6e93a39a6fec 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -174,6 +174,38 @@ intel_digital_connector_duplicate_state(struct
> > drm_connector *connector)
> >  	return &state->base;
> >  }
> >  
> > +/**
> > + * intel_digital_connector_needs_modeset - check if connector
> > needs a modeset
> > + */
> > +bool
> > +intel_digital_connector_needs_modeset(struct intel_atomic_state
> > *state,
> 
> Why "digital"? Oh because intel_atomic_get_old_connector_state()
> return
> a ditgital_connector_state. A bit surprising.

All the exported functions that handles intel_digital_connector in
intel_atomic.h have digital in the name, just keeping it consistent.

> 
> I suggest using just drm_connector_state here to keep this function
> totally generic.
> 
> > +				      struct intel_connector
> > *connector)
> > +{
> > +	struct intel_digital_connector_state *old_connector_state,
> > *new_connector_state;
> > +	struct intel_crtc *old_crtc, *new_crtc;
> > +	struct intel_crtc_state *new_crtc_state;
> > +
> > +	old_connector_state =
> > intel_atomic_get_old_connector_state(state,
> > +								   conn
> > ector);
> 
> Could be done when declaring the variable. Dunno which is prettier
> though.

Mostly avoiding lines over 80 columns

> 
> > +	if (old_connector_state->base.crtc)
> > +		old_crtc = to_intel_crtc(old_connector_state-
> > >base.crtc);
> > +	else
> > +		old_crtc = NULL;
> 
> Simple
> old_crtc = to_intel_crtc(old_connector_state->base.crtc); 
> will do. Can be done when declaring the variable as well.
> 
> > +
> > +	new_connector_state =
> > intel_atomic_get_new_connector_state(state,
> > +								   conn
> > ector);
> > +	if (new_connector_state->base.crtc) {
> > +		new_crtc = to_intel_crtc(new_connector_state-
> > >base.crtc);
> 
> ditto.
> 
> > +		new_crtc_state = intel_atomic_get_new_crtc_state(state,
> > new_crtc);
> 
> Then this just becomes
> if (new_crtc)
> 	new_crtc_state = ...;
> 
> Or maybe
> new_crtc_state = new_crtc ? get : NULL;
> but that could be a bit ugly.
> 
> 
> > +	} else {
> > +		new_crtc_state = NULL;
> > +		new_crtc = NULL;
> > +	}
> > +
> > +	return new_crtc != old_crtc ||
> > +	       (new_crtc &&
> > drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi));
> 
> Hmm. In fact this function could be one of those special cases where
> we
> might even want to use all drm_ types internally since we don't
> actually
> need anything else.

I'm fine in both ways, so I will be going back to drm_ types and
removing the digital from the function name.

> 
> > +}
> > +
> >  /**
> >   * intel_crtc_duplicate_state - duplicate crtc state
> >   * @crtc: drm crtc
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h
> > b/drivers/gpu/drm/i915/display/intel_atomic.h
> > index 7b49623419ba..ba9cc29a5865 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> > @@ -17,6 +17,7 @@ struct drm_device;
> >  struct drm_i915_private;
> >  struct drm_property;
> >  struct intel_atomic_state;
> > +struct intel_connector;
> >  struct intel_crtc;
> >  struct intel_crtc_state;
> >  
> > @@ -32,6 +33,8 @@ int intel_digital_connector_atomic_check(struct
> > drm_connector *conn,
> >  					 struct drm_atomic_state
> > *state);
> >  struct drm_connector_state *
> >  intel_digital_connector_duplicate_state(struct drm_connector
> > *connector);
> > +bool intel_digital_connector_needs_modeset(struct
> > intel_atomic_state *state,
> > +					   struct intel_connector
> > *connector);
> >  
> >  struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc
> > *crtc);
> >  void intel_crtc_destroy_state(struct drm_crtc *crtc,
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index b4e44d3cd275..39b00a19d752 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -6185,71 +6185,50 @@ intel_connector_primary_encoder(struct
> > intel_connector *connector)
> >  	return encoder;
> >  }
> >  
> > -static bool
> > -intel_connector_needs_modeset(struct intel_atomic_state *state,
> > -			      const struct drm_connector_state
> > *old_conn_state,
> > -			      const struct drm_connector_state
> > *new_conn_state)
> > -{
> > -	struct intel_crtc *old_crtc = old_conn_state->crtc ?
> > -				      to_intel_crtc(old_conn_state-
> > >crtc) : NULL;
> > -	struct intel_crtc *new_crtc = new_conn_state->crtc ?
> > -				      to_intel_crtc(new_conn_state-
> > >crtc) : NULL;
> > -
> > -	return new_crtc != old_crtc ||
> > -	       (new_crtc &&
> > -		needs_modeset(intel_atomic_get_new_crtc_state(state,
> > new_crtc)));
> > -}
> > -
> >  static void intel_encoders_update_prepare(struct
> > intel_atomic_state *state)
> >  {
> > -	struct drm_connector_state *old_conn_state;
> > -	struct drm_connector_state *new_conn_state;
> > -	struct drm_connector *conn;
> > +	struct intel_digital_connector_state *new_connector_state;
> > +	struct intel_connector *connector;
> >  	int i;
> >  
> > -	for_each_oldnew_connector_in_state(&state->base, conn,
> > -					   old_conn_state,
> > new_conn_state, i) {
> > +	for_each_new_intel_connector_in_state(state, connector,
> > +					      new_connector_state, i) {
> >  		struct intel_encoder *encoder;
> >  		struct intel_crtc *crtc;
> >  
> > -		if (!intel_connector_needs_modeset(state,
> > -						   old_conn_state,
> > -						   new_conn_state))
> > +		if (!intel_digital_connector_needs_modeset(state,
> > connector))
> >  			continue;
> >  
> > -		encoder =
> > intel_connector_primary_encoder(to_intel_connector(conn));
> > +		encoder = intel_connector_primary_encoder(connector);
> >  		if (!encoder->update_prepare)
> >  			continue;
> >  
> > -		crtc = new_conn_state->crtc ?
> > -			to_intel_crtc(new_conn_state->crtc) : NULL;
> > +		crtc = new_connector_state->base.crtc ?
> > +			to_intel_crtc(new_connector_state->base.crtc) :
> > NULL;
> >  		encoder->update_prepare(state, encoder, crtc);
> >  	}
> >  }
> >  
> >  static void intel_encoders_update_complete(struct
> > intel_atomic_state *state)
> >  {
> > -	struct drm_connector_state *old_conn_state;
> > -	struct drm_connector_state *new_conn_state;
> > -	struct drm_connector *conn;
> > +	struct intel_digital_connector_state *new_connector_state;
> > +	struct intel_connector *connector;
> >  	int i;
> >  
> > -	for_each_oldnew_connector_in_state(&state->base, conn,
> > -					   old_conn_state,
> > new_conn_state, i) {
> > +	for_each_new_intel_connector_in_state(state, connector,
> > +					      new_connector_state, i) {
> >  		struct intel_encoder *encoder;
> >  		struct intel_crtc *crtc;
> >  
> > -		if (!intel_connector_needs_modeset(state,
> > -						   old_conn_state,
> > -						   new_conn_state))
> > +		if (!intel_digital_connector_needs_modeset(state,
> > connector))
> >  			continue;
> >  
> > -		encoder =
> > intel_connector_primary_encoder(to_intel_connector(conn));
> > +		encoder = intel_connector_primary_encoder(connector);
> >  		if (!encoder->update_complete)
> >  			continue;
> >  
> > -		crtc = new_conn_state->crtc ?
> > -			to_intel_crtc(new_conn_state->crtc) : NULL;
> > +		crtc = new_connector_state->base.crtc ?
> > +			to_intel_crtc(new_connector_state->base.crtc) :
> > NULL;
> >  		encoder->update_complete(state, encoder, crtc);
> >  	}
> >  }
> > -- 
> > 2.24.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 rebased 07/11] drm/i915/tgl: Select master transcoder for MST stream

[Souza, Jose]

On Thu, 2019-12-12 at 22:44 +0200, Ville Syrjälä wrote:
> On Wed, Dec 11, 2019 at 10:45:22AM -0800, José Roberto de Souza
> wrote:
> > On TGL the blending of all the streams have moved from DDI to
> > transcoder, so now every transcoder working over the same MST port
> > must
> > send its stream to a master transcoder and master will send to DDI
> > respecting the time slots.
> > 
> > So here adding all the CRTCs that shares the same MST stream if
> > needed and computing their state again, it will pick the lowest
> > pipe/transcoder among the ones in the same stream to be master.
> > 
> > Most of the time skl_commit_modeset_enables() enables pipes in a
> > crescent order but due DDB overlapping it might not happen, this
> > scenarios will be handled in the next patch.
> > 
> > v2:
> > - Using recently added intel_crtc_state_reset() to set
> > mst_master_transcoder to invalid transcoder for all non gen12 & MST
> > code paths
> > - Setting lowest pipe/transcoder as master, previously it was the
> > first one but setting a predictable one will help in future MST e
> > port sync integration
> > - Moving to intel type as much as we can
> > 
> > BSpec: 50493
> > BSpec: 49190
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_atomic.c   |  14 ++
> >  drivers/gpu/drm/i915/display/intel_atomic.h   |   3 +
> >  drivers/gpu/drm/i915/display/intel_ddi.c      |  14 +-
> >  drivers/gpu/drm/i915/display/intel_display.c  |  13 +-
> >  .../drm/i915/display/intel_display_types.h    |   3 +
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c   | 139
> > ++++++++++++++++--
> >  drivers/gpu/drm/i915/display/intel_dp_mst.h   |   5 +
> >  7 files changed, 178 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index 6e93a39a6fec..69a0430c4638 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -206,6 +206,20 @@ intel_digital_connector_needs_modeset(struct
> > intel_atomic_state *state,
> >  	       (new_crtc &&
> > drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi));
> >  }
> >  
> > +struct intel_digital_connector_state *
> > +intel_atomic_get_digital_connector_state(struct intel_atomic_state
> > *state,
> > +					 struct intel_connector
> > *connector)
> > +{
> > +	struct drm_connector_state *connector_state;
> > +
> > +	connector_state = drm_atomic_get_connector_state(&state->base,
> > +							 &connector-
> > >base);
> > +	if (IS_ERR(connector_state))
> > +		return ERR_CAST(connector_state);
> > +
> > +	return to_intel_digital_connector_state(connector_state);
> > +}
> > +
> >  /**
> >   * intel_crtc_duplicate_state - duplicate crtc state
> >   * @crtc: drm crtc
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h
> > b/drivers/gpu/drm/i915/display/intel_atomic.h
> > index ba9cc29a5865..6e8638d83d28 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> > @@ -35,6 +35,9 @@ struct drm_connector_state *
> >  intel_digital_connector_duplicate_state(struct drm_connector
> > *connector);
> >  bool intel_digital_connector_needs_modeset(struct
> > intel_atomic_state *state,
> >  					   struct intel_connector
> > *connector);
> > +struct intel_digital_connector_state *
> > +intel_atomic_get_digital_connector_state(struct intel_atomic_state
> > *state,
> > +					 struct intel_connector
> > *connector);
> >  
> >  struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc
> > *crtc);
> >  void intel_crtc_destroy_state(struct drm_crtc *crtc,
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 5b6f32517c75..6ee5230045eb 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -1903,8 +1903,13 @@ intel_ddi_transcoder_func_reg_val_get(const
> > struct intel_crtc_state *crtc_state)
> >  		temp |= TRANS_DDI_MODE_SELECT_DP_MST;
> >  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> >  
> > -		if (INTEL_GEN(dev_priv) >= 12)
> > -			temp |=
> > TRANS_DDI_MST_TRANSPORT_SELECT(crtc_state->cpu_transcoder);
> > +		if (INTEL_GEN(dev_priv) >= 12) {
> > +			enum transcoder master;
> > +
> > +			master = crtc_state->mst_master_transcoder;
> > +			WARN_ON(master == INVALID_TRANSCODER);
> > +			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master);
> > +		}
> >  	} else {
> >  		temp |= TRANS_DDI_MODE_SELECT_DP_SST;
> >  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> > @@ -4377,6 +4382,11 @@ void intel_ddi_get_config(struct
> > intel_encoder *encoder,
> >  		pipe_config->output_types |= BIT(INTEL_OUTPUT_DP_MST);
> >  		pipe_config->lane_count =
> >  			((temp & DDI_PORT_WIDTH_MASK) >>
> > DDI_PORT_WIDTH_SHIFT) + 1;
> > +
> > +		if (INTEL_GEN(dev_priv) >= 12)
> > +			pipe_config->mst_master_transcoder =
> > +					REG_FIELD_GET(TRANS_DDI_MST_TRA
> > NSPORT_SELECT_MASK, temp);
> > +
> >  		intel_dp_get_m_n(intel_crtc, pipe_config);
> >  		break;
> >  	default:
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 39b00a19d752..1cecce2f54f8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -46,6 +46,7 @@
> >  #include "display/intel_crt.h"
> >  #include "display/intel_ddi.h"
> >  #include "display/intel_dp.h"
> > +#include "display/intel_dp_mst.h"
> >  #include "display/intel_dsi.h"
> >  #include "display/intel_dvo.h"
> >  #include "display/intel_gmbus.h"
> > @@ -11663,6 +11664,7 @@ static void intel_crtc_state_reset(struct
> > intel_crtc_state *crtc_state,
> >  	crtc_state->hsw_workaround_pipe = INVALID_PIPE;
> >  	crtc_state->output_format = INTEL_OUTPUT_FORMAT_INVALID;
> >  	crtc_state->scaler_state.scaler_id = -1;
> > +	crtc_state->mst_master_transcoder = INVALID_TRANSCODER;
> >  }
> >  
> >  /* Returns the currently programmed mode of the given encoder. */
> > @@ -12510,6 +12512,9 @@ static void intel_dump_pipe_config(const
> > struct intel_crtc_state *pipe_config,
> >  			      pipe_config->csc_mode, pipe_config-
> > >gamma_mode,
> >  			      pipe_config->gamma_enable, pipe_config-
> > >csc_enable);
> >  
> > +	DRM_DEBUG_KMS("MST master transcoder: %s\n",
> > +		      transcoder_name(pipe_config-
> > >mst_master_transcoder));
> > +
> >  dump_planes:
> >  	if (!state)
> >  		return;
> > @@ -12651,6 +12656,7 @@ intel_crtc_prepare_cleared_state(struct
> > intel_crtc_state *crtc_state)
> >  	memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls,
> >  	       sizeof(saved_state->icl_port_dplls));
> >  	saved_state->crc_enabled = crtc_state->crc_enabled;
> > +	saved_state->mst_master_transcoder = INVALID_TRANSCODER;
> >  	if (IS_G4X(dev_priv) ||
> >  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >  		saved_state->wm = crtc_state->wm;
> > @@ -13290,6 +13296,8 @@ intel_pipe_config_compare(const struct
> > intel_crtc_state *current_config,
> >  	PIPE_CONF_CHECK_I(dsc.dsc_split);
> >  	PIPE_CONF_CHECK_I(dsc.compressed_bpp);
> >  
> > +	PIPE_CONF_CHECK_I(mst_master_transcoder);
> > +
> >  #undef PIPE_CONF_CHECK_X
> >  #undef PIPE_CONF_CHECK_I
> >  #undef PIPE_CONF_CHECK_BOOL
> > @@ -14374,7 +14382,7 @@ static void
> > intel_commit_modeset_disables(struct intel_atomic_state *state)
> >  	u32 handled = 0;
> >  	int i;
> >  
> > -	/* Only disable port sync slaves */
> > +	/* Only disable port sync and MST slaves */
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> >  					    new_crtc_state, i) {
> >  		if (!needs_modeset(new_crtc_state))
> > @@ -14388,7 +14396,8 @@ static void
> > intel_commit_modeset_disables(struct intel_atomic_state *state)
> >  		 * slave CRTCs are disabled first and then master CRTC
> > since
> >  		 * Slave vblanks are masked till Master Vblanks.
> >  		 */
> > -		if (!is_trans_port_sync_slave(old_crtc_state))
> > +		if (!is_trans_port_sync_slave(old_crtc_state) &&
> > +		    !intel_dp_mst_is_slave_trans(old_crtc_state))
> >  			continue;
> >  
> >  		intel_pre_plane_update(state, crtc);
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 83ea04149b77..630a94892b7b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1054,6 +1054,9 @@ struct intel_crtc_state {
> >  
> >  	/* Bitmask to indicate slaves attached */
> >  	u8 sync_mode_slaves_mask;
> > +
> > +	/* Only valid on TGL+ */
> > +	enum transcoder mst_master_transcoder;
> >  };
> >  
> >  struct intel_crtc {
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 926e49f449a6..12f5e799d91f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -87,6 +87,49 @@ static int
> > intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Iterate over all connectors and set mst_master_transcoder to
> > the smallest
> > + * transcoder id that shares the same MST connector.
> > + */
> > +static int
> > +intel_dp_mst_master_trans_compute(struct intel_encoder *encoder,
> > +				  struct intel_crtc_state *crtc_state,
> > +				  struct drm_connector_state
> > *connector_state)
> > +{
> > +	struct intel_atomic_state *state =
> > to_intel_atomic_state(crtc_state->uapi.state);
> > +	struct intel_connector *connector =
> > to_intel_connector(connector_state->connector);
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > +	struct intel_digital_connector_state *iter_connector_state;
> > +	struct intel_connector *iter_connector;
> > +	int i;
> > +
> > +	if (INTEL_GEN(dev_priv) < 12)
> > +		return 0;
> > +
> > +	crtc_state->mst_master_transcoder = I915_MAX_TRANSCODERS;
> > +
> > +	for_each_new_intel_connector_in_state(state, iter_connector,
> > +					      iter_connector_state, i)
> > {
> > +		struct intel_crtc_state *iter_crtc_state;
> > +		struct intel_crtc *iter_crtc;
> > +
> > +		if (connector->mst_port != iter_connector->mst_port ||
> > +		    !iter_connector_state->base.crtc)
> > +			continue;
> > +
> > +		iter_crtc = to_intel_crtc(iter_connector_state-
> > >base.crtc);
> > +		iter_crtc_state =
> > intel_atomic_get_new_crtc_state(state,
> > +								  iter_
> > crtc);
> > +		if (!iter_crtc_state->uapi.active)
> > +			continue;
> > +
> > +		if (iter_crtc_state->cpu_transcoder < crtc_state-
> > >mst_master_transcoder)
> > +			crtc_state->mst_master_transcoder =
> > iter_crtc_state->cpu_transcoder;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> This looks like it can be a pure function that returns the
> master transcoder.

okay

> 
> > +
> >  static int intel_dp_mst_compute_config(struct intel_encoder
> > *encoder,
> >  				       struct intel_crtc_state
> > *pipe_config,
> >  				       struct drm_connector_state
> > *conn_state)
> > @@ -154,24 +197,88 @@ static int intel_dp_mst_compute_config(struct
> > intel_encoder *encoder,
> >  
> >  	intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
> >  
> > +	ret = intel_dp_mst_master_trans_compute(encoder, pipe_config,
> > +						conn_state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * If one of the connectors in a MST stream needs a modeset, mark
> > all CRTCs
> > + * that have a connector in the same MST stream as mode changed,
> > + * intel_modeset_pipe_config()+intel_crtc_check_fastset() will
> > take to do a
> > + * fastset when possible.
> > + */
> > +static int
> > +intel_dp_mst_atomic_master_trans_check(struct intel_connector
> > *connector,
> > +				       struct intel_atomic_state
> > *state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	struct drm_connector_list_iter connector_list_iter;
> > +	struct intel_connector *connector_iter;
> > +
> > +	if (INTEL_GEN(dev_priv) < 12)
> > +		return  0;
> > +
> > +	if (!intel_digital_connector_needs_modeset(state, connector))
> > +		return 0;
> > +
> > +	drm_connector_list_iter_begin(&dev_priv->drm,
> > &connector_list_iter);
> > +	for_each_intel_connector_iter(connector_iter,
> > &connector_list_iter) {
> > +		struct intel_digital_connector_state
> > *connector_iter_state;
> > +		struct intel_crtc_state *crtc_state;
> > +		struct intel_crtc *crtc;
> > +
> > +		if (connector_iter->mst_port != connector->mst_port)
> > +			continue;
> 
> Do we need to exclude the connector itself? I guess not really?

Not really but I will do it.

> 
> > +
> > +		connector_iter_state =
> > +			intel_atomic_get_digital_connector_state(state,
> > +								 connec
> > tor_iter);
> > +		if (IS_ERR(connector_iter_state)) {
> > +			drm_connector_list_iter_end(&connector_list_ite
> > r);
> > +			return PTR_ERR(connector_iter_state);
> > +		}
> > +
> > +		if (!connector_iter_state->base.crtc)
> > +			continue;
> > +
> > +		crtc = to_intel_crtc(connector_iter_state->base.crtc);
> > +		crtc_state = intel_atomic_get_crtc_state(&state->base,
> > crtc);
> > +		if (IS_ERR(crtc_state)) {
> > +			drm_connector_list_iter_end(&connector_list_ite
> > r);
> > +			return PTR_ERR(crtc_state);
> > +		}
> > +
> > +		crtc_state->uapi.mode_changed = true;
> > +	}
> > +	drm_connector_list_iter_end(&connector_list_iter);
> > +
> >  	return 0;
> >  }
> >  
> >  static int
> >  intel_dp_mst_atomic_check(struct drm_connector *connector,
> > -			  struct drm_atomic_state *state)
> > +			  struct drm_atomic_state *_state)
> >  {
> > +	struct intel_atomic_state *state =
> > to_intel_atomic_state(_state);
> >  	struct drm_connector_state *new_conn_state =
> > -		drm_atomic_get_new_connector_state(state, connector);
> > +		drm_atomic_get_new_connector_state(&state->base,
> > connector);
> >  	struct drm_connector_state *old_conn_state =
> > -		drm_atomic_get_old_connector_state(state, connector);
> > +		drm_atomic_get_old_connector_state(&state->base,
> > connector);
> >  	struct intel_connector *intel_connector =
> >  		to_intel_connector(connector);
> >  	struct drm_crtc *new_crtc = new_conn_state->crtc;
> >  	struct drm_dp_mst_topology_mgr *mgr;
> >  	int ret;
> >  
> > -	ret = intel_digital_connector_atomic_check(connector, state);
> > +	ret = intel_digital_connector_atomic_check(connector, &state-
> > >base);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = intel_dp_mst_atomic_master_trans_check(intel_connector,
> > state);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -182,12 +289,9 @@ intel_dp_mst_atomic_check(struct drm_connector
> > *connector,
> >  	 * connector
> >  	 */
> >  	if (new_crtc) {
> > -		struct intel_atomic_state *intel_state =
> > -			to_intel_atomic_state(state);
> >  		struct intel_crtc *intel_crtc =
> > to_intel_crtc(new_crtc);
> >  		struct intel_crtc_state *crtc_state =
> > -			intel_atomic_get_new_crtc_state(intel_state,
> > -							intel_crtc);
> > +			intel_atomic_get_new_crtc_state(state,
> > intel_crtc);
> >  
> >  		if (!crtc_state ||
> >  		    !drm_atomic_crtc_needs_modeset(&crtc_state->uapi)
> > ||
> > @@ -196,7 +300,7 @@ intel_dp_mst_atomic_check(struct drm_connector
> > *connector,
> >  	}
> >  
> >  	mgr = &enc_to_mst(old_conn_state->best_encoder)->primary-
> > >dp.mst_mgr;
> > -	ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
> > +	ret = drm_dp_atomic_release_vcpi_slots(&state->base, mgr,
> >  					       intel_connector->port);
> >  
> >  	return ret;
> > @@ -241,6 +345,9 @@ static void intel_mst_post_disable_dp(struct
> > intel_encoder *encoder,
> >  	intel_dp->active_mst_links--;
> >  	last_mst_stream = intel_dp->active_mst_links == 0;
> >  
> > +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream &&
> > +		!intel_dp_mst_is_master_trans(old_crtc_state));
> > +
> >  	/*
> >  	 * From TGL spec: "If multi-stream slave transcoder: Configure
> >  	 * Transcoder Clock Select to direct no clock to the
> > transcoder"
> > @@ -321,6 +428,9 @@ static void intel_mst_pre_enable_dp(struct
> > intel_encoder *encoder,
> >  	intel_mst->connector = connector;
> >  	first_mst_stream = intel_dp->active_mst_links == 0;
> >  
> > +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && first_mst_stream &&
> > +		!intel_dp_mst_is_master_trans(pipe_config));
> > +
> >  	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
> >  
> >  	if (first_mst_stream)
> > @@ -726,3 +836,14 @@ intel_dp_mst_encoder_cleanup(struct
> > intel_digital_port *intel_dig_port)
> >  	drm_dp_mst_topology_mgr_destroy(&intel_dp->mst_mgr);
> >  	/* encoders will get killed by normal cleanup */
> >  }
> > +
> > +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state
> > *crtc_state)
> > +{
> > +	return crtc_state->mst_master_transcoder == crtc_state-
> > >cpu_transcoder;
> > +}
> > +
> > +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state
> > *crtc_state)
> > +{
> > +	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER
> > &&
> > +	       crtc_state->mst_master_transcoder != crtc_state-
> > >cpu_transcoder;
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > index f660ad80db04..e40767f78343 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > @@ -6,10 +6,15 @@
> >  #ifndef __INTEL_DP_MST_H__
> >  #define __INTEL_DP_MST_H__
> >  
> > +#include <stdbool.h>
> 
> That doesn't seem like something you want to include in kernel code.

Thanks for the headsup, will replace it by #include <linux/types.h>

> 
> > +
> >  struct intel_digital_port;
> > +struct intel_crtc_state;
> >  
> >  int intel_dp_mst_encoder_init(struct intel_digital_port
> > *intel_dig_port, int conn_id);
> >  void intel_dp_mst_encoder_cleanup(struct intel_digital_port
> > *intel_dig_port);
> >  int intel_dp_mst_encoder_active_links(struct intel_digital_port
> > *intel_dig_port);
> > +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state
> > *crtc_state);
> > +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state
> > *crtc_state);
> >  
> >  #endif /* __INTEL_DP_MST_H__ */
> > -- 
> > 2.24.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 rebased 07/11] drm/i915/tgl: Select master transcoder for MST stream

[Souza, Jose]

On Fri, 2019-12-13 at 22:56 +0200, Ville Syrjälä wrote:
> On Thu, Dec 12, 2019 at 10:44:29PM +0200, Ville Syrjälä wrote:
> > On Wed, Dec 11, 2019 at 10:45:22AM -0800, José Roberto de Souza
> > wrote:
> > > On TGL the blending of all the streams have moved from DDI to
> > > transcoder, so now every transcoder working over the same MST
> > > port must
> > > send its stream to a master transcoder and master will send to
> > > DDI
> > > respecting the time slots.
> > > 
> > > So here adding all the CRTCs that shares the same MST stream if
> > > needed and computing their state again, it will pick the lowest
> > > pipe/transcoder among the ones in the same stream to be master.
> > > 
> > > Most of the time skl_commit_modeset_enables() enables pipes in a
> > > crescent order but due DDB overlapping it might not happen, this
> > > scenarios will be handled in the next patch.
> > > 
> > > v2:
> > > - Using recently added intel_crtc_state_reset() to set
> > > mst_master_transcoder to invalid transcoder for all non gen12 &
> > > MST
> > > code paths
> > > - Setting lowest pipe/transcoder as master, previously it was the
> > > first one but setting a predictable one will help in future MST e
> > > port sync integration
> > > - Moving to intel type as much as we can
> > > 
> > > BSpec: 50493
> > > BSpec: 49190
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_atomic.c   |  14 ++
> > >  drivers/gpu/drm/i915/display/intel_atomic.h   |   3 +
> > >  drivers/gpu/drm/i915/display/intel_ddi.c      |  14 +-
> > >  drivers/gpu/drm/i915/display/intel_display.c  |  13 +-
> > >  .../drm/i915/display/intel_display_types.h    |   3 +
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c   | 139
> > > ++++++++++++++++--
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.h   |   5 +
> > >  7 files changed, 178 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > index 6e93a39a6fec..69a0430c4638 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > @@ -206,6 +206,20 @@ intel_digital_connector_needs_modeset(struct
> > > intel_atomic_state *state,
> > >  	       (new_crtc &&
> > > drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi));
> > >  }
> > >  
> > > +struct intel_digital_connector_state *
> > > +intel_atomic_get_digital_connector_state(struct
> > > intel_atomic_state *state,
> > > +					 struct intel_connector
> > > *connector)
> > > +{
> > > +	struct drm_connector_state *connector_state;
> > > +
> > > +	connector_state = drm_atomic_get_connector_state(&state->base,
> > > +							 &connector-
> > > >base);
> > > +	if (IS_ERR(connector_state))
> > > +		return ERR_CAST(connector_state);
> > > +
> > > +	return to_intel_digital_connector_state(connector_state);
> > > +}
> > > +
> > >  /**
> > >   * intel_crtc_duplicate_state - duplicate crtc state
> > >   * @crtc: drm crtc
> > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h
> > > b/drivers/gpu/drm/i915/display/intel_atomic.h
> > > index ba9cc29a5865..6e8638d83d28 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> > > @@ -35,6 +35,9 @@ struct drm_connector_state *
> > >  intel_digital_connector_duplicate_state(struct drm_connector
> > > *connector);
> > >  bool intel_digital_connector_needs_modeset(struct
> > > intel_atomic_state *state,
> > >  					   struct intel_connector
> > > *connector);
> > > +struct intel_digital_connector_state *
> > > +intel_atomic_get_digital_connector_state(struct
> > > intel_atomic_state *state,
> > > +					 struct intel_connector
> > > *connector);
> > >  
> > >  struct drm_crtc_state *intel_crtc_duplicate_state(struct
> > > drm_crtc *crtc);
> > >  void intel_crtc_destroy_state(struct drm_crtc *crtc,
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index 5b6f32517c75..6ee5230045eb 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -1903,8 +1903,13 @@
> > > intel_ddi_transcoder_func_reg_val_get(const struct
> > > intel_crtc_state *crtc_state)
> > >  		temp |= TRANS_DDI_MODE_SELECT_DP_MST;
> > >  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> > >  
> > > -		if (INTEL_GEN(dev_priv) >= 12)
> > > -			temp |=
> > > TRANS_DDI_MST_TRANSPORT_SELECT(crtc_state->cpu_transcoder);
> > > +		if (INTEL_GEN(dev_priv) >= 12) {
> > > +			enum transcoder master;
> > > +
> > > +			master = crtc_state->mst_master_transcoder;
> > > +			WARN_ON(master == INVALID_TRANSCODER);
> > > +			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master);
> > > +		}
> > >  	} else {
> > >  		temp |= TRANS_DDI_MODE_SELECT_DP_SST;
> > >  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> > > @@ -4377,6 +4382,11 @@ void intel_ddi_get_config(struct
> > > intel_encoder *encoder,
> > >  		pipe_config->output_types |= BIT(INTEL_OUTPUT_DP_MST);
> > >  		pipe_config->lane_count =
> > >  			((temp & DDI_PORT_WIDTH_MASK) >>
> > > DDI_PORT_WIDTH_SHIFT) + 1;
> > > +
> > > +		if (INTEL_GEN(dev_priv) >= 12)
> > > +			pipe_config->mst_master_transcoder =
> > > +					REG_FIELD_GET(TRANS_DDI_MST_TRA
> > > NSPORT_SELECT_MASK, temp);
> > > +
> > >  		intel_dp_get_m_n(intel_crtc, pipe_config);
> > >  		break;
> > >  	default:
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 39b00a19d752..1cecce2f54f8 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -46,6 +46,7 @@
> > >  #include "display/intel_crt.h"
> > >  #include "display/intel_ddi.h"
> > >  #include "display/intel_dp.h"
> > > +#include "display/intel_dp_mst.h"
> > >  #include "display/intel_dsi.h"
> > >  #include "display/intel_dvo.h"
> > >  #include "display/intel_gmbus.h"
> > > @@ -11663,6 +11664,7 @@ static void intel_crtc_state_reset(struct
> > > intel_crtc_state *crtc_state,
> > >  	crtc_state->hsw_workaround_pipe = INVALID_PIPE;
> > >  	crtc_state->output_format = INTEL_OUTPUT_FORMAT_INVALID;
> > >  	crtc_state->scaler_state.scaler_id = -1;
> > > +	crtc_state->mst_master_transcoder = INVALID_TRANSCODER;
> > >  }
> > >  
> > >  /* Returns the currently programmed mode of the given encoder.
> > > */
> > > @@ -12510,6 +12512,9 @@ static void intel_dump_pipe_config(const
> > > struct intel_crtc_state *pipe_config,
> > >  			      pipe_config->csc_mode, pipe_config-
> > > >gamma_mode,
> > >  			      pipe_config->gamma_enable, pipe_config-
> > > >csc_enable);
> > >  
> > > +	DRM_DEBUG_KMS("MST master transcoder: %s\n",
> > > +		      transcoder_name(pipe_config-
> > > >mst_master_transcoder));
> > > +
> > >  dump_planes:
> > >  	if (!state)
> > >  		return;
> > > @@ -12651,6 +12656,7 @@ intel_crtc_prepare_cleared_state(struct
> > > intel_crtc_state *crtc_state)
> > >  	memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls,
> > >  	       sizeof(saved_state->icl_port_dplls));
> > >  	saved_state->crc_enabled = crtc_state->crc_enabled;
> > > +	saved_state->mst_master_transcoder = INVALID_TRANSCODER;
> > >  	if (IS_G4X(dev_priv) ||
> > >  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > >  		saved_state->wm = crtc_state->wm;
> > > @@ -13290,6 +13296,8 @@ intel_pipe_config_compare(const struct
> > > intel_crtc_state *current_config,
> > >  	PIPE_CONF_CHECK_I(dsc.dsc_split);
> > >  	PIPE_CONF_CHECK_I(dsc.compressed_bpp);
> > >  
> > > +	PIPE_CONF_CHECK_I(mst_master_transcoder);
> > > +
> > >  #undef PIPE_CONF_CHECK_X
> > >  #undef PIPE_CONF_CHECK_I
> > >  #undef PIPE_CONF_CHECK_BOOL
> > > @@ -14374,7 +14382,7 @@ static void
> > > intel_commit_modeset_disables(struct intel_atomic_state *state)
> > >  	u32 handled = 0;
> > >  	int i;
> > >  
> > > -	/* Only disable port sync slaves */
> > > +	/* Only disable port sync and MST slaves */
> > >  	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > old_crtc_state,
> > >  					    new_crtc_state, i) {
> > >  		if (!needs_modeset(new_crtc_state))
> > > @@ -14388,7 +14396,8 @@ static void
> > > intel_commit_modeset_disables(struct intel_atomic_state *state)
> > >  		 * slave CRTCs are disabled first and then master CRTC
> > > since
> > >  		 * Slave vblanks are masked till Master Vblanks.
> > >  		 */
> > > -		if (!is_trans_port_sync_slave(old_crtc_state))
> > > +		if (!is_trans_port_sync_slave(old_crtc_state) &&
> > > +		    !intel_dp_mst_is_slave_trans(old_crtc_state))
> > >  			continue;
> > >  
> > >  		intel_pre_plane_update(state, crtc);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 83ea04149b77..630a94892b7b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -1054,6 +1054,9 @@ struct intel_crtc_state {
> > >  
> > >  	/* Bitmask to indicate slaves attached */
> > >  	u8 sync_mode_slaves_mask;
> > > +
> > > +	/* Only valid on TGL+ */
> > > +	enum transcoder mst_master_transcoder;
> > >  };
> > >  
> > >  struct intel_crtc {
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > index 926e49f449a6..12f5e799d91f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > @@ -87,6 +87,49 @@ static int
> > > intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
> > >  	return 0;
> > >  }
> > >  
> > > +/*
> > > + * Iterate over all connectors and set mst_master_transcoder to
> > > the smallest
> > > + * transcoder id that shares the same MST connector.
> > > + */
> > > +static int
> > > +intel_dp_mst_master_trans_compute(struct intel_encoder *encoder,
> > > +				  struct intel_crtc_state *crtc_state,
> > > +				  struct drm_connector_state
> > > *connector_state)
> > > +{
> > > +	struct intel_atomic_state *state =
> > > to_intel_atomic_state(crtc_state->uapi.state);
> > > +	struct intel_connector *connector =
> > > to_intel_connector(connector_state->connector);
> > > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > +	struct intel_digital_connector_state *iter_connector_state;
> > > +	struct intel_connector *iter_connector;
> > > +	int i;
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 12)
> > > +		return 0;
> > > +
> > > +	crtc_state->mst_master_transcoder = I915_MAX_TRANSCODERS;
> > > +
> > > +	for_each_new_intel_connector_in_state(state, iter_connector,
> > > +					      iter_connector_state, i)
> > > {
> > > +		struct intel_crtc_state *iter_crtc_state;
> > > +		struct intel_crtc *iter_crtc;
> > > +
> > > +		if (connector->mst_port != iter_connector->mst_port ||
> > > +		    !iter_connector_state->base.crtc)
> > > +			continue;
> > > +
> > > +		iter_crtc = to_intel_crtc(iter_connector_state-
> > > >base.crtc);
> > > +		iter_crtc_state =
> > > intel_atomic_get_new_crtc_state(state,
> > > +								  iter_
> > > crtc);
> > > +		if (!iter_crtc_state->uapi.active)
> > > +			continue;
> > > +
> > > +		if (iter_crtc_state->cpu_transcoder < crtc_state-
> > > >mst_master_transcoder)
> > > +			crtc_state->mst_master_transcoder =
> > > iter_crtc_state->cpu_transcoder;
> 
> Was just reading Manasi's port sync stuff when I realized this won't
> work. We haven't necessarily compute cpu_transcoder for the other
> pipes yet. So I suggest we extract the code to compute the
> cpu_transcoder
> from intel_ddi_connector_get_hw_state() and use it everywhere,
> including
> here.

okay

> 
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > 
> > This looks like it can be a pure function that returns the
> > master transcoder.
> > 
> > > +
> > >  static int intel_dp_mst_compute_config(struct intel_encoder
> > > *encoder,
> > >  				       struct intel_crtc_state
> > > *pipe_config,
> > >  				       struct drm_connector_state
> > > *conn_state)
> > > @@ -154,24 +197,88 @@ static int
> > > intel_dp_mst_compute_config(struct intel_encoder *encoder,
> > >  
> > >  	intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
> > >  
> > > +	ret = intel_dp_mst_master_trans_compute(encoder, pipe_config,
> > > +						conn_state);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/*
> > > + * If one of the connectors in a MST stream needs a modeset,
> > > mark all CRTCs
> > > + * that have a connector in the same MST stream as mode changed,
> > > + * intel_modeset_pipe_config()+intel_crtc_check_fastset() will
> > > take to do a
> > > + * fastset when possible.
> > > + */
> > > +static int
> > > +intel_dp_mst_atomic_master_trans_check(struct intel_connector
> > > *connector,
> > > +				       struct intel_atomic_state
> > > *state)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > +	struct drm_connector_list_iter connector_list_iter;
> > > +	struct intel_connector *connector_iter;
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 12)
> > > +		return  0;
> > > +
> > > +	if (!intel_digital_connector_needs_modeset(state, connector))
> > > +		return 0;
> > > +
> > > +	drm_connector_list_iter_begin(&dev_priv->drm,
> > > &connector_list_iter);
> > > +	for_each_intel_connector_iter(connector_iter,
> > > &connector_list_iter) {
> > > +		struct intel_digital_connector_state
> > > *connector_iter_state;
> > > +		struct intel_crtc_state *crtc_state;
> > > +		struct intel_crtc *crtc;
> > > +
> > > +		if (connector_iter->mst_port != connector->mst_port)
> > > +			continue;
> > 
> > Do we need to exclude the connector itself? I guess not really?
> > 
> > > +
> > > +		connector_iter_state =
> > > +			intel_atomic_get_digital_connector_state(state,
> > > +								 connec
> > > tor_iter);
> > > +		if (IS_ERR(connector_iter_state)) {
> > > +			drm_connector_list_iter_end(&connector_list_ite
> > > r);
> > > +			return PTR_ERR(connector_iter_state);
> > > +		}
> > > +
> > > +		if (!connector_iter_state->base.crtc)
> > > +			continue;
> > > +
> > > +		crtc = to_intel_crtc(connector_iter_state->base.crtc);
> > > +		crtc_state = intel_atomic_get_crtc_state(&state->base,
> > > crtc);
> > > +		if (IS_ERR(crtc_state)) {
> > > +			drm_connector_list_iter_end(&connector_list_ite
> > > r);
> > > +			return PTR_ERR(crtc_state);
> > > +		}
> > > +
> > > +		crtc_state->uapi.mode_changed = true;
> > > +	}
> > > +	drm_connector_list_iter_end(&connector_list_iter);
> > > +
> > >  	return 0;
> > >  }
> > >  
> > >  static int
> > >  intel_dp_mst_atomic_check(struct drm_connector *connector,
> > > -			  struct drm_atomic_state *state)
> > > +			  struct drm_atomic_state *_state)
> > >  {
> > > +	struct intel_atomic_state *state =
> > > to_intel_atomic_state(_state);
> > >  	struct drm_connector_state *new_conn_state =
> > > -		drm_atomic_get_new_connector_state(state, connector);
> > > +		drm_atomic_get_new_connector_state(&state->base,
> > > connector);
> > >  	struct drm_connector_state *old_conn_state =
> > > -		drm_atomic_get_old_connector_state(state, connector);
> > > +		drm_atomic_get_old_connector_state(&state->base,
> > > connector);
> > >  	struct intel_connector *intel_connector =
> > >  		to_intel_connector(connector);
> > >  	struct drm_crtc *new_crtc = new_conn_state->crtc;
> > >  	struct drm_dp_mst_topology_mgr *mgr;
> > >  	int ret;
> > >  
> > > -	ret = intel_digital_connector_atomic_check(connector, state);
> > > +	ret = intel_digital_connector_atomic_check(connector, &state-
> > > >base);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = intel_dp_mst_atomic_master_trans_check(intel_connector,
> > > state);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > @@ -182,12 +289,9 @@ intel_dp_mst_atomic_check(struct
> > > drm_connector *connector,
> > >  	 * connector
> > >  	 */
> > >  	if (new_crtc) {
> > > -		struct intel_atomic_state *intel_state =
> > > -			to_intel_atomic_state(state);
> > >  		struct intel_crtc *intel_crtc =
> > > to_intel_crtc(new_crtc);
> > >  		struct intel_crtc_state *crtc_state =
> > > -			intel_atomic_get_new_crtc_state(intel_state,
> > > -							intel_crtc);
> > > +			intel_atomic_get_new_crtc_state(state,
> > > intel_crtc);
> > >  
> > >  		if (!crtc_state ||
> > >  		    !drm_atomic_crtc_needs_modeset(&crtc_state->uapi)
> > > ||
> > > @@ -196,7 +300,7 @@ intel_dp_mst_atomic_check(struct
> > > drm_connector *connector,
> > >  	}
> > >  
> > >  	mgr = &enc_to_mst(old_conn_state->best_encoder)->primary-
> > > >dp.mst_mgr;
> > > -	ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
> > > +	ret = drm_dp_atomic_release_vcpi_slots(&state->base, mgr,
> > >  					       intel_connector->port);
> > >  
> > >  	return ret;
> > > @@ -241,6 +345,9 @@ static void intel_mst_post_disable_dp(struct
> > > intel_encoder *encoder,
> > >  	intel_dp->active_mst_links--;
> > >  	last_mst_stream = intel_dp->active_mst_links == 0;
> > >  
> > > +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream &&
> > > +		!intel_dp_mst_is_master_trans(old_crtc_state));
> > > +
> > >  	/*
> > >  	 * From TGL spec: "If multi-stream slave transcoder: Configure
> > >  	 * Transcoder Clock Select to direct no clock to the
> > > transcoder"
> > > @@ -321,6 +428,9 @@ static void intel_mst_pre_enable_dp(struct
> > > intel_encoder *encoder,
> > >  	intel_mst->connector = connector;
> > >  	first_mst_stream = intel_dp->active_mst_links == 0;
> > >  
> > > +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && first_mst_stream &&
> > > +		!intel_dp_mst_is_master_trans(pipe_config));
> > > +
> > >  	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
> > >  
> > >  	if (first_mst_stream)
> > > @@ -726,3 +836,14 @@ intel_dp_mst_encoder_cleanup(struct
> > > intel_digital_port *intel_dig_port)
> > >  	drm_dp_mst_topology_mgr_destroy(&intel_dp->mst_mgr);
> > >  	/* encoders will get killed by normal cleanup */
> > >  }
> > > +
> > > +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state
> > > *crtc_state)
> > > +{
> > > +	return crtc_state->mst_master_transcoder == crtc_state-
> > > >cpu_transcoder;
> > > +}
> > > +
> > > +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state
> > > *crtc_state)
> > > +{
> > > +	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER
> > > &&
> > > +	       crtc_state->mst_master_transcoder != crtc_state-
> > > >cpu_transcoder;
> > > +}
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > index f660ad80db04..e40767f78343 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > @@ -6,10 +6,15 @@
> > >  #ifndef __INTEL_DP_MST_H__
> > >  #define __INTEL_DP_MST_H__
> > >  
> > > +#include <stdbool.h>
> > 
> > That doesn't seem like something you want to include in kernel
> > code.
> > 
> > > +
> > >  struct intel_digital_port;
> > > +struct intel_crtc_state;
> > >  
> > >  int intel_dp_mst_encoder_init(struct intel_digital_port
> > > *intel_dig_port, int conn_id);
> > >  void intel_dp_mst_encoder_cleanup(struct intel_digital_port
> > > *intel_dig_port);
> > >  int intel_dp_mst_encoder_active_links(struct intel_digital_port
> > > *intel_dig_port);
> > > +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state
> > > *crtc_state);
> > > +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state
> > > *crtc_state);
> > >  
> > >  #endif /* __INTEL_DP_MST_H__ */
> > > -- 
> > > 2.24.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 rebased 10/11] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Souza, Jose]

On Thu, 2019-12-12 at 23:28 +0200, Ville Syrjälä wrote:
> On Wed, Dec 11, 2019 at 10:45:25AM -0800, José Roberto de Souza
> wrote:
> > Check if fastset is allowed by external dependencies like other
> > pipes
> > and transcoders.
> > 
> > Right now it only forces a fullmodeset when the MST master
> > transcoder
> > did not changed but the pipe of the master transcoder needs a
> > fullmodeset so all slaves also needs to do a fullmodeset.
> > But it will probably be need for port sync as well.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 41
> > ++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 092412b10d7c..0c24d7dfa152 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -13930,11 +13930,52 @@ static int calc_watermark_data(struct
> > intel_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > +/**
> > + * Check if fastset is allowed by external dependencies like other
> > pipes and
> > + * transcoders.
> > + *
> > + * Right now it only forces a fullmodeset when the MST master
> > transcoder did
> > + * not changed but the pipe of the master transcoder needs a
> > fullmodeset so
> > + * all slaves also needs to do a fullmodeset.
> > + */
> > +static bool
> > +intel_crtc_check_external_dependencies_fastset(const struct
> > intel_crtc_state *old_crtc_state,
> > +					       struct intel_crtc_state
> > *new_crtc_state)
> > +{
> > +	struct intel_atomic_state *state =
> > to_intel_atomic_state(new_crtc_state->uapi.state);
> > +	struct drm_i915_private *dev_priv = to_i915(new_crtc_state-
> > >uapi.crtc->dev);
> > +	struct intel_crtc_state *new_crtc_state_iter;
> > +	struct intel_crtc *crtc_iter;
> > +	int i;
> > +
> > +	if (INTEL_GEN(dev_priv) < 12)
> > +		return true;
> > +
> > +	if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)
> > ||
> > +	    intel_dp_mst_is_master_trans(new_crtc_state))
> > +		return true;
> > +
> > +	for_each_new_intel_crtc_in_state(state, crtc_iter,
> > new_crtc_state_iter, i) {
> > +		if (new_crtc_state_iter->cpu_transcoder !=
> > +		    new_crtc_state->mst_master_transcoder)
> > +			continue;
> > +
> > +		return !needs_modeset(new_crtc_state_iter);
> > +	}
> > +
> > +	DRM_ERROR("Master MST transcoder of pipe not found\n");
> > +	return false;
> > +}
> > +
> >  static void intel_crtc_check_fastset(const struct intel_crtc_state
> > *old_crtc_state,
> >  				     struct intel_crtc_state
> > *new_crtc_state)
> >  {
> >  	if (!intel_pipe_config_compare(old_crtc_state, new_crtc_state,
> > true))
> >  		return;
> > +	if
> > (!intel_crtc_check_external_dependencies_fastset(old_crtc_state,
> > +							    new_crtc_st
> > ate))
> > +		return;
> 
> I don't think this will work. We've not yet .compute_config()'d
> everything so the master assignments are still up in the air.

As the MST master is the smallest pipe/transcoder id it is computed
first, so it works for MST but that is not the port sync case.
Will do a suggested bellow to already support port sync.


> 
> I think we need the logic higher up in intel_atomic_check():
> 
> for_each_crtc()
> 	compute_config()
> 	if (can_fastset())
> 		needs_modeset=false
> 		update_pipe=true;
> 	}
> }
> 
> for_each_crtc()
> 	if (mst_slave && master.needs_modeset() {
> 		needs_modeset=true;
> 		update_pipe=false;
> 	}
> 
> for_each_crtc()
> 	if (update_pipe)
> 		copy_over_old_state();
> 
> 	
> + we should probably rename/split update_pipes or add some
> lkind of needs_fastset() wrapper to make this less confusing.
> 
> 
> > +
> >  
> >  	new_crtc_state->uapi.mode_changed = false;
> >  	new_crtc_state->update_pipe = true;
> > -- 
> > 2.24.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 rebased 10/11] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Souza, Jose]

On Thu, 2019-12-12 at 13:41 -0800, Manasi Navare wrote:
> On Thu, Dec 12, 2019 at 11:28:30PM +0200, Ville Syrjälä wrote:
> > On Wed, Dec 11, 2019 at 10:45:25AM -0800, José Roberto de Souza
> > wrote:
> > > Check if fastset is allowed by external dependencies like other
> > > pipes
> > > and transcoders.
> > > 
> > > Right now it only forces a fullmodeset when the MST master
> > > transcoder
> > > did not changed but the pipe of the master transcoder needs a
> > > fullmodeset so all slaves also needs to do a fullmodeset.
> > > But it will probably be need for port sync as well.
> 
> But in case of port sync, now I have this function in
> intel_atomic_check()
> that will force a full modeset on all the connectors with has_tile if
> one of
> the tiled connectors needs a full modeset.
> So we wouldnt need to worry about fastset happening for one of the
> tiles in port sync.
> 
> This patch:
> https://patchwork.freedesktop.org/patch/344766/?series=70788&rev=1

This is not enough, test it or read the context of
intel_crtc_check_fastset()

> 
> Manasi
> 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 41
> > > ++++++++++++++++++++
> > >  1 file changed, 41 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 092412b10d7c..0c24d7dfa152 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -13930,11 +13930,52 @@ static int calc_watermark_data(struct
> > > intel_atomic_state *state)
> > >  	return 0;
> > >  }
> > >  
> > > +/**
> > > + * Check if fastset is allowed by external dependencies like
> > > other pipes and
> > > + * transcoders.
> > > + *
> > > + * Right now it only forces a fullmodeset when the MST master
> > > transcoder did
> > > + * not changed but the pipe of the master transcoder needs a
> > > fullmodeset so
> > > + * all slaves also needs to do a fullmodeset.
> > > + */
> > > +static bool
> > > +intel_crtc_check_external_dependencies_fastset(const struct
> > > intel_crtc_state *old_crtc_state,
> > > +					       struct intel_crtc_state
> > > *new_crtc_state)
> > > +{
> > > +	struct intel_atomic_state *state =
> > > to_intel_atomic_state(new_crtc_state->uapi.state);
> > > +	struct drm_i915_private *dev_priv = to_i915(new_crtc_state-
> > > >uapi.crtc->dev);
> > > +	struct intel_crtc_state *new_crtc_state_iter;
> > > +	struct intel_crtc *crtc_iter;
> > > +	int i;
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 12)
> > > +		return true;
> > > +
> > > +	if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)
> > > ||
> > > +	    intel_dp_mst_is_master_trans(new_crtc_state))
> > > +		return true;
> > > +
> > > +	for_each_new_intel_crtc_in_state(state, crtc_iter,
> > > new_crtc_state_iter, i) {
> > > +		if (new_crtc_state_iter->cpu_transcoder !=
> > > +		    new_crtc_state->mst_master_transcoder)
> > > +			continue;
> > > +
> > > +		return !needs_modeset(new_crtc_state_iter);
> > > +	}
> > > +
> > > +	DRM_ERROR("Master MST transcoder of pipe not found\n");
> > > +	return false;
> > > +}
> > > +
> > >  static void intel_crtc_check_fastset(const struct
> > > intel_crtc_state *old_crtc_state,
> > >  				     struct intel_crtc_state
> > > *new_crtc_state)
> > >  {
> > >  	if (!intel_pipe_config_compare(old_crtc_state, new_crtc_state,
> > > true))
> > >  		return;
> > > +	if
> > > (!intel_crtc_check_external_dependencies_fastset(old_crtc_state,
> > > +							    new_crtc_st
> > > ate))
> > > +		return;
> > 
> > I don't think this will work. We've not yet .compute_config()'d
> > everything so the master assignments are still up in the air.
> > 
> > I think we need the logic higher up in intel_atomic_check():
> > 
> > for_each_crtc()
> > 	compute_config()
> > 	if (can_fastset())
> > 		needs_modeset=false
> > 		update_pipe=true;
> > 	}
> > }
> > 
> > for_each_crtc()
> > 	if (mst_slave && master.needs_modeset() {
> > 		needs_modeset=true;
> > 		update_pipe=false;
> > 	}
> > 
> > for_each_crtc()
> > 	if (update_pipe)
> > 		copy_over_old_state();
> > 
> > 	
> > + we should probably rename/split update_pipes or add some
> > lkind of needs_fastset() wrapper to make this less confusing.
> > 
> > 
> > > +
> > >  
> > >  	new_crtc_state->uapi.mode_changed = false;
> > >  	new_crtc_state->update_pipe = true;
> > > -- 
> > > 2.24.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 rebased 07/11] drm/i915/tgl: Select master transcoder for MST stream

[Souza, Jose]

On Mon, 2019-12-16 at 09:23 -0800, José Roberto de Souza wrote:
> On Fri, 2019-12-13 at 22:56 +0200, Ville Syrjälä wrote:
> > On Thu, Dec 12, 2019 at 10:44:29PM +0200, Ville Syrjälä wrote:
> > > On Wed, Dec 11, 2019 at 10:45:22AM -0800, José Roberto de Souza
> > > wrote:
> > > > On TGL the blending of all the streams have moved from DDI to
> > > > transcoder, so now every transcoder working over the same MST
> > > > port must
> > > > send its stream to a master transcoder and master will send to
> > > > DDI
> > > > respecting the time slots.
> > > > 
> > > > So here adding all the CRTCs that shares the same MST stream if
> > > > needed and computing their state again, it will pick the lowest
> > > > pipe/transcoder among the ones in the same stream to be master.
> > > > 
> > > > Most of the time skl_commit_modeset_enables() enables pipes in
> > > > a
> > > > crescent order but due DDB overlapping it might not happen,
> > > > this
> > > > scenarios will be handled in the next patch.
> > > > 
> > > > v2:
> > > > - Using recently added intel_crtc_state_reset() to set
> > > > mst_master_transcoder to invalid transcoder for all non gen12 &
> > > > MST
> > > > code paths
> > > > - Setting lowest pipe/transcoder as master, previously it was
> > > > the
> > > > first one but setting a predictable one will help in future MST
> > > > e
> > > > port sync integration
> > > > - Moving to intel type as much as we can
> > > > 
> > > > BSpec: 50493
> > > > BSpec: 49190
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_atomic.c   |  14 ++
> > > >  drivers/gpu/drm/i915/display/intel_atomic.h   |   3 +
> > > >  drivers/gpu/drm/i915/display/intel_ddi.c      |  14 +-
> > > >  drivers/gpu/drm/i915/display/intel_display.c  |  13 +-
> > > >  .../drm/i915/display/intel_display_types.h    |   3 +
> > > >  drivers/gpu/drm/i915/display/intel_dp_mst.c   | 139
> > > > ++++++++++++++++--
> > > >  drivers/gpu/drm/i915/display/intel_dp_mst.h   |   5 +
> > > >  7 files changed, 178 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > index 6e93a39a6fec..69a0430c4638 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > @@ -206,6 +206,20 @@
> > > > intel_digital_connector_needs_modeset(struct
> > > > intel_atomic_state *state,
> > > >  	       (new_crtc &&
> > > > drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi));
> > > >  }
> > > >  
> > > > +struct intel_digital_connector_state *
> > > > +intel_atomic_get_digital_connector_state(struct
> > > > intel_atomic_state *state,
> > > > +					 struct intel_connector
> > > > *connector)
> > > > +{
> > > > +	struct drm_connector_state *connector_state;
> > > > +
> > > > +	connector_state =
> > > > drm_atomic_get_connector_state(&state->base,
> > > > +							 &conne
> > > > ctor-
> > > > > base);
> > > > +	if (IS_ERR(connector_state))
> > > > +		return ERR_CAST(connector_state);
> > > > +
> > > > +	return
> > > > to_intel_digital_connector_state(connector_state);
> > > > +}
> > > > +
> > > >  /**
> > > >   * intel_crtc_duplicate_state - duplicate crtc state
> > > >   * @crtc: drm crtc
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h
> > > > b/drivers/gpu/drm/i915/display/intel_atomic.h
> > > > index ba9cc29a5865..6e8638d83d28 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> > > > @@ -35,6 +35,9 @@ struct drm_connector_state *
> > > >  intel_digital_connector_duplicate_state(struct drm_connector
> > > > *connector);
> > > >  bool intel_digital_connector_needs_modeset(struct
> > > > intel_atomic_state *state,
> > > >  					   struct
> > > > intel_connector
> > > > *connector);
> > > > +struct intel_digital_connector_state *
> > > > +intel_atomic_get_digital_connector_state(struct
> > > > intel_atomic_state *state,
> > > > +					 struct intel_connector
> > > > *connector);
> > > >  
> > > >  struct drm_crtc_state *intel_crtc_duplicate_state(struct
> > > > drm_crtc *crtc);
> > > >  void intel_crtc_destroy_state(struct drm_crtc *crtc,
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > index 5b6f32517c75..6ee5230045eb 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > @@ -1903,8 +1903,13 @@
> > > > intel_ddi_transcoder_func_reg_val_get(const struct
> > > > intel_crtc_state *crtc_state)
> > > >  		temp |= TRANS_DDI_MODE_SELECT_DP_MST;
> > > >  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> > > >  
> > > > -		if (INTEL_GEN(dev_priv) >= 12)
> > > > -			temp |=
> > > > TRANS_DDI_MST_TRANSPORT_SELECT(crtc_state->cpu_transcoder);
> > > > +		if (INTEL_GEN(dev_priv) >= 12) {
> > > > +			enum transcoder master;
> > > > +
> > > > +			master = crtc_state-
> > > > >mst_master_transcoder;
> > > > +			WARN_ON(master == INVALID_TRANSCODER);
> > > > +			temp |=
> > > > TRANS_DDI_MST_TRANSPORT_SELECT(master);
> > > > +		}
> > > >  	} else {
> > > >  		temp |= TRANS_DDI_MODE_SELECT_DP_SST;
> > > >  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> > > > @@ -4377,6 +4382,11 @@ void intel_ddi_get_config(struct
> > > > intel_encoder *encoder,
> > > >  		pipe_config->output_types |=
> > > > BIT(INTEL_OUTPUT_DP_MST);
> > > >  		pipe_config->lane_count =
> > > >  			((temp & DDI_PORT_WIDTH_MASK) >>
> > > > DDI_PORT_WIDTH_SHIFT) + 1;
> > > > +
> > > > +		if (INTEL_GEN(dev_priv) >= 12)
> > > > +			pipe_config->mst_master_transcoder =
> > > > +					REG_FIELD_GET(TRANS_DDI
> > > > _MST_TRA
> > > > NSPORT_SELECT_MASK, temp);
> > > > +
> > > >  		intel_dp_get_m_n(intel_crtc, pipe_config);
> > > >  		break;
> > > >  	default:
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 39b00a19d752..1cecce2f54f8 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -46,6 +46,7 @@
> > > >  #include "display/intel_crt.h"
> > > >  #include "display/intel_ddi.h"
> > > >  #include "display/intel_dp.h"
> > > > +#include "display/intel_dp_mst.h"
> > > >  #include "display/intel_dsi.h"
> > > >  #include "display/intel_dvo.h"
> > > >  #include "display/intel_gmbus.h"
> > > > @@ -11663,6 +11664,7 @@ static void
> > > > intel_crtc_state_reset(struct
> > > > intel_crtc_state *crtc_state,
> > > >  	crtc_state->hsw_workaround_pipe = INVALID_PIPE;
> > > >  	crtc_state->output_format =
> > > > INTEL_OUTPUT_FORMAT_INVALID;
> > > >  	crtc_state->scaler_state.scaler_id = -1;
> > > > +	crtc_state->mst_master_transcoder = INVALID_TRANSCODER;
> > > >  }
> > > >  
> > > >  /* Returns the currently programmed mode of the given encoder.
> > > > */
> > > > @@ -12510,6 +12512,9 @@ static void
> > > > intel_dump_pipe_config(const
> > > > struct intel_crtc_state *pipe_config,
> > > >  			      pipe_config->csc_mode,
> > > > pipe_config-
> > > > > gamma_mode,
> > > >  			      pipe_config->gamma_enable,
> > > > pipe_config-
> > > > > csc_enable);
> > > >  
> > > > +	DRM_DEBUG_KMS("MST master transcoder: %s\n",
> > > > +		      transcoder_name(pipe_config-
> > > > > mst_master_transcoder));
> > > > +
> > > >  dump_planes:
> > > >  	if (!state)
> > > >  		return;
> > > > @@ -12651,6 +12656,7 @@ intel_crtc_prepare_cleared_state(struct
> > > > intel_crtc_state *crtc_state)
> > > >  	memcpy(saved_state->icl_port_dplls, crtc_state-
> > > > >icl_port_dplls,
> > > >  	       sizeof(saved_state->icl_port_dplls));
> > > >  	saved_state->crc_enabled = crtc_state->crc_enabled;
> > > > +	saved_state->mst_master_transcoder =
> > > > INVALID_TRANSCODER;
> > > >  	if (IS_G4X(dev_priv) ||
> > > >  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > >  		saved_state->wm = crtc_state->wm;
> > > > @@ -13290,6 +13296,8 @@ intel_pipe_config_compare(const struct
> > > > intel_crtc_state *current_config,
> > > >  	PIPE_CONF_CHECK_I(dsc.dsc_split);
> > > >  	PIPE_CONF_CHECK_I(dsc.compressed_bpp);
> > > >  
> > > > +	PIPE_CONF_CHECK_I(mst_master_transcoder);
> > > > +
> > > >  #undef PIPE_CONF_CHECK_X
> > > >  #undef PIPE_CONF_CHECK_I
> > > >  #undef PIPE_CONF_CHECK_BOOL
> > > > @@ -14374,7 +14382,7 @@ static void
> > > > intel_commit_modeset_disables(struct intel_atomic_state *state)
> > > >  	u32 handled = 0;
> > > >  	int i;
> > > >  
> > > > -	/* Only disable port sync slaves */
> > > > +	/* Only disable port sync and MST slaves */
> > > >  	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > > old_crtc_state,
> > > >  					    new_crtc_state, i)
> > > > {
> > > >  		if (!needs_modeset(new_crtc_state))
> > > > @@ -14388,7 +14396,8 @@ static void
> > > > intel_commit_modeset_disables(struct intel_atomic_state *state)
> > > >  		 * slave CRTCs are disabled first and then
> > > > master CRTC
> > > > since
> > > >  		 * Slave vblanks are masked till Master
> > > > Vblanks.
> > > >  		 */
> > > > -		if (!is_trans_port_sync_slave(old_crtc_state))
> > > > +		if (!is_trans_port_sync_slave(old_crtc_state)
> > > > &&
> > > > +		    !intel_dp_mst_is_slave_trans(old_crtc_state
> > > > ))
> > > >  			continue;
> > > >  
> > > >  		intel_pre_plane_update(state, crtc);
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index 83ea04149b77..630a94892b7b 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -1054,6 +1054,9 @@ struct intel_crtc_state {
> > > >  
> > > >  	/* Bitmask to indicate slaves attached */
> > > >  	u8 sync_mode_slaves_mask;
> > > > +
> > > > +	/* Only valid on TGL+ */
> > > > +	enum transcoder mst_master_transcoder;
> > > >  };
> > > >  
> > > >  struct intel_crtc {
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > index 926e49f449a6..12f5e799d91f 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > @@ -87,6 +87,49 @@ static int
> > > > intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +/*
> > > > + * Iterate over all connectors and set mst_master_transcoder
> > > > to
> > > > the smallest
> > > > + * transcoder id that shares the same MST connector.
> > > > + */
> > > > +static int
> > > > +intel_dp_mst_master_trans_compute(struct intel_encoder
> > > > *encoder,
> > > > +				  struct intel_crtc_state
> > > > *crtc_state,
> > > > +				  struct drm_connector_state
> > > > *connector_state)
> > > > +{
> > > > +	struct intel_atomic_state *state =
> > > > to_intel_atomic_state(crtc_state->uapi.state);
> > > > +	struct intel_connector *connector =
> > > > to_intel_connector(connector_state->connector);
> > > > +	struct drm_i915_private *dev_priv = to_i915(encoder-
> > > > >base.dev);
> > > > +	struct intel_digital_connector_state
> > > > *iter_connector_state;
> > > > +	struct intel_connector *iter_connector;
> > > > +	int i;
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 12)
> > > > +		return 0;
> > > > +
> > > > +	crtc_state->mst_master_transcoder =
> > > > I915_MAX_TRANSCODERS;
> > > > +
> > > > +	for_each_new_intel_connector_in_state(state,
> > > > iter_connector,
> > > > +					      iter_connector_st
> > > > ate, i)
> > > > {
> > > > +		struct intel_crtc_state *iter_crtc_state;
> > > > +		struct intel_crtc *iter_crtc;
> > > > +
> > > > +		if (connector->mst_port != iter_connector-
> > > > >mst_port ||
> > > > +		    !iter_connector_state->base.crtc)
> > > > +			continue;
> > > > +
> > > > +		iter_crtc = to_intel_crtc(iter_connector_state-
> > > > > base.crtc);
> > > > +		iter_crtc_state =
> > > > intel_atomic_get_new_crtc_state(state,
> > > > +								
> > > >   iter_
> > > > crtc);
> > > > +		if (!iter_crtc_state->uapi.active)
> > > > +			continue;
> > > > +
> > > > +		if (iter_crtc_state->cpu_transcoder <
> > > > crtc_state-
> > > > > mst_master_transcoder)
> > > > +			crtc_state->mst_master_transcoder =
> > > > iter_crtc_state->cpu_transcoder;
> > 
> > Was just reading Manasi's port sync stuff when I realized this
> > won't
> > work. We haven't necessarily compute cpu_transcoder for the other
> > pipes yet. So I suggest we extract the code to compute the
> > cpu_transcoder
> > from intel_ddi_connector_get_hw_state() and use it everywhere,
> > including
> > here.
> 
> okay

Thinking better we dont need it, only in the initial commit
cpu_transcoder will have a wrong value, after that the previous state
will be duplicated, user will change the properties in the drm struct,
check/commit and it will keep the previous cpu_transcoder.

And because MST do all the topology discovery the initial state do not
matter.

But if you still think is more clear do the pipe to transcoder cast(TGL
don`t have a EDP transcoder), I will do it.

> 
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > 
> > > This looks like it can be a pure function that returns the
> > > master transcoder.
> > > 
> > > > +
> > > >  static int intel_dp_mst_compute_config(struct intel_encoder
> > > > *encoder,
> > > >  				       struct intel_crtc_state
> > > > *pipe_config,
> > > >  				       struct
> > > > drm_connector_state
> > > > *conn_state)
> > > > @@ -154,24 +197,88 @@ static int
> > > > intel_dp_mst_compute_config(struct intel_encoder *encoder,
> > > >  
> > > >  	intel_ddi_compute_min_voltage_level(dev_priv,
> > > > pipe_config);
> > > >  
> > > > +	ret = intel_dp_mst_master_trans_compute(encoder,
> > > > pipe_config,
> > > > +						conn_state);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/*
> > > > + * If one of the connectors in a MST stream needs a modeset,
> > > > mark all CRTCs
> > > > + * that have a connector in the same MST stream as mode
> > > > changed,
> > > > + * intel_modeset_pipe_config()+intel_crtc_check_fastset() will
> > > > take to do a
> > > > + * fastset when possible.
> > > > + */
> > > > +static int
> > > > +intel_dp_mst_atomic_master_trans_check(struct intel_connector
> > > > *connector,
> > > > +				       struct
> > > > intel_atomic_state
> > > > *state)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(state-
> > > > >base.dev);
> > > > +	struct drm_connector_list_iter connector_list_iter;
> > > > +	struct intel_connector *connector_iter;
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 12)
> > > > +		return  0;
> > > > +
> > > > +	if (!intel_digital_connector_needs_modeset(state,
> > > > connector))
> > > > +		return 0;
> > > > +
> > > > +	drm_connector_list_iter_begin(&dev_priv->drm,
> > > > &connector_list_iter);
> > > > +	for_each_intel_connector_iter(connector_iter,
> > > > &connector_list_iter) {
> > > > +		struct intel_digital_connector_state
> > > > *connector_iter_state;
> > > > +		struct intel_crtc_state *crtc_state;
> > > > +		struct intel_crtc *crtc;
> > > > +
> > > > +		if (connector_iter->mst_port != connector-
> > > > >mst_port)
> > > > +			continue;
> > > 
> > > Do we need to exclude the connector itself? I guess not really?
> > > 
> > > > +
> > > > +		connector_iter_state =
> > > > +			intel_atomic_get_digital_connector_stat
> > > > e(state,
> > > > +								
> > > >  connec
> > > > tor_iter);
> > > > +		if (IS_ERR(connector_iter_state)) {
> > > > +			drm_connector_list_iter_end(&connector_
> > > > list_ite
> > > > r);
> > > > +			return PTR_ERR(connector_iter_state);
> > > > +		}
> > > > +
> > > > +		if (!connector_iter_state->base.crtc)
> > > > +			continue;
> > > > +
> > > > +		crtc = to_intel_crtc(connector_iter_state-
> > > > >base.crtc);
> > > > +		crtc_state =
> > > > intel_atomic_get_crtc_state(&state->base,
> > > > crtc);
> > > > +		if (IS_ERR(crtc_state)) {
> > > > +			drm_connector_list_iter_end(&connector_
> > > > list_ite
> > > > r);
> > > > +			return PTR_ERR(crtc_state);
> > > > +		}
> > > > +
> > > > +		crtc_state->uapi.mode_changed = true;
> > > > +	}
> > > > +	drm_connector_list_iter_end(&connector_list_iter);
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  
> > > >  static int
> > > >  intel_dp_mst_atomic_check(struct drm_connector *connector,
> > > > -			  struct drm_atomic_state *state)
> > > > +			  struct drm_atomic_state *_state)
> > > >  {
> > > > +	struct intel_atomic_state *state =
> > > > to_intel_atomic_state(_state);
> > > >  	struct drm_connector_state *new_conn_state =
> > > > -		drm_atomic_get_new_connector_state(state,
> > > > connector);
> > > > +		drm_atomic_get_new_connector_state(&state-
> > > > >base,
> > > > connector);
> > > >  	struct drm_connector_state *old_conn_state =
> > > > -		drm_atomic_get_old_connector_state(state,
> > > > connector);
> > > > +		drm_atomic_get_old_connector_state(&state-
> > > > >base,
> > > > connector);
> > > >  	struct intel_connector *intel_connector =
> > > >  		to_intel_connector(connector);
> > > >  	struct drm_crtc *new_crtc = new_conn_state->crtc;
> > > >  	struct drm_dp_mst_topology_mgr *mgr;
> > > >  	int ret;
> > > >  
> > > > -	ret = intel_digital_connector_atomic_check(connector,
> > > > state);
> > > > +	ret = intel_digital_connector_atomic_check(connector,
> > > > &state-
> > > > > base);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret =
> > > > intel_dp_mst_atomic_master_trans_check(intel_connector,
> > > > state);
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > @@ -182,12 +289,9 @@ intel_dp_mst_atomic_check(struct
> > > > drm_connector *connector,
> > > >  	 * connector
> > > >  	 */
> > > >  	if (new_crtc) {
> > > > -		struct intel_atomic_state *intel_state =
> > > > -			to_intel_atomic_state(state);
> > > >  		struct intel_crtc *intel_crtc =
> > > > to_intel_crtc(new_crtc);
> > > >  		struct intel_crtc_state *crtc_state =
> > > > -			intel_atomic_get_new_crtc_state(intel_s
> > > > tate,
> > > > -							intel_c
> > > > rtc);
> > > > +			intel_atomic_get_new_crtc_state(state,
> > > > intel_crtc);
> > > >  
> > > >  		if (!crtc_state ||
> > > >  		    !drm_atomic_crtc_needs_modeset(&crtc_state-
> > > > >uapi)
> > > > @@ -196,7 +300,7 @@ intel_dp_mst_atomic_check(struct
> > > > drm_connector *connector,
> > > >  	}
> > > >  
> > > >  	mgr = &enc_to_mst(old_conn_state->best_encoder)-
> > > > >primary-
> > > > > dp.mst_mgr;
> > > > -	ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
> > > > +	ret = drm_dp_atomic_release_vcpi_slots(&state->base,
> > > > mgr,
> > > >  					       intel_connector-
> > > > >port);
> > > >  
> > > >  	return ret;
> > > > @@ -241,6 +345,9 @@ static void
> > > > intel_mst_post_disable_dp(struct
> > > > intel_encoder *encoder,
> > > >  	intel_dp->active_mst_links--;
> > > >  	last_mst_stream = intel_dp->active_mst_links == 0;
> > > >  
> > > > +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream &&
> > > > +		!intel_dp_mst_is_master_trans(old_crtc_state));
> > > > +
> > > >  	/*
> > > >  	 * From TGL spec: "If multi-stream slave transcoder:
> > > > Configure
> > > >  	 * Transcoder Clock Select to direct no clock to the
> > > > transcoder"
> > > > @@ -321,6 +428,9 @@ static void intel_mst_pre_enable_dp(struct
> > > > intel_encoder *encoder,
> > > >  	intel_mst->connector = connector;
> > > >  	first_mst_stream = intel_dp->active_mst_links == 0;
> > > >  
> > > > +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && first_mst_stream
> > > > &&
> > > > +		!intel_dp_mst_is_master_trans(pipe_config));
> > > > +
> > > >  	DRM_DEBUG_KMS("active links %d\n", intel_dp-
> > > > >active_mst_links);
> > > >  
> > > >  	if (first_mst_stream)
> > > > @@ -726,3 +836,14 @@ intel_dp_mst_encoder_cleanup(struct
> > > > intel_digital_port *intel_dig_port)
> > > >  	drm_dp_mst_topology_mgr_destroy(&intel_dp->mst_mgr);
> > > >  	/* encoders will get killed by normal cleanup */
> > > >  }
> > > > +
> > > > +bool intel_dp_mst_is_master_trans(const struct
> > > > intel_crtc_state
> > > > *crtc_state)
> > > > +{
> > > > +	return crtc_state->mst_master_transcoder == crtc_state-
> > > > > cpu_transcoder;
> > > > +}
> > > > +
> > > > +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state
> > > > *crtc_state)
> > > > +{
> > > > +	return crtc_state->mst_master_transcoder !=
> > > > INVALID_TRANSCODER
> > > > &&
> > > > +	       crtc_state->mst_master_transcoder != crtc_state-
> > > > > cpu_transcoder;
> > > > +}
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > > index f660ad80db04..e40767f78343 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > > @@ -6,10 +6,15 @@
> > > >  #ifndef __INTEL_DP_MST_H__
> > > >  #define __INTEL_DP_MST_H__
> > > >  
> > > > +#include <stdbool.h>
> > > 
> > > That doesn't seem like something you want to include in kernel
> > > code.
> > > 
> > > > +
> > > >  struct intel_digital_port;
> > > > +struct intel_crtc_state;
> > > >  
> > > >  int intel_dp_mst_encoder_init(struct intel_digital_port
> > > > *intel_dig_port, int conn_id);
> > > >  void intel_dp_mst_encoder_cleanup(struct intel_digital_port
> > > > *intel_dig_port);
> > > >  int intel_dp_mst_encoder_active_links(struct
> > > > intel_digital_port
> > > > *intel_dig_port);
> > > > +bool intel_dp_mst_is_master_trans(const struct
> > > > intel_crtc_state
> > > > *crtc_state);
> > > > +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state
> > > > *crtc_state);
> > > >  
> > > >  #endif /* __INTEL_DP_MST_H__ */
> > > > -- 
> > > > 2.24.1
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 rebased 07/11] drm/i915/tgl: Select master transcoder for MST stream

[Ville Syrjälä]

On Mon, Dec 16, 2019 at 07:07:52PM +0000, Souza, Jose wrote:
> On Mon, 2019-12-16 at 09:23 -0800, José Roberto de Souza wrote:
> > On Fri, 2019-12-13 at 22:56 +0200, Ville Syrjälä wrote:
> > > On Thu, Dec 12, 2019 at 10:44:29PM +0200, Ville Syrjälä wrote:
> > > > On Wed, Dec 11, 2019 at 10:45:22AM -0800, José Roberto de Souza
> > > > wrote:
> > > > > On TGL the blending of all the streams have moved from DDI to
> > > > > transcoder, so now every transcoder working over the same MST
> > > > > port must
> > > > > send its stream to a master transcoder and master will send to
> > > > > DDI
> > > > > respecting the time slots.
> > > > > 
> > > > > So here adding all the CRTCs that shares the same MST stream if
> > > > > needed and computing their state again, it will pick the lowest
> > > > > pipe/transcoder among the ones in the same stream to be master.
> > > > > 
> > > > > Most of the time skl_commit_modeset_enables() enables pipes in
> > > > > a
> > > > > crescent order but due DDB overlapping it might not happen,
> > > > > this
> > > > > scenarios will be handled in the next patch.
> > > > > 
> > > > > v2:
> > > > > - Using recently added intel_crtc_state_reset() to set
> > > > > mst_master_transcoder to invalid transcoder for all non gen12 &
> > > > > MST
> > > > > code paths
> > > > > - Setting lowest pipe/transcoder as master, previously it was
> > > > > the
> > > > > first one but setting a predictable one will help in future MST
> > > > > e
> > > > > port sync integration
> > > > > - Moving to intel type as much as we can
> > > > > 
> > > > > BSpec: 50493
> > > > > BSpec: 49190
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_atomic.c   |  14 ++
> > > > >  drivers/gpu/drm/i915/display/intel_atomic.h   |   3 +
> > > > >  drivers/gpu/drm/i915/display/intel_ddi.c      |  14 +-
> > > > >  drivers/gpu/drm/i915/display/intel_display.c  |  13 +-
> > > > >  .../drm/i915/display/intel_display_types.h    |   3 +
> > > > >  drivers/gpu/drm/i915/display/intel_dp_mst.c   | 139
> > > > > ++++++++++++++++--
> > > > >  drivers/gpu/drm/i915/display/intel_dp_mst.h   |   5 +
> > > > >  7 files changed, 178 insertions(+), 13 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > > index 6e93a39a6fec..69a0430c4638 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > > @@ -206,6 +206,20 @@
> > > > > intel_digital_connector_needs_modeset(struct
> > > > > intel_atomic_state *state,
> > > > >  	       (new_crtc &&
> > > > > drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi));
> > > > >  }
> > > > >  
> > > > > +struct intel_digital_connector_state *
> > > > > +intel_atomic_get_digital_connector_state(struct
> > > > > intel_atomic_state *state,
> > > > > +					 struct intel_connector
> > > > > *connector)
> > > > > +{
> > > > > +	struct drm_connector_state *connector_state;
> > > > > +
> > > > > +	connector_state =
> > > > > drm_atomic_get_connector_state(&state->base,
> > > > > +							 &conne
> > > > > ctor-
> > > > > > base);
> > > > > +	if (IS_ERR(connector_state))
> > > > > +		return ERR_CAST(connector_state);
> > > > > +
> > > > > +	return
> > > > > to_intel_digital_connector_state(connector_state);
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * intel_crtc_duplicate_state - duplicate crtc state
> > > > >   * @crtc: drm crtc
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h
> > > > > b/drivers/gpu/drm/i915/display/intel_atomic.h
> > > > > index ba9cc29a5865..6e8638d83d28 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> > > > > @@ -35,6 +35,9 @@ struct drm_connector_state *
> > > > >  intel_digital_connector_duplicate_state(struct drm_connector
> > > > > *connector);
> > > > >  bool intel_digital_connector_needs_modeset(struct
> > > > > intel_atomic_state *state,
> > > > >  					   struct
> > > > > intel_connector
> > > > > *connector);
> > > > > +struct intel_digital_connector_state *
> > > > > +intel_atomic_get_digital_connector_state(struct
> > > > > intel_atomic_state *state,
> > > > > +					 struct intel_connector
> > > > > *connector);
> > > > >  
> > > > >  struct drm_crtc_state *intel_crtc_duplicate_state(struct
> > > > > drm_crtc *crtc);
> > > > >  void intel_crtc_destroy_state(struct drm_crtc *crtc,
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > index 5b6f32517c75..6ee5230045eb 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > @@ -1903,8 +1903,13 @@
> > > > > intel_ddi_transcoder_func_reg_val_get(const struct
> > > > > intel_crtc_state *crtc_state)
> > > > >  		temp |= TRANS_DDI_MODE_SELECT_DP_MST;
> > > > >  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> > > > >  
> > > > > -		if (INTEL_GEN(dev_priv) >= 12)
> > > > > -			temp |=
> > > > > TRANS_DDI_MST_TRANSPORT_SELECT(crtc_state->cpu_transcoder);
> > > > > +		if (INTEL_GEN(dev_priv) >= 12) {
> > > > > +			enum transcoder master;
> > > > > +
> > > > > +			master = crtc_state-
> > > > > >mst_master_transcoder;
> > > > > +			WARN_ON(master == INVALID_TRANSCODER);
> > > > > +			temp |=
> > > > > TRANS_DDI_MST_TRANSPORT_SELECT(master);
> > > > > +		}
> > > > >  	} else {
> > > > >  		temp |= TRANS_DDI_MODE_SELECT_DP_SST;
> > > > >  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> > > > > @@ -4377,6 +4382,11 @@ void intel_ddi_get_config(struct
> > > > > intel_encoder *encoder,
> > > > >  		pipe_config->output_types |=
> > > > > BIT(INTEL_OUTPUT_DP_MST);
> > > > >  		pipe_config->lane_count =
> > > > >  			((temp & DDI_PORT_WIDTH_MASK) >>
> > > > > DDI_PORT_WIDTH_SHIFT) + 1;
> > > > > +
> > > > > +		if (INTEL_GEN(dev_priv) >= 12)
> > > > > +			pipe_config->mst_master_transcoder =
> > > > > +					REG_FIELD_GET(TRANS_DDI
> > > > > _MST_TRA
> > > > > NSPORT_SELECT_MASK, temp);
> > > > > +
> > > > >  		intel_dp_get_m_n(intel_crtc, pipe_config);
> > > > >  		break;
> > > > >  	default:
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index 39b00a19d752..1cecce2f54f8 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -46,6 +46,7 @@
> > > > >  #include "display/intel_crt.h"
> > > > >  #include "display/intel_ddi.h"
> > > > >  #include "display/intel_dp.h"
> > > > > +#include "display/intel_dp_mst.h"
> > > > >  #include "display/intel_dsi.h"
> > > > >  #include "display/intel_dvo.h"
> > > > >  #include "display/intel_gmbus.h"
> > > > > @@ -11663,6 +11664,7 @@ static void
> > > > > intel_crtc_state_reset(struct
> > > > > intel_crtc_state *crtc_state,
> > > > >  	crtc_state->hsw_workaround_pipe = INVALID_PIPE;
> > > > >  	crtc_state->output_format =
> > > > > INTEL_OUTPUT_FORMAT_INVALID;
> > > > >  	crtc_state->scaler_state.scaler_id = -1;
> > > > > +	crtc_state->mst_master_transcoder = INVALID_TRANSCODER;
> > > > >  }
> > > > >  
> > > > >  /* Returns the currently programmed mode of the given encoder.
> > > > > */
> > > > > @@ -12510,6 +12512,9 @@ static void
> > > > > intel_dump_pipe_config(const
> > > > > struct intel_crtc_state *pipe_config,
> > > > >  			      pipe_config->csc_mode,
> > > > > pipe_config-
> > > > > > gamma_mode,
> > > > >  			      pipe_config->gamma_enable,
> > > > > pipe_config-
> > > > > > csc_enable);
> > > > >  
> > > > > +	DRM_DEBUG_KMS("MST master transcoder: %s\n",
> > > > > +		      transcoder_name(pipe_config-
> > > > > > mst_master_transcoder));
> > > > > +
> > > > >  dump_planes:
> > > > >  	if (!state)
> > > > >  		return;
> > > > > @@ -12651,6 +12656,7 @@ intel_crtc_prepare_cleared_state(struct
> > > > > intel_crtc_state *crtc_state)
> > > > >  	memcpy(saved_state->icl_port_dplls, crtc_state-
> > > > > >icl_port_dplls,
> > > > >  	       sizeof(saved_state->icl_port_dplls));
> > > > >  	saved_state->crc_enabled = crtc_state->crc_enabled;
> > > > > +	saved_state->mst_master_transcoder =
> > > > > INVALID_TRANSCODER;
> > > > >  	if (IS_G4X(dev_priv) ||
> > > > >  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > >  		saved_state->wm = crtc_state->wm;
> > > > > @@ -13290,6 +13296,8 @@ intel_pipe_config_compare(const struct
> > > > > intel_crtc_state *current_config,
> > > > >  	PIPE_CONF_CHECK_I(dsc.dsc_split);
> > > > >  	PIPE_CONF_CHECK_I(dsc.compressed_bpp);
> > > > >  
> > > > > +	PIPE_CONF_CHECK_I(mst_master_transcoder);
> > > > > +
> > > > >  #undef PIPE_CONF_CHECK_X
> > > > >  #undef PIPE_CONF_CHECK_I
> > > > >  #undef PIPE_CONF_CHECK_BOOL
> > > > > @@ -14374,7 +14382,7 @@ static void
> > > > > intel_commit_modeset_disables(struct intel_atomic_state *state)
> > > > >  	u32 handled = 0;
> > > > >  	int i;
> > > > >  
> > > > > -	/* Only disable port sync slaves */
> > > > > +	/* Only disable port sync and MST slaves */
> > > > >  	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > > > old_crtc_state,
> > > > >  					    new_crtc_state, i)
> > > > > {
> > > > >  		if (!needs_modeset(new_crtc_state))
> > > > > @@ -14388,7 +14396,8 @@ static void
> > > > > intel_commit_modeset_disables(struct intel_atomic_state *state)
> > > > >  		 * slave CRTCs are disabled first and then
> > > > > master CRTC
> > > > > since
> > > > >  		 * Slave vblanks are masked till Master
> > > > > Vblanks.
> > > > >  		 */
> > > > > -		if (!is_trans_port_sync_slave(old_crtc_state))
> > > > > +		if (!is_trans_port_sync_slave(old_crtc_state)
> > > > > &&
> > > > > +		    !intel_dp_mst_is_slave_trans(old_crtc_state
> > > > > ))
> > > > >  			continue;
> > > > >  
> > > > >  		intel_pre_plane_update(state, crtc);
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > index 83ea04149b77..630a94892b7b 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > @@ -1054,6 +1054,9 @@ struct intel_crtc_state {
> > > > >  
> > > > >  	/* Bitmask to indicate slaves attached */
> > > > >  	u8 sync_mode_slaves_mask;
> > > > > +
> > > > > +	/* Only valid on TGL+ */
> > > > > +	enum transcoder mst_master_transcoder;
> > > > >  };
> > > > >  
> > > > >  struct intel_crtc {
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > index 926e49f449a6..12f5e799d91f 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > @@ -87,6 +87,49 @@ static int
> > > > > intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > +/*
> > > > > + * Iterate over all connectors and set mst_master_transcoder
> > > > > to
> > > > > the smallest
> > > > > + * transcoder id that shares the same MST connector.
> > > > > + */
> > > > > +static int
> > > > > +intel_dp_mst_master_trans_compute(struct intel_encoder
> > > > > *encoder,
> > > > > +				  struct intel_crtc_state
> > > > > *crtc_state,
> > > > > +				  struct drm_connector_state
> > > > > *connector_state)
> > > > > +{
> > > > > +	struct intel_atomic_state *state =
> > > > > to_intel_atomic_state(crtc_state->uapi.state);
> > > > > +	struct intel_connector *connector =
> > > > > to_intel_connector(connector_state->connector);
> > > > > +	struct drm_i915_private *dev_priv = to_i915(encoder-
> > > > > >base.dev);
> > > > > +	struct intel_digital_connector_state
> > > > > *iter_connector_state;
> > > > > +	struct intel_connector *iter_connector;
> > > > > +	int i;
> > > > > +
> > > > > +	if (INTEL_GEN(dev_priv) < 12)
> > > > > +		return 0;
> > > > > +
> > > > > +	crtc_state->mst_master_transcoder =
> > > > > I915_MAX_TRANSCODERS;
> > > > > +
> > > > > +	for_each_new_intel_connector_in_state(state,
> > > > > iter_connector,
> > > > > +					      iter_connector_st
> > > > > ate, i)
> > > > > {
> > > > > +		struct intel_crtc_state *iter_crtc_state;
> > > > > +		struct intel_crtc *iter_crtc;
> > > > > +
> > > > > +		if (connector->mst_port != iter_connector-
> > > > > >mst_port ||
> > > > > +		    !iter_connector_state->base.crtc)
> > > > > +			continue;
> > > > > +
> > > > > +		iter_crtc = to_intel_crtc(iter_connector_state-
> > > > > > base.crtc);
> > > > > +		iter_crtc_state =
> > > > > intel_atomic_get_new_crtc_state(state,
> > > > > +								
> > > > >   iter_
> > > > > crtc);
> > > > > +		if (!iter_crtc_state->uapi.active)
> > > > > +			continue;
> > > > > +
> > > > > +		if (iter_crtc_state->cpu_transcoder <
> > > > > crtc_state-
> > > > > > mst_master_transcoder)
> > > > > +			crtc_state->mst_master_transcoder =
> > > > > iter_crtc_state->cpu_transcoder;
> > > 
> > > Was just reading Manasi's port sync stuff when I realized this
> > > won't
> > > work. We haven't necessarily compute cpu_transcoder for the other
> > > pipes yet. So I suggest we extract the code to compute the
> > > cpu_transcoder
> > > from intel_ddi_connector_get_hw_state() and use it everywhere,
> > > including
> > > here.
> > 
> > okay
> 
> Thinking better we dont need it, only in the initial commit
> cpu_transcoder will have a wrong value, after that the previous state
> will be duplicated, user will change the properties in the drm struct,
> check/commit and it will keep the previous cpu_transcoder.
> 
> And because MST do all the topology discovery the initial state do not
> matter.
> 
> But if you still think is more clear do the pipe to transcoder cast(TGL
> don`t have a EDP transcoder), I will do it.

Yeah, I don't really like relying on the old value when we in theory
are going to overwrite it. Yes, in this case we can know that it will
get computed to the same value, but still feels too magicy to rely on
that. It'll set a bad example of nothing else. Someone is like to
copypaste it for something less innocuous than cpu_transcoder.

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

[Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [v2,rebased,01/11] drm: Add __drm_atomic_helper_crtc_state_reset() & co. (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2,rebased,01/11] drm: Add __drm_atomic_helper_crtc_state_reset() & co. (rev2)
URL   : https://patchwork.freedesktop.org/series/70775/
State : failure

== Summary ==

Applying: drm: Add __drm_atomic_helper_crtc_state_reset() & co.
Applying: drm/i915: s/intel_crtc/crtc/ in intel_crtc_init()
Applying: drm/i915: Introduce intel_crtc_{alloc, free}()
Applying: drm/i915: Introduce intel_crtc_state_reset()
Applying: drm/i915: Introduce intel_plane_state_reset()
Applying: drm/i915/display: Share intel_connector_needs_modeset()
Applying: drm/i915/tgl: Select master transcoder for MST stream
Applying: drm/i915/display: Always enables MST master pipe first
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/display/intel_display.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/display/intel_display.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/display/intel_display.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0008 drm/i915/display: Always enables MST master pipe first
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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