From a145e793b2fe00da9a5811c1df7af60a46752109 Mon Sep 17 00:00:00 2001 From: "Kristian H. Kristensen" Date: Tue, 23 Mar 2021 13:35:31 -0700 Subject: [PATCH 1/3] drm/msm: Implement .gem_free_object_unlocked We use a llist and a worker to delay the object cleanup. This avoids taking mmap_sem and struct mutex in the wrong order when calling drm_gem_object_put-unlocked() from drm_gem_mmap(). Fixes lockdep problem with copy_from_user() in msm_ioctl_gem_submit(). Change-Id: Idfe54ae8108158b69f3835f26991642d1e21f8ee Signed-off-by: Kristian H. Kristensen Signed-off-by: Rob Clark Git-commit: 48e7f18392c66f9b69ebac11c54f1a2e033ced54 Git-repo: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git [samtran@codeaurora.org: resolve trivial merge conflict] Signed-off-by: Samantha Tran --- msm/msm_drv.c | 3 +++ msm/msm_drv.h | 5 +++++ msm/msm_gem.c | 31 +++++++++++++++++++++++++++++++ msm/msm_gem.h | 3 +++ 4 files changed, 42 insertions(+) diff --git a/msm/msm_drv.c b/msm/msm_drv.c index ede9d2bca7..e1d2093f51 100644 --- a/msm/msm_drv.c +++ b/msm/msm_drv.c @@ -792,6 +792,9 @@ static int msm_drm_component_init(struct device *dev) priv->wq = alloc_ordered_workqueue("msm_drm", 0); init_waitqueue_head(&priv->pending_crtcs_event); + INIT_WORK(&priv->free_work, msm_gem_free_work); + init_llist_head(&priv->free_list); + INIT_LIST_HEAD(&priv->client_event_list); INIT_LIST_HEAD(&priv->inactive_list); INIT_LIST_HEAD(&priv->vm_client_list); diff --git a/msm/msm_drv.h b/msm/msm_drv.h index 09accfe2db..f65fc6e1de 100644 --- a/msm/msm_drv.h +++ b/msm/msm_drv.h @@ -891,6 +891,10 @@ struct msm_drm_private { /* list of GEM objects: */ struct list_head inactive_list; + /* worker for delayed free of objects: */ + struct work_struct free_work; + struct llist_head free_list; + struct workqueue_struct *wq; /* crtcs pending async atomic updates: */ @@ -1130,6 +1134,7 @@ void *msm_gem_kernel_new_locked(struct drm_device *dev, uint32_t size, struct drm_gem_object **bo, uint64_t *iova); struct drm_gem_object *msm_gem_import(struct drm_device *dev, struct dma_buf *dmabuf, struct sg_table *sgt); +void msm_gem_free_work(struct work_struct *work); __printf(2, 3) void msm_gem_object_set_name(struct drm_gem_object *bo, const char *fmt, ...); diff --git a/msm/msm_gem.c b/msm/msm_gem.c index 2c6ab4091e..562691b6e4 100644 --- a/msm/msm_gem.c +++ b/msm/msm_gem.c @@ -1007,6 +1007,16 @@ void msm_gem_describe_objects(struct list_head *list, struct seq_file *m) void msm_gem_free_object(struct drm_gem_object *obj) { struct msm_gem_object *msm_obj = to_msm_bo(obj); + struct drm_device *dev = obj->dev; + struct msm_drm_private *priv = dev->dev_private; + + if (llist_add(&msm_obj->freed, &priv->free_list)) + queue_work(priv->wq, &priv->free_work); +} + +static void free_object(struct msm_gem_object *msm_obj) +{ + struct drm_gem_object *obj = &msm_obj->base; /* object should not be on active list: */ WARN_ON(is_active(msm_obj)); @@ -1048,6 +1058,27 @@ void msm_gem_free_object(struct drm_gem_object *obj) kfree(msm_obj); } +void msm_gem_free_work(struct work_struct *work) +{ + struct msm_drm_private *priv = container_of(work, struct msm_drm_private, free_work); + struct drm_device *dev = priv->dev; + struct llist_node *freed; + struct msm_gem_object *msm_obj, *next; + + while ((freed = llist_del_all(&priv->free_list))) { + + mutex_lock(&dev->struct_mutex); + + llist_for_each_entry_safe(msm_obj, next, freed, freed) + free_object(msm_obj); + + mutex_unlock(&dev->struct_mutex); + + if (need_resched()) + break; + } +} + /* convenience method to construct a GEM buffer object, and userspace handle */ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file, uint32_t size, uint32_t flags, uint32_t *handle, diff --git a/msm/msm_gem.h b/msm/msm_gem.h index 48759fdf96..3e224a5bc2 100644 --- a/msm/msm_gem.h +++ b/msm/msm_gem.h @@ -125,6 +125,8 @@ struct msm_gem_object { struct list_head vmas; /* list of msm_gem_vma */ + struct llist_node freed; + /* normally (resv == &_resv) except for imported bo's */ struct dma_resv *resv; struct dma_resv _resv; @@ -182,6 +184,7 @@ enum msm_gem_lock { void msm_gem_purge(struct drm_gem_object *obj, enum msm_gem_lock subclass); void msm_gem_vunmap(struct drm_gem_object *obj, enum msm_gem_lock subclass); +void msm_gem_free_work(struct work_struct *work); /* Created per submit-ioctl, to track bo's and cmdstream bufs, etc, * associated with the cmdstream submission for synchronization (and From fd50a1ec69d91f03a518bd3ef62f5b7c9a5d0b53 Mon Sep 17 00:00:00 2001 From: Rob Clark Date: Tue, 23 Mar 2021 13:49:55 -0700 Subject: [PATCH 2/3] drm/msm: Add priv->mm_lock to protect active/inactive lists Rather than relying on the big dev->struct_mutex hammer, introduce a more specific lock for protecting the bo lists. Change-Id: I4c876a1c3ae51ff62372703a99a8daff0c4a7950 Signed-off-by: Rob Clark Git-commit: d984457b31c4c53d2af374d5e78b3eb64debd483 Git-repo: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git [samtran@codeaurora.org: avoid changes related to debugfs and shrinker] Signed-off-by: Samantha Tran --- msm/msm_drv.c | 1 + msm/msm_drv.h | 13 ++++++++++++- msm/msm_gem.c | 15 +++++++-------- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/msm/msm_drv.c b/msm/msm_drv.c index e1d2093f51..061e50746d 100644 --- a/msm/msm_drv.c +++ b/msm/msm_drv.c @@ -798,6 +798,7 @@ static int msm_drm_component_init(struct device *dev) INIT_LIST_HEAD(&priv->client_event_list); INIT_LIST_HEAD(&priv->inactive_list); INIT_LIST_HEAD(&priv->vm_client_list); + mutex_init(&priv->mm_lock); mutex_init(&priv->vm_client_lock); diff --git a/msm/msm_drv.h b/msm/msm_drv.h index f65fc6e1de..02fa383f5f 100644 --- a/msm/msm_drv.h +++ b/msm/msm_drv.h @@ -888,8 +888,19 @@ struct msm_drm_private { struct msm_rd_state *hangrd; /* debugfs to dump hanging submits */ struct msm_perf_state *perf; - /* list of GEM objects: */ + /* + * List of inactive GEM objects. Every bo is either in the inactive_list + * or gpu->active_list (for the gpu it is active on[1]) + * + * These lists are protected by mm_lock. If struct_mutex is involved, it + * should be aquired prior to mm_lock. One should *not* hold mm_lock in + * get_pages()/vmap()/etc paths, as they can trigger the shrinker. + * + * [1] if someone ever added support for the old 2d cores, there could be + * more than one gpu object + */ struct list_head inactive_list; + struct mutex mm_lock; /* worker for delayed free of objects: */ struct work_struct free_work; diff --git a/msm/msm_gem.c b/msm/msm_gem.c index 562691b6e4..3e80e65262 100644 --- a/msm/msm_gem.c +++ b/msm/msm_gem.c @@ -1017,11 +1017,15 @@ void msm_gem_free_object(struct drm_gem_object *obj) static void free_object(struct msm_gem_object *msm_obj) { struct drm_gem_object *obj = &msm_obj->base; + struct drm_device *dev = obj->dev; + struct msm_drm_private *priv = dev->dev_private; /* object should not be on active list: */ WARN_ON(is_active(msm_obj)); + mutex_lock(&priv->mm_lock); list_del(&msm_obj->mm_list); + mutex_unlock(&priv->mm_lock); mutex_lock(&msm_obj->lock); @@ -1149,14 +1153,9 @@ static int msm_gem_new_impl(struct drm_device *dev, msm_obj->in_active_list = false; msm_obj->obj_dirty = false; - if (struct_mutex_locked) { - WARN_ON(!mutex_is_locked(&dev->struct_mutex)); - list_add_tail(&msm_obj->mm_list, &priv->inactive_list); - } else { - mutex_lock(&dev->struct_mutex); - list_add_tail(&msm_obj->mm_list, &priv->inactive_list); - mutex_unlock(&dev->struct_mutex); - } + mutex_lock(&priv->mm_lock); + list_add_tail(&msm_obj->mm_list, &priv->inactive_list); + mutex_unlock(&priv->mm_lock); *obj = &msm_obj->base; From 245ac819b6031ecba4b89fa65c9e7d617b68aded Mon Sep 17 00:00:00 2001 From: Rob Clark Date: Tue, 23 Mar 2021 14:05:41 -0700 Subject: [PATCH 3/3] drm/msm: remove msm_gem_free_work Now that we don't need struct_mutex in the free path, we can get rid of the asynchronous free all together. Change-Id: I82406450e3a5d0d49d3fb753c621f55e8f4af088 Signed-off-by: Rob Clark Git-commit: c951a9b284b907604759628d273901064c60d09f Git-repo: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git Signed-off-by: Samantha Tran --- msm/msm_drv.c | 3 --- msm/msm_drv.h | 5 ----- msm/msm_gem.c | 31 ------------------------------- msm/msm_gem.h | 1 - 4 files changed, 40 deletions(-) diff --git a/msm/msm_drv.c b/msm/msm_drv.c index 061e50746d..519357d37a 100644 --- a/msm/msm_drv.c +++ b/msm/msm_drv.c @@ -792,9 +792,6 @@ static int msm_drm_component_init(struct device *dev) priv->wq = alloc_ordered_workqueue("msm_drm", 0); init_waitqueue_head(&priv->pending_crtcs_event); - INIT_WORK(&priv->free_work, msm_gem_free_work); - init_llist_head(&priv->free_list); - INIT_LIST_HEAD(&priv->client_event_list); INIT_LIST_HEAD(&priv->inactive_list); INIT_LIST_HEAD(&priv->vm_client_list); diff --git a/msm/msm_drv.h b/msm/msm_drv.h index 02fa383f5f..3a5c19a73c 100644 --- a/msm/msm_drv.h +++ b/msm/msm_drv.h @@ -902,10 +902,6 @@ struct msm_drm_private { struct list_head inactive_list; struct mutex mm_lock; - /* worker for delayed free of objects: */ - struct work_struct free_work; - struct llist_head free_list; - struct workqueue_struct *wq; /* crtcs pending async atomic updates: */ @@ -1145,7 +1141,6 @@ void *msm_gem_kernel_new_locked(struct drm_device *dev, uint32_t size, struct drm_gem_object **bo, uint64_t *iova); struct drm_gem_object *msm_gem_import(struct drm_device *dev, struct dma_buf *dmabuf, struct sg_table *sgt); -void msm_gem_free_work(struct work_struct *work); __printf(2, 3) void msm_gem_object_set_name(struct drm_gem_object *bo, const char *fmt, ...); diff --git a/msm/msm_gem.c b/msm/msm_gem.c index 3e80e65262..f032efba6f 100644 --- a/msm/msm_gem.c +++ b/msm/msm_gem.c @@ -1010,16 +1010,6 @@ void msm_gem_free_object(struct drm_gem_object *obj) struct drm_device *dev = obj->dev; struct msm_drm_private *priv = dev->dev_private; - if (llist_add(&msm_obj->freed, &priv->free_list)) - queue_work(priv->wq, &priv->free_work); -} - -static void free_object(struct msm_gem_object *msm_obj) -{ - struct drm_gem_object *obj = &msm_obj->base; - struct drm_device *dev = obj->dev; - struct msm_drm_private *priv = dev->dev_private; - /* object should not be on active list: */ WARN_ON(is_active(msm_obj)); @@ -1062,27 +1052,6 @@ static void free_object(struct msm_gem_object *msm_obj) kfree(msm_obj); } -void msm_gem_free_work(struct work_struct *work) -{ - struct msm_drm_private *priv = container_of(work, struct msm_drm_private, free_work); - struct drm_device *dev = priv->dev; - struct llist_node *freed; - struct msm_gem_object *msm_obj, *next; - - while ((freed = llist_del_all(&priv->free_list))) { - - mutex_lock(&dev->struct_mutex); - - llist_for_each_entry_safe(msm_obj, next, freed, freed) - free_object(msm_obj); - - mutex_unlock(&dev->struct_mutex); - - if (need_resched()) - break; - } -} - /* convenience method to construct a GEM buffer object, and userspace handle */ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file, uint32_t size, uint32_t flags, uint32_t *handle, diff --git a/msm/msm_gem.h b/msm/msm_gem.h index 3e224a5bc2..eba6f94d4a 100644 --- a/msm/msm_gem.h +++ b/msm/msm_gem.h @@ -184,7 +184,6 @@ enum msm_gem_lock { void msm_gem_purge(struct drm_gem_object *obj, enum msm_gem_lock subclass); void msm_gem_vunmap(struct drm_gem_object *obj, enum msm_gem_lock subclass); -void msm_gem_free_work(struct work_struct *work); /* Created per submit-ioctl, to track bo's and cmdstream bufs, etc, * associated with the cmdstream submission for synchronization (and