drm reference counter connectors and fix lifetimes

drm reference counter connectors and fix lifetimes

[Dave Airlie]

I've been trolled since I did MST that I really didn't do a good job
on the connector lifetimes, so I finally felt guilty and had time to try
and fix this up.

This is a set of patches to handle connector lifetimes so that the connectors
don't go away in the middle of us doing something. I've done some testing
on this with kms_flip on my haswell laptop while undocking etc.

Daniel has been pestering me to finish it off, so I've cleaned up the last
few things in the mst patches at least for i915 and decided to send it out.

Dave.

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

[PATCH 01/15] drm/mode: rework drm_mode_object_put to drm_mode_object_unregister.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This changes the code to handle being called multiple times without
side effects. The new names seems more suitable for what it does.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_crtc.c          | 37 +++++++++++++++++++++----------------
 drivers/gpu/drm/drm_crtc_internal.h |  4 ++--
 drivers/gpu/drm/drm_modes.c         |  2 +-
 3 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e08f962..21cb8dc 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -323,19 +323,24 @@ static void drm_mode_object_register(struct drm_device *dev,
 }
 
 /**
- * drm_mode_object_put - free a modeset identifer
+ * drm_mode_object_unregister - free a modeset identifer
  * @dev: DRM device
  * @object: object to free
  *
- * Free @id from @dev's unique identifier pool. Note that despite the _get
- * postfix modeset identifiers are _not_ reference counted. Hence don't use this
+ * Free @id from @dev's unique identifier pool.
+ * This function can be called multiple times, and guards against
+ * multiple removals.
+ * These modeset identifiers are _not_ reference counted. Hence don't use this
  * for reference counted modeset objects like framebuffers.
  */
-void drm_mode_object_put(struct drm_device *dev,
+void drm_mode_object_unregister(struct drm_device *dev,
 			 struct drm_mode_object *object)
 {
 	mutex_lock(&dev->mode_config.idr_mutex);
-	idr_remove(&dev->mode_config.crtc_idr, object->id);
+	if (object->id) {
+		idr_remove(&dev->mode_config.crtc_idr, object->id);
+		object->id = 0;
+	}
 	mutex_unlock(&dev->mode_config.idr_mutex);
 }
 
@@ -705,7 +710,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 				       drm_num_crtcs(dev));
 	}
 	if (!crtc->name) {
-		drm_mode_object_put(dev, &crtc->base);
+		drm_mode_object_unregister(dev, &crtc->base);
 		return -ENOMEM;
 	}
 
@@ -747,7 +752,7 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
 
 	drm_modeset_lock_fini(&crtc->mutex);
 
-	drm_mode_object_put(dev, &crtc->base);
+	drm_mode_object_unregister(dev, &crtc->base);
 	list_del(&crtc->head);
 	dev->mode_config.num_crtc--;
 
@@ -972,7 +977,7 @@ out_put_id:
 		ida_remove(&config->connector_ida, connector->connector_id);
 out_put:
 	if (ret)
-		drm_mode_object_put(dev, &connector->base);
+		drm_mode_object_unregister(dev, &connector->base);
 
 out_unlock:
 	drm_modeset_unlock_all(dev);
@@ -1010,7 +1015,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
 		   connector->connector_id);
 
 	kfree(connector->display_info.bus_formats);
-	drm_mode_object_put(dev, &connector->base);
+	drm_mode_object_unregister(dev, &connector->base);
 	kfree(connector->name);
 	connector->name = NULL;
 	list_del(&connector->head);
@@ -1138,7 +1143,7 @@ int drm_encoder_init(struct drm_device *dev,
 
 out_put:
 	if (ret)
-		drm_mode_object_put(dev, &encoder->base);
+		drm_mode_object_unregister(dev, &encoder->base);
 
 out_unlock:
 	drm_modeset_unlock_all(dev);
@@ -1181,7 +1186,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
 	struct drm_device *dev = encoder->dev;
 
 	drm_modeset_lock_all(dev);
-	drm_mode_object_put(dev, &encoder->base);
+	drm_mode_object_unregister(dev, &encoder->base);
 	kfree(encoder->name);
 	list_del(&encoder->head);
 	dev->mode_config.num_encoder--;
@@ -1242,7 +1247,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 					    GFP_KERNEL);
 	if (!plane->format_types) {
 		DRM_DEBUG_KMS("out of memory when allocating plane\n");
-		drm_mode_object_put(dev, &plane->base);
+		drm_mode_object_unregister(dev, &plane->base);
 		return -ENOMEM;
 	}
 
@@ -1258,7 +1263,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 	}
 	if (!plane->name) {
 		kfree(plane->format_types);
-		drm_mode_object_put(dev, &plane->base);
+		drm_mode_object_unregister(dev, &plane->base);
 		return -ENOMEM;
 	}
 
@@ -1338,7 +1343,7 @@ void drm_plane_cleanup(struct drm_plane *plane)
 
 	drm_modeset_lock_all(dev);
 	kfree(plane->format_types);
-	drm_mode_object_put(dev, &plane->base);
+	drm_mode_object_unregister(dev, &plane->base);
 
 	BUG_ON(list_empty(&plane->head));
 
@@ -4029,7 +4034,7 @@ void drm_property_destroy(struct drm_device *dev, struct drm_property *property)
 
 	if (property->num_values)
 		kfree(property->values);
-	drm_mode_object_put(dev, &property->base);
+	drm_mode_object_unregister(dev, &property->base);
 	list_del(&property->head);
 	kfree(property);
 }
@@ -4307,7 +4312,7 @@ static void drm_property_free_blob(struct kref *kref)
 
 	list_del(&blob->head_global);
 	list_del(&blob->head_file);
-	drm_mode_object_put(blob->dev, &blob->base);
+	drm_mode_object_unregister(blob->dev, &blob->base);
 
 	kfree(blob);
 }
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 247dc8b..a78c138 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -33,8 +33,8 @@
 
 int drm_mode_object_get(struct drm_device *dev,
 			struct drm_mode_object *obj, uint32_t obj_type);
-void drm_mode_object_put(struct drm_device *dev,
-			 struct drm_mode_object *object);
+void drm_mode_object_unregister(struct drm_device *dev,
+				struct drm_mode_object *object);
 
 /* drm_atomic.c */
 int drm_atomic_get_property(struct drm_mode_object *obj,
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index f7448a5..7def3d5 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -98,7 +98,7 @@ void drm_mode_destroy(struct drm_device *dev, struct drm_display_mode *mode)
 	if (!mode)
 		return;
 
-	drm_mode_object_put(dev, &mode->base);
+	drm_mode_object_unregister(dev, &mode->base);
 
 	kfree(mode);
 }
-- 
2.5.5

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

[PATCH 02/15] drm/mode: move framebuffer_free up above framebuffer_init

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

A later patch will use it in framebuffer_init, and I want
to keep the diff cleaner.

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 21cb8dc..e69aac4 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -389,6 +389,35 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_mode_object_find);
 
+/* dev->mode_config.fb_lock must be held! */
+static void __drm_framebuffer_unregister(struct drm_device *dev,
+					 struct drm_framebuffer *fb)
+{
+	drm_mode_object_put(dev, &fb->base);
+
+	fb->base.id = 0;
+}
+
+static void drm_framebuffer_free(struct kref *kref)
+{
+	struct drm_framebuffer *fb =
+			container_of(kref, struct drm_framebuffer, refcount);
+	struct drm_device *dev = fb->dev;
+
+	/*
+	 * The lookup idr holds a weak reference, which has not necessarily been
+	 * removed at this point. Check for that.
+	 */
+	mutex_lock(&dev->mode_config.fb_lock);
+	if (fb->base.id) {
+		/* Mark fb as reaped and drop idr ref. */
+		__drm_framebuffer_unregister(dev, fb);
+	}
+	mutex_unlock(&dev->mode_config.fb_lock);
+
+	fb->funcs->destroy(fb);
+}
+
 /**
  * drm_framebuffer_init - initialize a framebuffer
  * @dev: DRM device
@@ -431,35 +460,6 @@ out:
 }
 EXPORT_SYMBOL(drm_framebuffer_init);
 
-/* dev->mode_config.fb_lock must be held! */
-static void __drm_framebuffer_unregister(struct drm_device *dev,
-					 struct drm_framebuffer *fb)
-{
-	drm_mode_object_put(dev, &fb->base);
-
-	fb->base.id = 0;
-}
-
-static void drm_framebuffer_free(struct kref *kref)
-{
-	struct drm_framebuffer *fb =
-			container_of(kref, struct drm_framebuffer, refcount);
-	struct drm_device *dev = fb->dev;
-
-	/*
-	 * The lookup idr holds a weak reference, which has not necessarily been
-	 * removed at this point. Check for that.
-	 */
-	mutex_lock(&dev->mode_config.fb_lock);
-	if (fb->base.id) {
-		/* Mark fb as reaped and drop idr ref. */
-		__drm_framebuffer_unregister(dev, fb);
-	}
-	mutex_unlock(&dev->mode_config.fb_lock);
-
-	fb->funcs->destroy(fb);
-}
-
 static struct drm_framebuffer *__drm_framebuffer_lookup(struct drm_device *dev,
 							uint32_t id)
 {
-- 
2.5.5

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

[PATCH 03/15] drm/modes: drop __drm_framebuffer_unregister.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

Just use the generic function.

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e69aac4..0ad1a92 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -389,15 +389,6 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_mode_object_find);
 
-/* dev->mode_config.fb_lock must be held! */
-static void __drm_framebuffer_unregister(struct drm_device *dev,
-					 struct drm_framebuffer *fb)
-{
-	drm_mode_object_put(dev, &fb->base);
-
-	fb->base.id = 0;
-}
-
 static void drm_framebuffer_free(struct kref *kref)
 {
 	struct drm_framebuffer *fb =
@@ -409,10 +400,7 @@ static void drm_framebuffer_free(struct kref *kref)
 	 * removed at this point. Check for that.
 	 */
 	mutex_lock(&dev->mode_config.fb_lock);
-	if (fb->base.id) {
-		/* Mark fb as reaped and drop idr ref. */
-		__drm_framebuffer_unregister(dev, fb);
-	}
+	drm_mode_object_unregister(dev, &fb->base);
 	mutex_unlock(&dev->mode_config.fb_lock);
 
 	fb->funcs->destroy(fb);
@@ -549,7 +537,7 @@ void drm_framebuffer_unregister_private(struct drm_framebuffer *fb)
 
 	mutex_lock(&dev->mode_config.fb_lock);
 	/* Mark fb as reaped and drop idr ref. */
-	__drm_framebuffer_unregister(dev, fb);
+	drm_mode_object_unregister(dev, &fb->base);
 	mutex_unlock(&dev->mode_config.fb_lock);
 }
 EXPORT_SYMBOL(drm_framebuffer_unregister_private);
-- 
2.5.5

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

[PATCH 04/15] drm/mode: introduce wrapper to read framebuffer refcount.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

Avoids drivers knowing where the kref is stored.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_crtc.c          | 2 +-
 drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
 drivers/gpu/drm/msm/msm_fb.c        | 2 +-
 drivers/gpu/drm/tegra/drm.c         | 2 +-
 include/drm/drm_crtc.h              | 5 +++++
 5 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 0ad1a92..8616737 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -612,7 +612,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 	 * in-use fb with fb-id == 0. Userspace is allowed to shoot its own foot
 	 * in this manner.
 	 */
-	if (atomic_read(&fb->refcount.refcount) > 1) {
+	if (drm_framebuffer_read_refcount(fb) > 1) {
 		drm_modeset_lock_all(dev);
 		/* remove from any CRTC */
 		drm_for_each_crtc(crtc, dev) {
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a0f1bd7..20d9300 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1908,7 +1908,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
                          fbdev_fb->base.depth,
                          fbdev_fb->base.bits_per_pixel,
                          fbdev_fb->base.modifier[0],
-                         atomic_read(&fbdev_fb->base.refcount.refcount));
+                         drm_framebuffer_read_refcount(&fbdev_fb->base));
                describe_obj(m, fbdev_fb->obj);
                seq_putc(m, '\n');
        }
@@ -1926,7 +1926,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 			   fb->base.depth,
 			   fb->base.bits_per_pixel,
 			   fb->base.modifier[0],
-			   atomic_read(&fb->base.refcount.refcount));
+			   drm_framebuffer_read_refcount(&fb->base));
 		describe_obj(m, fb->obj);
 		seq_putc(m, '\n');
 	}
diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
index a474d6c..17e0c9e 100644
--- a/drivers/gpu/drm/msm/msm_fb.c
+++ b/drivers/gpu/drm/msm/msm_fb.c
@@ -77,7 +77,7 @@ void msm_framebuffer_describe(struct drm_framebuffer *fb, struct seq_file *m)
 
 	seq_printf(m, "fb: %dx%d@%4.4s (%2d, ID:%d)\n",
 			fb->width, fb->height, (char *)&fb->pixel_format,
-			fb->refcount.refcount.counter, fb->base.id);
+			drm_framebuffer_read_refcount(fb), fb->base.id);
 
 	for (i = 0; i < n; i++) {
 		seq_printf(m, "   %d: offset=%d pitch=%d, obj: ",
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 8e6b18c..2be88eb 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -878,7 +878,7 @@ static int tegra_debugfs_framebuffers(struct seq_file *s, void *data)
 		seq_printf(s, "%3d: user size: %d x %d, depth %d, %d bpp, refcount %d\n",
 			   fb->base.id, fb->width, fb->height, fb->depth,
 			   fb->bits_per_pixel,
-			   atomic_read(&fb->refcount.refcount));
+			   drm_framebuffer_read_refcount(fb));
 	}
 
 	mutex_unlock(&drm->mode_config.fb_lock);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e0170bf..99a12f0 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -2608,6 +2608,11 @@ static inline uint32_t drm_color_lut_extract(uint32_t user_input,
 	return clamp_val(val, 0, max);
 }
 
+static inline uint32_t drm_framebuffer_read_refcount(struct drm_framebuffer *fb)
+{
+	return atomic_read(&fb->refcount.refcount);
+}
+
 /* 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

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

[PATCH 05/15] drm/mode: move framebuffer reference into object.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This is the initial code to add references to some mode objects.
In the future we need to start reference counting connectors so
firstly I want to reorganise the code so the framebuffer ref counting
uses the same paths.

This patch shouldn't change any functionality, just moves the kref.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_crtc.c | 72 ++++++++++++++++++++++++----------------------
 include/drm/drm_crtc.h     | 20 ++++++++++---
 2 files changed, 54 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 8616737..75a45e9 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -275,7 +275,8 @@ EXPORT_SYMBOL(drm_get_format_name);
 static int drm_mode_object_get_reg(struct drm_device *dev,
 				   struct drm_mode_object *obj,
 				   uint32_t obj_type,
-				   bool register_obj)
+				   bool register_obj,
+				   void (*obj_free_cb)(struct kref *kref))
 {
 	int ret;
 
@@ -288,6 +289,10 @@ static int drm_mode_object_get_reg(struct drm_device *dev,
 		 */
 		obj->id = ret;
 		obj->type = obj_type;
+		if (obj_free_cb) {
+			obj->free_cb = obj_free_cb;
+			kref_init(&obj->refcount);
+		}
 	}
 	mutex_unlock(&dev->mode_config.idr_mutex);
 
@@ -311,7 +316,7 @@ static int drm_mode_object_get_reg(struct drm_device *dev,
 int drm_mode_object_get(struct drm_device *dev,
 			struct drm_mode_object *obj, uint32_t obj_type)
 {
-	return drm_mode_object_get_reg(dev, obj, obj_type, true);
+	return drm_mode_object_get_reg(dev, obj, obj_type, true, NULL);
 }
 
 static void drm_mode_object_register(struct drm_device *dev,
@@ -389,10 +394,35 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_mode_object_find);
 
+void drm_mode_object_unreference(struct drm_mode_object *obj)
+{
+	if (obj->free_cb) {
+		DRM_DEBUG("OBJ ID: %d (%d)\n", obj->id, atomic_read(&obj->refcount.refcount));
+		kref_put(&obj->refcount, obj->free_cb);
+	}
+}
+EXPORT_SYMBOL(drm_mode_object_unreference);
+
+/**
+ * drm_mode_object_reference - incr the fb refcnt
+ * @obj: mode_object
+ *
+ * This function operates only on refcounted objects.
+ * This functions increments the object's refcount.
+ */
+void drm_mode_object_reference(struct drm_mode_object *obj)
+{
+	if (obj->free_cb) {
+		DRM_DEBUG("OBJ ID: %d (%d)\n", obj->id, atomic_read(&obj->refcount.refcount));
+		kref_get(&obj->refcount);
+	}
+}
+EXPORT_SYMBOL(drm_mode_object_reference);
+
 static void drm_framebuffer_free(struct kref *kref)
 {
 	struct drm_framebuffer *fb =
-			container_of(kref, struct drm_framebuffer, refcount);
+			container_of(kref, struct drm_framebuffer, base.refcount);
 	struct drm_device *dev = fb->dev;
 
 	/*
@@ -430,12 +460,12 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 	int ret;
 
 	mutex_lock(&dev->mode_config.fb_lock);
-	kref_init(&fb->refcount);
 	INIT_LIST_HEAD(&fb->filp_head);
 	fb->dev = dev;
 	fb->funcs = funcs;
 
-	ret = drm_mode_object_get(dev, &fb->base, DRM_MODE_OBJECT_FB);
+	ret = drm_mode_object_get_reg(dev, &fb->base, DRM_MODE_OBJECT_FB,
+				      true, drm_framebuffer_free);
 	if (ret)
 		goto out;
 
@@ -482,7 +512,7 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
 	mutex_lock(&dev->mode_config.fb_lock);
 	fb = __drm_framebuffer_lookup(dev, id);
 	if (fb) {
-		if (!kref_get_unless_zero(&fb->refcount))
+		if (!kref_get_unless_zero(&fb->base.refcount))
 			fb = NULL;
 	}
 	mutex_unlock(&dev->mode_config.fb_lock);
@@ -492,32 +522,6 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
 EXPORT_SYMBOL(drm_framebuffer_lookup);
 
 /**
- * drm_framebuffer_unreference - unref a framebuffer
- * @fb: framebuffer to unref
- *
- * This functions decrements the fb's refcount and frees it if it drops to zero.
- */
-void drm_framebuffer_unreference(struct drm_framebuffer *fb)
-{
-	DRM_DEBUG("%p: FB ID: %d (%d)\n", fb, fb->base.id, atomic_read(&fb->refcount.refcount));
-	kref_put(&fb->refcount, drm_framebuffer_free);
-}
-EXPORT_SYMBOL(drm_framebuffer_unreference);
-
-/**
- * drm_framebuffer_reference - incr the fb refcnt
- * @fb: framebuffer
- *
- * This functions increments the fb's refcount.
- */
-void drm_framebuffer_reference(struct drm_framebuffer *fb)
-{
-	DRM_DEBUG("%p: FB ID: %d (%d)\n", fb, fb->base.id, atomic_read(&fb->refcount.refcount));
-	kref_get(&fb->refcount);
-}
-EXPORT_SYMBOL(drm_framebuffer_reference);
-
-/**
  * drm_framebuffer_unregister_private - unregister a private fb from the lookup idr
  * @fb: fb to unregister
  *
@@ -902,7 +906,7 @@ 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);
+	ret = drm_mode_object_get_reg(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR, false, NULL);
 	if (ret)
 		goto out_unlock;
 
@@ -5922,7 +5926,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 	 */
 	WARN_ON(!list_empty(&dev->mode_config.fb_list));
 	list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
-		drm_framebuffer_free(&fb->refcount);
+		drm_framebuffer_free(&fb->base.refcount);
 	}
 
 	list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 99a12f0..576faf4 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -59,6 +59,8 @@ struct drm_mode_object {
 	uint32_t id;
 	uint32_t type;
 	struct drm_object_properties *properties;
+	struct kref refcount;
+	void (*free_cb)(struct kref *kref);
 };
 
 #define DRM_OBJECT_MAX_PROPERTY 24
@@ -233,8 +235,8 @@ struct drm_framebuffer {
 	 * should be deferred.  In cases like this, the driver would like to
 	 * hold a ref to the fb even though it has already been removed from
 	 * userspace perspective.
+	 * The refcount is stored inside the mode object.
 	 */
-	struct kref refcount;
 	/*
 	 * Place on the dev->mode_config.fb_list, access protected by
 	 * dev->mode_config.fb_lock.
@@ -2386,8 +2388,6 @@ extern int drm_framebuffer_init(struct drm_device *dev,
 				const struct drm_framebuffer_funcs *funcs);
 extern struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
 						      uint32_t id);
-extern void drm_framebuffer_unreference(struct drm_framebuffer *fb);
-extern void drm_framebuffer_reference(struct drm_framebuffer *fb);
 extern void drm_framebuffer_remove(struct drm_framebuffer *fb);
 extern void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
 extern void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
@@ -2445,6 +2445,8 @@ extern int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 					 int gamma_size);
 extern struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
 		uint32_t id, uint32_t type);
+void drm_mode_object_reference(struct drm_mode_object *obj);
+void drm_mode_object_unreference(struct drm_mode_object *obj);
 
 /* IOCTLs */
 extern int drm_mode_getresources(struct drm_device *dev,
@@ -2608,9 +2610,19 @@ static inline uint32_t drm_color_lut_extract(uint32_t user_input,
 	return clamp_val(val, 0, max);
 }
 
+static inline void drm_framebuffer_reference(struct drm_framebuffer *fb)
+{
+	drm_mode_object_reference(&fb->base);
+}
+
+static inline void drm_framebuffer_unreference(struct drm_framebuffer *fb)
+{
+	drm_mode_object_unreference(&fb->base);
+}
+
 static inline uint32_t drm_framebuffer_read_refcount(struct drm_framebuffer *fb)
 {
-	return atomic_read(&fb->refcount.refcount);
+	return atomic_read(&fb->base.refcount.refcount);
 }
 
 /* Plane list iterator for legacy (overlay only) planes. */
-- 
2.5.5

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

[PATCH 06/15] drm/mode: use _object_find to find framebuffers.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

No point have this code dupliated at this point, use the
_object_find code instead now.

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 75a45e9..0d75517 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -362,8 +362,7 @@ static struct drm_mode_object *_object_find(struct drm_device *dev,
 		obj = NULL;
 	/* don't leak out unref'd fb's */
 	if (obj &&
-	    (obj->type == DRM_MODE_OBJECT_FB ||
-	     obj->type == DRM_MODE_OBJECT_BLOB))
+	    obj->type == DRM_MODE_OBJECT_BLOB)
 		obj = NULL;
 	mutex_unlock(&dev->mode_config.idr_mutex);
 
@@ -478,23 +477,6 @@ out:
 }
 EXPORT_SYMBOL(drm_framebuffer_init);
 
-static struct drm_framebuffer *__drm_framebuffer_lookup(struct drm_device *dev,
-							uint32_t id)
-{
-	struct drm_mode_object *obj = NULL;
-	struct drm_framebuffer *fb;
-
-	mutex_lock(&dev->mode_config.idr_mutex);
-	obj = idr_find(&dev->mode_config.crtc_idr, id);
-	if (!obj || (obj->type != DRM_MODE_OBJECT_FB) || (obj->id != id))
-		fb = NULL;
-	else
-		fb = obj_to_fb(obj);
-	mutex_unlock(&dev->mode_config.idr_mutex);
-
-	return fb;
-}
-
 /**
  * drm_framebuffer_lookup - look up a drm framebuffer and grab a reference
  * @dev: drm device
@@ -507,11 +489,13 @@ static struct drm_framebuffer *__drm_framebuffer_lookup(struct drm_device *dev,
 struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
 					       uint32_t id)
 {
-	struct drm_framebuffer *fb;
+	struct drm_mode_object *obj;
+	struct drm_framebuffer *fb = NULL;
 
 	mutex_lock(&dev->mode_config.fb_lock);
-	fb = __drm_framebuffer_lookup(dev, id);
-	if (fb) {
+	obj = _object_find(dev, id, DRM_MODE_OBJECT_FB);
+	if (obj) {
+		fb = obj_to_fb(obj);
 		if (!kref_get_unless_zero(&fb->base.refcount))
 			fb = NULL;
 	}
@@ -3449,6 +3433,7 @@ int drm_mode_rmfb(struct drm_device *dev,
 {
 	struct drm_framebuffer *fb = NULL;
 	struct drm_framebuffer *fbl = NULL;
+	struct drm_mode_object *obj;
 	uint32_t *id = data;
 	int found = 0;
 
@@ -3457,10 +3442,10 @@ int drm_mode_rmfb(struct drm_device *dev,
 
 	mutex_lock(&file_priv->fbs_lock);
 	mutex_lock(&dev->mode_config.fb_lock);
-	fb = __drm_framebuffer_lookup(dev, *id);
-	if (!fb)
+	obj = _object_find(dev, *id, DRM_MODE_OBJECT_FB);
+	if (!obj)
 		goto fail_lookup;
-
+	fb = obj_to_fb(obj);
 	list_for_each_entry(fbl, &file_priv->fbs, filp_head)
 		if (fb == fbl)
 			found = 1;
-- 
2.5.5

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

[PATCH 07/15] drm/mode: reduce scope of fb_lock in framebuffer init

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

We don't need to hold the fb lock around the initialisation,
only around the list manipulaton.

So do the lock hold only around the register for now.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_crtc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 0d75517..1863879 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -458,21 +458,22 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 {
 	int ret;
 
-	mutex_lock(&dev->mode_config.fb_lock);
 	INIT_LIST_HEAD(&fb->filp_head);
 	fb->dev = dev;
 	fb->funcs = funcs;
 
 	ret = drm_mode_object_get_reg(dev, &fb->base, DRM_MODE_OBJECT_FB,
-				      true, drm_framebuffer_free);
+				      false, drm_framebuffer_free);
 	if (ret)
 		goto out;
 
+	mutex_lock(&dev->mode_config.fb_lock);
 	dev->mode_config.num_fb++;
 	list_add(&fb->head, &dev->mode_config.fb_list);
-out:
-	mutex_unlock(&dev->mode_config.fb_lock);
 
+	drm_mode_object_register(dev, &fb->base);
+	mutex_unlock(&dev->mode_config.fb_lock);
+out:
 	return ret;
 }
 EXPORT_SYMBOL(drm_framebuffer_init);
-- 
2.5.5

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

[PATCH 08/15] drm/mode: reduce lock hold in addfb2

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

No need to hold the lock while assigning the variable.

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 1863879..21cb998 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3405,11 +3405,11 @@ int drm_mode_addfb2(struct drm_device *dev,
 	if (IS_ERR(fb))
 		return PTR_ERR(fb);
 
-	/* Transfer ownership to the filp for reaping on close */
-
 	DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);
-	mutex_lock(&file_priv->fbs_lock);
 	r->fb_id = fb->base.id;
+
+	/* Transfer ownership to the filp for reaping on close */
+	mutex_lock(&file_priv->fbs_lock);
 	list_add(&fb->filp_head, &file_priv->fbs);
 	mutex_unlock(&file_priv->fbs_lock);
 
-- 
2.5.5

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

[PATCH 09/15] drm/modes: move reference taking into object lookup.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

When we lookup an ref counted object we now take a proper reference
using kref_get_unless_zero.

Framebuffer lookup no longer needs do this itself.

Convert rmfb to using framebuffer lookup and deal with the fact
it now gets an extra reference that we have to cleanup. This should
mean we can avoid holding fb_lock across rmfb. (if I'm wrong let me
know).

We also now only hold the fbs_lock around the list manipulation.

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 21cb998..e47c4a2 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -364,6 +364,11 @@ static struct drm_mode_object *_object_find(struct drm_device *dev,
 	if (obj &&
 	    obj->type == DRM_MODE_OBJECT_BLOB)
 		obj = NULL;
+
+	if (obj && obj->free_cb) {
+		if (!kref_get_unless_zero(&obj->refcount))
+			obj = NULL;
+	}
 	mutex_unlock(&dev->mode_config.idr_mutex);
 
 	return obj;
@@ -495,11 +500,8 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
 
 	mutex_lock(&dev->mode_config.fb_lock);
 	obj = _object_find(dev, id, DRM_MODE_OBJECT_FB);
-	if (obj) {
+	if (obj)
 		fb = obj_to_fb(obj);
-		if (!kref_get_unless_zero(&fb->base.refcount))
-			fb = NULL;
-	}
 	mutex_unlock(&dev->mode_config.fb_lock);
 
 	return fb;
@@ -3434,37 +3436,38 @@ int drm_mode_rmfb(struct drm_device *dev,
 {
 	struct drm_framebuffer *fb = NULL;
 	struct drm_framebuffer *fbl = NULL;
-	struct drm_mode_object *obj;
 	uint32_t *id = data;
 	int found = 0;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
 
+	fb = drm_framebuffer_lookup(dev, *id);
+	if (!fb)
+		return -ENOENT;
+
 	mutex_lock(&file_priv->fbs_lock);
-	mutex_lock(&dev->mode_config.fb_lock);
-	obj = _object_find(dev, *id, DRM_MODE_OBJECT_FB);
-	if (!obj)
-		goto fail_lookup;
-	fb = obj_to_fb(obj);
 	list_for_each_entry(fbl, &file_priv->fbs, filp_head)
 		if (fb == fbl)
 			found = 1;
-	if (!found)
-		goto fail_lookup;
+	if (!found) {
+		mutex_unlock(&file_priv->fbs_lock);
+		goto fail_unref;
+	}
 
 	list_del_init(&fb->filp_head);
-	mutex_unlock(&dev->mode_config.fb_lock);
 	mutex_unlock(&file_priv->fbs_lock);
 
+	/* we now own the reference that was stored in the fbs list */
 	drm_framebuffer_unreference(fb);
 
-	return 0;
+	/* drop the reference we picked up in framebuffer lookup */
+	drm_framebuffer_unreference(fb);
 
-fail_lookup:
-	mutex_unlock(&dev->mode_config.fb_lock);
-	mutex_unlock(&file_priv->fbs_lock);
+	return 0;
 
+fail_unref:
+	drm_framebuffer_unreference(fb);
 	return -ENOENT;
 }
 
-- 
2.5.5

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

[PATCH 10/15] drm/modes: reduce fb_lock to just protecting lists

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This reduces the fb_lock to just protecting the num_fb/fb_list.

I'd like to have some discussion on if this opens up any race
conditions.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_crtc.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e47c4a2..46f32f2 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -433,9 +433,7 @@ static void drm_framebuffer_free(struct kref *kref)
 	 * The lookup idr holds a weak reference, which has not necessarily been
 	 * removed at this point. Check for that.
 	 */
-	mutex_lock(&dev->mode_config.fb_lock);
 	drm_mode_object_unregister(dev, &fb->base);
-	mutex_unlock(&dev->mode_config.fb_lock);
 
 	fb->funcs->destroy(fb);
 }
@@ -475,9 +473,9 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 	mutex_lock(&dev->mode_config.fb_lock);
 	dev->mode_config.num_fb++;
 	list_add(&fb->head, &dev->mode_config.fb_list);
+	mutex_unlock(&dev->mode_config.fb_lock);
 
 	drm_mode_object_register(dev, &fb->base);
-	mutex_unlock(&dev->mode_config.fb_lock);
 out:
 	return ret;
 }
@@ -498,12 +496,9 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
 	struct drm_mode_object *obj;
 	struct drm_framebuffer *fb = NULL;
 
-	mutex_lock(&dev->mode_config.fb_lock);
 	obj = _object_find(dev, id, DRM_MODE_OBJECT_FB);
 	if (obj)
 		fb = obj_to_fb(obj);
-	mutex_unlock(&dev->mode_config.fb_lock);
-
 	return fb;
 }
 EXPORT_SYMBOL(drm_framebuffer_lookup);
@@ -526,10 +521,8 @@ void drm_framebuffer_unregister_private(struct drm_framebuffer *fb)
 
 	dev = fb->dev;
 
-	mutex_lock(&dev->mode_config.fb_lock);
 	/* Mark fb as reaped and drop idr ref. */
 	drm_mode_object_unregister(dev, &fb->base);
-	mutex_unlock(&dev->mode_config.fb_lock);
 }
 EXPORT_SYMBOL(drm_framebuffer_unregister_private);
 
-- 
2.5.5

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

[PATCH 11/15] drm/modes: stop handling framebuffer special

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

Since ref counting is in the object now we can just call the
normal interfaces.

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 46f32f2..f6bf828 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4810,19 +4810,7 @@ bool drm_property_change_valid_get(struct drm_property *property,
 		if (value == 0)
 			return true;
 
-		/* handle refcnt'd objects specially: */
-		if (property->values[0] == DRM_MODE_OBJECT_FB) {
-			struct drm_framebuffer *fb;
-			fb = drm_framebuffer_lookup(property->dev, value);
-			if (fb) {
-				*ref = &fb->base;
-				return true;
-			} else {
-				return false;
-			}
-		} else {
-			return _object_find(property->dev, value, property->values[0]) != NULL;
-		}
+		return _object_find(property->dev, value, property->values[0]) != NULL;
 	}
 
 	for (i = 0; i < property->num_values; i++)
@@ -4838,8 +4826,7 @@ void drm_property_change_valid_put(struct drm_property *property,
 		return;
 
 	if (drm_property_type_is(property, DRM_MODE_PROP_OBJECT)) {
-		if (property->values[0] == DRM_MODE_OBJECT_FB)
-			drm_framebuffer_unreference(obj_to_fb(ref));
+		drm_mode_object_unreference(ref);
 	} else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB))
 		drm_property_unreference_blob(obj_to_blob(ref));
 }
-- 
2.5.5

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

[PATCH 12/15] drm/modes: add connector reference counting.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

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

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_atomic.c    | 19 +++++++++++++++++--
 drivers/gpu/drm/drm_crtc.c      | 39 ++++++++++++++++++++++++++++++---------
 drivers/gpu/drm/drm_fb_helper.c | 38 ++++++--------------------------------
 include/drm/drm_crtc.h          | 13 ++++++++++++-
 4 files changed, 65 insertions(+), 44 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 f6bf828..90bc597 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -861,6 +861,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
@@ -886,7 +896,7 @@ 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;
 
@@ -2106,7 +2116,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;
@@ -2190,6 +2200,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);
 
@@ -2834,13 +2845,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);
@@ -2868,6 +2880,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);
@@ -4957,7 +4975,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
 	}
 	if (!obj->properties) {
 		ret = -EINVAL;
-		goto out;
+		goto out_unref;
 	}
 
 	ret = get_properties(obj, file_priv->atomic,
@@ -4965,6 +4983,8 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
 			(uint64_t __user *)(unsigned long)(arg->prop_values_ptr),
 			&arg->count_props);
 
+out_unref:
+	drm_mode_object_unreference(obj);
 out:
 	drm_modeset_unlock_all(dev);
 	return ret;
@@ -5007,25 +5027,25 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 		goto out;
 	}
 	if (!arg_obj->properties)
-		goto out;
+		goto out_unref;
 
 	for (i = 0; i < arg_obj->properties->count; i++)
 		if (arg_obj->properties->properties[i]->base.id == arg->prop_id)
 			break;
 
 	if (i == arg_obj->properties->count)
-		goto out;
+		goto out_unref;
 
 	prop_obj = drm_mode_object_find(dev, arg->prop_id,
 					DRM_MODE_OBJECT_PROPERTY);
 	if (!prop_obj) {
 		ret = -ENOENT;
-		goto out;
+		goto out_unref;
 	}
 	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:
@@ -5042,7 +5062,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 	}
 
 	drm_property_change_valid_put(property, ref);
-
+out_unref:
+	drm_mode_object_unreference(arg_obj);
 out:
 	drm_modeset_unlock_all(dev);
 	return ret;
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 855108e..3691565 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)
 {
@@ -213,10 +186,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);
@@ -2024,7 +1993,11 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 	/* need to set the modesets up here for use later */
 	/* fill out the connector<->crtc mappings into the modesets */
 	for (i = 0; i < fb_helper->crtc_count; i++) {
+		int j;
 		modeset = &fb_helper->crtc_info[i].mode_set;
+
+		for (j = 0; j < modeset->num_connectors; j++)
+			drm_connector_unreference(modeset->connectors[j]);
 		modeset->num_connectors = 0;
 		modeset->fb = NULL;
 	}
@@ -2045,6 +2018,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 				drm_mode_destroy(dev, modeset->mode);
 			modeset->mode = drm_mode_duplicate(dev,
 							   fb_crtc->desired_mode);
+			drm_connector_reference(fb_helper->connector_info[i]->connector);
 			modeset->connectors[modeset->num_connectors++] = fb_helper->connector_info[i]->connector;
 			modeset->fb = fb_helper->fb;
 			modeset->x = offset->x;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 576faf4..7e13127 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -2579,7 +2579,8 @@ 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,
+/* takes a reference */
+static inline struct drm_connector *drm_connector_lookup(struct drm_device *dev,
 		uint32_t id)
 {
 	struct drm_mode_object *mo;
@@ -2625,6 +2626,16 @@ static inline uint32_t drm_framebuffer_read_refcount(struct drm_framebuffer *fb)
 	return atomic_read(&fb->base.refcount.refcount);
 }
 
+static inline void drm_connector_reference(struct drm_connector *connector)
+{
+	drm_mode_object_reference(&connector->base);
+}
+
+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

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

[PATCH 13/15] drm: take references to connectors used in a modeset.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

As suggested by Daniel, if we are actively using the connector in a modeset
we don't want it to disappear from underneath us. This takes a reference
to the connector in the atomic paths when we are setting the state up,
and in the non-atomic paths when binding the encoder.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_atomic.c      | 11 ++++++++++-
 drivers/gpu/drm/drm_crtc_helper.c |  6 ++++++
 2 files changed, 16 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_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 79555d2..71b6c72 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

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

[PATCH 14/15] drm/i915/mst: use reference counted connectors.

[Dave Airlie]

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.

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

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index a2bd698..2e730b6 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -113,7 +113,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) {
@@ -138,10 +138,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);
@@ -181,7 +183,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);
@@ -199,7 +203,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");
@@ -248,7 +252,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;
 }
@@ -312,6 +316,11 @@ static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector)
 	struct edid *edid;
 	int ret;
 
+	if (!intel_connector->port || !intel_dp) {
+		ret = intel_connector_update_modes(connector, NULL);
+		return ret;
+	}
+
 	edid = drm_dp_mst_get_edid(connector, &intel_dp->mst_mgr, intel_connector->port);
 	if (!edid)
 		return 0;
@@ -328,6 +337,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_connector->port || !intel_dp)
+		return connector_status_disconnected;
 	return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr, intel_connector->port);
 }
 
@@ -393,6 +404,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;
 }
 
@@ -400,6 +413,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,29 +521,14 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct drm_device *dev = connector->dev;
 
-	/* 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);
-	}
-	drm_modeset_unlock_all(dev);
-
 	intel_connector->unregister(intel_connector);
 
 	drm_modeset_lock_all(dev);
 	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 4c027d6..0d2360e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -831,7 +831,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

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

[PATCH 15/15] drm/radeon/dp_mst: use connector ref counting

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

Use connector reference counting in radeon mst code.

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

diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c
index 43cffb5..4f1792a 100644
--- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
@@ -242,6 +242,7 @@ radeon_dp_mst_connector_destroy(struct drm_connector *connector)
 
 	drm_encoder_cleanup(&radeon_encoder->base);
 	kfree(radeon_encoder);
+
 	drm_connector_cleanup(connector);
 	kfree(radeon_connector);
 }
@@ -313,11 +314,9 @@ static void radeon_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	drm_modeset_lock_all(dev);
 	/* dpms off */
 	radeon_fb_remove_connector(rdev, connector);
-
-	drm_connector_cleanup(connector);
 	drm_modeset_unlock_all(dev);
 
-	kfree(connector);
+	drm_connector_unreference(connector);
 	DRM_DEBUG_KMS("\n");
 }
 
-- 
2.5.5

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

Re: [PATCH 01/15] drm/mode: rework drm_mode_object_put to drm_mode_object_unregister.

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:32PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This changes the code to handle being called multiple times without
> side effects. The new names seems more suitable for what it does.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

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

> ---
>  drivers/gpu/drm/drm_crtc.c          | 37 +++++++++++++++++++++----------------
>  drivers/gpu/drm/drm_crtc_internal.h |  4 ++--
>  drivers/gpu/drm/drm_modes.c         |  2 +-
>  3 files changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e08f962..21cb8dc 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -323,19 +323,24 @@ static void drm_mode_object_register(struct drm_device *dev,
>  }
>  
>  /**
> - * drm_mode_object_put - free a modeset identifer
> + * drm_mode_object_unregister - free a modeset identifer
>   * @dev: DRM device
>   * @object: object to free
>   *
> - * Free @id from @dev's unique identifier pool. Note that despite the _get
> - * postfix modeset identifiers are _not_ reference counted. Hence don't use this
> + * Free @id from @dev's unique identifier pool.
> + * This function can be called multiple times, and guards against
> + * multiple removals.
> + * These modeset identifiers are _not_ reference counted. Hence don't use this
>   * for reference counted modeset objects like framebuffers.
>   */
> -void drm_mode_object_put(struct drm_device *dev,
> +void drm_mode_object_unregister(struct drm_device *dev,
>  			 struct drm_mode_object *object)
>  {
>  	mutex_lock(&dev->mode_config.idr_mutex);
> -	idr_remove(&dev->mode_config.crtc_idr, object->id);
> +	if (object->id) {
> +		idr_remove(&dev->mode_config.crtc_idr, object->id);
> +		object->id = 0;
> +	}
>  	mutex_unlock(&dev->mode_config.idr_mutex);
>  }
>  
> @@ -705,7 +710,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  				       drm_num_crtcs(dev));
>  	}
>  	if (!crtc->name) {
> -		drm_mode_object_put(dev, &crtc->base);
> +		drm_mode_object_unregister(dev, &crtc->base);
>  		return -ENOMEM;
>  	}
>  
> @@ -747,7 +752,7 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
>  
>  	drm_modeset_lock_fini(&crtc->mutex);
>  
> -	drm_mode_object_put(dev, &crtc->base);
> +	drm_mode_object_unregister(dev, &crtc->base);
>  	list_del(&crtc->head);
>  	dev->mode_config.num_crtc--;
>  
> @@ -972,7 +977,7 @@ out_put_id:
>  		ida_remove(&config->connector_ida, connector->connector_id);
>  out_put:
>  	if (ret)
> -		drm_mode_object_put(dev, &connector->base);
> +		drm_mode_object_unregister(dev, &connector->base);
>  
>  out_unlock:
>  	drm_modeset_unlock_all(dev);
> @@ -1010,7 +1015,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  		   connector->connector_id);
>  
>  	kfree(connector->display_info.bus_formats);
> -	drm_mode_object_put(dev, &connector->base);
> +	drm_mode_object_unregister(dev, &connector->base);
>  	kfree(connector->name);
>  	connector->name = NULL;
>  	list_del(&connector->head);
> @@ -1138,7 +1143,7 @@ int drm_encoder_init(struct drm_device *dev,
>  
>  out_put:
>  	if (ret)
> -		drm_mode_object_put(dev, &encoder->base);
> +		drm_mode_object_unregister(dev, &encoder->base);
>  
>  out_unlock:
>  	drm_modeset_unlock_all(dev);
> @@ -1181,7 +1186,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
>  	struct drm_device *dev = encoder->dev;
>  
>  	drm_modeset_lock_all(dev);
> -	drm_mode_object_put(dev, &encoder->base);
> +	drm_mode_object_unregister(dev, &encoder->base);
>  	kfree(encoder->name);
>  	list_del(&encoder->head);
>  	dev->mode_config.num_encoder--;
> @@ -1242,7 +1247,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  					    GFP_KERNEL);
>  	if (!plane->format_types) {
>  		DRM_DEBUG_KMS("out of memory when allocating plane\n");
> -		drm_mode_object_put(dev, &plane->base);
> +		drm_mode_object_unregister(dev, &plane->base);
>  		return -ENOMEM;
>  	}
>  
> @@ -1258,7 +1263,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  	}
>  	if (!plane->name) {
>  		kfree(plane->format_types);
> -		drm_mode_object_put(dev, &plane->base);
> +		drm_mode_object_unregister(dev, &plane->base);
>  		return -ENOMEM;
>  	}
>  
> @@ -1338,7 +1343,7 @@ void drm_plane_cleanup(struct drm_plane *plane)
>  
>  	drm_modeset_lock_all(dev);
>  	kfree(plane->format_types);
> -	drm_mode_object_put(dev, &plane->base);
> +	drm_mode_object_unregister(dev, &plane->base);
>  
>  	BUG_ON(list_empty(&plane->head));
>  
> @@ -4029,7 +4034,7 @@ void drm_property_destroy(struct drm_device *dev, struct drm_property *property)
>  
>  	if (property->num_values)
>  		kfree(property->values);
> -	drm_mode_object_put(dev, &property->base);
> +	drm_mode_object_unregister(dev, &property->base);
>  	list_del(&property->head);
>  	kfree(property);
>  }
> @@ -4307,7 +4312,7 @@ static void drm_property_free_blob(struct kref *kref)
>  
>  	list_del(&blob->head_global);
>  	list_del(&blob->head_file);
> -	drm_mode_object_put(blob->dev, &blob->base);
> +	drm_mode_object_unregister(blob->dev, &blob->base);
>  
>  	kfree(blob);
>  }
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 247dc8b..a78c138 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -33,8 +33,8 @@
>  
>  int drm_mode_object_get(struct drm_device *dev,
>  			struct drm_mode_object *obj, uint32_t obj_type);
> -void drm_mode_object_put(struct drm_device *dev,
> -			 struct drm_mode_object *object);
> +void drm_mode_object_unregister(struct drm_device *dev,
> +				struct drm_mode_object *object);
>  
>  /* drm_atomic.c */
>  int drm_atomic_get_property(struct drm_mode_object *obj,
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index f7448a5..7def3d5 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -98,7 +98,7 @@ void drm_mode_destroy(struct drm_device *dev, struct drm_display_mode *mode)
>  	if (!mode)
>  		return;
>  
> -	drm_mode_object_put(dev, &mode->base);
> +	drm_mode_object_unregister(dev, &mode->base);
>  
>  	kfree(mode);
>  }
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 02/15] drm/mode: move framebuffer_free up above framebuffer_init

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:33PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> A later patch will use it in framebuffer_init, and I want
> to keep the diff cleaner.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_crtc.c | 58 +++++++++++++++++++++++-----------------------
>  1 file changed, 29 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 21cb8dc..e69aac4 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -389,6 +389,35 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_mode_object_find);
>  
> +/* dev->mode_config.fb_lock must be held! */
> +static void __drm_framebuffer_unregister(struct drm_device *dev,
> +					 struct drm_framebuffer *fb)
> +{
> +	drm_mode_object_put(dev, &fb->base);
> +
> +	fb->base.id = 0;
> +}
> +
> +static void drm_framebuffer_free(struct kref *kref)
> +{
> +	struct drm_framebuffer *fb =
> +			container_of(kref, struct drm_framebuffer, refcount);
> +	struct drm_device *dev = fb->dev;
> +
> +	/*
> +	 * The lookup idr holds a weak reference, which has not necessarily been
> +	 * removed at this point. Check for that.
> +	 */
> +	mutex_lock(&dev->mode_config.fb_lock);
> +	if (fb->base.id) {
> +		/* Mark fb as reaped and drop idr ref. */
> +		__drm_framebuffer_unregister(dev, fb);
> +	}
> +	mutex_unlock(&dev->mode_config.fb_lock);
> +
> +	fb->funcs->destroy(fb);
> +}
> +
>  /**
>   * drm_framebuffer_init - initialize a framebuffer
>   * @dev: DRM device
> @@ -431,35 +460,6 @@ out:
>  }
>  EXPORT_SYMBOL(drm_framebuffer_init);
>  
> -/* dev->mode_config.fb_lock must be held! */
> -static void __drm_framebuffer_unregister(struct drm_device *dev,
> -					 struct drm_framebuffer *fb)
> -{
> -	drm_mode_object_put(dev, &fb->base);
> -
> -	fb->base.id = 0;
> -}
> -
> -static void drm_framebuffer_free(struct kref *kref)
> -{
> -	struct drm_framebuffer *fb =
> -			container_of(kref, struct drm_framebuffer, refcount);
> -	struct drm_device *dev = fb->dev;
> -
> -	/*
> -	 * The lookup idr holds a weak reference, which has not necessarily been
> -	 * removed at this point. Check for that.
> -	 */
> -	mutex_lock(&dev->mode_config.fb_lock);
> -	if (fb->base.id) {
> -		/* Mark fb as reaped and drop idr ref. */
> -		__drm_framebuffer_unregister(dev, fb);
> -	}
> -	mutex_unlock(&dev->mode_config.fb_lock);
> -
> -	fb->funcs->destroy(fb);
> -}
> -
>  static struct drm_framebuffer *__drm_framebuffer_lookup(struct drm_device *dev,
>  							uint32_t id)
>  {
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 03/15] drm/modes: drop __drm_framebuffer_unregister.

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:34PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> Just use the generic function.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Maybe mention in the commit message that a side effect of this is that we
now also protect fb->base.id (at least when we clear it) with the idr
mutex.

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

> ---
>  drivers/gpu/drm/drm_crtc.c | 16 ++--------------
>  1 file changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e69aac4..0ad1a92 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -389,15 +389,6 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_mode_object_find);
>  
> -/* dev->mode_config.fb_lock must be held! */
> -static void __drm_framebuffer_unregister(struct drm_device *dev,
> -					 struct drm_framebuffer *fb)
> -{
> -	drm_mode_object_put(dev, &fb->base);
> -
> -	fb->base.id = 0;
> -}
> -
>  static void drm_framebuffer_free(struct kref *kref)
>  {
>  	struct drm_framebuffer *fb =
> @@ -409,10 +400,7 @@ static void drm_framebuffer_free(struct kref *kref)
>  	 * removed at this point. Check for that.
>  	 */
>  	mutex_lock(&dev->mode_config.fb_lock);
> -	if (fb->base.id) {
> -		/* Mark fb as reaped and drop idr ref. */
> -		__drm_framebuffer_unregister(dev, fb);
> -	}
> +	drm_mode_object_unregister(dev, &fb->base);
>  	mutex_unlock(&dev->mode_config.fb_lock);
>  
>  	fb->funcs->destroy(fb);
> @@ -549,7 +537,7 @@ void drm_framebuffer_unregister_private(struct drm_framebuffer *fb)
>  
>  	mutex_lock(&dev->mode_config.fb_lock);
>  	/* Mark fb as reaped and drop idr ref. */
> -	__drm_framebuffer_unregister(dev, fb);
> +	drm_mode_object_unregister(dev, &fb->base);
>  	mutex_unlock(&dev->mode_config.fb_lock);
>  }
>  EXPORT_SYMBOL(drm_framebuffer_unregister_private);
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 04/15] drm/mode: introduce wrapper to read framebuffer refcount.

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:35PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> Avoids drivers knowing where the kref is stored.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

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

> ---
>  drivers/gpu/drm/drm_crtc.c          | 2 +-
>  drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
>  drivers/gpu/drm/msm/msm_fb.c        | 2 +-
>  drivers/gpu/drm/tegra/drm.c         | 2 +-
>  include/drm/drm_crtc.h              | 5 +++++
>  5 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 0ad1a92..8616737 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -612,7 +612,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
>  	 * in-use fb with fb-id == 0. Userspace is allowed to shoot its own foot
>  	 * in this manner.
>  	 */
> -	if (atomic_read(&fb->refcount.refcount) > 1) {
> +	if (drm_framebuffer_read_refcount(fb) > 1) {
>  		drm_modeset_lock_all(dev);
>  		/* remove from any CRTC */
>  		drm_for_each_crtc(crtc, dev) {
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index a0f1bd7..20d9300 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1908,7 +1908,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
>                           fbdev_fb->base.depth,
>                           fbdev_fb->base.bits_per_pixel,
>                           fbdev_fb->base.modifier[0],
> -                         atomic_read(&fbdev_fb->base.refcount.refcount));
> +                         drm_framebuffer_read_refcount(&fbdev_fb->base));
>                 describe_obj(m, fbdev_fb->obj);
>                 seq_putc(m, '\n');
>         }
> @@ -1926,7 +1926,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
>  			   fb->base.depth,
>  			   fb->base.bits_per_pixel,
>  			   fb->base.modifier[0],
> -			   atomic_read(&fb->base.refcount.refcount));
> +			   drm_framebuffer_read_refcount(&fb->base));
>  		describe_obj(m, fb->obj);
>  		seq_putc(m, '\n');
>  	}
> diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
> index a474d6c..17e0c9e 100644
> --- a/drivers/gpu/drm/msm/msm_fb.c
> +++ b/drivers/gpu/drm/msm/msm_fb.c
> @@ -77,7 +77,7 @@ void msm_framebuffer_describe(struct drm_framebuffer *fb, struct seq_file *m)
>  
>  	seq_printf(m, "fb: %dx%d@%4.4s (%2d, ID:%d)\n",
>  			fb->width, fb->height, (char *)&fb->pixel_format,
> -			fb->refcount.refcount.counter, fb->base.id);
> +			drm_framebuffer_read_refcount(fb), fb->base.id);
>  
>  	for (i = 0; i < n; i++) {
>  		seq_printf(m, "   %d: offset=%d pitch=%d, obj: ",
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 8e6b18c..2be88eb 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -878,7 +878,7 @@ static int tegra_debugfs_framebuffers(struct seq_file *s, void *data)
>  		seq_printf(s, "%3d: user size: %d x %d, depth %d, %d bpp, refcount %d\n",
>  			   fb->base.id, fb->width, fb->height, fb->depth,
>  			   fb->bits_per_pixel,
> -			   atomic_read(&fb->refcount.refcount));
> +			   drm_framebuffer_read_refcount(fb));
>  	}
>  
>  	mutex_unlock(&drm->mode_config.fb_lock);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index e0170bf..99a12f0 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -2608,6 +2608,11 @@ static inline uint32_t drm_color_lut_extract(uint32_t user_input,
>  	return clamp_val(val, 0, max);
>  }
>  
> +static inline uint32_t drm_framebuffer_read_refcount(struct drm_framebuffer *fb)
> +{
> +	return atomic_read(&fb->refcount.refcount);
> +}
> +
>  /* 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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 05/15] drm/mode: move framebuffer reference into object.

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:36PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This is the initial code to add references to some mode objects.
> In the future we need to start reference counting connectors so
> firstly I want to reorganise the code so the framebuffer ref counting
> uses the same paths.
> 
> This patch shouldn't change any functionality, just moves the kref.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 72 ++++++++++++++++++++++++----------------------
>  include/drm/drm_crtc.h     | 20 ++++++++++---
>  2 files changed, 54 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 8616737..75a45e9 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -275,7 +275,8 @@ EXPORT_SYMBOL(drm_get_format_name);
>  static int drm_mode_object_get_reg(struct drm_device *dev,
>  				   struct drm_mode_object *obj,
>  				   uint32_t obj_type,
> -				   bool register_obj)
> +				   bool register_obj,
> +				   void (*obj_free_cb)(struct kref *kref))
>  {
>  	int ret;
>  
> @@ -288,6 +289,10 @@ static int drm_mode_object_get_reg(struct drm_device *dev,
>  		 */
>  		obj->id = ret;
>  		obj->type = obj_type;
> +		if (obj_free_cb) {
> +			obj->free_cb = obj_free_cb;
> +			kref_init(&obj->refcount);
> +		}
>  	}
>  	mutex_unlock(&dev->mode_config.idr_mutex);
>  
> @@ -311,7 +316,7 @@ static int drm_mode_object_get_reg(struct drm_device *dev,
>  int drm_mode_object_get(struct drm_device *dev,
>  			struct drm_mode_object *obj, uint32_t obj_type)
>  {
> -	return drm_mode_object_get_reg(dev, obj, obj_type, true);
> +	return drm_mode_object_get_reg(dev, obj, obj_type, true, NULL);
>  }
>  
>  static void drm_mode_object_register(struct drm_device *dev,
> @@ -389,10 +394,35 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_mode_object_find);
>  

Kerneldoc for this one would be nice.

> +void drm_mode_object_unreference(struct drm_mode_object *obj)
> +{
> +	if (obj->free_cb) {
> +		DRM_DEBUG("OBJ ID: %d (%d)\n", obj->id, atomic_read(&obj->refcount.refcount));
> +		kref_put(&obj->refcount, obj->free_cb);
> +	}
> +}
> +EXPORT_SYMBOL(drm_mode_object_unreference);
> +
> +/**
> + * drm_mode_object_reference - incr the fb refcnt
> + * @obj: mode_object
> + *
> + * This function operates only on refcounted objects.
> + * This functions increments the object's refcount.
> + */
> +void drm_mode_object_reference(struct drm_mode_object *obj)
> +{
> +	if (obj->free_cb) {
> +		DRM_DEBUG("OBJ ID: %d (%d)\n", obj->id, atomic_read(&obj->refcount.refcount));
> +		kref_get(&obj->refcount);
> +	}
> +}
> +EXPORT_SYMBOL(drm_mode_object_reference);
> +
>  static void drm_framebuffer_free(struct kref *kref)
>  {
>  	struct drm_framebuffer *fb =
> -			container_of(kref, struct drm_framebuffer, refcount);
> +			container_of(kref, struct drm_framebuffer, base.refcount);
>  	struct drm_device *dev = fb->dev;
>  
>  	/*
> @@ -430,12 +460,12 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
>  	int ret;
>  
>  	mutex_lock(&dev->mode_config.fb_lock);
> -	kref_init(&fb->refcount);
>  	INIT_LIST_HEAD(&fb->filp_head);
>  	fb->dev = dev;
>  	fb->funcs = funcs;
>  
> -	ret = drm_mode_object_get(dev, &fb->base, DRM_MODE_OBJECT_FB);
> +	ret = drm_mode_object_get_reg(dev, &fb->base, DRM_MODE_OBJECT_FB,
> +				      true, drm_framebuffer_free);
>  	if (ret)
>  		goto out;
>  
> @@ -482,7 +512,7 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
>  	mutex_lock(&dev->mode_config.fb_lock);
>  	fb = __drm_framebuffer_lookup(dev, id);
>  	if (fb) {
> -		if (!kref_get_unless_zero(&fb->refcount))
> +		if (!kref_get_unless_zero(&fb->base.refcount))
>  			fb = NULL;
>  	}
>  	mutex_unlock(&dev->mode_config.fb_lock);
> @@ -492,32 +522,6 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
>  EXPORT_SYMBOL(drm_framebuffer_lookup);
>  
>  /**
> - * drm_framebuffer_unreference - unref a framebuffer
> - * @fb: framebuffer to unref
> - *
> - * This functions decrements the fb's refcount and frees it if it drops to zero.
> - */
> -void drm_framebuffer_unreference(struct drm_framebuffer *fb)
> -{
> -	DRM_DEBUG("%p: FB ID: %d (%d)\n", fb, fb->base.id, atomic_read(&fb->refcount.refcount));
> -	kref_put(&fb->refcount, drm_framebuffer_free);
> -}
> -EXPORT_SYMBOL(drm_framebuffer_unreference);
> -
> -/**
> - * drm_framebuffer_reference - incr the fb refcnt
> - * @fb: framebuffer
> - *
> - * This functions increments the fb's refcount.
> - */
> -void drm_framebuffer_reference(struct drm_framebuffer *fb)
> -{
> -	DRM_DEBUG("%p: FB ID: %d (%d)\n", fb, fb->base.id, atomic_read(&fb->refcount.refcount));
> -	kref_get(&fb->refcount);
> -}
> -EXPORT_SYMBOL(drm_framebuffer_reference);
> -
> -/**
>   * drm_framebuffer_unregister_private - unregister a private fb from the lookup idr
>   * @fb: fb to unregister
>   *
> @@ -902,7 +906,7 @@ 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);
> +	ret = drm_mode_object_get_reg(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR, false, NULL);
>  	if (ret)
>  		goto out_unlock;
>  
> @@ -5922,7 +5926,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>  	 */
>  	WARN_ON(!list_empty(&dev->mode_config.fb_list));
>  	list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
> -		drm_framebuffer_free(&fb->refcount);
> +		drm_framebuffer_free(&fb->base.refcount);
>  	}
>  
>  	list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 99a12f0..576faf4 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -59,6 +59,8 @@ struct drm_mode_object {
>  	uint32_t id;
>  	uint32_t type;
>  	struct drm_object_properties *properties;
> +	struct kref refcount;
> +	void (*free_cb)(struct kref *kref);
>  };
>  
>  #define DRM_OBJECT_MAX_PROPERTY 24
> @@ -233,8 +235,8 @@ struct drm_framebuffer {
>  	 * should be deferred.  In cases like this, the driver would like to
>  	 * hold a ref to the fb even though it has already been removed from
>  	 * userspace perspective.

Bikeshed: Maybe new paragraph here for better formatting with kerneldoc.

> +	 * The refcount is stored inside the mode object.
>  	 */
> -	struct kref refcount;
>  	/*
>  	 * Place on the dev->mode_config.fb_list, access protected by
>  	 * dev->mode_config.fb_lock.
> @@ -2386,8 +2388,6 @@ extern int drm_framebuffer_init(struct drm_device *dev,
>  				const struct drm_framebuffer_funcs *funcs);
>  extern struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
>  						      uint32_t id);
> -extern void drm_framebuffer_unreference(struct drm_framebuffer *fb);
> -extern void drm_framebuffer_reference(struct drm_framebuffer *fb);
>  extern void drm_framebuffer_remove(struct drm_framebuffer *fb);
>  extern void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
>  extern void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
> @@ -2445,6 +2445,8 @@ extern int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
>  					 int gamma_size);
>  extern struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
>  		uint32_t id, uint32_t type);
> +void drm_mode_object_reference(struct drm_mode_object *obj);
> +void drm_mode_object_unreference(struct drm_mode_object *obj);
>  
>  /* IOCTLs */
>  extern int drm_mode_getresources(struct drm_device *dev,
> @@ -2608,9 +2610,19 @@ static inline uint32_t drm_color_lut_extract(uint32_t user_input,
>  	return clamp_val(val, 0, max);
>  }
>  
> +static inline void drm_framebuffer_reference(struct drm_framebuffer *fb)
> +{
> +	drm_mode_object_reference(&fb->base);
> +}
> +
> +static inline void drm_framebuffer_unreference(struct drm_framebuffer *fb)
> +{
> +	drm_mode_object_unreference(&fb->base);
> +}

You lost the kerneldoc for the above two.

With the kerneldoc commments above addressed:

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


> +
>  static inline uint32_t drm_framebuffer_read_refcount(struct drm_framebuffer *fb)
>  {
> -	return atomic_read(&fb->refcount.refcount);
> +	return atomic_read(&fb->base.refcount.refcount);
>  }
>  
>  /* Plane list iterator for legacy (overlay only) planes. */
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 06/15] drm/mode: use _object_find to find framebuffers.

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:37PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> No point have this code dupliated at this point, use the
> _object_find code instead now.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

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

> ---
>  drivers/gpu/drm/drm_crtc.c | 35 ++++++++++-------------------------
>  1 file changed, 10 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 75a45e9..0d75517 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -362,8 +362,7 @@ static struct drm_mode_object *_object_find(struct drm_device *dev,
>  		obj = NULL;
>  	/* don't leak out unref'd fb's */
>  	if (obj &&
> -	    (obj->type == DRM_MODE_OBJECT_FB ||
> -	     obj->type == DRM_MODE_OBJECT_BLOB))
> +	    obj->type == DRM_MODE_OBJECT_BLOB)
>  		obj = NULL;
>  	mutex_unlock(&dev->mode_config.idr_mutex);
>  
> @@ -478,23 +477,6 @@ out:
>  }
>  EXPORT_SYMBOL(drm_framebuffer_init);
>  
> -static struct drm_framebuffer *__drm_framebuffer_lookup(struct drm_device *dev,
> -							uint32_t id)
> -{
> -	struct drm_mode_object *obj = NULL;
> -	struct drm_framebuffer *fb;
> -
> -	mutex_lock(&dev->mode_config.idr_mutex);
> -	obj = idr_find(&dev->mode_config.crtc_idr, id);
> -	if (!obj || (obj->type != DRM_MODE_OBJECT_FB) || (obj->id != id))
> -		fb = NULL;
> -	else
> -		fb = obj_to_fb(obj);
> -	mutex_unlock(&dev->mode_config.idr_mutex);
> -
> -	return fb;
> -}
> -
>  /**
>   * drm_framebuffer_lookup - look up a drm framebuffer and grab a reference
>   * @dev: drm device
> @@ -507,11 +489,13 @@ static struct drm_framebuffer *__drm_framebuffer_lookup(struct drm_device *dev,
>  struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
>  					       uint32_t id)
>  {
> -	struct drm_framebuffer *fb;
> +	struct drm_mode_object *obj;
> +	struct drm_framebuffer *fb = NULL;
>  
>  	mutex_lock(&dev->mode_config.fb_lock);
> -	fb = __drm_framebuffer_lookup(dev, id);
> -	if (fb) {
> +	obj = _object_find(dev, id, DRM_MODE_OBJECT_FB);
> +	if (obj) {
> +		fb = obj_to_fb(obj);
>  		if (!kref_get_unless_zero(&fb->base.refcount))
>  			fb = NULL;
>  	}
> @@ -3449,6 +3433,7 @@ int drm_mode_rmfb(struct drm_device *dev,
>  {
>  	struct drm_framebuffer *fb = NULL;
>  	struct drm_framebuffer *fbl = NULL;
> +	struct drm_mode_object *obj;
>  	uint32_t *id = data;
>  	int found = 0;
>  
> @@ -3457,10 +3442,10 @@ int drm_mode_rmfb(struct drm_device *dev,
>  
>  	mutex_lock(&file_priv->fbs_lock);
>  	mutex_lock(&dev->mode_config.fb_lock);
> -	fb = __drm_framebuffer_lookup(dev, *id);
> -	if (!fb)
> +	obj = _object_find(dev, *id, DRM_MODE_OBJECT_FB);
> +	if (!obj)
>  		goto fail_lookup;
> -
> +	fb = obj_to_fb(obj);
>  	list_for_each_entry(fbl, &file_priv->fbs, filp_head)
>  		if (fb == fbl)
>  			found = 1;
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 07/15] drm/mode: reduce scope of fb_lock in framebuffer init

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:38PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> We don't need to hold the fb lock around the initialisation,
> only around the list manipulaton.
> 
> So do the lock hold only around the register for now.
> Signed-off-by: Dave Airlie <airlied@redhat.com>

This needs a bit more explanation added:

"Previously fb refcounting, and especially the weak reference
(kref_get_unless_zero) used in fb lookups have been protected by fb_lock.
But with the refactoring to share refcounting in the drm_mode_object base
class that switched to being protected by idr_mutex, which means fb_lock
critical sections can be reduced."

I also double-checked that we don't have any outdated comments that point
at the wrong lock, and didn't find any.

With the commit message augmented:

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

> ---
>  drivers/gpu/drm/drm_crtc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 0d75517..1863879 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -458,21 +458,22 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
>  {
>  	int ret;
>  
> -	mutex_lock(&dev->mode_config.fb_lock);
>  	INIT_LIST_HEAD(&fb->filp_head);
>  	fb->dev = dev;
>  	fb->funcs = funcs;
>  
>  	ret = drm_mode_object_get_reg(dev, &fb->base, DRM_MODE_OBJECT_FB,
> -				      true, drm_framebuffer_free);
> +				      false, drm_framebuffer_free);
>  	if (ret)
>  		goto out;
>  
> +	mutex_lock(&dev->mode_config.fb_lock);
>  	dev->mode_config.num_fb++;
>  	list_add(&fb->head, &dev->mode_config.fb_list);
> -out:
> -	mutex_unlock(&dev->mode_config.fb_lock);
>  
> +	drm_mode_object_register(dev, &fb->base);
> +	mutex_unlock(&dev->mode_config.fb_lock);
> +out:
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_framebuffer_init);
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 08/15] drm/mode: reduce lock hold in addfb2

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:39PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> No need to hold the lock while assigning the variable.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Not sure why exactly I put that under the lock, but the only thing that
can race here is rmfb while addfb2 is still doing it's thing, with a
correctly guess (easy to do since they're fully deterministic) fb_id.

But that race can't happen because rmfb checks whether the fb is
associated with the drm_file, and if not bails out. And since
mutex_lock/unlock together are a full barrier gcc can't even reorder
things so that it would be possible to return a 0 fb_id.

I think I convinced myself that this is totally safe ;-)

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

> ---
>  drivers/gpu/drm/drm_crtc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 1863879..21cb998 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3405,11 +3405,11 @@ int drm_mode_addfb2(struct drm_device *dev,
>  	if (IS_ERR(fb))
>  		return PTR_ERR(fb);
>  
> -	/* Transfer ownership to the filp for reaping on close */
> -
>  	DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);
> -	mutex_lock(&file_priv->fbs_lock);
>  	r->fb_id = fb->base.id;
> +
> +	/* Transfer ownership to the filp for reaping on close */
> +	mutex_lock(&file_priv->fbs_lock);
>  	list_add(&fb->filp_head, &file_priv->fbs);
>  	mutex_unlock(&file_priv->fbs_lock);
>  
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 09/15] drm/modes: move reference taking into object lookup.

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:40PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> When we lookup an ref counted object we now take a proper reference
> using kref_get_unless_zero.
> 
> Framebuffer lookup no longer needs do this itself.
> 
> Convert rmfb to using framebuffer lookup and deal with the fact
> it now gets an extra reference that we have to cleanup. This should
> mean we can avoid holding fb_lock across rmfb. (if I'm wrong let me
> know).
> 
> We also now only hold the fbs_lock around the list manipulation.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Needs the same comment as patch 7 added to the commit message:

"Previously fb refcounting, and especially the weak reference
(kref_get_unless_zero) used in fb lookups have been protected by fb_lock.
But with the refactoring to share refcounting in the drm_mode_object base
class that switched to being protected by idr_mutex, which means fb_lock
critical sections can be reduced."

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

> ---
>  drivers/gpu/drm/drm_crtc.c | 37 ++++++++++++++++++++-----------------
>  1 file changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 21cb998..e47c4a2 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -364,6 +364,11 @@ static struct drm_mode_object *_object_find(struct drm_device *dev,
>  	if (obj &&
>  	    obj->type == DRM_MODE_OBJECT_BLOB)
>  		obj = NULL;
> +
> +	if (obj && obj->free_cb) {
> +		if (!kref_get_unless_zero(&obj->refcount))
> +			obj = NULL;
> +	}
>  	mutex_unlock(&dev->mode_config.idr_mutex);
>  
>  	return obj;
> @@ -495,11 +500,8 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
>  
>  	mutex_lock(&dev->mode_config.fb_lock);
>  	obj = _object_find(dev, id, DRM_MODE_OBJECT_FB);
> -	if (obj) {
> +	if (obj)
>  		fb = obj_to_fb(obj);
> -		if (!kref_get_unless_zero(&fb->base.refcount))
> -			fb = NULL;
> -	}
>  	mutex_unlock(&dev->mode_config.fb_lock);
>  
>  	return fb;
> @@ -3434,37 +3436,38 @@ int drm_mode_rmfb(struct drm_device *dev,
>  {
>  	struct drm_framebuffer *fb = NULL;
>  	struct drm_framebuffer *fbl = NULL;
> -	struct drm_mode_object *obj;
>  	uint32_t *id = data;
>  	int found = 0;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
>  
> +	fb = drm_framebuffer_lookup(dev, *id);
> +	if (!fb)
> +		return -ENOENT;
> +
>  	mutex_lock(&file_priv->fbs_lock);
> -	mutex_lock(&dev->mode_config.fb_lock);
> -	obj = _object_find(dev, *id, DRM_MODE_OBJECT_FB);
> -	if (!obj)
> -		goto fail_lookup;
> -	fb = obj_to_fb(obj);
>  	list_for_each_entry(fbl, &file_priv->fbs, filp_head)
>  		if (fb == fbl)
>  			found = 1;
> -	if (!found)
> -		goto fail_lookup;
> +	if (!found) {
> +		mutex_unlock(&file_priv->fbs_lock);
> +		goto fail_unref;
> +	}
>  
>  	list_del_init(&fb->filp_head);
> -	mutex_unlock(&dev->mode_config.fb_lock);
>  	mutex_unlock(&file_priv->fbs_lock);
>  
> +	/* we now own the reference that was stored in the fbs list */
>  	drm_framebuffer_unreference(fb);
>  
> -	return 0;
> +	/* drop the reference we picked up in framebuffer lookup */
> +	drm_framebuffer_unreference(fb);
>  
> -fail_lookup:
> -	mutex_unlock(&dev->mode_config.fb_lock);
> -	mutex_unlock(&file_priv->fbs_lock);
> +	return 0;
>  
> +fail_unref:
> +	drm_framebuffer_unreference(fb);
>  	return -ENOENT;
>  }
>  
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 10/15] drm/modes: reduce fb_lock to just protecting lists

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:41PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This reduces the fb_lock to just protecting the num_fb/fb_list.
> 
> I'd like to have some discussion on if this opens up any race
> conditions.

Here's you're discussion ;-)

"Previously fb refcounting, and especially the weak reference
(kref_get_unless_zero) used in fb lookups have been protected by fb_lock.
But with the refactoring to share refcounting in the drm_mode_object base
class that switched to being protected by idr_mutex, which means fb_lock
critical sections can be reduced."

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

> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e47c4a2..46f32f2 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -433,9 +433,7 @@ static void drm_framebuffer_free(struct kref *kref)
>  	 * The lookup idr holds a weak reference, which has not necessarily been
>  	 * removed at this point. Check for that.
>  	 */
> -	mutex_lock(&dev->mode_config.fb_lock);
>  	drm_mode_object_unregister(dev, &fb->base);
> -	mutex_unlock(&dev->mode_config.fb_lock);
>  
>  	fb->funcs->destroy(fb);
>  }
> @@ -475,9 +473,9 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
>  	mutex_lock(&dev->mode_config.fb_lock);
>  	dev->mode_config.num_fb++;
>  	list_add(&fb->head, &dev->mode_config.fb_list);
> +	mutex_unlock(&dev->mode_config.fb_lock);
>  
>  	drm_mode_object_register(dev, &fb->base);
> -	mutex_unlock(&dev->mode_config.fb_lock);
>  out:
>  	return ret;
>  }
> @@ -498,12 +496,9 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
>  	struct drm_mode_object *obj;
>  	struct drm_framebuffer *fb = NULL;
>  
> -	mutex_lock(&dev->mode_config.fb_lock);
>  	obj = _object_find(dev, id, DRM_MODE_OBJECT_FB);
>  	if (obj)
>  		fb = obj_to_fb(obj);
> -	mutex_unlock(&dev->mode_config.fb_lock);
> -
>  	return fb;
>  }
>  EXPORT_SYMBOL(drm_framebuffer_lookup);
> @@ -526,10 +521,8 @@ void drm_framebuffer_unregister_private(struct drm_framebuffer *fb)
>  
>  	dev = fb->dev;
>  
> -	mutex_lock(&dev->mode_config.fb_lock);
>  	/* Mark fb as reaped and drop idr ref. */
>  	drm_mode_object_unregister(dev, &fb->base);
> -	mutex_unlock(&dev->mode_config.fb_lock);
>  }
>  EXPORT_SYMBOL(drm_framebuffer_unregister_private);
>  
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 11/15] drm/modes: stop handling framebuffer special

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:42PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> Since ref counting is in the object now we can just call the
> normal interfaces.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

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

> ---
>  drivers/gpu/drm/drm_crtc.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 46f32f2..f6bf828 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4810,19 +4810,7 @@ bool drm_property_change_valid_get(struct drm_property *property,
>  		if (value == 0)
>  			return true;
>  
> -		/* handle refcnt'd objects specially: */
> -		if (property->values[0] == DRM_MODE_OBJECT_FB) {
> -			struct drm_framebuffer *fb;
> -			fb = drm_framebuffer_lookup(property->dev, value);
> -			if (fb) {
> -				*ref = &fb->base;
> -				return true;
> -			} else {
> -				return false;
> -			}
> -		} else {
> -			return _object_find(property->dev, value, property->values[0]) != NULL;
> -		}
> +		return _object_find(property->dev, value, property->values[0]) != NULL;
>  	}
>  
>  	for (i = 0; i < property->num_values; i++)
> @@ -4838,8 +4826,7 @@ void drm_property_change_valid_put(struct drm_property *property,
>  		return;
>  
>  	if (drm_property_type_is(property, DRM_MODE_PROP_OBJECT)) {
> -		if (property->values[0] == DRM_MODE_OBJECT_FB)
> -			drm_framebuffer_unreference(obj_to_fb(ref));
> +		drm_mode_object_unreference(ref);
>  	} else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB))
>  		drm_property_unreference_blob(obj_to_blob(ref));
>  }
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: drm reference counter connectors and fix lifetimes

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:31PM +1000, Dave Airlie wrote:
> I've been trolled since I did MST that I really didn't do a good job
> on the connector lifetimes, so I finally felt guilty and had time to try
> and fix this up.
> 
> This is a set of patches to handle connector lifetimes so that the connectors
> don't go away in the middle of us doing something. I've done some testing
> on this with kms_flip on my haswell laptop while undocking etc.
> 
> Daniel has been pestering me to finish it off, so I've cleaned up the last
> few things in the mst patches at least for i915 and decided to send it out.

Ok, reviewed all the refcounting prep work. Just minor stuff, so I guess
you'll pull them into drm-next directly after polishing?

I have a bunch of pull requests I'll send out today, so could do a
drm-next day.

For the actual drm_connector refcounting I'd like to soak the prep patches
a bit first, and I also need to think a bit more about those ;-)

Cheers, Daniel
-- 
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

Re: [PATCH 13/15] drm: take references to connectors used in a modeset.

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:44PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> As suggested by Daniel, if we are actively using the connector in a modeset
> we don't want it to disappear from underneath us. This takes a reference
> to the connector in the atomic paths when we are setting the state up,
> and in the non-atomic paths when binding the encoder.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_atomic.c      | 11 ++++++++++-
>  drivers/gpu/drm/drm_crtc_helper.c |  6 ++++++
>  2 files changed, 16 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);

I'm super paranoid about the refcounting for modesets, since at least with
fbs that was where we had endless amounts of pain and random bugs.

- I think we should split this patch into atomic and legacy parts, since
  the code-paths are completely different.

- I'm not sure on the atomic version. I think conceptually it would be
  even cleaner for atomic to say that drm_connector_state holds a ref on
  the connector iff the crtc pointer is set. This would mean we grab the
  reference also in duplicate_state and drop it in destroy_state (and ofc
  update it in set_crtc_for_connector). It's a bit funny semantics, but by
  attaching the refcounting to the lifetime of the state struct we fully
  align with how refcounting is done for everything else in atomic. And
  avoiding special cases in refcounting is good imo.

  Also since we know that the state structures are tracked correctly I
  wouldn't have to think about correctness at all, makes the review easier
  ;-)

Cheers, Daniel

> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 79555d2..71b6c72 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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 14/15] drm/i915/mst: use reference counted connectors.

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:45PM +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.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Looks correct, but two comments for better api for shared helpers inline below.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 46 ++++++++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_drv.h    |  2 +-
>  2 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index a2bd698..2e730b6 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -113,7 +113,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) {
> @@ -138,10 +138,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);
> @@ -181,7 +183,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);
> @@ -199,7 +203,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");
> @@ -248,7 +252,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;
>  }
> @@ -312,6 +316,11 @@ static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector)
>  	struct edid *edid;
>  	int ret;
>  
> +	if (!intel_connector->port || !intel_dp) {
> +		ret = intel_connector_update_modes(connector, NULL);
> +		return ret;
> +	}
> +
>  	edid = drm_dp_mst_get_edid(connector, &intel_dp->mst_mgr, intel_connector->port);
>  	if (!edid)
>  		return 0;
> @@ -328,6 +337,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_connector->port || !intel_dp)
> +		return connector_status_disconnected;
>  	return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr, intel_connector->port);

Should we push this change and the preceeding one into dp helpers, i.e.
make them cope with a null port? Otoh more work to fish out the
->mst_mgr, so not sure.

Hm ... isn't the mst_mgr a redundant argument, and we could figure that
out purely from the port? Would be a bit of refactoring, but I think the
shared code for this crucial bit of semantics (there's no way the mst port
is ever connected if it's unplugged) would be good.

>  }
>  
> @@ -393,6 +404,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;
>  }
>  
> @@ -400,6 +413,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,29 +521,14 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
>  	struct drm_device *dev = connector->dev;
>  
> -	/* 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);
> -	}
> -	drm_modeset_unlock_all(dev);
> -
>  	intel_connector->unregister(intel_connector);
>  
>  	drm_modeset_lock_all(dev);
>  	intel_connector_remove_from_fbdev(intel_connector);
> -	drm_connector_cleanup(connector);
> +	intel_connector->mst_port = NULL;
>  	drm_modeset_unlock_all(dev);

Hm, ugly that we still need to grab modeset locks here. I'd like to avoid
that, since it could be a major stall. Maybe we need to have a separate
lock for the fbdev connector list, and then push the locking for that
(including refcounting) down into fbdev helpers?

>  
> -	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 4c027d6..0d2360e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -831,7 +831,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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 15/15] drm/radeon/dp_mst: use connector ref counting

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:46PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> Use connector reference counting in radeon mst code.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/radeon/radeon_dp_mst.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> index 43cffb5..4f1792a 100644
> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> @@ -242,6 +242,7 @@ radeon_dp_mst_connector_destroy(struct drm_connector *connector)
>  
>  	drm_encoder_cleanup(&radeon_encoder->base);
>  	kfree(radeon_encoder);
> +
>  	drm_connector_cleanup(connector);
>  	kfree(radeon_connector);
>  }
> @@ -313,11 +314,9 @@ static void radeon_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  	drm_modeset_lock_all(dev);
>  	/* dpms off */
>  	radeon_fb_remove_connector(rdev, connector);

Why not drop the force-off here, like for i915?
-Daniel

> -
> -	drm_connector_cleanup(connector);
>  	drm_modeset_unlock_all(dev);
>  
> -	kfree(connector);
> +	drm_connector_unreference(connector);
>  	DRM_DEBUG_KMS("\n");
>  }
>  
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 12/15] drm/modes: add connector reference counting.

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:43PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This uses the previous changes to add reference counting to the
> drm connector objects.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Bunch of nitpicks in this one. And I noticed a pretty serious issue we
have with the generic prop code since forever. I'll write testcases and
bugfixes for that (hopefully today).

> ---
>  drivers/gpu/drm/drm_atomic.c    | 19 +++++++++++++++++--
>  drivers/gpu/drm/drm_crtc.c      | 39 ++++++++++++++++++++++++++++++---------
>  drivers/gpu/drm/drm_fb_helper.c | 38 ++++++--------------------------------
>  include/drm/drm_crtc.h          | 13 ++++++++++++-
>  4 files changed, 65 insertions(+), 44 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);

Could we maybe do something like this to share the unreference calls:

		continue;
fail_loop:
		drm_mode_object_unreference(obj);
		goto out;

And then change all the goto out; in the loop to goto fail_loop;? Just a
suggestion, either is fine with me. I just like common exit code a lot.

>  	}
>  
>  	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 f6bf828..90bc597 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -861,6 +861,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
> @@ -886,7 +896,7 @@ 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);

Too long line.

>  	if (ret)
>  		goto out_unlock;
>  
> @@ -2106,7 +2116,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;
> @@ -2190,6 +2200,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);
>  
> @@ -2834,13 +2845,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);
> @@ -2868,6 +2880,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);
> @@ -4957,7 +4975,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,

Preexisting bug, but we don't filter out FB and BLOB properties here.
Which means at least we'll blow up on the WARN_ON, but I think we can also
nicely leak references with your patches now (because at least fb
references are grabbed in _object_find now) :(

We do filter out FB and BLOB obj with the obj->properties check, but
that's a use-after-free in 4.6 (and the ioctl will leak with your patches
now in 4.7).

>  	}
>  	if (!obj->properties) {
>  		ret = -EINVAL;
> -		goto out;
> +		goto out_unref;
>  	}
>  
>  	ret = get_properties(obj, file_priv->atomic,
> @@ -4965,6 +4983,8 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>  			(uint64_t __user *)(unsigned long)(arg->prop_values_ptr),
>  			&arg->count_props);
>  
> +out_unref:
> +	drm_mode_object_unreference(obj);
>  out:
>  	drm_modeset_unlock_all(dev);
>  	return ret;
> @@ -5007,25 +5027,25 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,

Same issue with FB and BLOB here as with get_property_ioctl.

>  		goto out;
>  	}
>  	if (!arg_obj->properties)
> -		goto out;
> +		goto out_unref;
>  
>  	for (i = 0; i < arg_obj->properties->count; i++)
>  		if (arg_obj->properties->properties[i]->base.id == arg->prop_id)
>  			break;
>  
>  	if (i == arg_obj->properties->count)
> -		goto out;
> +		goto out_unref;
>  
>  	prop_obj = drm_mode_object_find(dev, arg->prop_id,
>  					DRM_MODE_OBJECT_PROPERTY);
>  	if (!prop_obj) {
>  		ret = -ENOENT;
> -		goto out;
> +		goto out_unref;
>  	}
>  	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:
> @@ -5042,7 +5062,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>  	}
>  
>  	drm_property_change_valid_put(property, ref);
> -
> +out_unref:
> +	drm_mode_object_unreference(arg_obj);
>  out:
>  	drm_modeset_unlock_all(dev);
>  	return ret;
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 855108e..3691565 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);

I think it'd be good to split the changes for the fbdev helper into a
separate patch, for easier reviewing, and it seems inconsistent: You grab
a ref here, but don't drop it in remove_one_connector.

Also I think we need to grab references for all the intial connectors too,
and drop them on unload, in case you boot up (or reload) with an mst
connector plugged in. You do some of that in drm_setup_crtcs, but I think
it's better to do that in drm_fb_helper_single_add_all_connectors and the
final cleanup code, and not have any refcounting in drm_setup_crtc (but
instead just inherit the refcount from the connector_info array for the
fbdev modeset config structures).

>  	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)
>  {
> @@ -213,10 +186,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);
> @@ -2024,7 +1993,11 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>  	/* need to set the modesets up here for use later */
>  	/* fill out the connector<->crtc mappings into the modesets */
>  	for (i = 0; i < fb_helper->crtc_count; i++) {
> +		int j;
>  		modeset = &fb_helper->crtc_info[i].mode_set;
> +
> +		for (j = 0; j < modeset->num_connectors; j++)
> +			drm_connector_unreference(modeset->connectors[j]);
>  		modeset->num_connectors = 0;
>  		modeset->fb = NULL;
>  	}
> @@ -2045,6 +2018,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>  				drm_mode_destroy(dev, modeset->mode);
>  			modeset->mode = drm_mode_duplicate(dev,
>  							   fb_crtc->desired_mode);
> +			drm_connector_reference(fb_helper->connector_info[i]->connector);
>  			modeset->connectors[modeset->num_connectors++] = fb_helper->connector_info[i]->connector;
>  			modeset->fb = fb_helper->fb;
>  			modeset->x = offset->x;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 576faf4..7e13127 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -2579,7 +2579,8 @@ 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,
> +/* takes a reference */
> +static inline struct drm_connector *drm_connector_lookup(struct drm_device *dev,
>  		uint32_t id)
>  {
>  	struct drm_mode_object *mo;
> @@ -2625,6 +2626,16 @@ static inline uint32_t drm_framebuffer_read_refcount(struct drm_framebuffer *fb)
>  	return atomic_read(&fb->base.refcount.refcount);
>  }
>  
> +static inline void drm_connector_reference(struct drm_connector *connector)
> +{
> +	drm_mode_object_reference(&connector->base);
> +}
> +
> +static inline void drm_connector_unreference(struct drm_connector *connector)

kerneldoc for these two plus drm_connector_lookup would be good with the
usual few lines about the refcounting. That also gives you a good place to
put the "takes a reference" comment above.

> +{
> +	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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 13/15] drm: take references to connectors used in a modeset.

[Dave Airlie]

On 22 April 2016 at 18:49, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Apr 15, 2016 at 03:10:44PM +1000, Dave Airlie wrote:
>> From: Dave Airlie <airlied@redhat.com>
>>
>> As suggested by Daniel, if we are actively using the connector in a modeset
>> we don't want it to disappear from underneath us. This takes a reference
>> to the connector in the atomic paths when we are setting the state up,
>> and in the non-atomic paths when binding the encoder.
>>
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c      | 11 ++++++++++-
>>  drivers/gpu/drm/drm_crtc_helper.c |  6 ++++++
>>  2 files changed, 16 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);
>
> I'm super paranoid about the refcounting for modesets, since at least with
> fbs that was where we had endless amounts of pain and random bugs.
>
> - I think we should split this patch into atomic and legacy parts, since
>   the code-paths are completely different.
>
> - I'm not sure on the atomic version. I think conceptually it would be
>   even cleaner for atomic to say that drm_connector_state holds a ref on
>   the connector iff the crtc pointer is set. This would mean we grab the
>   reference also in duplicate_state and drop it in destroy_state (and ofc
>   update it in set_crtc_for_connector). It's a bit funny semantics, but by
>   attaching the refcounting to the lifetime of the state struct we fully
>   align with how refcounting is done for everything else in atomic. And
>   avoiding special cases in refcounting is good imo.
>
>   Also since we know that the state structures are tracked correctly I
>   wouldn't have to think about correctness at all, makes the review easier
>   ;-)

I'm okay on splitting things, just not sure what you want the atomic
thing to look like.

I'll repost a bit of the series and you can tell me if you agree!

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

Re: [PATCH 14/15] drm/i915/mst: use reference counted connectors.

[Dave Airlie]

On 22 April 2016 at 19:03, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Apr 15, 2016 at 03:10:45PM +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.
>>
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>
> Looks correct, but two comments for better api for shared helpers inline below.
> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/intel_dp_mst.c | 46 ++++++++++++++++++-------------------
>>  drivers/gpu/drm/i915/intel_drv.h    |  2 +-
>>  2 files changed, 24 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
>> index a2bd698..2e730b6 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
>> @@ -113,7 +113,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) {
>> @@ -138,10 +138,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);
>> @@ -181,7 +183,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);
>> @@ -199,7 +203,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");
>> @@ -248,7 +252,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;
>>  }
>> @@ -312,6 +316,11 @@ static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector)
>>       struct edid *edid;
>>       int ret;
>>
>> +     if (!intel_connector->port || !intel_dp) {
>> +             ret = intel_connector_update_modes(connector, NULL);
>> +             return ret;
>> +     }
>> +
>>       edid = drm_dp_mst_get_edid(connector, &intel_dp->mst_mgr, intel_connector->port);
>>       if (!edid)
>>               return 0;
>> @@ -328,6 +337,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_connector->port || !intel_dp)
>> +             return connector_status_disconnected;
>>       return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr, intel_connector->port);
>
> Should we push this change and the preceeding one into dp helpers, i.e.
> make them cope with a null port? Otoh more work to fish out the
> ->mst_mgr, so not sure.

Actually they cope with a NULL port fine, so we can drop a bit of
those two hunks,

>
> Hm ... isn't the mst_mgr a redundant argument, and we could figure that
> out purely from the port? Would be a bit of refactoring, but I think the
> shared code for this crucial bit of semantics (there's no way the mst port
> is ever connected if it's unplugged) would be good.

No the port isn't a reference counted object, it's just a point we
revalidate in the
mst code. The MST manager does't disappear. So you can't get from port
to anything
else until you check it's valid. You can't reference count port on the
connector or
else you just end up with circular references between the two.


>> -     drm_modeset_unlock_all(dev);
>> -
>>       intel_connector->unregister(intel_connector);
>>
>>       drm_modeset_lock_all(dev);
>>       intel_connector_remove_from_fbdev(intel_connector);
>> -     drm_connector_cleanup(connector);
>> +     intel_connector->mst_port = NULL;
>>       drm_modeset_unlock_all(dev);
>
> Hm, ugly that we still need to grab modeset locks here. I'd like to avoid
> that, since it could be a major stall. Maybe we need to have a separate
> lock for the fbdev connector list, and then push the locking for that
> (including refcounting) down into fbdev helpers?

If you plug out an MST device a stall isn't going to be a major worry, you
are going to get a modeset most likely straight away.

So I think we should only address this problem when we have this problem.

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

Re: [PATCH 14/15] drm/i915/mst: use reference counted connectors.

[Daniel Vetter]

On Wed, Apr 27, 2016 at 11:54:35AM +1000, Dave Airlie wrote:
> On 22 April 2016 at 19:03, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Apr 15, 2016 at 03:10:45PM +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.
> >>
> >> Signed-off-by: Dave Airlie <airlied@redhat.com>
> >
> > Looks correct, but two comments for better api for shared helpers inline below.
> > -Daniel
> >
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp_mst.c | 46 ++++++++++++++++++-------------------
> >>  drivers/gpu/drm/i915/intel_drv.h    |  2 +-
> >>  2 files changed, 24 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> >> index a2bd698..2e730b6 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> >> @@ -113,7 +113,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) {
> >> @@ -138,10 +138,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);
> >> @@ -181,7 +183,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);
> >> @@ -199,7 +203,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");
> >> @@ -248,7 +252,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;
> >>  }
> >> @@ -312,6 +316,11 @@ static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector)
> >>       struct edid *edid;
> >>       int ret;
> >>
> >> +     if (!intel_connector->port || !intel_dp) {
> >> +             ret = intel_connector_update_modes(connector, NULL);
> >> +             return ret;
> >> +     }
> >> +
> >>       edid = drm_dp_mst_get_edid(connector, &intel_dp->mst_mgr, intel_connector->port);
> >>       if (!edid)
> >>               return 0;
> >> @@ -328,6 +337,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_connector->port || !intel_dp)
> >> +             return connector_status_disconnected;
> >>       return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr, intel_connector->port);
> >
> > Should we push this change and the preceeding one into dp helpers, i.e.
> > make them cope with a null port? Otoh more work to fish out the
> > ->mst_mgr, so not sure.
> 
> Actually they cope with a NULL port fine, so we can drop a bit of
> those two hunks,
> 
> >
> > Hm ... isn't the mst_mgr a redundant argument, and we could figure that
> > out purely from the port? Would be a bit of refactoring, but I think the
> > shared code for this crucial bit of semantics (there's no way the mst port
> > is ever connected if it's unplugged) would be good.
> 
> No the port isn't a reference counted object, it's just a point we
> revalidate in the
> mst code. The MST manager does't disappear. So you can't get from port
> to anything
> else until you check it's valid. You can't reference count port on the
> connector or
> else you just end up with circular references between the two.

Yeah I think this is beyond my understanding of the mst helpers. Really
should fix that sometimes ;-)

> >> -     drm_modeset_unlock_all(dev);
> >> -
> >>       intel_connector->unregister(intel_connector);
> >>
> >>       drm_modeset_lock_all(dev);
> >>       intel_connector_remove_from_fbdev(intel_connector);
> >> -     drm_connector_cleanup(connector);
> >> +     intel_connector->mst_port = NULL;
> >>       drm_modeset_unlock_all(dev);
> >
> > Hm, ugly that we still need to grab modeset locks here. I'd like to avoid
> > that, since it could be a major stall. Maybe we need to have a separate
> > lock for the fbdev connector list, and then push the locking for that
> > (including refcounting) down into fbdev helpers?
> 
> If you plug out an MST device a stall isn't going to be a major worry, you
> are going to get a modeset most likely straight away.
> 
> So I think we should only address this problem when we have this problem.

Well it's more general unhappiness with how fbdev locking piggy-packs on
top of drm_modeset_lock_all(). At least ime reusing BKLs because they're
there already sooner or later leads to headaches - having separate locks
for separate things makes review easier. Avoid the stall is just a benefit
on top. Anyway, was just an idea.
-Daniel
-- 
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