[PATCH 0/9] drm/atomic: Add encoder_mask and connector_mask to crtc_state.

[PATCH 0/9] drm/atomic: Add encoder_mask and connector_mask to crtc_state.

[Maarten Lankhorst]

First 2 patches are fixes for a missing connector_state->connector. The atomic code
doesn't use it currently but with these patches it causes a null pointer dereference.

It's useful to know from the crtc_state what connectors and encoders are attached.
This makes it possible to do iterations over connectors and encoders with just the
crtc_state.

The drm/tegra patch is compile tested only.

Maarten Lankhorst (9):
  drm/i915: Set connector_state->connector correctly.
  drm/tegra: Assign conn_state->connector when allocating connector
    state.
  drm/core: Add drm_encoder_index.
  drm/core: Add drm_for_each_encoder_mask.
  drm/atomic: add connector mask to drm_crtc_state.
  drm/i915: Update connector_mask during readout.
  drm/atomic: Small documentation fix.
  drm/atomic: Remove drm_atomic_connectors_for_crtc.
  drm/atomic: Add encoder_mask to crtc_state.

 drivers/gpu/drm/drm_atomic.c         | 42 +++++++++++-------------------------
 drivers/gpu/drm/drm_atomic_helper.c  | 13 ++++++-----
 drivers/gpu/drm/drm_crtc.c           | 23 ++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c | 16 +++++++++++---
 drivers/gpu/drm/tegra/dsi.c          |  4 +++-
 drivers/gpu/drm/vc4/vc4_crtc.c       |  2 +-
 include/drm/drm_atomic.h             |  4 ----
 include/drm/drm_crtc.h               | 17 +++++++++++++++
 8 files changed, 75 insertions(+), 46 deletions(-)

-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/9] drm/i915: Set connector_state->connector correctly.

[Maarten Lankhorst]

I can't believe we didn't trip over this sooner..

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2917fef33f31..dcc7ec7665c2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6461,6 +6461,8 @@ int intel_connector_init(struct intel_connector *connector)
 	if (!connector_state)
 		return -ENOMEM;
 
+	connector_state->connector = &connector->base;
+
 	connector->base.state = connector_state;
 	return 0;
 }
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/9] drm/tegra: Assign conn_state->connector when allocating connector state.

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/tegra/dsi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index f0a138ef68ce..6b8ae3d08eeb 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -751,8 +751,10 @@ static void tegra_dsi_connector_reset(struct drm_connector *connector)
 	connector->state = NULL;
 
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (state)
+	if (state) {
+		state->base.connector = connector;
 		connector->state = &state->base;
+	}
 }
 
 static struct drm_connector_state *
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/9] drm/core: Add drm_encoder_index.

[Maarten Lankhorst]

This is useful for adding encoder_mask in crtc_state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 23 +++++++++++++++++++++++
 include/drm/drm_crtc.h     |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 32dd134700bd..904a34c560f4 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1121,6 +1121,29 @@ out_unlock:
 EXPORT_SYMBOL(drm_encoder_init);
 
 /**
+ * drm_encoder_index - find the index of a registered encoder
+ * @encoder: encoder to find index for
+ *
+ * Given a registered encoder, return the index of that encoder within a DRM
+ * device's list of encoders.
+ */
+unsigned int drm_encoder_index(struct drm_encoder *encoder)
+{
+	unsigned int index = 0;
+	struct drm_encoder *tmp;
+
+	drm_for_each_encoder(tmp, encoder->dev) {
+		if (tmp == encoder)
+			return index;
+
+		index++;
+	}
+
+	BUG();
+}
+EXPORT_SYMBOL(drm_encoder_index);
+
+/**
  * drm_encoder_cleanup - cleans up an initialised encoder
  * @encoder: encoder to cleanup
  *
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 173535a35d65..29cfb4f8f99d 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1241,6 +1241,7 @@ extern int drm_encoder_init(struct drm_device *dev,
 			    struct drm_encoder *encoder,
 			    const struct drm_encoder_funcs *funcs,
 			    int encoder_type);
+extern unsigned int drm_encoder_index(struct drm_encoder *encoder);
 
 /**
  * drm_encoder_crtc_ok - can a given crtc drive a given encoder?
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/9] drm/core: Add drm_for_each_encoder_mask.

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 include/drm/drm_crtc.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 29cfb4f8f99d..c54da2d297ec 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1172,6 +1172,17 @@ struct drm_mode_config {
 	list_for_each_entry((plane), &(dev)->mode_config.plane_list, head) \
 		if ((plane_mask) & (1 << drm_plane_index(plane)))
 
+/**
+ * drm_for_each_encoder_mask - iterate over encoders specified by bitmask
+ * @encoder: the loop cursor
+ * @dev: the DRM device
+ * @encoder_mask: bitmask of encoder indices
+ *
+ * Iterate over all encoders specified by bitmask.
+ */
+#define drm_for_each_encoder_mask(encoder, dev, encoder_mask) \
+	list_for_each_entry((encoder), &(dev)->mode_config.encoder_list, head) \
+		if ((encoder_mask) & (1 << drm_encoder_index(encoder)))
 
 #define obj_to_crtc(x) container_of(x, struct drm_crtc, base)
 #define obj_to_connector(x) container_of(x, struct drm_connector, base)
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/9] drm/atomic: add connector mask to drm_crtc_state.

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c | 11 +++++++++++
 include/drm/drm_crtc.h       |  3 +++
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index d8cc2328317a..f70350ca200f 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1042,10 +1042,21 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 {
 	struct drm_crtc_state *crtc_state;
 
+	if (conn_state->crtc && conn_state->crtc != crtc) {
+		crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state,
+								conn_state->crtc);
+
+		crtc_state->connector_mask &=
+			~(1 << drm_connector_index(conn_state->connector));
+	}
+
 	if (crtc) {
 		crtc_state = drm_atomic_get_crtc_state(conn_state->state, crtc);
 		if (IS_ERR(crtc_state))
 			return PTR_ERR(crtc_state);
+
+		crtc_state->connector_mask |=
+			1 << drm_connector_index(conn_state->connector);
 	}
 
 	conn_state->crtc = crtc;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c54da2d297ec..4999fe530f37 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -264,6 +264,7 @@ struct drm_atomic_state;
  * @active_changed: crtc_state->active has been toggled.
  * @connectors_changed: connectors to this crtc have been updated
  * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
+ * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
  * @last_vblank_count: for helpers and drivers to capture the vblank of the
  * 	update to ensure framebuffer cleanup isn't done too early
  * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
@@ -297,6 +298,8 @@ struct drm_crtc_state {
 	 */
 	u32 plane_mask;
 
+	u32 connector_mask;
+
 	/* last_vblank_count: for vblank waits before cleanup */
 	u32 last_vblank_count;
 
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/9] drm/i915: Update connector_mask during readout.

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dcc7ec7665c2..21b1984e828b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15309,6 +15309,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0);
 		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
+		crtc->base.state->connector_mask = 0;
 
 		/* Because we only establish the connector -> encoder ->
 		 * crtc links if something is active, this means the
@@ -15344,20 +15345,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 {
 	struct intel_connector *connector;
 	struct drm_device *dev = encoder->base.dev;
+	struct drm_crtc *crtc = encoder->base.crtc;
 	bool active = false;
 
 	/* We need to check both for a crtc link (meaning that the
 	 * encoder is active and trying to read from a pipe) and the
 	 * pipe itself being active. */
-	bool has_active_crtc = encoder->base.crtc &&
-		to_intel_crtc(encoder->base.crtc)->active;
+	bool has_active_crtc = crtc && crtc->state->active;
 
 	for_each_intel_connector(dev, connector) {
 		if (connector->base.encoder != &encoder->base)
 			continue;
 
 		active = true;
-		break;
+		if (!has_active_crtc)
+			break;
+
+		crtc->state->connector_mask |=
+			1 << drm_connector_index(&connector->base);
 	}
 
 	if (active && !has_active_crtc) {
-- 
2.1.0

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

[PATCH 7/9] drm/atomic: Small documentation fix.

[Maarten Lankhorst]

Use the correct function name for drm_atomic_clean_old_fb docs.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f70350ca200f..5789649a4099 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1447,7 +1447,7 @@ static int atomic_set_prop(struct drm_atomic_state *state,
 }
 
 /**
- * drm_atomic_update_old_fb -- Unset old_fb pointers and set plane->fb pointers.
+ * drm_atomic_clean_old_fb -- Unset old_fb pointers and set plane->fb pointers.
  *
  * @dev: drm device to check.
  * @plane_mask: plane mask for planes that were updated.
-- 
2.1.0

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

[PATCH 8/9] drm/atomic: Remove drm_atomic_connectors_for_crtc.

[Maarten Lankhorst]

Now that connector_mask is reliable there's no need for this
function any more.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c        | 29 -----------------------------
 drivers/gpu/drm/drm_atomic_helper.c |  9 ++-------
 drivers/gpu/drm/vc4/vc4_crtc.c      |  2 +-
 include/drm/drm_atomic.h            |  4 ----
 4 files changed, 3 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 5789649a4099..9db2941f636e 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1162,35 +1162,6 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
 EXPORT_SYMBOL(drm_atomic_add_affected_planes);
 
 /**
- * drm_atomic_connectors_for_crtc - count number of connected outputs
- * @state: atomic state
- * @crtc: DRM crtc
- *
- * This function counts all connectors which will be connected to @crtc
- * according to @state. Useful to recompute the enable state for @crtc.
- */
-int
-drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
-			       struct drm_crtc *crtc)
-{
-	struct drm_connector *connector;
-	struct drm_connector_state *conn_state;
-
-	int i, num_connected_connectors = 0;
-
-	for_each_connector_in_state(state, connector, conn_state, i) {
-		if (conn_state->crtc == crtc)
-			num_connected_connectors++;
-	}
-
-	DRM_DEBUG_ATOMIC("State %p has %i connectors for [CRTC:%d]\n",
-			 state, num_connected_connectors, crtc->base.id);
-
-	return num_connected_connectors;
-}
-EXPORT_SYMBOL(drm_atomic_connectors_for_crtc);
-
-/**
  * drm_atomic_legacy_backoff - locking backoff for legacy ioctls
  * @state: atomic state
  *
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index cee31d43cd1c..fb79c54b2aed 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -420,8 +420,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 	 * crtc only changed its mode but has the same set of connectors.
 	 */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		int num_connectors;
-
 		/*
 		 * We must set ->active_changed after walking connectors for
 		 * otherwise an update that only changes active would result in
@@ -449,10 +447,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		if (ret != 0)
 			return ret;
 
-		num_connectors = drm_atomic_connectors_for_crtc(state,
-								crtc);
-
-		if (crtc_state->enable != !!num_connectors) {
+		if (crtc_state->enable != !!crtc_state->connector_mask) {
 			DRM_DEBUG_ATOMIC("[CRTC:%d] enabled/connectors mismatch\n",
 					 crtc->base.id);
 
@@ -1668,7 +1663,7 @@ static int update_output_state(struct drm_atomic_state *state,
 		if (crtc == set->crtc)
 			continue;
 
-		if (!drm_atomic_connectors_for_crtc(state, crtc)) {
+		if (!crtc_state->connector_mask) {
 			ret = drm_atomic_set_mode_prop_for_crtc(crtc_state,
 								NULL);
 			if (ret < 0)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 7a9f4768591e..d62a541110a3 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -327,7 +327,7 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
 	/* The pixelvalve can only feed one encoder (and encoders are
 	 * 1:1 with connectors.)
 	 */
-	if (drm_atomic_connectors_for_crtc(state->state, crtc) > 1)
+	if (hweight32(state->connector_mask) > 1)
 		return -EINVAL;
 
 	drm_atomic_crtc_state_for_each_plane(plane, state) {
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 4b74c97d297a..cdc97589b5ce 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -130,10 +130,6 @@ int __must_check
 drm_atomic_add_affected_planes(struct drm_atomic_state *state,
 			       struct drm_crtc *crtc);
 
-int
-drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
-			       struct drm_crtc *crtc);
-
 void drm_atomic_legacy_backoff(struct drm_atomic_state *state);
 
 void
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 9/9] drm/atomic: Add encoder_mask to crtc_state.

[Maarten Lankhorst]

This allows iteration over encoders without requiring connection_mutex.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c  | 4 ++++
 drivers/gpu/drm/i915/intel_display.c | 3 +++
 include/drm/drm_crtc.h               | 2 ++
 3 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index fb79c54b2aed..f3fd8f131f92 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state)
 			continue;
 
 		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
+
+		crtc_state->encoder_mask = 0;
 	}
 
 	for_each_connector_in_state(state, connector, conn_state, i) {
@@ -288,6 +290,8 @@ mode_fixup(struct drm_atomic_state *state)
 		 * it away), so we won't call ->mode_fixup twice.
 		 */
 		encoder = conn_state->best_encoder;
+		crtc_state->encoder_mask |= 1 << drm_encoder_index(encoder);
+
 		funcs = encoder->helper_private;
 		if (!funcs)
 			continue;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 21b1984e828b..3317db3806a8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15310,6 +15310,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
 		crtc->base.state->connector_mask = 0;
+		crtc->base.state->encoder_mask = 0;
 
 		/* Because we only establish the connector -> encoder ->
 		 * crtc links if something is active, this means the
@@ -15363,6 +15364,8 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 
 		crtc->state->connector_mask |=
 			1 << drm_connector_index(&connector->base);
+		crtc->state->encoder_mask |=
+			1 << drm_encoder_index(&encoder->base);
 	}
 
 	if (active && !has_active_crtc) {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 4999fe530f37..7ea83b6a2864 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -265,6 +265,7 @@ struct drm_atomic_state;
  * @connectors_changed: connectors to this crtc have been updated
  * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
  * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
+ * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
  * @last_vblank_count: for helpers and drivers to capture the vblank of the
  * 	update to ensure framebuffer cleanup isn't done too early
  * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
@@ -299,6 +300,7 @@ struct drm_crtc_state {
 	u32 plane_mask;
 
 	u32 connector_mask;
+	u32 encoder_mask;
 
 	/* last_vblank_count: for vblank waits before cleanup */
 	u32 last_vblank_count;
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/9] drm/tegra: Assign conn_state->connector when allocating connector state.

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 281 bytes --]

On Tue, Nov 24, 2015 at 10:34:29AM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/tegra/dsi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 2/9] drm/tegra: Assign conn_state->connector when allocating connector state.

[Daniel Vetter]

On Tue, Nov 24, 2015 at 10:34:29AM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/tegra/dsi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index f0a138ef68ce..6b8ae3d08eeb 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -751,8 +751,10 @@ static void tegra_dsi_connector_reset(struct drm_connector *connector)
>  	connector->state = NULL;
>  
>  	state = kzalloc(sizeof(*state), GFP_KERNEL);
> -	if (state)
> +	if (state) {
> +		state->base.connector = connector;
>  		connector->state = &state->base;
> +	}

Hm, we might want __ versions of all the _reset hooks if this becomes more
common. I do wonder a bit why it isn't since a lot of drivers overwrite
state structures by now, and then the provided reset functions aren't
sufficient really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/9] drm/i915: Update connector_mask during readout.

[Daniel Vetter]

On Tue, Nov 24, 2015 at 10:34:33AM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index dcc7ec7665c2..21b1984e828b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15309,6 +15309,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0);
>  		crtc->base.state->active = crtc->active;
>  		crtc->base.enabled = crtc->active;
> +		crtc->base.state->connector_mask = 0;
>  
>  		/* Because we only establish the connector -> encoder ->
>  		 * crtc links if something is active, this means the
> @@ -15344,20 +15345,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
>  {
>  	struct intel_connector *connector;
>  	struct drm_device *dev = encoder->base.dev;
> +	struct drm_crtc *crtc = encoder->base.crtc;
>  	bool active = false;
>  
>  	/* We need to check both for a crtc link (meaning that the
>  	 * encoder is active and trying to read from a pipe) and the
>  	 * pipe itself being active. */
> -	bool has_active_crtc = encoder->base.crtc &&
> -		to_intel_crtc(encoder->base.crtc)->active;
> +	bool has_active_crtc = crtc && crtc->state->active;
>  
>  	for_each_intel_connector(dev, connector) {
>  		if (connector->base.encoder != &encoder->base)
>  			continue;
>  
>  		active = true;
> -		break;
> +		if (!has_active_crtc)
> +			break;
> +
> +		crtc->state->connector_mask |=
> +			1 << drm_connector_index(&connector->base);

We might want to use the official set_crtc_for_connector/plane in all our
hw state readout code. Is that possible? Would be perfect as a prep patch.
-Daniel

>  	}
>  
>  	if (active && !has_active_crtc) {
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/9] drm/atomic: Small documentation fix.

[Daniel Vetter]

On Tue, Nov 24, 2015 at 10:34:34AM +0100, Maarten Lankhorst wrote:
> Use the correct function name for drm_atomic_clean_old_fb docs.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Applied to topic/drm-misc, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index f70350ca200f..5789649a4099 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1447,7 +1447,7 @@ static int atomic_set_prop(struct drm_atomic_state *state,
>  }
>  
>  /**
> - * drm_atomic_update_old_fb -- Unset old_fb pointers and set plane->fb pointers.
> + * drm_atomic_clean_old_fb -- Unset old_fb pointers and set plane->fb pointers.
>   *
>   * @dev: drm device to check.
>   * @plane_mask: plane mask for planes that were updated.
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/9] drm/tegra: Assign conn_state->connector when allocating connector state.

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2371 bytes --]

On Tue, Nov 24, 2015 at 11:37:47AM +0100, Daniel Vetter wrote:
> On Tue, Nov 24, 2015 at 10:34:29AM +0100, Maarten Lankhorst wrote:
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/tegra/dsi.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> > index f0a138ef68ce..6b8ae3d08eeb 100644
> > --- a/drivers/gpu/drm/tegra/dsi.c
> > +++ b/drivers/gpu/drm/tegra/dsi.c
> > @@ -751,8 +751,10 @@ static void tegra_dsi_connector_reset(struct drm_connector *connector)
> >  	connector->state = NULL;
> >  
> >  	state = kzalloc(sizeof(*state), GFP_KERNEL);
> > -	if (state)
> > +	if (state) {
> > +		state->base.connector = connector;
> >  		connector->state = &state->base;
> > +	}
> 
> Hm, we might want __ versions of all the _reset hooks if this becomes more
> common. I do wonder a bit why it isn't since a lot of drivers overwrite
> state structures by now, and then the provided reset functions aren't
> sufficient really.

We already have the __ versions for duplicate_state helpers, but the
problem with reset helpers is that they need to know the allocation
size...

Actually, that's true of the duplicate_state helpers as well, and the __
variants don't actually allocate the memory but rather just copy the
state from old to new. So we could do something just like that for the
reset helpers:

	void __drm_atomic_helper_connector_reset(struct drm_connector *connector,
						 struct drm_connector_state *state)
	{
		state->base.connector = connector;
		connector->state = state;
	}

and use like this:

	static void tegra_dsi_connector_reset(struct drm_connector *connector)
	{
		struct tegra_dsi_state *state;
		...
		state = kzalloc(sizeof(*state), GFP_KERNEL);
		if (state)
			__drm_atomic_helper_connector_reset(connector, &state->base);
	}

I think back at the time when we did this for duplicate_state helpers we
had a discussion about the usefulness of this, but this patchset clearly
shows that this kind of change, which applies to a number of drivers, is
going to happen again and again, so the only way to avoid mistakes or an
extensive set of changes across all drivers is by putting this into a
helper, even if it's only two assignments now.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 2/9] drm/tegra: Assign conn_state->connector when allocating connector state.

[Maarten Lankhorst]

Op 24-11-15 om 11:51 schreef Thierry Reding:
> On Tue, Nov 24, 2015 at 11:37:47AM +0100, Daniel Vetter wrote:
>> On Tue, Nov 24, 2015 at 10:34:29AM +0100, Maarten Lankhorst wrote:
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/tegra/dsi.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>>> index f0a138ef68ce..6b8ae3d08eeb 100644
>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>> @@ -751,8 +751,10 @@ static void tegra_dsi_connector_reset(struct drm_connector *connector)
>>>  	connector->state = NULL;
>>>  
>>>  	state = kzalloc(sizeof(*state), GFP_KERNEL);
>>> -	if (state)
>>> +	if (state) {
>>> +		state->base.connector = connector;
>>>  		connector->state = &state->base;
>>> +	}
>> Hm, we might want __ versions of all the _reset hooks if this becomes more
>> common. I do wonder a bit why it isn't since a lot of drivers overwrite
>> state structures by now, and then the provided reset functions aren't
>> sufficient really.
> We already have the __ versions for duplicate_state helpers, but the
> problem with reset helpers is that they need to know the allocation
> size...
>
> Actually, that's true of the duplicate_state helpers as well, and the __
> variants don't actually allocate the memory but rather just copy the
> state from old to new. So we could do something just like that for the
> reset helpers:
>
> 	void __drm_atomic_helper_connector_reset(struct drm_connector *connector,
> 						 struct drm_connector_state *state)
> 	{
> 		state->base.connector = connector;
> 		connector->state = state;
> 	}
>
> and use like this:
>
> 	static void tegra_dsi_connector_reset(struct drm_connector *connector)
> 	{
> 		struct tegra_dsi_state *state;
> 		...
> 		state = kzalloc(sizeof(*state), GFP_KERNEL);
> 		if (state)
> 			__drm_atomic_helper_connector_reset(connector, &state->base);
> 	}
>
> I think back at the time when we did this for duplicate_state helpers we
> had a discussion about the usefulness of this, but this patchset clearly
> shows that this kind of change, which applies to a number of drivers, is
> going to happen again and again, so the only way to avoid mistakes or an
> extensive set of changes across all drivers is by putting this into a
> helper, even if it's only two assignments now.
Yeah was considering it, but it felt a bit overkill for something that only holds a pointer to crtc, best_encoder and connector..

If this is the way to go I'll resend
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Update connector_mask during readout.

[Maarten Lankhorst]

Op 24-11-15 om 11:38 schreef Daniel Vetter:
> On Tue, Nov 24, 2015 at 10:34:33AM +0100, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index dcc7ec7665c2..21b1984e828b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -15309,6 +15309,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>>  		WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0);
>>  		crtc->base.state->active = crtc->active;
>>  		crtc->base.enabled = crtc->active;
>> +		crtc->base.state->connector_mask = 0;
>>  
>>  		/* Because we only establish the connector -> encoder ->
>>  		 * crtc links if something is active, this means the
>> @@ -15344,20 +15345,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
>>  {
>>  	struct intel_connector *connector;
>>  	struct drm_device *dev = encoder->base.dev;
>> +	struct drm_crtc *crtc = encoder->base.crtc;
>>  	bool active = false;
>>  
>>  	/* We need to check both for a crtc link (meaning that the
>>  	 * encoder is active and trying to read from a pipe) and the
>>  	 * pipe itself being active. */
>> -	bool has_active_crtc = encoder->base.crtc &&
>> -		to_intel_crtc(encoder->base.crtc)->active;
>> +	bool has_active_crtc = crtc && crtc->state->active;
>>  
>>  	for_each_intel_connector(dev, connector) {
>>  		if (connector->base.encoder != &encoder->base)
>>  			continue;
>>  
>>  		active = true;
>> -		break;
>> +		if (!has_active_crtc)
>> +			break;
>> +
>> +		crtc->state->connector_mask |=
>> +			1 << drm_connector_index(&connector->base);
> We might want to use the official set_crtc_for_connector/plane in all our
> hw state readout code. Is that possible? Would be perfect as a prep patch.
>
No, unfortunately that needs the full atomic state because it calls drm_atomic_get_crtc_state.

~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/i915: Set connector_state->connector using the helper.

[Maarten Lankhorst]

The atomic helper sets connector_state->connector, which the i915
code didn't. This will become a problem when we start using it.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2917fef33f31..d1acdcba580f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6455,13 +6455,11 @@ static void intel_connector_check_state(struct intel_connector *connector)
 
 int intel_connector_init(struct intel_connector *connector)
 {
-	struct drm_connector_state *connector_state;
+	drm_atomic_helper_connector_reset(&connector->base);
 
-	connector_state = kzalloc(sizeof *connector_state, GFP_KERNEL);
-	if (!connector_state)
+	if (!connector->base.state)
 		return -ENOMEM;
 
-	connector->base.state = connector_state;
 	return 0;
 }
 
-- 
2.1.0

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

[PATCH 2/3] drm/atomic: Add __drm_atomic_helper_connector_reset.

[Maarten Lankhorst]

This is useful for drivers that subclass connector_state, like tegra.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 30 ++++++++++++++++++++++++++----
 include/drm/drm_atomic_helper.h     |  2 ++
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index cee31d43cd1c..5d23f818faec 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2359,6 +2359,28 @@ void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
 EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
 
 /**
+ * __drm_atomic_helper_connector_reset - default ->reset hook for connectors
+ * @connector: drm connector
+ * @conn_state: connector state to assign
+ *
+ * Initializes conn_state and assigns it to connector->state, usually
+ * required at when initializing the drivers or when called from the
+ * ->reset hook.
+ *
+ * This is useful for drivers that subclass the connector state.
+ */
+void
+__drm_atomic_helper_connector_reset(struct drm_connector *connector,
+				    struct drm_connector_state *conn_state)
+{
+	if (conn_state)
+		conn_state->connector = connector;
+
+	connector->state = conn_state;
+}
+EXPORT_SYMBOL(__drm_atomic_helper_connector_reset);
+
+/**
  * drm_atomic_helper_connector_reset - default ->reset hook for connectors
  * @connector: drm connector
  *
@@ -2368,11 +2390,11 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
  */
 void drm_atomic_helper_connector_reset(struct drm_connector *connector)
 {
-	kfree(connector->state);
-	connector->state = kzalloc(sizeof(*connector->state), GFP_KERNEL);
+	struct drm_connector_state *conn_state =
+		kzalloc(sizeof(*conn_state), GFP_KERNEL);
 
-	if (connector->state)
-		connector->state->connector = connector;
+	kfree(connector->state);
+	__drm_atomic_helper_connector_reset(connector, conn_state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_connector_reset);
 
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 8cba54a2a0a0..3e00faeea3d0 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -118,6 +118,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
 void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
 					  struct drm_plane_state *state);
 
+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);
 void
 __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/tegra: Use __drm_atomic_helper_reset_connector for subclassing connector state.

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/tegra/dsi.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index f0a138ef68ce..33ad50487f2e 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -745,14 +745,11 @@ static void tegra_dsi_soft_reset(struct tegra_dsi *dsi)
 
 static void tegra_dsi_connector_reset(struct drm_connector *connector)
 {
-	struct tegra_dsi_state *state;
+	struct tegra_dsi_state *state =
+		kzalloc(sizeof(*state), GFP_KERNEL);
 
 	kfree(connector->state);
-	connector->state = NULL;
-
-	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (state)
-		connector->state = &state->base;
+	__drm_atomic_helper_connector_reset(connector, &state->base);
 }
 
 static struct drm_connector_state *
-- 
2.1.0

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

Re: [Intel-gfx] [PATCH 4/9] drm/core: Add drm_for_each_encoder_mask.

[Jani Nikula]

On Tue, 24 Nov 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> +#define drm_for_each_encoder_mask(encoder, dev, encoder_mask) \
> +	list_for_each_entry((encoder), &(dev)->mode_config.encoder_list, head) \
> +		if ((encoder_mask) & (1 << drm_encoder_index(encoder)))

How about

	if (!((encoder_mask) & (1 << drm_encoder_index(encoder)))); else

to avoid dangling else problems?

This inspired me to write [1].

BR,
Jani.


[1] http://patchwork.freedesktop.org/patch/msgid/1448386585-4144-1-git-send-email-jani.nikula@intel.com


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/9] drm/core: Add drm_for_each_encoder_mask.

[David Herrmann]

Hi

On Tue, Nov 24, 2015 at 7:00 PM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Tue, 24 Nov 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
>> +#define drm_for_each_encoder_mask(encoder, dev, encoder_mask) \
>> +     list_for_each_entry((encoder), &(dev)->mode_config.encoder_list, head) \
>> +             if ((encoder_mask) & (1 << drm_encoder_index(encoder)))
>
> How about
>
>         if (!((encoder_mask) & (1 << drm_encoder_index(encoder)))); else
>
> to avoid dangling else problems?

YES! Please use inverted conditions in macros. Otherwise, looks good to me.

But I think an empty block "{ }" is preferable over the empty
statement. llvm tends to warn about empty statements.

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

Re: [Intel-gfx] [PATCH 9/9] drm/atomic: Add encoder_mask to crtc_state.

[Daniel Vetter]

On Tue, Nov 24, 2015 at 10:34:36AM +0100, Maarten Lankhorst wrote:
> This allows iteration over encoders without requiring connection_mutex.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c  | 4 ++++
>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>  include/drm/drm_crtc.h               | 2 ++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index fb79c54b2aed..f3fd8f131f92 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state)
>  			continue;
>  
>  		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
> +
> +		crtc_state->encoder_mask = 0;

Hm, I think a small function to set the best_encoder (like we do to set
the crtc for connector or planes) would be good. Otherwise we'll frob
around the code and forget this, and much confusion will ensue.

That helper should probably be in core drm_atomic.c, like the other
set_foo_for_bar helpers.
-Daniel

>  	}
>  
>  	for_each_connector_in_state(state, connector, conn_state, i) {
> @@ -288,6 +290,8 @@ mode_fixup(struct drm_atomic_state *state)
>  		 * it away), so we won't call ->mode_fixup twice.
>  		 */
>  		encoder = conn_state->best_encoder;
> +		crtc_state->encoder_mask |= 1 << drm_encoder_index(encoder);
> +
>  		funcs = encoder->helper_private;
>  		if (!funcs)
>  			continue;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 21b1984e828b..3317db3806a8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15310,6 +15310,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		crtc->base.state->active = crtc->active;
>  		crtc->base.enabled = crtc->active;
>  		crtc->base.state->connector_mask = 0;
> +		crtc->base.state->encoder_mask = 0;
>  
>  		/* Because we only establish the connector -> encoder ->
>  		 * crtc links if something is active, this means the
> @@ -15363,6 +15364,8 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
>  
>  		crtc->state->connector_mask |=
>  			1 << drm_connector_index(&connector->base);
> +		crtc->state->encoder_mask |=
> +			1 << drm_encoder_index(&encoder->base);
>  	}
>  
>  	if (active && !has_active_crtc) {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 4999fe530f37..7ea83b6a2864 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -265,6 +265,7 @@ struct drm_atomic_state;
>   * @connectors_changed: connectors to this crtc have been updated
>   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
>   * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
> + * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
>   * @last_vblank_count: for helpers and drivers to capture the vblank of the
>   * 	update to ensure framebuffer cleanup isn't done too early
>   * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
> @@ -299,6 +300,7 @@ struct drm_crtc_state {
>  	u32 plane_mask;
>  
>  	u32 connector_mask;
> +	u32 encoder_mask;
>  
>  	/* last_vblank_count: for vblank waits before cleanup */
>  	u32 last_vblank_count;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 9/9] drm/atomic: Add encoder_mask to crtc_state.

[Maarten Lankhorst]

Op 03-12-15 om 10:53 schreef Daniel Vetter:
> On Tue, Nov 24, 2015 at 10:34:36AM +0100, Maarten Lankhorst wrote:
>> This allows iteration over encoders without requiring connection_mutex.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c  | 4 ++++
>>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>>  include/drm/drm_crtc.h               | 2 ++
>>  3 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index fb79c54b2aed..f3fd8f131f92 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state)
>>  			continue;
>>  
>>  		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
>> +
>> +		crtc_state->encoder_mask = 0;
> Hm, I think a small function to set the best_encoder (like we do to set
> the crtc for connector or planes) would be good. Otherwise we'll frob
> around the code and forget this, and much confusion will ensue.
>
> That helper should probably be in core drm_atomic.c, like the other
> set_foo_for_bar helpers.
As always only i915 assigns best_encoder outside drm_atomic_helper_check_modeset, and the i915 calls can't be fixed because of hw readout. :(
At the time of mode_fixup all encoders should have been updated, so I'm not sure adding a helper for best_encoder would help much..

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

Re: [Intel-gfx] [PATCH 9/9] drm/atomic: Add encoder_mask to crtc_state.

[Daniel Vetter]

On Thu, Dec 03, 2015 at 12:01:02PM +0100, Maarten Lankhorst wrote:
> Op 03-12-15 om 10:53 schreef Daniel Vetter:
> > On Tue, Nov 24, 2015 at 10:34:36AM +0100, Maarten Lankhorst wrote:
> >> This allows iteration over encoders without requiring connection_mutex.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_atomic_helper.c  | 4 ++++
> >>  drivers/gpu/drm/i915/intel_display.c | 3 +++
> >>  include/drm/drm_crtc.h               | 2 ++
> >>  3 files changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index fb79c54b2aed..f3fd8f131f92 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state)
> >>  			continue;
> >>  
> >>  		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
> >> +
> >> +		crtc_state->encoder_mask = 0;
> > Hm, I think a small function to set the best_encoder (like we do to set
> > the crtc for connector or planes) would be good. Otherwise we'll frob
> > around the code and forget this, and much confusion will ensue.
> >
> > That helper should probably be in core drm_atomic.c, like the other
> > set_foo_for_bar helpers.
> As always only i915 assigns best_encoder outside drm_atomic_helper_check_modeset, and the i915 calls can't be fixed because of hw readout. :(
> At the time of mode_fixup all encoders should have been updated, so I'm not sure adding a helper for best_encoder would help much..

I've meant just for the atomic helpers. i915 is a mess, yes, but that's
not really an excuse to not make shared code pretty ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/atomic: Add __drm_atomic_helper_connector_reset.

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1457 bytes --]

On Tue, Nov 24, 2015 at 02:35:33PM +0100, Maarten Lankhorst wrote:
> This is useful for drivers that subclass connector_state, like tegra.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 30 ++++++++++++++++++++++++++----
>  include/drm/drm_atomic_helper.h     |  2 ++
>  2 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index cee31d43cd1c..5d23f818faec 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2359,6 +2359,28 @@ void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
>  EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
>  
>  /**
> + * __drm_atomic_helper_connector_reset - default ->reset hook for connectors

That's misleading because you can't use this as a ->reset() hook as-is.
Maybe something like this would be better as a synopsis:

	__drm_atomic_helper_connector_reset - reset connector state

Then the description below gives the details about what it does and when
to use it, which looks fine to me.

> + * @connector: drm connector
> + * @conn_state: connector state to assign
> + *
> + * Initializes conn_state and assigns it to connector->state, usually
> + * required at when initializing the drivers or when called from the

s/required at/required/

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 2/3] drm/atomic: Add __drm_atomic_helper_connector_reset.

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 178 bytes --]

On Tue, Nov 24, 2015 at 02:35:33PM +0100, Maarten Lankhorst wrote:
> This is useful for drivers that subclass connector_state, like tegra.

One more nit: s/tegra/Tegra/

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/tegra: Use __drm_atomic_helper_reset_connector for subclassing connector state.

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 878 bytes --]

On Tue, Nov 24, 2015 at 02:35:34PM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/tegra/dsi.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index f0a138ef68ce..33ad50487f2e 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -745,14 +745,11 @@ static void tegra_dsi_soft_reset(struct tegra_dsi *dsi)
>  
>  static void tegra_dsi_connector_reset(struct drm_connector *connector)
>  {
> -	struct tegra_dsi_state *state;
> +	struct tegra_dsi_state *state =
> +		kzalloc(sizeof(*state), GFP_KERNEL);

I think this could use a check just for safety. It's unlikely to ever
happen, but just in case, better allow to fail gracefully than crash.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 5/9] drm/atomic: add connector mask to drm_crtc_state.

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 439 bytes --]

On Tue, Nov 24, 2015 at 10:34:32AM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c | 11 +++++++++++
>  include/drm/drm_crtc.h       |  3 +++
>  2 files changed, 14 insertions(+)

I think this could use a proper commit message. As it is, it's
completely unclear as to what this mask will be useful for.

Other than that looks good to me.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/9] drm/atomic: Remove drm_atomic_connectors_for_crtc.

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2005 bytes --]

On Tue, Nov 24, 2015 at 10:34:35AM +0100, Maarten Lankhorst wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index cee31d43cd1c..fb79c54b2aed 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -420,8 +420,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  	 * crtc only changed its mode but has the same set of connectors.
>  	 */
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		int num_connectors;
> -
>  		/*
>  		 * We must set ->active_changed after walking connectors for
>  		 * otherwise an update that only changes active would result in
> @@ -449,10 +447,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  		if (ret != 0)
>  			return ret;
>  
> -		num_connectors = drm_atomic_connectors_for_crtc(state,
> -								crtc);
> -
> -		if (crtc_state->enable != !!num_connectors) {
> +		if (crtc_state->enable != !!crtc_state->connector_mask) {

I have difficulty to doubly negate in my head, so something like this
would be a lot clearer in my opinion:

	bool enable = crtc_state->connector_mask != 0;

	...

	if (crtc_state->enable != enable)
		...

Or perhaps even:

	bool disable = crtc_state->connector_mask == 0;

	...

	if (crtc_state->enable == disable)
		...

>  			DRM_DEBUG_ATOMIC("[CRTC:%d] enabled/connectors mismatch\n",
>  					 crtc->base.id);
>  
> @@ -1668,7 +1663,7 @@ static int update_output_state(struct drm_atomic_state *state,
>  		if (crtc == set->crtc)
>  			continue;
>  
> -		if (!drm_atomic_connectors_for_crtc(state, crtc)) {
> +		if (!crtc_state->connector_mask) {

Similarly I think it would be more natural to say:

		if (crtc->state->connector_mask == 0)

here.

Anyway, this is mostly about personal taste, and the change looks
correct to me (after checking twice that I got the double negation
right), so:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/9] drm/i915: Update connector_mask during readout.

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 302 bytes --]

On Tue, Nov 24, 2015 at 10:34:33AM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 3/3] drm/tegra: Use __drm_atomic_helper_reset_connector for subclassing connector state.

[Maarten Lankhorst]

Op 07-12-15 om 11:02 schreef Thierry Reding:
> On Tue, Nov 24, 2015 at 02:35:34PM +0100, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/tegra/dsi.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>> index f0a138ef68ce..33ad50487f2e 100644
>> --- a/drivers/gpu/drm/tegra/dsi.c
>> +++ b/drivers/gpu/drm/tegra/dsi.c
>> @@ -745,14 +745,11 @@ static void tegra_dsi_soft_reset(struct tegra_dsi *dsi)
>>  
>>  static void tegra_dsi_connector_reset(struct drm_connector *connector)
>>  {
>> -	struct tegra_dsi_state *state;
>> +	struct tegra_dsi_state *state =
>> +		kzalloc(sizeof(*state), GFP_KERNEL);
> I think this could use a check just for safety. It's unlikely to ever
> happen, but just in case, better allow to fail gracefully than crash.

I didn't bother because drm_atomic_helper_connector_reset has the same kind of failure.

~Maarten

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/9] drm/atomic: Remove drm_atomic_connectors_for_crtc.

[Maarten Lankhorst]

Op 07-12-15 om 11:34 schreef Thierry Reding:
> On Tue, Nov 24, 2015 at 10:34:35AM +0100, Maarten Lankhorst wrote:
> [...]
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index cee31d43cd1c..fb79c54b2aed 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -420,8 +420,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>>  	 * crtc only changed its mode but has the same set of connectors.
>>  	 */
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> -		int num_connectors;
>> -
>>  		/*
>>  		 * We must set ->active_changed after walking connectors for
>>  		 * otherwise an update that only changes active would result in
>> @@ -449,10 +447,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>>  		if (ret != 0)
>>  			return ret;
>>  
>> -		num_connectors = drm_atomic_connectors_for_crtc(state,
>> -								crtc);
>> -
>> -		if (crtc_state->enable != !!num_connectors) {
>> +		if (crtc_state->enable != !!crtc_state->connector_mask) {
> I have difficulty to doubly negate in my head, so something like this
> would be a lot clearer in my opinion:
Maybe if enable == !connector_mask?

> 	bool enable = crtc_state->connector_mask != 0;
>
> 	...
>
> 	if (crtc_state->enable != enable)
> 		...
>
> Or perhaps even:
>
> 	bool disable = crtc_state->connector_mask == 0;
>
> 	...
>
> 	if (crtc_state->enable == disable)
> 		...
>
>>  			DRM_DEBUG_ATOMIC("[CRTC:%d] enabled/connectors mismatch\n",
>>  					 crtc->base.id);
>>  
>> @@ -1668,7 +1663,7 @@ static int update_output_state(struct drm_atomic_state *state,
>>  		if (crtc == set->crtc)
>>  			continue;
>>  
>> -		if (!drm_atomic_connectors_for_crtc(state, crtc)) {
>> +		if (!crtc_state->connector_mask) {
> Similarly I think it would be more natural to say:
>
> 		if (crtc->state->connector_mask == 0)
>
> here.
>
> Anyway, this is mostly about personal taste, and the change looks
> correct to me (after checking twice that I got the double negation
> right), so:
>
> Reviewed-by: Thierry Reding <treding@nvidia.com>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 9/9] drm/atomic: Add encoder_mask to crtc_state.

[Maarten Lankhorst]

Op 04-12-15 om 09:12 schreef Daniel Vetter:
> On Thu, Dec 03, 2015 at 12:01:02PM +0100, Maarten Lankhorst wrote:
>> Op 03-12-15 om 10:53 schreef Daniel Vetter:
>>> On Tue, Nov 24, 2015 at 10:34:36AM +0100, Maarten Lankhorst wrote:
>>>> This allows iteration over encoders without requiring connection_mutex.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_atomic_helper.c  | 4 ++++
>>>>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>>>>  include/drm/drm_crtc.h               | 2 ++
>>>>  3 files changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index fb79c54b2aed..f3fd8f131f92 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>> @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state)
>>>>  			continue;
>>>>  
>>>>  		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
>>>> +
>>>> +		crtc_state->encoder_mask = 0;
>>> Hm, I think a small function to set the best_encoder (like we do to set
>>> the crtc for connector or planes) would be good. Otherwise we'll frob
>>> around the code and forget this, and much confusion will ensue.
>>>
>>> That helper should probably be in core drm_atomic.c, like the other
>>> set_foo_for_bar helpers.
>> As always only i915 assigns best_encoder outside drm_atomic_helper_check_modeset, and the i915 calls can't be fixed because of hw readout. :(
>> At the time of mode_fixup all encoders should have been updated, so I'm not sure adding a helper for best_encoder would help much..
> I've meant just for the atomic helpers. i915 is a mess, yes, but that's
> not really an excuse to not make shared code pretty ;-)
>
It's not really possible to do it in a helper. The encoder might be moved with the connector, or have a fixed mapping depending on crtc. (i915 MST)

So unfortunately there can be no generic helper, but it has to be dealt with in this function, when assigning best_encoder per crtc.

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

Re: [PATCH 9/9] drm/atomic: Add encoder_mask to crtc_state.

[Daniel Vetter]

On Mon, Dec 14, 2015 at 01:06:12PM +0100, Maarten Lankhorst wrote:
> Op 04-12-15 om 09:12 schreef Daniel Vetter:
> > On Thu, Dec 03, 2015 at 12:01:02PM +0100, Maarten Lankhorst wrote:
> >> Op 03-12-15 om 10:53 schreef Daniel Vetter:
> >>> On Tue, Nov 24, 2015 at 10:34:36AM +0100, Maarten Lankhorst wrote:
> >>>> This allows iteration over encoders without requiring connection_mutex.
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/drm_atomic_helper.c  | 4 ++++
> >>>>  drivers/gpu/drm/i915/intel_display.c | 3 +++
> >>>>  include/drm/drm_crtc.h               | 2 ++
> >>>>  3 files changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>>> index fb79c54b2aed..f3fd8f131f92 100644
> >>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>>> @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state)
> >>>>  			continue;
> >>>>  
> >>>>  		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
> >>>> +
> >>>> +		crtc_state->encoder_mask = 0;
> >>> Hm, I think a small function to set the best_encoder (like we do to set
> >>> the crtc for connector or planes) would be good. Otherwise we'll frob
> >>> around the code and forget this, and much confusion will ensue.
> >>>
> >>> That helper should probably be in core drm_atomic.c, like the other
> >>> set_foo_for_bar helpers.
> >> As always only i915 assigns best_encoder outside drm_atomic_helper_check_modeset, and the i915 calls can't be fixed because of hw readout. :(
> >> At the time of mode_fixup all encoders should have been updated, so I'm not sure adding a helper for best_encoder would help much..
> > I've meant just for the atomic helpers. i915 is a mess, yes, but that's
> > not really an excuse to not make shared code pretty ;-)
> >
> It's not really possible to do it in a helper. The encoder might be
> moved with the connector, or have a fixed mapping depending on crtc.
> (i915 MST)
> 
> So unfortunately there can be no generic helper, but it has to be dealt
> with in this function, when assigning best_encoder per crtc.

I meant a drm_atomic_set_best_encoder function, which sets both
best_encoder and updates the encoder_mask for the crtc. Why would that not
work? Of course actually figuring out what the best_encoder is would not
be done by that function, it would only update the book-keeping. And then
we could reuse it in our state reconstruction code too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 9/9] drm/atomic: Add encoder_mask to crtc_state.

[Maarten Lankhorst]

Op 15-12-15 om 10:17 schreef Daniel Vetter:
> On Mon, Dec 14, 2015 at 01:06:12PM +0100, Maarten Lankhorst wrote:
>> Op 04-12-15 om 09:12 schreef Daniel Vetter:
>>> On Thu, Dec 03, 2015 at 12:01:02PM +0100, Maarten Lankhorst wrote:
>>>> Op 03-12-15 om 10:53 schreef Daniel Vetter:
>>>>> On Tue, Nov 24, 2015 at 10:34:36AM +0100, Maarten Lankhorst wrote:
>>>>>> This allows iteration over encoders without requiring connection_mutex.
>>>>>>
>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/drm_atomic_helper.c  | 4 ++++
>>>>>>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>>>>>>  include/drm/drm_crtc.h               | 2 ++
>>>>>>  3 files changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>>>> index fb79c54b2aed..f3fd8f131f92 100644
>>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>>>> @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state)
>>>>>>  			continue;
>>>>>>  
>>>>>>  		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
>>>>>> +
>>>>>> +		crtc_state->encoder_mask = 0;
>>>>> Hm, I think a small function to set the best_encoder (like we do to set
>>>>> the crtc for connector or planes) would be good. Otherwise we'll frob
>>>>> around the code and forget this, and much confusion will ensue.
>>>>>
>>>>> That helper should probably be in core drm_atomic.c, like the other
>>>>> set_foo_for_bar helpers.
>>>> As always only i915 assigns best_encoder outside drm_atomic_helper_check_modeset, and the i915 calls can't be fixed because of hw readout. :(
>>>> At the time of mode_fixup all encoders should have been updated, so I'm not sure adding a helper for best_encoder would help much..
>>> I've meant just for the atomic helpers. i915 is a mess, yes, but that's
>>> not really an excuse to not make shared code pretty ;-)
>>>
>> It's not really possible to do it in a helper. The encoder might be
>> moved with the connector, or have a fixed mapping depending on crtc.
>> (i915 MST)
>>
>> So unfortunately there can be no generic helper, but it has to be dealt
>> with in this function, when assigning best_encoder per crtc.
> I meant a drm_atomic_set_best_encoder function, which sets both
> best_encoder and updates the encoder_mask for the crtc. Why would that not
> work? Of course actually figuring out what the best_encoder is would not
> be done by that function, it would only update the book-keeping. And then
> we could reuse it in our state reconstruction code too.
How do you want to do this race free in case of a fixed mapping of encoder to crtc?

MST display, con 1 & 2 with enc 1 & 2 and crtc 1 & 2.

crtc1 has con1 and enc1
crtc2 has con2 and enc2

Modeset with con1 moved to crtc2, and con2 to crtc1.

No matter what order you use, if you clear anything in drm_atomic_set_best_encoder you would have a mismatch here.

con1 first mapped to enc2:

crtc1->encoder_mask = 0
crtc2->encoder_mask = enc2 | enc2 = enc2

con2 mapped to enc1, it was previously mapped to enc2, so lets clear it..:
crtc1->encoder_mask = enc1.
crtc2->encoder_mask = 0 <-- Oops!

Same story if you do con2 first.

This is why I chose to clear encoder_mask in mode_fixup instead.

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

Re: [Intel-gfx] [PATCH 9/9] drm/atomic: Add encoder_mask to crtc_state.

[Daniel Vetter]

On Thu, Dec 17, 2015 at 10:06:53AM +0100, Maarten Lankhorst wrote:
> Op 15-12-15 om 10:17 schreef Daniel Vetter:
> > On Mon, Dec 14, 2015 at 01:06:12PM +0100, Maarten Lankhorst wrote:
> >> Op 04-12-15 om 09:12 schreef Daniel Vetter:
> >>> On Thu, Dec 03, 2015 at 12:01:02PM +0100, Maarten Lankhorst wrote:
> >>>> Op 03-12-15 om 10:53 schreef Daniel Vetter:
> >>>>> On Tue, Nov 24, 2015 at 10:34:36AM +0100, Maarten Lankhorst wrote:
> >>>>>> This allows iteration over encoders without requiring connection_mutex.
> >>>>>>
> >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>>> ---
> >>>>>>  drivers/gpu/drm/drm_atomic_helper.c  | 4 ++++
> >>>>>>  drivers/gpu/drm/i915/intel_display.c | 3 +++
> >>>>>>  include/drm/drm_crtc.h               | 2 ++
> >>>>>>  3 files changed, 9 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>> index fb79c54b2aed..f3fd8f131f92 100644
> >>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>> @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state)
> >>>>>>  			continue;
> >>>>>>  
> >>>>>>  		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
> >>>>>> +
> >>>>>> +		crtc_state->encoder_mask = 0;
> >>>>> Hm, I think a small function to set the best_encoder (like we do to set
> >>>>> the crtc for connector or planes) would be good. Otherwise we'll frob
> >>>>> around the code and forget this, and much confusion will ensue.
> >>>>>
> >>>>> That helper should probably be in core drm_atomic.c, like the other
> >>>>> set_foo_for_bar helpers.
> >>>> As always only i915 assigns best_encoder outside drm_atomic_helper_check_modeset, and the i915 calls can't be fixed because of hw readout. :(
> >>>> At the time of mode_fixup all encoders should have been updated, so I'm not sure adding a helper for best_encoder would help much..
> >>> I've meant just for the atomic helpers. i915 is a mess, yes, but that's
> >>> not really an excuse to not make shared code pretty ;-)
> >>>
> >> It's not really possible to do it in a helper. The encoder might be
> >> moved with the connector, or have a fixed mapping depending on crtc.
> >> (i915 MST)
> >>
> >> So unfortunately there can be no generic helper, but it has to be dealt
> >> with in this function, when assigning best_encoder per crtc.
> > I meant a drm_atomic_set_best_encoder function, which sets both
> > best_encoder and updates the encoder_mask for the crtc. Why would that not
> > work? Of course actually figuring out what the best_encoder is would not
> > be done by that function, it would only update the book-keeping. And then
> > we could reuse it in our state reconstruction code too.
> How do you want to do this race free in case of a fixed mapping of encoder to crtc?
> 
> MST display, con 1 & 2 with enc 1 & 2 and crtc 1 & 2.
> 
> crtc1 has con1 and enc1
> crtc2 has con2 and enc2
> 
> Modeset with con1 moved to crtc2, and con2 to crtc1.
> 
> No matter what order you use, if you clear anything in drm_atomic_set_best_encoder you would have a mismatch here.
> 
> con1 first mapped to enc2:
> 
> crtc1->encoder_mask = 0
> crtc2->encoder_mask = enc2 | enc2 = enc2
> 
> con2 mapped to enc1, it was previously mapped to enc2, so lets clear it..:
> crtc1->encoder_mask = enc1.
> crtc2->encoder_mask = 0 <-- Oops!
> 
> Same story if you do con2 first.
> 
> This is why I chose to clear encoder_mask in mode_fixup instead.

With the recent fix to reassing encoders only once this should work, since
it's really 4 steps:

1. steal enc2  from crtc2. We already figure out which connector enc2 has
used, and by looking at con2->state->crtc we can get at the old crtc. So
should be easy to clear crtc2_state->encoder_mask to no longer contain
enc2. I.e. we have enc2, conn2 as known, so just do:

	get_crtc_state(conn2->state->crtc)->encoder_mask &= ~encoder_id(enc2);

2. We notice that conn1 already has a encoder assigned (enc1), and we need
to clear that first. This step is new (well we could hide it in the
set_best_encoder function), but works exactly like step 1.

3. add enc2 to crtc1: That's the easy part, just or in the new bit. Easy:

	crtc1_sate->encoder_mask |= encoder_id(enc2);	

With this enc2 is updated, and enc1 is cleared everywhere, including
encoder_masks.

4. add enc1 for con1 like step 3.

The important bit is simply to not overwrite a pointer before clearing the
mask for the old value completely. If you look at set_crtc_* funcs that's
exactly what they're doing. And since we can always get at the old state
it should always be possible to clear out the old mask.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel