On Sun, Oct 04, 2020 at 12:21:34PM -0700, Rob Clark wrote:

From: Rob Clark robdclark@chromium.org

It is somewhat redundant with the gpu tracepoints, and anyways not too useful to justify spamming the log when debug traces are enabled.

Reviewed-by: Jordan Crouse jcrouse@codeaurora.org

Signed-off-by: Rob Clark robdclark@chromium.org

drivers/gpu/drm/msm/msm_gpu.c | 1 - 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index 55d16489d0f3..31fce3ac0cdc 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -535,7 +535,6 @@ static void recover_worker(struct work_struct *work)

static void hangcheck_timer_reset(struct msm_gpu *gpu) {

• DBG("%s", gpu->name); mod_timer(&gpu->hangcheck_timer, round_jiffies_up(jiffies + DRM_MSM_HANGCHECK_JIFFIES));

}

2.26.2

---

Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno

On Sun, Oct 04, 2020 at 12:21:35PM -0700, Rob Clark wrote:

From: Rob Clark robdclark@chromium.org

Small cleanup, update_fences() is used in the hangcheck path, but also in the normal retire path.

Reviewed-by: Jordan Crouse jcrouse@codeaurora.org

Signed-off-by: Rob Clark robdclark@chromium.org

drivers/gpu/drm/msm/msm_gpu.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index 31fce3ac0cdc..ca8c95b32c8b 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -265,6 +265,20 @@ int msm_gpu_hw_init(struct msm_gpu *gpu) return ret; }

+static void update_fences(struct msm_gpu *gpu, struct msm_ringbuffer *ring,

• uint32_t fence)
  

+{

• struct msm_gem_submit *submit;
• list_for_each_entry(submit, &ring->submits, node) {

• if (submit->seqno > fence)
  

• 	break;
  

• msm_update_fence(submit->ring->fctx,
  

• 	submit->fence->seqno);
  

• }

+}

#ifdef CONFIG_DEV_COREDUMP static ssize_t msm_gpu_devcoredump_read(char *buffer, loff_t offset, size_t count, void *data, size_t datalen) @@ -411,20 +425,6 @@ static void msm_gpu_crashstate_capture(struct msm_gpu *gpu,

• Hangcheck detection for locked gpu:

*/

-static void update_fences(struct msm_gpu *gpu, struct msm_ringbuffer *ring,

• uint32_t fence)
  

-{

• struct msm_gem_submit *submit;
• list_for_each_entry(submit, &ring->submits, node) {

• if (submit->seqno > fence)
  

• 	break;
  

• msm_update_fence(submit->ring->fctx,
  

• 	submit->fence->seqno);
  

• }

-}

static struct msm_gem_submit * find_submit(struct msm_ringbuffer *ring, uint32_t fence) { -- 2.26.2

---

Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno

On Sun, Oct 4, 2020 at 9:21 PM Rob Clark robdclark@gmail.com wrote:

From: Rob Clark robdclark@chromium.org

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 robdclark@chromium.org

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_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_obj->lock);
  
  

@@ -1103,14 +1112,9 @@ static struct drm_gem_object *_msm_gem_new(struct drm_device *dev, mapping_set_gfp_mask(obj->filp->f_mapping, GFP_HIGHUSER); }

•   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);
  
    return obj;
  
  

@@ -1178,9 +1182,9 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,

    mutex_unlock(&msm_obj->lock);

•   mutex_lock(&dev->struct_mutex);
  

•   mutex_lock(&priv->mm_lock);
    list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
  

•   mutex_unlock(&dev->struct_mutex);
  

•   mutex_unlock(&priv->mm_lock);
  
    return obj;
  
  

diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c b/drivers/gpu/drm/msm/msm_gem_shrinker.c index 482576d7a39a..c41b84a3a484 100644 --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c @@ -51,11 +51,15 @@ msm_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc) if (!msm_gem_shrinker_lock(dev, &unlock)) return 0;

•   mutex_lock(&priv->mm_lock);
  

•   list_for_each_entry(msm_obj, &priv->inactive_list, mm_list) {
            if (is_purgeable(msm_obj))
                    count += msm_obj->base.size >> PAGE_SHIFT;
    }
  
  

•   mutex_unlock(&priv->mm_lock);
  

•   if (unlock)
            mutex_unlock(&dev->struct_mutex);
  
  

@@ -75,6 +79,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) if (!msm_gem_shrinker_lock(dev, &unlock)) return SHRINK_STOP;

•   mutex_lock(&priv->mm_lock);
  

•   list_for_each_entry(msm_obj, &priv->inactive_list, mm_list) {
            if (freed >= sc->nr_to_scan)
                    break;
  

@@ -84,6 +90,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) } }

•   mutex_unlock(&priv->mm_lock);
  

•   if (unlock)
            mutex_unlock(&dev->struct_mutex);
  
  

@@ -106,6 +114,8 @@ msm_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr) if (!msm_gem_shrinker_lock(dev, &unlock)) return NOTIFY_DONE;

•   mutex_lock(&priv->mm_lock);
  

•   list_for_each_entry(msm_obj, &priv->inactive_list, mm_list) {
            if (is_vunmapable(msm_obj)) {
                    msm_gem_vunmap(&msm_obj->base, OBJ_LOCK_SHRINKER);
  

@@ -118,6 +128,8 @@ msm_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr) } }

•   mutex_unlock(&priv->mm_lock);
  

•   if (unlock)
            mutex_unlock(&dev->struct_mutex);
  
  

diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h index 6c9e1fdc1a76..1806e87600c0 100644 --- a/drivers/gpu/drm/msm/msm_gpu.h +++ b/drivers/gpu/drm/msm/msm_gpu.h @@ -94,7 +94,10 @@ struct msm_gpu { struct msm_ringbuffer *rb[MSM_GPU_MAX_RINGS]; int nr_rings;

•   /* list of GEM active objects: */
  

•   /*
  

•    * List of GEM active objects on this gpu.  Protected by
  

•    * msm_drm_private::mm_lock
  

•    */
    struct list_head active_list;
  
    /* does gpu need hw_init? */
  

-- 2.26.2

On Sun, Oct 04, 2020 at 12:21:36PM -0700, Rob Clark wrote:

From: Rob Clark robdclark@chromium.org

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 jcrouse@codeaurora.org

Signed-off-by: Rob Clark robdclark@chromium.org

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_obj->lock);

@@ -1103,14 +1112,9 @@ static struct drm_gem_object *_msm_gem_new(struct drm_device *dev, mapping_set_gfp_mask(obj->filp->f_mapping, GFP_HIGHUSER); }

• 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);

  return obj;

@@ -1178,9 +1182,9 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,

mutex_unlock(&msm_obj->lock);

• mutex_lock(&dev->struct_mutex);

• mutex_lock(&priv->mm_lock); list_add_tail(&msm_obj->mm_list, &priv->inactive_list);

• mutex_unlock(&dev->struct_mutex);

• mutex_unlock(&priv->mm_lock);

  return obj;

diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c b/drivers/gpu/drm/msm/msm_gem_shrinker.c index 482576d7a39a..c41b84a3a484 100644 --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c @@ -51,11 +51,15 @@ msm_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc) if (!msm_gem_shrinker_lock(dev, &unlock)) return 0;

• mutex_lock(&priv->mm_lock);

• list_for_each_entry(msm_obj, &priv->inactive_list, mm_list) { if (is_purgeable(msm_obj)) count += msm_obj->base.size >> PAGE_SHIFT; }

• mutex_unlock(&priv->mm_lock);

• if (unlock) mutex_unlock(&dev->struct_mutex);

@@ -75,6 +79,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) if (!msm_gem_shrinker_lock(dev, &unlock)) return SHRINK_STOP;

• mutex_lock(&priv->mm_lock);
• list_for_each_entry(msm_obj, &priv->inactive_list, mm_list) { if (freed >= sc->nr_to_scan) break;

@@ -84,6 +90,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) } }

• mutex_unlock(&priv->mm_lock);
• if (unlock) mutex_unlock(&dev->struct_mutex);

@@ -106,6 +114,8 @@ msm_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr) if (!msm_gem_shrinker_lock(dev, &unlock)) return NOTIFY_DONE;

• mutex_lock(&priv->mm_lock);
• list_for_each_entry(msm_obj, &priv->inactive_list, mm_list) { if (is_vunmapable(msm_obj)) { msm_gem_vunmap(&msm_obj->base, OBJ_LOCK_SHRINKER);

@@ -118,6 +128,8 @@ msm_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr) } }

• mutex_unlock(&priv->mm_lock);
• if (unlock) mutex_unlock(&dev->struct_mutex);

diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h index 6c9e1fdc1a76..1806e87600c0 100644 --- a/drivers/gpu/drm/msm/msm_gpu.h +++ b/drivers/gpu/drm/msm/msm_gpu.h @@ -94,7 +94,10 @@ struct msm_gpu { struct msm_ringbuffer *rb[MSM_GPU_MAX_RINGS]; int nr_rings;

• /* list of GEM active objects: */

• /*

• * List of GEM active objects on this gpu.  Protected by
  

• * msm_drm_private::mm_lock
  

• */
  

  struct list_head active_list;

  /* does gpu need hw_init? */

-- 2.26.2

---

Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno

On Sun, Oct 04, 2020 at 12:21:37PM -0700, Rob Clark wrote:

From: Rob Clark robdclark@chromium.org

Before adding another lock, give ring->lock a more descriptive name.

Reviewed-by: Jordan Crouse jcrouse@codeaurora.org

Signed-off-by: Rob Clark robdclark@chromium.org

drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 ++-- drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 12 ++++++------ drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 ++-- drivers/gpu/drm/msm/msm_ringbuffer.c | 2 +- drivers/gpu/drm/msm/msm_ringbuffer.h | 7 ++++++- 5 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c index c941c8138f25..543437a2186e 100644 --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c @@ -36,7 +36,7 @@ void a5xx_flush(struct msm_gpu *gpu, struct msm_ringbuffer *ring, OUT_RING(ring, upper_32_bits(shadowptr(a5xx_gpu, ring))); }

• spin_lock_irqsave(&ring->lock, flags);

• spin_lock_irqsave(&ring->preempt_lock, flags);

  /* Copy the shadow to the actual register */ ring->cur = ring->next;

@@ -44,7 +44,7 @@ void a5xx_flush(struct msm_gpu *gpu, struct msm_ringbuffer *ring, /* Make sure to wrap wptr if we need to */ wptr = get_wptr(ring);

• spin_unlock_irqrestore(&ring->lock, flags);

• spin_unlock_irqrestore(&ring->preempt_lock, flags);

  /* Make sure everything is posted before making a decision */ mb();

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_preempt.c b/drivers/gpu/drm/msm/adreno/a5xx_preempt.c index 7e04509c4e1f..183de1139eeb 100644 --- a/drivers/gpu/drm/msm/adreno/a5xx_preempt.c +++ b/drivers/gpu/drm/msm/adreno/a5xx_preempt.c @@ -45,9 +45,9 @@ static inline void update_wptr(struct msm_gpu *gpu, struct msm_ringbuffer *ring) if (!ring) return;

• spin_lock_irqsave(&ring->lock, flags);

• spin_lock_irqsave(&ring->preempt_lock, flags); wptr = get_wptr(ring);

• spin_unlock_irqrestore(&ring->lock, flags);

• spin_unlock_irqrestore(&ring->preempt_lock, flags);

  gpu_write(gpu, REG_A5XX_CP_RB_WPTR, wptr);

} @@ -62,9 +62,9 @@ static struct msm_ringbuffer *get_next_ring(struct msm_gpu *gpu) bool empty; struct msm_ringbuffer *ring = gpu->rb[i];

• spin_lock_irqsave(&ring->lock, flags);
  

• spin_lock_irqsave(&ring->preempt_lock, flags);
  

  empty = (get_wptr(ring) == ring->memptrs->rptr);

• spin_unlock_irqrestore(&ring->lock, flags);
  

• spin_unlock_irqrestore(&ring->preempt_lock, flags);
  
  

  if (!empty) return ring;

@@ -132,9 +132,9 @@ void a5xx_preempt_trigger(struct msm_gpu *gpu) }

/* Make sure the wptr doesn't update while we're in motion */

• spin_lock_irqsave(&ring->lock, flags);

• spin_lock_irqsave(&ring->preempt_lock, flags); a5xx_gpu->preempt[ring->id]->wptr = get_wptr(ring);

• spin_unlock_irqrestore(&ring->lock, flags);

• spin_unlock_irqrestore(&ring->preempt_lock, flags);

  /* Set the address of the incoming preemption record */ gpu_write64(gpu, REG_A5XX_CP_CONTEXT_SWITCH_RESTORE_ADDR_LO,

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c index 8915882e4444..fc85f008d69d 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c @@ -65,7 +65,7 @@ static void a6xx_flush(struct msm_gpu *gpu, struct msm_ringbuffer *ring) OUT_RING(ring, upper_32_bits(shadowptr(a6xx_gpu, ring))); }

• spin_lock_irqsave(&ring->lock, flags);

• spin_lock_irqsave(&ring->preempt_lock, flags);

  /* Copy the shadow to the actual register */ ring->cur = ring->next;

@@ -73,7 +73,7 @@ static void a6xx_flush(struct msm_gpu *gpu, struct msm_ringbuffer *ring) /* Make sure to wrap wptr if we need to */ wptr = get_wptr(ring);

• spin_unlock_irqrestore(&ring->lock, flags);

• spin_unlock_irqrestore(&ring->preempt_lock, flags);

  /* Make sure everything is posted before making a decision */ mb();

diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c index 935bf9b1d941..1b6958e908dc 100644 --- a/drivers/gpu/drm/msm/msm_ringbuffer.c +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c @@ -46,7 +46,7 @@ struct msm_ringbuffer *msm_ringbuffer_new(struct msm_gpu *gpu, int id, ring->memptrs_iova = memptrs_iova;

INIT_LIST_HEAD(&ring->submits);

• spin_lock_init(&ring->lock);

• spin_lock_init(&ring->preempt_lock);

  snprintf(name, sizeof(name), "gpu-ring-%d", ring->id);

diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.h b/drivers/gpu/drm/msm/msm_ringbuffer.h index 0987d6bf848c..4956d1bc5d0e 100644 --- a/drivers/gpu/drm/msm/msm_ringbuffer.h +++ b/drivers/gpu/drm/msm/msm_ringbuffer.h @@ -46,7 +46,12 @@ struct msm_ringbuffer { struct msm_rbmemptrs *memptrs; uint64_t memptrs_iova; struct msm_fence_context *fctx;

• spinlock_t lock;

• /*

• * preempt_lock protects preemption and serializes wptr updates against
  

• * preemption.  Can be aquired from irq context.
  

• */
  

• spinlock_t preempt_lock;

};

struct msm_ringbuffer *msm_ringbuffer_new(struct msm_gpu *gpu, int id,

2.26.2

On Sun, Oct 04, 2020 at 12:21:38PM -0700, Rob Clark wrote:

From: Rob Clark robdclark@chromium.org

One less place to rely on dev->struct_mutex.

Reviewed-by: Jordan Crouse jcrouse@codeaurora.org

Signed-off-by: Rob Clark robdclark@chromium.org

drivers/gpu/drm/msm/msm_gem_submit.c | 2 ++ drivers/gpu/drm/msm/msm_gpu.c | 37 ++++++++++++++++++++++------ drivers/gpu/drm/msm/msm_ringbuffer.c | 1 + drivers/gpu/drm/msm/msm_ringbuffer.h | 6 +++++ 4 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index aa5c60a7132d..e1d1f005b3d4 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -63,7 +63,9 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, void msm_gem_submit_free(struct msm_gem_submit *submit) { dma_fence_put(submit->fence);

• spin_lock(&submit->ring->submit_lock); list_del(&submit->node);
• spin_unlock(&submit->ring->submit_lock); put_pid(submit->pid); msm_submitqueue_put(submit->queue);

diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index ca8c95b32c8b..8d1e254f964a 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -270,6 +270,7 @@ static void update_fences(struct msm_gpu *gpu, struct msm_ringbuffer *ring, { struct msm_gem_submit *submit;

• spin_lock(&ring->submit_lock); list_for_each_entry(submit, &ring->submits, node) { if (submit->seqno > fence) break;

@@ -277,6 +278,7 @@ static void update_fences(struct msm_gpu *gpu, struct msm_ringbuffer *ring, msm_update_fence(submit->ring->fctx, submit->fence->seqno); }

• spin_unlock(&ring->submit_lock);

}

#ifdef CONFIG_DEV_COREDUMP @@ -430,11 +432,14 @@ find_submit(struct msm_ringbuffer *ring, uint32_t fence) { struct msm_gem_submit *submit;

• WARN_ON(!mutex_is_locked(&ring->gpu->dev->struct_mutex));
• list_for_each_entry(submit, &ring->submits, node)

• if (submit->seqno == fence)
  

• spin_lock(&ring->submit_lock);

• list_for_each_entry(submit, &ring->submits, node) {

• if (submit->seqno == fence) {
  

• 	spin_unlock(&ring->submit_lock);
  return submit;
  

• }
  

• }

• spin_unlock(&ring->submit_lock);

  return NULL;

} @@ -523,8 +528,10 @@ static void recover_worker(struct work_struct *work) for (i = 0; i < gpu->nr_rings; i++) { struct msm_ringbuffer *ring = gpu->rb[i];

• 	spin_lock(&ring->submit_lock);
  list_for_each_entry(submit, &ring->submits, node)
  	gpu->funcs->submit(gpu, submit);
  

• 	spin_unlock(&ring->submit_lock);
  

  } }

@@ -711,7 +718,6 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring, static void retire_submits(struct msm_gpu *gpu) { struct drm_device *dev = gpu->dev;

• struct msm_gem_submit *submit, *tmp; int i;

  WARN_ON(!mutex_is_locked(&dev->struct_mutex));

@@ -720,9 +726,24 @@ static void retire_submits(struct msm_gpu *gpu) for (i = 0; i < gpu->nr_rings; i++) { struct msm_ringbuffer *ring = gpu->rb[i];

• list_for_each_entry_safe(submit, tmp, &ring->submits, node) {
  

• 	if (dma_fence_is_signaled(submit->fence))
  

• while (true) {
  

• 	struct msm_gem_submit *submit = NULL;
  

• 	spin_lock(&ring->submit_lock);
  

• 	submit = list_first_entry_or_null(&ring->submits,
  

• 			struct msm_gem_submit, node);
  

• 	spin_unlock(&ring->submit_lock);
  

• 	/*
  

• 	 * If no submit, we are done.  If submit->fence hasn't
  

• 	 * been signalled, then later submits are not signalled
  

• 	 * either, so we are also done.
  

• 	 */
  

• 	if (submit && dma_fence_is_signaled(submit->fence)) {
  	retire_submit(gpu, ring, submit);
  

• 	} else {
  

• 		break;
  

• 	}
  

  } }

} @@ -765,7 +786,9 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)

submit->seqno = ++ring->seqno;

• spin_lock(&ring->submit_lock); list_add_tail(&submit->node, &ring->submits);

• spin_unlock(&ring->submit_lock);

  msm_rd_dump_submit(priv->rd, submit, NULL);

diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c index 1b6958e908dc..4d2a2a4abef8 100644 --- a/drivers/gpu/drm/msm/msm_ringbuffer.c +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c @@ -46,6 +46,7 @@ struct msm_ringbuffer *msm_ringbuffer_new(struct msm_gpu *gpu, int id, ring->memptrs_iova = memptrs_iova;

INIT_LIST_HEAD(&ring->submits);

• spin_lock_init(&ring->submit_lock); spin_lock_init(&ring->preempt_lock);

  snprintf(name, sizeof(name), "gpu-ring-%d", ring->id);

diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.h b/drivers/gpu/drm/msm/msm_ringbuffer.h index 4956d1bc5d0e..fe55d4a1aa16 100644 --- a/drivers/gpu/drm/msm/msm_ringbuffer.h +++ b/drivers/gpu/drm/msm/msm_ringbuffer.h @@ -39,7 +39,13 @@ struct msm_ringbuffer { int id; struct drm_gem_object *bo; uint32_t *start, *end, *cur, *next;

• /*

• * List of in-flight submits on this ring.  Protected by submit_lock.
  

• */
  

  struct list_head submits;
• spinlock_t submit_lock;
• uint64_t iova; uint32_t seqno; uint32_t hangcheck_fence;

-- 2.26.2

---

Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno

On Sun, Oct 04, 2020 at 12:21:39PM -0700, Rob Clark wrote:

From: Rob Clark robdclark@chromium.org

Before we remove dev->struct_mutex from the retire path, we have to deal with the situation of a submit retiring before the submit ioctl returns.

To deal with this, ring->submits will hold a reference to the submit, which is dropped when the submit is retired. And the submit ioctl path holds it's own ref, which it drops when it is done with the submit.

Also, add to submit list *after* getting/pinning bo's, to prevent badness in case the completed fence is corrupted, and retire_worker mistakenly believes the submit is done too early.

Signed-off-by: Rob Clark robdclark@chromium.org

Why not embed the dma_fence instead of pointer and use that refcount? i915 does that, and imo kinda makes sense instead of more refcounted things. But might not make sense for msm. -Daniel

---

drivers/gpu/drm/msm/msm_drv.h | 1 - drivers/gpu/drm/msm/msm_gem.h | 13 +++++++++++++ drivers/gpu/drm/msm/msm_gem_submit.c | 12 ++++++------ drivers/gpu/drm/msm/msm_gpu.c | 21 ++++++++++++++++----- 4 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index 50978e5db376..535f9e718e2d 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -277,7 +277,6 @@ void msm_unregister_mmu(struct drm_device *dev, struct msm_mmu *mmu);

bool msm_use_mmu(struct drm_device *dev);

-void msm_gem_submit_free(struct msm_gem_submit *submit); int msm_ioctl_gem_submit(struct drm_device *dev, void *data, struct drm_file *file);

diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index a1bf741b9b89..e05b1530aca6 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -136,6 +136,7 @@ void msm_gem_free_work(struct work_struct *work);

• lasts for the duration of the submit-ioctl.

*/ struct msm_gem_submit {

• struct kref ref; struct drm_device *dev; struct msm_gpu *gpu; struct msm_gem_address_space *aspace;

@@ -169,6 +170,18 @@ struct msm_gem_submit { } bos[]; };

+void __msm_gem_submit_destroy(struct kref *kref);

+static inline void msm_gem_submit_get(struct msm_gem_submit *submit) +{

• kref_get(&submit->ref);

+}

+static inline void msm_gem_submit_put(struct msm_gem_submit *submit) +{

• kref_put(&submit->ref, __msm_gem_submit_destroy);

+}

/* helper to determine of a buffer in submit should be dumped, used for both

• devcoredump and debugfs cmdstream dumping:

*/ diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index e1d1f005b3d4..7d653bdc92dc 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -42,6 +42,7 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, if (!submit) return NULL;

• kref_init(&submit->ref); submit->dev = dev; submit->aspace = queue->ctx->aspace; submit->gpu = gpu;

@@ -60,12 +61,12 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, return submit; }

-void msm_gem_submit_free(struct msm_gem_submit *submit) +void __msm_gem_submit_destroy(struct kref *kref) {

• struct msm_gem_submit *submit =

• 	container_of(kref, struct msm_gem_submit, ref);
  

• dma_fence_put(submit->fence);

• spin_lock(&submit->ring->submit_lock);
• list_del(&submit->node);
• spin_unlock(&submit->ring->submit_lock); put_pid(submit->pid); msm_submitqueue_put(submit->queue);

@@ -805,8 +806,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, submit_cleanup(submit); if (has_ww_ticket) ww_acquire_fini(&submit->ticket);

• if (ret)

• msm_gem_submit_free(submit);
  

• msm_gem_submit_put(submit);

out_unlock: if (ret && (out_fence_fd >= 0)) put_unused_fd(out_fence_fd); diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index 8d1e254f964a..fd3fc6f36ab1 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -712,7 +712,12 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring,

pm_runtime_mark_last_busy(&gpu->pdev->dev); pm_runtime_put_autosuspend(&gpu->pdev->dev);

• msm_gem_submit_free(submit);

• spin_lock(&ring->submit_lock);
• list_del(&submit->node);
• spin_unlock(&ring->submit_lock);
• msm_gem_submit_put(submit);

}

static void retire_submits(struct msm_gpu *gpu) @@ -786,10 +791,6 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)

submit->seqno = ++ring->seqno;

• spin_lock(&ring->submit_lock);

• list_add_tail(&submit->node, &ring->submits);

• spin_unlock(&ring->submit_lock);

• msm_rd_dump_submit(priv->rd, submit, NULL);

  update_sw_cntrs(gpu);

@@ -816,6 +817,16 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit) msm_gem_active_get(drm_obj, gpu); }

• /*

• * ring->submits holds a ref to the submit, to deal with the case
  

• * that a submit completes before msm_ioctl_gem_submit() returns.
  

• */
  

• msm_gem_submit_get(submit);
• spin_lock(&ring->submit_lock);
• list_add_tail(&submit->node, &ring->submits);
• spin_unlock(&ring->submit_lock);
• gpu->funcs->submit(gpu, submit); priv->lastctx = submit->queue->ctx;

-- 2.26.2

On Sun, Oct 04, 2020 at 12:21:39PM -0700, Rob Clark wrote:

From: Rob Clark robdclark@chromium.org

Before we remove dev->struct_mutex from the retire path, we have to deal with the situation of a submit retiring before the submit ioctl returns.

To deal with this, ring->submits will hold a reference to the submit, which is dropped when the submit is retired. And the submit ioctl path holds it's own ref, which it drops when it is done with the submit.

Also, add to submit list *after* getting/pinning bo's, to prevent badness in case the completed fence is corrupted, and retire_worker mistakenly believes the submit is done too early.

Reviewed-by: Jordan Crouse jcrouse@codeaurora.org

Signed-off-by: Rob Clark robdclark@chromium.org

drivers/gpu/drm/msm/msm_drv.h | 1 - drivers/gpu/drm/msm/msm_gem.h | 13 +++++++++++++ drivers/gpu/drm/msm/msm_gem_submit.c | 12 ++++++------ drivers/gpu/drm/msm/msm_gpu.c | 21 ++++++++++++++++----- 4 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index 50978e5db376..535f9e718e2d 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -277,7 +277,6 @@ void msm_unregister_mmu(struct drm_device *dev, struct msm_mmu *mmu);

bool msm_use_mmu(struct drm_device *dev);

-void msm_gem_submit_free(struct msm_gem_submit *submit); int msm_ioctl_gem_submit(struct drm_device *dev, void *data, struct drm_file *file);

diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index a1bf741b9b89..e05b1530aca6 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -136,6 +136,7 @@ void msm_gem_free_work(struct work_struct *work);

• lasts for the duration of the submit-ioctl.

*/ struct msm_gem_submit {

• struct kref ref; struct drm_device *dev; struct msm_gpu *gpu; struct msm_gem_address_space *aspace;

@@ -169,6 +170,18 @@ struct msm_gem_submit { } bos[]; };

+void __msm_gem_submit_destroy(struct kref *kref);

+static inline void msm_gem_submit_get(struct msm_gem_submit *submit) +{

• kref_get(&submit->ref);

+}

+static inline void msm_gem_submit_put(struct msm_gem_submit *submit) +{

• kref_put(&submit->ref, __msm_gem_submit_destroy);

+}

/* helper to determine of a buffer in submit should be dumped, used for both

• devcoredump and debugfs cmdstream dumping:

*/ diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index e1d1f005b3d4..7d653bdc92dc 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -42,6 +42,7 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, if (!submit) return NULL;

• kref_init(&submit->ref); submit->dev = dev; submit->aspace = queue->ctx->aspace; submit->gpu = gpu;

@@ -60,12 +61,12 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, return submit; }

-void msm_gem_submit_free(struct msm_gem_submit *submit) +void __msm_gem_submit_destroy(struct kref *kref) {

• struct msm_gem_submit *submit =

• 	container_of(kref, struct msm_gem_submit, ref);
  

• dma_fence_put(submit->fence);

• spin_lock(&submit->ring->submit_lock);
• list_del(&submit->node);
• spin_unlock(&submit->ring->submit_lock); put_pid(submit->pid); msm_submitqueue_put(submit->queue);

@@ -805,8 +806,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, submit_cleanup(submit); if (has_ww_ticket) ww_acquire_fini(&submit->ticket);

• if (ret)

• msm_gem_submit_free(submit);
  

• msm_gem_submit_put(submit);

out_unlock: if (ret && (out_fence_fd >= 0)) put_unused_fd(out_fence_fd); diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index 8d1e254f964a..fd3fc6f36ab1 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -712,7 +712,12 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring,

pm_runtime_mark_last_busy(&gpu->pdev->dev); pm_runtime_put_autosuspend(&gpu->pdev->dev);

• msm_gem_submit_free(submit);

• spin_lock(&ring->submit_lock);
• list_del(&submit->node);
• spin_unlock(&ring->submit_lock);
• msm_gem_submit_put(submit);

}

static void retire_submits(struct msm_gpu *gpu) @@ -786,10 +791,6 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)

submit->seqno = ++ring->seqno;

• spin_lock(&ring->submit_lock);

• list_add_tail(&submit->node, &ring->submits);

• spin_unlock(&ring->submit_lock);

• msm_rd_dump_submit(priv->rd, submit, NULL);

  update_sw_cntrs(gpu);

@@ -816,6 +817,16 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit) msm_gem_active_get(drm_obj, gpu); }

• /*

• * ring->submits holds a ref to the submit, to deal with the case
  

• * that a submit completes before msm_ioctl_gem_submit() returns.
  

• */
  

• msm_gem_submit_get(submit);
• spin_lock(&ring->submit_lock);
• list_add_tail(&submit->node, &ring->submits);
• spin_unlock(&ring->submit_lock);
• gpu->funcs->submit(gpu, submit); priv->lastctx = submit->queue->ctx;

-- 2.26.2

---

Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno

On Sun, Oct 04, 2020 at 12:21:40PM -0700, Rob Clark wrote:

From: Rob Clark robdclark@chromium.org

It cannot be atomically updated with obj->active_count, and the only purpose is a useless WARN_ON() (which becomes a buggy WARN_ON() once retire_submits() is not serialized with incoming submits via struct_mutex)

Somebody will prove me wrong but the longer we go without 2D the less likely it is that we'll ever see it.

Reviewed-by: Jordan Crouse jcrouse@codeaurora.org

Signed-off-by: Rob Clark robdclark@chromium.org

drivers/gpu/drm/msm/msm_gem.c | 2 -- drivers/gpu/drm/msm/msm_gem.h | 1 - drivers/gpu/drm/msm/msm_gpu.c | 5 ----- 3 files changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index b04ed8b52f9d..c52a3969e60b 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -753,7 +753,6 @@ void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu)

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);

@@ -769,7 +768,6 @@ void msm_gem_active_put(struct drm_gem_object *obj)

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);

diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index e05b1530aca6..61147bd96b06 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -64,7 +64,6 @@ struct msm_gem_object { * */ struct list_head mm_list;

• struct msm_gpu *gpu; /* non-null if active */

  /* Transiently in the process of submit ioctl, objects associated

  • with the submit are on submit->bo_list.. this only lasts for

diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index fd3fc6f36ab1..c9ff19a75169 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -800,11 +800,6 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit) struct drm_gem_object *drm_obj = &msm_obj->base; uint64_t iova;

• /* can't happen yet.. but when we add 2d support we'll have
  

•  * to deal w/ cross-ring synchronization:
  

•  */
  

• WARN_ON(is_active(msm_obj) && (msm_obj->gpu != gpu));
  

• /* submit takes a reference to the bo and iova until retired: */ drm_gem_object_get(&msm_obj->base); msm_gem_get_and_pin_iova(&msm_obj->base, submit->aspace, &iova);

-- 2.26.2

---

Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno

On Sun, Oct 04, 2020 at 12:21:41PM -0700, Rob Clark wrote:

From: Rob Clark robdclark@chromium.org

Now that we are not relying on dev->struct_mutex to protect the ring->submits lists, drop the struct_mutex lock.

Reviewed-by: Jordan Crouse jcrouse@codeaurora.org

Signed-off-by: Rob Clark robdclark@chromium.org

drivers/gpu/drm/msm/msm_gpu.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index c9ff19a75169..5e351d1c00e9 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -707,7 +707,7 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring,

msm_gem_active_put(&msm_obj->base);
msm_gem_unpin_iova(&msm_obj->base, submit->aspace);

• drm_gem_object_put_locked(&msm_obj->base);
  

• drm_gem_object_put(&msm_obj->base);
  

  }

  pm_runtime_mark_last_busy(&gpu->pdev->dev);

@@ -722,11 +722,8 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring,

static void retire_submits(struct msm_gpu *gpu) {

• struct drm_device *dev = gpu->dev; int i;

• WARN_ON(!mutex_is_locked(&dev->struct_mutex));

• /* Retire the commits starting with highest priority */ for (i = 0; i < gpu->nr_rings; i++) { struct msm_ringbuffer *ring = gpu->rb[i];

@@ -756,15 +753,12 @@ static void retire_submits(struct msm_gpu *gpu) static void retire_worker(struct work_struct *work) { struct msm_gpu *gpu = container_of(work, struct msm_gpu, retire_work);

• struct drm_device *dev = gpu->dev; int i;

  for (i = 0; i < gpu->nr_rings; i++) update_fences(gpu, gpu->rb[i], gpu->rb[i]->memptrs->fence);

• mutex_lock(&dev->struct_mutex); retire_submits(gpu);

• mutex_unlock(&dev->struct_mutex);

}

/* call from irq handler to schedule work to retire bo's */

2.26.2

---

Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno

On Mon, Oct 05, 2020 at 05:24:19PM +0800, Hillf Danton wrote:

On Sun, 4 Oct 2020 12:21:45

...

From: Rob Clark robdclark@chromium.org

Now that the inactive_list is protected by mm_lock, and everything else on per-obj basis is protected by obj->lock, we no longer depend on struct_mutex.

Signed-off-by: Rob Clark robdclark@chromium.org

drivers/gpu/drm/msm/msm_gem.c | 1 - drivers/gpu/drm/msm/msm_gem_shrinker.c | 54 -------------------------- 2 files changed, 55 deletions(-)

[...]

...

@@ -71,13 +33,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) { struct msm_drm_private *priv = container_of(shrinker, struct msm_drm_private, shrinker);

• struct drm_device *dev = priv->dev; struct msm_gem_object *msm_obj; unsigned long freed = 0;

• bool unlock;

• if (!msm_gem_shrinker_lock(dev, &unlock))

• return SHRINK_STOP;
  
  

  mutex_lock(&priv->mm_lock);

Better if the change in behavior is documented that SHRINK_STOP will no longer be needed.

btw I read through this and noticed you have your own obj lock, plus mutex_lock_nested. I strongly recommend to just cut over to dma_resv_lock for all object lock needs (soc drivers have been terrible with this unfortuntaly), and in the shrinker just use dma_resv_trylock instead of trying to play clever games outsmarting lockdep.

I recently wrote an entire blog length rant on why I think mutex_lock_nested is too dangerous to be useful:

https://blog.ffwll.ch/2020/08/lockdep-false-positives.html

Not anything about this here, just general comment. The problem extends to shmem helpers and all that also having their own locks for everything. -Daniel

On Mon, 5 Oct 2020 18:17:01 Kristian H. Kristensen wrote:

On Mon, Oct 5, 2020 at 4:02 PM Daniel Vetter daniel@ffwll.ch wrote:

...

On Mon, Oct 05, 2020 at 05:24:19PM +0800, Hillf Danton wrote:

...

On Sun, 4 Oct 2020 12:21:45

...

From: Rob Clark robdclark@chromium.org

Now that the inactive_list is protected by mm_lock, and everything else on per-obj basis is protected by obj->lock, we no longer depend on struct_mutex.

Signed-off-by: Rob Clark robdclark@chromium.org

drivers/gpu/drm/msm/msm_gem.c | 1 - drivers/gpu/drm/msm/msm_gem_shrinker.c | 54 -------------------------- 2 files changed, 55 deletions(-)

[...]

...

@@ -71,13 +33,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) { struct msm_drm_private *priv = container_of(shrinker, struct msm_drm_private, shrinker);

• struct drm_device *dev = priv->dev; struct msm_gem_object *msm_obj; unsigned long freed = 0;

• bool unlock;

• if (!msm_gem_shrinker_lock(dev, &unlock))

•       return SHRINK_STOP;
  
  

  mutex_lock(&priv->mm_lock);

Better if the change in behavior is documented that SHRINK_STOP will no longer be needed.

btw I read through this and noticed you have your own obj lock, plus mutex_lock_nested. I strongly recommend to just cut over to dma_resv_lock for all object lock needs (soc drivers have been terrible with this unfortuntaly), and in the shrinker just use dma_resv_trylock instead of trying to play clever games outsmarting lockdep.

The trylock makes page reclaimers turn to their next target e.g. inode cache instead of waiting for the mutex to be released. It makes sense for instance in scenarios of mild memory pressure.

...

I recently wrote an entire blog length rant on why I think mutex_lock_nested is too dangerous to be useful:

https://blog.ffwll.ch/2020/08/lockdep-false-positives.html

Not anything about this here, just general comment. The problem extends to shmem helpers and all that also having their own locks for everything.

This is definitely a tangible improvement though - very happy to see msm_gem_shrinker_lock() go.

Reviewed-by: Kristian H. Kristensen hoegsberg@google.com

...

-Daniel

Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Mon, 5 Oct 2020 20:40:12 Rob Clark robdclark@gmail.com wrote:

On Mon, Oct 5, 2020 at 5:44 PM Hillf Danton hdanton@sina.com wrote:

...

On Mon, 5 Oct 2020 18:17:01 Kristian H. Kristensen wrote:

...

On Mon, Oct 5, 2020 at 4:02 PM Daniel Vetter daniel@ffwll.ch wrote:

...

On Mon, Oct 05, 2020 at 05:24:19PM +0800, Hillf Danton wrote:

...

On Sun, 4 Oct 2020 12:21:45

...

From: Rob Clark robdclark@chromium.org

Now that the inactive_list is protected by mm_lock, and everything else on per-obj basis is protected by obj->lock, we no longer depend on struct_mutex.

Signed-off-by: Rob Clark robdclark@chromium.org

drivers/gpu/drm/msm/msm_gem.c | 1 - drivers/gpu/drm/msm/msm_gem_shrinker.c | 54 -------------------------- 2 files changed, 55 deletions(-)

[...]

...

@@ -71,13 +33,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) { struct msm_drm_private *priv = container_of(shrinker, struct msm_drm_private, shrinker);

• struct drm_device *dev = priv->dev; struct msm_gem_object *msm_obj; unsigned long freed = 0;

• bool unlock;

• if (!msm_gem_shrinker_lock(dev, &unlock))

•       return SHRINK_STOP;
  
  

  mutex_lock(&priv->mm_lock);

Better if the change in behavior is documented that SHRINK_STOP will no longer be needed.

btw I read through this and noticed you have your own obj lock, plus mutex_lock_nested. I strongly recommend to just cut over to dma_resv_lock for all object lock needs (soc drivers have been terrible with this unfortuntaly), and in the shrinker just use dma_resv_trylock instead of trying to play clever games outsmarting lockdep.

The trylock makes page reclaimers turn to their next target e.g. inode cache instead of waiting for the mutex to be released. It makes sense for instance in scenarios of mild memory pressure.

is there some behind-the-scenes signalling for this, or is this just down to what the shrinker callbacks return?

Lets see what Dave may have in his mind about your questions.

Generally when we get into shrinking, there are a big set of purgable bo's to consider, so the shrinker callback return wouldn't be considering just one potentially lock contended bo (buffer object). Ie failing one trylock, we just move on to the next.

fwiw, what I've seen on the userspace bo cache vs shrinker (anything that is shrinker potential is in userspace bo cache and MADV(WONTNEED)) is that in steady state I see a very strong recycling of bo's (which avoids allocating and mmap'ing or mapping to gpu a new buffer object), so it is definitely a win in mmap/realloc bandwidth.. in steady state there is a lot of free and realloc of same-sized buffers from frame to frame.

But in transient situations like moving to new game level when there is a heavy memory pressure and lots of freeing old buffers/textures/etc and then allocating new ones, I see shrinker kicking in hard (in android situations, not so much so with traditional linux userspace)

BR, -R

...

...

...

I recently wrote an entire blog length rant on why I think mutex_lock_nested is too dangerous to be useful:

https://blog.ffwll.ch/2020/08/lockdep-false-positives.html

Not anything about this here, just general comment. The problem extends to shmem helpers and all that also having their own locks for everything.

This is definitely a tangible improvement though - very happy to see msm_gem_shrinker_lock() go.

Reviewed-by: Kristian H. Kristensen hoegsberg@google.com

...

-Daniel

Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Mon, Oct 5, 2020 at 7:02 AM Daniel Vetter daniel@ffwll.ch wrote:

On Mon, Oct 05, 2020 at 05:24:19PM +0800, Hillf Danton wrote:

...

On Sun, 4 Oct 2020 12:21:45

...

From: Rob Clark robdclark@chromium.org

Now that the inactive_list is protected by mm_lock, and everything else on per-obj basis is protected by obj->lock, we no longer depend on struct_mutex.

Signed-off-by: Rob Clark robdclark@chromium.org

drivers/gpu/drm/msm/msm_gem.c | 1 - drivers/gpu/drm/msm/msm_gem_shrinker.c | 54 -------------------------- 2 files changed, 55 deletions(-)

[...]

...

@@ -71,13 +33,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) { struct msm_drm_private *priv = container_of(shrinker, struct msm_drm_private, shrinker);

• struct drm_device *dev = priv->dev; struct msm_gem_object *msm_obj; unsigned long freed = 0;

• bool unlock;

• if (!msm_gem_shrinker_lock(dev, &unlock))

•       return SHRINK_STOP;
  
  

  mutex_lock(&priv->mm_lock);

Better if the change in behavior is documented that SHRINK_STOP will no longer be needed.

btw I read through this and noticed you have your own obj lock, plus mutex_lock_nested. I strongly recommend to just cut over to dma_resv_lock for all object lock needs (soc drivers have been terrible with this unfortuntaly), and in the shrinker just use dma_resv_trylock instead of trying to play clever games outsmarting lockdep.

I recently wrote an entire blog length rant on why I think mutex_lock_nested is too dangerous to be useful:

https://blog.ffwll.ch/2020/08/lockdep-false-positives.html

Not anything about this here, just general comment. The problem extends to shmem helpers and all that also having their own locks for everything.

the shrinker lock class has existed for a while.. and is based on the idea that anything in the get-pages/vmap path cannot happen on a WONTNEED bo.. although perhaps there should be a few more 'if (WARN_ON(obj->madv != WILLNEED)) return -EBUSY'..

replacing obj->lock with dma_resv lock, might be a nice cleanup.. but I think it will be a bit churny..

BR, -R

-Daniel

Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch