[PATCH 19/35] drm: revamp locking around fb creation/destruction

From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: DRI Development <dri-devel@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: [PATCH 19/35] drm: revamp locking around fb creation/destruction
Date: Thu, 10 Jan 2013 21:48:00 +0100	[thread overview]
Message-ID: <1357850897-27102-20-git-send-email-daniel.vetter@ffwll.ch> (raw)
In-Reply-To: <1357850897-27102-1-git-send-email-daniel.vetter@ffwll.ch>

Well, at least step 1. The goal here is that framebuffer objects can
survive outside of the mode_config lock, with just a reference held
as protection. The first step to get there is to introduce a special
fb_lock which protects fb lookup, creation and destruction, to make
them appear atomic.

This new fb_lock can nest within the mode_config lock. But the idea is
(once the reference counting part is completed) that we only quickly
take that fb_lock to lookup a framebuffer and grab a reference,
without any other locks involved.

vmwgfx is the only driver which does framebuffer lookups itself, also
wrap those calls to drm_mode_object_find with the new lock.

Also protect the fb_list walking in i915 and omapdrm with the new lock.

As a slight complication there's also the list of user-created fbs
attached to the file private. The problem now is that at fclose() time
we need to walk that list, eventually do a modeset call to remove the
fb from active usage (and are required to be able to take the
mode_config lock), but in the end we need to grab the new fb_lock to
remove the fb from the list. The easiest solution is to add another
mutex to protect this per-file list.

Currently that new fbs_lock nests within the modeset locks and so
appears redudant. But later patches will switch around this sequence
so that taking the modeset locks in the fb destruction path is
optional in the fastpath. Ultimately the goal is that addfb and rmfb
do not require the mode_config lock, since otherwise they have the
potential to introduce stalls in the pageflip sequence of a compositor
(if the compositor e.g. switches to a fullscreen client or if it
enables a plane). But that requires a few more steps and hoops to jump
through.

Note that framebuffer creation/destruction is now double-protected -
once by the fb_lock and in parts by the idr_lock. The later would be
unnecessariy if framebuffers would have their own idr allocator. But
that's material for another patch (series).

v2: Properly initialize the fb->filp_head list in _init, otherwise the
newly added WARN to check whether the fb isn't on a fpriv list any
more will fail for driver-private objects.

v3: Fixup two error-case unlock bugs spotted by Richard Wilbur.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c             |  116 ++++++++++++++++++++++----------
 drivers/gpu/drm/drm_fops.c             |    1 +
 drivers/gpu/drm/i915/i915_debugfs.c    |    5 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c  |    4 ++
 drivers/staging/omapdrm/omap_debugfs.c |    2 +
 include/drm/drmP.h                     |    8 +++
 include/drm/drm_crtc.h                 |   14 ++++
 7 files changed, 113 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 457e4faf..d387ddb 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -262,15 +262,21 @@ again:
 
 	mutex_lock(&dev->mode_config.idr_mutex);
 	ret = idr_get_new_above(&dev->mode_config.crtc_idr, obj, 1, &new_id);
+
+	if (!ret) {
+		/*
+		 * Set up the object linking under the protection of the idr
+		 * lock so that other users can't see inconsistent state.
+		 */
+		obj->id = new_id;
+		obj->type = obj_type;
+	}
 	mutex_unlock(&dev->mode_config.idr_mutex);
+
 	if (ret == -EAGAIN)
 		goto again;
-	else if (ret)
-		return ret;
 
-	obj->id = new_id;
-	obj->type = obj_type;
-	return 0;
+	return ret;
 }
 
 /**
@@ -312,6 +318,12 @@ EXPORT_SYMBOL(drm_mode_object_find);
  * Allocates an ID for the framebuffer's parent mode object, sets its mode
  * functions & device file and adds it to the master fd list.
  *
+ * IMPORTANT:
+ * This functions publishes the fb and makes it available for concurrent access
+ * by other users. Which means by this point the fb _must_ be fully set up -
+ * since all the fb attributes are invariant over its lifetime, no further
+ * locking but only correct reference counting is required.
+ *
  * RETURNS:
  * Zero on success, error code on failure.
  */
@@ -320,16 +332,20 @@ 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);
 	if (ret)
-		return ret;
+		goto out;
 
-	fb->dev = dev;
-	fb->funcs = funcs;
 	dev->mode_config.num_fb++;
 	list_add(&fb->head, &dev->mode_config.fb_list);
+out:
+	mutex_unlock(&dev->mode_config.fb_lock);
 
 	return 0;
 }
@@ -385,8 +401,10 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
 	 * this.)
 	 */
 	drm_mode_object_put(dev, &fb->base);
+	mutex_lock(&dev->mode_config.fb_lock);
 	list_del(&fb->head);
 	dev->mode_config.num_fb--;
+	mutex_unlock(&dev->mode_config.fb_lock);
 }
 EXPORT_SYMBOL(drm_framebuffer_cleanup);
 
@@ -406,6 +424,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 	int ret;
 
 	WARN_ON(!drm_modeset_is_locked(dev));
+	WARN_ON(!list_empty(&fb->filp_head));
 
 	/* remove from any CRTC */
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
@@ -432,8 +451,6 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 		}
 	}
 
-	list_del(&fb->filp_head);
-
 	drm_framebuffer_unreference(fb);
 }
 EXPORT_SYMBOL(drm_framebuffer_remove);
@@ -989,6 +1006,7 @@ void drm_mode_config_init(struct drm_device *dev)
 {
 	mutex_init(&dev->mode_config.mutex);
 	mutex_init(&dev->mode_config.idr_mutex);
+	mutex_init(&dev->mode_config.fb_lock);
 	INIT_LIST_HEAD(&dev->mode_config.fb_list);
 	INIT_LIST_HEAD(&dev->mode_config.crtc_list);
 	INIT_LIST_HEAD(&dev->mode_config.connector_list);
@@ -1091,6 +1109,9 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 		drm_property_destroy(dev, property);
 	}
 
+	/* Single-threaded teardown context, so it's not requied to grab the
+	 * fb_lock to protect against concurrent fb_list access. Contrary, it
+	 * would actually deadlock with the drm_framebuffer_cleanup function. */
 	list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
 		drm_framebuffer_remove(fb);
 	}
@@ -1220,8 +1241,8 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
 
-	drm_modeset_lock_all(dev);
 
+	mutex_lock(&file_priv->fbs_lock);
 	/*
 	 * For the non-control nodes we need to limit the list of resources
 	 * by IDs in the group list for this node
@@ -1229,6 +1250,23 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	list_for_each(lh, &file_priv->fbs)
 		fb_count++;
 
+	/* handle this in 4 parts */
+	/* FBs */
+	if (card_res->count_fbs >= fb_count) {
+		copied = 0;
+		fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
+		list_for_each_entry(fb, &file_priv->fbs, filp_head) {
+			if (put_user(fb->base.id, fb_id + copied)) {
+				mutex_unlock(&file_priv->fbs_lock);
+				return -EFAULT;
+			}
+			copied++;
+		}
+	}
+	card_res->count_fbs = fb_count;
+	mutex_unlock(&file_priv->fbs_lock);
+
+	drm_modeset_lock_all(dev);
 	mode_group = &file_priv->master->minor->mode_group;
 	if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
 
@@ -1252,21 +1290,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	card_res->max_width = dev->mode_config.max_width;
 	card_res->min_width = dev->mode_config.min_width;
 
-	/* handle this in 4 parts */
-	/* FBs */
-	if (card_res->count_fbs >= fb_count) {
-		copied = 0;
-		fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
-		list_for_each_entry(fb, &file_priv->fbs, filp_head) {
-			if (put_user(fb->base.id, fb_id + copied)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			copied++;
-		}
-	}
-	card_res->count_fbs = fb_count;
-
 	/* CRTCs */
 	if (card_res->count_crtcs >= crtc_count) {
 		copied = 0;
@@ -1765,8 +1788,10 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	}
 	crtc = obj_to_crtc(obj);
 
+	mutex_lock(&dev->mode_config.fb_lock);
 	obj = drm_mode_object_find(dev, plane_req->fb_id,
 				   DRM_MODE_OBJECT_FB);
+	mutex_unlock(&dev->mode_config.fb_lock);
 	if (!obj) {
 		DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
 			      plane_req->fb_id);
@@ -1908,8 +1933,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			}
 			fb = crtc->fb;
 		} else {
+			mutex_lock(&dev->mode_config.fb_lock);
 			obj = drm_mode_object_find(dev, crtc_req->fb_id,
 						   DRM_MODE_OBJECT_FB);
+			mutex_unlock(&dev->mode_config.fb_lock);
 			if (!obj) {
 				DRM_DEBUG_KMS("Unknown FB ID%d\n",
 						crtc_req->fb_id);
@@ -2151,16 +2178,17 @@ int drm_mode_addfb(struct drm_device *dev,
 	fb = dev->mode_config.funcs->fb_create(dev, file_priv, &r);
 	if (IS_ERR(fb)) {
 		DRM_DEBUG_KMS("could not create framebuffer\n");
-		ret = PTR_ERR(fb);
-		goto out;
+		drm_modeset_unlock_all(dev);
+		return PTR_ERR(fb);
 	}
 
+	mutex_lock(&file_priv->fbs_lock);
 	or->fb_id = fb->base.id;
 	list_add(&fb->filp_head, &file_priv->fbs);
 	DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);
-
-out:
+	mutex_unlock(&file_priv->fbs_lock);
 	drm_modeset_unlock_all(dev);
+
 	return ret;
 }
 
@@ -2333,16 +2361,18 @@ int drm_mode_addfb2(struct drm_device *dev,
 	fb = dev->mode_config.funcs->fb_create(dev, file_priv, r);
 	if (IS_ERR(fb)) {
 		DRM_DEBUG_KMS("could not create framebuffer\n");
-		ret = PTR_ERR(fb);
-		goto out;
+		drm_modeset_unlock_all(dev);
+		return PTR_ERR(fb);
 	}
 
+	mutex_lock(&file_priv->fbs_lock);
 	r->fb_id = fb->base.id;
 	list_add(&fb->filp_head, &file_priv->fbs);
 	DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);
+	mutex_unlock(&file_priv->fbs_lock);
 
-out:
 	drm_modeset_unlock_all(dev);
+
 	return ret;
 }
 
@@ -2373,27 +2403,34 @@ int drm_mode_rmfb(struct drm_device *dev,
 		return -EINVAL;
 
 	drm_modeset_lock_all(dev);
+	mutex_lock(&dev->mode_config.fb_lock);
 	obj = drm_mode_object_find(dev, *id, DRM_MODE_OBJECT_FB);
 	/* TODO check that we really get a framebuffer back. */
 	if (!obj) {
+		mutex_unlock(&dev->mode_config.fb_lock);
 		ret = -EINVAL;
 		goto out;
 	}
 	fb = obj_to_fb(obj);
+	mutex_unlock(&dev->mode_config.fb_lock);
 
+	mutex_lock(&file_priv->fbs_lock);
 	list_for_each_entry(fbl, &file_priv->fbs, filp_head)
 		if (fb == fbl)
 			found = 1;
-
 	if (!found) {
 		ret = -EINVAL;
+		mutex_unlock(&file_priv->fbs_lock);
 		goto out;
 	}
 
-	drm_framebuffer_remove(fb);
+	list_del_init(&fb->filp_head);
+	mutex_unlock(&file_priv->fbs_lock);
 
+	drm_framebuffer_remove(fb);
 out:
 	drm_modeset_unlock_all(dev);
+
 	return ret;
 }
 
@@ -2422,7 +2459,9 @@ int drm_mode_getfb(struct drm_device *dev,
 		return -EINVAL;
 
 	drm_modeset_lock_all(dev);
+	mutex_lock(&dev->mode_config.fb_lock);
 	obj = drm_mode_object_find(dev, r->fb_id, DRM_MODE_OBJECT_FB);
+	mutex_unlock(&dev->mode_config.fb_lock);
 	if (!obj) {
 		ret = -EINVAL;
 		goto out;
@@ -2460,7 +2499,9 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
 		return -EINVAL;
 
 	drm_modeset_lock_all(dev);
+	mutex_lock(&dev->mode_config.fb_lock);
 	obj = drm_mode_object_find(dev, r->fb_id, DRM_MODE_OBJECT_FB);
+	mutex_unlock(&dev->mode_config.fb_lock);
 	if (!obj) {
 		ret = -EINVAL;
 		goto out_err1;
@@ -2535,9 +2576,12 @@ void drm_fb_release(struct drm_file *priv)
 	struct drm_framebuffer *fb, *tfb;
 
 	drm_modeset_lock_all(dev);
+	mutex_lock(&priv->fbs_lock);
 	list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
+		list_del_init(&fb->filp_head);
 		drm_framebuffer_remove(fb);
 	}
+	mutex_unlock(&priv->fbs_lock);
 	drm_modeset_unlock_all(dev);
 }
 
@@ -3542,7 +3586,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if (crtc->funcs->page_flip == NULL)
 		goto out;
 
+	mutex_lock(&dev->mode_config.fb_lock);
 	obj = drm_mode_object_find(dev, page_flip->fb_id, DRM_MODE_OBJECT_FB);
+	mutex_unlock(&dev->mode_config.fb_lock);
 	if (!obj)
 		goto out;
 	fb = obj_to_fb(obj);
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 133b413..13fdcd1 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -276,6 +276,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
 
 	INIT_LIST_HEAD(&priv->lhead);
 	INIT_LIST_HEAD(&priv->fbs);
+	mutex_init(&priv->fbs_lock);
 	INIT_LIST_HEAD(&priv->event_list);
 	init_waitqueue_head(&priv->event_wait);
 	priv->event_space = 4096; /* set aside 4k for event buffer */
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f7d88e9..1269a46 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1447,7 +1447,9 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 		   fb->base.bits_per_pixel);
 	describe_obj(m, fb->obj);
 	seq_printf(m, "\n");
+	mutex_unlock(&dev->mode_config.mutex);
 
+	mutex_lock(&dev->mode_config.fb_lock);
 	list_for_each_entry(fb, &dev->mode_config.fb_list, base.head) {
 		if (&fb->base == ifbdev->helper.fb)
 			continue;
@@ -1460,8 +1462,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 		describe_obj(m, fb->obj);
 		seq_printf(m, "\n");
 	}
-
-	mutex_unlock(&dev->mode_config.mutex);
+	mutex_unlock(&dev->mode_config.fb_lock);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
index a135498..0d6a161 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
@@ -165,7 +165,9 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
 
 	drm_modeset_lock_all(dev);
 
+	mutex_lock(&dev->mode_config.fb_lock);
 	obj = drm_mode_object_find(dev, arg->fb_id, DRM_MODE_OBJECT_FB);
+	mutex_unlock(&dev->mode_config.fb_lock);
 	if (!obj) {
 		DRM_ERROR("Invalid framebuffer id.\n");
 		ret = -EINVAL;
@@ -248,7 +250,9 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data,
 
 	drm_modeset_lock_all(dev);
 
+	mutex_lock(&dev->mode_config.fb_lock);
 	obj = drm_mode_object_find(dev, arg->fb_id, DRM_MODE_OBJECT_FB);
+	mutex_unlock(&dev->mode_config.fb_lock);
 	if (!obj) {
 		DRM_ERROR("Invalid framebuffer id.\n");
 		ret = -EINVAL;
diff --git a/drivers/staging/omapdrm/omap_debugfs.c b/drivers/staging/omapdrm/omap_debugfs.c
index 2f122e0..e95540b 100644
--- a/drivers/staging/omapdrm/omap_debugfs.c
+++ b/drivers/staging/omapdrm/omap_debugfs.c
@@ -72,6 +72,7 @@ static int fb_show(struct seq_file *m, void *arg)
 	seq_printf(m, "fbcon ");
 	omap_framebuffer_describe(priv->fbdev->fb, m);
 
+	mutex_lock(&dev->mode_config.fb_lock);
 	list_for_each_entry(fb, &dev->mode_config.fb_list, head) {
 		if (fb == priv->fbdev->fb)
 			continue;
@@ -79,6 +80,7 @@ static int fb_show(struct seq_file *m, void *arg)
 		seq_printf(m, "user ");
 		omap_framebuffer_describe(fb, m);
 	}
+	mutex_unlock(&dev->mode_config.fb_lock);
 
 	mutex_unlock(&dev->struct_mutex);
 	mutex_unlock(&dev->mode_config.mutex);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3c609ab..e74731c 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -446,7 +446,15 @@ struct drm_file {
 	int is_master; /* this file private is a master for a minor */
 	struct drm_master *master; /* master this node is currently associated with
 				      N.B. not always minor->master */
+
+	/**
+	 * fbs - List of framebuffers associated with this file.
+	 *
+	 * Protected by fbs_lock. Note that the fbs list holds a reference on
+	 * the fb object to prevent it from untimely disappearing.
+	 */
 	struct list_head fbs;
+	struct mutex fbs_lock;
 
 	wait_queue_head_t event_wait;
 	struct list_head event_list;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c89b116..c35a807 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -254,6 +254,10 @@ struct drm_framebuffer {
 	 * userspace perspective.
 	 */
 	struct kref refcount;
+	/*
+	 * Place on the dev->mode_config.fb_list, access protected by
+	 * dev->mode_config.fb_lock.
+	 */
 	struct list_head head;
 	struct drm_mode_object base;
 	const struct drm_framebuffer_funcs *funcs;
@@ -780,8 +784,18 @@ struct drm_mode_config {
 	struct mutex idr_mutex; /* for IDR management */
 	struct idr crtc_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */
 	/* this is limited to one for now */
+
+
+	/**
+	 * fb_lock - mutex to protect fb state
+	 *
+	 * Besides the global fb list his also protects the fbs list in the
+	 * file_priv
+	 */
+	struct mutex fb_lock;
 	int num_fb;
 	struct list_head fb_list;
+
 	int num_connector;
 	struct list_head connector_list;
 	int num_encoder;
-- 
1.7.10.4
