On Mon, Dec 11, 2017 at 11:39 AM, Chris Wilson <ch...@chris-wilson.co.uk> wrote:
> Teach lockdep to track the device's internal mmapping separately
> from the generic lockclass over all other inodes. Since this is device
> private we wish to allow a different locking hierarchy than is typified
> by the requirement for the mmap_rwsem being the outermost lock for
> handling pagefaults. By giving the internal mmap_rwsem a distinct
> lockclass, lockdep can identify it and learn/enforce its distinct locking
> requirements.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104209
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
I think both the commit message and comment are a bit too fluffy - the
critical bit is that we're biting ourselves on gtt mmaps from
usersptr, and that's explicitly not allowed exactly because it would
deadlock.
I'm also not sure it's a good idea to implement this in generic code,
since this is a very i915 specific issue, and other drivers (who might
be a lot less sloppy here) will now no longer get reports about this
deadlock.
Aside from that I'm not really sure why you think the bugzilla link is
a false positive: The mapping->rwsem is the one for the gtt in both
cases I think.
-Daniel
> ---
> drivers/gpu/drm/drm_drv.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 9acc1e157813..21ad06c3d684 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -393,6 +393,7 @@ static struct file_system_type drm_fs_type = {
>
> static struct inode *drm_fs_inode_new(void)
> {
> + static struct lock_class_key lockclass;
> struct inode *inode;
> int r;
>
> @@ -403,8 +404,22 @@ static struct inode *drm_fs_inode_new(void)
> }
>
> inode = alloc_anon_inode(drm_fs_mnt->mnt_sb);
> - if (IS_ERR(inode))
> + if (IS_ERR(inode)) {
> simple_release_fs(&drm_fs_mnt, &drm_fs_cnt);
> + return inode;
> + }
> +
> + /*
> + * Teach lockdep to track the device's internal mmapping separately
> + * from all other inodes. Since this is device private we wish to
> + * allow a different locking hierarchy than is typified by the
> + * requirement for the mmap_rwsem being the outermost lock for
> + * handling pagefaults. By giving the internal mmap_rwsem a distinct
> + * lockclass, lockdep can identify it and thereby learn and enforce
> its
> + * distinct locking requirements.
> + */
> + lockdep_set_class_and_name(&inode->i_mapping->i_mmap_rwsem,
> + &lockclass, "drm_fs_inode");
>
> return inode;
> }
> --
> 2.15.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
