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

[Intel-gfx] [PATCH v3 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 v3 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 64e4bfb0dfc9..eb93b14b970d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -15787,14 +15787,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);
@@ -15802,15 +15802,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;
@@ -15820,7 +15820,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);
@@ -15828,7 +15828,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) ||
@@ -15849,32 +15849,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;
 
@@ -15884,7 +15884,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 v3 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 eb93b14b970d..13ced6a3f7d7 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);
@@ -10630,7 +10629,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;
 
@@ -15695,25 +15694,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;
 }
 
@@ -15784,27 +15770,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);
@@ -15855,11 +15863,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;
@@ -15879,12 +15882,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 v3 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 13ced6a3f7d7..8e3e05cfcb27 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);
@@ -10629,8 +10628,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);
@@ -11678,6 +11675,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)
@@ -11703,7 +11714,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);
@@ -13555,18 +13566,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);
 
@@ -15694,15 +15701,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, \
@@ -15785,9 +15783,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;
@@ -17408,8 +17406,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 v3 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 v3 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 moved it to a better
place intel_atomic.c.

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

No behavior changes intended here.

v3:
- removed digital from exported version of intel_connector_needs_modeset
- rollback connector to drm type

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  | 18 +++++++
 drivers/gpu/drm/i915/display/intel_atomic.h  |  2 +
 drivers/gpu/drm/i915/display/intel_display.c | 53 ++++++--------------
 3 files changed, 36 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..b7dda18b6f29 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -174,6 +174,24 @@ intel_digital_connector_duplicate_state(struct drm_connector *connector)
 	return &state->base;
 }
 
+/**
+ * intel_connector_needs_modeset - check if connector needs a modeset
+ */
+bool
+intel_connector_needs_modeset(struct intel_atomic_state *state,
+			      struct drm_connector *connector)
+{
+	const struct drm_connector_state *old_conn_state, *new_conn_state;
+
+	old_conn_state = drm_atomic_get_old_connector_state(&state->base, connector);
+	new_conn_state = drm_atomic_get_new_connector_state(&state->base, connector);
+
+	return old_conn_state->crtc != new_conn_state->crtc ||
+	       (new_conn_state->crtc &&
+		drm_atomic_crtc_needs_modeset(drm_atomic_get_new_crtc_state(&state->base,
+									    new_conn_state->crtc)));
+}
+
 /**
  * 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..a7d1a8576c48 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.h
+++ b/drivers/gpu/drm/i915/display/intel_atomic.h
@@ -32,6 +32,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_connector_needs_modeset(struct intel_atomic_state *state,
+				   struct drm_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 8e3e05cfcb27..0ee2e86a8826 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_connector_needs_modeset(state, &connector->base))
 			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_connector_needs_modeset(state, &connector->base))
 			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 v3 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

v3:
- Now intel_dp_mst_master_trans_compute() returns the MST master transcoder
- Replaced stdbool.h by linux/types.h
- Skip the connector being checked in
intel_dp_mst_atomic_master_trans_check()
- Using pipe instead of transcoder to compute MST master

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   |   4 +
 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   | 146 ++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_dp_mst.h   |   5 +
 7 files changed, 186 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index b7dda18b6f29..0ee7760fb24a 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -192,6 +192,20 @@ intel_connector_needs_modeset(struct intel_atomic_state *state,
 									    new_conn_state->crtc)));
 }
 
+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 a7d1a8576c48..74c749dbfb4f 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;
 
@@ -34,6 +35,9 @@ struct drm_connector_state *
 intel_digital_connector_duplicate_state(struct drm_connector *connector);
 bool intel_connector_needs_modeset(struct intel_atomic_state *state,
 				   struct drm_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 0ee2e86a8826..e976f82a0db7 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"
@@ -11666,6 +11667,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. */
@@ -12513,6 +12515,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;
@@ -12654,6 +12659,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;
@@ -13293,6 +13299,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
@@ -14377,7 +14385,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))
@@ -14391,7 +14399,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..9395c611c485 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -87,6 +87,54 @@ 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 enum transcoder
+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;
+	enum pipe ret = I915_MAX_PIPES;
+	int i;
+
+	if (INTEL_GEN(dev_priv) < 12)
+		return INVALID_TRANSCODER;
+
+	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;
+
+		/*
+		 * Using crtc->pipe because crtc_state->cpu_transcoder is
+		 * computed, so others pipe cpu_transcoder could have being
+		 * computed yet
+		 */
+		if (iter_crtc->pipe < ret)
+			ret = iter_crtc->pipe;
+	}
+
+	/* Simple cast works because TGL don't have a eDP transcoder */
+	return (enum transcoder)ret;
+}
+
 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 +202,90 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
 
 	intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
 
+	pipe_config->mst_master_transcoder = 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_connector_needs_modeset(state, &connector->base))
+		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 ||
+		    connector_iter == connector)
+			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 +296,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 +307,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 +352,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 +435,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 +843,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..854724f68f09 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 <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

[Intel-gfx] [PATCH v3 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.

v3: rebased

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 | 88 +++++++++++++++-----
 1 file changed, 65 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index e976f82a0db7..176e31de68e7 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14553,15 +14553,20 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 	u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
 	u8 required_slices = state->wm_results.ddb.enabled_slices;
 	struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
-	u8 dirty_pipes = 0;
+	u8 update_pipes = 0, modeset_pipes = 0;
 	int i;
 
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		if (!new_crtc_state->hw.active)
+			continue;
+
 		/* ignore allocations for crtc's that have been turned off. */
-		if (!needs_modeset(new_crtc_state) && new_crtc_state->hw.active)
+		if (!needs_modeset(new_crtc_state)) {
 			entries[i] = old_crtc_state->wm.skl.ddb;
-		if (new_crtc_state->hw.active)
-			dirty_pipes |= BIT(crtc->pipe);
+			update_pipes |= BIT(crtc->pipe);
+		} else {
+			modeset_pipes |= BIT(modeset_pipes);
+		}
 	}
 
 	/* If 2nd DBuf slice required, enable it here */
@@ -14571,16 +14576,18 @@ 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
 	 */
-	while (dirty_pipes) {
+	while (update_pipes) {
 		for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
 						    new_crtc_state, i) {
 			enum pipe pipe = crtc->pipe;
-			bool modeset = needs_modeset(new_crtc_state);
 
-			if ((dirty_pipes & BIT(pipe)) == 0)
+			if ((update_pipes & BIT(pipe)) == 0)
 				continue;
 
 			if (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
@@ -14589,20 +14596,10 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 				continue;
 
 			entries[i] = new_crtc_state->wm.skl.ddb;
-			dirty_pipes &= ~BIT(pipe);
-
-			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);
-			}
+			update_pipes &= ~BIT(pipe);
+
+			intel_update_crtc(crtc, state, old_crtc_state,
+					  new_crtc_state);
 
 			/*
 			 * If this is an already active pipe, it's DDB changed,
@@ -14612,11 +14609,56 @@ 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 && dirty_pipes)
+			    update_pipes)
 				intel_wait_for_vblank(dev_priv, pipe);
+
 		}
 	}
 
+	/*
+	 * 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) {
+		enum pipe pipe = crtc->pipe;
+
+		if ((modeset_pipes & BIT(pipe)) == 0)
+			continue;
+
+		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
+		    is_trans_port_sync_slave(new_crtc_state))
+			continue;
+
+		modeset_pipes &= ~BIT(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) {
+		enum pipe pipe = crtc->pipe;
+
+		if ((modeset_pipes & BIT(pipe)) == 0)
+			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 v3 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 176e31de68e7..f1c0687bf69b 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 9395c611c485..dba74711c415 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -337,6 +337,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)
@@ -355,6 +406,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"
@@ -365,19 +422,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 854724f68f09..f058facfa1a5 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 v3 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 this patch only forces a fullmodeset in MST slaves of MST
masters that needs a fullmodeset but it will be needed for port sync
as well.

v3:
- moved handling to intel_atomic_check() this way is guarantee that
all pipes will have its state computed

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 | 63 ++++++++++++++++----
 1 file changed, 50 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index f1c0687bf69b..2627a7bcb45c 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13941,19 +13941,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
 
 	new_crtc_state->uapi.mode_changed = false;
 	new_crtc_state->update_pipe = true;
-
-	/*
-	 * If we're not doing the full modeset we want to
-	 * keep the current M/N values as they may be
-	 * sufficiently different to the computed values
-	 * to cause problems.
-	 *
-	 * FIXME: should really copy more fuzzy state here
-	 */
-	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
-	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
-	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
-	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
 }
 
 static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
@@ -14121,6 +14108,56 @@ static int intel_atomic_check(struct drm_device *dev,
 			any_ms = true;
 	}
 
+	/**
+	 * 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.
+	 */
+	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
+		struct intel_crtc_state *new_crtc_state_iter;
+		struct intel_crtc *crtc_iter;
+		int j;
+
+		if (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
+		    needs_modeset(new_crtc_state))
+			continue;
+
+		for_each_new_intel_crtc_in_state(state, crtc_iter,
+						 new_crtc_state_iter, j) {
+			if (new_crtc_state->mst_master_transcoder !=
+			    new_crtc_state_iter->cpu_transcoder)
+				continue;
+
+			if (needs_modeset(new_crtc_state_iter)) {
+				new_crtc_state->uapi.mode_changed = true;
+				new_crtc_state->update_pipe = false;
+			}
+			break;
+		}
+	}
+
+	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+					    new_crtc_state, i) {
+		if (needs_modeset(new_crtc_state))
+			continue;
+
+		/*
+		 * If we're not doing the full modeset we want to
+		 * keep the current M/N values as they may be
+		 * sufficiently different to the computed values
+		 * to cause problems.
+		 *
+		 * FIXME: should really copy more fuzzy state here
+		 */
+		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
+		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
+		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
+		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
+	}
+
 	if (any_ms && !check_digital_port_conflicts(state)) {
 		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
 		ret = EINVAL;
-- 
2.24.1

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

[Intel-gfx] [PATCH v3 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 2627a7bcb45c..2621b3e5cd60 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14504,6 +14504,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

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

[Lucas De Marchi]

On Mon, Dec 16, 2019 at 02:07:32PM -0800, Jose Souza wrote:
>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

humn... Ville, do you plan to apply the series from
https://patchwork.freedesktop.org/series/69129/ ?

I mentioned this typo there

Lucas De Marchi

>+ * @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 mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Lucas De Marchi]

On Mon, Dec 16, 2019 at 02:07:37PM -0800, Jose 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 moved it to a better
>place intel_atomic.c.
>
>That allowed us to convert the whole
>intel_encoders_update_prepare/complete to intel types too.
>
>No behavior changes intended here.
>
>v3:
>- removed digital from exported version of intel_connector_needs_modeset
>- rollback connector to drm type
>
>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  | 18 +++++++
> drivers/gpu/drm/i915/display/intel_atomic.h  |  2 +
> drivers/gpu/drm/i915/display/intel_display.c | 53 ++++++--------------
> 3 files changed, 36 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..b7dda18b6f29 100644
>--- a/drivers/gpu/drm/i915/display/intel_atomic.c
>+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
>@@ -174,6 +174,24 @@ intel_digital_connector_duplicate_state(struct drm_connector *connector)
> 	return &state->base;
> }
>
>+/**
>+ * intel_connector_needs_modeset - check if connector needs a modeset
>+ */
>+bool
>+intel_connector_needs_modeset(struct intel_atomic_state *state,
>+			      struct drm_connector *connector)
>+{
>+	const struct drm_connector_state *old_conn_state, *new_conn_state;
>+
>+	old_conn_state = drm_atomic_get_old_connector_state(&state->base, connector);
>+	new_conn_state = drm_atomic_get_new_connector_state(&state->base, connector);
>+
>+	return old_conn_state->crtc != new_conn_state->crtc ||
>+	       (new_conn_state->crtc &&
>+		drm_atomic_crtc_needs_modeset(drm_atomic_get_new_crtc_state(&state->base,
>+									    new_conn_state->crtc)));
>+}
>+
> /**
>  * 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..a7d1a8576c48 100644
>--- a/drivers/gpu/drm/i915/display/intel_atomic.h
>+++ b/drivers/gpu/drm/i915/display/intel_atomic.h
>@@ -32,6 +32,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_connector_needs_modeset(struct intel_atomic_state *state,
>+				   struct drm_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 8e3e05cfcb27..0ee2e86a8826 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;

what's up with this fight new_conn_state vs new_connector_state? It' s
the first time in the driver we would actually drive such conversion.
I'd say to just keep the old name.

I'm not sure I like the conversion to use the connector rather than the
connector_state because we are likely to have the state around in the
caller. Anyway, I'm ok with it.

Lucas De Marchi

>+	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_connector_needs_modeset(state, &connector->base))
> 			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_connector_needs_modeset(state, &connector->base))
> 			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 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 [v3,01/11] drm: Add __drm_atomic_helper_crtc_state_reset() & co.

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_7578 -> Patchwork_15803
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_15803 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_15803, 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_15803/index.html

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-byt-j1900:       [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-byt-j1900/igt@gem_exec_suspend@basic-s3.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-byt-j1900/igt@gem_exec_suspend@basic-s3.html

  * igt@kms_busy@basic-flip-pipe-b:
    - fi-cfl-guc:         [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-cfl-guc/igt@kms_busy@basic-flip-pipe-b.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-cfl-guc/igt@kms_busy@basic-flip-pipe-b.html
    - fi-cfl-8700k:       [PASS][5] -> [INCOMPLETE][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-cfl-8700k/igt@kms_busy@basic-flip-pipe-b.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-cfl-8700k/igt@kms_busy@basic-flip-pipe-b.html

  * igt@kms_busy@basic-flip-pipe-c:
    - fi-kbl-7500u:       [PASS][7] -> [INCOMPLETE][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-kbl-7500u/igt@kms_busy@basic-flip-pipe-c.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-kbl-7500u/igt@kms_busy@basic-flip-pipe-c.html
    - fi-skl-guc:         [PASS][9] -> [INCOMPLETE][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-skl-guc/igt@kms_busy@basic-flip-pipe-c.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-skl-guc/igt@kms_busy@basic-flip-pipe-c.html

  * igt@runner@aborted:
    - fi-whl-u:           NOTRUN -> [FAIL][11]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-whl-u/igt@runner@aborted.html
    - fi-bxt-dsi:         NOTRUN -> [FAIL][12]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-bxt-dsi/igt@runner@aborted.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_blt:
    - fi-byt-j1900:       [PASS][13] -> [DMESG-FAIL][14] ([i915#725])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-byt-j1900/igt@i915_selftest@live_blt.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-byt-j1900/igt@i915_selftest@live_blt.html

  * igt@kms_busy@basic-flip-pipe-b:
    - fi-skl-6770hq:      [PASS][15] -> [INCOMPLETE][16] ([i915#198])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-skl-6770hq/igt@kms_busy@basic-flip-pipe-b.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-skl-6770hq/igt@kms_busy@basic-flip-pipe-b.html
    - fi-skl-lmem:        [PASS][17] -> [INCOMPLETE][18] ([i915#198])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-skl-lmem/igt@kms_busy@basic-flip-pipe-b.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-skl-lmem/igt@kms_busy@basic-flip-pipe-b.html
    - fi-cml-u2:          [PASS][19] -> [TIMEOUT][20] ([i915#109] / [i915#449])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-cml-u2/igt@kms_busy@basic-flip-pipe-b.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-cml-u2/igt@kms_busy@basic-flip-pipe-b.html
    - fi-apl-guc:         [PASS][21] -> [INCOMPLETE][22] ([fdo#103927])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-apl-guc/igt@kms_busy@basic-flip-pipe-b.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-apl-guc/igt@kms_busy@basic-flip-pipe-b.html
    - fi-bxt-dsi:         [PASS][23] -> [TIMEOUT][24] ([i915#109] / [i915#449])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-bxt-dsi/igt@kms_busy@basic-flip-pipe-b.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-bxt-dsi/igt@kms_busy@basic-flip-pipe-b.html
    - fi-skl-6600u:       [PASS][25] -> [TIMEOUT][26] ([i915#109] / [i915#449])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-skl-6600u/igt@kms_busy@basic-flip-pipe-b.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-skl-6600u/igt@kms_busy@basic-flip-pipe-b.html
    - fi-whl-u:           [PASS][27] -> [TIMEOUT][28] ([i915#109] / [i915#449])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-whl-u/igt@kms_busy@basic-flip-pipe-b.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-whl-u/igt@kms_busy@basic-flip-pipe-b.html
    - fi-icl-y:           [PASS][29] -> [TIMEOUT][30] ([i915#109] / [i915#449])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-icl-y/igt@kms_busy@basic-flip-pipe-b.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-icl-y/igt@kms_busy@basic-flip-pipe-b.html
    - fi-glk-dsi:         [PASS][31] -> [TIMEOUT][32] ([i915#109] / [i915#449])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-glk-dsi/igt@kms_busy@basic-flip-pipe-b.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-glk-dsi/igt@kms_busy@basic-flip-pipe-b.html
    - fi-tgl-y:           [PASS][33] -> [TIMEOUT][34] ([i915#109] / [i915#449] / [i915#561])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-tgl-y/igt@kms_busy@basic-flip-pipe-b.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-tgl-y/igt@kms_busy@basic-flip-pipe-b.html
    - fi-icl-guc:         [PASS][35] -> [TIMEOUT][36] ([i915#109] / [i915#449])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-icl-guc/igt@kms_busy@basic-flip-pipe-b.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-icl-guc/igt@kms_busy@basic-flip-pipe-b.html
    - fi-kbl-r:           [PASS][37] -> [TIMEOUT][38] ([i915#109] / [i915#449])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-kbl-r/igt@kms_busy@basic-flip-pipe-b.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-kbl-r/igt@kms_busy@basic-flip-pipe-b.html

  
#### Possible fixes ####

  * igt@gem_exec_parallel@basic:
    - {fi-tgl-u}:         [INCOMPLETE][39] ([i915#476]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-tgl-u/igt@gem_exec_parallel@basic.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-tgl-u/igt@gem_exec_parallel@basic.html

  * igt@i915_selftest@live_gem_contexts:
    - fi-byt-n2820:       [DMESG-FAIL][41] ([i915#722]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-byt-n2820/igt@i915_selftest@live_gem_contexts.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-byt-n2820/igt@i915_selftest@live_gem_contexts.html

  
#### Warnings ####

  * igt@debugfs_test@read_all_entries:
    - fi-kbl-x1275:       [DMESG-WARN][43] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][44] ([i915#62] / [i915#92])
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-kbl-x1275/igt@debugfs_test@read_all_entries.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-kbl-x1275/igt@debugfs_test@read_all_entries.html

  * igt@gem_exec_suspend@basic-s0:
    - fi-kbl-x1275:       [DMESG-WARN][45] ([i915#62] / [i915#92]) -> [DMESG-WARN][46] ([i915#62] / [i915#92] / [i915#95])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-kbl-x1275/igt@gem_exec_suspend@basic-s0.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-kbl-x1275/igt@gem_exec_suspend@basic-s0.html

  * igt@kms_busy@basic-flip-pipe-b:
    - fi-kbl-x1275:       [DMESG-WARN][47] ([i915#62] / [i915#92] / [i915#95]) -> [INCOMPLETE][48] ([i915#605])
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-kbl-x1275/igt@kms_busy@basic-flip-pipe-b.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-kbl-x1275/igt@kms_busy@basic-flip-pipe-b.html

  * igt@runner@aborted:
    - fi-icl-guc:         [FAIL][49] ([fdo#110943] / [fdo#111093]) -> [FAIL][50] ([fdo#111093] / [i915#287] / [k.org#203557])
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7578/fi-icl-guc/igt@runner@aborted.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/fi-icl-guc/igt@runner@aborted.html

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

  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#110943]: https://bugs.freedesktop.org/show_bug.cgi?id=110943
  [fdo#111093]: https://bugs.freedesktop.org/show_bug.cgi?id=111093
  [i915#109]: https://gitlab.freedesktop.org/drm/intel/issues/109
  [i915#198]: https://gitlab.freedesktop.org/drm/intel/issues/198
  [i915#287]: https://gitlab.freedesktop.org/drm/intel/issues/287
  [i915#449]: https://gitlab.freedesktop.org/drm/intel/issues/449
  [i915#476]: https://gitlab.freedesktop.org/drm/intel/issues/476
  [i915#561]: https://gitlab.freedesktop.org/drm/intel/issues/561
  [i915#605]: https://gitlab.freedesktop.org/drm/intel/issues/605
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#707]: https://gitlab.freedesktop.org/drm/intel/issues/707
  [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
  [k.org#203557]: https://bugzilla.kernel.org/show_bug.cgi?id=203557


Participating hosts (51 -> 44)
------------------------------

  Additional (1): fi-kbl-soraka 
  Missing    (8): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-u3 fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7578 -> Patchwork_15803

  CI-20190529: 20190529
  CI_DRM_7578: cc329d389f5609d2969d0797bc96f754adb26d62 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5349: 048f58513d8b8ec6bb307a939f0ac959bc0f0e10 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_15803: ae948caa25b0896e58dc261f45fc491b04daec25 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

ae948caa25b0 drm/i915/display: Add comment to a function that probably can be removed
4f454f4f7104 drm/i915/display: Check if pipe fastset is allowed by external dependencies
366edbcdd352 drm/i915/dp: Fix MST disable sequences
d7e4652cba05 drm/i915/display: Always enables MST master pipe first
a1c682bbdecc drm/i915/tgl: Select master transcoder for MST stream
f4e33c1eded8 drm/i915/display: Share intel_connector_needs_modeset()
63cf263b701d drm/i915: Introduce intel_plane_state_reset()
6e965a363495 drm/i915: Introduce intel_crtc_state_reset()
daf1ac79fce6 drm/i915: Introduce intel_crtc_{alloc, free}()
b02152ee12cf drm/i915: s/intel_crtc/crtc/ in intel_crtc_init()
b2d4346269f9 drm: Add __drm_atomic_helper_crtc_state_reset() & co.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15803/index.html
_______________________________________________
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 [v3,01/11] drm: Add __drm_atomic_helper_crtc_state_reset() & co. (rev2)

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_7581 -> Patchwork_15813
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_15813 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_15813, 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_15813/index.html

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@kms_busy@basic-flip-pipe-b:
    - fi-cfl-guc:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-cfl-guc/igt@kms_busy@basic-flip-pipe-b.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-cfl-guc/igt@kms_busy@basic-flip-pipe-b.html
    - fi-cfl-8700k:       [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-cfl-8700k/igt@kms_busy@basic-flip-pipe-b.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-cfl-8700k/igt@kms_busy@basic-flip-pipe-b.html

  * igt@kms_busy@basic-flip-pipe-c:
    - fi-kbl-7500u:       [PASS][5] -> [INCOMPLETE][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-kbl-7500u/igt@kms_busy@basic-flip-pipe-c.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-kbl-7500u/igt@kms_busy@basic-flip-pipe-c.html
    - fi-skl-guc:         [PASS][7] -> [INCOMPLETE][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-skl-guc/igt@kms_busy@basic-flip-pipe-c.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-skl-guc/igt@kms_busy@basic-flip-pipe-c.html

  * igt@runner@aborted:
    - fi-whl-u:           NOTRUN -> [FAIL][9]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-whl-u/igt@runner@aborted.html
    - fi-bxt-dsi:         NOTRUN -> [FAIL][10]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-bxt-dsi/igt@runner@aborted.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_busy@basic-flip-pipe-b:
    - fi-skl-6770hq:      [PASS][11] -> [INCOMPLETE][12] ([i915#198])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-skl-6770hq/igt@kms_busy@basic-flip-pipe-b.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-skl-6770hq/igt@kms_busy@basic-flip-pipe-b.html
    - fi-skl-lmem:        [PASS][13] -> [INCOMPLETE][14] ([i915#198])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-skl-lmem/igt@kms_busy@basic-flip-pipe-b.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-skl-lmem/igt@kms_busy@basic-flip-pipe-b.html
    - fi-cml-u2:          [PASS][15] -> [TIMEOUT][16] ([i915#109] / [i915#449])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-cml-u2/igt@kms_busy@basic-flip-pipe-b.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-cml-u2/igt@kms_busy@basic-flip-pipe-b.html
    - fi-apl-guc:         [PASS][17] -> [INCOMPLETE][18] ([fdo#103927])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-apl-guc/igt@kms_busy@basic-flip-pipe-b.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-apl-guc/igt@kms_busy@basic-flip-pipe-b.html
    - fi-bxt-dsi:         [PASS][19] -> [TIMEOUT][20] ([i915#109] / [i915#449])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-bxt-dsi/igt@kms_busy@basic-flip-pipe-b.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-bxt-dsi/igt@kms_busy@basic-flip-pipe-b.html
    - fi-skl-6600u:       [PASS][21] -> [TIMEOUT][22] ([i915#109] / [i915#449])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-skl-6600u/igt@kms_busy@basic-flip-pipe-b.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-skl-6600u/igt@kms_busy@basic-flip-pipe-b.html
    - fi-whl-u:           [PASS][23] -> [TIMEOUT][24] ([i915#109] / [i915#449])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-whl-u/igt@kms_busy@basic-flip-pipe-b.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-whl-u/igt@kms_busy@basic-flip-pipe-b.html
    - fi-icl-y:           [PASS][25] -> [TIMEOUT][26] ([i915#109] / [i915#449])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-icl-y/igt@kms_busy@basic-flip-pipe-b.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-icl-y/igt@kms_busy@basic-flip-pipe-b.html
    - fi-glk-dsi:         [PASS][27] -> [TIMEOUT][28] ([i915#109] / [i915#449])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-glk-dsi/igt@kms_busy@basic-flip-pipe-b.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-glk-dsi/igt@kms_busy@basic-flip-pipe-b.html
    - fi-tgl-y:           [PASS][29] -> [TIMEOUT][30] ([i915#109] / [i915#449] / [i915#561])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-tgl-y/igt@kms_busy@basic-flip-pipe-b.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-tgl-y/igt@kms_busy@basic-flip-pipe-b.html
    - fi-kbl-soraka:      [PASS][31] -> [TIMEOUT][32] ([i915#109] / [i915#449])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-kbl-soraka/igt@kms_busy@basic-flip-pipe-b.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-kbl-soraka/igt@kms_busy@basic-flip-pipe-b.html
    - fi-icl-guc:         [PASS][33] -> [TIMEOUT][34] ([i915#109] / [i915#449])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-icl-guc/igt@kms_busy@basic-flip-pipe-b.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-icl-guc/igt@kms_busy@basic-flip-pipe-b.html
    - fi-kbl-r:           [PASS][35] -> [TIMEOUT][36] ([i915#109] / [i915#449])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-kbl-r/igt@kms_busy@basic-flip-pipe-b.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-kbl-r/igt@kms_busy@basic-flip-pipe-b.html

  
#### Possible fixes ####

  * igt@gem_close_race@basic-threads:
    - fi-byt-n2820:       [TIMEOUT][37] ([i915#816]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-byt-n2820/igt@gem_close_race@basic-threads.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-byt-n2820/igt@gem_close_race@basic-threads.html

  * igt@gem_exec_gttfill@basic:
    - {fi-tgl-guc}:       [INCOMPLETE][39] ([fdo#111593]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-tgl-guc/igt@gem_exec_gttfill@basic.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-tgl-guc/igt@gem_exec_gttfill@basic.html

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-ilk-650:         [DMESG-WARN][41] ([i915#116]) -> [PASS][42] +1 similar issue
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-ilk-650/igt@i915_module_load@reload-with-fault-injection.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-ilk-650/igt@i915_module_load@reload-with-fault-injection.html

  
#### Warnings ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-kbl-x1275:       [DMESG-WARN][43] ([fdo#107139] / [i915#62] / [i915#92]) -> [DMESG-WARN][44] ([fdo#107139] / [i915#62] / [i915#92] / [i915#95])
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-kbl-x1275/igt@gem_exec_suspend@basic-s4-devices.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-kbl-x1275/igt@gem_exec_suspend@basic-s4-devices.html

  * igt@i915_selftest@live_blt:
    - fi-ivb-3770:        [DMESG-FAIL][45] ([i915#725]) -> [DMESG-FAIL][46] ([i915#770])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-ivb-3770/igt@i915_selftest@live_blt.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-ivb-3770/igt@i915_selftest@live_blt.html
    - fi-hsw-4770r:       [DMESG-FAIL][47] ([i915#553] / [i915#725]) -> [DMESG-FAIL][48] ([i915#725])
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-hsw-4770r/igt@i915_selftest@live_blt.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-hsw-4770r/igt@i915_selftest@live_blt.html

  * igt@kms_busy@basic-flip-pipe-b:
    - fi-kbl-x1275:       [DMESG-WARN][49] ([i915#62] / [i915#92]) -> [INCOMPLETE][50] ([i915#605])
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-kbl-x1275/igt@kms_busy@basic-flip-pipe-b.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-kbl-x1275/igt@kms_busy@basic-flip-pipe-b.html

  * igt@runner@aborted:
    - fi-icl-guc:         [FAIL][51] ([fdo#110943] / [fdo#111093]) -> [FAIL][52] ([fdo#111093] / [i915#287] / [k.org#203557])
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7581/fi-icl-guc/igt@runner@aborted.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15813/fi-icl-guc/igt@runner@aborted.html

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

  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#107139]: https://bugs.freedesktop.org/show_bug.cgi?id=107139
  [fdo#110943]: https://bugs.freedesktop.org/show_bug.cgi?id=110943
  [fdo#111093]: https://bugs.freedesktop.org/show_bug.cgi?id=111093
  [fdo#111593]: https://bugs.freedesktop.org/show_bug.cgi?id=111593
  [i915#109]: https://gitlab.freedesktop.org/drm/intel/issues/109
  [i915#116]: https://gitlab.freedesktop.org/drm/intel/issues/116
  [i915#198]: https://gitlab.freedesktop.org/drm/intel/issues/198
  [i915#287]: https://gitlab.freedesktop.org/drm/intel/issues/287
  [i915#449]: https://gitlab.freedesktop.org/drm/intel/issues/449
  [i915#476]: https://gitlab.freedesktop.org/drm/intel/issues/476
  [i915#553]: https://gitlab.freedesktop.org/drm/intel/issues/553
  [i915#561]: https://gitlab.freedesktop.org/drm/intel/issues/561
  [i915#605]: https://gitlab.freedesktop.org/drm/intel/issues/605
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725
  [i915#770]: https://gitlab.freedesktop.org/drm/intel/issues/770
  [i915#816]: https://gitlab.freedesktop.org/drm/intel/issues/816
  [i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95
  [k.org#203557]: https://bugzilla.kernel.org/show_bug.cgi?id=203557


Participating hosts (53 -> 47)
------------------------------

  Additional (1): fi-blb-e6850 
  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7581 -> Patchwork_15813

  CI-20190529: 20190529
  CI_DRM_7581: 5ccf72e75044ac46e1db5b66d9a9780b316a7c26 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5349: 048f58513d8b8ec6bb307a939f0ac959bc0f0e10 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_15813: 2a21719011109e35fb1260a45c700a128030c45d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

2a2171901110 drm/i915/display: Add comment to a function that probably can be removed
2b87688a3815 drm/i915/display: Check if pipe fastset is allowed by external dependencies
2bff3c1a0e10 drm/i915/dp: Fix MST disable sequences
d47b4a3a790c drm/i915/display: Always enables MST master pipe first
c3c03b1f66d2 drm/i915/tgl: Select master transcoder for MST stream
b4d6fefb2a73 drm/i915/display: Share intel_connector_needs_modeset()
4ba5962e635b drm/i915: Introduce intel_plane_state_reset()
36f03a633586 drm/i915: Introduce intel_crtc_state_reset()
5cc530fd92e0 drm/i915: Introduce intel_crtc_{alloc, free}()
6ceb6d64933d drm/i915: s/intel_crtc/crtc/ in intel_crtc_init()
fb54fab06250 drm: Add __drm_atomic_helper_crtc_state_reset() & co.

== Logs ==

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

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

[Souza, Jose]

On Mon, 2019-12-16 at 15:52 -0800, Lucas De Marchi wrote:
> On Mon, Dec 16, 2019 at 02:07:37PM -0800, Jose 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 moved it to a
> > better
> > place intel_atomic.c.
> > 
> > That allowed us to convert the whole
> > intel_encoders_update_prepare/complete to intel types too.
> > 
> > No behavior changes intended here.
> > 
> > v3:
> > - removed digital from exported version of
> > intel_connector_needs_modeset
> > - rollback connector to drm type
> > 
> > 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  | 18 +++++++
> > drivers/gpu/drm/i915/display/intel_atomic.h  |  2 +
> > drivers/gpu/drm/i915/display/intel_display.c | 53 ++++++-----------
> > ---
> > 3 files changed, 36 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..b7dda18b6f29 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -174,6 +174,24 @@ intel_digital_connector_duplicate_state(struct
> > drm_connector *connector)
> > 	return &state->base;
> > }
> > 
> > +/**
> > + * intel_connector_needs_modeset - check if connector needs a
> > modeset
> > + */
> > +bool
> > +intel_connector_needs_modeset(struct intel_atomic_state *state,
> > +			      struct drm_connector *connector)
> > +{
> > +	const struct drm_connector_state *old_conn_state,
> > *new_conn_state;
> > +
> > +	old_conn_state = drm_atomic_get_old_connector_state(&state-
> > >base, connector);
> > +	new_conn_state = drm_atomic_get_new_connector_state(&state-
> > >base, connector);
> > +
> > +	return old_conn_state->crtc != new_conn_state->crtc ||
> > +	       (new_conn_state->crtc &&
> > +		drm_atomic_crtc_needs_modeset(drm_atomic_get_new_crtc_s
> > tate(&state->base,
> > +									
> >     new_conn_state->crtc)));
> > +}
> > +
> > /**
> >  * 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..a7d1a8576c48 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> > @@ -32,6 +32,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_connector_needs_modeset(struct intel_atomic_state
> > *state,
> > +				   struct drm_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 8e3e05cfcb27..0ee2e86a8826 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;
> 
> what's up with this fight new_conn_state vs new_connector_state? It'
> s
> the first time in the driver we would actually drive such conversion.
> I'd say to just keep the old name.

Ville asked me to rename conn to connector in the new functions that
I'm adding as this function was heavily modified I did the same here.
I will rollback if agreed by you and Ville(the ones helping with TGL
MST)

> 
> I'm not sure I like the conversion to use the connector rather than
> the
> connector_state because we are likely to have the state around in the
> caller. Anyway, I'm ok with it.
> 
> Lucas De Marchi
> 
> > +	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_connector_needs_modeset(state, &connector-
> > >base))
> > 			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_connector_needs_modeset(state, &connector-
> > >base))
> > 			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 mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Lucas De Marchi]

On Tue, Dec 17, 2019 at 08:39:15AM -0800, Jose Souza wrote:
>On Mon, 2019-12-16 at 15:52 -0800, Lucas De Marchi wrote:
>> On Mon, Dec 16, 2019 at 02:07:37PM -0800, Jose 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 moved it to a
>> > better
>> > place intel_atomic.c.
>> >
>> > That allowed us to convert the whole
>> > intel_encoders_update_prepare/complete to intel types too.
>> >
>> > No behavior changes intended here.
>> >
>> > v3:
>> > - removed digital from exported version of
>> > intel_connector_needs_modeset
>> > - rollback connector to drm type
>> >
>> > 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  | 18 +++++++
>> > drivers/gpu/drm/i915/display/intel_atomic.h  |  2 +
>> > drivers/gpu/drm/i915/display/intel_display.c | 53 ++++++-----------
>> > ---
>> > 3 files changed, 36 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..b7dda18b6f29 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
>> > @@ -174,6 +174,24 @@ intel_digital_connector_duplicate_state(struct
>> > drm_connector *connector)
>> > 	return &state->base;
>> > }
>> >
>> > +/**
>> > + * intel_connector_needs_modeset - check if connector needs a
>> > modeset
>> > + */
>> > +bool
>> > +intel_connector_needs_modeset(struct intel_atomic_state *state,
>> > +			      struct drm_connector *connector)
>> > +{
>> > +	const struct drm_connector_state *old_conn_state,
>> > *new_conn_state;
>> > +
>> > +	old_conn_state = drm_atomic_get_old_connector_state(&state-
>> > >base, connector);
>> > +	new_conn_state = drm_atomic_get_new_connector_state(&state-
>> > >base, connector);
>> > +
>> > +	return old_conn_state->crtc != new_conn_state->crtc ||
>> > +	       (new_conn_state->crtc &&
>> > +		drm_atomic_crtc_needs_modeset(drm_atomic_get_new_crtc_s
>> > tate(&state->base,
>> > +									
>> >     new_conn_state->crtc)));
>> > +}
>> > +
>> > /**
>> >  * 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..a7d1a8576c48 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
>> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
>> > @@ -32,6 +32,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_connector_needs_modeset(struct intel_atomic_state
>> > *state,
>> > +				   struct drm_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 8e3e05cfcb27..0ee2e86a8826 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;
>>
>> what's up with this fight new_conn_state vs new_connector_state? It'
>> s
>> the first time in the driver we would actually drive such conversion.
>> I'd say to just keep the old name.
>
>Ville asked me to rename conn to connector in the new functions that
>I'm adding as this function was heavily modified I did the same here.
>I will rollback if agreed by you and Ville(the ones helping with TGL
>MST)

I think that was about conn -> connector, not *conn_state ->
*connector_state, wasn't it?

Lucas De Marchi

>
>>
>> I'm not sure I like the conversion to use the connector rather than
>> the
>> connector_state because we are likely to have the state around in the
>> caller. Anyway, I'm ok with it.
>>
>> Lucas De Marchi
>>
>> > +	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_connector_needs_modeset(state, &connector-
>> > >base))
>> > 			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_connector_needs_modeset(state, &connector-
>> > >base))
>> > 			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 mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Tue, Dec 17, 2019 at 09:37:03AM -0800, Lucas De Marchi wrote:
> On Tue, Dec 17, 2019 at 08:39:15AM -0800, Jose Souza wrote:
> >On Mon, 2019-12-16 at 15:52 -0800, Lucas De Marchi wrote:
> >> On Mon, Dec 16, 2019 at 02:07:37PM -0800, Jose 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 moved it to a
> >> > better
> >> > place intel_atomic.c.
> >> >
> >> > That allowed us to convert the whole
> >> > intel_encoders_update_prepare/complete to intel types too.
> >> >
> >> > No behavior changes intended here.
> >> >
> >> > v3:
> >> > - removed digital from exported version of
> >> > intel_connector_needs_modeset
> >> > - rollback connector to drm type
> >> >
> >> > 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  | 18 +++++++
> >> > drivers/gpu/drm/i915/display/intel_atomic.h  |  2 +
> >> > drivers/gpu/drm/i915/display/intel_display.c | 53 ++++++-----------
> >> > ---
> >> > 3 files changed, 36 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..b7dda18b6f29 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> >> > @@ -174,6 +174,24 @@ intel_digital_connector_duplicate_state(struct
> >> > drm_connector *connector)
> >> > 	return &state->base;
> >> > }
> >> >
> >> > +/**
> >> > + * intel_connector_needs_modeset - check if connector needs a
> >> > modeset
> >> > + */
> >> > +bool
> >> > +intel_connector_needs_modeset(struct intel_atomic_state *state,
> >> > +			      struct drm_connector *connector)
> >> > +{
> >> > +	const struct drm_connector_state *old_conn_state,
> >> > *new_conn_state;
> >> > +
> >> > +	old_conn_state = drm_atomic_get_old_connector_state(&state-
> >> > >base, connector);
> >> > +	new_conn_state = drm_atomic_get_new_connector_state(&state-
> >> > >base, connector);
> >> > +
> >> > +	return old_conn_state->crtc != new_conn_state->crtc ||
> >> > +	       (new_conn_state->crtc &&
> >> > +		drm_atomic_crtc_needs_modeset(drm_atomic_get_new_crtc_s
> >> > tate(&state->base,
> >> > +									
> >> >     new_conn_state->crtc)));
> >> > +}
> >> > +
> >> > /**
> >> >  * 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..a7d1a8576c48 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> >> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> >> > @@ -32,6 +32,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_connector_needs_modeset(struct intel_atomic_state
> >> > *state,
> >> > +				   struct drm_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 8e3e05cfcb27..0ee2e86a8826 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;
> >>
> >> what's up with this fight new_conn_state vs new_connector_state? It'
> >> s
> >> the first time in the driver we would actually drive such conversion.
> >> I'd say to just keep the old name.
> >
> >Ville asked me to rename conn to connector in the new functions that
> >I'm adding as this function was heavily modified I did the same here.
> >I will rollback if agreed by you and Ville(the ones helping with TGL
> >MST)
> 
> I think that was about conn -> connector, not *conn_state ->
> *connector_state, wasn't it?

I think we generally have full 'connector' and abbreviated
'conn_state'. A bit inconsistent in itself but IMO still better
to stick to common naming conventions so the poor brain has an
easier job when reading code. We could perhaps do a
s/connector/conn/ over the whole codebase to make things a bit
shorter in a bunch of places.

-- 
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 v3 06/11] drm/i915/display: Share intel_connector_needs_modeset()

[Ville Syrjälä]

On Mon, Dec 16, 2019 at 02:07:37PM -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 moved it to a better
> place intel_atomic.c.
> 
> That allowed us to convert the whole
> intel_encoders_update_prepare/complete to intel types too.
> 
> No behavior changes intended here.
> 
> v3:
> - removed digital from exported version of intel_connector_needs_modeset
> - rollback connector to drm type
> 
> 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  | 18 +++++++
>  drivers/gpu/drm/i915/display/intel_atomic.h  |  2 +
>  drivers/gpu/drm/i915/display/intel_display.c | 53 ++++++--------------
>  3 files changed, 36 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..b7dda18b6f29 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -174,6 +174,24 @@ intel_digital_connector_duplicate_state(struct drm_connector *connector)
>  	return &state->base;
>  }
>  
> +/**
> + * intel_connector_needs_modeset - check if connector needs a modeset
> + */
> +bool
> +intel_connector_needs_modeset(struct intel_atomic_state *state,
> +			      struct drm_connector *connector)
> +{
> +	const struct drm_connector_state *old_conn_state, *new_conn_state;
> +
> +	old_conn_state = drm_atomic_get_old_connector_state(&state->base, connector);
> +	new_conn_state = drm_atomic_get_new_connector_state(&state->base, connector);
> +
> +	return old_conn_state->crtc != new_conn_state->crtc ||
> +	       (new_conn_state->crtc &&
> +		drm_atomic_crtc_needs_modeset(drm_atomic_get_new_crtc_state(&state->base,
> +									    new_conn_state->crtc)));
> +}
> +
>  /**
>   * 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..a7d1a8576c48 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> @@ -32,6 +32,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_connector_needs_modeset(struct intel_atomic_state *state,
> +				   struct drm_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 8e3e05cfcb27..0ee2e86a8826 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;

Not sure why we changed the types here. I suppose it works if we're
careful in what we access in the connector state, but IIRC not
all connectors use intel_digital_connector_state so this could
potetially trip someone up in the future. So I'd leave these alone.

The movement of the function itself is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


>  	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_connector_needs_modeset(state, &connector->base))
>  			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_connector_needs_modeset(state, &connector->base))
>  			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 v3 08/11] drm/i915/display: Always enables MST master pipe first

[Ville Syrjälä]

On Mon, Dec 16, 2019 at 02:07:39PM -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.
> 
> v3: rebased
> 
> 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 | 88 +++++++++++++++-----
>  1 file changed, 65 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index e976f82a0db7..176e31de68e7 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14553,15 +14553,20 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  	u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
>  	u8 required_slices = state->wm_results.ddb.enabled_slices;
>  	struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
> -	u8 dirty_pipes = 0;
> +	u8 update_pipes = 0, modeset_pipes = 0;
>  	int i;
>  
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		if (!new_crtc_state->hw.active)
> +			continue;
> +
>  		/* ignore allocations for crtc's that have been turned off. */
> -		if (!needs_modeset(new_crtc_state) && new_crtc_state->hw.active)
> +		if (!needs_modeset(new_crtc_state)) {
>  			entries[i] = old_crtc_state->wm.skl.ddb;
> -		if (new_crtc_state->hw.active)
> -			dirty_pipes |= BIT(crtc->pipe);
> +			update_pipes |= BIT(crtc->pipe);
> +		} else {
> +			modeset_pipes |= BIT(modeset_pipes);
> +		}
>  	}
>  
>  	/* If 2nd DBuf slice required, enable it here */
> @@ -14571,16 +14576,18 @@ 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
>  	 */
> -	while (dirty_pipes) {
> +	while (update_pipes) {
>  		for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  						    new_crtc_state, i) {
>  			enum pipe pipe = crtc->pipe;
> -			bool modeset = needs_modeset(new_crtc_state);
>  
> -			if ((dirty_pipes & BIT(pipe)) == 0)
> +			if ((update_pipes & BIT(pipe)) == 0)
>  				continue;
>  
>  			if (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> @@ -14589,20 +14596,10 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  				continue;
>  
>  			entries[i] = new_crtc_state->wm.skl.ddb;
> -			dirty_pipes &= ~BIT(pipe);
> -
> -			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);
> -			}
> +			update_pipes &= ~BIT(pipe);
> +
> +			intel_update_crtc(crtc, state, old_crtc_state,
> +					  new_crtc_state);
>  
>  			/*
>  			 * If this is an already active pipe, it's DDB changed,
> @@ -14612,11 +14609,56 @@ 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 && dirty_pipes)
> +			    update_pipes)

Needs to be update_pipes|modeset_pipes.

>  				intel_wait_for_vblank(dev_priv, pipe);
> +

Extra newline.

>  		}
>  	}
>  
> +	/*
> +	 * 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) {
> +		enum pipe pipe = crtc->pipe;
> +
> +		if ((modeset_pipes & BIT(pipe)) == 0)
> +			continue;
> +
> +		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> +		    is_trans_port_sync_slave(new_crtc_state))
> +			continue;
> +
> +		modeset_pipes &= ~BIT(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) {
> +		enum pipe pipe = crtc->pipe;
> +
> +		if ((modeset_pipes & BIT(pipe)) == 0)
> +			continue;
> +
> +		if (is_trans_port_sync_slave(new_crtc_state))
> +			continue;

If we clear the modeset_pipes bit for these in the previous loop we
could avoid this check here entirely.

Still missing the DDB overlap WARNs that I think would be good to
have in these two enable loops.

Looks nice otherwise.

> +
> +		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 v3 10/11] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Ville Syrjälä]

On Mon, Dec 16, 2019 at 02:07:41PM -0800, José Roberto de Souza wrote:
> Check if fastset is allowed by external dependencies like other pipes
> and transcoders.
> 
> Right now this patch only forces a fullmodeset in MST slaves of MST
> masters that needs a fullmodeset but it will be needed for port sync
> as well.
> 
> v3:
> - moved handling to intel_atomic_check() this way is guarantee that
> all pipes will have its state computed
> 
> 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 | 63 ++++++++++++++++----
>  1 file changed, 50 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index f1c0687bf69b..2627a7bcb45c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13941,19 +13941,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
>  
>  	new_crtc_state->uapi.mode_changed = false;
>  	new_crtc_state->update_pipe = true;
> -
> -	/*
> -	 * If we're not doing the full modeset we want to
> -	 * keep the current M/N values as they may be
> -	 * sufficiently different to the computed values
> -	 * to cause problems.
> -	 *
> -	 * FIXME: should really copy more fuzzy state here
> -	 */
> -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
>  }
>  
>  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> @@ -14121,6 +14108,56 @@ static int intel_atomic_check(struct drm_device *dev,
>  			any_ms = true;
>  	}
>  
> +	/**
> +	 * 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.
> +	 */
> +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		struct intel_crtc_state *new_crtc_state_iter;
> +		struct intel_crtc *crtc_iter;
> +		int j;
> +
> +		if (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
> +		    needs_modeset(new_crtc_state))

shouldn't this be
if (!mst_master || !needs_modeset)
	continue;
?

> +			continue;
> +
> +		for_each_new_intel_crtc_in_state(state, crtc_iter,
> +						 new_crtc_state_iter, j) {
> +			if (new_crtc_state->mst_master_transcoder !=
> +			    new_crtc_state_iter->cpu_transcoder)
> +				continue;
> +
> +			if (needs_modeset(new_crtc_state_iter)) {
> +				new_crtc_state->uapi.mode_changed = true;
> +				new_crtc_state->update_pipe = false;
> +			}
> +			break;

Hmm. So I guess the break here is because we're going to keep iterating
the outer loop anyway? Don't think that works with the s/slave/master/
fix above.

I suggest moving this inner loop into a separate function entirely.
Could simply pass in the master_transcoder to that function and then
we wouldn't even have this clash with variable names for the two
crtcs/crtc_states.

Somethig like (with better funciton names ofc :)

mst_modeset_slaves(state, master_transcoder)
{
	for_each() {
		if (master_transcoder == crtc_state->master_transcoder)
			modeset=1;
	}
}

atomic_check()
{
	for_each()
		compute();

	for_each() {
		if (master && needs_modeset()
			mst_modeset_slaves(state, crtc_state->cpu_transcoder);
	}

	for_each() {
		if (fastset)
			copy();
	}
}

Or do we also need to force a modeset for the master if a slave needs a
modeset? I'm trying to think if there's a way that we could end up with
a slave changing its master (thus needing modeset) without the master
being flagged for modeset...

> +		}
> +	}
> +
> +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +					    new_crtc_state, i) {
> +		if (needs_modeset(new_crtc_state))
> +			continue;
> +
> +		/*
> +		 * If we're not doing the full modeset we want to
> +		 * keep the current M/N values as they may be
> +		 * sufficiently different to the computed values
> +		 * to cause problems.
> +		 *
> +		 * FIXME: should really copy more fuzzy state here
> +		 */
> +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;

A separate function for this fastset state copy might also be nice so we
don't have to see these mundane details in this higher level code.

> +	}
> +
>  	if (any_ms && !check_digital_port_conflicts(state)) {
>  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
>  		ret = EINVAL;
> -- 
> 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 v3 10/11] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Ville Syrjälä]

On Wed, Dec 18, 2019 at 07:00:19PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 16, 2019 at 02:07:41PM -0800, José Roberto de Souza wrote:
> > Check if fastset is allowed by external dependencies like other pipes
> > and transcoders.
> > 
> > Right now this patch only forces a fullmodeset in MST slaves of MST
> > masters that needs a fullmodeset but it will be needed for port sync
> > as well.
> > 
> > v3:
> > - moved handling to intel_atomic_check() this way is guarantee that
> > all pipes will have its state computed
> > 
> > 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 | 63 ++++++++++++++++----
> >  1 file changed, 50 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index f1c0687bf69b..2627a7bcb45c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -13941,19 +13941,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
> >  
> >  	new_crtc_state->uapi.mode_changed = false;
> >  	new_crtc_state->update_pipe = true;
> > -
> > -	/*
> > -	 * If we're not doing the full modeset we want to
> > -	 * keep the current M/N values as they may be
> > -	 * sufficiently different to the computed values
> > -	 * to cause problems.
> > -	 *
> > -	 * FIXME: should really copy more fuzzy state here
> > -	 */
> > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> >  }
> >  
> >  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> > @@ -14121,6 +14108,56 @@ static int intel_atomic_check(struct drm_device *dev,
> >  			any_ms = true;
> >  	}
> >  
> > +	/**
> > +	 * 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.
> > +	 */
> > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > +		struct intel_crtc_state *new_crtc_state_iter;
> > +		struct intel_crtc *crtc_iter;
> > +		int j;
> > +
> > +		if (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > +		    needs_modeset(new_crtc_state))
> 
> shouldn't this be
> if (!mst_master || !needs_modeset)
> 	continue;
> ?
> 
> > +			continue;
> > +
> > +		for_each_new_intel_crtc_in_state(state, crtc_iter,
> > +						 new_crtc_state_iter, j) {
> > +			if (new_crtc_state->mst_master_transcoder !=
> > +			    new_crtc_state_iter->cpu_transcoder)
> > +				continue;
> > +
> > +			if (needs_modeset(new_crtc_state_iter)) {
> > +				new_crtc_state->uapi.mode_changed = true;
> > +				new_crtc_state->update_pipe = false;
> > +			}
> > +			break;
> 
> Hmm. So I guess the break here is because we're going to keep iterating
> the outer loop anyway? Don't think that works with the s/slave/master/
> fix above.
> 
> I suggest moving this inner loop into a separate function entirely.
> Could simply pass in the master_transcoder to that function and then
> we wouldn't even have this clash with variable names for the two
> crtcs/crtc_states.
> 
> Somethig like (with better funciton names ofc :)
> 
> mst_modeset_slaves(state, master_transcoder)
> {
> 	for_each() {
> 		if (master_transcoder == crtc_state->master_transcoder)
> 			modeset=1;
> 	}
> }
> 
> atomic_check()
> {
> 	for_each()
> 		compute();
> 
> 	for_each() {
> 		if (master && needs_modeset()
> 			mst_modeset_slaves(state, crtc_state->cpu_transcoder);
> 	}
> 
> 	for_each() {
> 		if (fastset)
> 			copy();
> 	}
> }
> 
> Or do we also need to force a modeset for the master if a slave needs a
> modeset? I'm trying to think if there's a way that we could end up with
> a slave changing its master (thus needing modeset) without the master
> being flagged for modeset...

How about this:

1. Enable pipe B (master) + pipe C (slave)
2. Enable pipe A (should become master)

Hmm. Scratch that. Pipe B will have it's master_transcoder change from
INVALID to A so it'll get flagged for a modeset. Seems I need a more
heavy duty thinking cap for this one...

-- 
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 v3 06/11] drm/i915/display: Share intel_connector_needs_modeset()

[Souza, Jose]

On Wed, 2019-12-18 at 18:38 +0200, Ville Syrjälä wrote:
> On Mon, Dec 16, 2019 at 02:07:37PM -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 moved it to a
> > better
> > place intel_atomic.c.
> > 
> > That allowed us to convert the whole
> > intel_encoders_update_prepare/complete to intel types too.
> > 
> > No behavior changes intended here.
> > 
> > v3:
> > - removed digital from exported version of
> > intel_connector_needs_modeset
> > - rollback connector to drm type
> > 
> > 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  | 18 +++++++
> >  drivers/gpu/drm/i915/display/intel_atomic.h  |  2 +
> >  drivers/gpu/drm/i915/display/intel_display.c | 53 ++++++--------
> > ------
> >  3 files changed, 36 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..b7dda18b6f29 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -174,6 +174,24 @@ intel_digital_connector_duplicate_state(struct
> > drm_connector *connector)
> >  	return &state->base;
> >  }
> >  
> > +/**
> > + * intel_connector_needs_modeset - check if connector needs a
> > modeset
> > + */
> > +bool
> > +intel_connector_needs_modeset(struct intel_atomic_state *state,
> > +			      struct drm_connector *connector)
> > +{
> > +	const struct drm_connector_state *old_conn_state,
> > *new_conn_state;
> > +
> > +	old_conn_state = drm_atomic_get_old_connector_state(&state-
> > >base, connector);
> > +	new_conn_state = drm_atomic_get_new_connector_state(&state-
> > >base, connector);
> > +
> > +	return old_conn_state->crtc != new_conn_state->crtc ||
> > +	       (new_conn_state->crtc &&
> > +		drm_atomic_crtc_needs_modeset(drm_atomic_get_new_crtc_s
> > tate(&state->base,
> > +									
> >     new_conn_state->crtc)));
> > +}
> > +
> >  /**
> >   * 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..a7d1a8576c48 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> > @@ -32,6 +32,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_connector_needs_modeset(struct intel_atomic_state
> > *state,
> > +				   struct drm_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 8e3e05cfcb27..0ee2e86a8826 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;
> 
> Not sure why we changed the types here. I suppose it works if we're
> careful in what we access in the connector state, but IIRC not
> all connectors use intel_digital_connector_state so this could
> potetially trip someone up in the future. So I'd leave these alone.

Okay rolling back to drm_connector_state

> 
> The movement of the function itself is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 
> >  	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_connector_needs_modeset(state, &connector-
> > >base))
> >  			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_connector_needs_modeset(state, &connector-
> > >base))
> >  			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 v3 08/11] drm/i915/display: Always enables MST master pipe first

[Souza, Jose]

On Wed, 2019-12-18 at 18:43 +0200, Ville Syrjälä wrote:
> On Mon, Dec 16, 2019 at 02:07:39PM -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.
> > 
> > v3: rebased
> > 
> > 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 | 88 +++++++++++++++-
> > ----
> >  1 file changed, 65 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index e976f82a0db7..176e31de68e7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14553,15 +14553,20 @@ static void
> > skl_commit_modeset_enables(struct intel_atomic_state *state)
> >  	u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
> >  	u8 required_slices = state->wm_results.ddb.enabled_slices;
> >  	struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
> > -	u8 dirty_pipes = 0;
> > +	u8 update_pipes = 0, modeset_pipes = 0;
> >  	int i;
> >  
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state, new_crtc_state, i) {
> > +		if (!new_crtc_state->hw.active)
> > +			continue;
> > +
> >  		/* ignore allocations for crtc's that have been turned
> > off. */
> > -		if (!needs_modeset(new_crtc_state) && new_crtc_state-
> > >hw.active)
> > +		if (!needs_modeset(new_crtc_state)) {
> >  			entries[i] = old_crtc_state->wm.skl.ddb;
> > -		if (new_crtc_state->hw.active)
> > -			dirty_pipes |= BIT(crtc->pipe);
> > +			update_pipes |= BIT(crtc->pipe);
> > +		} else {
> > +			modeset_pipes |= BIT(modeset_pipes);
> > +		}
> >  	}
> >  
> >  	/* If 2nd DBuf slice required, enable it here */
> > @@ -14571,16 +14576,18 @@ 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
> >  	 */
> > -	while (dirty_pipes) {
> > +	while (update_pipes) {
> >  		for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> >  						    new_crtc_state, i)
> > {
> >  			enum pipe pipe = crtc->pipe;
> > -			bool modeset = needs_modeset(new_crtc_state);
> >  
> > -			if ((dirty_pipes & BIT(pipe)) == 0)
> > +			if ((update_pipes & BIT(pipe)) == 0)
> >  				continue;
> >  
> >  			if
> > (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> > @@ -14589,20 +14596,10 @@ static void
> > skl_commit_modeset_enables(struct intel_atomic_state *state)
> >  				continue;
> >  
> >  			entries[i] = new_crtc_state->wm.skl.ddb;
> > -			dirty_pipes &= ~BIT(pipe);
> > -
> > -			if (modeset &&
> > is_trans_port_sync_mode(new_crtc_state)) {
> > -				if
> > (is_trans_port_sync_master(new_crtc_state))
> > -					intel_update_trans_port_sync_cr
> > tcs(crtc,
> > -									
> >    state,
> > -									
> >    old_crtc_state,
> > -									
> >    new_crtc_state);
> > -				else
> > -					continue;
> > -			} else {
> > -				intel_update_crtc(crtc, state,
> > old_crtc_state,
> > -						  new_crtc_state);
> > -			}
> > +			update_pipes &= ~BIT(pipe);
> > +
> > +			intel_update_crtc(crtc, state, old_crtc_state,
> > +					  new_crtc_state);
> >  
> >  			/*
> >  			 * If this is an already active pipe, it's DDB
> > changed,
> > @@ -14612,11 +14609,56 @@ 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 && dirty_pipes)
> > +			    update_pipes)
> 
> Needs to be update_pipes|modeset_pipes.

Okay

> 
> >  				intel_wait_for_vblank(dev_priv, pipe);
> > +
> 
> Extra newline.
> 
> >  		}
> >  	}
> >  
> > +	/*
> > +	 * 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) {
> > +		enum pipe pipe = crtc->pipe;
> > +
> > +		if ((modeset_pipes & BIT(pipe)) == 0)
> > +			continue;
> > +
> > +		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > +		    is_trans_port_sync_slave(new_crtc_state))
> > +			continue;
> > +
> > +		modeset_pipes &= ~BIT(pipe);
> > +
> > +		if (is_trans_port_sync_mode(new_crtc_state))
> > +			intel_update_trans_port_sync_crtcs(crtc, state,
> > +							   old_crtc_sta
> > te,
> > +							   new_crtc_sta
> > te);
> > +		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) {
> > +		enum pipe pipe = crtc->pipe;
> > +
> > +		if ((modeset_pipes & BIT(pipe)) == 0)
> > +			continue;
> > +
> > +		if (is_trans_port_sync_slave(new_crtc_state))
> > +			continue;
> 
> If we clear the modeset_pipes bit for these in the previous loop we
> could avoid this check here entirely.

For me this is cleaner and remind us to rework
intel_update_trans_port_sync_crtcs() otherwise to do what you suggested
the loop above will need.

struct intel_crtc *slave_crtc = intel_get_slave_crtc(new_crtc_state);

modeset_pipes &= ~BIT(slave_crtc->pipe);
I'm fine with both ways.

> 
> Still missing the DDB overlap WARNs that I think would be good to
> have in these two enable loops.

Why add the warning in those 2 loops? Those are pipes that are disabled at this point so they will not overlap.

If you really want, this will be enough?

WARN_ON(skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
entries, INTEL_NUM_PIPES(dev_priv), i)) 

> 
> Looks nice otherwise.
> 
> > +
> > +		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

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

[Souza, Jose]

On Wed, 2019-12-18 at 19:10 +0200, Ville Syrjälä wrote:
> On Wed, Dec 18, 2019 at 07:00:19PM +0200, Ville Syrjälä wrote:
> > On Mon, Dec 16, 2019 at 02:07:41PM -0800, José Roberto de Souza
> > wrote:
> > > Check if fastset is allowed by external dependencies like other
> > > pipes
> > > and transcoders.
> > > 
> > > Right now this patch only forces a fullmodeset in MST slaves of
> > > MST
> > > masters that needs a fullmodeset but it will be needed for port
> > > sync
> > > as well.
> > > 
> > > v3:
> > > - moved handling to intel_atomic_check() this way is guarantee
> > > that
> > > all pipes will have its state computed
> > > 
> > > 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 | 63
> > > ++++++++++++++++----
> > >  1 file changed, 50 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index f1c0687bf69b..2627a7bcb45c 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -13941,19 +13941,6 @@ static void
> > > intel_crtc_check_fastset(const struct intel_crtc_state
> > > *old_crtc_sta
> > >  
> > >  	new_crtc_state->uapi.mode_changed = false;
> > >  	new_crtc_state->update_pipe = true;
> > > -
> > > -	/*
> > > -	 * If we're not doing the full modeset we want to
> > > -	 * keep the current M/N values as they may be
> > > -	 * sufficiently different to the computed values
> > > -	 * to cause problems.
> > > -	 *
> > > -	 * FIXME: should really copy more fuzzy state here
> > > -	 */
> > > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > >  }
> > >  
> > >  static int intel_crtc_add_planes_to_state(struct
> > > intel_atomic_state *state,
> > > @@ -14121,6 +14108,56 @@ static int intel_atomic_check(struct
> > > drm_device *dev,
> > >  			any_ms = true;
> > >  	}
> > >  
> > > +	/**
> > > +	 * 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.
> > > +	 */
> > > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > > i) {
> > > +		struct intel_crtc_state *new_crtc_state_iter;
> > > +		struct intel_crtc *crtc_iter;
> > > +		int j;
> > > +
> > > +		if (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > > +		    needs_modeset(new_crtc_state))
> > 
> > shouldn't this be
> > if (!mst_master || !needs_modeset)
> > 	continue;

When iterating over MST pipes in older gens
intel_dp_mst_is_master_trans() and intel_dp_mst_is_slave_trans() will
always return false and if this one is intel_dp_mst_is_master_trans()
we can skip it.
In case it is a intel_dp_mst_is_slave_trans() and if it needs a modeset
already we don't need to force anything.


> > ?
> > 
> > > +			continue;
> > > +
> > > +		for_each_new_intel_crtc_in_state(state, crtc_iter,
> > > +						 new_crtc_state_iter,
> > > j) {
> > > +			if (new_crtc_state->mst_master_transcoder !=
> > > +			    new_crtc_state_iter->cpu_transcoder)
> > > +				continue;
> > > +
> > > +			if (needs_modeset(new_crtc_state_iter)) {
> > > +				new_crtc_state->uapi.mode_changed =
> > > true;
> > > +				new_crtc_state->update_pipe = false;
> > > +			}
> > > +			break;
> > 
> > Hmm. So I guess the break here is because we're going to keep
> > iterating
> > the outer loop anyway? Don't think that works with the
> > s/slave/master/
> > fix above.

This inner loop is to find the master of the pipe being check, when we
find it we can break and process the next pipe.

> > 
> > I suggest moving this inner loop into a separate function entirely.
> > Could simply pass in the master_transcoder to that function and
> > then
> > we wouldn't even have this clash with variable names for the two
> > crtcs/crtc_states.
> > 
> > Somethig like (with better funciton names ofc :)
> > 
> > mst_modeset_slaves(state, master_transcoder)
> > {
> > 	for_each() {
> > 		if (master_transcoder == crtc_state->master_transcoder)
> > 			modeset=1;
> > 	}
> > }

Sounds good the function above.

> > 
> > atomic_check()
> > {
> > 	for_each()
> > 		compute();
> > 
> > 	for_each() {
> > 		if (master && needs_modeset()
> > 			mst_modeset_slaves(state, crtc_state-
> > >cpu_transcoder);
> > 	}
> > 
> > 	for_each() {
> > 		if (fastset)
> > 			copy();
> > 	}
> > }
> > 
> > Or do we also need to force a modeset for the master if a slave
> > needs a
> > modeset? I'm trying to think if there's a way that we could end up
> > with
> > a slave changing its master (thus needing modeset) without the
> > master
> > being flagged for modeset...
> 
> How about this:
> 
> 1. Enable pipe B (master) + pipe C (slave)
> 2. Enable pipe A (should become master)
> 
> Hmm. Scratch that. Pipe B will have it's master_transcoder change
> from
> INVALID to A so it'll get flagged for a modeset. Seems I need a more
> heavy duty thinking cap for this one...
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Wed, Dec 18, 2019 at 05:27:00PM +0000, Souza, Jose wrote:
> On Wed, 2019-12-18 at 18:43 +0200, Ville Syrjälä wrote:
> > On Mon, Dec 16, 2019 at 02:07:39PM -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.
> > > 
> > > v3: rebased
> > > 
> > > 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 | 88 +++++++++++++++-
> > > ----
> > >  1 file changed, 65 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index e976f82a0db7..176e31de68e7 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14553,15 +14553,20 @@ static void
> > > skl_commit_modeset_enables(struct intel_atomic_state *state)
> > >  	u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
> > >  	u8 required_slices = state->wm_results.ddb.enabled_slices;
> > >  	struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
> > > -	u8 dirty_pipes = 0;
> > > +	u8 update_pipes = 0, modeset_pipes = 0;
> > >  	int i;
> > >  
> > >  	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > old_crtc_state, new_crtc_state, i) {
> > > +		if (!new_crtc_state->hw.active)
> > > +			continue;
> > > +
> > >  		/* ignore allocations for crtc's that have been turned
> > > off. */
> > > -		if (!needs_modeset(new_crtc_state) && new_crtc_state-
> > > >hw.active)
> > > +		if (!needs_modeset(new_crtc_state)) {
> > >  			entries[i] = old_crtc_state->wm.skl.ddb;
> > > -		if (new_crtc_state->hw.active)
> > > -			dirty_pipes |= BIT(crtc->pipe);
> > > +			update_pipes |= BIT(crtc->pipe);
> > > +		} else {
> > > +			modeset_pipes |= BIT(modeset_pipes);
> > > +		}
> > >  	}
> > >  
> > >  	/* If 2nd DBuf slice required, enable it here */
> > > @@ -14571,16 +14576,18 @@ 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
> > >  	 */
> > > -	while (dirty_pipes) {
> > > +	while (update_pipes) {
> > >  		for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > old_crtc_state,
> > >  						    new_crtc_state, i)
> > > {
> > >  			enum pipe pipe = crtc->pipe;
> > > -			bool modeset = needs_modeset(new_crtc_state);
> > >  
> > > -			if ((dirty_pipes & BIT(pipe)) == 0)
> > > +			if ((update_pipes & BIT(pipe)) == 0)
> > >  				continue;
> > >  
> > >  			if
> > > (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> > > @@ -14589,20 +14596,10 @@ static void
> > > skl_commit_modeset_enables(struct intel_atomic_state *state)
> > >  				continue;
> > >  
> > >  			entries[i] = new_crtc_state->wm.skl.ddb;
> > > -			dirty_pipes &= ~BIT(pipe);
> > > -
> > > -			if (modeset &&
> > > is_trans_port_sync_mode(new_crtc_state)) {
> > > -				if
> > > (is_trans_port_sync_master(new_crtc_state))
> > > -					intel_update_trans_port_sync_cr
> > > tcs(crtc,
> > > -									
> > >    state,
> > > -									
> > >    old_crtc_state,
> > > -									
> > >    new_crtc_state);
> > > -				else
> > > -					continue;
> > > -			} else {
> > > -				intel_update_crtc(crtc, state,
> > > old_crtc_state,
> > > -						  new_crtc_state);
> > > -			}
> > > +			update_pipes &= ~BIT(pipe);
> > > +
> > > +			intel_update_crtc(crtc, state, old_crtc_state,
> > > +					  new_crtc_state);
> > >  
> > >  			/*
> > >  			 * If this is an already active pipe, it's DDB
> > > changed,
> > > @@ -14612,11 +14609,56 @@ 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 && dirty_pipes)
> > > +			    update_pipes)
> > 
> > Needs to be update_pipes|modeset_pipes.
> 
> Okay
> 
> > 
> > >  				intel_wait_for_vblank(dev_priv, pipe);
> > > +
> > 
> > Extra newline.
> > 
> > >  		}
> > >  	}
> > >  
> > > +	/*
> > > +	 * 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) {
> > > +		enum pipe pipe = crtc->pipe;
> > > +
> > > +		if ((modeset_pipes & BIT(pipe)) == 0)
> > > +			continue;
> > > +
> > > +		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > > +		    is_trans_port_sync_slave(new_crtc_state))
> > > +			continue;
> > > +
> > > +		modeset_pipes &= ~BIT(pipe);
> > > +
> > > +		if (is_trans_port_sync_mode(new_crtc_state))
> > > +			intel_update_trans_port_sync_crtcs(crtc, state,
> > > +							   old_crtc_sta
> > > te,
> > > +							   new_crtc_sta
> > > te);
> > > +		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) {
> > > +		enum pipe pipe = crtc->pipe;
> > > +
> > > +		if ((modeset_pipes & BIT(pipe)) == 0)
> > > +			continue;
> > > +
> > > +		if (is_trans_port_sync_slave(new_crtc_state))
> > > +			continue;
> > 
> > If we clear the modeset_pipes bit for these in the previous loop we
> > could avoid this check here entirely.
> 
> For me this is cleaner and remind us to rework
> intel_update_trans_port_sync_crtcs() otherwise to do what you suggested
> the loop above will need.
> 
> struct intel_crtc *slave_crtc = intel_get_slave_crtc(new_crtc_state);
> 
> modeset_pipes &= ~BIT(slave_crtc->pipe);
> I'm fine with both ways.

I can live with either one. Though we should perhaps remove the bit
from the mask at some point, and then we could
WARN_ON(modeset_pipes != 0) at the very end.

> 
> > 
> > Still missing the DDB overlap WARNs that I think would be good to
> > have in these two enable loops.
> 
> Why add the warning in those 2 loops? Those are pipes that are disabled at this point so they will not overlap.

They could overlap with already enabled pipes if we either somehow
failed to update all already enabled pipes, or we miscomputed the
ddb entirely. I think for this a bit of self checking is in order.

> 
> If you really want, this will be enough?
> 
> WARN_ON(skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> entries, INTEL_NUM_PIPES(dev_priv), i)) 

+ entries[i] = new_crtc_state->ddb; 

so we'll also catch any overlaps between pipes undergoing modesets.

> 
> > 
> > Looks nice otherwise.
> > 
> > > +
> > > +		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