History

drm: revamp locking around fb creation/destruction

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.

Daniel Vetter committed 13 years ago

Commit 4b096ac1 parent dac35667 files · +113 −37Browse files

Unified · Split

7 files
  1. drivers/gpu/drm/drm_crtc.c +81 −35
  2. drivers/gpu/drm/drm_fops.c +1 −0
  3. drivers/gpu/drm/i915/i915_debugfs.c +3 −2
  4. drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c +4 −0
  5. drivers/staging/omapdrm/omap_debugfs.c +2 −0
  6. include/drm/drmP.h +8 −0
  7. include/drm/drm_crtc.h +14 −0
drivers/gpu/drm/drm_crtc.c+81−35
@@ -262,15 +262,21 @@ again: · 261 unmodified lines
262262
263263 mutex_lock(&dev->mode_config.idr_mutex);
264264 ret = idr_get_new_above(&dev->mode_config.crtc_idr, obj, 1, &new_id);
265+
266+ if (!ret) {
267+ /*
268+ * Set up the object linking under the protection of the idr
269+ * lock so that other users can't see inconsistent state.
270+ */
271+ obj->id = new_id;
272+ obj->type = obj_type;
273+ }
265274 mutex_unlock(&dev->mode_config.idr_mutex);
275+
266276 if (ret == -EAGAIN)
267277 goto again;
268− else if (ret)
269− return ret;
270278
271− obj->id = new_id;
272− obj->type = obj_type;
273− return 0;
279+ return ret;
274280 }
275281
276282 /**
@@ -312,6 +318,12 @@ EXPORT_SYMBOL(drm_mode_object_find); · 35 unmodified lines
312318 * Allocates an ID for the framebuffer's parent mode object, sets its mode
313319 * functions & device file and adds it to the master fd list.
314320 *
321+ * IMPORTANT:
322+ * This functions publishes the fb and makes it available for concurrent access
323+ * by other users. Which means by this point the fb _must_ be fully set up -
324+ * since all the fb attributes are invariant over its lifetime, no further
325+ * locking but only correct reference counting is required.
326+ *
315327 * RETURNS:
316328 * Zero on success, error code on failure.
317329 */
@@ -320,16 +332,20 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb, · 2 unmodified lines
320332 {
321333 int ret;
322334
335+ mutex_lock(&dev->mode_config.fb_lock);
323336 kref_init(&fb->refcount);
337+ INIT_LIST_HEAD(&fb->filp_head);
338+ fb->dev = dev;
339+ fb->funcs = funcs;
324340
325341 ret = drm_mode_object_get(dev, &fb->base, DRM_MODE_OBJECT_FB);
326342 if (ret)
327− return ret;
343+ goto out;
328344
329− fb->dev = dev;
330− fb->funcs = funcs;
331345 dev->mode_config.num_fb++;
332346 list_add(&fb->head, &dev->mode_config.fb_list);
347+out:
348+ mutex_unlock(&dev->mode_config.fb_lock);
333349
334350 return 0;
335351 }
@@ -385,8 +401,10 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb) · 49 unmodified lines
385401 * this.)
386402 */
387403 drm_mode_object_put(dev, &fb->base);
404+ mutex_lock(&dev->mode_config.fb_lock);
388405 list_del(&fb->head);
389406 dev->mode_config.num_fb--;
407+ mutex_unlock(&dev->mode_config.fb_lock);
390408 }
391409 EXPORT_SYMBOL(drm_framebuffer_cleanup);
392410
@@ -406,6 +424,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) · 13 unmodified lines
406424 int ret;
407425
408426 WARN_ON(!drm_modeset_is_locked(dev));
427+ WARN_ON(!list_empty(&fb->filp_head));
409428
410429 /* remove from any CRTC */
411430 list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
@@ -432,8 +451,6 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) · 20 unmodified lines
432451 }
433452 }
434453
435− list_del(&fb->filp_head);
436−
437454 drm_framebuffer_unreference(fb);
438455 }
439456 EXPORT_SYMBOL(drm_framebuffer_remove);
@@ -989,6 +1006,7 @@ void drm_mode_config_init(struct drm_device *dev) · 549 unmodified lines
9891006 {
9901007 mutex_init(&dev->mode_config.mutex);
9911008 mutex_init(&dev->mode_config.idr_mutex);
1009+ mutex_init(&dev->mode_config.fb_lock);
9921010 INIT_LIST_HEAD(&dev->mode_config.fb_list);
9931011 INIT_LIST_HEAD(&dev->mode_config.crtc_list);
9941012 INIT_LIST_HEAD(&dev->mode_config.connector_list);
@@ -1091,6 +1109,9 @@ void drm_mode_config_cleanup(struct drm_device *dev) · 96 unmodified lines
10911109 drm_property_destroy(dev, property);
10921110 }
10931111
1112+ /* Single-threaded teardown context, so it's not requied to grab the
1113+ * fb_lock to protect against concurrent fb_list access. Contrary, it
1114+ * would actually deadlock with the drm_framebuffer_cleanup function. */
10941115 list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
10951116 drm_framebuffer_remove(fb);
10961117 }
@@ -1220,8 +1241,8 @@ int drm_mode_getresources(struct drm_device *dev, void *data, · 123 unmodified lines
12201241 if (!drm_core_check_feature(dev, DRIVER_MODESET))
12211242 return -EINVAL;
12221243
1223− drm_modeset_lock_all(dev);
12241244
1245+ mutex_lock(&file_priv->fbs_lock);
12251246 /*
12261247 * For the non-control nodes we need to limit the list of resources
12271248 * by IDs in the group list for this node
@@ -1229,6 +1250,23 @@ int drm_mode_getresources(struct drm_device *dev, void *data, · 1 unmodified line
12291250 list_for_each(lh, &file_priv->fbs)
12301251 fb_count++;
12311252
1253+ /* handle this in 4 parts */
1254+ /* FBs */
1255+ if (card_res->count_fbs >= fb_count) {
1256+ copied = 0;
1257+ fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
1258+ list_for_each_entry(fb, &file_priv->fbs, filp_head) {
1259+ if (put_user(fb->base.id, fb_id + copied)) {
1260+ mutex_unlock(&file_priv->fbs_lock);
1261+ return -EFAULT;
1262+ }
1263+ copied++;
1264+ }
1265+ }
1266+ card_res->count_fbs = fb_count;
1267+ mutex_unlock(&file_priv->fbs_lock);
1268+
1269+ drm_modeset_lock_all(dev);
12321270 mode_group = &file_priv->master->minor->mode_group;
12331271 if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
12341272
@@ -1252,21 +1290,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data, · 17 unmodified lines
12521290 card_res->max_width = dev->mode_config.max_width;
12531291 card_res->min_width = dev->mode_config.min_width;
12541292
1255− /* handle this in 4 parts */
1256− /* FBs */
1257− if (card_res->count_fbs >= fb_count) {
1258− copied = 0;
1259− fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
1260− list_for_each_entry(fb, &file_priv->fbs, filp_head) {
1261− if (put_user(fb->base.id, fb_id + copied)) {
1262− ret = -EFAULT;
1263− goto out;
1264− }
1265− copied++;
1266− }
1267− }
1268− card_res->count_fbs = fb_count;
1269−
12701293 /* CRTCs */
12711294 if (card_res->count_crtcs >= crtc_count) {
12721295 copied = 0;
@@ -1765,8 +1788,10 @@ int drm_mode_setplane(struct drm_device *dev, void *data, · 492 unmodified lines
17651788 }
17661789 crtc = obj_to_crtc(obj);
17671790
1791+ mutex_lock(&dev->mode_config.fb_lock);
17681792 obj = drm_mode_object_find(dev, plane_req->fb_id,
17691793 DRM_MODE_OBJECT_FB);
1794+ mutex_unlock(&dev->mode_config.fb_lock);
17701795 if (!obj) {
17711796 DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
17721797 plane_req->fb_id);
@@ -1908,8 +1933,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, · 135 unmodified lines
19081933 }
19091934 fb = crtc->fb;
19101935 } else {
1936+ mutex_lock(&dev->mode_config.fb_lock);
19111937 obj = drm_mode_object_find(dev, crtc_req->fb_id,
19121938 DRM_MODE_OBJECT_FB);
1939+ mutex_unlock(&dev->mode_config.fb_lock);
19131940 if (!obj) {
19141941 DRM_DEBUG_KMS("Unknown FB ID%d\n",
19151942 crtc_req->fb_id);
@@ -2151,16 +2178,17 @@ int drm_mode_addfb(struct drm_device *dev, · 235 unmodified lines
21512178 fb = dev->mode_config.funcs->fb_create(dev, file_priv, &r);
21522179 if (IS_ERR(fb)) {
21532180 DRM_DEBUG_KMS("could not create framebuffer\n");
2154− ret = PTR_ERR(fb);
2155− goto out;
2181+ drm_modeset_unlock_all(dev);
2182+ return PTR_ERR(fb);
21562183 }
21572184
2185+ mutex_lock(&file_priv->fbs_lock);
21582186 or->fb_id = fb->base.id;
21592187 list_add(&fb->filp_head, &file_priv->fbs);
21602188 DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);
2161−
2162−out:
2189+ mutex_unlock(&file_priv->fbs_lock);
21632190 drm_modeset_unlock_all(dev);
2191+
21642192 return ret;
21652193 }
21662194
@@ -2333,16 +2361,18 @@ int drm_mode_addfb2(struct drm_device *dev, · 166 unmodified lines
23332361 fb = dev->mode_config.funcs->fb_create(dev, file_priv, r);
23342362 if (IS_ERR(fb)) {
23352363 DRM_DEBUG_KMS("could not create framebuffer\n");
2336− ret = PTR_ERR(fb);
2337− goto out;
2364+ drm_modeset_unlock_all(dev);
2365+ return PTR_ERR(fb);
23382366 }
23392367
2368+ mutex_lock(&file_priv->fbs_lock);
23402369 r->fb_id = fb->base.id;
23412370 list_add(&fb->filp_head, &file_priv->fbs);
23422371 DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);
2372+ mutex_unlock(&file_priv->fbs_lock);
23432373
2344−out:
23452374 drm_modeset_unlock_all(dev);
2375+
23462376 return ret;
23472377 }
23482378
@@ -2373,27 +2403,34 @@ int drm_mode_rmfb(struct drm_device *dev, · 24 unmodified lines
23732403 return -EINVAL;
23742404
23752405 drm_modeset_lock_all(dev);
2406+ mutex_lock(&dev->mode_config.fb_lock);
23762407 obj = drm_mode_object_find(dev, *id, DRM_MODE_OBJECT_FB);
23772408 /* TODO check that we really get a framebuffer back. */
23782409 if (!obj) {
2410+ mutex_unlock(&dev->mode_config.fb_lock);
23792411 ret = -EINVAL;
23802412 goto out;
23812413 }
23822414 fb = obj_to_fb(obj);
2415+ mutex_unlock(&dev->mode_config.fb_lock);
23832416
2417+ mutex_lock(&file_priv->fbs_lock);
23842418 list_for_each_entry(fbl, &file_priv->fbs, filp_head)
23852419 if (fb == fbl)
23862420 found = 1;
2387−
23882421 if (!found) {
23892422 ret = -EINVAL;
2423+ mutex_unlock(&file_priv->fbs_lock);
23902424 goto out;
23912425 }
23922426
2393− drm_framebuffer_remove(fb);
2427+ list_del_init(&fb->filp_head);
2428+ mutex_unlock(&file_priv->fbs_lock);
23942429
2430+ drm_framebuffer_remove(fb);
23952431 out:
23962432 drm_modeset_unlock_all(dev);
2433+
23972434 return ret;
23982435 }
23992436
@@ -2422,7 +2459,9 @@ int drm_mode_getfb(struct drm_device *dev, · 22 unmodified lines
24222459 return -EINVAL;
24232460
24242461 drm_modeset_lock_all(dev);
2462+ mutex_lock(&dev->mode_config.fb_lock);
24252463 obj = drm_mode_object_find(dev, r->fb_id, DRM_MODE_OBJECT_FB);
2464+ mutex_unlock(&dev->mode_config.fb_lock);
24262465 if (!obj) {
24272466 ret = -EINVAL;
24282467 goto out;
@@ -2460,7 +2499,9 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev, · 31 unmodified lines
24602499 return -EINVAL;
24612500
24622501 drm_modeset_lock_all(dev);
2502+ mutex_lock(&dev->mode_config.fb_lock);
24632503 obj = drm_mode_object_find(dev, r->fb_id, DRM_MODE_OBJECT_FB);
2504+ mutex_unlock(&dev->mode_config.fb_lock);
24642505 if (!obj) {
24652506 ret = -EINVAL;
24662507 goto out_err1;
@@ -2535,9 +2576,12 @@ void drm_fb_release(struct drm_file *priv) · 68 unmodified lines
25352576 struct drm_framebuffer *fb, *tfb;
25362577
25372578 drm_modeset_lock_all(dev);
2579+ mutex_lock(&priv->fbs_lock);
25382580 list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
2581+ list_del_init(&fb->filp_head);
25392582 drm_framebuffer_remove(fb);
25402583 }
2584+ mutex_unlock(&priv->fbs_lock);
25412585 drm_modeset_unlock_all(dev);
25422586 }
25432587
@@ -3542,7 +3586,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, · 998 unmodified lines
35423586 if (crtc->funcs->page_flip == NULL)
35433587 goto out;
35443588
3589+ mutex_lock(&dev->mode_config.fb_lock);
35453590 obj = drm_mode_object_find(dev, page_flip->fb_id, DRM_MODE_OBJECT_FB);
3591+ mutex_unlock(&dev->mode_config.fb_lock);
35463592 if (!obj)
35473593 goto out;
35483594 fb = obj_to_fb(obj);
drivers/gpu/drm/drm_fops.c+1−0
@@ -276,6 +276,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, · 275 unmodified lines
276276
277277 INIT_LIST_HEAD(&priv->lhead);
278278 INIT_LIST_HEAD(&priv->fbs);
279+ mutex_init(&priv->fbs_lock);
279280 INIT_LIST_HEAD(&priv->event_list);
280281 init_waitqueue_head(&priv->event_wait);
281282 priv->event_space = 4096; /* set aside 4k for event buffer */
drivers/gpu/drm/i915/i915_debugfs.c+3−2
@@ -1374,7 +1374,9 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data) · 1373 unmodified lines
13741374 fb->base.bits_per_pixel);
13751375 describe_obj(m, fb->obj);
13761376 seq_printf(m, "\n");
1377+ mutex_unlock(&dev->mode_config.mutex);
13771378
1379+ mutex_lock(&dev->mode_config.fb_lock);
13781380 list_for_each_entry(fb, &dev->mode_config.fb_list, base.head) {
13791381 if (&fb->base == ifbdev->helper.fb)
13801382 continue;
@@ -1387,8 +1389,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data) · 6 unmodified lines
13871389 describe_obj(m, fb->obj);
13881390 seq_printf(m, "\n");
13891391 }
1390−
1391− mutex_unlock(&dev->mode_config.mutex);
1392+ mutex_unlock(&dev->mode_config.fb_lock);
13921393
13931394 return 0;
13941395 }
drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c+4−0
@@ -165,7 +165,9 @@ int vmw_present_ioctl(struct drm_device *dev, void *data, · 164 unmodified lines
165165
166166 drm_modeset_lock_all(dev);
167167
168+ mutex_lock(&dev->mode_config.fb_lock);
168169 obj = drm_mode_object_find(dev, arg->fb_id, DRM_MODE_OBJECT_FB);
170+ mutex_unlock(&dev->mode_config.fb_lock);
169171 if (!obj) {
170172 DRM_ERROR("Invalid framebuffer id.\n");
171173 ret = -EINVAL;
@@ -248,7 +250,9 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data, · 76 unmodified lines
248250
249251 drm_modeset_lock_all(dev);
250252
253+ mutex_lock(&dev->mode_config.fb_lock);
251254 obj = drm_mode_object_find(dev, arg->fb_id, DRM_MODE_OBJECT_FB);
255+ mutex_unlock(&dev->mode_config.fb_lock);
252256 if (!obj) {
253257 DRM_ERROR("Invalid framebuffer id.\n");
254258 ret = -EINVAL;
drivers/staging/omapdrm/omap_debugfs.c+2−0
@@ -72,6 +72,7 @@ static int fb_show(struct seq_file *m, void *arg) · 71 unmodified lines
7272 seq_printf(m, "fbcon ");
7373 omap_framebuffer_describe(priv->fbdev->fb, m);
7474
75+ mutex_lock(&dev->mode_config.fb_lock);
7576 list_for_each_entry(fb, &dev->mode_config.fb_list, head) {
7677 if (fb == priv->fbdev->fb)
7778 continue;
@@ -79,6 +80,7 @@ static int fb_show(struct seq_file *m, void *arg) · 1 unmodified line
7980 seq_printf(m, "user ");
8081 omap_framebuffer_describe(fb, m);
8182 }
83+ mutex_unlock(&dev->mode_config.fb_lock);
8284
8385 mutex_unlock(&dev->struct_mutex);
8486 mutex_unlock(&dev->mode_config.mutex);
include/drm/drmP.h+8−0
@@ -446,7 +446,15 @@ struct drm_file { · 445 unmodified lines
446446 int is_master; /* this file private is a master for a minor */
447447 struct drm_master *master; /* master this node is currently associated with
448448 N.B. not always minor->master */
449+
450+ /**
451+ * fbs - List of framebuffers associated with this file.
452+ *
453+ * Protected by fbs_lock. Note that the fbs list holds a reference on
454+ * the fb object to prevent it from untimely disappearing.
455+ */
449456 struct list_head fbs;
457+ struct mutex fbs_lock;
450458
451459 wait_queue_head_t event_wait;
452460 struct list_head event_list;
include/drm/drm_crtc.h+14−0
@@ -254,6 +254,10 @@ struct drm_framebuffer { · 253 unmodified lines
254254 * userspace perspective.
255255 */
256256 struct kref refcount;
257+ /*
258+ * Place on the dev->mode_config.fb_list, access protected by
259+ * dev->mode_config.fb_lock.
260+ */
257261 struct list_head head;
258262 struct drm_mode_object base;
259263 const struct drm_framebuffer_funcs *funcs;
@@ -780,8 +784,18 @@ struct drm_mode_config { · 520 unmodified lines
780784 struct mutex idr_mutex; /* for IDR management */
781785 struct idr crtc_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */
782786 /* this is limited to one for now */
787+
788+
789+ /**
790+ * fb_lock - mutex to protect fb state
791+ *
792+ * Besides the global fb list his also protects the fbs list in the
793+ * file_priv
794+ */
795+ struct mutex fb_lock;
783796 int num_fb;
784797 struct list_head fb_list;
798+
785799 int num_connector;
786800 struct list_head connector_list;
787801 int num_encoder;

Keyboard shortcuts

General

Show keyboard shortcuts
?
Focus the current search
/

Issues and pull requests

Open filters
f

Issues, pull requests and Inbox

Select next item
j↓
Select previous item
k↑
Open selected item
Enter
Dismiss filters or clear selection
Esc

Repository code

Go to file
t