[Intel-gfx] [PATCH] drm: Make each driver's struct_mutex its own subclass
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-struct_mutex-its-own-subclass/20161210-33 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'
[Intel-gfx] [PATCH] drm: Make each driver's struct_mutex its own subclass
On Sat, Dec 10, 2016 at 10:36 PM, Chris Wilson
wrote:
> On Sat, Dec 10, 2016 at 09:28:02PM +, Chris Wilson wrote:
>> On Sat, Dec 10, 2016 at 09:23:35PM +, 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 +, 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
>> > > > Reported-by: Hans de Goede
>> > > > Signed-off-by: Chris Wilson
>> > >
>> > > 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(>struct_mutex);
> -
> - if (dev->driver->gem_free_object_unlocked)
> + if (dev->driver->gem_free_object_unlocked) {
> kref_put(>refcount, drm_gem_object_free);
> - else if (kref_put_mutex(>refcount, drm_gem_object_free,
> + return;
> + }
> +
> + might_lock(>struct_mutex);
> + if (kref_put_mutex(>refcount, drm_gem_object_free,
> >struct_mutex))
> mutex_unlock(>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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
[Intel-gfx] [PATCH] drm: Make each driver's struct_mutex its own subclass
On Fri, Dec 09, 2016 at 04:52:32PM +, 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
> Reported-by: Hans de Goede
> Signed-off-by: Chris Wilson
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(>buf_lock);
> spin_lock_init(>event_lock);
> - mutex_init(>struct_mutex);
> + __mutex_init(>struct_mutex,
> + ">struct_mutex",
> + >class.struct_mutex_key);
> mutex_init(>filelist_mutex);
> mutex_init(>ctxlist_mutex);
> mutex_init(>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_LEGACY0x2
> @@ -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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[Intel-gfx] [PATCH] drm: Make each driver's struct_mutex its own subclass
On Sat, Dec 10, 2016 at 09:28:02PM +, Chris Wilson wrote:
> On Sat, Dec 10, 2016 at 09:23:35PM +, 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 +, 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
> > > > Reported-by: Hans de Goede
> > > > Signed-off-by: Chris Wilson
> > >
> > > 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(>struct_mutex);
-
- if (dev->driver->gem_free_object_unlocked)
+ if (dev->driver->gem_free_object_unlocked) {
kref_put(>refcount, drm_gem_object_free);
- else if (kref_put_mutex(>refcount, drm_gem_object_free,
+ return;
+ }
+
+ might_lock(>struct_mutex);
+ if (kref_put_mutex(>refcount, drm_gem_object_free,
>struct_mutex))
mutex_unlock(>struct_mutex);
}
That's a false might_lock() that really should be pushed to kref_put_mutex()
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
[Intel-gfx] [PATCH] drm: Make each driver's struct_mutex its own subclass
On Sat, Dec 10, 2016 at 09:23:35PM +, 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 +, 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 > > > Reported-by: Hans de Goede > > > Signed-off-by: Chris Wilson > > > > 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 -- Chris Wilson, Intel Open Source Technology Centre
[Intel-gfx] [PATCH] drm: Make each driver's struct_mutex its own subclass
On Sat, Dec 10, 2016 at 10:19:30PM +0100, Daniel Vetter wrote: > On Fri, Dec 09, 2016 at 04:52:32PM +, 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 > > Reported-by: Hans de Goede > > Signed-off-by: Chris Wilson > > 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 -- Chris Wilson, Intel Open Source Technology Centre
