Re: [Freedreno] [PATCH 04/14] drm/msm: Add priv->mm_lock to protect active/inactive lists
On Sun, Oct 04, 2020 at 12:21:36PM -0700, Rob Clark wrote: > From: Rob Clark > > Rather than relying on the big dev->struct_mutex hammer, introduce a > more specific lock for protecting the bo lists. Most excellent. Reviewed-by: Jordan Crouse > Signed-off-by: Rob Clark > --- > drivers/gpu/drm/msm/msm_debugfs.c | 7 +++ > drivers/gpu/drm/msm/msm_drv.c | 1 + > drivers/gpu/drm/msm/msm_drv.h | 13 +++- > drivers/gpu/drm/msm/msm_gem.c | 28 +++--- > drivers/gpu/drm/msm/msm_gem_shrinker.c | 12 +++ > drivers/gpu/drm/msm/msm_gpu.h | 5 - > 6 files changed, 52 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_debugfs.c > b/drivers/gpu/drm/msm/msm_debugfs.c > index ee2e270f464c..64afbed89821 100644 > --- a/drivers/gpu/drm/msm/msm_debugfs.c > +++ b/drivers/gpu/drm/msm/msm_debugfs.c > @@ -112,6 +112,11 @@ static int msm_gem_show(struct drm_device *dev, struct > seq_file *m) > { > struct msm_drm_private *priv = dev->dev_private; > struct msm_gpu *gpu = priv->gpu; > + int ret; > + > + ret = mutex_lock_interruptible(&priv->mm_lock); > + if (ret) > + return ret; > > if (gpu) { > seq_printf(m, "Active Objects (%s):\n", gpu->name); > @@ -121,6 +126,8 @@ static int msm_gem_show(struct drm_device *dev, struct > seq_file *m) > seq_printf(m, "Inactive Objects:\n"); > msm_gem_describe_objects(&priv->inactive_list, m); > > + mutex_unlock(&priv->mm_lock); > + > return 0; > } > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index 49685571dc0e..dc6efc089285 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -441,6 +441,7 @@ static int msm_drm_init(struct device *dev, struct > drm_driver *drv) > init_llist_head(&priv->free_list); > > INIT_LIST_HEAD(&priv->inactive_list); > + mutex_init(&priv->mm_lock); > > drm_mode_config_init(ddev); > > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > index b9dd8f8f4887..50978e5db376 100644 > --- a/drivers/gpu/drm/msm/msm_drv.h > +++ b/drivers/gpu/drm/msm/msm_drv.h > @@ -174,8 +174,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/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c > index a870b3ad129d..b04ed8b52f9d 100644 > --- a/drivers/gpu/drm/msm/msm_gem.c > +++ b/drivers/gpu/drm/msm/msm_gem.c > @@ -746,13 +746,17 @@ int msm_gem_sync_object(struct drm_gem_object *obj, > void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu) > { > struct msm_gem_object *msm_obj = to_msm_bo(obj); > - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); > + struct msm_drm_private *priv = obj->dev->dev_private; > + > + might_sleep(); > WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED); > > if (!atomic_fetch_inc(&msm_obj->active_count)) { > + mutex_lock(&priv->mm_lock); > msm_obj->gpu = gpu; > list_del_init(&msm_obj->mm_list); > list_add_tail(&msm_obj->mm_list, &gpu->active_list); > + mutex_unlock(&priv->mm_lock); > } > } > > @@ -761,12 +765,14 @@ void msm_gem_active_put(struct drm_gem_object *obj) > struct msm_gem_object *msm_obj = to_msm_bo(obj); > struct msm_drm_private *priv = obj->dev->dev_private; > > - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); > + might_sleep(); > > if (!atomic_dec_return(&msm_obj->active_count)) { > + mutex_lock(&priv->mm_lock); > msm_obj->gpu = NULL; > list_del_init(&msm_obj->mm_list); > list_add_tail(&msm_obj->mm_list, &priv->inactive_list); > + mutex_unlock(&priv->mm_lock); > } > } > > @@ -921,13 +927,16 @@ 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; > > WARN_ON(!mutex_is_locked(&dev->struct
Re: [PATCH 04/14] drm/msm: Add priv->mm_lock to protect active/inactive lists
On Sun, Oct 4, 2020 at 3:15 PM Daniel Vetter wrote: > > On Sun, Oct 4, 2020 at 9:21 PM Rob Clark wrote: > > > > From: Rob Clark > > > > Rather than relying on the big dev->struct_mutex hammer, introduce a > > more specific lock for protecting the bo lists. > > > > Signed-off-by: Rob Clark > > --- > > drivers/gpu/drm/msm/msm_debugfs.c | 7 +++ > > drivers/gpu/drm/msm/msm_drv.c | 1 + > > drivers/gpu/drm/msm/msm_drv.h | 13 +++- > > drivers/gpu/drm/msm/msm_gem.c | 28 +++--- > > drivers/gpu/drm/msm/msm_gem_shrinker.c | 12 +++ > > drivers/gpu/drm/msm/msm_gpu.h | 5 - > > 6 files changed, 52 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/msm_debugfs.c > > b/drivers/gpu/drm/msm/msm_debugfs.c > > index ee2e270f464c..64afbed89821 100644 > > --- a/drivers/gpu/drm/msm/msm_debugfs.c > > +++ b/drivers/gpu/drm/msm/msm_debugfs.c > > @@ -112,6 +112,11 @@ static int msm_gem_show(struct drm_device *dev, struct > > seq_file *m) > > { > > struct msm_drm_private *priv = dev->dev_private; > > struct msm_gpu *gpu = priv->gpu; > > + int ret; > > + > > + ret = mutex_lock_interruptible(&priv->mm_lock); > > + if (ret) > > + return ret; > > > > if (gpu) { > > seq_printf(m, "Active Objects (%s):\n", gpu->name); > > @@ -121,6 +126,8 @@ static int msm_gem_show(struct drm_device *dev, struct > > seq_file *m) > > seq_printf(m, "Inactive Objects:\n"); > > msm_gem_describe_objects(&priv->inactive_list, m); > > > > + mutex_unlock(&priv->mm_lock); > > + > > return 0; > > } > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > > index 49685571dc0e..dc6efc089285 100644 > > --- a/drivers/gpu/drm/msm/msm_drv.c > > +++ b/drivers/gpu/drm/msm/msm_drv.c > > @@ -441,6 +441,7 @@ static int msm_drm_init(struct device *dev, struct > > drm_driver *drv) > > init_llist_head(&priv->free_list); > > > > INIT_LIST_HEAD(&priv->inactive_list); > > + mutex_init(&priv->mm_lock); > > I highly recommend you drop a > > fs_reclaim_acquire(GFP_KERNEL); > might_lock(&priv->mm_lock); > fs_reclaim_release(GFP_KERNEL); > > in here to teach lockdep about your ordering against the shrinker. > Gives you full testing every boot, even if your shrinker never gets > called. Good idea.. (tbf, I have tested this with android+lockdep which pretty is great shrinker exercise.. but immediate notification of future problems is a good plan) BR, -R
Re: [PATCH 04/14] drm/msm: Add priv->mm_lock to protect active/inactive lists
On Sun, Oct 4, 2020 at 9:21 PM Rob Clark wrote: > > From: Rob Clark > > Rather than relying on the big dev->struct_mutex hammer, introduce a > more specific lock for protecting the bo lists. > > Signed-off-by: Rob Clark > --- > drivers/gpu/drm/msm/msm_debugfs.c | 7 +++ > drivers/gpu/drm/msm/msm_drv.c | 1 + > drivers/gpu/drm/msm/msm_drv.h | 13 +++- > drivers/gpu/drm/msm/msm_gem.c | 28 +++--- > drivers/gpu/drm/msm/msm_gem_shrinker.c | 12 +++ > drivers/gpu/drm/msm/msm_gpu.h | 5 - > 6 files changed, 52 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_debugfs.c > b/drivers/gpu/drm/msm/msm_debugfs.c > index ee2e270f464c..64afbed89821 100644 > --- a/drivers/gpu/drm/msm/msm_debugfs.c > +++ b/drivers/gpu/drm/msm/msm_debugfs.c > @@ -112,6 +112,11 @@ static int msm_gem_show(struct drm_device *dev, struct > seq_file *m) > { > struct msm_drm_private *priv = dev->dev_private; > struct msm_gpu *gpu = priv->gpu; > + int ret; > + > + ret = mutex_lock_interruptible(&priv->mm_lock); > + if (ret) > + return ret; > > if (gpu) { > seq_printf(m, "Active Objects (%s):\n", gpu->name); > @@ -121,6 +126,8 @@ static int msm_gem_show(struct drm_device *dev, struct > seq_file *m) > seq_printf(m, "Inactive Objects:\n"); > msm_gem_describe_objects(&priv->inactive_list, m); > > + mutex_unlock(&priv->mm_lock); > + > return 0; > } > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index 49685571dc0e..dc6efc089285 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -441,6 +441,7 @@ static int msm_drm_init(struct device *dev, struct > drm_driver *drv) > init_llist_head(&priv->free_list); > > INIT_LIST_HEAD(&priv->inactive_list); > + mutex_init(&priv->mm_lock); I highly recommend you drop a fs_reclaim_acquire(GFP_KERNEL); might_lock(&priv->mm_lock); fs_reclaim_release(GFP_KERNEL); in here to teach lockdep about your ordering against the shrinker. Gives you full testing every boot, even if your shrinker never gets called. -Daniel > > drm_mode_config_init(ddev); > > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > index b9dd8f8f4887..50978e5db376 100644 > --- a/drivers/gpu/drm/msm/msm_drv.h > +++ b/drivers/gpu/drm/msm/msm_drv.h > @@ -174,8 +174,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/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c > index a870b3ad129d..b04ed8b52f9d 100644 > --- a/drivers/gpu/drm/msm/msm_gem.c > +++ b/drivers/gpu/drm/msm/msm_gem.c > @@ -746,13 +746,17 @@ int msm_gem_sync_object(struct drm_gem_object *obj, > void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu) > { > struct msm_gem_object *msm_obj = to_msm_bo(obj); > - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); > + struct msm_drm_private *priv = obj->dev->dev_private; > + > + might_sleep(); > WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED); > > if (!atomic_fetch_inc(&msm_obj->active_count)) { > + mutex_lock(&priv->mm_lock); > msm_obj->gpu = gpu; > list_del_init(&msm_obj->mm_list); > list_add_tail(&msm_obj->mm_list, &gpu->active_list); > + mutex_unlock(&priv->mm_lock); > } > } > > @@ -761,12 +765,14 @@ void msm_gem_active_put(struct drm_gem_object *obj) > struct msm_gem_object *msm_obj = to_msm_bo(obj); > struct msm_drm_private *priv = obj->dev->dev_private; > > - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); > + might_sleep(); > > if (!atomic_dec_return(&msm_obj->active_count)) { > + mutex_lock(&priv->mm_lock); > msm_obj->gpu = NULL; > list_del_init(&msm_obj->mm_list); > list_add_tail(&msm_obj->mm_list, &priv->inactive_list); > + mutex_unlock(&priv->mm_
[PATCH 04/14] drm/msm: Add priv->mm_lock to protect active/inactive lists
From: Rob Clark Rather than relying on the big dev->struct_mutex hammer, introduce a more specific lock for protecting the bo lists. Signed-off-by: Rob Clark --- drivers/gpu/drm/msm/msm_debugfs.c | 7 +++ drivers/gpu/drm/msm/msm_drv.c | 1 + drivers/gpu/drm/msm/msm_drv.h | 13 +++- drivers/gpu/drm/msm/msm_gem.c | 28 +++--- drivers/gpu/drm/msm/msm_gem_shrinker.c | 12 +++ drivers/gpu/drm/msm/msm_gpu.h | 5 - 6 files changed, 52 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c index ee2e270f464c..64afbed89821 100644 --- a/drivers/gpu/drm/msm/msm_debugfs.c +++ b/drivers/gpu/drm/msm/msm_debugfs.c @@ -112,6 +112,11 @@ static int msm_gem_show(struct drm_device *dev, struct seq_file *m) { struct msm_drm_private *priv = dev->dev_private; struct msm_gpu *gpu = priv->gpu; + int ret; + + ret = mutex_lock_interruptible(&priv->mm_lock); + if (ret) + return ret; if (gpu) { seq_printf(m, "Active Objects (%s):\n", gpu->name); @@ -121,6 +126,8 @@ static int msm_gem_show(struct drm_device *dev, struct seq_file *m) seq_printf(m, "Inactive Objects:\n"); msm_gem_describe_objects(&priv->inactive_list, m); + mutex_unlock(&priv->mm_lock); + return 0; } diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 49685571dc0e..dc6efc089285 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -441,6 +441,7 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) init_llist_head(&priv->free_list); INIT_LIST_HEAD(&priv->inactive_list); + mutex_init(&priv->mm_lock); drm_mode_config_init(ddev); diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index b9dd8f8f4887..50978e5db376 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -174,8 +174,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/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index a870b3ad129d..b04ed8b52f9d 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -746,13 +746,17 @@ int msm_gem_sync_object(struct drm_gem_object *obj, void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu) { struct msm_gem_object *msm_obj = to_msm_bo(obj); - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); + struct msm_drm_private *priv = obj->dev->dev_private; + + might_sleep(); WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED); if (!atomic_fetch_inc(&msm_obj->active_count)) { + mutex_lock(&priv->mm_lock); msm_obj->gpu = gpu; list_del_init(&msm_obj->mm_list); list_add_tail(&msm_obj->mm_list, &gpu->active_list); + mutex_unlock(&priv->mm_lock); } } @@ -761,12 +765,14 @@ void msm_gem_active_put(struct drm_gem_object *obj) struct msm_gem_object *msm_obj = to_msm_bo(obj); struct msm_drm_private *priv = obj->dev->dev_private; - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); + might_sleep(); if (!atomic_dec_return(&msm_obj->active_count)) { + mutex_lock(&priv->mm_lock); msm_obj->gpu = NULL; list_del_init(&msm_obj->mm_list); list_add_tail(&msm_obj->mm_list, &priv->inactive_list); + mutex_unlock(&priv->mm_lock); } } @@ -921,13 +927,16 @@ 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; WARN_ON(!mutex_is_locked(&dev->struct_mutex)); /* 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_ob