public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH 1/5] drm/modes: add connector reference counting. (v2)
@ 2016-04-27  2:03 Dave Airlie
  2016-04-27  2:03 ` [PATCH 2/5] drm/fb_helper: add connector reference counting Dave Airlie
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Dave Airlie @ 2016-04-27  2:03 UTC (permalink / raw)
  To: dri-devel, intel-gfx

From: Dave Airlie <airlied@redhat.com>

This uses the previous changes to add reference counts
to drm connector objects.

v2: move fbdev changes to their own patch.
add some kerneldoc
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++++++--
 drivers/gpu/drm/drm_crtc.c   | 28 ++++++++++++++++++++++++----
 include/drm/drm_crtc.h       | 32 +++++++++++++++++++++++++++++++-
 3 files changed, 72 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 8ee1db8..9d5e3c8 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -154,6 +154,7 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 						       state->connector_states[i]);
 		state->connectors[i] = NULL;
 		state->connector_states[i] = NULL;
+		drm_connector_unreference(connector);
 	}
 
 	for (i = 0; i < config->num_crtc; i++) {
@@ -924,6 +925,7 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
 	if (!connector_state)
 		return ERR_PTR(-ENOMEM);
 
+	drm_connector_reference(connector);
 	state->connector_states[index] = connector_state;
 	state->connectors[index] = connector;
 	connector_state->state = state;
@@ -1614,12 +1616,19 @@ retry:
 		}
 
 		obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
-		if (!obj || !obj->properties) {
+		if (!obj) {
+			ret = -ENOENT;
+			goto out;
+		}
+
+		if (!obj->properties) {
+			drm_mode_object_unreference(obj);
 			ret = -ENOENT;
 			goto out;
 		}
 
 		if (get_user(count_props, count_props_ptr + copied_objs)) {
+			drm_mode_object_unreference(obj);
 			ret = -EFAULT;
 			goto out;
 		}
@@ -1632,12 +1641,14 @@ retry:
 			struct drm_property *prop;
 
 			if (get_user(prop_id, props_ptr + copied_props)) {
+				drm_mode_object_unreference(obj);
 				ret = -EFAULT;
 				goto out;
 			}
 
 			prop = drm_property_find(dev, prop_id);
 			if (!prop) {
+				drm_mode_object_unreference(obj);
 				ret = -ENOENT;
 				goto out;
 			}
@@ -1645,13 +1656,16 @@ retry:
 			if (copy_from_user(&prop_value,
 					   prop_values_ptr + copied_props,
 					   sizeof(prop_value))) {
+				drm_mode_object_unreference(obj);
 				ret = -EFAULT;
 				goto out;
 			}
 
 			ret = atomic_set_prop(state, obj, prop, prop_value);
-			if (ret)
+			if (ret) {
+				drm_mode_object_unreference(obj);
 				goto out;
+			}
 
 			copied_props++;
 		}
@@ -1662,6 +1676,7 @@ retry:
 			plane_mask |= (1 << drm_plane_index(plane));
 			plane->old_fb = plane->fb;
 		}
+		drm_mode_object_unreference(obj);
 	}
 
 	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 4e5b015..a78e202 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -862,6 +862,16 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
 		      mode->interlace ?  " interlaced" : "");
 }
 
+static void drm_connector_free(struct kref *kref)
+{
+	struct drm_connector *connector =
+		container_of(kref, struct drm_connector, base.refcount);
+	struct drm_device *dev = connector->dev;
+
+	drm_mode_object_unregister(dev, &connector->base);
+	connector->funcs->destroy(connector);
+}
+
 /**
  * drm_connector_init - Init a preallocated connector
  * @dev: DRM device
@@ -887,7 +897,9 @@ int drm_connector_init(struct drm_device *dev,
 
 	drm_modeset_lock_all(dev);
 
-	ret = drm_mode_object_get_reg(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR, false, NULL);
+	ret = drm_mode_object_get_reg(dev, &connector->base,
+				      DRM_MODE_OBJECT_CONNECTOR,
+				      false, drm_connector_free);
 	if (ret)
 		goto out_unlock;
 
@@ -2147,7 +2159,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 
 	mutex_lock(&dev->mode_config.mutex);
 
-	connector = drm_connector_find(dev, out_resp->connector_id);
+	connector = drm_connector_lookup(dev, out_resp->connector_id);
 	if (!connector) {
 		ret = -ENOENT;
 		goto out_unlock;
@@ -2231,6 +2243,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 out:
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 
+	drm_connector_unreference(connector);
 out_unlock:
 	mutex_unlock(&dev->mode_config.mutex);
 
@@ -2875,13 +2888,14 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 		}
 
 		for (i = 0; i < crtc_req->count_connectors; i++) {
+			connector_set[i] = NULL;
 			set_connectors_ptr = (uint32_t __user *)(unsigned long)crtc_req->set_connectors_ptr;
 			if (get_user(out_id, &set_connectors_ptr[i])) {
 				ret = -EFAULT;
 				goto out;
 			}
 
-			connector = drm_connector_find(dev, out_id);
+			connector = drm_connector_lookup(dev, out_id);
 			if (!connector) {
 				DRM_DEBUG_KMS("Connector id %d unknown\n",
 						out_id);
@@ -2909,6 +2923,12 @@ out:
 	if (fb)
 		drm_framebuffer_unreference(fb);
 
+	if (connector_set) {
+		for (i = 0; i < crtc_req->count_connectors; i++) {
+			if (connector_set[i])
+				drm_connector_unreference(connector_set[i]);
+		}
+	}
 	kfree(connector_set);
 	drm_mode_destroy(dev, mode);
 	drm_modeset_unlock_all(dev);
@@ -4999,7 +5019,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 	property = obj_to_property(prop_obj);
 
 	if (!drm_property_change_valid_get(property, arg->value, &ref))
-		goto out;
+		goto out_unref;
 
 	switch (arg_obj->type) {
 	case DRM_MODE_OBJECT_CONNECTOR:
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 297e527..16a98d4 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -2571,7 +2571,15 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
 	return mo ? obj_to_encoder(mo) : NULL;
 }
 
-static inline struct drm_connector *drm_connector_find(struct drm_device *dev,
+/*
+ * drm_connector_lookup - lookup connector object
+ * @dev: DRM device
+ * @id: connector object id
+ *
+ * This function looks up the connector object specified by id
+ * add takes a reference to it.
+ */
+static inline struct drm_connector *drm_connector_lookup(struct drm_device *dev,
 		uint32_t id)
 {
 	struct drm_mode_object *mo;
@@ -2639,6 +2647,28 @@ static inline uint32_t drm_framebuffer_read_refcount(struct drm_framebuffer *fb)
 	return atomic_read(&fb->base.refcount.refcount);
 }
 
+/*
+ * drm_connector_reference - incr the connector refcnt
+ * @connector: connector
+ *
+ * This function increments the connector's refcount.
+ */
+static inline void drm_connector_reference(struct drm_connector *connector)
+{
+	drm_mode_object_reference(&connector->base);
+}
+
+/**
+ * drm_connector_unreference - unref a connector
+ * @connector: connector to unref
+ *
+ * This function decrements the connector's refcount and frees it if it drops to zero.
+ */
+static inline void drm_connector_unreference(struct drm_connector *connector)
+{
+	drm_mode_object_unreference(&connector->base);
+}
+
 /* Plane list iterator for legacy (overlay only) planes. */
 #define drm_for_each_legacy_plane(plane, dev) \
 	list_for_each_entry(plane, &(dev)->mode_config.plane_list, head) \
-- 
2.5.5

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/5] drm/fb_helper: add connector reference counting.
  2016-04-27  2:03 [PATCH 1/5] drm/modes: add connector reference counting. (v2) Dave Airlie
@ 2016-04-27  2:03 ` Dave Airlie
  2016-04-27  7:33   ` [Intel-gfx] " Daniel Vetter
  2016-04-27  2:03 ` [PATCH 3/5] drm/crtc: take references to connectors used in a modeset Dave Airlie
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Dave Airlie @ 2016-04-27  2:03 UTC (permalink / raw)
  To: dri-devel, intel-gfx

From: Dave Airlie <airlied@redhat.com>

This takes a reference count when fbdev adds the connector,
and drops it when it removes the connector.

It also drops the now unneeded code to find connectors
and remove the from the modeset as they are reference counted.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 34 ++--------------------------------
 1 file changed, 2 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 855108e..b2f58fb 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -153,40 +153,13 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
 	if (!fb_helper_connector)
 		return -ENOMEM;
 
+	drm_connector_reference(connector);
 	fb_helper_connector->connector = connector;
 	fb_helper->connector_info[fb_helper->connector_count++] = fb_helper_connector;
 	return 0;
 }
 EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
 
-static void remove_from_modeset(struct drm_mode_set *set,
-		struct drm_connector *connector)
-{
-	int i, j;
-
-	for (i = 0; i < set->num_connectors; i++) {
-		if (set->connectors[i] == connector)
-			break;
-	}
-
-	if (i == set->num_connectors)
-		return;
-
-	for (j = i + 1; j < set->num_connectors; j++) {
-		set->connectors[j - 1] = set->connectors[j];
-	}
-	set->num_connectors--;
-
-	/*
-	 * TODO maybe need to makes sure we set it back to !=NULL somewhere?
-	 */
-	if (set->num_connectors == 0) {
-		set->fb = NULL;
-		drm_mode_destroy(connector->dev, set->mode);
-		set->mode = NULL;
-	}
-}
-
 int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 				       struct drm_connector *connector)
 {
@@ -206,6 +179,7 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 	if (i == fb_helper->connector_count)
 		return -EINVAL;
 	fb_helper_connector = fb_helper->connector_info[i];
+	drm_connector_unreference(fb_helper_connector->connector);
 
 	for (j = i + 1; j < fb_helper->connector_count; j++) {
 		fb_helper->connector_info[j - 1] = fb_helper->connector_info[j];
@@ -213,10 +187,6 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 	fb_helper->connector_count--;
 	kfree(fb_helper_connector);
 
-	/* also cleanup dangling references to the connector: */
-	for (i = 0; i < fb_helper->crtc_count; i++)
-		remove_from_modeset(&fb_helper->crtc_info[i].mode_set, connector);
-
 	return 0;
 }
 EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);
-- 
2.5.5

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/5] drm/crtc: take references to connectors used in a modeset.
  2016-04-27  2:03 [PATCH 1/5] drm/modes: add connector reference counting. (v2) Dave Airlie
  2016-04-27  2:03 ` [PATCH 2/5] drm/fb_helper: add connector reference counting Dave Airlie
@ 2016-04-27  2:03 ` Dave Airlie
  2016-04-27  7:07   ` Daniel Stone
  2016-04-27  7:34   ` [Intel-gfx] " Daniel Vetter
  2016-04-27  2:03 ` [PATCH 4/5] drm/atomic: use connector references Dave Airlie
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 14+ messages in thread
From: Dave Airlie @ 2016-04-27  2:03 UTC (permalink / raw)
  To: dri-devel, intel-gfx

From: Dave Airlie <airlied@redhat.com>

This just takes a reference on the connector when we set a mode
in the non-atomic paths.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_crtc_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 66ca313..29b7835 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -456,6 +456,9 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
 			 * between them is henceforth no longer available.
 			 */
 			connector->dpms = DRM_MODE_DPMS_OFF;
+
+			/* we keep a reference while the encoder is bound */
+			drm_connector_unreference(connector);
 		}
 	}
 
@@ -635,9 +638,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 			mode_changed = true;
 			/* If the encoder is reused for another connector, then
 			 * the appropriate crtc will be set later.
+			 * take a reference only if we haven't had an encoder before.
 			 */
 			if (connector->encoder)
 				connector->encoder->crtc = NULL;
+			else
+				drm_connector_reference(connector);
 			connector->encoder = new_encoder;
 		}
 	}
-- 
2.5.5

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/5] drm/atomic: use connector references
  2016-04-27  2:03 [PATCH 1/5] drm/modes: add connector reference counting. (v2) Dave Airlie
  2016-04-27  2:03 ` [PATCH 2/5] drm/fb_helper: add connector reference counting Dave Airlie
  2016-04-27  2:03 ` [PATCH 3/5] drm/crtc: take references to connectors used in a modeset Dave Airlie
@ 2016-04-27  2:03 ` Dave Airlie
  2016-04-27  7:13   ` Daniel Stone
  2016-04-27  7:22   ` Daniel Stone
  2016-04-27  2:03 ` [PATCH 5/5] drm/i915/mst: use reference counted connectors. (v2) Dave Airlie
  2016-04-27  7:28 ` [Intel-gfx] [PATCH 1/5] drm/modes: add connector reference counting. (v2) Daniel Vetter
  4 siblings, 2 replies; 14+ messages in thread
From: Dave Airlie @ 2016-04-27  2:03 UTC (permalink / raw)
  To: dri-devel, intel-gfx

From: Dave Airlie <airlied@redhat.com>

Take a reference when setting a crtc on a connecter,
also take one when duplicating if a crtc is set,
and drop one on destroy if a crtc is set.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_atomic.c        | 11 ++++++++++-
 drivers/gpu/drm/drm_atomic_helper.c |  4 ++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 9d5e3c8..d899dac 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1159,7 +1159,7 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 				  struct drm_crtc *crtc)
 {
 	struct drm_crtc_state *crtc_state;
-
+	bool had_crtc = conn_state->crtc ? true : false;
 	if (conn_state->crtc && conn_state->crtc != crtc) {
 		crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state,
 								conn_state->crtc);
@@ -1179,6 +1179,15 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 
 	conn_state->crtc = crtc;
 
+	/* If we had no crtc then got one, add a reference,
+	 * if we had a crtc and are going to none, drop a reference,
+	 * otherwise just keep the reference we have.
+	 */
+	if (!had_crtc && crtc)
+		drm_connector_reference(conn_state->connector);
+	else if (!crtc && had_crtc)
+		 drm_connector_unreference(conn_state->connector);
+
 	if (crtc)
 		DRM_DEBUG_ATOMIC("Link connector state %p to [CRTC:%d:%s]\n",
 				 conn_state, crtc->base.id, crtc->name);
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d25abce..a29deac 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2762,6 +2762,8 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
 					    struct drm_connector_state *state)
 {
 	memcpy(state, connector->state, sizeof(*state));
+	if (state->crtc)
+		drm_connector_reference(connector);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
 
@@ -2889,6 +2891,8 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
 	 * state will automatically do the right thing if code is ever added
 	 * to this function.
 	 */
+	if (state->crtc)
+		drm_connector_unreference(connector);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
 
-- 
2.5.5

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/5] drm/i915/mst: use reference counted connectors. (v2)
  2016-04-27  2:03 [PATCH 1/5] drm/modes: add connector reference counting. (v2) Dave Airlie
                   ` (2 preceding siblings ...)
  2016-04-27  2:03 ` [PATCH 4/5] drm/atomic: use connector references Dave Airlie
@ 2016-04-27  2:03 ` Dave Airlie
  2016-04-27  7:52   ` Daniel Vetter
  2016-04-27  7:28 ` [Intel-gfx] [PATCH 1/5] drm/modes: add connector reference counting. (v2) Daniel Vetter
  4 siblings, 1 reply; 14+ messages in thread
From: Dave Airlie @ 2016-04-27  2:03 UTC (permalink / raw)
  To: dri-devel, intel-gfx

From: Dave Airlie <airlied@redhat.com>

Don't just free the connector when we get the destroy callback.

Drop a reference to it, and set it's mst_port to NULL so
no more mst work is done on it.

v2: core mst accepts NULLs fine. Cleanup EDID code properly.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 45 ++++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_drv.h    |  2 +-
 2 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 94b4e83..4330b21 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -109,7 +109,7 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder)
 
 	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
 
-	drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, intel_mst->port);
+	drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, intel_mst->connector->port);
 
 	ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);
 	if (ret) {
@@ -134,10 +134,12 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder)
 	/* and this can also fail */
 	drm_dp_update_payload_part2(&intel_dp->mst_mgr);
 
-	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, intel_mst->port);
+	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, intel_mst->connector->port);
 
 	intel_dp->active_mst_links--;
-	intel_mst->port = NULL;
+
+	drm_connector_unreference(&intel_mst->connector->base);
+	intel_mst->connector = NULL;
 	if (intel_dp->active_mst_links == 0) {
 		intel_dig_port->base.post_disable(&intel_dig_port->base);
 		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
@@ -177,7 +179,9 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
 	found->encoder = encoder;
 
 	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
-	intel_mst->port = found->port;
+
+	intel_mst->connector = found;
+	drm_connector_reference(&intel_mst->connector->base);
 
 	if (intel_dp->active_mst_links == 0) {
 		intel_prepare_ddi_buffer(&intel_dig_port->base);
@@ -195,7 +199,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
 	}
 
 	ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
-				       intel_mst->port,
+				       intel_mst->connector->port,
 				       intel_crtc->config->pbn, &slots);
 	if (ret == false) {
 		DRM_ERROR("failed to allocate vcpi\n");
@@ -244,7 +248,7 @@ static bool intel_dp_mst_enc_get_hw_state(struct intel_encoder *encoder,
 {
 	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
 	*pipe = intel_mst->pipe;
-	if (intel_mst->port)
+	if (intel_mst->connector)
 		return true;
 	return false;
 }
@@ -308,10 +312,11 @@ static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector)
 	struct edid *edid;
 	int ret;
 
-	edid = drm_dp_mst_get_edid(connector, &intel_dp->mst_mgr, intel_connector->port);
-	if (!edid)
-		return 0;
+	if (!intel_dp) {
+		return intel_connector_update_modes(connector, NULL);
+	}
 
+	edid = drm_dp_mst_get_edid(connector, &intel_dp->mst_mgr, intel_connector->port);
 	ret = intel_connector_update_modes(connector, edid);
 	kfree(edid);
 
@@ -324,6 +329,8 @@ intel_dp_mst_detect(struct drm_connector *connector, bool force)
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct intel_dp *intel_dp = intel_connector->mst_port;
 
+	if (!intel_dp)
+		return connector_status_disconnected;
 	return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr, intel_connector->port);
 }
 
@@ -389,6 +396,8 @@ static struct drm_encoder *intel_mst_atomic_best_encoder(struct drm_connector *c
 	struct intel_dp *intel_dp = intel_connector->mst_port;
 	struct intel_crtc *crtc = to_intel_crtc(state->crtc);
 
+	if (!intel_dp)
+		return NULL;
 	return &intel_dp->mst_encoders[crtc->pipe]->base.base;
 }
 
@@ -396,6 +405,8 @@ static struct drm_encoder *intel_mst_best_encoder(struct drm_connector *connecto
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct intel_dp *intel_dp = intel_connector->mst_port;
+	if (!intel_dp)
+		return NULL;
 	return &intel_dp->mst_encoders[0]->base.base;
 }
 
@@ -506,23 +517,11 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 
 	/* need to nuke the connector */
 	drm_modeset_lock_all(dev);
-	if (connector->state->crtc) {
-		struct drm_mode_set set;
-		int ret;
-
-		memset(&set, 0, sizeof(set));
-		set.crtc = connector->state->crtc,
-
-		ret = drm_atomic_helper_set_config(&set);
-
-		WARN(ret, "Disabling mst crtc failed with %i\n", ret);
-	}
-
 	intel_connector_remove_from_fbdev(intel_connector);
-	drm_connector_cleanup(connector);
+	intel_connector->mst_port = NULL;
 	drm_modeset_unlock_all(dev);
 
-	kfree(intel_connector);
+	drm_connector_unreference(&intel_connector->base);
 	DRM_DEBUG_KMS("\n");
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e0fcfa1..288135de 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -878,7 +878,7 @@ struct intel_dp_mst_encoder {
 	struct intel_encoder base;
 	enum pipe pipe;
 	struct intel_digital_port *primary;
-	void *port; /* store this opaque as its illegal to dereference it */
+	struct intel_connector *connector;
 };
 
 static inline enum dpio_channel
-- 
2.5.5

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/5] drm/crtc: take references to connectors used in a modeset.
  2016-04-27  2:03 ` [PATCH 3/5] drm/crtc: take references to connectors used in a modeset Dave Airlie
@ 2016-04-27  7:07   ` Daniel Stone
  2016-04-27  7:34   ` [Intel-gfx] " Daniel Vetter
  1 sibling, 0 replies; 14+ messages in thread
From: Daniel Stone @ 2016-04-27  7:07 UTC (permalink / raw)
  To: Dave Airlie; +Cc: intel-gfx, dri-devel

Hi,

On 27 April 2016 at 03:03, Dave Airlie <airlied@gmail.com> wrote:
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 66ca313..29b7835 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -456,6 +456,9 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
>                          * between them is henceforth no longer available.
>                          */
>                         connector->dpms = DRM_MODE_DPMS_OFF;
> +
> +                       /* we keep a reference while the encoder is bound */
> +                       drm_connector_unreference(connector);
>                 }
>         }
>
> @@ -635,9 +638,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
>                         mode_changed = true;
>                         /* If the encoder is reused for another connector, then
>                          * the appropriate crtc will be set later.
> +                        * take a reference only if we haven't had an encoder before.
>                          */
>                         if (connector->encoder)
>                                 connector->encoder->crtc = NULL;
> +                       else
> +                               drm_connector_reference(connector);
>                         connector->encoder = new_encoder;
>                 }
>         }

This new ref leaks in the if (fail) goto fail path below, and I can't
quite convince myself that it's correct in the case where they share
an encoder. How about this:
  - unconditionally ref every connector in set->connectors before
doing anything which can fail
  - on successful exit, unconditionally unref every connector in save_connectors
  - in the fail label, unconditionally unref every connector in set->connectors

Not quite as efficient as trying to do the only-ref-on-change dance,
but much easier to prove correct, as well as matching the atomic state
approach, if you imagine save_connectors as the current state, and
set->connectors as the new/req state. Plus, refcounting really isn't
the expensive part of this operation. ;)

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/5] drm/atomic: use connector references
  2016-04-27  2:03 ` [PATCH 4/5] drm/atomic: use connector references Dave Airlie
@ 2016-04-27  7:13   ` Daniel Stone
  2016-04-27  7:47     ` [Intel-gfx] " Daniel Vetter
  2016-04-27  7:22   ` Daniel Stone
  1 sibling, 1 reply; 14+ messages in thread
From: Daniel Stone @ 2016-04-27  7:13 UTC (permalink / raw)
  To: Dave Airlie; +Cc: intel-gfx, dri-devel

Hi,

On 27 April 2016 at 03:03, Dave Airlie <airlied@gmail.com> wrote:
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9d5e3c8..d899dac 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1179,6 +1179,15 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>
>         conn_state->crtc = crtc;
>
> +       /* If we had no crtc then got one, add a reference,
> +        * if we had a crtc and are going to none, drop a reference,
> +        * otherwise just keep the reference we have.
> +        */
> +       if (!had_crtc && crtc)
> +               drm_connector_reference(conn_state->connector);
> +       else if (!crtc && had_crtc)
> +                drm_connector_unreference(conn_state->connector);

Similarly to the non-atomic comments, can you drop the had_crtc test
here, and always ref if crtc, and unref if !crtc?

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/5] drm/atomic: use connector references
  2016-04-27  2:03 ` [PATCH 4/5] drm/atomic: use connector references Dave Airlie
  2016-04-27  7:13   ` Daniel Stone
@ 2016-04-27  7:22   ` Daniel Stone
  1 sibling, 0 replies; 14+ messages in thread
From: Daniel Stone @ 2016-04-27  7:22 UTC (permalink / raw)
  To: Dave Airlie; +Cc: intel-gfx, dri-devel

Hi,

On 27 April 2016 at 03:03, Dave Airlie <airlied@gmail.com> wrote:
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9d5e3c8..d899dac 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1179,6 +1179,15 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>
>         conn_state->crtc = crtc;
>
> +       /* If we had no crtc then got one, add a reference,
> +        * if we had a crtc and are going to none, drop a reference,
> +        * otherwise just keep the reference we have.
> +        */
> +       if (!had_crtc && crtc)
> +               drm_connector_reference(conn_state->connector);
> +       else if (!crtc && had_crtc)
> +                drm_connector_unreference(conn_state->connector);

Similarly to the non-atomic comments, can you drop the had_crtc test
here, and always ref if crtc, and unref if !crtc?

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/modes: add connector reference counting. (v2)
  2016-04-27  2:03 [PATCH 1/5] drm/modes: add connector reference counting. (v2) Dave Airlie
                   ` (3 preceding siblings ...)
  2016-04-27  2:03 ` [PATCH 5/5] drm/i915/mst: use reference counted connectors. (v2) Dave Airlie
@ 2016-04-27  7:28 ` Daniel Vetter
  4 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2016-04-27  7:28 UTC (permalink / raw)
  To: Dave Airlie; +Cc: intel-gfx, dri-devel

On Wed, Apr 27, 2016 at 12:03:25PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This uses the previous changes to add reference counts
> to drm connector objects.
> 
> v2: move fbdev changes to their own patch.
> add some kerneldoc
> Signed-off-by: Dave Airlie <airlied@redhat.com>

[cut]

> @@ -2639,6 +2647,28 @@ static inline uint32_t drm_framebuffer_read_refcount(struct drm_framebuffer *fb)
>  	return atomic_read(&fb->base.refcount.refcount);
>  }
>  
> +/*

/**

otherwise kerneldoc doesn't pick it up. With that fixed

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> + * drm_connector_reference - incr the connector refcnt
> + * @connector: connector
> + *
> + * This function increments the connector's refcount.
> + */
> +static inline void drm_connector_reference(struct drm_connector *connector)
> +{
> +	drm_mode_object_reference(&connector->base);
> +}
> +
> +/**
> + * drm_connector_unreference - unref a connector
> + * @connector: connector to unref
> + *
> + * This function decrements the connector's refcount and frees it if it drops to zero.
> + */
> +static inline void drm_connector_unreference(struct drm_connector *connector)
> +{
> +	drm_mode_object_unreference(&connector->base);
> +}
> +
>  /* Plane list iterator for legacy (overlay only) planes. */
>  #define drm_for_each_legacy_plane(plane, dev) \
>  	list_for_each_entry(plane, &(dev)->mode_config.plane_list, head) \
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH 2/5] drm/fb_helper: add connector reference counting.
  2016-04-27  2:03 ` [PATCH 2/5] drm/fb_helper: add connector reference counting Dave Airlie
@ 2016-04-27  7:33   ` Daniel Vetter
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2016-04-27  7:33 UTC (permalink / raw)
  To: Dave Airlie; +Cc: intel-gfx, dri-devel

On Wed, Apr 27, 2016 at 12:03:26PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This takes a reference count when fbdev adds the connector,
> and drops it when it removes the connector.
> 
> It also drops the now unneeded code to find connectors
> and remove the from the modeset as they are reference counted.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Ok, I screwed up part of my first review, and add_all_connectors calls
add_one_connector. That looks like we'll leak non-mst connectors when
unloading i915. I think we need to add a loop to drm_fb_helper_fini and
call drm_fb_helper_remove_one_connector on all the connectors still in the
helper list. I think otherwise it looks good.

Would be really good to give this is a spin with full debugging enabled
when reloading i915.ko. Just to make sure I didn't miss anything. Or cc
intel-gfx and CI should be able to pick it up and give it a spin.
-Daniel

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 34 ++--------------------------------
>  1 file changed, 2 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 855108e..b2f58fb 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -153,40 +153,13 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
>  	if (!fb_helper_connector)
>  		return -ENOMEM;
>  
> +	drm_connector_reference(connector);
>  	fb_helper_connector->connector = connector;
>  	fb_helper->connector_info[fb_helper->connector_count++] = fb_helper_connector;
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
>  
> -static void remove_from_modeset(struct drm_mode_set *set,
> -		struct drm_connector *connector)
> -{
> -	int i, j;
> -
> -	for (i = 0; i < set->num_connectors; i++) {
> -		if (set->connectors[i] == connector)
> -			break;
> -	}
> -
> -	if (i == set->num_connectors)
> -		return;
> -
> -	for (j = i + 1; j < set->num_connectors; j++) {
> -		set->connectors[j - 1] = set->connectors[j];
> -	}
> -	set->num_connectors--;
> -
> -	/*
> -	 * TODO maybe need to makes sure we set it back to !=NULL somewhere?
> -	 */
> -	if (set->num_connectors == 0) {
> -		set->fb = NULL;
> -		drm_mode_destroy(connector->dev, set->mode);
> -		set->mode = NULL;
> -	}
> -}
> -
>  int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  				       struct drm_connector *connector)
>  {
> @@ -206,6 +179,7 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  	if (i == fb_helper->connector_count)
>  		return -EINVAL;
>  	fb_helper_connector = fb_helper->connector_info[i];
> +	drm_connector_unreference(fb_helper_connector->connector);
>  
>  	for (j = i + 1; j < fb_helper->connector_count; j++) {
>  		fb_helper->connector_info[j - 1] = fb_helper->connector_info[j];
> @@ -213,10 +187,6 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  	fb_helper->connector_count--;
>  	kfree(fb_helper_connector);
>  
> -	/* also cleanup dangling references to the connector: */
> -	for (i = 0; i < fb_helper->crtc_count; i++)
> -		remove_from_modeset(&fb_helper->crtc_info[i].mode_set, connector);
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH 3/5] drm/crtc: take references to connectors used in a modeset.
  2016-04-27  2:03 ` [PATCH 3/5] drm/crtc: take references to connectors used in a modeset Dave Airlie
  2016-04-27  7:07   ` Daniel Stone
@ 2016-04-27  7:34   ` Daniel Vetter
  2016-04-27  7:48     ` Daniel Vetter
  1 sibling, 1 reply; 14+ messages in thread
From: Daniel Vetter @ 2016-04-27  7:34 UTC (permalink / raw)
  To: Dave Airlie; +Cc: intel-gfx, dri-devel

On Wed, Apr 27, 2016 at 12:03:27PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This just takes a reference on the connector when we set a mode
> in the non-atomic paths.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_crtc_helper.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 66ca313..29b7835 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -456,6 +456,9 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
>  			 * between them is henceforth no longer available.
>  			 */
>  			connector->dpms = DRM_MODE_DPMS_OFF;
> +
> +			/* we keep a reference while the encoder is bound */
> +			drm_connector_unreference(connector);
>  		}
>  	}
>  
> @@ -635,9 +638,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
>  			mode_changed = true;
>  			/* If the encoder is reused for another connector, then
>  			 * the appropriate crtc will be set later.
> +			 * take a reference only if we haven't had an encoder before.
>  			 */
>  			if (connector->encoder)
>  				connector->encoder->crtc = NULL;
> +			else
> +				drm_connector_reference(connector);
>  			connector->encoder = new_encoder;
>  		}
>  	}
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH 4/5] drm/atomic: use connector references
  2016-04-27  7:13   ` Daniel Stone
@ 2016-04-27  7:47     ` Daniel Vetter
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2016-04-27  7:47 UTC (permalink / raw)
  To: Daniel Stone; +Cc: intel-gfx, dri-devel

On Wed, Apr 27, 2016 at 08:13:04AM +0100, Daniel Stone wrote:
> Hi,
> 
> On 27 April 2016 at 03:03, Dave Airlie <airlied@gmail.com> wrote:
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 9d5e3c8..d899dac 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1179,6 +1179,15 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
> >
> >         conn_state->crtc = crtc;
> >
> > +       /* If we had no crtc then got one, add a reference,
> > +        * if we had a crtc and are going to none, drop a reference,
> > +        * otherwise just keep the reference we have.
> > +        */
> > +       if (!had_crtc && crtc)
> > +               drm_connector_reference(conn_state->connector);
> > +       else if (!crtc && had_crtc)
> > +                drm_connector_unreference(conn_state->connector);
> 
> Similarly to the non-atomic comments, can you drop the had_crtc test
> here, and always ref if crtc, and unref if !crtc?

Yeah, that's more in line with how we refcount other stuff in atomic. With
that bikeshed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

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

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/5] drm/crtc: take references to connectors used in a modeset.
  2016-04-27  7:34   ` [Intel-gfx] " Daniel Vetter
@ 2016-04-27  7:48     ` Daniel Vetter
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2016-04-27  7:48 UTC (permalink / raw)
  To: Dave Airlie; +Cc: intel-gfx, dri-devel

On Wed, Apr 27, 2016 at 09:34:34AM +0200, Daniel Vetter wrote:
> On Wed, Apr 27, 2016 at 12:03:27PM +1000, Dave Airlie wrote:
> > From: Dave Airlie <airlied@redhat.com>
> > 
> > This just takes a reference on the connector when we set a mode
> > in the non-atomic paths.
> > 
> > Signed-off-by: Dave Airlie <airlied@redhat.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Ok I feel real silly now reading Daniel Stone's reply ;-) r-b retracted
...

Cheers, Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_crtc_helper.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> > index 66ca313..29b7835 100644
> > --- a/drivers/gpu/drm/drm_crtc_helper.c
> > +++ b/drivers/gpu/drm/drm_crtc_helper.c
> > @@ -456,6 +456,9 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
> >  			 * between them is henceforth no longer available.
> >  			 */
> >  			connector->dpms = DRM_MODE_DPMS_OFF;
> > +
> > +			/* we keep a reference while the encoder is bound */
> > +			drm_connector_unreference(connector);
> >  		}
> >  	}
> >  
> > @@ -635,9 +638,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
> >  			mode_changed = true;
> >  			/* If the encoder is reused for another connector, then
> >  			 * the appropriate crtc will be set later.
> > +			 * take a reference only if we haven't had an encoder before.
> >  			 */
> >  			if (connector->encoder)
> >  				connector->encoder->crtc = NULL;
> > +			else
> > +				drm_connector_reference(connector);
> >  			connector->encoder = new_encoder;
> >  		}
> >  	}
> > -- 
> > 2.5.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5] drm/i915/mst: use reference counted connectors. (v2)
  2016-04-27  2:03 ` [PATCH 5/5] drm/i915/mst: use reference counted connectors. (v2) Dave Airlie
@ 2016-04-27  7:52   ` Daniel Vetter
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2016-04-27  7:52 UTC (permalink / raw)
  To: Dave Airlie; +Cc: intel-gfx, dri-devel

On Wed, Apr 27, 2016 at 12:03:29PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> Don't just free the connector when we get the destroy callback.
> 
> Drop a reference to it, and set it's mst_port to NULL so
> no more mst work is done on it.
> 
> v2: core mst accepts NULLs fine. Cleanup EDID code properly.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 45 ++++++++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_drv.h    |  2 +-
>  2 files changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 94b4e83..4330b21 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -109,7 +109,7 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder)
>  
>  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
>  
> -	drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, intel_mst->port);
> +	drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, intel_mst->connector->port);
>  
>  	ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);
>  	if (ret) {
> @@ -134,10 +134,12 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder)
>  	/* and this can also fail */
>  	drm_dp_update_payload_part2(&intel_dp->mst_mgr);
>  
> -	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, intel_mst->port);
> +	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, intel_mst->connector->port);
>  
>  	intel_dp->active_mst_links--;
> -	intel_mst->port = NULL;
> +
> +	drm_connector_unreference(&intel_mst->connector->base);

Shouldn't atomic core take care of the refcounting of the active connector
for us now?

> +	intel_mst->connector = NULL;
>  	if (intel_dp->active_mst_links == 0) {
>  		intel_dig_port->base.post_disable(&intel_dig_port->base);
>  		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> @@ -177,7 +179,9 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
>  	found->encoder = encoder;
>  
>  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> -	intel_mst->port = found->port;
> +
> +	intel_mst->connector = found;
> +	drm_connector_reference(&intel_mst->connector->base);

Same here. I think removing those would be good, to avoid inflicting
refcounting headaches on drivers. Didn't notice this last time around.

With that change Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


>  
>  	if (intel_dp->active_mst_links == 0) {
>  		intel_prepare_ddi_buffer(&intel_dig_port->base);
> @@ -195,7 +199,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
>  	}
>  
>  	ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
> -				       intel_mst->port,
> +				       intel_mst->connector->port,
>  				       intel_crtc->config->pbn, &slots);
>  	if (ret == false) {
>  		DRM_ERROR("failed to allocate vcpi\n");
> @@ -244,7 +248,7 @@ static bool intel_dp_mst_enc_get_hw_state(struct intel_encoder *encoder,
>  {
>  	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
>  	*pipe = intel_mst->pipe;
> -	if (intel_mst->port)
> +	if (intel_mst->connector)
>  		return true;
>  	return false;
>  }
> @@ -308,10 +312,11 @@ static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector)
>  	struct edid *edid;
>  	int ret;
>  
> -	edid = drm_dp_mst_get_edid(connector, &intel_dp->mst_mgr, intel_connector->port);
> -	if (!edid)
> -		return 0;
> +	if (!intel_dp) {
> +		return intel_connector_update_modes(connector, NULL);
> +	}
>  
> +	edid = drm_dp_mst_get_edid(connector, &intel_dp->mst_mgr, intel_connector->port);
>  	ret = intel_connector_update_modes(connector, edid);
>  	kfree(edid);
>  
> @@ -324,6 +329,8 @@ intel_dp_mst_detect(struct drm_connector *connector, bool force)
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
>  	struct intel_dp *intel_dp = intel_connector->mst_port;
>  
> +	if (!intel_dp)
> +		return connector_status_disconnected;
>  	return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr, intel_connector->port);
>  }
>  
> @@ -389,6 +396,8 @@ static struct drm_encoder *intel_mst_atomic_best_encoder(struct drm_connector *c
>  	struct intel_dp *intel_dp = intel_connector->mst_port;
>  	struct intel_crtc *crtc = to_intel_crtc(state->crtc);
>  
> +	if (!intel_dp)
> +		return NULL;
>  	return &intel_dp->mst_encoders[crtc->pipe]->base.base;
>  }
>  
> @@ -396,6 +405,8 @@ static struct drm_encoder *intel_mst_best_encoder(struct drm_connector *connecto
>  {
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
>  	struct intel_dp *intel_dp = intel_connector->mst_port;
> +	if (!intel_dp)
> +		return NULL;
>  	return &intel_dp->mst_encoders[0]->base.base;
>  }
>  
> @@ -506,23 +517,11 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  
>  	/* need to nuke the connector */
>  	drm_modeset_lock_all(dev);
> -	if (connector->state->crtc) {
> -		struct drm_mode_set set;
> -		int ret;
> -
> -		memset(&set, 0, sizeof(set));
> -		set.crtc = connector->state->crtc,
> -
> -		ret = drm_atomic_helper_set_config(&set);
> -
> -		WARN(ret, "Disabling mst crtc failed with %i\n", ret);
> -	}
> -
>  	intel_connector_remove_from_fbdev(intel_connector);
> -	drm_connector_cleanup(connector);
> +	intel_connector->mst_port = NULL;
>  	drm_modeset_unlock_all(dev);
>  
> -	kfree(intel_connector);
> +	drm_connector_unreference(&intel_connector->base);
>  	DRM_DEBUG_KMS("\n");
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e0fcfa1..288135de 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -878,7 +878,7 @@ struct intel_dp_mst_encoder {
>  	struct intel_encoder base;
>  	enum pipe pipe;
>  	struct intel_digital_port *primary;
> -	void *port; /* store this opaque as its illegal to dereference it */
> +	struct intel_connector *connector;
>  };
>  
>  static inline enum dpio_channel
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2016-04-27  7:53 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-27  2:03 [PATCH 1/5] drm/modes: add connector reference counting. (v2) Dave Airlie
2016-04-27  2:03 ` [PATCH 2/5] drm/fb_helper: add connector reference counting Dave Airlie
2016-04-27  7:33   ` [Intel-gfx] " Daniel Vetter
2016-04-27  2:03 ` [PATCH 3/5] drm/crtc: take references to connectors used in a modeset Dave Airlie
2016-04-27  7:07   ` Daniel Stone
2016-04-27  7:34   ` [Intel-gfx] " Daniel Vetter
2016-04-27  7:48     ` Daniel Vetter
2016-04-27  2:03 ` [PATCH 4/5] drm/atomic: use connector references Dave Airlie
2016-04-27  7:13   ` Daniel Stone
2016-04-27  7:47     ` [Intel-gfx] " Daniel Vetter
2016-04-27  7:22   ` Daniel Stone
2016-04-27  2:03 ` [PATCH 5/5] drm/i915/mst: use reference counted connectors. (v2) Dave Airlie
2016-04-27  7:52   ` Daniel Vetter
2016-04-27  7:28 ` [Intel-gfx] [PATCH 1/5] drm/modes: add connector reference counting. (v2) Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox