[PATCH 0/3] drm/i915: Convert tv/dp_mst and crt connector properties to atomic.

[PATCH 0/3] drm/i915: Convert tv/dp_mst and crt connector properties to atomic.

[Maarten Lankhorst]

intel_tv is a proof of concept conversion to discuss the API.
The other 2 connectors are straightforward.

Maarten Lankhorst (3):
  drm/i915: Convert intel_tv connector properties to atomic.
  drm/i915: Convert intel_dp_mst connector properties to atomic.
  drm/i915: Convert intel_crt connector properties to atomic.

 drivers/gpu/drm/i915/intel_crt.c    |   9 +-
 drivers/gpu/drm/i915/intel_dp_mst.c |  10 +-
 drivers/gpu/drm/i915/intel_tv.c     | 238 ++++++++++++++++++++----------------
 3 files changed, 138 insertions(+), 119 deletions(-)

-- 
2.7.4

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

[PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic.

[Maarten Lankhorst]

As a proof of concept, first try to convert intel_tv, which is a rarely
used connector. It has 5 properties, tv format and 4 margins.

I'm less certain about the state behavior itself, should we pass a size
parameter to intel_connector_alloc instead, so duplicate_state
can be done globally if it can be blindly copied?

Can we also have a atomic_check function for connectors, so the
crtc_state->connectors_changed can be set there? It would be cleaner
and more atomic-like.

To match the legacy behavior, format can be changed by probing just like
in legacy mode.

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

diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 6ed1a3ce47b7..0fb1d8621fe8 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -48,8 +48,6 @@ struct intel_tv {
 	struct intel_encoder base;
 
 	int type;
-	const char *tv_format;
-	int margin[4];
 	u32 save_TV_H_CTL_1;
 	u32 save_TV_H_CTL_2;
 	u32 save_TV_H_CTL_3;
@@ -85,6 +83,16 @@ struct intel_tv {
 	u32 save_TV_CTL;
 };
 
+struct intel_tv_connector_state {
+	struct drm_connector_state base;
+
+	int format;
+	int margin[4];
+};
+
+#define to_intel_tv_connector_state(state) \
+	container_of((state), struct intel_tv_connector_state, base)
+
 struct video_levels {
 	u16 blank, black;
 	u8 burst;
@@ -873,32 +881,18 @@ intel_disable_tv(struct intel_encoder *encoder,
 	I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
 }
 
-static const struct tv_mode *
-intel_tv_mode_lookup(const char *tv_format)
+static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
 {
-	int i;
+	int format = to_intel_tv_connector_state(conn_state)->format;
 
-	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
-		const struct tv_mode *tv_mode = &tv_modes[i];
-
-		if (!strcmp(tv_format, tv_mode->name))
-			return tv_mode;
-	}
-	return NULL;
-}
-
-static const struct tv_mode *
-intel_tv_mode_find(struct intel_tv *intel_tv)
-{
-	return intel_tv_mode_lookup(intel_tv->tv_format);
+	return &tv_modes[format];
 }
 
 static enum drm_mode_status
 intel_tv_mode_valid(struct drm_connector *connector,
 		    struct drm_display_mode *mode)
 {
-	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
 	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
 
 	if (mode->clock > max_dotclk)
@@ -925,8 +919,7 @@ intel_tv_compute_config(struct intel_encoder *encoder,
 			struct intel_crtc_state *pipe_config,
 			struct drm_connector_state *conn_state)
 {
-	struct intel_tv *intel_tv = enc_to_tv(encoder);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
 
 	if (!tv_mode)
 		return false;
@@ -1032,7 +1025,7 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
 	struct intel_tv *intel_tv = enc_to_tv(encoder);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
 	u32 tv_ctl;
 	u32 scctl1, scctl2, scctl3;
 	int i, j;
@@ -1041,6 +1034,8 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
 	bool burst_ena;
 	int xpos = 0x0, ypos = 0x0;
 	unsigned int xsize, ysize;
+	struct intel_tv_connector_state *tv_state =
+		to_intel_tv_connector_state(conn_state);
 
 	if (!tv_mode)
 		return;	/* can't happen (mode_prepare prevents this) */
@@ -1135,12 +1130,12 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
 	else
 		ysize = 2*tv_mode->nbr_end + 1;
 
-	xpos += intel_tv->margin[TV_MARGIN_LEFT];
-	ypos += intel_tv->margin[TV_MARGIN_TOP];
-	xsize -= (intel_tv->margin[TV_MARGIN_LEFT] +
-		  intel_tv->margin[TV_MARGIN_RIGHT]);
-	ysize -= (intel_tv->margin[TV_MARGIN_TOP] +
-		  intel_tv->margin[TV_MARGIN_BOTTOM]);
+	xpos += tv_state->margin[TV_MARGIN_LEFT];
+	ypos += tv_state->margin[TV_MARGIN_TOP];
+	xsize -= (tv_state->margin[TV_MARGIN_LEFT] +
+		  tv_state->margin[TV_MARGIN_RIGHT]);
+	ysize -= (tv_state->margin[TV_MARGIN_TOP] +
+		  tv_state->margin[TV_MARGIN_BOTTOM]);
 	I915_WRITE(TV_WIN_POS, (xpos<<16)|ypos);
 	I915_WRITE(TV_WIN_SIZE, (xsize<<16)|ysize);
 
@@ -1288,7 +1283,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
 static void intel_tv_find_better_format(struct drm_connector *connector)
 {
 	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
 	int i;
 
 	if ((intel_tv->type == DRM_MODE_CONNECTOR_Component) ==
@@ -1304,9 +1299,7 @@ static void intel_tv_find_better_format(struct drm_connector *connector)
 			break;
 	}
 
-	intel_tv->tv_format = tv_mode->name;
-	drm_object_property_set_value(&connector->base,
-		connector->dev->mode_config.tv_mode_property, i);
+	to_intel_tv_connector_state(connector->state)->format = i;
 }
 
 /**
@@ -1344,18 +1337,17 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 		} else
 			status = connector_status_unknown;
 
+		if (status == connector_status_connected) {
+			intel_tv->type = type;
+			intel_tv_find_better_format(connector);
+		}
+
 		drm_modeset_drop_locks(&ctx);
 		drm_modeset_acquire_fini(&ctx);
-	} else
-		return connector->status;
 
-	if (status != connector_status_connected)
 		return status;
-
-	intel_tv->type = type;
-	intel_tv_find_better_format(connector);
-
-	return connector_status_connected;
+	} else
+		return connector->status;
 }
 
 static const struct input_res {
@@ -1378,8 +1370,7 @@ static void
 intel_tv_chose_preferred_modes(struct drm_connector *connector,
 			       struct drm_display_mode *mode_ptr)
 {
-	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
 
 	if (tv_mode->nbr_end < 480 && mode_ptr->vdisplay == 480)
 		mode_ptr->type |= DRM_MODE_TYPE_PREFERRED;
@@ -1403,8 +1394,7 @@ static int
 intel_tv_get_modes(struct drm_connector *connector)
 {
 	struct drm_display_mode *mode_ptr;
-	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
 	int j, count = 0;
 	u64 tmp;
 
@@ -1462,56 +1452,97 @@ intel_tv_destroy(struct drm_connector *connector)
 	kfree(connector);
 }
 
-
 static int
-intel_tv_set_property(struct drm_connector *connector, struct drm_property *property,
-		      uint64_t val)
+intel_tv_atomic_set_property(struct drm_connector *connector,
+			     struct drm_connector_state *conn_state,
+			     struct drm_property *property,
+			     uint64_t val)
 {
 	struct drm_device *dev = connector->dev;
-	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	struct drm_crtc *crtc = intel_tv->base.base.crtc;
-	int ret = 0;
+
+	struct intel_tv_connector_state *tv_state =
+		to_intel_tv_connector_state(conn_state);
 	bool changed = false;
 
-	ret = drm_object_property_set_value(&connector->base, property, val);
-	if (ret < 0)
-		goto out;
-
-	if (property == dev->mode_config.tv_left_margin_property &&
-		intel_tv->margin[TV_MARGIN_LEFT] != val) {
-		intel_tv->margin[TV_MARGIN_LEFT] = val;
-		changed = true;
-	} else if (property == dev->mode_config.tv_right_margin_property &&
-		intel_tv->margin[TV_MARGIN_RIGHT] != val) {
-		intel_tv->margin[TV_MARGIN_RIGHT] = val;
-		changed = true;
-	} else if (property == dev->mode_config.tv_top_margin_property &&
-		intel_tv->margin[TV_MARGIN_TOP] != val) {
-		intel_tv->margin[TV_MARGIN_TOP] = val;
-		changed = true;
-	} else if (property == dev->mode_config.tv_bottom_margin_property &&
-		intel_tv->margin[TV_MARGIN_BOTTOM] != val) {
-		intel_tv->margin[TV_MARGIN_BOTTOM] = val;
-		changed = true;
+	if (property == dev->mode_config.tv_left_margin_property) {
+		changed = tv_state->margin[TV_MARGIN_LEFT] != val;
+
+		tv_state->margin[TV_MARGIN_LEFT] = val;
+	} else if (property == dev->mode_config.tv_right_margin_property) {
+		changed = tv_state->margin[TV_MARGIN_RIGHT] != val;
+
+		tv_state->margin[TV_MARGIN_RIGHT] = val;
+	} else if (property == dev->mode_config.tv_top_margin_property) {
+		changed = tv_state->margin[TV_MARGIN_TOP] != val;
+
+		tv_state->margin[TV_MARGIN_TOP] = val;
+	} else if (property == dev->mode_config.tv_bottom_margin_property) {
+		changed = tv_state->margin[TV_MARGIN_BOTTOM] != val;
+
+		tv_state->margin[TV_MARGIN_BOTTOM] = val;
 	} else if (property == dev->mode_config.tv_mode_property) {
 		if (val >= ARRAY_SIZE(tv_modes)) {
-			ret = -EINVAL;
-			goto out;
+			DRM_DEBUG_ATOMIC("value %llu out of tv_modes array bounds\n", val);
+
+			return -EINVAL;
 		}
-		if (!strcmp(intel_tv->tv_format, tv_modes[val].name))
-			goto out;
 
-		intel_tv->tv_format = tv_modes[val].name;
-		changed = true;
+		changed = tv_state->format != val;
+		tv_state->format = val;
+	} else {
+		DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name);
+		return -EINVAL;
+	}
+
+	/* Trigger a modeset when connector properties are changed. */
+	if (changed && conn_state->crtc) {
+		struct drm_crtc_state *crtc_state =
+			drm_atomic_get_existing_crtc_state(conn_state->state,
+							   conn_state->crtc);
+
+		crtc_state->connectors_changed = true;
+	}
+	return 0;
+}
+
+static int
+intel_tv_atomic_get_property(struct drm_connector *connector,
+				    const struct drm_connector_state *state,
+				    struct drm_property *property,
+				    uint64_t *val)
+{
+	struct drm_device *dev = connector->dev;
+	struct intel_tv_connector_state *tv_state =
+		to_intel_tv_connector_state(state);
+
+	if (property == dev->mode_config.tv_left_margin_property)
+		*val = tv_state->margin[TV_MARGIN_LEFT];
+	else if (property == dev->mode_config.tv_right_margin_property)
+		*val = tv_state->margin[TV_MARGIN_RIGHT];
+	else if (property == dev->mode_config.tv_top_margin_property)
+		*val = tv_state->margin[TV_MARGIN_TOP];
+	else if (property == dev->mode_config.tv_bottom_margin_property)
+		*val = tv_state->margin[TV_MARGIN_BOTTOM];
+	else if (property == dev->mode_config.tv_mode_property) {
+		*val = tv_state->format;
 	} else {
-		ret = -EINVAL;
-		goto out;
+		DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name);
+		return -EINVAL;
 	}
 
-	if (changed && crtc)
-		intel_crtc_restore_mode(crtc);
-out:
-	return ret;
+	return 0;
+}
+
+static struct drm_connector_state *
+intel_tv_duplicate_state(struct drm_connector *connector)
+{
+	struct intel_tv_connector_state *state;
+
+	state = kmemdup(&connector->state, sizeof(*state), GFP_KERNEL);
+	if (state)
+		__drm_atomic_helper_connector_duplicate_state(connector, &state->base);
+
+	return &state->base;
 }
 
 static const struct drm_connector_funcs intel_tv_connector_funcs = {
@@ -1520,11 +1551,12 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = {
 	.late_register = intel_connector_register,
 	.early_unregister = intel_connector_unregister,
 	.destroy = intel_tv_destroy,
-	.set_property = intel_tv_set_property,
-	.atomic_get_property = intel_connector_atomic_get_property,
+	.set_property = drm_atomic_helper_connector_set_property,
+	.atomic_set_property = intel_tv_atomic_set_property,
+	.atomic_get_property = intel_tv_atomic_get_property,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_duplicate_state = intel_tv_duplicate_state,
 };
 
 static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = {
@@ -1547,6 +1579,7 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 	u32 tv_dac_on, tv_dac_off, save_tv_dac;
 	const char *tv_format_names[ARRAY_SIZE(tv_modes)];
 	int i, initial_mode = 0;
+	struct intel_tv_connector_state *state;
 
 	if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED)
 		return;
@@ -1580,16 +1613,17 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 		return;
 
 	intel_tv = kzalloc(sizeof(*intel_tv), GFP_KERNEL);
-	if (!intel_tv) {
-		return;
-	}
-
-	intel_connector = intel_connector_alloc();
-	if (!intel_connector) {
+	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!intel_tv || !intel_connector || !state) {
+		kfree(intel_connector);
+		kfree(state);
 		kfree(intel_tv);
 		return;
 	}
 
+	__drm_atomic_helper_connector_reset(&intel_connector->base, &state->base);
+
 	intel_encoder = &intel_tv->base;
 	connector = &intel_connector->base;
 
@@ -1629,12 +1663,12 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 	intel_tv->type = DRM_MODE_CONNECTOR_Unknown;
 
 	/* BIOS margin values */
-	intel_tv->margin[TV_MARGIN_LEFT] = 54;
-	intel_tv->margin[TV_MARGIN_TOP] = 36;
-	intel_tv->margin[TV_MARGIN_RIGHT] = 46;
-	intel_tv->margin[TV_MARGIN_BOTTOM] = 37;
+	state->margin[TV_MARGIN_LEFT] = 54;
+	state->margin[TV_MARGIN_TOP] = 36;
+	state->margin[TV_MARGIN_RIGHT] = 46;
+	state->margin[TV_MARGIN_BOTTOM] = 37;
 
-	intel_tv->tv_format = tv_modes[initial_mode].name;
+	state->format = initial_mode;
 
 	drm_connector_helper_add(connector, &intel_tv_connector_helper_funcs);
 	connector->interlace_allowed = false;
@@ -1648,17 +1682,17 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 				      tv_format_names);
 
 	drm_object_attach_property(&connector->base, dev->mode_config.tv_mode_property,
-				   initial_mode);
+				   state->format);
 	drm_object_attach_property(&connector->base,
 				   dev->mode_config.tv_left_margin_property,
-				   intel_tv->margin[TV_MARGIN_LEFT]);
+				   state->margin[TV_MARGIN_LEFT]);
 	drm_object_attach_property(&connector->base,
 				   dev->mode_config.tv_top_margin_property,
-				   intel_tv->margin[TV_MARGIN_TOP]);
+				   state->margin[TV_MARGIN_TOP]);
 	drm_object_attach_property(&connector->base,
 				   dev->mode_config.tv_right_margin_property,
-				   intel_tv->margin[TV_MARGIN_RIGHT]);
+				   state->margin[TV_MARGIN_RIGHT]);
 	drm_object_attach_property(&connector->base,
 				   dev->mode_config.tv_bottom_margin_property,
-				   intel_tv->margin[TV_MARGIN_BOTTOM]);
+				   state->margin[TV_MARGIN_BOTTOM]);
 }
-- 
2.7.4

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

[PATCH 2/3] drm/i915: Convert intel_dp_mst connector properties to atomic.

[Maarten Lankhorst]

MST doesn't support setting any properties, but it should still
use the atomic helper for setting properties.

Only path and tile properties are supported (read-only), so keep
using the i915 helper for obtaining those properties.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 094cbdcbcd6d..ab756672e2cf 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -294,14 +294,6 @@ intel_dp_mst_detect(struct drm_connector *connector, bool force)
 	return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr, intel_connector->port);
 }
 
-static int
-intel_dp_mst_set_property(struct drm_connector *connector,
-			  struct drm_property *property,
-			  uint64_t val)
-{
-	return 0;
-}
-
 static void
 intel_dp_mst_connector_destroy(struct drm_connector *connector)
 {
@@ -318,7 +310,7 @@ static const struct drm_connector_funcs intel_dp_mst_connector_funcs = {
 	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_dp_mst_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
-	.set_property = intel_dp_mst_set_property,
+	.set_property = drm_atomic_helper_connector_set_property,
 	.atomic_get_property = intel_connector_atomic_get_property,
 	.late_register = intel_connector_register,
 	.early_unregister = intel_connector_unregister,
-- 
2.7.4

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

[PATCH 3/3] drm/i915: Convert intel_crt connector properties to atomic.

[Maarten Lankhorst]

No properties are supported, so just use the helper and reject
everything.

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

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 8c82607294c6..4ee4df37e683 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -778,13 +778,6 @@ static int intel_crt_get_modes(struct drm_connector *connector)
 	return ret;
 }
 
-static int intel_crt_set_property(struct drm_connector *connector,
-				  struct drm_property *property,
-				  uint64_t value)
-{
-	return 0;
-}
-
 void intel_crt_reset(struct drm_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
@@ -816,7 +809,7 @@ static const struct drm_connector_funcs intel_crt_connector_funcs = {
 	.late_register = intel_connector_register,
 	.early_unregister = intel_connector_unregister,
 	.destroy = intel_crt_destroy,
-	.set_property = intel_crt_set_property,
+	.set_property = drm_atomic_helper_connector_set_property,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_get_property = intel_connector_atomic_get_property,
-- 
2.7.4

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

✓ Fi.CI.BAT: success for drm/i915: Convert tv/dp_mst and crt connector properties to atomic.

[Patchwork]

== Series Details ==

Series: drm/i915: Convert tv/dp_mst and crt connector properties to atomic.
URL   : https://patchwork.freedesktop.org/series/20990/
State : success

== Summary ==

Series 20990v1 drm/i915: Convert tv/dp_mst and crt connector properties to atomic.
https://patchwork.freedesktop.org/api/1.0/series/20990/revisions/1/mbox/

Test gem_ctx_create:
        Subgroup basic-files:
                pass       -> INCOMPLETE (fi-skl-6700k) fdo#100130
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-bxt-t5700) fdo#100125

fdo#100130 https://bugs.freedesktop.org/show_bug.cgi?id=100130
fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 474s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 616s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 543s
fi-bxt-t5700     total:278  pass:257  dwarn:1   dfail:0   fail:0   skip:20  time: 610s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 507s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 501s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 446s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 435s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 437s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 507s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 491s
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time: 480s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 509s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 601s
fi-skl-6700k     total:17   pass:16   dwarn:0   dfail:0   fail:0   skip:0   time: 0s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 548s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 554s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time: 421s

510c200742ced5a91d07e48220b669a3c9b30c0c drm-tip: 2017y-03m-09d-15h-21m-14s UTC integration manifest
0f77d01 drm/i915: Convert intel_crt connector properties to atomic.
465968e drm/i915: Convert intel_dp_mst connector properties to atomic.
423cf50 drm/i915: Convert intel_tv connector properties to atomic.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4116/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic.

[Ville Syrjälä]

On Thu, Mar 09, 2017 at 02:06:15PM +0100, Maarten Lankhorst wrote:
> As a proof of concept, first try to convert intel_tv, which is a rarely
> used connector. It has 5 properties, tv format and 4 margins.

Since it's so rare, if you want someone to actually test the code
it'll probably make sense to pick another connector ;)

> 
> I'm less certain about the state behavior itself, should we pass a size
> parameter to intel_connector_alloc instead, so duplicate_state
> can be done globally if it can be blindly copied?
> 
> Can we also have a atomic_check function for connectors, so the
> crtc_state->connectors_changed can be set there? It would be cleaner
> and more atomic-like.

Hmm. I think it migth be really useful only if we have some
interactions between multiple properties that really need to be
checked. We might have those already I suppose but we don't seem
to check any of it currently. So as a first step I guess we can
just keep ignoring any such issues.

> 
> To match the legacy behavior, format can be changed by probing just like
> in legacy mode.

Self modifying state irks me, but it's what we've been doing so I guess
we should keep it.

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_tv.c | 238 +++++++++++++++++++++++-----------------
>  1 file changed, 136 insertions(+), 102 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 6ed1a3ce47b7..0fb1d8621fe8 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -48,8 +48,6 @@ struct intel_tv {
>  	struct intel_encoder base;
>  
>  	int type;
> -	const char *tv_format;
> -	int margin[4];
>  	u32 save_TV_H_CTL_1;
>  	u32 save_TV_H_CTL_2;
>  	u32 save_TV_H_CTL_3;
> @@ -85,6 +83,16 @@ struct intel_tv {
>  	u32 save_TV_CTL;
>  };
>  
> +struct intel_tv_connector_state {
> +	struct drm_connector_state base;
> +
> +	int format;
> +	int margin[4];
> +};
> +
> +#define to_intel_tv_connector_state(state) \
> +	container_of((state), struct intel_tv_connector_state, base)
> +
>  struct video_levels {
>  	u16 blank, black;
>  	u8 burst;
> @@ -873,32 +881,18 @@ intel_disable_tv(struct intel_encoder *encoder,
>  	I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
>  }
>  
> -static const struct tv_mode *
> -intel_tv_mode_lookup(const char *tv_format)
> +static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
>  {
> -	int i;
> +	int format = to_intel_tv_connector_state(conn_state)->format;
>  
> -	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
> -		const struct tv_mode *tv_mode = &tv_modes[i];
> -
> -		if (!strcmp(tv_format, tv_mode->name))
> -			return tv_mode;
> -	}
> -	return NULL;
> -}
> -
> -static const struct tv_mode *
> -intel_tv_mode_find(struct intel_tv *intel_tv)
> -{
> -	return intel_tv_mode_lookup(intel_tv->tv_format);
> +	return &tv_modes[format];
>  }
>  
>  static enum drm_mode_status
>  intel_tv_mode_valid(struct drm_connector *connector,
>  		    struct drm_display_mode *mode)
>  {
> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> +	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);

It feels a bit fishy to use the state here. Generally that's a no-no.
But in this case I wonder if it's the right choice after all. 

Not sure if some kind of "automatic" enum value might also work. It
would at least avoid the self modifying property problem. Although I
wonder if the user would still like to know what was actually used
if they chose they automatic mode, so we might need a self modifying
RO property for the current mode anyway.

But that still leaves the problem of how the user would know which modes
they should be able to use if .get_modes()/.mode_valid() doesn't respect
the users choice of the tv format. Hmm, tricky. Might be the self
modifying property is the only good choice.

But if we would use the state here, what's the story with locking going
to be? connection_mutex is what protects this stuff, but we're not
holding that during mode enumeration.

>  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
>  
>  	if (mode->clock > max_dotclk)
> @@ -925,8 +919,7 @@ intel_tv_compute_config(struct intel_encoder *encoder,
>  			struct intel_crtc_state *pipe_config,
>  			struct drm_connector_state *conn_state)
>  {
> -	struct intel_tv *intel_tv = enc_to_tv(encoder);
> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> +	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
>  
>  	if (!tv_mode)
>  		return false;
> @@ -1032,7 +1025,7 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>  	struct intel_tv *intel_tv = enc_to_tv(encoder);
> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> +	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
>  	u32 tv_ctl;
>  	u32 scctl1, scctl2, scctl3;
>  	int i, j;
> @@ -1041,6 +1034,8 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>  	bool burst_ena;
>  	int xpos = 0x0, ypos = 0x0;
>  	unsigned int xsize, ysize;
> +	struct intel_tv_connector_state *tv_state =
> +		to_intel_tv_connector_state(conn_state);
>  
>  	if (!tv_mode)
>  		return;	/* can't happen (mode_prepare prevents this) */
> @@ -1135,12 +1130,12 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>  	else
>  		ysize = 2*tv_mode->nbr_end + 1;
>  
> -	xpos += intel_tv->margin[TV_MARGIN_LEFT];
> -	ypos += intel_tv->margin[TV_MARGIN_TOP];
> -	xsize -= (intel_tv->margin[TV_MARGIN_LEFT] +
> -		  intel_tv->margin[TV_MARGIN_RIGHT]);
> -	ysize -= (intel_tv->margin[TV_MARGIN_TOP] +
> -		  intel_tv->margin[TV_MARGIN_BOTTOM]);
> +	xpos += tv_state->margin[TV_MARGIN_LEFT];
> +	ypos += tv_state->margin[TV_MARGIN_TOP];
> +	xsize -= (tv_state->margin[TV_MARGIN_LEFT] +
> +		  tv_state->margin[TV_MARGIN_RIGHT]);
> +	ysize -= (tv_state->margin[TV_MARGIN_TOP] +
> +		  tv_state->margin[TV_MARGIN_BOTTOM]);
>  	I915_WRITE(TV_WIN_POS, (xpos<<16)|ypos);
>  	I915_WRITE(TV_WIN_SIZE, (xsize<<16)|ysize);
>  
> @@ -1288,7 +1283,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
>  static void intel_tv_find_better_format(struct drm_connector *connector)
>  {
>  	struct intel_tv *intel_tv = intel_attached_tv(connector);
> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> +	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
>  	int i;
>  
>  	if ((intel_tv->type == DRM_MODE_CONNECTOR_Component) ==
> @@ -1304,9 +1299,7 @@ static void intel_tv_find_better_format(struct drm_connector *connector)
>  			break;
>  	}
>  
> -	intel_tv->tv_format = tv_mode->name;
> -	drm_object_property_set_value(&connector->base,
> -		connector->dev->mode_config.tv_mode_property, i);
> +	to_intel_tv_connector_state(connector->state)->format = i;
>  }
>  
>  /**
> @@ -1344,18 +1337,17 @@ intel_tv_detect(struct drm_connector *connector, bool force)
>  		} else
>  			status = connector_status_unknown;
>  
> +		if (status == connector_status_connected) {
> +			intel_tv->type = type;
> +			intel_tv_find_better_format(connector);
> +		}
> +
>  		drm_modeset_drop_locks(&ctx);
>  		drm_modeset_acquire_fini(&ctx);
> -	} else
> -		return connector->status;
>  
> -	if (status != connector_status_connected)
>  		return status;
> -
> -	intel_tv->type = type;
> -	intel_tv_find_better_format(connector);
> -
> -	return connector_status_connected;
> +	} else
> +		return connector->status;
>  }
>  
>  static const struct input_res {
> @@ -1378,8 +1370,7 @@ static void
>  intel_tv_chose_preferred_modes(struct drm_connector *connector,
>  			       struct drm_display_mode *mode_ptr)
>  {
> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> +	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
>  
>  	if (tv_mode->nbr_end < 480 && mode_ptr->vdisplay == 480)
>  		mode_ptr->type |= DRM_MODE_TYPE_PREFERRED;
> @@ -1403,8 +1394,7 @@ static int
>  intel_tv_get_modes(struct drm_connector *connector)
>  {
>  	struct drm_display_mode *mode_ptr;
> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> +	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
>  	int j, count = 0;
>  	u64 tmp;
>  
> @@ -1462,56 +1452,97 @@ intel_tv_destroy(struct drm_connector *connector)
>  	kfree(connector);
>  }
>  
> -
>  static int
> -intel_tv_set_property(struct drm_connector *connector, struct drm_property *property,
> -		      uint64_t val)
> +intel_tv_atomic_set_property(struct drm_connector *connector,
> +			     struct drm_connector_state *conn_state,
> +			     struct drm_property *property,
> +			     uint64_t val)
>  {
>  	struct drm_device *dev = connector->dev;
> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
> -	struct drm_crtc *crtc = intel_tv->base.base.crtc;
> -	int ret = 0;
> +
> +	struct intel_tv_connector_state *tv_state =
> +		to_intel_tv_connector_state(conn_state);
>  	bool changed = false;
>  
> -	ret = drm_object_property_set_value(&connector->base, property, val);
> -	if (ret < 0)
> -		goto out;
> -
> -	if (property == dev->mode_config.tv_left_margin_property &&
> -		intel_tv->margin[TV_MARGIN_LEFT] != val) {
> -		intel_tv->margin[TV_MARGIN_LEFT] = val;
> -		changed = true;
> -	} else if (property == dev->mode_config.tv_right_margin_property &&
> -		intel_tv->margin[TV_MARGIN_RIGHT] != val) {
> -		intel_tv->margin[TV_MARGIN_RIGHT] = val;
> -		changed = true;
> -	} else if (property == dev->mode_config.tv_top_margin_property &&
> -		intel_tv->margin[TV_MARGIN_TOP] != val) {
> -		intel_tv->margin[TV_MARGIN_TOP] = val;
> -		changed = true;
> -	} else if (property == dev->mode_config.tv_bottom_margin_property &&
> -		intel_tv->margin[TV_MARGIN_BOTTOM] != val) {
> -		intel_tv->margin[TV_MARGIN_BOTTOM] = val;
> -		changed = true;
> +	if (property == dev->mode_config.tv_left_margin_property) {
> +		changed = tv_state->margin[TV_MARGIN_LEFT] != val;
> +
> +		tv_state->margin[TV_MARGIN_LEFT] = val;
> +	} else if (property == dev->mode_config.tv_right_margin_property) {
> +		changed = tv_state->margin[TV_MARGIN_RIGHT] != val;
> +
> +		tv_state->margin[TV_MARGIN_RIGHT] = val;
> +	} else if (property == dev->mode_config.tv_top_margin_property) {
> +		changed = tv_state->margin[TV_MARGIN_TOP] != val;
> +
> +		tv_state->margin[TV_MARGIN_TOP] = val;
> +	} else if (property == dev->mode_config.tv_bottom_margin_property) {
> +		changed = tv_state->margin[TV_MARGIN_BOTTOM] != val;
> +
> +		tv_state->margin[TV_MARGIN_BOTTOM] = val;
>  	} else if (property == dev->mode_config.tv_mode_property) {
>  		if (val >= ARRAY_SIZE(tv_modes)) {
> -			ret = -EINVAL;
> -			goto out;
> +			DRM_DEBUG_ATOMIC("value %llu out of tv_modes array bounds\n", val);
> +
> +			return -EINVAL;
>  		}
> -		if (!strcmp(intel_tv->tv_format, tv_modes[val].name))
> -			goto out;
>  
> -		intel_tv->tv_format = tv_modes[val].name;
> -		changed = true;
> +		changed = tv_state->format != val;
> +		tv_state->format = val;
> +	} else {
> +		DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name);
> +		return -EINVAL;
> +	}
> +
> +	/* Trigger a modeset when connector properties are changed. */
> +	if (changed && conn_state->crtc) {
> +		struct drm_crtc_state *crtc_state =
> +			drm_atomic_get_existing_crtc_state(conn_state->state,
> +							   conn_state->crtc);
> +
> +		crtc_state->connectors_changed = true;
> +	}
> +	return 0;
> +}
> +
> +static int
> +intel_tv_atomic_get_property(struct drm_connector *connector,
> +				    const struct drm_connector_state *state,
> +				    struct drm_property *property,
> +				    uint64_t *val)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct intel_tv_connector_state *tv_state =
> +		to_intel_tv_connector_state(state);
> +
> +	if (property == dev->mode_config.tv_left_margin_property)
> +		*val = tv_state->margin[TV_MARGIN_LEFT];
> +	else if (property == dev->mode_config.tv_right_margin_property)
> +		*val = tv_state->margin[TV_MARGIN_RIGHT];
> +	else if (property == dev->mode_config.tv_top_margin_property)
> +		*val = tv_state->margin[TV_MARGIN_TOP];
> +	else if (property == dev->mode_config.tv_bottom_margin_property)
> +		*val = tv_state->margin[TV_MARGIN_BOTTOM];
> +	else if (property == dev->mode_config.tv_mode_property) {
> +		*val = tv_state->format;
>  	} else {
> -		ret = -EINVAL;
> -		goto out;
> +		DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name);
> +		return -EINVAL;
>  	}
>  
> -	if (changed && crtc)
> -		intel_crtc_restore_mode(crtc);
> -out:
> -	return ret;
> +	return 0;
> +}
> +
> +static struct drm_connector_state *
> +intel_tv_duplicate_state(struct drm_connector *connector)
> +{
> +	struct intel_tv_connector_state *state;
> +
> +	state = kmemdup(&connector->state, sizeof(*state), GFP_KERNEL);
> +	if (state)
> +		__drm_atomic_helper_connector_duplicate_state(connector, &state->base);
> +
> +	return &state->base;
>  }
>  
>  static const struct drm_connector_funcs intel_tv_connector_funcs = {
> @@ -1520,11 +1551,12 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = {
>  	.late_register = intel_connector_register,
>  	.early_unregister = intel_connector_unregister,
>  	.destroy = intel_tv_destroy,
> -	.set_property = intel_tv_set_property,
> -	.atomic_get_property = intel_connector_atomic_get_property,
> +	.set_property = drm_atomic_helper_connector_set_property,
> +	.atomic_set_property = intel_tv_atomic_set_property,
> +	.atomic_get_property = intel_tv_atomic_get_property,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_duplicate_state = intel_tv_duplicate_state,
>  };
>  
>  static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = {
> @@ -1547,6 +1579,7 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  	u32 tv_dac_on, tv_dac_off, save_tv_dac;
>  	const char *tv_format_names[ARRAY_SIZE(tv_modes)];
>  	int i, initial_mode = 0;
> +	struct intel_tv_connector_state *state;
>  
>  	if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED)
>  		return;
> @@ -1580,16 +1613,17 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  		return;
>  
>  	intel_tv = kzalloc(sizeof(*intel_tv), GFP_KERNEL);
> -	if (!intel_tv) {
> -		return;
> -	}
> -
> -	intel_connector = intel_connector_alloc();
> -	if (!intel_connector) {
> +	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
> +	state = kzalloc(sizeof(*state), GFP_KERNEL);
> +	if (!intel_tv || !intel_connector || !state) {
> +		kfree(intel_connector);
> +		kfree(state);
>  		kfree(intel_tv);
>  		return;
>  	}
>  
> +	__drm_atomic_helper_connector_reset(&intel_connector->base, &state->base);
> +
>  	intel_encoder = &intel_tv->base;
>  	connector = &intel_connector->base;
>  
> @@ -1629,12 +1663,12 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  	intel_tv->type = DRM_MODE_CONNECTOR_Unknown;
>  
>  	/* BIOS margin values */
> -	intel_tv->margin[TV_MARGIN_LEFT] = 54;
> -	intel_tv->margin[TV_MARGIN_TOP] = 36;
> -	intel_tv->margin[TV_MARGIN_RIGHT] = 46;
> -	intel_tv->margin[TV_MARGIN_BOTTOM] = 37;
> +	state->margin[TV_MARGIN_LEFT] = 54;
> +	state->margin[TV_MARGIN_TOP] = 36;
> +	state->margin[TV_MARGIN_RIGHT] = 46;
> +	state->margin[TV_MARGIN_BOTTOM] = 37;
>  
> -	intel_tv->tv_format = tv_modes[initial_mode].name;
> +	state->format = initial_mode;
>  
>  	drm_connector_helper_add(connector, &intel_tv_connector_helper_funcs);
>  	connector->interlace_allowed = false;
> @@ -1648,17 +1682,17 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  				      tv_format_names);
>  
>  	drm_object_attach_property(&connector->base, dev->mode_config.tv_mode_property,
> -				   initial_mode);
> +				   state->format);
>  	drm_object_attach_property(&connector->base,
>  				   dev->mode_config.tv_left_margin_property,
> -				   intel_tv->margin[TV_MARGIN_LEFT]);
> +				   state->margin[TV_MARGIN_LEFT]);
>  	drm_object_attach_property(&connector->base,
>  				   dev->mode_config.tv_top_margin_property,
> -				   intel_tv->margin[TV_MARGIN_TOP]);
> +				   state->margin[TV_MARGIN_TOP]);
>  	drm_object_attach_property(&connector->base,
>  				   dev->mode_config.tv_right_margin_property,
> -				   intel_tv->margin[TV_MARGIN_RIGHT]);
> +				   state->margin[TV_MARGIN_RIGHT]);
>  	drm_object_attach_property(&connector->base,
>  				   dev->mode_config.tv_bottom_margin_property,
> -				   intel_tv->margin[TV_MARGIN_BOTTOM]);
> +				   state->margin[TV_MARGIN_BOTTOM]);
>  }
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic.

[Maarten Lankhorst]

Op 09-03-17 om 18:36 schreef Ville Syrjälä:
> On Thu, Mar 09, 2017 at 02:06:15PM +0100, Maarten Lankhorst wrote:
>> As a proof of concept, first try to convert intel_tv, which is a rarely
>> used connector. It has 5 properties, tv format and 4 margins.
> Since it's so rare, if you want someone to actually test the code
> it'll probably make sense to pick another connector ;)
Yeah but the properties are among the most annoying, with the self modifying code and using state in mode_detect().
>> I'm less certain about the state behavior itself, should we pass a size
>> parameter to intel_connector_alloc instead, so duplicate_state
>> can be done globally if it can be blindly copied?
>>
>> Can we also have a atomic_check function for connectors, so the
>> crtc_state->connectors_changed can be set there? It would be cleaner
>> and more atomic-like.
> Hmm. I think it migth be really useful only if we have some
> interactions between multiple properties that really need to be
> checked. We might have those already I suppose but we don't seem
> to check any of it currently. So as a first step I guess we can
> just keep ignoring any such issues.
Well it might be, for example not all properties may be set yet so you can only do a sane check in a separate step.
>> To match the legacy behavior, format can be changed by probing just like
>> in legacy mode.
> Self modifying state irks me, but it's what we've been doing so I guess
> we should keep it.
Yeah, I hate it too.
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_tv.c | 238 +++++++++++++++++++++++-----------------
>>  1 file changed, 136 insertions(+), 102 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
>> index 6ed1a3ce47b7..0fb1d8621fe8 100644
>> --- a/drivers/gpu/drm/i915/intel_tv.c
>> +++ b/drivers/gpu/drm/i915/intel_tv.c
>> @@ -48,8 +48,6 @@ struct intel_tv {
>>  	struct intel_encoder base;
>>  
>>  	int type;
>> -	const char *tv_format;
>> -	int margin[4];
>>  	u32 save_TV_H_CTL_1;
>>  	u32 save_TV_H_CTL_2;
>>  	u32 save_TV_H_CTL_3;
>> @@ -85,6 +83,16 @@ struct intel_tv {
>>  	u32 save_TV_CTL;
>>  };
>>  
>> +struct intel_tv_connector_state {
>> +	struct drm_connector_state base;
>> +
>> +	int format;
>> +	int margin[4];
>> +};
>> +
>> +#define to_intel_tv_connector_state(state) \
>> +	container_of((state), struct intel_tv_connector_state, base)
>> +
>>  struct video_levels {
>>  	u16 blank, black;
>>  	u8 burst;
>> @@ -873,32 +881,18 @@ intel_disable_tv(struct intel_encoder *encoder,
>>  	I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
>>  }
>>  
>> -static const struct tv_mode *
>> -intel_tv_mode_lookup(const char *tv_format)
>> +static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
>>  {
>> -	int i;
>> +	int format = to_intel_tv_connector_state(conn_state)->format;
>>  
>> -	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
>> -		const struct tv_mode *tv_mode = &tv_modes[i];
>> -
>> -		if (!strcmp(tv_format, tv_mode->name))
>> -			return tv_mode;
>> -	}
>> -	return NULL;
>> -}
>> -
>> -static const struct tv_mode *
>> -intel_tv_mode_find(struct intel_tv *intel_tv)
>> -{
>> -	return intel_tv_mode_lookup(intel_tv->tv_format);
>> +	return &tv_modes[format];
>>  }
>>  
>>  static enum drm_mode_status
>>  intel_tv_mode_valid(struct drm_connector *connector,
>>  		    struct drm_display_mode *mode)
>>  {
>> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>> +	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
> It feels a bit fishy to use the state here. Generally that's a no-no.
> But in this case I wonder if it's the right choice after all. 
>
> Not sure if some kind of "automatic" enum value might also work. It
> would at least avoid the self modifying property problem. Although I
> wonder if the user would still like to know what was actually used
> if they chose they automatic mode, so we might need a self modifying
> RO property for the current mode anyway.
>
> But that still leaves the problem of how the user would know which modes
> they should be able to use if .get_modes()/.mode_valid() doesn't respect
> the users choice of the tv format. Hmm, tricky. Might be the self
> modifying property is the only good choice.
>
> But if we would use the state here, what's the story with locking going
> to be? connection_mutex is what protects this stuff, but we're not
> holding that during mode enumeration.
Yeah locking is tricky, honestly I have no idea what would be the right thing to do here..

I don't really see a good solution, or at least one that would work correctly with atomic properties.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic.

[Ville Syrjälä]

On Mon, Mar 13, 2017 at 10:22:51AM +0100, Maarten Lankhorst wrote:
> Op 09-03-17 om 18:36 schreef Ville Syrjälä:
> > On Thu, Mar 09, 2017 at 02:06:15PM +0100, Maarten Lankhorst wrote:
> >> As a proof of concept, first try to convert intel_tv, which is a rarely
> >> used connector. It has 5 properties, tv format and 4 margins.
> > Since it's so rare, if you want someone to actually test the code
> > it'll probably make sense to pick another connector ;)
> Yeah but the properties are among the most annoying, with the self modifying code and using state in mode_detect().
> >> I'm less certain about the state behavior itself, should we pass a size
> >> parameter to intel_connector_alloc instead, so duplicate_state
> >> can be done globally if it can be blindly copied?
> >>
> >> Can we also have a atomic_check function for connectors, so the
> >> crtc_state->connectors_changed can be set there? It would be cleaner
> >> and more atomic-like.
> > Hmm. I think it migth be really useful only if we have some
> > interactions between multiple properties that really need to be
> > checked. We might have those already I suppose but we don't seem
> > to check any of it currently. So as a first step I guess we can
> > just keep ignoring any such issues.
> Well it might be, for example not all properties may be set yet so you can only do a sane check in a separate step.
> >> To match the legacy behavior, format can be changed by probing just like
> >> in legacy mode.
> > Self modifying state irks me, but it's what we've been doing so I guess
> > we should keep it.
> Yeah, I hate it too.
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_tv.c | 238 +++++++++++++++++++++++-----------------
> >>  1 file changed, 136 insertions(+), 102 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> >> index 6ed1a3ce47b7..0fb1d8621fe8 100644
> >> --- a/drivers/gpu/drm/i915/intel_tv.c
> >> +++ b/drivers/gpu/drm/i915/intel_tv.c
> >> @@ -48,8 +48,6 @@ struct intel_tv {
> >>  	struct intel_encoder base;
> >>  
> >>  	int type;
> >> -	const char *tv_format;
> >> -	int margin[4];
> >>  	u32 save_TV_H_CTL_1;
> >>  	u32 save_TV_H_CTL_2;
> >>  	u32 save_TV_H_CTL_3;
> >> @@ -85,6 +83,16 @@ struct intel_tv {
> >>  	u32 save_TV_CTL;
> >>  };
> >>  
> >> +struct intel_tv_connector_state {
> >> +	struct drm_connector_state base;
> >> +
> >> +	int format;
> >> +	int margin[4];
> >> +};
> >> +
> >> +#define to_intel_tv_connector_state(state) \
> >> +	container_of((state), struct intel_tv_connector_state, base)
> >> +
> >>  struct video_levels {
> >>  	u16 blank, black;
> >>  	u8 burst;
> >> @@ -873,32 +881,18 @@ intel_disable_tv(struct intel_encoder *encoder,
> >>  	I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
> >>  }
> >>  
> >> -static const struct tv_mode *
> >> -intel_tv_mode_lookup(const char *tv_format)
> >> +static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
> >>  {
> >> -	int i;
> >> +	int format = to_intel_tv_connector_state(conn_state)->format;
> >>  
> >> -	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
> >> -		const struct tv_mode *tv_mode = &tv_modes[i];
> >> -
> >> -		if (!strcmp(tv_format, tv_mode->name))
> >> -			return tv_mode;
> >> -	}
> >> -	return NULL;
> >> -}
> >> -
> >> -static const struct tv_mode *
> >> -intel_tv_mode_find(struct intel_tv *intel_tv)
> >> -{
> >> -	return intel_tv_mode_lookup(intel_tv->tv_format);
> >> +	return &tv_modes[format];
> >>  }
> >>  
> >>  static enum drm_mode_status
> >>  intel_tv_mode_valid(struct drm_connector *connector,
> >>  		    struct drm_display_mode *mode)
> >>  {
> >> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
> >> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> >> +	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
> > It feels a bit fishy to use the state here. Generally that's a no-no.
> > But in this case I wonder if it's the right choice after all. 
> >
> > Not sure if some kind of "automatic" enum value might also work. It
> > would at least avoid the self modifying property problem. Although I
> > wonder if the user would still like to know what was actually used
> > if they chose they automatic mode, so we might need a self modifying
> > RO property for the current mode anyway.
> >
> > But that still leaves the problem of how the user would know which modes
> > they should be able to use if .get_modes()/.mode_valid() doesn't respect
> > the users choice of the tv format. Hmm, tricky. Might be the self
> > modifying property is the only good choice.
> >
> > But if we would use the state here, what's the story with locking going
> > to be? connection_mutex is what protects this stuff, but we're not
> > holding that during mode enumeration.
> Yeah locking is tricky, honestly I have no idea what would be the right thing to do here..
> 
> I don't really see a good solution, or at least one that would work correctly with atomic properties.

Maybe we need to keep the format information in both intel_tv and the
state. We'd use the intel_tv->format during detect, and during
duplicate_state we'd do 'state->format = intel_tv->format' and during
commit we'd do 'intel_tv->format = state->format' ?

Still self modifying, and somewhat racy still, but at least we
shouldn't explode on account of the connector->state dereference.

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

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic.

[Maarten Lankhorst]

Op 13-03-17 om 10:29 schreef Ville Syrjälä:
> On Mon, Mar 13, 2017 at 10:22:51AM +0100, Maarten Lankhorst wrote:
>> Op 09-03-17 om 18:36 schreef Ville Syrjälä:
>>> On Thu, Mar 09, 2017 at 02:06:15PM +0100, Maarten Lankhorst wrote:
>>>> As a proof of concept, first try to convert intel_tv, which is a rarely
>>>> used connector. It has 5 properties, tv format and 4 margins.
>>> Since it's so rare, if you want someone to actually test the code
>>> it'll probably make sense to pick another connector ;)
>> Yeah but the properties are among the most annoying, with the self modifying code and using state in mode_detect().
>>>> I'm less certain about the state behavior itself, should we pass a size
>>>> parameter to intel_connector_alloc instead, so duplicate_state
>>>> can be done globally if it can be blindly copied?
>>>>
>>>> Can we also have a atomic_check function for connectors, so the
>>>> crtc_state->connectors_changed can be set there? It would be cleaner
>>>> and more atomic-like.
>>> Hmm. I think it migth be really useful only if we have some
>>> interactions between multiple properties that really need to be
>>> checked. We might have those already I suppose but we don't seem
>>> to check any of it currently. So as a first step I guess we can
>>> just keep ignoring any such issues.
>> Well it might be, for example not all properties may be set yet so you can only do a sane check in a separate step.
>>>> To match the legacy behavior, format can be changed by probing just like
>>>> in legacy mode.
>>> Self modifying state irks me, but it's what we've been doing so I guess
>>> we should keep it.
>> Yeah, I hate it too.
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_tv.c | 238 +++++++++++++++++++++++-----------------
>>>>  1 file changed, 136 insertions(+), 102 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
>>>> index 6ed1a3ce47b7..0fb1d8621fe8 100644
>>>> --- a/drivers/gpu/drm/i915/intel_tv.c
>>>> +++ b/drivers/gpu/drm/i915/intel_tv.c
>>>> @@ -48,8 +48,6 @@ struct intel_tv {
>>>>  	struct intel_encoder base;
>>>>  
>>>>  	int type;
>>>> -	const char *tv_format;
>>>> -	int margin[4];
>>>>  	u32 save_TV_H_CTL_1;
>>>>  	u32 save_TV_H_CTL_2;
>>>>  	u32 save_TV_H_CTL_3;
>>>> @@ -85,6 +83,16 @@ struct intel_tv {
>>>>  	u32 save_TV_CTL;
>>>>  };
>>>>  
>>>> +struct intel_tv_connector_state {
>>>> +	struct drm_connector_state base;
>>>> +
>>>> +	int format;
>>>> +	int margin[4];
>>>> +};
>>>> +
>>>> +#define to_intel_tv_connector_state(state) \
>>>> +	container_of((state), struct intel_tv_connector_state, base)
>>>> +
>>>>  struct video_levels {
>>>>  	u16 blank, black;
>>>>  	u8 burst;
>>>> @@ -873,32 +881,18 @@ intel_disable_tv(struct intel_encoder *encoder,
>>>>  	I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
>>>>  }
>>>>  
>>>> -static const struct tv_mode *
>>>> -intel_tv_mode_lookup(const char *tv_format)
>>>> +static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
>>>>  {
>>>> -	int i;
>>>> +	int format = to_intel_tv_connector_state(conn_state)->format;
>>>>  
>>>> -	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
>>>> -		const struct tv_mode *tv_mode = &tv_modes[i];
>>>> -
>>>> -		if (!strcmp(tv_format, tv_mode->name))
>>>> -			return tv_mode;
>>>> -	}
>>>> -	return NULL;
>>>> -}
>>>> -
>>>> -static const struct tv_mode *
>>>> -intel_tv_mode_find(struct intel_tv *intel_tv)
>>>> -{
>>>> -	return intel_tv_mode_lookup(intel_tv->tv_format);
>>>> +	return &tv_modes[format];
>>>>  }
>>>>  
>>>>  static enum drm_mode_status
>>>>  intel_tv_mode_valid(struct drm_connector *connector,
>>>>  		    struct drm_display_mode *mode)
>>>>  {
>>>> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
>>>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>>>> +	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
>>> It feels a bit fishy to use the state here. Generally that's a no-no.
>>> But in this case I wonder if it's the right choice after all. 
>>>
>>> Not sure if some kind of "automatic" enum value might also work. It
>>> would at least avoid the self modifying property problem. Although I
>>> wonder if the user would still like to know what was actually used
>>> if they chose they automatic mode, so we might need a self modifying
>>> RO property for the current mode anyway.
>>>
>>> But that still leaves the problem of how the user would know which modes
>>> they should be able to use if .get_modes()/.mode_valid() doesn't respect
>>> the users choice of the tv format. Hmm, tricky. Might be the self
>>> modifying property is the only good choice.
>>>
>>> But if we would use the state here, what's the story with locking going
>>> to be? connection_mutex is what protects this stuff, but we're not
>>> holding that during mode enumeration.
>> Yeah locking is tricky, honestly I have no idea what would be the right thing to do here..
>>
>> I don't really see a good solution, or at least one that would work correctly with atomic properties.
> Maybe we need to keep the format information in both intel_tv and the
> state. We'd use the intel_tv->format during detect, and during
> duplicate_state we'd do 'state->format = intel_tv->format' and during
> commit we'd do 'intel_tv->format = state->format' ?
>
> Still self modifying, and somewhat racy still, but at least we
> shouldn't explode on account of the connector->state dereference.
>
I thought about that, but where do you want to update it in atomic commit?

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

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic.

[Ville Syrjälä]

On Mon, Mar 13, 2017 at 11:38:33AM +0100, Maarten Lankhorst wrote:
> Op 13-03-17 om 10:29 schreef Ville Syrjälä:
> > On Mon, Mar 13, 2017 at 10:22:51AM +0100, Maarten Lankhorst wrote:
> >> Op 09-03-17 om 18:36 schreef Ville Syrjälä:
> >>> On Thu, Mar 09, 2017 at 02:06:15PM +0100, Maarten Lankhorst wrote:
> >>>> As a proof of concept, first try to convert intel_tv, which is a rarely
> >>>> used connector. It has 5 properties, tv format and 4 margins.
> >>> Since it's so rare, if you want someone to actually test the code
> >>> it'll probably make sense to pick another connector ;)
> >> Yeah but the properties are among the most annoying, with the self modifying code and using state in mode_detect().
> >>>> I'm less certain about the state behavior itself, should we pass a size
> >>>> parameter to intel_connector_alloc instead, so duplicate_state
> >>>> can be done globally if it can be blindly copied?
> >>>>
> >>>> Can we also have a atomic_check function for connectors, so the
> >>>> crtc_state->connectors_changed can be set there? It would be cleaner
> >>>> and more atomic-like.
> >>> Hmm. I think it migth be really useful only if we have some
> >>> interactions between multiple properties that really need to be
> >>> checked. We might have those already I suppose but we don't seem
> >>> to check any of it currently. So as a first step I guess we can
> >>> just keep ignoring any such issues.
> >> Well it might be, for example not all properties may be set yet so you can only do a sane check in a separate step.
> >>>> To match the legacy behavior, format can be changed by probing just like
> >>>> in legacy mode.
> >>> Self modifying state irks me, but it's what we've been doing so I guess
> >>> we should keep it.
> >> Yeah, I hate it too.
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/intel_tv.c | 238 +++++++++++++++++++++++-----------------
> >>>>  1 file changed, 136 insertions(+), 102 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> >>>> index 6ed1a3ce47b7..0fb1d8621fe8 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_tv.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_tv.c
> >>>> @@ -48,8 +48,6 @@ struct intel_tv {
> >>>>  	struct intel_encoder base;
> >>>>  
> >>>>  	int type;
> >>>> -	const char *tv_format;
> >>>> -	int margin[4];
> >>>>  	u32 save_TV_H_CTL_1;
> >>>>  	u32 save_TV_H_CTL_2;
> >>>>  	u32 save_TV_H_CTL_3;
> >>>> @@ -85,6 +83,16 @@ struct intel_tv {
> >>>>  	u32 save_TV_CTL;
> >>>>  };
> >>>>  
> >>>> +struct intel_tv_connector_state {
> >>>> +	struct drm_connector_state base;
> >>>> +
> >>>> +	int format;
> >>>> +	int margin[4];
> >>>> +};
> >>>> +
> >>>> +#define to_intel_tv_connector_state(state) \
> >>>> +	container_of((state), struct intel_tv_connector_state, base)
> >>>> +
> >>>>  struct video_levels {
> >>>>  	u16 blank, black;
> >>>>  	u8 burst;
> >>>> @@ -873,32 +881,18 @@ intel_disable_tv(struct intel_encoder *encoder,
> >>>>  	I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
> >>>>  }
> >>>>  
> >>>> -static const struct tv_mode *
> >>>> -intel_tv_mode_lookup(const char *tv_format)
> >>>> +static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
> >>>>  {
> >>>> -	int i;
> >>>> +	int format = to_intel_tv_connector_state(conn_state)->format;
> >>>>  
> >>>> -	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
> >>>> -		const struct tv_mode *tv_mode = &tv_modes[i];
> >>>> -
> >>>> -		if (!strcmp(tv_format, tv_mode->name))
> >>>> -			return tv_mode;
> >>>> -	}
> >>>> -	return NULL;
> >>>> -}
> >>>> -
> >>>> -static const struct tv_mode *
> >>>> -intel_tv_mode_find(struct intel_tv *intel_tv)
> >>>> -{
> >>>> -	return intel_tv_mode_lookup(intel_tv->tv_format);
> >>>> +	return &tv_modes[format];
> >>>>  }
> >>>>  
> >>>>  static enum drm_mode_status
> >>>>  intel_tv_mode_valid(struct drm_connector *connector,
> >>>>  		    struct drm_display_mode *mode)
> >>>>  {
> >>>> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
> >>>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> >>>> +	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
> >>> It feels a bit fishy to use the state here. Generally that's a no-no.
> >>> But in this case I wonder if it's the right choice after all. 
> >>>
> >>> Not sure if some kind of "automatic" enum value might also work. It
> >>> would at least avoid the self modifying property problem. Although I
> >>> wonder if the user would still like to know what was actually used
> >>> if they chose they automatic mode, so we might need a self modifying
> >>> RO property for the current mode anyway.
> >>>
> >>> But that still leaves the problem of how the user would know which modes
> >>> they should be able to use if .get_modes()/.mode_valid() doesn't respect
> >>> the users choice of the tv format. Hmm, tricky. Might be the self
> >>> modifying property is the only good choice.
> >>>
> >>> But if we would use the state here, what's the story with locking going
> >>> to be? connection_mutex is what protects this stuff, but we're not
> >>> holding that during mode enumeration.
> >> Yeah locking is tricky, honestly I have no idea what would be the right thing to do here..
> >>
> >> I don't really see a good solution, or at least one that would work correctly with atomic properties.
> > Maybe we need to keep the format information in both intel_tv and the
> > state. We'd use the intel_tv->format during detect, and during
> > duplicate_state we'd do 'state->format = intel_tv->format' and during
> > commit we'd do 'intel_tv->format = state->format' ?
> >
> > Still self modifying, and somewhat racy still, but at least we
> > shouldn't explode on account of the connector->state dereference.
> >
> I thought about that, but where do you want to update it in atomic commit?

Does it matter? Somewhere around the swap_state I suppose?

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

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic.

[Maarten Lankhorst]

Op 13-03-17 om 11:55 schreef Ville Syrjälä:
> On Mon, Mar 13, 2017 at 11:38:33AM +0100, Maarten Lankhorst wrote:
>> Op 13-03-17 om 10:29 schreef Ville Syrjälä:
>>> On Mon, Mar 13, 2017 at 10:22:51AM +0100, Maarten Lankhorst wrote:
>>>> Op 09-03-17 om 18:36 schreef Ville Syrjälä:
>>>>> On Thu, Mar 09, 2017 at 02:06:15PM +0100, Maarten Lankhorst wrote:
>>>>>> As a proof of concept, first try to convert intel_tv, which is a rarely
>>>>>> used connector. It has 5 properties, tv format and 4 margins.
>>>>> Since it's so rare, if you want someone to actually test the code
>>>>> it'll probably make sense to pick another connector ;)
>>>> Yeah but the properties are among the most annoying, with the self modifying code and using state in mode_detect().
>>>>>> I'm less certain about the state behavior itself, should we pass a size
>>>>>> parameter to intel_connector_alloc instead, so duplicate_state
>>>>>> can be done globally if it can be blindly copied?
>>>>>>
>>>>>> Can we also have a atomic_check function for connectors, so the
>>>>>> crtc_state->connectors_changed can be set there? It would be cleaner
>>>>>> and more atomic-like.
>>>>> Hmm. I think it migth be really useful only if we have some
>>>>> interactions between multiple properties that really need to be
>>>>> checked. We might have those already I suppose but we don't seem
>>>>> to check any of it currently. So as a first step I guess we can
>>>>> just keep ignoring any such issues.
>>>> Well it might be, for example not all properties may be set yet so you can only do a sane check in a separate step.
>>>>>> To match the legacy behavior, format can be changed by probing just like
>>>>>> in legacy mode.
>>>>> Self modifying state irks me, but it's what we've been doing so I guess
>>>>> we should keep it.
>>>> Yeah, I hate it too.
>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/i915/intel_tv.c | 238 +++++++++++++++++++++++-----------------
>>>>>>  1 file changed, 136 insertions(+), 102 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
>>>>>> index 6ed1a3ce47b7..0fb1d8621fe8 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_tv.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_tv.c
>>>>>> @@ -48,8 +48,6 @@ struct intel_tv {
>>>>>>  	struct intel_encoder base;
>>>>>>  
>>>>>>  	int type;
>>>>>> -	const char *tv_format;
>>>>>> -	int margin[4];
>>>>>>  	u32 save_TV_H_CTL_1;
>>>>>>  	u32 save_TV_H_CTL_2;
>>>>>>  	u32 save_TV_H_CTL_3;
>>>>>> @@ -85,6 +83,16 @@ struct intel_tv {
>>>>>>  	u32 save_TV_CTL;
>>>>>>  };
>>>>>>  
>>>>>> +struct intel_tv_connector_state {
>>>>>> +	struct drm_connector_state base;
>>>>>> +
>>>>>> +	int format;
>>>>>> +	int margin[4];
>>>>>> +};
>>>>>> +
>>>>>> +#define to_intel_tv_connector_state(state) \
>>>>>> +	container_of((state), struct intel_tv_connector_state, base)
>>>>>> +
>>>>>>  struct video_levels {
>>>>>>  	u16 blank, black;
>>>>>>  	u8 burst;
>>>>>> @@ -873,32 +881,18 @@ intel_disable_tv(struct intel_encoder *encoder,
>>>>>>  	I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
>>>>>>  }
>>>>>>  
>>>>>> -static const struct tv_mode *
>>>>>> -intel_tv_mode_lookup(const char *tv_format)
>>>>>> +static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
>>>>>>  {
>>>>>> -	int i;
>>>>>> +	int format = to_intel_tv_connector_state(conn_state)->format;
>>>>>>  
>>>>>> -	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
>>>>>> -		const struct tv_mode *tv_mode = &tv_modes[i];
>>>>>> -
>>>>>> -		if (!strcmp(tv_format, tv_mode->name))
>>>>>> -			return tv_mode;
>>>>>> -	}
>>>>>> -	return NULL;
>>>>>> -}
>>>>>> -
>>>>>> -static const struct tv_mode *
>>>>>> -intel_tv_mode_find(struct intel_tv *intel_tv)
>>>>>> -{
>>>>>> -	return intel_tv_mode_lookup(intel_tv->tv_format);
>>>>>> +	return &tv_modes[format];
>>>>>>  }
>>>>>>  
>>>>>>  static enum drm_mode_status
>>>>>>  intel_tv_mode_valid(struct drm_connector *connector,
>>>>>>  		    struct drm_display_mode *mode)
>>>>>>  {
>>>>>> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
>>>>>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>>>>>> +	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
>>>>> It feels a bit fishy to use the state here. Generally that's a no-no.
>>>>> But in this case I wonder if it's the right choice after all. 
>>>>>
>>>>> Not sure if some kind of "automatic" enum value might also work. It
>>>>> would at least avoid the self modifying property problem. Although I
>>>>> wonder if the user would still like to know what was actually used
>>>>> if they chose they automatic mode, so we might need a self modifying
>>>>> RO property for the current mode anyway.
>>>>>
>>>>> But that still leaves the problem of how the user would know which modes
>>>>> they should be able to use if .get_modes()/.mode_valid() doesn't respect
>>>>> the users choice of the tv format. Hmm, tricky. Might be the self
>>>>> modifying property is the only good choice.
>>>>>
>>>>> But if we would use the state here, what's the story with locking going
>>>>> to be? connection_mutex is what protects this stuff, but we're not
>>>>> holding that during mode enumeration.
>>>> Yeah locking is tricky, honestly I have no idea what would be the right thing to do here..
>>>>
>>>> I don't really see a good solution, or at least one that would work correctly with atomic properties.
>>> Maybe we need to keep the format information in both intel_tv and the
>>> state. We'd use the intel_tv->format during detect, and during
>>> duplicate_state we'd do 'state->format = intel_tv->format' and during
>>> commit we'd do 'intel_tv->format = state->format' ?
>>>
>>> Still self modifying, and somewhat racy still, but at least we
>>> shouldn't explode on account of the connector->state dereference.
>>>
>> I thought about that, but where do you want to update it in atomic commit?
> Does it matter? Somewhere around the swap_state I suppose?
>
Hmm, I guess I'll add a connector->set_state hook for this, lets see how far I can get with a less racy v2..

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

[PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2.

[Maarten Lankhorst]

As a proof of concept, first try to convert intel_tv, which is a rarely
used connector. It has 5 properties, tv format and 4 margins.

I'm less certain about the state behavior itself, should we pass a size
parameter to intel_connector_alloc instead, so duplicate_state
can be done globally if it can be blindly copied?

Can we also have a atomic_check function for connectors, so the
crtc_state->connectors_changed can be set there? It would be cleaner
and more atomic-like.

To match the legacy behavior, format can be changed by probing just like
in legacy mode.

Changes since v1:
- Add intel_encoder->swap_state to allow updating connector state.
- Add intel_tv->format for detect_mode and mode_valid, updated on atomic commit.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  15 ++
 drivers/gpu/drm/i915/intel_drv.h     |   4 +
 drivers/gpu/drm/i915/intel_tv.c      | 259 +++++++++++++++++++++--------------
 3 files changed, 176 insertions(+), 102 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ac25c9bc8b81..18b7e7546ee1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12954,6 +12954,20 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
 				  to_intel_plane(plane)->frontbuffer_bit);
 }
 
+static void swap_connector_state(struct drm_atomic_state *state)
+{
+	struct drm_connector *connector;
+	struct drm_connector_state *new_conn_state;
+	int i;
+
+	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
+		struct intel_connector *conn = to_intel_connector(connector);
+
+		if (conn->swap_state)
+			conn->swap_state(conn, new_conn_state);
+	}
+}
+
 /**
  * intel_atomic_commit - commit validated state object
  * @dev: DRM device
@@ -13012,6 +13026,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		dev_priv->cdclk.logical = intel_state->cdclk.logical;
 		dev_priv->cdclk.actual = intel_state->cdclk.actual;
 	}
+	swap_connector_state(state);
 
 	drm_atomic_state_get(state);
 	INIT_WORK(&state->commit_work,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 937623ff6d7c..b7b93799d288 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -307,6 +307,10 @@ struct intel_connector {
 	 * and active (i.e. dpms ON state). */
 	bool (*get_hw_state)(struct intel_connector *);
 
+	/* Update device state with the new atomic state. */
+	void (*swap_state)(struct intel_connector *,
+			     struct drm_connector_state *);
+
 	/* Panel info for eDP and LVDS */
 	struct intel_panel panel;
 
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 6ed1a3ce47b7..b87d51e61975 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -48,8 +48,6 @@ struct intel_tv {
 	struct intel_encoder base;
 
 	int type;
-	const char *tv_format;
-	int margin[4];
 	u32 save_TV_H_CTL_1;
 	u32 save_TV_H_CTL_2;
 	u32 save_TV_H_CTL_3;
@@ -83,8 +81,20 @@ struct intel_tv {
 
 	u32 save_TV_DAC;
 	u32 save_TV_CTL;
+
+	int format;
+};
+
+struct intel_tv_connector_state {
+	struct drm_connector_state base;
+
+	int format;
+	int margin[4];
 };
 
+#define to_intel_tv_connector_state(state) \
+	container_of((state), struct intel_tv_connector_state, base)
+
 struct video_levels {
 	u16 blank, black;
 	u8 burst;
@@ -848,6 +858,17 @@ intel_tv_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe)
 }
 
 static void
+intel_tv_swap_state(struct intel_connector *connector,
+		    struct drm_connector_state *conn_state)
+{
+	struct intel_tv *intel_tv = intel_attached_tv(&connector->base);
+	struct intel_tv_connector_state *tv_state =
+		to_intel_tv_connector_state(conn_state);
+
+	intel_tv->format = tv_state->format;
+}
+
+static void
 intel_enable_tv(struct intel_encoder *encoder,
 		struct intel_crtc_state *pipe_config,
 		struct drm_connector_state *conn_state)
@@ -873,32 +894,25 @@ intel_disable_tv(struct intel_encoder *encoder,
 	I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
 }
 
-static const struct tv_mode *
-intel_tv_mode_lookup(const char *tv_format)
+static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
 {
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
-		const struct tv_mode *tv_mode = &tv_modes[i];
+	int format = to_intel_tv_connector_state(conn_state)->format;
 
-		if (!strcmp(tv_format, tv_mode->name))
-			return tv_mode;
-	}
-	return NULL;
+	return &tv_modes[format];
 }
 
-static const struct tv_mode *
-intel_tv_mode_find(struct intel_tv *intel_tv)
+static const struct tv_mode *intel_tv_mode_find_committed(struct drm_connector *connector)
 {
-	return intel_tv_mode_lookup(intel_tv->tv_format);
+	int format = intel_attached_tv(connector)->format;
+
+	return &tv_modes[format];
 }
 
 static enum drm_mode_status
 intel_tv_mode_valid(struct drm_connector *connector,
 		    struct drm_display_mode *mode)
 {
-	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find_committed(connector);
 	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
 
 	if (mode->clock > max_dotclk)
@@ -925,8 +939,7 @@ intel_tv_compute_config(struct intel_encoder *encoder,
 			struct intel_crtc_state *pipe_config,
 			struct drm_connector_state *conn_state)
 {
-	struct intel_tv *intel_tv = enc_to_tv(encoder);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
 
 	if (!tv_mode)
 		return false;
@@ -1032,7 +1045,7 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
 	struct intel_tv *intel_tv = enc_to_tv(encoder);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
 	u32 tv_ctl;
 	u32 scctl1, scctl2, scctl3;
 	int i, j;
@@ -1041,6 +1054,8 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
 	bool burst_ena;
 	int xpos = 0x0, ypos = 0x0;
 	unsigned int xsize, ysize;
+	struct intel_tv_connector_state *tv_state =
+		to_intel_tv_connector_state(conn_state);
 
 	if (!tv_mode)
 		return;	/* can't happen (mode_prepare prevents this) */
@@ -1135,12 +1150,12 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
 	else
 		ysize = 2*tv_mode->nbr_end + 1;
 
-	xpos += intel_tv->margin[TV_MARGIN_LEFT];
-	ypos += intel_tv->margin[TV_MARGIN_TOP];
-	xsize -= (intel_tv->margin[TV_MARGIN_LEFT] +
-		  intel_tv->margin[TV_MARGIN_RIGHT]);
-	ysize -= (intel_tv->margin[TV_MARGIN_TOP] +
-		  intel_tv->margin[TV_MARGIN_BOTTOM]);
+	xpos += tv_state->margin[TV_MARGIN_LEFT];
+	ypos += tv_state->margin[TV_MARGIN_TOP];
+	xsize -= (tv_state->margin[TV_MARGIN_LEFT] +
+		  tv_state->margin[TV_MARGIN_RIGHT]);
+	ysize -= (tv_state->margin[TV_MARGIN_TOP] +
+		  tv_state->margin[TV_MARGIN_BOTTOM]);
 	I915_WRITE(TV_WIN_POS, (xpos<<16)|ypos);
 	I915_WRITE(TV_WIN_SIZE, (xsize<<16)|ysize);
 
@@ -1288,7 +1303,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
 static void intel_tv_find_better_format(struct drm_connector *connector)
 {
 	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
 	int i;
 
 	if ((intel_tv->type == DRM_MODE_CONNECTOR_Component) ==
@@ -1304,9 +1319,8 @@ static void intel_tv_find_better_format(struct drm_connector *connector)
 			break;
 	}
 
-	intel_tv->tv_format = tv_mode->name;
-	drm_object_property_set_value(&connector->base,
-		connector->dev->mode_config.tv_mode_property, i);
+	to_intel_tv_connector_state(connector->state)->format = i;
+	intel_attached_tv(connector)->format = i;
 }
 
 /**
@@ -1344,18 +1358,17 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 		} else
 			status = connector_status_unknown;
 
+		if (status == connector_status_connected) {
+			intel_tv->type = type;
+			intel_tv_find_better_format(connector);
+		}
+
 		drm_modeset_drop_locks(&ctx);
 		drm_modeset_acquire_fini(&ctx);
-	} else
-		return connector->status;
 
-	if (status != connector_status_connected)
 		return status;
-
-	intel_tv->type = type;
-	intel_tv_find_better_format(connector);
-
-	return connector_status_connected;
+	} else
+		return connector->status;
 }
 
 static const struct input_res {
@@ -1375,12 +1388,9 @@ static const struct input_res {
  * Chose preferred mode  according to line number of TV format
  */
 static void
-intel_tv_chose_preferred_modes(struct drm_connector *connector,
+intel_tv_chose_preferred_modes(const struct tv_mode *tv_mode,
 			       struct drm_display_mode *mode_ptr)
 {
-	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
-
 	if (tv_mode->nbr_end < 480 && mode_ptr->vdisplay == 480)
 		mode_ptr->type |= DRM_MODE_TYPE_PREFERRED;
 	else if (tv_mode->nbr_end > 480) {
@@ -1403,8 +1413,7 @@ static int
 intel_tv_get_modes(struct drm_connector *connector)
 {
 	struct drm_display_mode *mode_ptr;
-	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find_committed(connector);
 	int j, count = 0;
 	u64 tmp;
 
@@ -1447,7 +1456,7 @@ intel_tv_get_modes(struct drm_connector *connector)
 		mode_ptr->clock = (int) tmp;
 
 		mode_ptr->type = DRM_MODE_TYPE_DRIVER;
-		intel_tv_chose_preferred_modes(connector, mode_ptr);
+		intel_tv_chose_preferred_modes(tv_mode, mode_ptr);
 		drm_mode_probed_add(connector, mode_ptr);
 		count++;
 	}
@@ -1462,56 +1471,97 @@ intel_tv_destroy(struct drm_connector *connector)
 	kfree(connector);
 }
 
-
 static int
-intel_tv_set_property(struct drm_connector *connector, struct drm_property *property,
-		      uint64_t val)
+intel_tv_atomic_set_property(struct drm_connector *connector,
+			     struct drm_connector_state *conn_state,
+			     struct drm_property *property,
+			     uint64_t val)
 {
 	struct drm_device *dev = connector->dev;
-	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	struct drm_crtc *crtc = intel_tv->base.base.crtc;
-	int ret = 0;
+
+	struct intel_tv_connector_state *tv_state =
+		to_intel_tv_connector_state(conn_state);
 	bool changed = false;
 
-	ret = drm_object_property_set_value(&connector->base, property, val);
-	if (ret < 0)
-		goto out;
-
-	if (property == dev->mode_config.tv_left_margin_property &&
-		intel_tv->margin[TV_MARGIN_LEFT] != val) {
-		intel_tv->margin[TV_MARGIN_LEFT] = val;
-		changed = true;
-	} else if (property == dev->mode_config.tv_right_margin_property &&
-		intel_tv->margin[TV_MARGIN_RIGHT] != val) {
-		intel_tv->margin[TV_MARGIN_RIGHT] = val;
-		changed = true;
-	} else if (property == dev->mode_config.tv_top_margin_property &&
-		intel_tv->margin[TV_MARGIN_TOP] != val) {
-		intel_tv->margin[TV_MARGIN_TOP] = val;
-		changed = true;
-	} else if (property == dev->mode_config.tv_bottom_margin_property &&
-		intel_tv->margin[TV_MARGIN_BOTTOM] != val) {
-		intel_tv->margin[TV_MARGIN_BOTTOM] = val;
-		changed = true;
+	if (property == dev->mode_config.tv_left_margin_property) {
+		changed = tv_state->margin[TV_MARGIN_LEFT] != val;
+
+		tv_state->margin[TV_MARGIN_LEFT] = val;
+	} else if (property == dev->mode_config.tv_right_margin_property) {
+		changed = tv_state->margin[TV_MARGIN_RIGHT] != val;
+
+		tv_state->margin[TV_MARGIN_RIGHT] = val;
+	} else if (property == dev->mode_config.tv_top_margin_property) {
+		changed = tv_state->margin[TV_MARGIN_TOP] != val;
+
+		tv_state->margin[TV_MARGIN_TOP] = val;
+	} else if (property == dev->mode_config.tv_bottom_margin_property) {
+		changed = tv_state->margin[TV_MARGIN_BOTTOM] != val;
+
+		tv_state->margin[TV_MARGIN_BOTTOM] = val;
 	} else if (property == dev->mode_config.tv_mode_property) {
 		if (val >= ARRAY_SIZE(tv_modes)) {
-			ret = -EINVAL;
-			goto out;
+			DRM_DEBUG_ATOMIC("value %llu out of tv_modes array bounds\n", val);
+
+			return -EINVAL;
 		}
-		if (!strcmp(intel_tv->tv_format, tv_modes[val].name))
-			goto out;
 
-		intel_tv->tv_format = tv_modes[val].name;
-		changed = true;
+		changed = tv_state->format != val;
+		tv_state->format = val;
+	} else {
+		DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name);
+		return -EINVAL;
+	}
+
+	/* Trigger a modeset when connector properties are changed. */
+	if (changed && conn_state->crtc) {
+		struct drm_crtc_state *crtc_state =
+			drm_atomic_get_existing_crtc_state(conn_state->state,
+							   conn_state->crtc);
+
+		crtc_state->connectors_changed = true;
+	}
+	return 0;
+}
+
+static int
+intel_tv_atomic_get_property(struct drm_connector *connector,
+				    const struct drm_connector_state *state,
+				    struct drm_property *property,
+				    uint64_t *val)
+{
+	struct drm_device *dev = connector->dev;
+	struct intel_tv_connector_state *tv_state =
+		to_intel_tv_connector_state(state);
+
+	if (property == dev->mode_config.tv_left_margin_property)
+		*val = tv_state->margin[TV_MARGIN_LEFT];
+	else if (property == dev->mode_config.tv_right_margin_property)
+		*val = tv_state->margin[TV_MARGIN_RIGHT];
+	else if (property == dev->mode_config.tv_top_margin_property)
+		*val = tv_state->margin[TV_MARGIN_TOP];
+	else if (property == dev->mode_config.tv_bottom_margin_property)
+		*val = tv_state->margin[TV_MARGIN_BOTTOM];
+	else if (property == dev->mode_config.tv_mode_property) {
+		*val = tv_state->format;
 	} else {
-		ret = -EINVAL;
-		goto out;
+		DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name);
+		return -EINVAL;
 	}
 
-	if (changed && crtc)
-		intel_crtc_restore_mode(crtc);
-out:
-	return ret;
+	return 0;
+}
+
+static struct drm_connector_state *
+intel_tv_duplicate_state(struct drm_connector *connector)
+{
+	struct intel_tv_connector_state *state;
+
+	state = kmemdup(&connector->state, sizeof(*state), GFP_KERNEL);
+	if (state)
+		__drm_atomic_helper_connector_duplicate_state(connector, &state->base);
+
+	return &state->base;
 }
 
 static const struct drm_connector_funcs intel_tv_connector_funcs = {
@@ -1520,11 +1570,12 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = {
 	.late_register = intel_connector_register,
 	.early_unregister = intel_connector_unregister,
 	.destroy = intel_tv_destroy,
-	.set_property = intel_tv_set_property,
-	.atomic_get_property = intel_connector_atomic_get_property,
+	.set_property = drm_atomic_helper_connector_set_property,
+	.atomic_set_property = intel_tv_atomic_set_property,
+	.atomic_get_property = intel_tv_atomic_get_property,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_duplicate_state = intel_tv_duplicate_state,
 };
 
 static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = {
@@ -1547,6 +1598,7 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 	u32 tv_dac_on, tv_dac_off, save_tv_dac;
 	const char *tv_format_names[ARRAY_SIZE(tv_modes)];
 	int i, initial_mode = 0;
+	struct intel_tv_connector_state *state;
 
 	if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED)
 		return;
@@ -1580,16 +1632,17 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 		return;
 
 	intel_tv = kzalloc(sizeof(*intel_tv), GFP_KERNEL);
-	if (!intel_tv) {
-		return;
-	}
-
-	intel_connector = intel_connector_alloc();
-	if (!intel_connector) {
+	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!intel_tv || !intel_connector || !state) {
+		kfree(intel_connector);
+		kfree(state);
 		kfree(intel_tv);
 		return;
 	}
 
+	__drm_atomic_helper_connector_reset(&intel_connector->base, &state->base);
+
 	intel_encoder = &intel_tv->base;
 	connector = &intel_connector->base;
 
@@ -1617,6 +1670,7 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 	intel_encoder->disable = intel_disable_tv;
 	intel_encoder->get_hw_state = intel_tv_get_hw_state;
 	intel_connector->get_hw_state = intel_connector_get_hw_state;
+	intel_connector->swap_state = intel_tv_swap_state;
 
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
 
@@ -1629,12 +1683,13 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 	intel_tv->type = DRM_MODE_CONNECTOR_Unknown;
 
 	/* BIOS margin values */
-	intel_tv->margin[TV_MARGIN_LEFT] = 54;
-	intel_tv->margin[TV_MARGIN_TOP] = 36;
-	intel_tv->margin[TV_MARGIN_RIGHT] = 46;
-	intel_tv->margin[TV_MARGIN_BOTTOM] = 37;
+	state->margin[TV_MARGIN_LEFT] = 54;
+	state->margin[TV_MARGIN_TOP] = 36;
+	state->margin[TV_MARGIN_RIGHT] = 46;
+	state->margin[TV_MARGIN_BOTTOM] = 37;
 
-	intel_tv->tv_format = tv_modes[initial_mode].name;
+	state->format = initial_mode;
+	intel_tv->format = initial_mode;
 
 	drm_connector_helper_add(connector, &intel_tv_connector_helper_funcs);
 	connector->interlace_allowed = false;
@@ -1648,17 +1703,17 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 				      tv_format_names);
 
 	drm_object_attach_property(&connector->base, dev->mode_config.tv_mode_property,
-				   initial_mode);
+				   state->format);
 	drm_object_attach_property(&connector->base,
 				   dev->mode_config.tv_left_margin_property,
-				   intel_tv->margin[TV_MARGIN_LEFT]);
+				   state->margin[TV_MARGIN_LEFT]);
 	drm_object_attach_property(&connector->base,
 				   dev->mode_config.tv_top_margin_property,
-				   intel_tv->margin[TV_MARGIN_TOP]);
+				   state->margin[TV_MARGIN_TOP]);
 	drm_object_attach_property(&connector->base,
 				   dev->mode_config.tv_right_margin_property,
-				   intel_tv->margin[TV_MARGIN_RIGHT]);
+				   state->margin[TV_MARGIN_RIGHT]);
 	drm_object_attach_property(&connector->base,
 				   dev->mode_config.tv_bottom_margin_property,
-				   intel_tv->margin[TV_MARGIN_BOTTOM]);
+				   state->margin[TV_MARGIN_BOTTOM]);
 }
-- 
2.7.4


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

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2.

[Ville Syrjälä]

On Mon, Mar 13, 2017 at 05:10:28PM +0100, Maarten Lankhorst wrote:
> As a proof of concept, first try to convert intel_tv, which is a rarely
> used connector. It has 5 properties, tv format and 4 margins.
> 
> I'm less certain about the state behavior itself, should we pass a size
> parameter to intel_connector_alloc instead, so duplicate_state
> can be done globally if it can be blindly copied?
> 
> Can we also have a atomic_check function for connectors, so the
> crtc_state->connectors_changed can be set there? It would be cleaner
> and more atomic-like.
> 
> To match the legacy behavior, format can be changed by probing just like
> in legacy mode.
> 
> Changes since v1:
> - Add intel_encoder->swap_state to allow updating connector state.
> - Add intel_tv->format for detect_mode and mode_valid, updated on atomic commit.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  15 ++
>  drivers/gpu/drm/i915/intel_drv.h     |   4 +
>  drivers/gpu/drm/i915/intel_tv.c      | 259 +++++++++++++++++++++--------------
>  3 files changed, 176 insertions(+), 102 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ac25c9bc8b81..18b7e7546ee1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12954,6 +12954,20 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
>  				  to_intel_plane(plane)->frontbuffer_bit);
>  }
>  
> +static void swap_connector_state(struct drm_atomic_state *state)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_state *new_conn_state;
> +	int i;
> +
> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> +		struct intel_connector *conn = to_intel_connector(connector);
> +
> +		if (conn->swap_state)
> +			conn->swap_state(conn, new_conn_state);
> +	}
> +}
> +
>  /**
>   * intel_atomic_commit - commit validated state object
>   * @dev: DRM device
> @@ -13012,6 +13026,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		dev_priv->cdclk.logical = intel_state->cdclk.logical;
>  		dev_priv->cdclk.actual = intel_state->cdclk.actual;
>  	}
> +	swap_connector_state(state);
>  
>  	drm_atomic_state_get(state);
>  	INIT_WORK(&state->commit_work,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 937623ff6d7c..b7b93799d288 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -307,6 +307,10 @@ struct intel_connector {
>  	 * and active (i.e. dpms ON state). */
>  	bool (*get_hw_state)(struct intel_connector *);
>  
> +	/* Update device state with the new atomic state. */
> +	void (*swap_state)(struct intel_connector *,
> +			     struct drm_connector_state *);
> +
>  	/* Panel info for eDP and LVDS */
>  	struct intel_panel panel;
>  
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 6ed1a3ce47b7..b87d51e61975 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -48,8 +48,6 @@ struct intel_tv {
>  	struct intel_encoder base;
>  
>  	int type;
> -	const char *tv_format;
> -	int margin[4];
>  	u32 save_TV_H_CTL_1;
>  	u32 save_TV_H_CTL_2;
>  	u32 save_TV_H_CTL_3;
> @@ -83,8 +81,20 @@ struct intel_tv {
>  
>  	u32 save_TV_DAC;
>  	u32 save_TV_CTL;
> +
> +	int format;
> +};
> +
> +struct intel_tv_connector_state {
> +	struct drm_connector_state base;
> +
> +	int format;
> +	int margin[4];
>  };
>  
> +#define to_intel_tv_connector_state(state) \
> +	container_of((state), struct intel_tv_connector_state, base)
> +
>  struct video_levels {
>  	u16 blank, black;
>  	u8 burst;
> @@ -848,6 +858,17 @@ intel_tv_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe)
>  }
>  
>  static void
> +intel_tv_swap_state(struct intel_connector *connector,
> +		    struct drm_connector_state *conn_state)
> +{
> +	struct intel_tv *intel_tv = intel_attached_tv(&connector->base);
> +	struct intel_tv_connector_state *tv_state =
> +		to_intel_tv_connector_state(conn_state);
> +
> +	intel_tv->format = tv_state->format;
> +}
> +
> +static void
>  intel_enable_tv(struct intel_encoder *encoder,
>  		struct intel_crtc_state *pipe_config,
>  		struct drm_connector_state *conn_state)
> @@ -873,32 +894,25 @@ intel_disable_tv(struct intel_encoder *encoder,
>  	I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
>  }
>  
> -static const struct tv_mode *
> -intel_tv_mode_lookup(const char *tv_format)
> +static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
>  {
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
> -		const struct tv_mode *tv_mode = &tv_modes[i];
> +	int format = to_intel_tv_connector_state(conn_state)->format;
>  
> -		if (!strcmp(tv_format, tv_mode->name))
> -			return tv_mode;
> -	}
> -	return NULL;
> +	return &tv_modes[format];
>  }
>  
> -static const struct tv_mode *
> -intel_tv_mode_find(struct intel_tv *intel_tv)
> +static const struct tv_mode *intel_tv_mode_find_committed(struct drm_connector *connector)
>  {
> -	return intel_tv_mode_lookup(intel_tv->tv_format);
> +	int format = intel_attached_tv(connector)->format;
> +
> +	return &tv_modes[format];
>  }
>  
>  static enum drm_mode_status
>  intel_tv_mode_valid(struct drm_connector *connector,
>  		    struct drm_display_mode *mode)
>  {
> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> +	const struct tv_mode *tv_mode = intel_tv_mode_find_committed(connector);
>  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
>  
>  	if (mode->clock > max_dotclk)
> @@ -925,8 +939,7 @@ intel_tv_compute_config(struct intel_encoder *encoder,
>  			struct intel_crtc_state *pipe_config,
>  			struct drm_connector_state *conn_state)
>  {
> -	struct intel_tv *intel_tv = enc_to_tv(encoder);
> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> +	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
>  
>  	if (!tv_mode)
>  		return false;
> @@ -1032,7 +1045,7 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>  	struct intel_tv *intel_tv = enc_to_tv(encoder);
> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> +	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
>  	u32 tv_ctl;
>  	u32 scctl1, scctl2, scctl3;
>  	int i, j;
> @@ -1041,6 +1054,8 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>  	bool burst_ena;
>  	int xpos = 0x0, ypos = 0x0;
>  	unsigned int xsize, ysize;
> +	struct intel_tv_connector_state *tv_state =
> +		to_intel_tv_connector_state(conn_state);
>  
>  	if (!tv_mode)
>  		return;	/* can't happen (mode_prepare prevents this) */
> @@ -1135,12 +1150,12 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>  	else
>  		ysize = 2*tv_mode->nbr_end + 1;
>  
> -	xpos += intel_tv->margin[TV_MARGIN_LEFT];
> -	ypos += intel_tv->margin[TV_MARGIN_TOP];
> -	xsize -= (intel_tv->margin[TV_MARGIN_LEFT] +
> -		  intel_tv->margin[TV_MARGIN_RIGHT]);
> -	ysize -= (intel_tv->margin[TV_MARGIN_TOP] +
> -		  intel_tv->margin[TV_MARGIN_BOTTOM]);
> +	xpos += tv_state->margin[TV_MARGIN_LEFT];
> +	ypos += tv_state->margin[TV_MARGIN_TOP];
> +	xsize -= (tv_state->margin[TV_MARGIN_LEFT] +
> +		  tv_state->margin[TV_MARGIN_RIGHT]);
> +	ysize -= (tv_state->margin[TV_MARGIN_TOP] +
> +		  tv_state->margin[TV_MARGIN_BOTTOM]);
>  	I915_WRITE(TV_WIN_POS, (xpos<<16)|ypos);
>  	I915_WRITE(TV_WIN_SIZE, (xsize<<16)|ysize);
>  
> @@ -1288,7 +1303,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
>  static void intel_tv_find_better_format(struct drm_connector *connector)
>  {
>  	struct intel_tv *intel_tv = intel_attached_tv(connector);
> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> +	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);

intel_tv_mode_find_committed() ?

>  	int i;
>  
>  	if ((intel_tv->type == DRM_MODE_CONNECTOR_Component) ==
> @@ -1304,9 +1319,8 @@ static void intel_tv_find_better_format(struct drm_connector *connector)
>  			break;
>  	}
>  
> -	intel_tv->tv_format = tv_mode->name;
> -	drm_object_property_set_value(&connector->base,
> -		connector->dev->mode_config.tv_mode_property, i);
> +	to_intel_tv_connector_state(connector->state)->format = i;

I was thinking we'd do this in duplicate_state.

With these two changes we shouldn't have to access ->state in detect()
AFAICS.

> +	intel_attached_tv(connector)->format = i;
>  }
>  
>  /**
> @@ -1344,18 +1358,17 @@ intel_tv_detect(struct drm_connector *connector, bool force)
>  		} else
>  			status = connector_status_unknown;
>  
> +		if (status == connector_status_connected) {
> +			intel_tv->type = type;
> +			intel_tv_find_better_format(connector);
> +		}
> +
>  		drm_modeset_drop_locks(&ctx);
>  		drm_modeset_acquire_fini(&ctx);
> -	} else
> -		return connector->status;
>  
> -	if (status != connector_status_connected)
>  		return status;
> -
> -	intel_tv->type = type;
> -	intel_tv_find_better_format(connector);
> -
> -	return connector_status_connected;
> +	} else
> +		return connector->status;
>  }
>  
>  static const struct input_res {
> @@ -1375,12 +1388,9 @@ static const struct input_res {
>   * Chose preferred mode  according to line number of TV format
>   */
>  static void
> -intel_tv_chose_preferred_modes(struct drm_connector *connector,
> +intel_tv_chose_preferred_modes(const struct tv_mode *tv_mode,
>  			       struct drm_display_mode *mode_ptr)
>  {
> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> -
>  	if (tv_mode->nbr_end < 480 && mode_ptr->vdisplay == 480)
>  		mode_ptr->type |= DRM_MODE_TYPE_PREFERRED;
>  	else if (tv_mode->nbr_end > 480) {
> @@ -1403,8 +1413,7 @@ static int
>  intel_tv_get_modes(struct drm_connector *connector)
>  {
>  	struct drm_display_mode *mode_ptr;
> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> +	const struct tv_mode *tv_mode = intel_tv_mode_find_committed(connector);
>  	int j, count = 0;
>  	u64 tmp;
>  
> @@ -1447,7 +1456,7 @@ intel_tv_get_modes(struct drm_connector *connector)
>  		mode_ptr->clock = (int) tmp;
>  
>  		mode_ptr->type = DRM_MODE_TYPE_DRIVER;
> -		intel_tv_chose_preferred_modes(connector, mode_ptr);
> +		intel_tv_chose_preferred_modes(tv_mode, mode_ptr);
                         ^^^^^

Pre-existing typo :) Might want to fix that up too.
                 

>  		drm_mode_probed_add(connector, mode_ptr);
>  		count++;
>  	}
> @@ -1462,56 +1471,97 @@ intel_tv_destroy(struct drm_connector *connector)
>  	kfree(connector);
>  }
>  
> -
>  static int
> -intel_tv_set_property(struct drm_connector *connector, struct drm_property *property,
> -		      uint64_t val)
> +intel_tv_atomic_set_property(struct drm_connector *connector,
> +			     struct drm_connector_state *conn_state,
> +			     struct drm_property *property,
> +			     uint64_t val)
>  {
>  	struct drm_device *dev = connector->dev;
> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
> -	struct drm_crtc *crtc = intel_tv->base.base.crtc;
> -	int ret = 0;
> +
> +	struct intel_tv_connector_state *tv_state =
> +		to_intel_tv_connector_state(conn_state);
>  	bool changed = false;
>  
> -	ret = drm_object_property_set_value(&connector->base, property, val);
> -	if (ret < 0)
> -		goto out;
> -
> -	if (property == dev->mode_config.tv_left_margin_property &&
> -		intel_tv->margin[TV_MARGIN_LEFT] != val) {
> -		intel_tv->margin[TV_MARGIN_LEFT] = val;
> -		changed = true;
> -	} else if (property == dev->mode_config.tv_right_margin_property &&
> -		intel_tv->margin[TV_MARGIN_RIGHT] != val) {
> -		intel_tv->margin[TV_MARGIN_RIGHT] = val;
> -		changed = true;
> -	} else if (property == dev->mode_config.tv_top_margin_property &&
> -		intel_tv->margin[TV_MARGIN_TOP] != val) {
> -		intel_tv->margin[TV_MARGIN_TOP] = val;
> -		changed = true;
> -	} else if (property == dev->mode_config.tv_bottom_margin_property &&
> -		intel_tv->margin[TV_MARGIN_BOTTOM] != val) {
> -		intel_tv->margin[TV_MARGIN_BOTTOM] = val;
> -		changed = true;
> +	if (property == dev->mode_config.tv_left_margin_property) {
> +		changed = tv_state->margin[TV_MARGIN_LEFT] != val;
> +
> +		tv_state->margin[TV_MARGIN_LEFT] = val;
> +	} else if (property == dev->mode_config.tv_right_margin_property) {
> +		changed = tv_state->margin[TV_MARGIN_RIGHT] != val;
> +
> +		tv_state->margin[TV_MARGIN_RIGHT] = val;
> +	} else if (property == dev->mode_config.tv_top_margin_property) {
> +		changed = tv_state->margin[TV_MARGIN_TOP] != val;
> +
> +		tv_state->margin[TV_MARGIN_TOP] = val;
> +	} else if (property == dev->mode_config.tv_bottom_margin_property) {
> +		changed = tv_state->margin[TV_MARGIN_BOTTOM] != val;
> +
> +		tv_state->margin[TV_MARGIN_BOTTOM] = val;
>  	} else if (property == dev->mode_config.tv_mode_property) {
>  		if (val >= ARRAY_SIZE(tv_modes)) {
> -			ret = -EINVAL;
> -			goto out;
> +			DRM_DEBUG_ATOMIC("value %llu out of tv_modes array bounds\n", val);
> +
> +			return -EINVAL;
>  		}
> -		if (!strcmp(intel_tv->tv_format, tv_modes[val].name))
> -			goto out;
>  
> -		intel_tv->tv_format = tv_modes[val].name;
> -		changed = true;
> +		changed = tv_state->format != val;
> +		tv_state->format = val;
> +	} else {
> +		DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name);
> +		return -EINVAL;
> +	}
> +
> +	/* Trigger a modeset when connector properties are changed. */
> +	if (changed && conn_state->crtc) {
> +		struct drm_crtc_state *crtc_state =
> +			drm_atomic_get_existing_crtc_state(conn_state->state,
> +							   conn_state->crtc);
> +
> +		crtc_state->connectors_changed = true;
> +	}
> +	return 0;
> +}
> +
> +static int
> +intel_tv_atomic_get_property(struct drm_connector *connector,
> +				    const struct drm_connector_state *state,
> +				    struct drm_property *property,
> +				    uint64_t *val)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct intel_tv_connector_state *tv_state =
> +		to_intel_tv_connector_state(state);
> +
> +	if (property == dev->mode_config.tv_left_margin_property)
> +		*val = tv_state->margin[TV_MARGIN_LEFT];
> +	else if (property == dev->mode_config.tv_right_margin_property)
> +		*val = tv_state->margin[TV_MARGIN_RIGHT];
> +	else if (property == dev->mode_config.tv_top_margin_property)
> +		*val = tv_state->margin[TV_MARGIN_TOP];
> +	else if (property == dev->mode_config.tv_bottom_margin_property)
> +		*val = tv_state->margin[TV_MARGIN_BOTTOM];
> +	else if (property == dev->mode_config.tv_mode_property) {
> +		*val = tv_state->format;
>  	} else {
> -		ret = -EINVAL;
> -		goto out;
> +		DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name);
> +		return -EINVAL;
>  	}
>  
> -	if (changed && crtc)
> -		intel_crtc_restore_mode(crtc);
> -out:
> -	return ret;
> +	return 0;
> +}
> +
> +static struct drm_connector_state *
> +intel_tv_duplicate_state(struct drm_connector *connector)
> +{
> +	struct intel_tv_connector_state *state;
> +
> +	state = kmemdup(&connector->state, sizeof(*state), GFP_KERNEL);
> +	if (state)
> +		__drm_atomic_helper_connector_duplicate_state(connector, &state->base);
> +
> +	return &state->base;
>  }
>  
>  static const struct drm_connector_funcs intel_tv_connector_funcs = {
> @@ -1520,11 +1570,12 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = {
>  	.late_register = intel_connector_register,
>  	.early_unregister = intel_connector_unregister,
>  	.destroy = intel_tv_destroy,
> -	.set_property = intel_tv_set_property,
> -	.atomic_get_property = intel_connector_atomic_get_property,
> +	.set_property = drm_atomic_helper_connector_set_property,
> +	.atomic_set_property = intel_tv_atomic_set_property,
> +	.atomic_get_property = intel_tv_atomic_get_property,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_duplicate_state = intel_tv_duplicate_state,
>  };
>  
>  static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = {
> @@ -1547,6 +1598,7 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  	u32 tv_dac_on, tv_dac_off, save_tv_dac;
>  	const char *tv_format_names[ARRAY_SIZE(tv_modes)];
>  	int i, initial_mode = 0;
> +	struct intel_tv_connector_state *state;
>  
>  	if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED)
>  		return;
> @@ -1580,16 +1632,17 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  		return;
>  
>  	intel_tv = kzalloc(sizeof(*intel_tv), GFP_KERNEL);
> -	if (!intel_tv) {
> -		return;
> -	}
> -
> -	intel_connector = intel_connector_alloc();
> -	if (!intel_connector) {
> +	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
> +	state = kzalloc(sizeof(*state), GFP_KERNEL);
> +	if (!intel_tv || !intel_connector || !state) {
> +		kfree(intel_connector);
> +		kfree(state);
>  		kfree(intel_tv);
>  		return;
>  	}
>  
> +	__drm_atomic_helper_connector_reset(&intel_connector->base, &state->base);
> +
>  	intel_encoder = &intel_tv->base;
>  	connector = &intel_connector->base;
>  
> @@ -1617,6 +1670,7 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  	intel_encoder->disable = intel_disable_tv;
>  	intel_encoder->get_hw_state = intel_tv_get_hw_state;
>  	intel_connector->get_hw_state = intel_connector_get_hw_state;
> +	intel_connector->swap_state = intel_tv_swap_state;
>  
>  	intel_connector_attach_encoder(intel_connector, intel_encoder);
>  
> @@ -1629,12 +1683,13 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  	intel_tv->type = DRM_MODE_CONNECTOR_Unknown;
>  
>  	/* BIOS margin values */
> -	intel_tv->margin[TV_MARGIN_LEFT] = 54;
> -	intel_tv->margin[TV_MARGIN_TOP] = 36;
> -	intel_tv->margin[TV_MARGIN_RIGHT] = 46;
> -	intel_tv->margin[TV_MARGIN_BOTTOM] = 37;
> +	state->margin[TV_MARGIN_LEFT] = 54;
> +	state->margin[TV_MARGIN_TOP] = 36;
> +	state->margin[TV_MARGIN_RIGHT] = 46;
> +	state->margin[TV_MARGIN_BOTTOM] = 37;
>  
> -	intel_tv->tv_format = tv_modes[initial_mode].name;
> +	state->format = initial_mode;
> +	intel_tv->format = initial_mode;
>  
>  	drm_connector_helper_add(connector, &intel_tv_connector_helper_funcs);
>  	connector->interlace_allowed = false;
> @@ -1648,17 +1703,17 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  				      tv_format_names);
>  
>  	drm_object_attach_property(&connector->base, dev->mode_config.tv_mode_property,
> -				   initial_mode);
> +				   state->format);
>  	drm_object_attach_property(&connector->base,
>  				   dev->mode_config.tv_left_margin_property,
> -				   intel_tv->margin[TV_MARGIN_LEFT]);
> +				   state->margin[TV_MARGIN_LEFT]);
>  	drm_object_attach_property(&connector->base,
>  				   dev->mode_config.tv_top_margin_property,
> -				   intel_tv->margin[TV_MARGIN_TOP]);
> +				   state->margin[TV_MARGIN_TOP]);
>  	drm_object_attach_property(&connector->base,
>  				   dev->mode_config.tv_right_margin_property,
> -				   intel_tv->margin[TV_MARGIN_RIGHT]);
> +				   state->margin[TV_MARGIN_RIGHT]);
>  	drm_object_attach_property(&connector->base,
>  				   dev->mode_config.tv_bottom_margin_property,
> -				   intel_tv->margin[TV_MARGIN_BOTTOM]);
> +				   state->margin[TV_MARGIN_BOTTOM]);
>  }
> -- 
> 2.7.4
> 

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

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2.

[Maarten Lankhorst]

Op 13-03-17 om 17:22 schreef Ville Syrjälä:
> On Mon, Mar 13, 2017 at 05:10:28PM +0100, Maarten Lankhorst wrote:
>> As a proof of concept, first try to convert intel_tv, which is a rarely
>> used connector. It has 5 properties, tv format and 4 margins.
>>
>> I'm less certain about the state behavior itself, should we pass a size
>> parameter to intel_connector_alloc instead, so duplicate_state
>> can be done globally if it can be blindly copied?
>>
>> Can we also have a atomic_check function for connectors, so the
>> crtc_state->connectors_changed can be set there? It would be cleaner
>> and more atomic-like.
>>
>> To match the legacy behavior, format can be changed by probing just like
>> in legacy mode.
>>
>> Changes since v1:
>> - Add intel_encoder->swap_state to allow updating connector state.
>> - Add intel_tv->format for detect_mode and mode_valid, updated on atomic commit.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c |  15 ++
>>  drivers/gpu/drm/i915/intel_drv.h     |   4 +
>>  drivers/gpu/drm/i915/intel_tv.c      | 259 +++++++++++++++++++++--------------
>>  3 files changed, 176 insertions(+), 102 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index ac25c9bc8b81..18b7e7546ee1 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12954,6 +12954,20 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
>>  				  to_intel_plane(plane)->frontbuffer_bit);
>>  }
>>  
>> +static void swap_connector_state(struct drm_atomic_state *state)
>> +{
>> +	struct drm_connector *connector;
>> +	struct drm_connector_state *new_conn_state;
>> +	int i;
>> +
>> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
>> +		struct intel_connector *conn = to_intel_connector(connector);
>> +
>> +		if (conn->swap_state)
>> +			conn->swap_state(conn, new_conn_state);
>> +	}
>> +}
>> +
>>  /**
>>   * intel_atomic_commit - commit validated state object
>>   * @dev: DRM device
>> @@ -13012,6 +13026,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  		dev_priv->cdclk.logical = intel_state->cdclk.logical;
>>  		dev_priv->cdclk.actual = intel_state->cdclk.actual;
>>  	}
>> +	swap_connector_state(state);
>>  
>>  	drm_atomic_state_get(state);
>>  	INIT_WORK(&state->commit_work,
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 937623ff6d7c..b7b93799d288 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -307,6 +307,10 @@ struct intel_connector {
>>  	 * and active (i.e. dpms ON state). */
>>  	bool (*get_hw_state)(struct intel_connector *);
>>  
>> +	/* Update device state with the new atomic state. */
>> +	void (*swap_state)(struct intel_connector *,
>> +			     struct drm_connector_state *);
>> +
>>  	/* Panel info for eDP and LVDS */
>>  	struct intel_panel panel;
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
>> index 6ed1a3ce47b7..b87d51e61975 100644
>> --- a/drivers/gpu/drm/i915/intel_tv.c
>> +++ b/drivers/gpu/drm/i915/intel_tv.c
>> @@ -48,8 +48,6 @@ struct intel_tv {
>>  	struct intel_encoder base;
>>  
>>  	int type;
>> -	const char *tv_format;
>> -	int margin[4];
>>  	u32 save_TV_H_CTL_1;
>>  	u32 save_TV_H_CTL_2;
>>  	u32 save_TV_H_CTL_3;
>> @@ -83,8 +81,20 @@ struct intel_tv {
>>  
>>  	u32 save_TV_DAC;
>>  	u32 save_TV_CTL;
>> +
>> +	int format;
>> +};
>> +
>> +struct intel_tv_connector_state {
>> +	struct drm_connector_state base;
>> +
>> +	int format;
>> +	int margin[4];
>>  };
>>  
>> +#define to_intel_tv_connector_state(state) \
>> +	container_of((state), struct intel_tv_connector_state, base)
>> +
>>  struct video_levels {
>>  	u16 blank, black;
>>  	u8 burst;
>> @@ -848,6 +858,17 @@ intel_tv_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe)
>>  }
>>  
>>  static void
>> +intel_tv_swap_state(struct intel_connector *connector,
>> +		    struct drm_connector_state *conn_state)
>> +{
>> +	struct intel_tv *intel_tv = intel_attached_tv(&connector->base);
>> +	struct intel_tv_connector_state *tv_state =
>> +		to_intel_tv_connector_state(conn_state);
>> +
>> +	intel_tv->format = tv_state->format;
>> +}
>> +
>> +static void
>>  intel_enable_tv(struct intel_encoder *encoder,
>>  		struct intel_crtc_state *pipe_config,
>>  		struct drm_connector_state *conn_state)
>> @@ -873,32 +894,25 @@ intel_disable_tv(struct intel_encoder *encoder,
>>  	I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
>>  }
>>  
>> -static const struct tv_mode *
>> -intel_tv_mode_lookup(const char *tv_format)
>> +static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
>>  {
>> -	int i;
>> -
>> -	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
>> -		const struct tv_mode *tv_mode = &tv_modes[i];
>> +	int format = to_intel_tv_connector_state(conn_state)->format;
>>  
>> -		if (!strcmp(tv_format, tv_mode->name))
>> -			return tv_mode;
>> -	}
>> -	return NULL;
>> +	return &tv_modes[format];
>>  }
>>  
>> -static const struct tv_mode *
>> -intel_tv_mode_find(struct intel_tv *intel_tv)
>> +static const struct tv_mode *intel_tv_mode_find_committed(struct drm_connector *connector)
>>  {
>> -	return intel_tv_mode_lookup(intel_tv->tv_format);
>> +	int format = intel_attached_tv(connector)->format;
>> +
>> +	return &tv_modes[format];
>>  }
>>  
>>  static enum drm_mode_status
>>  intel_tv_mode_valid(struct drm_connector *connector,
>>  		    struct drm_display_mode *mode)
>>  {
>> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>> +	const struct tv_mode *tv_mode = intel_tv_mode_find_committed(connector);
>>  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
>>  
>>  	if (mode->clock > max_dotclk)
>> @@ -925,8 +939,7 @@ intel_tv_compute_config(struct intel_encoder *encoder,
>>  			struct intel_crtc_state *pipe_config,
>>  			struct drm_connector_state *conn_state)
>>  {
>> -	struct intel_tv *intel_tv = enc_to_tv(encoder);
>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>> +	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
>>  
>>  	if (!tv_mode)
>>  		return false;
>> @@ -1032,7 +1045,7 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>>  	struct intel_tv *intel_tv = enc_to_tv(encoder);
>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>> +	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
>>  	u32 tv_ctl;
>>  	u32 scctl1, scctl2, scctl3;
>>  	int i, j;
>> @@ -1041,6 +1054,8 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>>  	bool burst_ena;
>>  	int xpos = 0x0, ypos = 0x0;
>>  	unsigned int xsize, ysize;
>> +	struct intel_tv_connector_state *tv_state =
>> +		to_intel_tv_connector_state(conn_state);
>>  
>>  	if (!tv_mode)
>>  		return;	/* can't happen (mode_prepare prevents this) */
>> @@ -1135,12 +1150,12 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>>  	else
>>  		ysize = 2*tv_mode->nbr_end + 1;
>>  
>> -	xpos += intel_tv->margin[TV_MARGIN_LEFT];
>> -	ypos += intel_tv->margin[TV_MARGIN_TOP];
>> -	xsize -= (intel_tv->margin[TV_MARGIN_LEFT] +
>> -		  intel_tv->margin[TV_MARGIN_RIGHT]);
>> -	ysize -= (intel_tv->margin[TV_MARGIN_TOP] +
>> -		  intel_tv->margin[TV_MARGIN_BOTTOM]);
>> +	xpos += tv_state->margin[TV_MARGIN_LEFT];
>> +	ypos += tv_state->margin[TV_MARGIN_TOP];
>> +	xsize -= (tv_state->margin[TV_MARGIN_LEFT] +
>> +		  tv_state->margin[TV_MARGIN_RIGHT]);
>> +	ysize -= (tv_state->margin[TV_MARGIN_TOP] +
>> +		  tv_state->margin[TV_MARGIN_BOTTOM]);
>>  	I915_WRITE(TV_WIN_POS, (xpos<<16)|ypos);
>>  	I915_WRITE(TV_WIN_SIZE, (xsize<<16)|ysize);
>>  
>> @@ -1288,7 +1303,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
>>  static void intel_tv_find_better_format(struct drm_connector *connector)
>>  {
>>  	struct intel_tv *intel_tv = intel_attached_tv(connector);
>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>> +	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
> intel_tv_mode_find_committed() ?
Either works here. We're holding the relevant lock.
>
>>  	int i;
>>  
>>  	if ((intel_tv->type == DRM_MODE_CONNECTOR_Component) ==
>> @@ -1304,9 +1319,8 @@ static void intel_tv_find_better_format(struct drm_connector *connector)
>>  			break;
>>  	}
>>  
>> -	intel_tv->tv_format = tv_mode->name;
>> -	drm_object_property_set_value(&connector->base,
>> -		connector->dev->mode_config.tv_mode_property, i);
>> +	to_intel_tv_connector_state(connector->state)->format = i;
> I was thinking we'd do this in duplicate_state.
This would cause get_property to return stale values since it would not duplicate the state.
> With these two changes we shouldn't have to access ->state in detect()
> AFAICS.
I think it's the only thing that makes sense.

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

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2.

[Maarten Lankhorst]

Op 13-03-17 om 17:22 schreef Ville Syrjälä:
> On Mon, Mar 13, 2017 at 05:10:28PM +0100, Maarten Lankhorst wrote:
>> As a proof of concept, first try to convert intel_tv, which is a rarely
>> used connector. It has 5 properties, tv format and 4 margins.
>>
>> I'm less certain about the state behavior itself, should we pass a size
>> parameter to intel_connector_alloc instead, so duplicate_state
>> can be done globally if it can be blindly copied?
>>
>> Can we also have a atomic_check function for connectors, so the
>> crtc_state->connectors_changed can be set there? It would be cleaner
>> and more atomic-like.
>>
>> To match the legacy behavior, format can be changed by probing just like
>> in legacy mode.
>>
>> Changes since v1:
>> - Add intel_encoder->swap_state to allow updating connector state.
>> - Add intel_tv->format for detect_mode and mode_valid, updated on atomic commit.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c |  15 ++
>>  drivers/gpu/drm/i915/intel_drv.h     |   4 +
>>  drivers/gpu/drm/i915/intel_tv.c      | 259 +++++++++++++++++++++--------------
>>  3 files changed, 176 insertions(+), 102 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index ac25c9bc8b81..18b7e7546ee1 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12954,6 +12954,20 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
>>  				  to_intel_plane(plane)->frontbuffer_bit);
>>  }
>>  
>> +static void swap_connector_state(struct drm_atomic_state *state)
>> +{
>> +	struct drm_connector *connector;
>> +	struct drm_connector_state *new_conn_state;
>> +	int i;
>> +
>> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
>> +		struct intel_connector *conn = to_intel_connector(connector);
>> +
>> +		if (conn->swap_state)
>> +			conn->swap_state(conn, new_conn_state);
>> +	}
>> +}
>> +
>>  /**
>>   * intel_atomic_commit - commit validated state object
>>   * @dev: DRM device
>> @@ -13012,6 +13026,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  		dev_priv->cdclk.logical = intel_state->cdclk.logical;
>>  		dev_priv->cdclk.actual = intel_state->cdclk.actual;
>>  	}
>> +	swap_connector_state(state);
>>  
>>  	drm_atomic_state_get(state);
>>  	INIT_WORK(&state->commit_work,
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 937623ff6d7c..b7b93799d288 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -307,6 +307,10 @@ struct intel_connector {
>>  	 * and active (i.e. dpms ON state). */
>>  	bool (*get_hw_state)(struct intel_connector *);
>>  
>> +	/* Update device state with the new atomic state. */
>> +	void (*swap_state)(struct intel_connector *,
>> +			     struct drm_connector_state *);
>> +
>>  	/* Panel info for eDP and LVDS */
>>  	struct intel_panel panel;
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
>> index 6ed1a3ce47b7..b87d51e61975 100644
>> --- a/drivers/gpu/drm/i915/intel_tv.c
>> +++ b/drivers/gpu/drm/i915/intel_tv.c
>> @@ -48,8 +48,6 @@ struct intel_tv {
>>  	struct intel_encoder base;
>>  
>>  	int type;
>> -	const char *tv_format;
>> -	int margin[4];
>>  	u32 save_TV_H_CTL_1;
>>  	u32 save_TV_H_CTL_2;
>>  	u32 save_TV_H_CTL_3;
>> @@ -83,8 +81,20 @@ struct intel_tv {
>>  
>>  	u32 save_TV_DAC;
>>  	u32 save_TV_CTL;
>> +
>> +	int format;
>> +};
>> +
>> +struct intel_tv_connector_state {
>> +	struct drm_connector_state base;
>> +
>> +	int format;
>> +	int margin[4];
>>  };
>>  
>> +#define to_intel_tv_connector_state(state) \
>> +	container_of((state), struct intel_tv_connector_state, base)
>> +
>>  struct video_levels {
>>  	u16 blank, black;
>>  	u8 burst;
>> @@ -848,6 +858,17 @@ intel_tv_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe)
>>  }
>>  
>>  static void
>> +intel_tv_swap_state(struct intel_connector *connector,
>> +		    struct drm_connector_state *conn_state)
>> +{
>> +	struct intel_tv *intel_tv = intel_attached_tv(&connector->base);
>> +	struct intel_tv_connector_state *tv_state =
>> +		to_intel_tv_connector_state(conn_state);
>> +
>> +	intel_tv->format = tv_state->format;
>> +}
>> +
>> +static void
>>  intel_enable_tv(struct intel_encoder *encoder,
>>  		struct intel_crtc_state *pipe_config,
>>  		struct drm_connector_state *conn_state)
>> @@ -873,32 +894,25 @@ intel_disable_tv(struct intel_encoder *encoder,
>>  	I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
>>  }
>>  
>> -static const struct tv_mode *
>> -intel_tv_mode_lookup(const char *tv_format)
>> +static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
>>  {
>> -	int i;
>> -
>> -	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
>> -		const struct tv_mode *tv_mode = &tv_modes[i];
>> +	int format = to_intel_tv_connector_state(conn_state)->format;
>>  
>> -		if (!strcmp(tv_format, tv_mode->name))
>> -			return tv_mode;
>> -	}
>> -	return NULL;
>> +	return &tv_modes[format];
>>  }
>>  
>> -static const struct tv_mode *
>> -intel_tv_mode_find(struct intel_tv *intel_tv)
>> +static const struct tv_mode *intel_tv_mode_find_committed(struct drm_connector *connector)
>>  {
>> -	return intel_tv_mode_lookup(intel_tv->tv_format);
>> +	int format = intel_attached_tv(connector)->format;
>> +
>> +	return &tv_modes[format];
>>  }
>>  
>>  static enum drm_mode_status
>>  intel_tv_mode_valid(struct drm_connector *connector,
>>  		    struct drm_display_mode *mode)
>>  {
>> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>> +	const struct tv_mode *tv_mode = intel_tv_mode_find_committed(connector);
>>  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
>>  
>>  	if (mode->clock > max_dotclk)
>> @@ -925,8 +939,7 @@ intel_tv_compute_config(struct intel_encoder *encoder,
>>  			struct intel_crtc_state *pipe_config,
>>  			struct drm_connector_state *conn_state)
>>  {
>> -	struct intel_tv *intel_tv = enc_to_tv(encoder);
>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>> +	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
>>  
>>  	if (!tv_mode)
>>  		return false;
>> @@ -1032,7 +1045,7 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>>  	struct intel_tv *intel_tv = enc_to_tv(encoder);
>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>> +	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
>>  	u32 tv_ctl;
>>  	u32 scctl1, scctl2, scctl3;
>>  	int i, j;
>> @@ -1041,6 +1054,8 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>>  	bool burst_ena;
>>  	int xpos = 0x0, ypos = 0x0;
>>  	unsigned int xsize, ysize;
>> +	struct intel_tv_connector_state *tv_state =
>> +		to_intel_tv_connector_state(conn_state);
>>  
>>  	if (!tv_mode)
>>  		return;	/* can't happen (mode_prepare prevents this) */
>> @@ -1135,12 +1150,12 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>>  	else
>>  		ysize = 2*tv_mode->nbr_end + 1;
>>  
>> -	xpos += intel_tv->margin[TV_MARGIN_LEFT];
>> -	ypos += intel_tv->margin[TV_MARGIN_TOP];
>> -	xsize -= (intel_tv->margin[TV_MARGIN_LEFT] +
>> -		  intel_tv->margin[TV_MARGIN_RIGHT]);
>> -	ysize -= (intel_tv->margin[TV_MARGIN_TOP] +
>> -		  intel_tv->margin[TV_MARGIN_BOTTOM]);
>> +	xpos += tv_state->margin[TV_MARGIN_LEFT];
>> +	ypos += tv_state->margin[TV_MARGIN_TOP];
>> +	xsize -= (tv_state->margin[TV_MARGIN_LEFT] +
>> +		  tv_state->margin[TV_MARGIN_RIGHT]);
>> +	ysize -= (tv_state->margin[TV_MARGIN_TOP] +
>> +		  tv_state->margin[TV_MARGIN_BOTTOM]);
>>  	I915_WRITE(TV_WIN_POS, (xpos<<16)|ypos);
>>  	I915_WRITE(TV_WIN_SIZE, (xsize<<16)|ysize);
>>  
>> @@ -1288,7 +1303,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
>>  static void intel_tv_find_better_format(struct drm_connector *connector)
>>  {
>>  	struct intel_tv *intel_tv = intel_attached_tv(connector);
>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>> +	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
> intel_tv_mode_find_committed() ?
Either is fine, but yeah latter is probably more correct.
>
>>  	int i;
>>  
>>  	if ((intel_tv->type == DRM_MODE_CONNECTOR_Component) ==
>> @@ -1304,9 +1319,8 @@ static void intel_tv_find_better_format(struct drm_connector *connector)
>>  			break;
>>  	}
>>  
>> -	intel_tv->tv_format = tv_mode->name;
>> -	drm_object_property_set_value(&connector->base,
>> -		connector->dev->mode_config.tv_mode_property, i);
>> +	to_intel_tv_connector_state(connector->state)->format = i;
> I was thinking we'd do this in duplicate_state.
>
> With these two changes we shouldn't have to access ->state in detect()
> AFAICS.
Hm I think I might need to keep it here. That would return old values when userspace calls get_property, we don't want that. :)
>
>> +	intel_attached_tv(connector)->format = i;
>>  }
>>  
>>  /**
>> @@ -1344,18 +1358,17 @@ intel_tv_detect(struct drm_connector *connector, bool force)
>>  		} else
>>  			status = connector_status_unknown;
>>  
>> +		if (status == connector_status_connected) {
>> +			intel_tv->type = type;
>> +			intel_tv_find_better_format(connector);
>> +		}
>> +
>>  		drm_modeset_drop_locks(&ctx);
>>  		drm_modeset_acquire_fini(&ctx);
>> -	} else
>> -		return connector->status;
>>  
>> -	if (status != connector_status_connected)
>>  		return status;
>> -
>> -	intel_tv->type = type;
>> -	intel_tv_find_better_format(connector);
>> -
>> -	return connector_status_connected;
>> +	} else
>> +		return connector->status;
>>  }
>>  
>>  static const struct input_res {
>> @@ -1375,12 +1388,9 @@ static const struct input_res {
>>   * Chose preferred mode  according to line number of TV format
>>   */
>>  static void
>> -intel_tv_chose_preferred_modes(struct drm_connector *connector,
>> +intel_tv_chose_preferred_modes(const struct tv_mode *tv_mode,
>>  			       struct drm_display_mode *mode_ptr)
>>  {
>> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>> -
>>  	if (tv_mode->nbr_end < 480 && mode_ptr->vdisplay == 480)
>>  		mode_ptr->type |= DRM_MODE_TYPE_PREFERRED;
>>  	else if (tv_mode->nbr_end > 480) {
>> @@ -1403,8 +1413,7 @@ static int
>>  intel_tv_get_modes(struct drm_connector *connector)
>>  {
>>  	struct drm_display_mode *mode_ptr;
>> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>> +	const struct tv_mode *tv_mode = intel_tv_mode_find_committed(connector);
>>  	int j, count = 0;
>>  	u64 tmp;
>>  
>> @@ -1447,7 +1456,7 @@ intel_tv_get_modes(struct drm_connector *connector)
>>  		mode_ptr->clock = (int) tmp;
>>  
>>  		mode_ptr->type = DRM_MODE_TYPE_DRIVER;
>> -		intel_tv_chose_preferred_modes(connector, mode_ptr);
>> +		intel_tv_chose_preferred_modes(tv_mode, mode_ptr);
>                          ^^^^^
>
> Pre-existing typo :) Might want to fix that up too.
Indeed.
>                  
>
>>  		drm_mode_probed_add(connector, mode_ptr);
>>  		count++;
>>  	}
>> @@ -1462,56 +1471,97 @@ intel_tv_destroy(struct drm_connector *connector)
>>  	kfree(connector);
>>  }
>>  
>> -
>>  static int
>> -intel_tv_set_property(struct drm_connector *connector, struct drm_property *property,
>> -		      uint64_t val)
>> +intel_tv_atomic_set_property(struct drm_connector *connector,
>> +			     struct drm_connector_state *conn_state,
>> +			     struct drm_property *property,
>> +			     uint64_t val)
>>  {
>>  	struct drm_device *dev = connector->dev;
>> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
>> -	struct drm_crtc *crtc = intel_tv->base.base.crtc;
>> -	int ret = 0;
>> +
>> +	struct intel_tv_connector_state *tv_state =
>> +		to_intel_tv_connector_state(conn_state);
>>  	bool changed = false;
>>  
>> -	ret = drm_object_property_set_value(&connector->base, property, val);
>> -	if (ret < 0)
>> -		goto out;
>> -
>> -	if (property == dev->mode_config.tv_left_margin_property &&
>> -		intel_tv->margin[TV_MARGIN_LEFT] != val) {
>> -		intel_tv->margin[TV_MARGIN_LEFT] = val;
>> -		changed = true;
>> -	} else if (property == dev->mode_config.tv_right_margin_property &&
>> -		intel_tv->margin[TV_MARGIN_RIGHT] != val) {
>> -		intel_tv->margin[TV_MARGIN_RIGHT] = val;
>> -		changed = true;
>> -	} else if (property == dev->mode_config.tv_top_margin_property &&
>> -		intel_tv->margin[TV_MARGIN_TOP] != val) {
>> -		intel_tv->margin[TV_MARGIN_TOP] = val;
>> -		changed = true;
>> -	} else if (property == dev->mode_config.tv_bottom_margin_property &&
>> -		intel_tv->margin[TV_MARGIN_BOTTOM] != val) {
>> -		intel_tv->margin[TV_MARGIN_BOTTOM] = val;
>> -		changed = true;
>> +	if (property == dev->mode_config.tv_left_margin_property) {
>> +		changed = tv_state->margin[TV_MARGIN_LEFT] != val;
>> +
>> +		tv_state->margin[TV_MARGIN_LEFT] = val;
>> +	} else if (property == dev->mode_config.tv_right_margin_property) {
>> +		changed = tv_state->margin[TV_MARGIN_RIGHT] != val;
>> +
>> +		tv_state->margin[TV_MARGIN_RIGHT] = val;
>> +	} else if (property == dev->mode_config.tv_top_margin_property) {
>> +		changed = tv_state->margin[TV_MARGIN_TOP] != val;
>> +
>> +		tv_state->margin[TV_MARGIN_TOP] = val;
>> +	} else if (property == dev->mode_config.tv_bottom_margin_property) {
>> +		changed = tv_state->margin[TV_MARGIN_BOTTOM] != val;
>> +
>> +		tv_state->margin[TV_MARGIN_BOTTOM] = val;
>>  	} else if (property == dev->mode_config.tv_mode_property) {
>>  		if (val >= ARRAY_SIZE(tv_modes)) {
>> -			ret = -EINVAL;
>> -			goto out;
>> +			DRM_DEBUG_ATOMIC("value %llu out of tv_modes array bounds\n", val);
>> +
>> +			return -EINVAL;
>>  		}
>> -		if (!strcmp(intel_tv->tv_format, tv_modes[val].name))
>> -			goto out;
>>  
>> -		intel_tv->tv_format = tv_modes[val].name;
>> -		changed = true;
>> +		changed = tv_state->format != val;
>> +		tv_state->format = val;
>> +	} else {
>> +		DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Trigger a modeset when connector properties are changed. */
>> +	if (changed && conn_state->crtc) {
>> +		struct drm_crtc_state *crtc_state =
>> +			drm_atomic_get_existing_crtc_state(conn_state->state,
>> +							   conn_state->crtc);
>> +
>> +		crtc_state->connectors_changed = true;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int
>> +intel_tv_atomic_get_property(struct drm_connector *connector,
>> +				    const struct drm_connector_state *state,
>> +				    struct drm_property *property,
>> +				    uint64_t *val)
>> +{
>> +	struct drm_device *dev = connector->dev;
>> +	struct intel_tv_connector_state *tv_state =
>> +		to_intel_tv_connector_state(state);
>> +
>> +	if (property == dev->mode_config.tv_left_margin_property)
>> +		*val = tv_state->margin[TV_MARGIN_LEFT];
>> +	else if (property == dev->mode_config.tv_right_margin_property)
>> +		*val = tv_state->margin[TV_MARGIN_RIGHT];
>> +	else if (property == dev->mode_config.tv_top_margin_property)
>> +		*val = tv_state->margin[TV_MARGIN_TOP];
>> +	else if (property == dev->mode_config.tv_bottom_margin_property)
>> +		*val = tv_state->margin[TV_MARGIN_BOTTOM];
>> +	else if (property == dev->mode_config.tv_mode_property) {
>> +		*val = tv_state->format;
>>  	} else {
>> -		ret = -EINVAL;
>> -		goto out;
>> +		DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name);
>> +		return -EINVAL;
>>  	}
>>  
>> -	if (changed && crtc)
>> -		intel_crtc_restore_mode(crtc);
>> -out:
>> -	return ret;
>> +	return 0;
>> +}
>> +
>> +static struct drm_connector_state *
>> +intel_tv_duplicate_state(struct drm_connector *connector)
>> +{
>> +	struct intel_tv_connector_state *state;
>> +
>> +	state = kmemdup(&connector->state, sizeof(*state), GFP_KERNEL);
>> +	if (state)
>> +		__drm_atomic_helper_connector_duplicate_state(connector, &state->base);
>> +
>> +	return &state->base;
>>  }
>>  
>>  static const struct drm_connector_funcs intel_tv_connector_funcs = {
>> @@ -1520,11 +1570,12 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = {
>>  	.late_register = intel_connector_register,
>>  	.early_unregister = intel_connector_unregister,
>>  	.destroy = intel_tv_destroy,
>> -	.set_property = intel_tv_set_property,
>> -	.atomic_get_property = intel_connector_atomic_get_property,
>> +	.set_property = drm_atomic_helper_connector_set_property,
>> +	.atomic_set_property = intel_tv_atomic_set_property,
>> +	.atomic_get_property = intel_tv_atomic_get_property,
>>  	.fill_modes = drm_helper_probe_single_connector_modes,
>>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> +	.atomic_duplicate_state = intel_tv_duplicate_state,
>>  };
>>  
>>  static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = {
>> @@ -1547,6 +1598,7 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>>  	u32 tv_dac_on, tv_dac_off, save_tv_dac;
>>  	const char *tv_format_names[ARRAY_SIZE(tv_modes)];
>>  	int i, initial_mode = 0;
>> +	struct intel_tv_connector_state *state;
>>  
>>  	if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED)
>>  		return;
>> @@ -1580,16 +1632,17 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>>  		return;
>>  
>>  	intel_tv = kzalloc(sizeof(*intel_tv), GFP_KERNEL);
>> -	if (!intel_tv) {
>> -		return;
>> -	}
>> -
>> -	intel_connector = intel_connector_alloc();
>> -	if (!intel_connector) {
>> +	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
>> +	state = kzalloc(sizeof(*state), GFP_KERNEL);
>> +	if (!intel_tv || !intel_connector || !state) {
>> +		kfree(intel_connector);
>> +		kfree(state);
>>  		kfree(intel_tv);
>>  		return;
>>  	}
>>  
>> +	__drm_atomic_helper_connector_reset(&intel_connector->base, &state->base);
>> +
>>  	intel_encoder = &intel_tv->base;
>>  	connector = &intel_connector->base;
>>  
>> @@ -1617,6 +1670,7 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>>  	intel_encoder->disable = intel_disable_tv;
>>  	intel_encoder->get_hw_state = intel_tv_get_hw_state;
>>  	intel_connector->get_hw_state = intel_connector_get_hw_state;
>> +	intel_connector->swap_state = intel_tv_swap_state;
>>  
>>  	intel_connector_attach_encoder(intel_connector, intel_encoder);
>>  
>> @@ -1629,12 +1683,13 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>>  	intel_tv->type = DRM_MODE_CONNECTOR_Unknown;
>>  
>>  	/* BIOS margin values */
>> -	intel_tv->margin[TV_MARGIN_LEFT] = 54;
>> -	intel_tv->margin[TV_MARGIN_TOP] = 36;
>> -	intel_tv->margin[TV_MARGIN_RIGHT] = 46;
>> -	intel_tv->margin[TV_MARGIN_BOTTOM] = 37;
>> +	state->margin[TV_MARGIN_LEFT] = 54;
>> +	state->margin[TV_MARGIN_TOP] = 36;
>> +	state->margin[TV_MARGIN_RIGHT] = 46;
>> +	state->margin[TV_MARGIN_BOTTOM] = 37;
>>  
>> -	intel_tv->tv_format = tv_modes[initial_mode].name;
>> +	state->format = initial_mode;
>> +	intel_tv->format = initial_mode;
>>  
>>  	drm_connector_helper_add(connector, &intel_tv_connector_helper_funcs);
>>  	connector->interlace_allowed = false;
>> @@ -1648,17 +1703,17 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>>  				      tv_format_names);
>>  
>>  	drm_object_attach_property(&connector->base, dev->mode_config.tv_mode_property,
>> -				   initial_mode);
>> +				   state->format);
>>  	drm_object_attach_property(&connector->base,
>>  				   dev->mode_config.tv_left_margin_property,
>> -				   intel_tv->margin[TV_MARGIN_LEFT]);
>> +				   state->margin[TV_MARGIN_LEFT]);
>>  	drm_object_attach_property(&connector->base,
>>  				   dev->mode_config.tv_top_margin_property,
>> -				   intel_tv->margin[TV_MARGIN_TOP]);
>> +				   state->margin[TV_MARGIN_TOP]);
>>  	drm_object_attach_property(&connector->base,
>>  				   dev->mode_config.tv_right_margin_property,
>> -				   intel_tv->margin[TV_MARGIN_RIGHT]);
>> +				   state->margin[TV_MARGIN_RIGHT]);
>>  	drm_object_attach_property(&connector->base,
>>  				   dev->mode_config.tv_bottom_margin_property,
>> -				   intel_tv->margin[TV_MARGIN_BOTTOM]);
>> +				   state->margin[TV_MARGIN_BOTTOM]);
>>  }
>> -- 
>> 2.7.4
>>

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

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2.

[Ville Syrjälä]

On Mon, Mar 13, 2017 at 05:35:42PM +0100, Maarten Lankhorst wrote:
> Op 13-03-17 om 17:22 schreef Ville Syrjälä:
> > On Mon, Mar 13, 2017 at 05:10:28PM +0100, Maarten Lankhorst wrote:
> >> As a proof of concept, first try to convert intel_tv, which is a rarely
> >> used connector. It has 5 properties, tv format and 4 margins.
> >>
> >> I'm less certain about the state behavior itself, should we pass a size
> >> parameter to intel_connector_alloc instead, so duplicate_state
> >> can be done globally if it can be blindly copied?
> >>
> >> Can we also have a atomic_check function for connectors, so the
> >> crtc_state->connectors_changed can be set there? It would be cleaner
> >> and more atomic-like.
> >>
> >> To match the legacy behavior, format can be changed by probing just like
> >> in legacy mode.
> >>
> >> Changes since v1:
> >> - Add intel_encoder->swap_state to allow updating connector state.
> >> - Add intel_tv->format for detect_mode and mode_valid, updated on atomic commit.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c |  15 ++
> >>  drivers/gpu/drm/i915/intel_drv.h     |   4 +
> >>  drivers/gpu/drm/i915/intel_tv.c      | 259 +++++++++++++++++++++--------------
> >>  3 files changed, 176 insertions(+), 102 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index ac25c9bc8b81..18b7e7546ee1 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -12954,6 +12954,20 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
> >>  				  to_intel_plane(plane)->frontbuffer_bit);
> >>  }
> >>  
> >> +static void swap_connector_state(struct drm_atomic_state *state)
> >> +{
> >> +	struct drm_connector *connector;
> >> +	struct drm_connector_state *new_conn_state;
> >> +	int i;
> >> +
> >> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> >> +		struct intel_connector *conn = to_intel_connector(connector);
> >> +
> >> +		if (conn->swap_state)
> >> +			conn->swap_state(conn, new_conn_state);
> >> +	}
> >> +}
> >> +
> >>  /**
> >>   * intel_atomic_commit - commit validated state object
> >>   * @dev: DRM device
> >> @@ -13012,6 +13026,7 @@ static int intel_atomic_commit(struct drm_device *dev,
> >>  		dev_priv->cdclk.logical = intel_state->cdclk.logical;
> >>  		dev_priv->cdclk.actual = intel_state->cdclk.actual;
> >>  	}
> >> +	swap_connector_state(state);
> >>  
> >>  	drm_atomic_state_get(state);
> >>  	INIT_WORK(&state->commit_work,
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 937623ff6d7c..b7b93799d288 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -307,6 +307,10 @@ struct intel_connector {
> >>  	 * and active (i.e. dpms ON state). */
> >>  	bool (*get_hw_state)(struct intel_connector *);
> >>  
> >> +	/* Update device state with the new atomic state. */
> >> +	void (*swap_state)(struct intel_connector *,
> >> +			     struct drm_connector_state *);
> >> +
> >>  	/* Panel info for eDP and LVDS */
> >>  	struct intel_panel panel;
> >>  
> >> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> >> index 6ed1a3ce47b7..b87d51e61975 100644
> >> --- a/drivers/gpu/drm/i915/intel_tv.c
> >> +++ b/drivers/gpu/drm/i915/intel_tv.c
> >> @@ -48,8 +48,6 @@ struct intel_tv {
> >>  	struct intel_encoder base;
> >>  
> >>  	int type;
> >> -	const char *tv_format;
> >> -	int margin[4];
> >>  	u32 save_TV_H_CTL_1;
> >>  	u32 save_TV_H_CTL_2;
> >>  	u32 save_TV_H_CTL_3;
> >> @@ -83,8 +81,20 @@ struct intel_tv {
> >>  
> >>  	u32 save_TV_DAC;
> >>  	u32 save_TV_CTL;
> >> +
> >> +	int format;
> >> +};
> >> +
> >> +struct intel_tv_connector_state {
> >> +	struct drm_connector_state base;
> >> +
> >> +	int format;
> >> +	int margin[4];
> >>  };
> >>  
> >> +#define to_intel_tv_connector_state(state) \
> >> +	container_of((state), struct intel_tv_connector_state, base)
> >> +
> >>  struct video_levels {
> >>  	u16 blank, black;
> >>  	u8 burst;
> >> @@ -848,6 +858,17 @@ intel_tv_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe)
> >>  }
> >>  
> >>  static void
> >> +intel_tv_swap_state(struct intel_connector *connector,
> >> +		    struct drm_connector_state *conn_state)
> >> +{
> >> +	struct intel_tv *intel_tv = intel_attached_tv(&connector->base);
> >> +	struct intel_tv_connector_state *tv_state =
> >> +		to_intel_tv_connector_state(conn_state);
> >> +
> >> +	intel_tv->format = tv_state->format;
> >> +}
> >> +
> >> +static void
> >>  intel_enable_tv(struct intel_encoder *encoder,
> >>  		struct intel_crtc_state *pipe_config,
> >>  		struct drm_connector_state *conn_state)
> >> @@ -873,32 +894,25 @@ intel_disable_tv(struct intel_encoder *encoder,
> >>  	I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
> >>  }
> >>  
> >> -static const struct tv_mode *
> >> -intel_tv_mode_lookup(const char *tv_format)
> >> +static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
> >>  {
> >> -	int i;
> >> -
> >> -	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
> >> -		const struct tv_mode *tv_mode = &tv_modes[i];
> >> +	int format = to_intel_tv_connector_state(conn_state)->format;
> >>  
> >> -		if (!strcmp(tv_format, tv_mode->name))
> >> -			return tv_mode;
> >> -	}
> >> -	return NULL;
> >> +	return &tv_modes[format];
> >>  }
> >>  
> >> -static const struct tv_mode *
> >> -intel_tv_mode_find(struct intel_tv *intel_tv)
> >> +static const struct tv_mode *intel_tv_mode_find_committed(struct drm_connector *connector)
> >>  {
> >> -	return intel_tv_mode_lookup(intel_tv->tv_format);
> >> +	int format = intel_attached_tv(connector)->format;
> >> +
> >> +	return &tv_modes[format];
> >>  }
> >>  
> >>  static enum drm_mode_status
> >>  intel_tv_mode_valid(struct drm_connector *connector,
> >>  		    struct drm_display_mode *mode)
> >>  {
> >> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
> >> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> >> +	const struct tv_mode *tv_mode = intel_tv_mode_find_committed(connector);
> >>  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
> >>  
> >>  	if (mode->clock > max_dotclk)
> >> @@ -925,8 +939,7 @@ intel_tv_compute_config(struct intel_encoder *encoder,
> >>  			struct intel_crtc_state *pipe_config,
> >>  			struct drm_connector_state *conn_state)
> >>  {
> >> -	struct intel_tv *intel_tv = enc_to_tv(encoder);
> >> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> >> +	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
> >>  
> >>  	if (!tv_mode)
> >>  		return false;
> >> @@ -1032,7 +1045,7 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
> >>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >>  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> >>  	struct intel_tv *intel_tv = enc_to_tv(encoder);
> >> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> >> +	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
> >>  	u32 tv_ctl;
> >>  	u32 scctl1, scctl2, scctl3;
> >>  	int i, j;
> >> @@ -1041,6 +1054,8 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
> >>  	bool burst_ena;
> >>  	int xpos = 0x0, ypos = 0x0;
> >>  	unsigned int xsize, ysize;
> >> +	struct intel_tv_connector_state *tv_state =
> >> +		to_intel_tv_connector_state(conn_state);
> >>  
> >>  	if (!tv_mode)
> >>  		return;	/* can't happen (mode_prepare prevents this) */
> >> @@ -1135,12 +1150,12 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
> >>  	else
> >>  		ysize = 2*tv_mode->nbr_end + 1;
> >>  
> >> -	xpos += intel_tv->margin[TV_MARGIN_LEFT];
> >> -	ypos += intel_tv->margin[TV_MARGIN_TOP];
> >> -	xsize -= (intel_tv->margin[TV_MARGIN_LEFT] +
> >> -		  intel_tv->margin[TV_MARGIN_RIGHT]);
> >> -	ysize -= (intel_tv->margin[TV_MARGIN_TOP] +
> >> -		  intel_tv->margin[TV_MARGIN_BOTTOM]);
> >> +	xpos += tv_state->margin[TV_MARGIN_LEFT];
> >> +	ypos += tv_state->margin[TV_MARGIN_TOP];
> >> +	xsize -= (tv_state->margin[TV_MARGIN_LEFT] +
> >> +		  tv_state->margin[TV_MARGIN_RIGHT]);
> >> +	ysize -= (tv_state->margin[TV_MARGIN_TOP] +
> >> +		  tv_state->margin[TV_MARGIN_BOTTOM]);
> >>  	I915_WRITE(TV_WIN_POS, (xpos<<16)|ypos);
> >>  	I915_WRITE(TV_WIN_SIZE, (xsize<<16)|ysize);
> >>  
> >> @@ -1288,7 +1303,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
> >>  static void intel_tv_find_better_format(struct drm_connector *connector)
> >>  {
> >>  	struct intel_tv *intel_tv = intel_attached_tv(connector);
> >> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> >> +	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
> > intel_tv_mode_find_committed() ?
> Either works here. We're holding the relevant lock.

Oh you moved the call. Hmm. Do we even need to do that? I thought we
could just leave the in detect as is.

> >
> >>  	int i;
> >>  
> >>  	if ((intel_tv->type == DRM_MODE_CONNECTOR_Component) ==
> >> @@ -1304,9 +1319,8 @@ static void intel_tv_find_better_format(struct drm_connector *connector)
> >>  			break;
> >>  	}
> >>  
> >> -	intel_tv->tv_format = tv_mode->name;
> >> -	drm_object_property_set_value(&connector->base,
> >> -		connector->dev->mode_config.tv_mode_property, i);
> >> +	to_intel_tv_connector_state(connector->state)->format = i;
> > I was thinking we'd do this in duplicate_state.
> This would cause get_property to return stale values since it would not duplicate the state.

Hmm. So it would return the "old" value between detect and commit, once
commit happens it would again return the value detect() gave us
(assuming the user didn't override it). Is that bad behaviour or not?
Not sure.

I guess I'll need to go read the ddx code to see what, if anything, it
might do with this stuff...

> > With these two changes we shouldn't have to access ->state in detect()
> > AFAICS.
> I think it's the only thing that makes sense.

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

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2.

[Maarten Lankhorst]

Op 13-03-17 om 17:44 schreef Ville Syrjälä:
> On Mon, Mar 13, 2017 at 05:35:42PM +0100, Maarten Lankhorst wrote:
>> Op 13-03-17 om 17:22 schreef Ville Syrjälä:
>>> On Mon, Mar 13, 2017 at 05:10:28PM +0100, Maarten Lankhorst wrote:
>>>> As a proof of concept, first try to convert intel_tv, which is a rarely
>>>> used connector. It has 5 properties, tv format and 4 margins.
>>>>
>>>> I'm less certain about the state behavior itself, should we pass a size
>>>> parameter to intel_connector_alloc instead, so duplicate_state
>>>> can be done globally if it can be blindly copied?
>>>>
>>>> Can we also have a atomic_check function for connectors, so the
>>>> crtc_state->connectors_changed can be set there? It would be cleaner
>>>> and more atomic-like.
>>>>
>>>> To match the legacy behavior, format can be changed by probing just like
>>>> in legacy mode.
>>>>
>>>> Changes since v1:
>>>> - Add intel_encoder->swap_state to allow updating connector state.
>>>> - Add intel_tv->format for detect_mode and mode_valid, updated on atomic commit.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_display.c |  15 ++
>>>>  drivers/gpu/drm/i915/intel_drv.h     |   4 +
>>>>  drivers/gpu/drm/i915/intel_tv.c      | 259 +++++++++++++++++++++--------------
>>>>  3 files changed, 176 insertions(+), 102 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index ac25c9bc8b81..18b7e7546ee1 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -12954,6 +12954,20 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
>>>>  				  to_intel_plane(plane)->frontbuffer_bit);
>>>>  }
>>>>  
>>>> +static void swap_connector_state(struct drm_atomic_state *state)
>>>> +{
>>>> +	struct drm_connector *connector;
>>>> +	struct drm_connector_state *new_conn_state;
>>>> +	int i;
>>>> +
>>>> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
>>>> +		struct intel_connector *conn = to_intel_connector(connector);
>>>> +
>>>> +		if (conn->swap_state)
>>>> +			conn->swap_state(conn, new_conn_state);
>>>> +	}
>>>> +}
>>>> +
>>>>  /**
>>>>   * intel_atomic_commit - commit validated state object
>>>>   * @dev: DRM device
>>>> @@ -13012,6 +13026,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>>>>  		dev_priv->cdclk.logical = intel_state->cdclk.logical;
>>>>  		dev_priv->cdclk.actual = intel_state->cdclk.actual;
>>>>  	}
>>>> +	swap_connector_state(state);
>>>>  
>>>>  	drm_atomic_state_get(state);
>>>>  	INIT_WORK(&state->commit_work,
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>> index 937623ff6d7c..b7b93799d288 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -307,6 +307,10 @@ struct intel_connector {
>>>>  	 * and active (i.e. dpms ON state). */
>>>>  	bool (*get_hw_state)(struct intel_connector *);
>>>>  
>>>> +	/* Update device state with the new atomic state. */
>>>> +	void (*swap_state)(struct intel_connector *,
>>>> +			     struct drm_connector_state *);
>>>> +
>>>>  	/* Panel info for eDP and LVDS */
>>>>  	struct intel_panel panel;
>>>>  
>>>> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
>>>> index 6ed1a3ce47b7..b87d51e61975 100644
>>>> --- a/drivers/gpu/drm/i915/intel_tv.c
>>>> +++ b/drivers/gpu/drm/i915/intel_tv.c
>>>> @@ -48,8 +48,6 @@ struct intel_tv {
>>>>  	struct intel_encoder base;
>>>>  
>>>>  	int type;
>>>> -	const char *tv_format;
>>>> -	int margin[4];
>>>>  	u32 save_TV_H_CTL_1;
>>>>  	u32 save_TV_H_CTL_2;
>>>>  	u32 save_TV_H_CTL_3;
>>>> @@ -83,8 +81,20 @@ struct intel_tv {
>>>>  
>>>>  	u32 save_TV_DAC;
>>>>  	u32 save_TV_CTL;
>>>> +
>>>> +	int format;
>>>> +};
>>>> +
>>>> +struct intel_tv_connector_state {
>>>> +	struct drm_connector_state base;
>>>> +
>>>> +	int format;
>>>> +	int margin[4];
>>>>  };
>>>>  
>>>> +#define to_intel_tv_connector_state(state) \
>>>> +	container_of((state), struct intel_tv_connector_state, base)
>>>> +
>>>>  struct video_levels {
>>>>  	u16 blank, black;
>>>>  	u8 burst;
>>>> @@ -848,6 +858,17 @@ intel_tv_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe)
>>>>  }
>>>>  
>>>>  static void
>>>> +intel_tv_swap_state(struct intel_connector *connector,
>>>> +		    struct drm_connector_state *conn_state)
>>>> +{
>>>> +	struct intel_tv *intel_tv = intel_attached_tv(&connector->base);
>>>> +	struct intel_tv_connector_state *tv_state =
>>>> +		to_intel_tv_connector_state(conn_state);
>>>> +
>>>> +	intel_tv->format = tv_state->format;
>>>> +}
>>>> +
>>>> +static void
>>>>  intel_enable_tv(struct intel_encoder *encoder,
>>>>  		struct intel_crtc_state *pipe_config,
>>>>  		struct drm_connector_state *conn_state)
>>>> @@ -873,32 +894,25 @@ intel_disable_tv(struct intel_encoder *encoder,
>>>>  	I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
>>>>  }
>>>>  
>>>> -static const struct tv_mode *
>>>> -intel_tv_mode_lookup(const char *tv_format)
>>>> +static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
>>>>  {
>>>> -	int i;
>>>> -
>>>> -	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
>>>> -		const struct tv_mode *tv_mode = &tv_modes[i];
>>>> +	int format = to_intel_tv_connector_state(conn_state)->format;
>>>>  
>>>> -		if (!strcmp(tv_format, tv_mode->name))
>>>> -			return tv_mode;
>>>> -	}
>>>> -	return NULL;
>>>> +	return &tv_modes[format];
>>>>  }
>>>>  
>>>> -static const struct tv_mode *
>>>> -intel_tv_mode_find(struct intel_tv *intel_tv)
>>>> +static const struct tv_mode *intel_tv_mode_find_committed(struct drm_connector *connector)
>>>>  {
>>>> -	return intel_tv_mode_lookup(intel_tv->tv_format);
>>>> +	int format = intel_attached_tv(connector)->format;
>>>> +
>>>> +	return &tv_modes[format];
>>>>  }
>>>>  
>>>>  static enum drm_mode_status
>>>>  intel_tv_mode_valid(struct drm_connector *connector,
>>>>  		    struct drm_display_mode *mode)
>>>>  {
>>>> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
>>>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>>>> +	const struct tv_mode *tv_mode = intel_tv_mode_find_committed(connector);
>>>>  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
>>>>  
>>>>  	if (mode->clock > max_dotclk)
>>>> @@ -925,8 +939,7 @@ intel_tv_compute_config(struct intel_encoder *encoder,
>>>>  			struct intel_crtc_state *pipe_config,
>>>>  			struct drm_connector_state *conn_state)
>>>>  {
>>>> -	struct intel_tv *intel_tv = enc_to_tv(encoder);
>>>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>>>> +	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
>>>>  
>>>>  	if (!tv_mode)
>>>>  		return false;
>>>> @@ -1032,7 +1045,7 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>>>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>>>  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>>>>  	struct intel_tv *intel_tv = enc_to_tv(encoder);
>>>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>>>> +	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
>>>>  	u32 tv_ctl;
>>>>  	u32 scctl1, scctl2, scctl3;
>>>>  	int i, j;
>>>> @@ -1041,6 +1054,8 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>>>>  	bool burst_ena;
>>>>  	int xpos = 0x0, ypos = 0x0;
>>>>  	unsigned int xsize, ysize;
>>>> +	struct intel_tv_connector_state *tv_state =
>>>> +		to_intel_tv_connector_state(conn_state);
>>>>  
>>>>  	if (!tv_mode)
>>>>  		return;	/* can't happen (mode_prepare prevents this) */
>>>> @@ -1135,12 +1150,12 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
>>>>  	else
>>>>  		ysize = 2*tv_mode->nbr_end + 1;
>>>>  
>>>> -	xpos += intel_tv->margin[TV_MARGIN_LEFT];
>>>> -	ypos += intel_tv->margin[TV_MARGIN_TOP];
>>>> -	xsize -= (intel_tv->margin[TV_MARGIN_LEFT] +
>>>> -		  intel_tv->margin[TV_MARGIN_RIGHT]);
>>>> -	ysize -= (intel_tv->margin[TV_MARGIN_TOP] +
>>>> -		  intel_tv->margin[TV_MARGIN_BOTTOM]);
>>>> +	xpos += tv_state->margin[TV_MARGIN_LEFT];
>>>> +	ypos += tv_state->margin[TV_MARGIN_TOP];
>>>> +	xsize -= (tv_state->margin[TV_MARGIN_LEFT] +
>>>> +		  tv_state->margin[TV_MARGIN_RIGHT]);
>>>> +	ysize -= (tv_state->margin[TV_MARGIN_TOP] +
>>>> +		  tv_state->margin[TV_MARGIN_BOTTOM]);
>>>>  	I915_WRITE(TV_WIN_POS, (xpos<<16)|ypos);
>>>>  	I915_WRITE(TV_WIN_SIZE, (xsize<<16)|ysize);
>>>>  
>>>> @@ -1288,7 +1303,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
>>>>  static void intel_tv_find_better_format(struct drm_connector *connector)
>>>>  {
>>>>  	struct intel_tv *intel_tv = intel_attached_tv(connector);
>>>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>>>> +	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
>>> intel_tv_mode_find_committed() ?
>> Either works here. We're holding the relevant lock.
> Oh you moved the call. Hmm. Do we even need to do that? I thought we
> could just leave the in detect as is.
>
>>>>  	int i;
>>>>  
>>>>  	if ((intel_tv->type == DRM_MODE_CONNECTOR_Component) ==
>>>> @@ -1304,9 +1319,8 @@ static void intel_tv_find_better_format(struct drm_connector *connector)
>>>>  			break;
>>>>  	}
>>>>  
>>>> -	intel_tv->tv_format = tv_mode->name;
>>>> -	drm_object_property_set_value(&connector->base,
>>>> -		connector->dev->mode_config.tv_mode_property, i);
>>>> +	to_intel_tv_connector_state(connector->state)->format = i;
>>> I was thinking we'd do this in duplicate_state.
>> This would cause get_property to return stale values since it would not duplicate the state.
> Hmm. So it would return the "old" value between detect and commit, once
> commit happens it would again return the value detect() gave us
> (assuming the user didn't override it). Is that bad behaviour or not?
> Not sure.
It's definitely a difference with the old code. So prone to regression if we introduce a behavioral change, best keep it the way it is.
> I guess I'll need to go read the ddx code to see what, if anything, it
> might do with this stuff...
>
>>> With these two changes we shouldn't have to access ->state in detect()
>>> AFAICS.
>> I think it's the only thing that makes sense.


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

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2.

[Ville Syrjälä]

On Mon, Mar 13, 2017 at 05:10:28PM +0100, Maarten Lankhorst wrote:
> As a proof of concept, first try to convert intel_tv, which is a rarely
> used connector. It has 5 properties, tv format and 4 margins.
> 
> I'm less certain about the state behavior itself, should we pass a size
> parameter to intel_connector_alloc instead, so duplicate_state
> can be done globally if it can be blindly copied?
> 
> Can we also have a atomic_check function for connectors, so the
> crtc_state->connectors_changed can be set there? It would be cleaner
> and more atomic-like.
> 
> To match the legacy behavior, format can be changed by probing just like
> in legacy mode.
> 
> Changes since v1:
> - Add intel_encoder->swap_state to allow updating connector state.
> - Add intel_tv->format for detect_mode and mode_valid, updated on atomic commit.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  15 ++
>  drivers/gpu/drm/i915/intel_drv.h     |   4 +
>  drivers/gpu/drm/i915/intel_tv.c      | 259 +++++++++++++++++++++--------------
>  3 files changed, 176 insertions(+), 102 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ac25c9bc8b81..18b7e7546ee1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
<snip>
> +
> +static int
> +intel_tv_atomic_get_property(struct drm_connector *connector,
> +				    const struct drm_connector_state *state,
> +				    struct drm_property *property,
> +				    uint64_t *val)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct intel_tv_connector_state *tv_state =
> +		to_intel_tv_connector_state(state);
> +
> +	if (property == dev->mode_config.tv_left_margin_property)
> +		*val = tv_state->margin[TV_MARGIN_LEFT];
> +	else if (property == dev->mode_config.tv_right_margin_property)
> +		*val = tv_state->margin[TV_MARGIN_RIGHT];
> +	else if (property == dev->mode_config.tv_top_margin_property)
> +		*val = tv_state->margin[TV_MARGIN_TOP];
> +	else if (property == dev->mode_config.tv_bottom_margin_property)
> +		*val = tv_state->margin[TV_MARGIN_BOTTOM];
> +	else if (property == dev->mode_config.tv_mode_property) {
> +		*val = tv_state->format;

Actually, aren't all of these handled by drm_atomic_connector_set_property()
already?

And same deal with drm_atomic_connector_get_property() AFAICS.

>  	} else {
> -		ret = -EINVAL;
> -		goto out;
> +		DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name);
> +		return -EINVAL;
>  	}
>  
> -	if (changed && crtc)
> -		intel_crtc_restore_mode(crtc);
> -out:
> -	return ret;
> +	return 0;
> +}
> +
> +static struct drm_connector_state *
> +intel_tv_duplicate_state(struct drm_connector *connector)
> +{
> +	struct intel_tv_connector_state *state;
> +
> +	state = kmemdup(&connector->state, sizeof(*state), GFP_KERNEL);
> +	if (state)
> +		__drm_atomic_helper_connector_duplicate_state(connector, &state->base);
> +
> +	return &state->base;
>  }
>  
>  static const struct drm_connector_funcs intel_tv_connector_funcs = {
> @@ -1520,11 +1570,12 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = {
>  	.late_register = intel_connector_register,
>  	.early_unregister = intel_connector_unregister,
>  	.destroy = intel_tv_destroy,
> -	.set_property = intel_tv_set_property,
> -	.atomic_get_property = intel_connector_atomic_get_property,
> +	.set_property = drm_atomic_helper_connector_set_property,
> +	.atomic_set_property = intel_tv_atomic_set_property,
> +	.atomic_get_property = intel_tv_atomic_get_property,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_duplicate_state = intel_tv_duplicate_state,
>  };
>  
>  static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = {
> @@ -1547,6 +1598,7 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  	u32 tv_dac_on, tv_dac_off, save_tv_dac;
>  	const char *tv_format_names[ARRAY_SIZE(tv_modes)];
>  	int i, initial_mode = 0;
> +	struct intel_tv_connector_state *state;
>  
>  	if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED)
>  		return;
> @@ -1580,16 +1632,17 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  		return;
>  
>  	intel_tv = kzalloc(sizeof(*intel_tv), GFP_KERNEL);
> -	if (!intel_tv) {
> -		return;
> -	}
> -
> -	intel_connector = intel_connector_alloc();
> -	if (!intel_connector) {
> +	intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL);
> +	state = kzalloc(sizeof(*state), GFP_KERNEL);
> +	if (!intel_tv || !intel_connector || !state) {
> +		kfree(intel_connector);
> +		kfree(state);
>  		kfree(intel_tv);
>  		return;
>  	}
>  
> +	__drm_atomic_helper_connector_reset(&intel_connector->base, &state->base);
> +
>  	intel_encoder = &intel_tv->base;
>  	connector = &intel_connector->base;
>  
> @@ -1617,6 +1670,7 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  	intel_encoder->disable = intel_disable_tv;
>  	intel_encoder->get_hw_state = intel_tv_get_hw_state;
>  	intel_connector->get_hw_state = intel_connector_get_hw_state;
> +	intel_connector->swap_state = intel_tv_swap_state;
>  
>  	intel_connector_attach_encoder(intel_connector, intel_encoder);
>  
> @@ -1629,12 +1683,13 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  	intel_tv->type = DRM_MODE_CONNECTOR_Unknown;
>  
>  	/* BIOS margin values */
> -	intel_tv->margin[TV_MARGIN_LEFT] = 54;
> -	intel_tv->margin[TV_MARGIN_TOP] = 36;
> -	intel_tv->margin[TV_MARGIN_RIGHT] = 46;
> -	intel_tv->margin[TV_MARGIN_BOTTOM] = 37;
> +	state->margin[TV_MARGIN_LEFT] = 54;
> +	state->margin[TV_MARGIN_TOP] = 36;
> +	state->margin[TV_MARGIN_RIGHT] = 46;
> +	state->margin[TV_MARGIN_BOTTOM] = 37;
>  
> -	intel_tv->tv_format = tv_modes[initial_mode].name;
> +	state->format = initial_mode;
> +	intel_tv->format = initial_mode;
>  
>  	drm_connector_helper_add(connector, &intel_tv_connector_helper_funcs);
>  	connector->interlace_allowed = false;
> @@ -1648,17 +1703,17 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  				      tv_format_names);
>  
>  	drm_object_attach_property(&connector->base, dev->mode_config.tv_mode_property,
> -				   initial_mode);
> +				   state->format);
>  	drm_object_attach_property(&connector->base,
>  				   dev->mode_config.tv_left_margin_property,
> -				   intel_tv->margin[TV_MARGIN_LEFT]);
> +				   state->margin[TV_MARGIN_LEFT]);
>  	drm_object_attach_property(&connector->base,
>  				   dev->mode_config.tv_top_margin_property,
> -				   intel_tv->margin[TV_MARGIN_TOP]);
> +				   state->margin[TV_MARGIN_TOP]);
>  	drm_object_attach_property(&connector->base,
>  				   dev->mode_config.tv_right_margin_property,
> -				   intel_tv->margin[TV_MARGIN_RIGHT]);
> +				   state->margin[TV_MARGIN_RIGHT]);
>  	drm_object_attach_property(&connector->base,
>  				   dev->mode_config.tv_bottom_margin_property,
> -				   intel_tv->margin[TV_MARGIN_BOTTOM]);
> +				   state->margin[TV_MARGIN_BOTTOM]);
>  }
> -- 
> 2.7.4
> 

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

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2.

[Maarten Lankhorst]

Op 14-03-17 om 19:36 schreef Ville Syrjälä:
> On Mon, Mar 13, 2017 at 05:10:28PM +0100, Maarten Lankhorst wrote:
>> As a proof of concept, first try to convert intel_tv, which is a rarely
>> used connector. It has 5 properties, tv format and 4 margins.
>>
>> I'm less certain about the state behavior itself, should we pass a size
>> parameter to intel_connector_alloc instead, so duplicate_state
>> can be done globally if it can be blindly copied?
>>
>> Can we also have a atomic_check function for connectors, so the
>> crtc_state->connectors_changed can be set there? It would be cleaner
>> and more atomic-like.
>>
>> To match the legacy behavior, format can be changed by probing just like
>> in legacy mode.
>>
>> Changes since v1:
>> - Add intel_encoder->swap_state to allow updating connector state.
>> - Add intel_tv->format for detect_mode and mode_valid, updated on atomic commit.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c |  15 ++
>>  drivers/gpu/drm/i915/intel_drv.h     |   4 +
>>  drivers/gpu/drm/i915/intel_tv.c      | 259 +++++++++++++++++++++--------------
>>  3 files changed, 176 insertions(+), 102 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index ac25c9bc8b81..18b7e7546ee1 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
> <snip>
>> +
>> +static int
>> +intel_tv_atomic_get_property(struct drm_connector *connector,
>> +				    const struct drm_connector_state *state,
>> +				    struct drm_property *property,
>> +				    uint64_t *val)
>> +{
>> +	struct drm_device *dev = connector->dev;
>> +	struct intel_tv_connector_state *tv_state =
>> +		to_intel_tv_connector_state(state);
>> +
>> +	if (property == dev->mode_config.tv_left_margin_property)
>> +		*val = tv_state->margin[TV_MARGIN_LEFT];
>> +	else if (property == dev->mode_config.tv_right_margin_property)
>> +		*val = tv_state->margin[TV_MARGIN_RIGHT];
>> +	else if (property == dev->mode_config.tv_top_margin_property)
>> +		*val = tv_state->margin[TV_MARGIN_TOP];
>> +	else if (property == dev->mode_config.tv_bottom_margin_property)
>> +		*val = tv_state->margin[TV_MARGIN_BOTTOM];
>> +	else if (property == dev->mode_config.tv_mode_property) {
>> +		*val = tv_state->format;
> Actually, aren't all of these handled by drm_atomic_connector_set_property()
> already?
>
> And same deal with drm_atomic_connector_get_property() AFAICS.
Argh, indeed. Thanks for noticing.. That should reduce the amount of boilerplate.

Is there hw for doing actual testing on the tv connector? If I actually tested this on real hw I would have noticed this.

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

[PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3.

[Maarten Lankhorst]

As a proof of concept, first try to convert intel_tv, which is a rarely
used connector. It has 5 properties, tv format and 4 margins.

I'm less certain about the state behavior itself, should we pass a size
parameter to intel_connector_alloc instead, so duplicate_state
can be done globally if it can be blindly copied?

Can we also have a atomic_check function for connectors, so the
crtc_state->connectors_changed can be set there? It would be cleaner
and more atomic-like.

To match the legacy behavior, format can be changed by probing just like
in legacy mode.

Changes since v1:
- Add intel_encoder->swap_state to allow updating connector state.
- Add intel_tv->format for detect_mode and mode_valid, updated on atomic commit.
Changes since v2:
- Fix typo in tv_choose_preferred modes function name.
- Assignment of tv properties is done in core, so intel_tv only needs
  a atomic_check function. Thanks Ville!

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  15 +++
 drivers/gpu/drm/i915/intel_drv.h     |   4 +
 drivers/gpu/drm/i915/intel_tv.c      | 200 ++++++++++++++++-------------------
 3 files changed, 110 insertions(+), 109 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ac25c9bc8b81..18b7e7546ee1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12954,6 +12954,20 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
 				  to_intel_plane(plane)->frontbuffer_bit);
 }
 
+static void swap_connector_state(struct drm_atomic_state *state)
+{
+	struct drm_connector *connector;
+	struct drm_connector_state *new_conn_state;
+	int i;
+
+	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
+		struct intel_connector *conn = to_intel_connector(connector);
+
+		if (conn->swap_state)
+			conn->swap_state(conn, new_conn_state);
+	}
+}
+
 /**
  * intel_atomic_commit - commit validated state object
  * @dev: DRM device
@@ -13012,6 +13026,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		dev_priv->cdclk.logical = intel_state->cdclk.logical;
 		dev_priv->cdclk.actual = intel_state->cdclk.actual;
 	}
+	swap_connector_state(state);
 
 	drm_atomic_state_get(state);
 	INIT_WORK(&state->commit_work,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 937623ff6d7c..b7b93799d288 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -307,6 +307,10 @@ struct intel_connector {
 	 * and active (i.e. dpms ON state). */
 	bool (*get_hw_state)(struct intel_connector *);
 
+	/* Update device state with the new atomic state. */
+	void (*swap_state)(struct intel_connector *,
+			     struct drm_connector_state *);
+
 	/* Panel info for eDP and LVDS */
 	struct intel_panel panel;
 
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 6ed1a3ce47b7..eeed17e6741a 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -48,8 +48,6 @@ struct intel_tv {
 	struct intel_encoder base;
 
 	int type;
-	const char *tv_format;
-	int margin[4];
 	u32 save_TV_H_CTL_1;
 	u32 save_TV_H_CTL_2;
 	u32 save_TV_H_CTL_3;
@@ -83,6 +81,8 @@ struct intel_tv {
 
 	u32 save_TV_DAC;
 	u32 save_TV_CTL;
+
+	int format;
 };
 
 struct video_levels {
@@ -848,6 +848,15 @@ intel_tv_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe)
 }
 
 static void
+intel_tv_swap_state(struct intel_connector *connector,
+		    struct drm_connector_state *conn_state)
+{
+	struct intel_tv *intel_tv = intel_attached_tv(&connector->base);
+
+	intel_tv->format = conn_state->tv.mode;
+}
+
+static void
 intel_enable_tv(struct intel_encoder *encoder,
 		struct intel_crtc_state *pipe_config,
 		struct drm_connector_state *conn_state)
@@ -873,32 +882,25 @@ intel_disable_tv(struct intel_encoder *encoder,
 	I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
 }
 
-static const struct tv_mode *
-intel_tv_mode_lookup(const char *tv_format)
+static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
 {
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
-		const struct tv_mode *tv_mode = &tv_modes[i];
+	int format = conn_state->tv.mode;
 
-		if (!strcmp(tv_format, tv_mode->name))
-			return tv_mode;
-	}
-	return NULL;
+	return &tv_modes[format];
 }
 
-static const struct tv_mode *
-intel_tv_mode_find(struct intel_tv *intel_tv)
+static const struct tv_mode *intel_tv_mode_find_committed(struct drm_connector *connector)
 {
-	return intel_tv_mode_lookup(intel_tv->tv_format);
+	int format = intel_attached_tv(connector)->format;
+
+	return &tv_modes[format];
 }
 
 static enum drm_mode_status
 intel_tv_mode_valid(struct drm_connector *connector,
 		    struct drm_display_mode *mode)
 {
-	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find_committed(connector);
 	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
 
 	if (mode->clock > max_dotclk)
@@ -925,8 +927,7 @@ intel_tv_compute_config(struct intel_encoder *encoder,
 			struct intel_crtc_state *pipe_config,
 			struct drm_connector_state *conn_state)
 {
-	struct intel_tv *intel_tv = enc_to_tv(encoder);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
 
 	if (!tv_mode)
 		return false;
@@ -1032,7 +1033,7 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
 	struct intel_tv *intel_tv = enc_to_tv(encoder);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
 	u32 tv_ctl;
 	u32 scctl1, scctl2, scctl3;
 	int i, j;
@@ -1135,12 +1136,12 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
 	else
 		ysize = 2*tv_mode->nbr_end + 1;
 
-	xpos += intel_tv->margin[TV_MARGIN_LEFT];
-	ypos += intel_tv->margin[TV_MARGIN_TOP];
-	xsize -= (intel_tv->margin[TV_MARGIN_LEFT] +
-		  intel_tv->margin[TV_MARGIN_RIGHT]);
-	ysize -= (intel_tv->margin[TV_MARGIN_TOP] +
-		  intel_tv->margin[TV_MARGIN_BOTTOM]);
+	xpos += conn_state->tv.margins.left;
+	ypos += conn_state->tv.margins.top;
+	xsize -= (conn_state->tv.margins.left +
+		  conn_state->tv.margins.right);
+	ysize -= (conn_state->tv.margins.top +
+		  conn_state->tv.margins.bottom);
 	I915_WRITE(TV_WIN_POS, (xpos<<16)|ypos);
 	I915_WRITE(TV_WIN_SIZE, (xsize<<16)|ysize);
 
@@ -1288,7 +1289,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
 static void intel_tv_find_better_format(struct drm_connector *connector)
 {
 	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
 	int i;
 
 	if ((intel_tv->type == DRM_MODE_CONNECTOR_Component) ==
@@ -1304,9 +1305,8 @@ static void intel_tv_find_better_format(struct drm_connector *connector)
 			break;
 	}
 
-	intel_tv->tv_format = tv_mode->name;
-	drm_object_property_set_value(&connector->base,
-		connector->dev->mode_config.tv_mode_property, i);
+	connector->state->tv.mode = i;
+	intel_attached_tv(connector)->format = i;
 }
 
 /**
@@ -1344,18 +1344,17 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 		} else
 			status = connector_status_unknown;
 
+		if (status == connector_status_connected) {
+			intel_tv->type = type;
+			intel_tv_find_better_format(connector);
+		}
+
 		drm_modeset_drop_locks(&ctx);
 		drm_modeset_acquire_fini(&ctx);
-	} else
-		return connector->status;
 
-	if (status != connector_status_connected)
 		return status;
-
-	intel_tv->type = type;
-	intel_tv_find_better_format(connector);
-
-	return connector_status_connected;
+	} else
+		return connector->status;
 }
 
 static const struct input_res {
@@ -1375,12 +1374,9 @@ static const struct input_res {
  * Chose preferred mode  according to line number of TV format
  */
 static void
-intel_tv_chose_preferred_modes(struct drm_connector *connector,
+intel_tv_choose_preferred_modes(const struct tv_mode *tv_mode,
 			       struct drm_display_mode *mode_ptr)
 {
-	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
-
 	if (tv_mode->nbr_end < 480 && mode_ptr->vdisplay == 480)
 		mode_ptr->type |= DRM_MODE_TYPE_PREFERRED;
 	else if (tv_mode->nbr_end > 480) {
@@ -1403,8 +1399,7 @@ static int
 intel_tv_get_modes(struct drm_connector *connector)
 {
 	struct drm_display_mode *mode_ptr;
-	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv_mode_find_committed(connector);
 	int j, count = 0;
 	u64 tmp;
 
@@ -1447,7 +1442,7 @@ intel_tv_get_modes(struct drm_connector *connector)
 		mode_ptr->clock = (int) tmp;
 
 		mode_ptr->type = DRM_MODE_TYPE_DRIVER;
-		intel_tv_chose_preferred_modes(connector, mode_ptr);
+		intel_tv_choose_preferred_modes(tv_mode, mode_ptr);
 		drm_mode_probed_add(connector, mode_ptr);
 		count++;
 	}
@@ -1462,66 +1457,13 @@ intel_tv_destroy(struct drm_connector *connector)
 	kfree(connector);
 }
 
-
-static int
-intel_tv_set_property(struct drm_connector *connector, struct drm_property *property,
-		      uint64_t val)
-{
-	struct drm_device *dev = connector->dev;
-	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	struct drm_crtc *crtc = intel_tv->base.base.crtc;
-	int ret = 0;
-	bool changed = false;
-
-	ret = drm_object_property_set_value(&connector->base, property, val);
-	if (ret < 0)
-		goto out;
-
-	if (property == dev->mode_config.tv_left_margin_property &&
-		intel_tv->margin[TV_MARGIN_LEFT] != val) {
-		intel_tv->margin[TV_MARGIN_LEFT] = val;
-		changed = true;
-	} else if (property == dev->mode_config.tv_right_margin_property &&
-		intel_tv->margin[TV_MARGIN_RIGHT] != val) {
-		intel_tv->margin[TV_MARGIN_RIGHT] = val;
-		changed = true;
-	} else if (property == dev->mode_config.tv_top_margin_property &&
-		intel_tv->margin[TV_MARGIN_TOP] != val) {
-		intel_tv->margin[TV_MARGIN_TOP] = val;
-		changed = true;
-	} else if (property == dev->mode_config.tv_bottom_margin_property &&
-		intel_tv->margin[TV_MARGIN_BOTTOM] != val) {
-		intel_tv->margin[TV_MARGIN_BOTTOM] = val;
-		changed = true;
-	} else if (property == dev->mode_config.tv_mode_property) {
-		if (val >= ARRAY_SIZE(tv_modes)) {
-			ret = -EINVAL;
-			goto out;
-		}
-		if (!strcmp(intel_tv->tv_format, tv_modes[val].name))
-			goto out;
-
-		intel_tv->tv_format = tv_modes[val].name;
-		changed = true;
-	} else {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	if (changed && crtc)
-		intel_crtc_restore_mode(crtc);
-out:
-	return ret;
-}
-
 static const struct drm_connector_funcs intel_tv_connector_funcs = {
 	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_tv_detect,
 	.late_register = intel_connector_register,
 	.early_unregister = intel_connector_unregister,
 	.destroy = intel_tv_destroy,
-	.set_property = intel_tv_set_property,
-	.atomic_get_property = intel_connector_atomic_get_property,
+	.set_property = drm_atomic_helper_connector_set_property,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
@@ -1532,6 +1474,41 @@ static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs =
 	.get_modes = intel_tv_get_modes,
 };
 
+static int intel_tv_atomic_check(struct drm_encoder *encoder,
+				 struct drm_crtc_state *crtc_state,
+				 struct drm_connector_state *new_state)
+{
+	struct drm_connector_state *old_state = new_state->connector->state;
+
+	if (old_state->tv.mode != new_state->tv.mode ||
+	    old_state->tv.margins.left != new_state->tv.margins.left ||
+	    old_state->tv.margins.right != new_state->tv.margins.right ||
+	    old_state->tv.margins.top != new_state->tv.margins.top ||
+	    old_state->tv.margins.bottom != new_state->tv.margins.bottom) {
+		int ret;
+
+		crtc_state->connectors_changed = true;
+
+		/*
+		 * This is called after drm_atomic_helper_check_modeset adds
+		 * affected planes/connectors, so we have to add them manually.
+		 */
+		ret = drm_atomic_add_affected_connectors(new_state->state, crtc_state->crtc);
+		if (ret)
+			return ret;
+
+		ret = drm_atomic_add_affected_planes(new_state->state, crtc_state->crtc);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct drm_encoder_helper_funcs intel_tv_enc_helper_funcs = {
+	.atomic_check = intel_tv_atomic_check,
+};
+
 static const struct drm_encoder_funcs intel_tv_enc_funcs = {
 	.destroy = intel_encoder_destroy,
 };
@@ -1547,6 +1524,7 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 	u32 tv_dac_on, tv_dac_off, save_tv_dac;
 	const char *tv_format_names[ARRAY_SIZE(tv_modes)];
 	int i, initial_mode = 0;
+	struct drm_connector_state *state;
 
 	if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED)
 		return;
@@ -1592,6 +1570,7 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 
 	intel_encoder = &intel_tv->base;
 	connector = &intel_connector->base;
+	state = connector->state;
 
 	/* The documentation, for the older chipsets at least, recommend
 	 * using a polling method rather than hotplug detection for TVs.
@@ -1617,6 +1596,7 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 	intel_encoder->disable = intel_disable_tv;
 	intel_encoder->get_hw_state = intel_tv_get_hw_state;
 	intel_connector->get_hw_state = intel_connector_get_hw_state;
+	intel_connector->swap_state = intel_tv_swap_state;
 
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
 
@@ -1627,14 +1607,16 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 	intel_encoder->cloneable = 0;
 	intel_encoder->base.possible_crtcs = ((1 << 0) | (1 << 1));
 	intel_tv->type = DRM_MODE_CONNECTOR_Unknown;
+	drm_encoder_helper_add(&intel_encoder->base, &intel_tv_enc_helper_funcs);
 
 	/* BIOS margin values */
-	intel_tv->margin[TV_MARGIN_LEFT] = 54;
-	intel_tv->margin[TV_MARGIN_TOP] = 36;
-	intel_tv->margin[TV_MARGIN_RIGHT] = 46;
-	intel_tv->margin[TV_MARGIN_BOTTOM] = 37;
+	state->tv.margins.left = 54;
+	state->tv.margins.top = 36;
+	state->tv.margins.right = 46;
+	state->tv.margins.bottom = 37;
 
-	intel_tv->tv_format = tv_modes[initial_mode].name;
+	state->tv.mode = initial_mode;
+	intel_tv->format = initial_mode;
 
 	drm_connector_helper_add(connector, &intel_tv_connector_helper_funcs);
 	connector->interlace_allowed = false;
@@ -1648,17 +1630,17 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 				      tv_format_names);
 
 	drm_object_attach_property(&connector->base, dev->mode_config.tv_mode_property,
-				   initial_mode);
+				   state->tv.mode);
 	drm_object_attach_property(&connector->base,
 				   dev->mode_config.tv_left_margin_property,
-				   intel_tv->margin[TV_MARGIN_LEFT]);
+				   state->tv.margins.left);
 	drm_object_attach_property(&connector->base,
 				   dev->mode_config.tv_top_margin_property,
-				   intel_tv->margin[TV_MARGIN_TOP]);
+				   state->tv.margins.top);
 	drm_object_attach_property(&connector->base,
 				   dev->mode_config.tv_right_margin_property,
-				   intel_tv->margin[TV_MARGIN_RIGHT]);
+				   state->tv.margins.right);
 	drm_object_attach_property(&connector->base,
 				   dev->mode_config.tv_bottom_margin_property,
-				   intel_tv->margin[TV_MARGIN_BOTTOM]);
+				   state->tv.margins.bottom);
 }
-- 
2.7.4


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

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3.

[Daniel Vetter]

On Wed, Mar 15, 2017 at 11:08:56AM +0100, Maarten Lankhorst wrote:
> As a proof of concept, first try to convert intel_tv, which is a rarely
> used connector. It has 5 properties, tv format and 4 margins.
> 
> I'm less certain about the state behavior itself, should we pass a size
> parameter to intel_connector_alloc instead, so duplicate_state
> can be done globally if it can be blindly copied?
> 
> Can we also have a atomic_check function for connectors, so the
> crtc_state->connectors_changed can be set there? It would be cleaner
> and more atomic-like.
> 
> To match the legacy behavior, format can be changed by probing just like
> in legacy mode.
> 
> Changes since v1:
> - Add intel_encoder->swap_state to allow updating connector state.
> - Add intel_tv->format for detect_mode and mode_valid, updated on atomic commit.
> Changes since v2:
> - Fix typo in tv_choose_preferred modes function name.
> - Assignment of tv properties is done in core, so intel_tv only needs
>   a atomic_check function. Thanks Ville!
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  15 +++
>  drivers/gpu/drm/i915/intel_drv.h     |   4 +
>  drivers/gpu/drm/i915/intel_tv.c      | 200 ++++++++++++++++-------------------
>  3 files changed, 110 insertions(+), 109 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ac25c9bc8b81..18b7e7546ee1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12954,6 +12954,20 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
>  				  to_intel_plane(plane)->frontbuffer_bit);
>  }
>  
> +static void swap_connector_state(struct drm_atomic_state *state)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_state *new_conn_state;
> +	int i;
> +
> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> +		struct intel_connector *conn = to_intel_connector(connector);
> +
> +		if (conn->swap_state)
> +			conn->swap_state(conn, new_conn_state);
> +	}
> +}

So yeah I'm late, but this is pretty much exactly what DK's private state
object stuff was meant for. Have you looked into rebasing on top of that?
I think hand-rolling private state stuff everywhere isn't really good.

Afaics DK's patch series is ready for merging into drm-misc, so shouldn't
end up blocking you.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3.

[Maarten Lankhorst]

Op 16-03-17 om 11:48 schreef Daniel Vetter:
> On Wed, Mar 15, 2017 at 11:08:56AM +0100, Maarten Lankhorst wrote:
>> As a proof of concept, first try to convert intel_tv, which is a rarely
>> used connector. It has 5 properties, tv format and 4 margins.
>>
>> I'm less certain about the state behavior itself, should we pass a size
>> parameter to intel_connector_alloc instead, so duplicate_state
>> can be done globally if it can be blindly copied?
>>
>> Can we also have a atomic_check function for connectors, so the
>> crtc_state->connectors_changed can be set there? It would be cleaner
>> and more atomic-like.
>>
>> To match the legacy behavior, format can be changed by probing just like
>> in legacy mode.
>>
>> Changes since v1:
>> - Add intel_encoder->swap_state to allow updating connector state.
>> - Add intel_tv->format for detect_mode and mode_valid, updated on atomic commit.
>> Changes since v2:
>> - Fix typo in tv_choose_preferred modes function name.
>> - Assignment of tv properties is done in core, so intel_tv only needs
>>   a atomic_check function. Thanks Ville!
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c |  15 +++
>>  drivers/gpu/drm/i915/intel_drv.h     |   4 +
>>  drivers/gpu/drm/i915/intel_tv.c      | 200 ++++++++++++++++-------------------
>>  3 files changed, 110 insertions(+), 109 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index ac25c9bc8b81..18b7e7546ee1 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12954,6 +12954,20 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
>>  				  to_intel_plane(plane)->frontbuffer_bit);
>>  }
>>  
>> +static void swap_connector_state(struct drm_atomic_state *state)
>> +{
>> +	struct drm_connector *connector;
>> +	struct drm_connector_state *new_conn_state;
>> +	int i;
>> +
>> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
>> +		struct intel_connector *conn = to_intel_connector(connector);
>> +
>> +		if (conn->swap_state)
>> +			conn->swap_state(conn, new_conn_state);
>> +	}
>> +}
> So yeah I'm late, but this is pretty much exactly what DK's private state
> object stuff was meant for. Have you looked into rebasing on top of that?
> I think hand-rolling private state stuff everywhere isn't really good.
>
> Afaics DK's patch series is ready for merging into drm-misc, so shouldn't
> end up blocking you.
This swap_state hook is used for connector state. We could probably add a swap_state hook that drivers
can use for this in the core, but the connector state is already part of the state, adding it twice is silly.

Instead, would adding a hook to the core be useful?

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

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3.

[Daniel Vetter]

On Thu, Mar 16, 2017 at 01:05:06PM +0100, Maarten Lankhorst wrote:
> Op 16-03-17 om 11:48 schreef Daniel Vetter:
> > On Wed, Mar 15, 2017 at 11:08:56AM +0100, Maarten Lankhorst wrote:
> >> As a proof of concept, first try to convert intel_tv, which is a rarely
> >> used connector. It has 5 properties, tv format and 4 margins.
> >>
> >> I'm less certain about the state behavior itself, should we pass a size
> >> parameter to intel_connector_alloc instead, so duplicate_state
> >> can be done globally if it can be blindly copied?
> >>
> >> Can we also have a atomic_check function for connectors, so the
> >> crtc_state->connectors_changed can be set there? It would be cleaner
> >> and more atomic-like.
> >>
> >> To match the legacy behavior, format can be changed by probing just like
> >> in legacy mode.
> >>
> >> Changes since v1:
> >> - Add intel_encoder->swap_state to allow updating connector state.
> >> - Add intel_tv->format for detect_mode and mode_valid, updated on atomic commit.
> >> Changes since v2:
> >> - Fix typo in tv_choose_preferred modes function name.
> >> - Assignment of tv properties is done in core, so intel_tv only needs
> >>   a atomic_check function. Thanks Ville!
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c |  15 +++
> >>  drivers/gpu/drm/i915/intel_drv.h     |   4 +
> >>  drivers/gpu/drm/i915/intel_tv.c      | 200 ++++++++++++++++-------------------
> >>  3 files changed, 110 insertions(+), 109 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index ac25c9bc8b81..18b7e7546ee1 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -12954,6 +12954,20 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
> >>  				  to_intel_plane(plane)->frontbuffer_bit);
> >>  }
> >>  
> >> +static void swap_connector_state(struct drm_atomic_state *state)
> >> +{
> >> +	struct drm_connector *connector;
> >> +	struct drm_connector_state *new_conn_state;
> >> +	int i;
> >> +
> >> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> >> +		struct intel_connector *conn = to_intel_connector(connector);
> >> +
> >> +		if (conn->swap_state)
> >> +			conn->swap_state(conn, new_conn_state);
> >> +	}
> >> +}
> > So yeah I'm late, but this is pretty much exactly what DK's private state
> > object stuff was meant for. Have you looked into rebasing on top of that?
> > I think hand-rolling private state stuff everywhere isn't really good.
> >
> > Afaics DK's patch series is ready for merging into drm-misc, so shouldn't
> > end up blocking you.
> This swap_state hook is used for connector state. We could probably add a swap_state hook that drivers
> can use for this in the core, but the connector state is already part of the state, adding it twice is silly.
> 
> Instead, would adding a hook to the core be useful?

Why do you need a hook to swap the state? The core already does that for
you ... to_tv_state(intel_tv->base.base->state) is probably what you want,
instead of deref'ing a new intel_tv->format.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3.

[Maarten Lankhorst]

Op 16-03-17 om 15:56 schreef Daniel Vetter:
> On Thu, Mar 16, 2017 at 01:05:06PM +0100, Maarten Lankhorst wrote:
>> Op 16-03-17 om 11:48 schreef Daniel Vetter:
>>> On Wed, Mar 15, 2017 at 11:08:56AM +0100, Maarten Lankhorst wrote:
>>>> As a proof of concept, first try to convert intel_tv, which is a rarely
>>>> used connector. It has 5 properties, tv format and 4 margins.
>>>>
>>>> I'm less certain about the state behavior itself, should we pass a size
>>>> parameter to intel_connector_alloc instead, so duplicate_state
>>>> can be done globally if it can be blindly copied?
>>>>
>>>> Can we also have a atomic_check function for connectors, so the
>>>> crtc_state->connectors_changed can be set there? It would be cleaner
>>>> and more atomic-like.
>>>>
>>>> To match the legacy behavior, format can be changed by probing just like
>>>> in legacy mode.
>>>>
>>>> Changes since v1:
>>>> - Add intel_encoder->swap_state to allow updating connector state.
>>>> - Add intel_tv->format for detect_mode and mode_valid, updated on atomic commit.
>>>> Changes since v2:
>>>> - Fix typo in tv_choose_preferred modes function name.
>>>> - Assignment of tv properties is done in core, so intel_tv only needs
>>>>   a atomic_check function. Thanks Ville!
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_display.c |  15 +++
>>>>  drivers/gpu/drm/i915/intel_drv.h     |   4 +
>>>>  drivers/gpu/drm/i915/intel_tv.c      | 200 ++++++++++++++++-------------------
>>>>  3 files changed, 110 insertions(+), 109 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index ac25c9bc8b81..18b7e7546ee1 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -12954,6 +12954,20 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
>>>>  				  to_intel_plane(plane)->frontbuffer_bit);
>>>>  }
>>>>  
>>>> +static void swap_connector_state(struct drm_atomic_state *state)
>>>> +{
>>>> +	struct drm_connector *connector;
>>>> +	struct drm_connector_state *new_conn_state;
>>>> +	int i;
>>>> +
>>>> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
>>>> +		struct intel_connector *conn = to_intel_connector(connector);
>>>> +
>>>> +		if (conn->swap_state)
>>>> +			conn->swap_state(conn, new_conn_state);
>>>> +	}
>>>> +}
>>> So yeah I'm late, but this is pretty much exactly what DK's private state
>>> object stuff was meant for. Have you looked into rebasing on top of that?
>>> I think hand-rolling private state stuff everywhere isn't really good.
>>>
>>> Afaics DK's patch series is ready for merging into drm-misc, so shouldn't
>>> end up blocking you.
>> This swap_state hook is used for connector state. We could probably add a swap_state hook that drivers
>> can use for this in the core, but the connector state is already part of the state, adding it twice is silly.
>>
>> Instead, would adding a hook to the core be useful?
> Why do you need a hook to swap the state? The core already does that for
> you ... to_tv_state(intel_tv->base.base->state) is probably what you want,
> instead of deref'ing a new intel_tv->format.
> -Daniel

Well in some way I need to access connector_state->tv.mode in mode_valid, which doesn't hold any connector state locks.
I think setting it from the state is the easiest way to do so. But I'm open to other approaches. :)

~Maarten

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

Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3.

[Daniel Vetter]

On Thu, Mar 16, 2017 at 04:27:42PM +0100, Maarten Lankhorst wrote:
> Op 16-03-17 om 15:56 schreef Daniel Vetter:
> > On Thu, Mar 16, 2017 at 01:05:06PM +0100, Maarten Lankhorst wrote:
> >> Op 16-03-17 om 11:48 schreef Daniel Vetter:
> >>> On Wed, Mar 15, 2017 at 11:08:56AM +0100, Maarten Lankhorst wrote:
> >>>> As a proof of concept, first try to convert intel_tv, which is a rarely
> >>>> used connector. It has 5 properties, tv format and 4 margins.
> >>>>
> >>>> I'm less certain about the state behavior itself, should we pass a size
> >>>> parameter to intel_connector_alloc instead, so duplicate_state
> >>>> can be done globally if it can be blindly copied?
> >>>>
> >>>> Can we also have a atomic_check function for connectors, so the
> >>>> crtc_state->connectors_changed can be set there? It would be cleaner
> >>>> and more atomic-like.
> >>>>
> >>>> To match the legacy behavior, format can be changed by probing just like
> >>>> in legacy mode.
> >>>>
> >>>> Changes since v1:
> >>>> - Add intel_encoder->swap_state to allow updating connector state.
> >>>> - Add intel_tv->format for detect_mode and mode_valid, updated on atomic commit.
> >>>> Changes since v2:
> >>>> - Fix typo in tv_choose_preferred modes function name.
> >>>> - Assignment of tv properties is done in core, so intel_tv only needs
> >>>>   a atomic_check function. Thanks Ville!
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/intel_display.c |  15 +++
> >>>>  drivers/gpu/drm/i915/intel_drv.h     |   4 +
> >>>>  drivers/gpu/drm/i915/intel_tv.c      | 200 ++++++++++++++++-------------------
> >>>>  3 files changed, 110 insertions(+), 109 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>> index ac25c9bc8b81..18b7e7546ee1 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>> @@ -12954,6 +12954,20 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
> >>>>  				  to_intel_plane(plane)->frontbuffer_bit);
> >>>>  }
> >>>>  
> >>>> +static void swap_connector_state(struct drm_atomic_state *state)
> >>>> +{
> >>>> +	struct drm_connector *connector;
> >>>> +	struct drm_connector_state *new_conn_state;
> >>>> +	int i;
> >>>> +
> >>>> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> >>>> +		struct intel_connector *conn = to_intel_connector(connector);
> >>>> +
> >>>> +		if (conn->swap_state)
> >>>> +			conn->swap_state(conn, new_conn_state);
> >>>> +	}
> >>>> +}
> >>> So yeah I'm late, but this is pretty much exactly what DK's private state
> >>> object stuff was meant for. Have you looked into rebasing on top of that?
> >>> I think hand-rolling private state stuff everywhere isn't really good.
> >>>
> >>> Afaics DK's patch series is ready for merging into drm-misc, so shouldn't
> >>> end up blocking you.
> >> This swap_state hook is used for connector state. We could probably add a swap_state hook that drivers
> >> can use for this in the core, but the connector state is already part of the state, adding it twice is silly.
> >>
> >> Instead, would adding a hook to the core be useful?
> > Why do you need a hook to swap the state? The core already does that for
> > you ... to_tv_state(intel_tv->base.base->state) is probably what you want,
> > instead of deref'ing a new intel_tv->format.
> > -Daniel
> 
> Well in some way I need to access connector_state->tv.mode in mode_valid, which doesn't hold any connector state locks.
> I think setting it from the state is the easiest way to do so. But I'm open to other approaches. :)

Well fundamentally that access is going to be racy, so you better put a
READ_ONCE into mode_valid. But once we accept that it's racy, we might as
well chase connector->state, the only thing we need is to rcu-protected
the freeing of old atomic state.

I've typed such a patch yesterday (for vblank access to crtc->state from
vblank interrupts), but then thrown it away again. Because it didn't fix
my vblank issue correctly.

I'll resurrect and send out.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx