With prime, we are running into false circular dependencies based on the order in which two drivers may lock their own struct_mutex wrt to a common lock (like the reservation->lock). Work around this by adding the lock_class_key to the struct drm_driver such that each driver can have its own subclass of struct_mutex. Circular dependencies between drivers will now be ignored, but real circular dependencies on any one mutex will still be caught. A driver creating more than one device will still need to be careful!
Reported-by: Tobias Klausmann tobias.johannes.klausmann@mni.thm.de Reported-by: Hans de Goede hdegoede@redhat.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/drm_drv.c | 4 +++- include/drm/drm_drv.h | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 2fa4e4fa7c33..82b521146e71 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -488,7 +488,9 @@ int drm_dev_init(struct drm_device *dev,
spin_lock_init(&dev->buf_lock); spin_lock_init(&dev->event_lock); - mutex_init(&dev->struct_mutex); + __mutex_init(&dev->struct_mutex, + "&dev->struct_mutex", + &driver->class.struct_mutex_key); mutex_init(&dev->filelist_mutex); mutex_init(&dev->ctxlist_mutex); mutex_init(&dev->master_mutex); diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 554104ccb939..5d521923404a 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -39,6 +39,10 @@ struct dma_buf_attachment; struct drm_display_mode; struct drm_mode_create_dumb;
+struct drm_driver_class { + struct lock_class_key struct_mutex_key; +}; + /* driver capabilities and requirements mask */ #define DRIVER_USE_AGP 0x1 #define DRIVER_LEGACY 0x2 @@ -64,6 +68,8 @@ struct drm_mode_create_dumb; * structure for GEM drivers. */ struct drm_driver { + struct drm_driver_class class; + int (*load) (struct drm_device *, unsigned long flags); int (*firstopen) (struct drm_device *); int (*open) (struct drm_device *, struct drm_file *);
Hi Chris,
[auto build test WARNING on drm/drm-next] [also build test WARNING on next-20161209] [cannot apply to v4.9-rc8] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-Make-each-driver-s... base: git://people.freedesktop.org/~airlied/linux.git drm-next reproduce: make htmldocs
All warnings (new ones prefixed by >>):
make[3]: warning: jobserver unavailable: using -j1. Add '+' to parent make rule. include/linux/init.h:1: warning: no structured comments found include/linux/workqueue.h:392: warning: No description found for parameter '...' include/linux/workqueue.h:392: warning: Excess function parameter 'args' description in 'alloc_workqueue' include/linux/workqueue.h:413: warning: No description found for parameter '...' include/linux/workqueue.h:413: warning: Excess function parameter 'args' description in 'alloc_ordered_workqueue' include/linux/kthread.h:26: warning: No description found for parameter '...' kernel/sys.c:1: warning: no structured comments found drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found include/sound/core.h:324: warning: No description found for parameter '...' include/sound/core.h:335: warning: No description found for parameter '...' include/sound/core.h:388: warning: No description found for parameter '...'
include/drm/drm_drv.h:415: warning: No description found for parameter 'class'
include/drm/drm_drv.h:415: warning: No description found for parameter 'load' include/drm/drm_drv.h:415: warning: No description found for parameter 'firstopen' include/drm/drm_drv.h:415: warning: No description found for parameter 'open' include/drm/drm_drv.h:415: warning: No description found for parameter 'preclose' include/drm/drm_drv.h:415: warning: No description found for parameter 'postclose' include/drm/drm_drv.h:415: warning: No description found for parameter 'lastclose' include/drm/drm_drv.h:415: warning: No description found for parameter 'unload' include/drm/drm_drv.h:415: warning: No description found for parameter 'dma_ioctl' include/drm/drm_drv.h:415: warning: No description found for parameter 'dma_quiescent' include/drm/drm_drv.h:415: warning: No description found for parameter 'context_dtor' include/drm/drm_drv.h:415: warning: No description found for parameter 'set_busid' include/drm/drm_drv.h:415: warning: No description found for parameter 'irq_handler' include/drm/drm_drv.h:415: warning: No description found for parameter 'irq_preinstall' include/drm/drm_drv.h:415: warning: No description found for parameter 'irq_postinstall' include/drm/drm_drv.h:415: warning: No description found for parameter 'irq_uninstall' include/drm/drm_drv.h:415: warning: No description found for parameter 'debugfs_init' include/drm/drm_drv.h:415: warning: No description found for parameter 'debugfs_cleanup' include/drm/drm_drv.h:415: warning: No description found for parameter 'gem_open_object' include/drm/drm_drv.h:415: warning: No description found for parameter 'gem_close_object' include/drm/drm_drv.h:415: warning: No description found for parameter 'prime_handle_to_fd' include/drm/drm_drv.h:415: warning: No description found for parameter 'prime_fd_to_handle' include/drm/drm_drv.h:415: warning: No description found for parameter 'gem_prime_export' include/drm/drm_drv.h:415: warning: No description found for parameter 'gem_prime_import' include/drm/drm_drv.h:415: warning: No description found for parameter 'gem_prime_pin' include/drm/drm_drv.h:415: warning: No description found for parameter 'gem_prime_unpin' include/drm/drm_drv.h:415: warning: No description found for parameter 'gem_prime_res_obj' include/drm/drm_drv.h:415: warning: No description found for parameter 'gem_prime_get_sg_table' include/drm/drm_drv.h:415: warning: No description found for parameter 'gem_prime_import_sg_table' include/drm/drm_drv.h:415: warning: No description found for parameter 'gem_prime_vmap' include/drm/drm_drv.h:415: warning: No description found for parameter 'gem_prime_vunmap' include/drm/drm_drv.h:415: warning: No description found for parameter 'gem_prime_mmap' include/drm/drm_drv.h:415: warning: No description found for parameter 'vgaarb_irq' include/drm/drm_drv.h:415: warning: No description found for parameter 'gem_vm_ops' include/drm/drm_drv.h:415: warning: No description found for parameter 'major' include/drm/drm_drv.h:415: warning: No description found for parameter 'minor' include/drm/drm_drv.h:415: warning: No description found for parameter 'patchlevel' include/drm/drm_drv.h:415: warning: No description found for parameter 'name' include/drm/drm_drv.h:415: warning: No description found for parameter 'desc' include/drm/drm_drv.h:415: warning: No description found for parameter 'date' include/drm/drm_drv.h:415: warning: No description found for parameter 'driver_features' include/drm/drm_drv.h:415: warning: No description found for parameter 'dev_priv_size' include/drm/drm_drv.h:415: warning: No description found for parameter 'ioctls' include/drm/drm_drv.h:415: warning: No description found for parameter 'num_ioctls' include/drm/drm_drv.h:415: warning: No description found for parameter 'fops' include/drm/drm_drv.h:415: warning: No description found for parameter 'legacy_dev_list' include/media/media-entity.h:1054: warning: No description found for parameter '...' include/net/mac80211.h:3207: ERROR: Unexpected indentation. include/net/mac80211.h:3210: WARNING: Block quote ends without a blank line; unexpected unindent. include/net/mac80211.h:3212: ERROR: Unexpected indentation. include/net/mac80211.h:3213: WARNING: Block quote ends without a blank line; unexpected unindent. include/net/mac80211.h:1772: ERROR: Unexpected indentation. include/net/mac80211.h:1776: WARNING: Block quote ends without a blank line; unexpected unindent. kernel/sched/fair.c:7259: WARNING: Inline emphasis start-string without end-string. kernel/time/timer.c:1240: ERROR: Unexpected indentation. kernel/time/timer.c:1242: ERROR: Unexpected indentation. kernel/time/timer.c:1243: WARNING: Block quote ends without a blank line; unexpected unindent. include/linux/wait.h:121: WARNING: Block quote ends without a blank line; unexpected unindent. include/linux/wait.h:124: ERROR: Unexpected indentation. include/linux/wait.h:126: WARNING: Block quote ends without a blank line; unexpected unindent. kernel/time/hrtimer.c:1021: WARNING: Block quote ends without a blank line; unexpected unindent. kernel/signal.c:317: WARNING: Inline literal start-string without end-string. drivers/base/firmware_class.c:1348: WARNING: Bullet list ends without a blank line; unexpected unindent. drivers/message/fusion/mptbase.c:5054: WARNING: Definition list ends without a blank line; unexpected unindent. drivers/tty/serial/serial_core.c:1893: WARNING: Definition list ends without a blank line; unexpected unindent. include/linux/spi/spi.h:369: ERROR: Unexpected indentation. WARNING: dvipng command 'dvipng' cannot be run (needed for math display), check the imgmath_dvipng setting
vim +/class +415 include/drm/drm_drv.h
85e634bc Daniel Vetter 2016-11-14 399 int major; 85e634bc Daniel Vetter 2016-11-14 400 int minor; 85e634bc Daniel Vetter 2016-11-14 401 int patchlevel; 85e634bc Daniel Vetter 2016-11-14 402 char *name; 85e634bc Daniel Vetter 2016-11-14 403 char *desc; 85e634bc Daniel Vetter 2016-11-14 404 char *date; 85e634bc Daniel Vetter 2016-11-14 405 85e634bc Daniel Vetter 2016-11-14 406 u32 driver_features; 85e634bc Daniel Vetter 2016-11-14 407 int dev_priv_size; 85e634bc Daniel Vetter 2016-11-14 408 const struct drm_ioctl_desc *ioctls; 85e634bc Daniel Vetter 2016-11-14 409 int num_ioctls; 85e634bc Daniel Vetter 2016-11-14 410 const struct file_operations *fops; 85e634bc Daniel Vetter 2016-11-14 411 85e634bc Daniel Vetter 2016-11-14 412 /* List of devices hanging off this driver with stealth attach. */ 85e634bc Daniel Vetter 2016-11-14 413 struct list_head legacy_dev_list; 85e634bc Daniel Vetter 2016-11-14 414 }; 85e634bc Daniel Vetter 2016-11-14 @415 85e634bc Daniel Vetter 2016-11-14 416 extern __printf(6, 7) 85e634bc Daniel Vetter 2016-11-14 417 void drm_dev_printk(const struct device *dev, const char *level, 85e634bc Daniel Vetter 2016-11-14 418 unsigned int category, const char *function_name, 85e634bc Daniel Vetter 2016-11-14 419 const char *prefix, const char *format, ...); 85e634bc Daniel Vetter 2016-11-14 420 extern __printf(3, 4) 85e634bc Daniel Vetter 2016-11-14 421 void drm_printk(const char *level, unsigned int category, 85e634bc Daniel Vetter 2016-11-14 422 const char *format, ...); 85e634bc Daniel Vetter 2016-11-14 423 extern unsigned int drm_debug;
:::::: The code at line 415 was first introduced by commit :::::: 85e634bce01af582a0fa549c904154b0e3c56db5 drm: Extract drm_drv.h
:::::: TO: Daniel Vetter daniel.vetter@ffwll.ch :::::: CC: Daniel Vetter daniel.vetter@ffwll.ch
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Fri, Dec 09, 2016 at 04:52:32PM +0000, Chris Wilson wrote:
With prime, we are running into false circular dependencies based on the order in which two drivers may lock their own struct_mutex wrt to a common lock (like the reservation->lock). Work around this by adding the lock_class_key to the struct drm_driver such that each driver can have its own subclass of struct_mutex. Circular dependencies between drivers will now be ignored, but real circular dependencies on any one mutex will still be caught. A driver creating more than one device will still need to be careful!
Reported-by: Tobias Klausmann tobias.johannes.klausmann@mni.thm.de Reported-by: Hans de Goede hdegoede@redhat.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk
Where does this even happen? i915, msm and udl are the only drivers left over that do struct_mutex, and i915 can't really share buffers with msm, and udl doesn't do reservations. How exactly does this still go boom in latest upstream? -Daniel
drivers/gpu/drm/drm_drv.c | 4 +++- include/drm/drm_drv.h | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 2fa4e4fa7c33..82b521146e71 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -488,7 +488,9 @@ int drm_dev_init(struct drm_device *dev,
spin_lock_init(&dev->buf_lock); spin_lock_init(&dev->event_lock);
- mutex_init(&dev->struct_mutex);
- __mutex_init(&dev->struct_mutex,
"&dev->struct_mutex",&driver->class.struct_mutex_key);diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 554104ccb939..5d521923404a 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -39,6 +39,10 @@ struct dma_buf_attachment; struct drm_display_mode; struct drm_mode_create_dumb;
+struct drm_driver_class {
- struct lock_class_key struct_mutex_key;
+};
/* driver capabilities and requirements mask */ #define DRIVER_USE_AGP 0x1 #define DRIVER_LEGACY 0x2 @@ -64,6 +68,8 @@ struct drm_mode_create_dumb;
- structure for GEM drivers.
*/ struct drm_driver {
- struct drm_driver_class class;
- int (*load) (struct drm_device *, unsigned long flags); int (*firstopen) (struct drm_device *); int (*open) (struct drm_device *, struct drm_file *);
-- 2.11.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Sat, Dec 10, 2016 at 10:19:30PM +0100, Daniel Vetter wrote:
On Fri, Dec 09, 2016 at 04:52:32PM +0000, Chris Wilson wrote:
With prime, we are running into false circular dependencies based on the order in which two drivers may lock their own struct_mutex wrt to a common lock (like the reservation->lock). Work around this by adding the lock_class_key to the struct drm_driver such that each driver can have its own subclass of struct_mutex. Circular dependencies between drivers will now be ignored, but real circular dependencies on any one mutex will still be caught. A driver creating more than one device will still need to be careful!
Reported-by: Tobias Klausmann tobias.johannes.klausmann@mni.thm.de Reported-by: Hans de Goede hdegoede@redhat.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk
Where does this even happen? i915, msm and udl are the only drivers left over that do struct_mutex, and i915 can't really share buffers with msm, and udl doesn't do reservations. How exactly does this still go boom in latest upstream?
How about cc: stable?
The reports are nouveau vs i915. I was quite pleased with the drm_driver_class! -Chris
On Sat, Dec 10, 2016 at 09:23:35PM +0000, Chris Wilson wrote:
On Sat, Dec 10, 2016 at 10:19:30PM +0100, Daniel Vetter wrote:
On Fri, Dec 09, 2016 at 04:52:32PM +0000, Chris Wilson wrote:
With prime, we are running into false circular dependencies based on the order in which two drivers may lock their own struct_mutex wrt to a common lock (like the reservation->lock). Work around this by adding the lock_class_key to the struct drm_driver such that each driver can have its own subclass of struct_mutex. Circular dependencies between drivers will now be ignored, but real circular dependencies on any one mutex will still be caught. A driver creating more than one device will still need to be careful!
Reported-by: Tobias Klausmann tobias.johannes.klausmann@mni.thm.de Reported-by: Hans de Goede hdegoede@redhat.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk
Where does this even happen? i915, msm and udl are the only drivers left over that do struct_mutex, and i915 can't really share buffers with msm, and udl doesn't do reservations. How exactly does this still go boom in latest upstream?
How about cc: stable?
The reports are nouveau vs i915. I was quite pleased with the drm_driver_class!
Ah, you may have removed any direct calls to struct_mutex from nouveau, but it is still using struct_mutex around its GEM bo references.
git grep drm_gem_object_unreference_unlocked -- drivers/gpu/drm/nouveau/ | wc -l 13 -Chris
On Sat, Dec 10, 2016 at 09:28:02PM +0000, Chris Wilson wrote:
On Sat, Dec 10, 2016 at 09:23:35PM +0000, Chris Wilson wrote:
On Sat, Dec 10, 2016 at 10:19:30PM +0100, Daniel Vetter wrote:
On Fri, Dec 09, 2016 at 04:52:32PM +0000, Chris Wilson wrote:
With prime, we are running into false circular dependencies based on the order in which two drivers may lock their own struct_mutex wrt to a common lock (like the reservation->lock). Work around this by adding the lock_class_key to the struct drm_driver such that each driver can have its own subclass of struct_mutex. Circular dependencies between drivers will now be ignored, but real circular dependencies on any one mutex will still be caught. A driver creating more than one device will still need to be careful!
Reported-by: Tobias Klausmann tobias.johannes.klausmann@mni.thm.de Reported-by: Hans de Goede hdegoede@redhat.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk
Where does this even happen? i915, msm and udl are the only drivers left over that do struct_mutex, and i915 can't really share buffers with msm, and udl doesn't do reservations. How exactly does this still go boom in latest upstream?
How about cc: stable?
The reports are nouveau vs i915. I was quite pleased with the drm_driver_class!
Ah, you may have removed any direct calls to struct_mutex from nouveau, but it is still using struct_mutex around its GEM bo references.
git grep drm_gem_object_unreference_unlocked -- drivers/gpu/drm/nouveau/ | wc -l 13
Either s/drm_gem_object_unreference_unlocked/__drm_gem_object_unreference/
Or
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 465bacd0a630..824a7780de06 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -826,11 +826,13 @@ drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) return;
dev = obj->dev; - might_lock(&dev->struct_mutex); - - if (dev->driver->gem_free_object_unlocked) + if (dev->driver->gem_free_object_unlocked) { kref_put(&obj->refcount, drm_gem_object_free); - else if (kref_put_mutex(&obj->refcount, drm_gem_object_free, + return; + } + + might_lock(&dev->struct_mutex); + if (kref_put_mutex(&obj->refcount, drm_gem_object_free, &dev->struct_mutex)) mutex_unlock(&dev->struct_mutex); }
That's a false might_lock() that really should be pushed to kref_put_mutex() -Chris
On Sat, Dec 10, 2016 at 10:36 PM, Chris Wilson chris@chris-wilson.co.uk wrote:
On Sat, Dec 10, 2016 at 09:28:02PM +0000, Chris Wilson wrote:
On Sat, Dec 10, 2016 at 09:23:35PM +0000, Chris Wilson wrote:
On Sat, Dec 10, 2016 at 10:19:30PM +0100, Daniel Vetter wrote:
On Fri, Dec 09, 2016 at 04:52:32PM +0000, Chris Wilson wrote:
With prime, we are running into false circular dependencies based on the order in which two drivers may lock their own struct_mutex wrt to a common lock (like the reservation->lock). Work around this by adding the lock_class_key to the struct drm_driver such that each driver can have its own subclass of struct_mutex. Circular dependencies between drivers will now be ignored, but real circular dependencies on any one mutex will still be caught. A driver creating more than one device will still need to be careful!
Reported-by: Tobias Klausmann tobias.johannes.klausmann@mni.thm.de Reported-by: Hans de Goede hdegoede@redhat.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk
Where does this even happen? i915, msm and udl are the only drivers left over that do struct_mutex, and i915 can't really share buffers with msm, and udl doesn't do reservations. How exactly does this still go boom in latest upstream?
How about cc: stable?
The reports are nouveau vs i915. I was quite pleased with the drm_driver_class!
Ah, you may have removed any direct calls to struct_mutex from nouveau, but it is still using struct_mutex around its GEM bo references.
git grep drm_gem_object_unreference_unlocked -- drivers/gpu/drm/nouveau/ | wc -l 13
Either s/drm_gem_object_unreference_unlocked/__drm_gem_object_unreference/
Or
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 465bacd0a630..824a7780de06 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -826,11 +826,13 @@ drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) return;
dev = obj->dev;
might_lock(&dev->struct_mutex);if (dev->driver->gem_free_object_unlocked)
if (dev->driver->gem_free_object_unlocked) { kref_put(&obj->refcount, drm_gem_object_free);
else if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
return;}might_lock(&dev->struct_mutex);if (kref_put_mutex(&obj->refcount, drm_gem_object_free, &dev->struct_mutex)) mutex_unlock(&dev->struct_mutex);}
That's a false might_lock() that really should be pushed to kref_put_mutex()
My thinking behind adding that might_lock was to make sure core code is still save for drivers which rely upon the struct mutex, by essentially enlisting _all_ drivers to validate this. Given that struct_mutex is indeed on its demise, or at least gem_free_object_unlocked is popular enough maybe time to change that? I.e. I like your diff here ;-) -Daniel
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
kbuild test robot