[PATCH 1/8] drm/panfrost: Make panfrost_job_run() return an ERR_PTR() instead of NULL

[PATCH 1/8] drm/panfrost: Make panfrost_job_run() return an ERR_PTR() instead of NULL

[Boris Brezillon]

If we don't do that, dma_fence_set_error() complains (called from
drm_sched_main()).

Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_job.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 21f34d44aac2..cdd9448fbbdd 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -328,13 +328,13 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
 	struct dma_fence *fence = NULL;
 
 	if (unlikely(job->base.s_fence->finished.error))
-		return NULL;
+		return ERR_PTR(job->base.s_fence->finished.error);
 
 	pfdev->jobs[slot] = job;
 
 	fence = panfrost_fence_create(pfdev, slot);
 	if (IS_ERR(fence))
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	if (job->done_fence)
 		dma_fence_put(job->done_fence);
-- 
2.23.0

[PATCH 2/8] drm/panfrost: Fix a race in panfrost_ioctl_madvise()

[Boris Brezillon]

If 2 threads change the MADVISE property of the same BO in parallel we
might end up with an shmem->madv value that's inconsistent with the
presence of the BO in the shrinker list.

The easiest solution to fix that is to protect the
drm_gem_shmem_madvise() call with the shrinker lock.

Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index f21bc8a7ee3a..efc0a24d1f4c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -347,20 +347,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
 		return -ENOENT;
 	}
 
+	mutex_lock(&pfdev->shrinker_lock);
 	args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);
 
 	if (args->retained) {
 		struct panfrost_gem_object *bo = to_panfrost_bo(gem_obj);
 
-		mutex_lock(&pfdev->shrinker_lock);
-
 		if (args->madv == PANFROST_MADV_DONTNEED)
-			list_add_tail(&bo->base.madv_list, &pfdev->shrinker_list);
+			list_add_tail(&bo->base.madv_list,
+				      &pfdev->shrinker_list);
 		else if (args->madv == PANFROST_MADV_WILLNEED)
 			list_del_init(&bo->base.madv_list);
-
-		mutex_unlock(&pfdev->shrinker_lock);
 	}
+	mutex_unlock(&pfdev->shrinker_lock);
 
 	drm_gem_object_put_unlocked(gem_obj);
 	return 0;
-- 
2.23.0

[PATCH 3/8] drm/panfrost: Fix a BO leak in panfrost_ioctl_mmap_bo()

[Boris Brezillon]

We should release the reference we grabbed when an error occurs.

Fixes: 187d2929206e ("drm/panfrost: Add support for GPU heap allocations")
Cc: <stable@vger.kernel.org>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index efc0a24d1f4c..2630c5027c63 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -303,14 +303,17 @@ static int panfrost_ioctl_mmap_bo(struct drm_device *dev, void *data,
 	}
 
 	/* Don't allow mmapping of heap objects as pages are not pinned. */
-	if (to_panfrost_bo(gem_obj)->is_heap)
-		return -EINVAL;
+	if (to_panfrost_bo(gem_obj)->is_heap) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	ret = drm_gem_create_mmap_offset(gem_obj);
 	if (ret == 0)
 		args->offset = drm_vma_node_offset_addr(&gem_obj->vma_node);
-	drm_gem_object_put_unlocked(gem_obj);
 
+out:
+	drm_gem_object_put_unlocked(gem_obj);
 	return ret;
 }
 
-- 
2.23.0

[PATCH 4/8] drm/panfrost: Fix a race in panfrost_gem_free_object()

[Boris Brezillon]

panfrost_gem_shrinker_scan() might purge a BO (release the sgt and
kill the GPU mapping) that's being freed by panfrost_gem_free_object()
if we don't remove the BO from the shrinker list at the beginning of
panfrost_gem_free_object().

Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_gem.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index acb07fe06580..daf4c55a2863 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -19,6 +19,16 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
 	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
 	struct panfrost_device *pfdev = obj->dev->dev_private;
 
+	/*
+	 * Make sure the BO is no longer inserted in the shrinker list before
+	 * taking care of the destruction itself. If we don't do that we have a
+	 * race condition between this function and what's done in
+	 * panfrost_gem_shrinker_scan().
+	 */
+	mutex_lock(&pfdev->shrinker_lock);
+	list_del_init(&bo->base.madv_list);
+	mutex_unlock(&pfdev->shrinker_lock);
+
 	if (bo->sgts) {
 		int i;
 		int n_sgt = bo->base.base.size / SZ_2M;
@@ -33,11 +43,6 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
 		kfree(bo->sgts);
 	}
 
-	mutex_lock(&pfdev->shrinker_lock);
-	if (!list_empty(&bo->base.madv_list))
-		list_del(&bo->base.madv_list);
-	mutex_unlock(&pfdev->shrinker_lock);
-
 	drm_gem_shmem_free_object(obj);
 }
 
-- 
2.23.0

[PATCH 5/8] drm/panfrost: Open/close the perfcnt BO

[Boris Brezillon]

Commit a5efb4c9a562 ("drm/panfrost: Restructure the GEM object creation")
moved the drm_mm_insert_node_generic() call to the gem->open() hook,
but forgot to update perfcnt accordingly.

Patch the perfcnt logic to call panfrost_gem_open/close() where
appropriate.

Fixes: a5efb4c9a562 ("drm/panfrost: Restructure the GEM object creation")
Cc: <stable@vger.kernel.org>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c     |  2 +-
 drivers/gpu/drm/panfrost/panfrost_gem.c     |  4 ++--
 drivers/gpu/drm/panfrost/panfrost_gem.h     |  4 ++++
 drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 23 +++++++++++++--------
 drivers/gpu/drm/panfrost/panfrost_perfcnt.h |  2 +-
 5 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 2630c5027c63..1c67ac434e10 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -445,7 +445,7 @@ panfrost_postclose(struct drm_device *dev, struct drm_file *file)
 {
 	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
 
-	panfrost_perfcnt_close(panfrost_priv);
+	panfrost_perfcnt_close(file);
 	panfrost_job_close(panfrost_priv);
 
 	panfrost_mmu_pgtable_free(panfrost_priv);
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index daf4c55a2863..92a95210a899 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -46,7 +46,7 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
 	drm_gem_shmem_free_object(obj);
 }
 
-static int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)
+int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)
 {
 	int ret;
 	size_t size = obj->size;
@@ -85,7 +85,7 @@ static int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_p
 	return ret;
 }
 
-static void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv)
+void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv)
 {
 	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
 	struct panfrost_file_priv *priv = file_priv->driver_priv;
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
index 50920819cc16..4b17e7308764 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
@@ -45,6 +45,10 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv,
 				u32 flags,
 				uint32_t *handle);
 
+int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv);
+void panfrost_gem_close(struct drm_gem_object *obj,
+			struct drm_file *file_priv);
+
 void panfrost_gem_shrinker_init(struct drm_device *dev);
 void panfrost_gem_shrinker_cleanup(struct drm_device *dev);
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
index 2dba192bf198..2c04e858c50a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
+++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
@@ -67,9 +67,10 @@ static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev)
 }
 
 static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
-					  struct panfrost_file_priv *user,
+					  struct drm_file *file_priv,
 					  unsigned int counterset)
 {
+	struct panfrost_file_priv *user = file_priv->driver_priv;
 	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
 	struct drm_gem_shmem_object *bo;
 	u32 cfg;
@@ -91,14 +92,14 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 	perfcnt->bo = to_panfrost_bo(&bo->base);
 
 	/* Map the perfcnt buf in the address space attached to file_priv. */
-	ret = panfrost_mmu_map(perfcnt->bo);
+	ret = panfrost_gem_open(&perfcnt->bo->base.base, file_priv);
 	if (ret)
 		goto err_put_bo;
 
 	perfcnt->buf = drm_gem_shmem_vmap(&bo->base);
 	if (IS_ERR(perfcnt->buf)) {
 		ret = PTR_ERR(perfcnt->buf);
-		goto err_put_bo;
+		goto err_close_bo;
 	}
 
 	/*
@@ -157,14 +158,17 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 
 err_vunmap:
 	drm_gem_shmem_vunmap(&perfcnt->bo->base.base, perfcnt->buf);
+err_close_bo:
+	panfrost_gem_close(&perfcnt->bo->base.base, file_priv);
 err_put_bo:
 	drm_gem_object_put_unlocked(&bo->base);
 	return ret;
 }
 
 static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
-					   struct panfrost_file_priv *user)
+					   struct drm_file *file_priv)
 {
+	struct panfrost_file_priv *user = file_priv->driver_priv;
 	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
 
 	if (user != perfcnt->user)
@@ -180,6 +184,7 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
 	perfcnt->user = NULL;
 	drm_gem_shmem_vunmap(&perfcnt->bo->base.base, perfcnt->buf);
 	perfcnt->buf = NULL;
+	panfrost_gem_close(&perfcnt->bo->base.base, file_priv);
 	drm_gem_object_put_unlocked(&perfcnt->bo->base.base);
 	perfcnt->bo = NULL;
 	pm_runtime_mark_last_busy(pfdev->dev);
@@ -191,7 +196,6 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
 int panfrost_ioctl_perfcnt_enable(struct drm_device *dev, void *data,
 				  struct drm_file *file_priv)
 {
-	struct panfrost_file_priv *pfile = file_priv->driver_priv;
 	struct panfrost_device *pfdev = dev->dev_private;
 	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
 	struct drm_panfrost_perfcnt_enable *req = data;
@@ -207,10 +211,10 @@ int panfrost_ioctl_perfcnt_enable(struct drm_device *dev, void *data,
 
 	mutex_lock(&perfcnt->lock);
 	if (req->enable)
-		ret = panfrost_perfcnt_enable_locked(pfdev, pfile,
+		ret = panfrost_perfcnt_enable_locked(pfdev, file_priv,
 						     req->counterset);
 	else
-		ret = panfrost_perfcnt_disable_locked(pfdev, pfile);
+		ret = panfrost_perfcnt_disable_locked(pfdev, file_priv);
 	mutex_unlock(&perfcnt->lock);
 
 	return ret;
@@ -248,15 +252,16 @@ int panfrost_ioctl_perfcnt_dump(struct drm_device *dev, void *data,
 	return ret;
 }
 
-void panfrost_perfcnt_close(struct panfrost_file_priv *pfile)
+void panfrost_perfcnt_close(struct drm_file *file_priv)
 {
+	struct panfrost_file_priv *pfile = file_priv->driver_priv;
 	struct panfrost_device *pfdev = pfile->pfdev;
 	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
 
 	pm_runtime_get_sync(pfdev->dev);
 	mutex_lock(&perfcnt->lock);
 	if (perfcnt->user == pfile)
-		panfrost_perfcnt_disable_locked(pfdev, pfile);
+		panfrost_perfcnt_disable_locked(pfdev, file_priv);
 	mutex_unlock(&perfcnt->lock);
 	pm_runtime_mark_last_busy(pfdev->dev);
 	pm_runtime_put_autosuspend(pfdev->dev);
diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.h b/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
index 13b8fdaa1b43..8bbcf5f5fb33 100644
--- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
+++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
@@ -9,7 +9,7 @@ void panfrost_perfcnt_sample_done(struct panfrost_device *pfdev);
 void panfrost_perfcnt_clean_cache_done(struct panfrost_device *pfdev);
 int panfrost_perfcnt_init(struct panfrost_device *pfdev);
 void panfrost_perfcnt_fini(struct panfrost_device *pfdev);
-void panfrost_perfcnt_close(struct panfrost_file_priv *pfile);
+void panfrost_perfcnt_close(struct drm_file *file_priv);
 int panfrost_ioctl_perfcnt_enable(struct drm_device *dev, void *data,
 				  struct drm_file *file_priv);
 int panfrost_ioctl_perfcnt_dump(struct drm_device *dev, void *data,
-- 
2.23.0

[PATCH 6/8] drm/panfrost: Make sure imported/exported BOs are never purged

[Boris Brezillon]

We don't want imported/exported BOs to be purges, as those are shared
with other processes that might still use them. We should also refuse
to export a BO if it's been marked purgeable or has already been purged.

Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 19 ++++++++++++++++-
 drivers/gpu/drm/panfrost/panfrost_gem.c | 27 +++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 1c67ac434e10..751df975534f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -343,6 +343,7 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
 	struct drm_panfrost_madvise *args = data;
 	struct panfrost_device *pfdev = dev->dev_private;
 	struct drm_gem_object *gem_obj;
+	int ret;
 
 	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
 	if (!gem_obj) {
@@ -350,6 +351,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
 		return -ENOENT;
 	}
 
+	/*
+	 * We don't want to mark exported/imported BOs as purgeable: we're not
+	 * the only owner in that case.
+	 */
+	mutex_lock(&dev->object_name_lock);
+	if (gem_obj->dma_buf)
+		ret = -EINVAL;
+	else
+		ret = 0;
+
+	if (ret)
+		goto out_unlock_object_name;
+
 	mutex_lock(&pfdev->shrinker_lock);
 	args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);
 
@@ -364,8 +378,11 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
 	}
 	mutex_unlock(&pfdev->shrinker_lock);
 
+out_unlock_object_name:
+	mutex_unlock(&dev->object_name_lock);
+
 	drm_gem_object_put_unlocked(gem_obj);
-	return 0;
+	return ret;
 }
 
 int panfrost_unstable_ioctl_check(void)
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 92a95210a899..31d6417dd21c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -99,6 +99,32 @@ void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv)
 	spin_unlock(&priv->mm_lock);
 }
 
+static struct dma_buf *
+panfrost_gem_export(struct drm_gem_object *obj, int flags)
+{
+	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
+	int ret;
+
+	/*
+	 * We must make sure the BO has not been marked purgeable/purged before
+	 * adding the mapping.
+	 * Note that we don't need to protect this test with a lock because
+	 * &drm_gem_object_funcs.export() is called with
+	 * &drm_device.object_lock held, and panfrost_ioctl_madvise() takes
+	 * this lock before calling drm_gem_shmem_madvise() (the function that
+	 * modifies bo->base.madv).
+	 */
+	if (bo->base.madv == PANFROST_MADV_WILLNEED)
+		ret = -EINVAL;
+	else
+		ret = 0;
+
+	if (ret)
+		return ERR_PTR(ret);
+
+	return drm_gem_prime_export(obj, flags);
+}
+
 static int panfrost_gem_pin(struct drm_gem_object *obj)
 {
 	if (to_panfrost_bo(obj)->is_heap)
@@ -112,6 +138,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
 	.open = panfrost_gem_open,
 	.close = panfrost_gem_close,
 	.print_info = drm_gem_shmem_print_info,
+	.export = panfrost_gem_export,
 	.pin = panfrost_gem_pin,
 	.unpin = drm_gem_shmem_unpin,
 	.get_sg_table = drm_gem_shmem_get_sg_table,
-- 
2.23.0

[PATCH 7/8] drm/panfrost: Add the panfrost_gem_mapping concept

[Boris Brezillon]

With the introduction of per-FD address space, the same BO can be mapped
in different address space if the BO is globally visible (GEM_FLINK)
and opened in different context. The current implementation does not
take case into account, and attaches the mapping directly to the
panfrost_gem_object.

Let's create a panfrost_gem_mapping struct and allow multiple mappings
per BO.

The mappings are refcounted, which helps solve another problem where
mappings were teared down (GEM handle closed by userspace) while GPU
jobs accessing those BOs were still in-flight. Jobs now keep a
reference on the mappings they use.

Fixes: a5efb4c9a562 ("drm/panfrost: Restructure the GEM object creation")
Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
Cc: <stable@vger.kernel.org>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c       |  92 +++++++++++-
 drivers/gpu/drm/panfrost/panfrost_gem.c       | 140 +++++++++++++++---
 drivers/gpu/drm/panfrost/panfrost_gem.h       |  41 ++++-
 .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |   4 +-
 drivers/gpu/drm/panfrost/panfrost_job.c       |  13 +-
 drivers/gpu/drm/panfrost/panfrost_job.h       |   1 +
 drivers/gpu/drm/panfrost/panfrost_mmu.c       |  61 ++++----
 drivers/gpu/drm/panfrost/panfrost_mmu.h       |   6 +-
 drivers/gpu/drm/panfrost/panfrost_perfcnt.c   |  34 +++--
 9 files changed, 318 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 751df975534f..b406b5243b40 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -78,8 +78,10 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
 static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
 		struct drm_file *file)
 {
+	struct panfrost_file_priv *priv = file->driver_priv;
 	struct panfrost_gem_object *bo;
 	struct drm_panfrost_create_bo *args = data;
+	struct panfrost_gem_mapping *mapping;
 
 	if (!args->size || args->pad ||
 	    (args->flags & ~(PANFROST_BO_NOEXEC | PANFROST_BO_HEAP)))
@@ -95,7 +97,14 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
 	if (IS_ERR(bo))
 		return PTR_ERR(bo);
 
-	args->offset = bo->node.start << PAGE_SHIFT;
+	mapping = panfrost_gem_mapping_get(bo, priv);
+	if (!mapping) {
+		drm_gem_object_put_unlocked(&bo->base.base);
+		return -EINVAL;
+	}
+
+	args->offset = mapping->mmnode.start << PAGE_SHIFT;
+	panfrost_gem_mapping_put(mapping);
 
 	return 0;
 }
@@ -119,6 +128,11 @@ panfrost_lookup_bos(struct drm_device *dev,
 		  struct drm_panfrost_submit *args,
 		  struct panfrost_job *job)
 {
+	struct panfrost_file_priv *priv = file_priv->driver_priv;
+	struct panfrost_gem_object *bo;
+	unsigned int i;
+	int ret;
+
 	job->bo_count = args->bo_handle_count;
 
 	if (!job->bo_count)
@@ -130,9 +144,32 @@ panfrost_lookup_bos(struct drm_device *dev,
 	if (!job->implicit_fences)
 		return -ENOMEM;
 
-	return drm_gem_objects_lookup(file_priv,
-				      (void __user *)(uintptr_t)args->bo_handles,
-				      job->bo_count, &job->bos);
+	ret = drm_gem_objects_lookup(file_priv,
+				     (void __user *)(uintptr_t)args->bo_handles,
+				     job->bo_count, &job->bos);
+	if (ret)
+		return ret;
+
+	job->mappings = kvmalloc_array(job->bo_count,
+				       sizeof(struct panfrost_gem_mapping *),
+				       GFP_KERNEL | __GFP_ZERO);
+	if (!job->mappings)
+		return -ENOMEM;
+
+	for (i = 0; i < job->bo_count; i++) {
+		struct panfrost_gem_mapping *mapping;
+
+		bo = to_panfrost_bo(job->bos[i]);
+		mapping = panfrost_gem_mapping_get(bo, priv);
+		if (!mapping) {
+			ret = -EINVAL;
+			break;
+		}
+
+		job->mappings[i] = mapping;
+	}
+
+	return ret;
 }
 
 /**
@@ -320,7 +357,9 @@ static int panfrost_ioctl_mmap_bo(struct drm_device *dev, void *data,
 static int panfrost_ioctl_get_bo_offset(struct drm_device *dev, void *data,
 			    struct drm_file *file_priv)
 {
+	struct panfrost_file_priv *priv = file_priv->driver_priv;
 	struct drm_panfrost_get_bo_offset *args = data;
+	struct panfrost_gem_mapping *mapping;
 	struct drm_gem_object *gem_obj;
 	struct panfrost_gem_object *bo;
 
@@ -331,18 +370,25 @@ static int panfrost_ioctl_get_bo_offset(struct drm_device *dev, void *data,
 	}
 	bo = to_panfrost_bo(gem_obj);
 
-	args->offset = bo->node.start << PAGE_SHIFT;
-
+	mapping = panfrost_gem_mapping_get(bo, priv);
 	drm_gem_object_put_unlocked(gem_obj);
+
+	if (!mapping)
+		return -EINVAL;
+
+	args->offset = mapping->mmnode.start << PAGE_SHIFT;
+	panfrost_gem_mapping_put(mapping);
 	return 0;
 }
 
 static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
 				  struct drm_file *file_priv)
 {
+	struct panfrost_file_priv *priv = file_priv->driver_priv;
 	struct drm_panfrost_madvise *args = data;
 	struct panfrost_device *pfdev = dev->dev_private;
 	struct drm_gem_object *gem_obj;
+	struct panfrost_gem_object *bo;
 	int ret;
 
 	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
@@ -364,18 +410,48 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
 	if (ret)
 		goto out_unlock_object_name;
 
+	bo = to_panfrost_bo(gem_obj);
+
 	mutex_lock(&pfdev->shrinker_lock);
+	mutex_lock(&bo->mappings.lock);
+	if (args->madv == PANFROST_MADV_DONTNEED) {
+		struct panfrost_gem_mapping *first, *last;
+
+		first = list_first_entry(&bo->mappings.list,
+					 struct panfrost_gem_mapping,
+					 node);
+		last = list_last_entry(&bo->mappings.list,
+				       struct panfrost_gem_mapping,
+				       node);
+
+		/*
+		 * If we want to mark the BO purgeable, there must be only one
+		 * user: the caller FD.
+		 * We could do something smarter and mark the BO purgeable only
+		 * when all its users have marked it purgeable, but globally
+		 * visible/shared BOs are likely to never be marked purgeable
+		 * anyway, so let's not bother.
+		 */
+		if (first != last || WARN_ON_ONCE(first->mmu != &priv->mmu)) {
+			ret = -EINVAL;
+			goto out_unlock_mappings;
+		}
+	}
+
 	args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);
 
 	if (args->retained) {
-		struct panfrost_gem_object *bo = to_panfrost_bo(gem_obj);
-
 		if (args->madv == PANFROST_MADV_DONTNEED)
 			list_add_tail(&bo->base.madv_list,
 				      &pfdev->shrinker_list);
 		else if (args->madv == PANFROST_MADV_WILLNEED)
 			list_del_init(&bo->base.madv_list);
 	}
+
+	ret = 0;
+
+out_unlock_mappings:
+	mutex_unlock(&bo->mappings.lock);
 	mutex_unlock(&pfdev->shrinker_lock);
 
 out_unlock_object_name:
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 31d6417dd21c..5a2c463c45d3 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -29,6 +29,12 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
 	list_del_init(&bo->base.madv_list);
 	mutex_unlock(&pfdev->shrinker_lock);
 
+	/*
+	 * If we still have mappings attached to the BO, there's a problem in
+	 * our refcounting.
+	 */
+	WARN_ON_ONCE(!list_empty(&bo->mappings.list));
+
 	if (bo->sgts) {
 		int i;
 		int n_sgt = bo->base.base.size / SZ_2M;
@@ -46,6 +52,70 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
 	drm_gem_shmem_free_object(obj);
 }
 
+struct panfrost_gem_mapping *
+panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
+			 struct panfrost_file_priv *priv)
+{
+	struct panfrost_gem_mapping *mapping = NULL, *iter;
+
+	mutex_lock(&bo->mappings.lock);
+	list_for_each_entry(iter, &bo->mappings.list, node) {
+		if (iter->mmu == &priv->mmu) {
+			kref_get(&iter->refcount);
+			mapping = iter;
+		}
+	}
+	mutex_unlock(&bo->mappings.lock);
+
+	return mapping;
+}
+
+static void
+panfrost_gem_teardown_mapping(struct panfrost_gem_mapping *mapping)
+{
+	struct panfrost_file_priv *priv;
+
+	if (mapping->active)
+		panfrost_mmu_unmap(mapping);
+
+	priv = container_of(mapping->mmu, struct panfrost_file_priv, mmu);
+	spin_lock(&priv->mm_lock);
+	if (drm_mm_node_allocated(&mapping->mmnode))
+		drm_mm_remove_node(&mapping->mmnode);
+	spin_unlock(&priv->mm_lock);
+}
+
+static void panfrost_gem_mapping_release(struct kref *kref)
+{
+	struct panfrost_gem_mapping *mapping;
+	struct panfrost_file_priv *priv;
+
+	mapping = container_of(kref, struct panfrost_gem_mapping, refcount);
+	priv = container_of(mapping->mmu, struct panfrost_file_priv, mmu);
+
+	panfrost_gem_teardown_mapping(mapping);
+	drm_gem_object_put_unlocked(&mapping->obj->base.base);
+	kfree(mapping);
+}
+
+void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping)
+{
+	if (!mapping)
+		return;
+
+	kref_put(&mapping->refcount, panfrost_gem_mapping_release);
+}
+
+void panfrost_gem_teardown_mappings(struct panfrost_gem_object *bo)
+{
+	struct panfrost_gem_mapping *mapping;
+
+	mutex_lock(&bo->mappings.lock);
+	list_for_each_entry(mapping, &bo->mappings.list, node)
+		panfrost_gem_teardown_mapping(mapping);
+	mutex_unlock(&bo->mappings.lock);
+}
+
 int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)
 {
 	int ret;
@@ -54,6 +124,16 @@ int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)
 	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
 	unsigned long color = bo->noexec ? PANFROST_BO_NOEXEC : 0;
 	struct panfrost_file_priv *priv = file_priv->driver_priv;
+	struct panfrost_gem_mapping *mapping;
+
+	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
+	if (!mapping)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&mapping->node);
+	kref_init(&mapping->refcount);
+	drm_gem_object_get(obj);
+	mapping->obj = bo;
 
 	/*
 	 * Executable buffers cannot cross a 16MB boundary as the program
@@ -66,37 +146,61 @@ int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)
 	else
 		align = size >= SZ_2M ? SZ_2M >> PAGE_SHIFT : 0;
 
-	bo->mmu = &priv->mmu;
+	mapping->mmu = &priv->mmu;
 	spin_lock(&priv->mm_lock);
-	ret = drm_mm_insert_node_generic(&priv->mm, &bo->node,
+	ret = drm_mm_insert_node_generic(&priv->mm, &mapping->mmnode,
 					 size >> PAGE_SHIFT, align, color, 0);
 	spin_unlock(&priv->mm_lock);
 	if (ret)
-		return ret;
+		goto err;
 
 	if (!bo->is_heap) {
-		ret = panfrost_mmu_map(bo);
-		if (ret) {
-			spin_lock(&priv->mm_lock);
-			drm_mm_remove_node(&bo->node);
-			spin_unlock(&priv->mm_lock);
-		}
+		ret = panfrost_mmu_map(mapping);
+		if (ret)
+			goto err;
 	}
+
+	mutex_lock(&obj->dev->object_name_lock);
+	/*
+	 * We must make sure the BO has not been marked purgeable before
+	 * adding the mapping.
+	 */
+	if (bo->base.madv == PANFROST_MADV_WILLNEED) {
+		mutex_lock(&bo->mappings.lock);
+		list_add_tail(&mapping->node, &bo->mappings.list);
+		mutex_unlock(&bo->mappings.lock);
+	} else {
+		ret = -EINVAL;
+	}
+	mutex_unlock(&obj->dev->object_name_lock);
+
+	if (ret)
+		goto err;
+
+	return 0;
+
+err:
+	panfrost_gem_mapping_put(mapping);
 	return ret;
 }
 
 void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv)
 {
-	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
 	struct panfrost_file_priv *priv = file_priv->driver_priv;
+	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
+	struct panfrost_gem_mapping *mapping = NULL, *iter;
+
+	mutex_lock(&bo->mappings.lock);
+	list_for_each_entry(iter, &bo->mappings.list, node) {
+		if (iter->mmu == &priv->mmu) {
+			mapping = iter;
+			list_del(&iter->node);
+			break;
+		}
+	}
+	mutex_unlock(&bo->mappings.lock);
 
-	if (bo->is_mapped)
-		panfrost_mmu_unmap(bo);
-
-	spin_lock(&priv->mm_lock);
-	if (drm_mm_node_allocated(&bo->node))
-		drm_mm_remove_node(&bo->node);
-	spin_unlock(&priv->mm_lock);
+	panfrost_gem_mapping_put(mapping);
 }
 
 static struct dma_buf *
@@ -163,6 +267,8 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t
 	if (!obj)
 		return NULL;
 
+	INIT_LIST_HEAD(&obj->mappings.list);
+	mutex_init(&obj->mappings.lock);
 	obj->base.base.funcs = &panfrost_gem_funcs;
 
 	return &obj->base.base;
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
index 4b17e7308764..ca1bc9019600 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
@@ -13,23 +13,46 @@ struct panfrost_gem_object {
 	struct drm_gem_shmem_object base;
 	struct sg_table *sgts;
 
-	struct panfrost_mmu *mmu;
-	struct drm_mm_node node;
-	bool is_mapped		:1;
+	/*
+	 * Use a list for now. If searching a mapping ever becomes the
+	 * bottleneck, we should consider using an RB-tree, or even better,
+	 * let the core store drm_gem_object_mapping entries (where we
+	 * could place driver specific data) instead of drm_gem_object ones
+	 * in its drm_file->object_idr table.
+	 *
+	 * struct drm_gem_object_mapping {
+	 *	struct drm_gem_object *obj;
+	 *	void *driver_priv;
+	 * };
+	 */
+	struct {
+		struct list_head list;
+		struct mutex lock;
+	} mappings;
+
 	bool noexec		:1;
 	bool is_heap		:1;
 };
 
+struct panfrost_gem_mapping {
+	struct list_head node;
+	struct kref refcount;
+	struct panfrost_gem_object *obj;
+	struct drm_mm_node mmnode;
+	struct panfrost_mmu *mmu;
+	bool active		:1;
+};
+
 static inline
 struct  panfrost_gem_object *to_panfrost_bo(struct drm_gem_object *obj)
 {
 	return container_of(to_drm_gem_shmem_obj(obj), struct panfrost_gem_object, base);
 }
 
-static inline
-struct  panfrost_gem_object *drm_mm_node_to_panfrost_bo(struct drm_mm_node *node)
+static inline struct panfrost_gem_mapping *
+drm_mm_node_to_panfrost_mapping(struct drm_mm_node *node)
 {
-	return container_of(node, struct panfrost_gem_object, node);
+	return container_of(node, struct panfrost_gem_mapping, mmnode);
 }
 
 struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t size);
@@ -49,6 +72,12 @@ int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv);
 void panfrost_gem_close(struct drm_gem_object *obj,
 			struct drm_file *file_priv);
 
+struct panfrost_gem_mapping *
+panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
+			 struct panfrost_file_priv *priv);
+void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping);
+void panfrost_gem_teardown_mappings(struct panfrost_gem_object *bo);
+
 void panfrost_gem_shrinker_init(struct drm_device *dev);
 void panfrost_gem_shrinker_cleanup(struct drm_device *dev);
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
index 458f0fa68111..b36df326c860 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
@@ -39,11 +39,13 @@ panfrost_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc
 static bool panfrost_gem_purge(struct drm_gem_object *obj)
 {
 	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
+	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
+
 
 	if (!mutex_trylock(&shmem->pages_lock))
 		return false;
 
-	panfrost_mmu_unmap(to_panfrost_bo(obj));
+	panfrost_gem_teardown_mappings(bo);
 	drm_gem_shmem_purge_locked(obj);
 
 	mutex_unlock(&shmem->pages_lock);
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index cdd9448fbbdd..c85d45be3b5e 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -269,9 +269,20 @@ static void panfrost_job_cleanup(struct kref *ref)
 	dma_fence_put(job->done_fence);
 	dma_fence_put(job->render_done_fence);
 
-	if (job->bos) {
+	if (job->mappings) {
 		for (i = 0; i < job->bo_count; i++)
+			panfrost_gem_mapping_put(job->mappings[i]);
+		kvfree(job->mappings);
+	}
+
+	if (job->bos) {
+		struct panfrost_gem_object *bo;
+
+		for (i = 0; i < job->bo_count; i++) {
+			bo = to_panfrost_bo(job->bos[i]);
 			drm_gem_object_put_unlocked(job->bos[i]);
+		}
+
 		kvfree(job->bos);
 	}
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
index 62454128a792..bbd3ba97ff67 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.h
+++ b/drivers/gpu/drm/panfrost/panfrost_job.h
@@ -32,6 +32,7 @@ struct panfrost_job {
 
 	/* Exclusive fences we have taken from the BOs to wait for */
 	struct dma_fence **implicit_fences;
+	struct panfrost_gem_mapping **mappings;
 	struct drm_gem_object **bos;
 	u32 bo_count;
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index a3ed64a1f15e..763cfca886a7 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -269,14 +269,15 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
 	return 0;
 }
 
-int panfrost_mmu_map(struct panfrost_gem_object *bo)
+int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
 {
+	struct panfrost_gem_object *bo = mapping->obj;
 	struct drm_gem_object *obj = &bo->base.base;
 	struct panfrost_device *pfdev = to_panfrost_device(obj->dev);
 	struct sg_table *sgt;
 	int prot = IOMMU_READ | IOMMU_WRITE;
 
-	if (WARN_ON(bo->is_mapped))
+	if (WARN_ON(mapping->active))
 		return 0;
 
 	if (bo->noexec)
@@ -286,25 +287,28 @@ int panfrost_mmu_map(struct panfrost_gem_object *bo)
 	if (WARN_ON(IS_ERR(sgt)))
 		return PTR_ERR(sgt);
 
-	mmu_map_sg(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT, prot, sgt);
-	bo->is_mapped = true;
+	mmu_map_sg(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT,
+		   prot, sgt);
+	mapping->active = true;
 
 	return 0;
 }
 
-void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
+void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping)
 {
+	struct panfrost_gem_object *bo = mapping->obj;
 	struct drm_gem_object *obj = &bo->base.base;
 	struct panfrost_device *pfdev = to_panfrost_device(obj->dev);
-	struct io_pgtable_ops *ops = bo->mmu->pgtbl_ops;
-	u64 iova = bo->node.start << PAGE_SHIFT;
-	size_t len = bo->node.size << PAGE_SHIFT;
+	struct io_pgtable_ops *ops = mapping->mmu->pgtbl_ops;
+	u64 iova = mapping->mmnode.start << PAGE_SHIFT;
+	size_t len = mapping->mmnode.size << PAGE_SHIFT;
 	size_t unmapped_len = 0;
 
-	if (WARN_ON(!bo->is_mapped))
+	if (WARN_ON(!mapping->active))
 		return;
 
-	dev_dbg(pfdev->dev, "unmap: as=%d, iova=%llx, len=%zx", bo->mmu->as, iova, len);
+	dev_dbg(pfdev->dev, "unmap: as=%d, iova=%llx, len=%zx",
+		mapping->mmu->as, iova, len);
 
 	while (unmapped_len < len) {
 		size_t unmapped_page;
@@ -318,8 +322,9 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
 		unmapped_len += pgsize;
 	}
 
-	panfrost_mmu_flush_range(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT, len);
-	bo->is_mapped = false;
+	panfrost_mmu_flush_range(pfdev, mapping->mmu,
+				 mapping->mmnode.start << PAGE_SHIFT, len);
+	mapping->active = false;
 }
 
 static void mmu_tlb_inv_context_s1(void *cookie)
@@ -394,10 +399,10 @@ void panfrost_mmu_pgtable_free(struct panfrost_file_priv *priv)
 	free_io_pgtable_ops(mmu->pgtbl_ops);
 }
 
-static struct panfrost_gem_object *
-addr_to_drm_mm_node(struct panfrost_device *pfdev, int as, u64 addr)
+static struct panfrost_gem_mapping *
+addr_to_mapping(struct panfrost_device *pfdev, int as, u64 addr)
 {
-	struct panfrost_gem_object *bo = NULL;
+	struct panfrost_gem_mapping *mapping = NULL;
 	struct panfrost_file_priv *priv;
 	struct drm_mm_node *node;
 	u64 offset = addr >> PAGE_SHIFT;
@@ -418,8 +423,9 @@ addr_to_drm_mm_node(struct panfrost_device *pfdev, int as, u64 addr)
 	drm_mm_for_each_node(node, &priv->mm) {
 		if (offset >= node->start &&
 		    offset < (node->start + node->size)) {
-			bo = drm_mm_node_to_panfrost_bo(node);
-			drm_gem_object_get(&bo->base.base);
+			mapping = drm_mm_node_to_panfrost_mapping(node);
+
+			kref_get(&mapping->refcount);
 			break;
 		}
 	}
@@ -427,7 +433,7 @@ addr_to_drm_mm_node(struct panfrost_device *pfdev, int as, u64 addr)
 	spin_unlock(&priv->mm_lock);
 out:
 	spin_unlock(&pfdev->as_lock);
-	return bo;
+	return mapping;
 }
 
 #define NUM_FAULT_PAGES (SZ_2M / PAGE_SIZE)
@@ -436,28 +442,30 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 				       u64 addr)
 {
 	int ret, i;
+	struct panfrost_gem_mapping *bomapping;
 	struct panfrost_gem_object *bo;
 	struct address_space *mapping;
 	pgoff_t page_offset;
 	struct sg_table *sgt;
 	struct page **pages;
 
-	bo = addr_to_drm_mm_node(pfdev, as, addr);
-	if (!bo)
+	bomapping = addr_to_mapping(pfdev, as, addr);
+	if (!bomapping)
 		return -ENOENT;
 
+	bo = bomapping->obj;
 	if (!bo->is_heap) {
 		dev_WARN(pfdev->dev, "matching BO is not heap type (GPU VA = %llx)",
-			 bo->node.start << PAGE_SHIFT);
+			 bomapping->mmnode.start << PAGE_SHIFT);
 		ret = -EINVAL;
 		goto err_bo;
 	}
-	WARN_ON(bo->mmu->as != as);
+	WARN_ON(bomapping->mmu->as != as);
 
 	/* Assume 2MB alignment and size multiple */
 	addr &= ~((u64)SZ_2M - 1);
 	page_offset = addr >> PAGE_SHIFT;
-	page_offset -= bo->node.start;
+	page_offset -= bomapping->mmnode.start;
 
 	mutex_lock(&bo->base.pages_lock);
 
@@ -509,13 +517,14 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 		goto err_map;
 	}
 
-	mmu_map_sg(pfdev, bo->mmu, addr, IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
+	mmu_map_sg(pfdev, bomapping->mmu, addr,
+		   IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
 
-	bo->is_mapped = true;
+	bomapping->active = true;
 
 	dev_dbg(pfdev->dev, "mapped page fault @ AS%d %llx", as, addr);
 
-	drm_gem_object_put_unlocked(&bo->base.base);
+	panfrost_gem_mapping_put(bomapping);
 
 	return 0;
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.h b/drivers/gpu/drm/panfrost/panfrost_mmu.h
index 7c5b6775ae23..44fc2edf63ce 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.h
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.h
@@ -4,12 +4,12 @@
 #ifndef __PANFROST_MMU_H__
 #define __PANFROST_MMU_H__
 
-struct panfrost_gem_object;
+struct panfrost_gem_mapping;
 struct panfrost_file_priv;
 struct panfrost_mmu;
 
-int panfrost_mmu_map(struct panfrost_gem_object *bo);
-void panfrost_mmu_unmap(struct panfrost_gem_object *bo);
+int panfrost_mmu_map(struct panfrost_gem_mapping *mapping);
+void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping);
 
 int panfrost_mmu_init(struct panfrost_device *pfdev);
 void panfrost_mmu_fini(struct panfrost_device *pfdev);
diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
index 2c04e858c50a..684820448be3 100644
--- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
+++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
@@ -25,7 +25,7 @@
 #define V4_SHADERS_PER_COREGROUP	4
 
 struct panfrost_perfcnt {
-	struct panfrost_gem_object *bo;
+	struct panfrost_gem_mapping *mapping;
 	size_t bosize;
 	void *buf;
 	struct panfrost_file_priv *user;
@@ -49,7 +49,7 @@ static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev)
 	int ret;
 
 	reinit_completion(&pfdev->perfcnt->dump_comp);
-	gpuva = pfdev->perfcnt->bo->node.start << PAGE_SHIFT;
+	gpuva = pfdev->perfcnt->mapping->mmnode.start << PAGE_SHIFT;
 	gpu_write(pfdev, GPU_PERFCNT_BASE_LO, gpuva);
 	gpu_write(pfdev, GPU_PERFCNT_BASE_HI, gpuva >> 32);
 	gpu_write(pfdev, GPU_INT_CLEAR,
@@ -89,17 +89,22 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 	if (IS_ERR(bo))
 		return PTR_ERR(bo);
 
-	perfcnt->bo = to_panfrost_bo(&bo->base);
-
 	/* Map the perfcnt buf in the address space attached to file_priv. */
-	ret = panfrost_gem_open(&perfcnt->bo->base.base, file_priv);
+	ret = panfrost_gem_open(&bo->base, file_priv);
 	if (ret)
 		goto err_put_bo;
 
+	perfcnt->mapping = panfrost_gem_mapping_get(to_panfrost_bo(&bo->base),
+						    user);
+	if (!perfcnt->mapping) {
+		ret = -EINVAL;
+		goto err_close_bo;
+	}
+
 	perfcnt->buf = drm_gem_shmem_vmap(&bo->base);
 	if (IS_ERR(perfcnt->buf)) {
 		ret = PTR_ERR(perfcnt->buf);
-		goto err_close_bo;
+		goto err_put_mapping;
 	}
 
 	/*
@@ -154,12 +159,17 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
 		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
 
+	/* The BO ref is retained by the mapping. */
+	drm_gem_object_put_unlocked(&bo->base);
+
 	return 0;
 
 err_vunmap:
-	drm_gem_shmem_vunmap(&perfcnt->bo->base.base, perfcnt->buf);
+	drm_gem_shmem_vunmap(&bo->base, perfcnt->buf);
+err_put_mapping:
+	panfrost_gem_mapping_put(perfcnt->mapping);
 err_close_bo:
-	panfrost_gem_close(&perfcnt->bo->base.base, file_priv);
+	panfrost_gem_close(&bo->base, file_priv);
 err_put_bo:
 	drm_gem_object_put_unlocked(&bo->base);
 	return ret;
@@ -182,11 +192,11 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
 		  GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF));
 
 	perfcnt->user = NULL;
-	drm_gem_shmem_vunmap(&perfcnt->bo->base.base, perfcnt->buf);
+	drm_gem_shmem_vunmap(&perfcnt->mapping->obj->base.base, perfcnt->buf);
 	perfcnt->buf = NULL;
-	panfrost_gem_close(&perfcnt->bo->base.base, file_priv);
-	drm_gem_object_put_unlocked(&perfcnt->bo->base.base);
-	perfcnt->bo = NULL;
+	panfrost_gem_close(&perfcnt->mapping->obj->base.base, file_priv);
+	panfrost_gem_mapping_put(perfcnt->mapping);
+	perfcnt->mapping = NULL;
 	pm_runtime_mark_last_busy(pfdev->dev);
 	pm_runtime_put_autosuspend(pfdev->dev);
 
-- 
2.23.0

[PATCH 8/8] drm/panfrost: Make sure the shrinker does not reclaim referenced BOs

[Boris Brezillon]

Userspace might tag a BO purgeable while it's still referenced by GPU
jobs. We need to make sure the shrinker does not purge such BOs until
all jobs referencing it are finished.

Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c          | 1 +
 drivers/gpu/drm/panfrost/panfrost_gem.h          | 6 ++++++
 drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c | 2 ++
 drivers/gpu/drm/panfrost/panfrost_job.c          | 7 ++++++-
 4 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index b406b5243b40..297c0e7304d2 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -166,6 +166,7 @@ panfrost_lookup_bos(struct drm_device *dev,
 			break;
 		}
 
+		atomic_inc(&bo->gpu_usecount);
 		job->mappings[i] = mapping;
 	}
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
index ca1bc9019600..b3517ff9630c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
@@ -30,6 +30,12 @@ struct panfrost_gem_object {
 		struct mutex lock;
 	} mappings;
 
+	/*
+	 * Count the number of jobs referencing this BO so we don't let the
+	 * shrinker reclaim this object prematurely.
+	 */
+	atomic_t gpu_usecount;
+
 	bool noexec		:1;
 	bool is_heap		:1;
 };
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
index b36df326c860..288e46c40673 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
@@ -41,6 +41,8 @@ static bool panfrost_gem_purge(struct drm_gem_object *obj)
 	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
 	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
 
+	if (atomic_read(&bo->gpu_usecount))
+		return false;
 
 	if (!mutex_trylock(&shmem->pages_lock))
 		return false;
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index c85d45be3b5e..2b12aa87ff32 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -270,8 +270,13 @@ static void panfrost_job_cleanup(struct kref *ref)
 	dma_fence_put(job->render_done_fence);
 
 	if (job->mappings) {
-		for (i = 0; i < job->bo_count; i++)
+		for (i = 0; i < job->bo_count; i++) {
+			if (!job->mappings[i])
+				break;
+
+			atomic_dec(&job->mappings[i]->obj->gpu_usecount);
 			panfrost_gem_mapping_put(job->mappings[i]);
+		}
 		kvfree(job->mappings);
 	}
 
-- 
2.23.0

Re: [PATCH 6/8] drm/panfrost: Make sure imported/exported BOs are never purged

[Boris Brezillon]

On Fri, 29 Nov 2019 14:59:06 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> We don't want imported/exported BOs to be purges, as those are shared
> with other processes that might still use them. We should also refuse
> to export a BO if it's been marked purgeable or has already been purged.
> 
> Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 19 ++++++++++++++++-
>  drivers/gpu/drm/panfrost/panfrost_gem.c | 27 +++++++++++++++++++++++++
>  2 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 1c67ac434e10..751df975534f 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -343,6 +343,7 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  	struct drm_panfrost_madvise *args = data;
>  	struct panfrost_device *pfdev = dev->dev_private;
>  	struct drm_gem_object *gem_obj;
> +	int ret;
>  
>  	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
>  	if (!gem_obj) {
> @@ -350,6 +351,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  		return -ENOENT;
>  	}
>  
> +	/*
> +	 * We don't want to mark exported/imported BOs as purgeable: we're not
> +	 * the only owner in that case.
> +	 */
> +	mutex_lock(&dev->object_name_lock);
> +	if (gem_obj->dma_buf)
> +		ret = -EINVAL;
> +	else
> +		ret = 0;
> +
> +	if (ret)
> +		goto out_unlock_object_name;
> +
>  	mutex_lock(&pfdev->shrinker_lock);
>  	args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);
>  
> @@ -364,8 +378,11 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  	}
>  	mutex_unlock(&pfdev->shrinker_lock);
>  
> +out_unlock_object_name:
> +	mutex_unlock(&dev->object_name_lock);
> +
>  	drm_gem_object_put_unlocked(gem_obj);
> -	return 0;
> +	return ret;
>  }
>  
>  int panfrost_unstable_ioctl_check(void)
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 92a95210a899..31d6417dd21c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -99,6 +99,32 @@ void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv)
>  	spin_unlock(&priv->mm_lock);
>  }
>  
> +static struct dma_buf *
> +panfrost_gem_export(struct drm_gem_object *obj, int flags)
> +{
> +	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
> +	int ret;
> +
> +	/*
> +	 * We must make sure the BO has not been marked purgeable/purged before
> +	 * adding the mapping.

	   ^s/adding the mapping/exporting it/

> +	 * Note that we don't need to protect this test with a lock because
> +	 * &drm_gem_object_funcs.export() is called with
> +	 * &drm_device.object_lock held, and panfrost_ioctl_madvise() takes
> +	 * this lock before calling drm_gem_shmem_madvise() (the function that
> +	 * modifies bo->base.madv).
> +	 */
> +	if (bo->base.madv == PANFROST_MADV_WILLNEED)
> +		ret = -EINVAL;
> +	else
> +		ret = 0;
> +
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return drm_gem_prime_export(obj, flags);
> +}
> +
>  static int panfrost_gem_pin(struct drm_gem_object *obj)
>  {
>  	if (to_panfrost_bo(obj)->is_heap)
> @@ -112,6 +138,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
>  	.open = panfrost_gem_open,
>  	.close = panfrost_gem_close,
>  	.print_info = drm_gem_shmem_print_info,
> +	.export = panfrost_gem_export,
>  	.pin = panfrost_gem_pin,
>  	.unpin = drm_gem_shmem_unpin,
>  	.get_sg_table = drm_gem_shmem_get_sg_table,

Re: [PATCH 1/8] drm/panfrost: Make panfrost_job_run() return an ERR_PTR() instead of NULL

[Steven Price]

On 29/11/2019 13:59, Boris Brezillon wrote:
> If we don't do that, dma_fence_set_error() complains (called from
> drm_sched_main()).
> 
> Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

This might be worth doing, but actually it's not Panfrost that is broken
it's the callers, see [1] and [2]. So I don't think we want the
Fixes/stable tag.

[1] https://patchwork.kernel.org/patch/11218399/
[2] https://patchwork.kernel.org/patch/11267073/

> ---
>  drivers/gpu/drm/panfrost/panfrost_job.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 21f34d44aac2..cdd9448fbbdd 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -328,13 +328,13 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
>  	struct dma_fence *fence = NULL;
>  
>  	if (unlikely(job->base.s_fence->finished.error))
> -		return NULL;
> +		return ERR_PTR(job->base.s_fence->finished.error);
>  
>  	pfdev->jobs[slot] = job;
>  
>  	fence = panfrost_fence_create(pfdev, slot);
>  	if (IS_ERR(fence))
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);

Why override the error from panfrost_fence_create? In this case we can just:

	return fence;

Steve

>  
>  	if (job->done_fence)
>  		dma_fence_put(job->done_fence);
> 

Re: [PATCH 2/8] drm/panfrost: Fix a race in panfrost_ioctl_madvise()

[Steven Price]

On 29/11/2019 13:59, Boris Brezillon wrote:
> If 2 threads change the MADVISE property of the same BO in parallel we
> might end up with an shmem->madv value that's inconsistent with the
> presence of the BO in the shrinker list.

I'm a bit worried from the point of view of user space sanity that you
observed this - but clearly the kernel should be robust!

> 
> The easiest solution to fix that is to protect the
> drm_gem_shmem_madvise() call with the shrinker lock.
> 
> Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index f21bc8a7ee3a..efc0a24d1f4c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -347,20 +347,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  		return -ENOENT;
>  	}
>  
> +	mutex_lock(&pfdev->shrinker_lock);
>  	args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);
>  
>  	if (args->retained) {
>  		struct panfrost_gem_object *bo = to_panfrost_bo(gem_obj);
>  
> -		mutex_lock(&pfdev->shrinker_lock);
> -
>  		if (args->madv == PANFROST_MADV_DONTNEED)
> -			list_add_tail(&bo->base.madv_list, &pfdev->shrinker_list);
> +			list_add_tail(&bo->base.madv_list,
> +				      &pfdev->shrinker_list);
>  		else if (args->madv == PANFROST_MADV_WILLNEED)
>  			list_del_init(&bo->base.madv_list);
> -
> -		mutex_unlock(&pfdev->shrinker_lock);
>  	}
> +	mutex_unlock(&pfdev->shrinker_lock);
>  
>  	drm_gem_object_put_unlocked(gem_obj);
>  	return 0;
> 

Re: [PATCH 3/8] drm/panfrost: Fix a BO leak in panfrost_ioctl_mmap_bo()

[Steven Price]

On 29/11/2019 13:59, Boris Brezillon wrote:
> We should release the reference we grabbed when an error occurs.
> 
> Fixes: 187d2929206e ("drm/panfrost: Add support for GPU heap allocations")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index efc0a24d1f4c..2630c5027c63 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -303,14 +303,17 @@ static int panfrost_ioctl_mmap_bo(struct drm_device *dev, void *data,
>  	}
>  
>  	/* Don't allow mmapping of heap objects as pages are not pinned. */
> -	if (to_panfrost_bo(gem_obj)->is_heap)
> -		return -EINVAL;
> +	if (to_panfrost_bo(gem_obj)->is_heap) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	ret = drm_gem_create_mmap_offset(gem_obj);
>  	if (ret == 0)
>  		args->offset = drm_vma_node_offset_addr(&gem_obj->vma_node);
> -	drm_gem_object_put_unlocked(gem_obj);
>  
> +out:
> +	drm_gem_object_put_unlocked(gem_obj);
>  	return ret;
>  }
>  
> 

Re: [PATCH 4/8] drm/panfrost: Fix a race in panfrost_gem_free_object()

[Steven Price]

On 29/11/2019 13:59, Boris Brezillon wrote:
> panfrost_gem_shrinker_scan() might purge a BO (release the sgt and
> kill the GPU mapping) that's being freed by panfrost_gem_free_object()
> if we don't remove the BO from the shrinker list at the beginning of
> panfrost_gem_free_object().
> 
> Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panfrost/panfrost_gem.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index acb07fe06580..daf4c55a2863 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -19,6 +19,16 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
>  	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
>  	struct panfrost_device *pfdev = obj->dev->dev_private;
>  
> +	/*
> +	 * Make sure the BO is no longer inserted in the shrinker list before
> +	 * taking care of the destruction itself. If we don't do that we have a
> +	 * race condition between this function and what's done in
> +	 * panfrost_gem_shrinker_scan().
> +	 */
> +	mutex_lock(&pfdev->shrinker_lock);
> +	list_del_init(&bo->base.madv_list);
> +	mutex_unlock(&pfdev->shrinker_lock);
> +
>  	if (bo->sgts) {
>  		int i;
>  		int n_sgt = bo->base.base.size / SZ_2M;
> @@ -33,11 +43,6 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
>  		kfree(bo->sgts);
>  	}
>  
> -	mutex_lock(&pfdev->shrinker_lock);
> -	if (!list_empty(&bo->base.madv_list))
> -		list_del(&bo->base.madv_list);
> -	mutex_unlock(&pfdev->shrinker_lock);
> -
>  	drm_gem_shmem_free_object(obj);
>  }
>  
> 

Re: [PATCH 1/8] drm/panfrost: Make panfrost_job_run() return an ERR_PTR() instead of NULL

[Boris Brezillon]

On Fri, 29 Nov 2019 14:19:50 +0000
Steven Price <steven.price@arm.com> wrote:

> On 29/11/2019 13:59, Boris Brezillon wrote:
> > If we don't do that, dma_fence_set_error() complains (called from
> > drm_sched_main()).
> > 
> > Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> 
> This might be worth doing, but actually it's not Panfrost that is broken
> it's the callers, see [1] and [2]. So I don't think we want the
> Fixes/stable tag.

Okay.

> 
> [1] https://patchwork.kernel.org/patch/11218399/
> [2] https://patchwork.kernel.org/patch/11267073/
> 
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_job.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> > index 21f34d44aac2..cdd9448fbbdd 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> > @@ -328,13 +328,13 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
> >  	struct dma_fence *fence = NULL;
> >  
> >  	if (unlikely(job->base.s_fence->finished.error))
> > -		return NULL;
> > +		return ERR_PTR(job->base.s_fence->finished.error);

Hm, so we can keep the return NULL here if [1] is applied (the error
is preserved), but I'm not sure it's clearer that way.

> >  
> >  	pfdev->jobs[slot] = job;
> >  
> >  	fence = panfrost_fence_create(pfdev, slot);
> >  	if (IS_ERR(fence))
> > -		return NULL;
> > +		return ERR_PTR(-ENOMEM);  

This one should be fixed though, otherwise the error is never updated,
so I'm wondering if it doesn't deserve a Fixes tag in the end...

> 
> Why override the error from panfrost_fence_create? In this case we can just:
> 
> 	return fence;

Indeed.

> 
> Steve
> 
> >  
> >  	if (job->done_fence)
> >  		dma_fence_put(job->done_fence);
> >   
> 

Re: [PATCH 2/8] drm/panfrost: Fix a race in panfrost_ioctl_madvise()

[Boris Brezillon]

On Fri, 29 Nov 2019 14:24:48 +0000
Steven Price <steven.price@arm.com> wrote:

> On 29/11/2019 13:59, Boris Brezillon wrote:
> > If 2 threads change the MADVISE property of the same BO in parallel we
> > might end up with an shmem->madv value that's inconsistent with the
> > presence of the BO in the shrinker list.  
> 
> I'm a bit worried from the point of view of user space sanity that you
> observed this - but clearly the kernel should be robust!

It's not something I observed, just found the race by inspecting the
code, and I thought it was worth fixing it.

> 
> > 
> > The easiest solution to fix that is to protect the
> > drm_gem_shmem_madvise() call with the shrinker lock.
> > 
> > Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> 
> Reviewed-by: Steven Price <steven.price@arm.com>

Thanks.

> 
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_drv.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index f21bc8a7ee3a..efc0a24d1f4c 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -347,20 +347,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> >  		return -ENOENT;
> >  	}
> >  
> > +	mutex_lock(&pfdev->shrinker_lock);
> >  	args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);
> >  
> >  	if (args->retained) {
> >  		struct panfrost_gem_object *bo = to_panfrost_bo(gem_obj);
> >  
> > -		mutex_lock(&pfdev->shrinker_lock);
> > -
> >  		if (args->madv == PANFROST_MADV_DONTNEED)
> > -			list_add_tail(&bo->base.madv_list, &pfdev->shrinker_list);
> > +			list_add_tail(&bo->base.madv_list,
> > +				      &pfdev->shrinker_list);
> >  		else if (args->madv == PANFROST_MADV_WILLNEED)
> >  			list_del_init(&bo->base.madv_list);
> > -
> > -		mutex_unlock(&pfdev->shrinker_lock);
> >  	}
> > +	mutex_unlock(&pfdev->shrinker_lock);
> >  
> >  	drm_gem_object_put_unlocked(gem_obj);
> >  	return 0;
> >   
> 

Re: [PATCH 5/8] drm/panfrost: Open/close the perfcnt BO

[Steven Price]

On 29/11/2019 13:59, Boris Brezillon wrote:
> Commit a5efb4c9a562 ("drm/panfrost: Restructure the GEM object creation")
> moved the drm_mm_insert_node_generic() call to the gem->open() hook,
> but forgot to update perfcnt accordingly.
> 
> Patch the perfcnt logic to call panfrost_gem_open/close() where
> appropriate.
> 
> Fixes: a5efb4c9a562 ("drm/panfrost: Restructure the GEM object creation")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

Steve

> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c     |  2 +-
>  drivers/gpu/drm/panfrost/panfrost_gem.c     |  4 ++--
>  drivers/gpu/drm/panfrost/panfrost_gem.h     |  4 ++++
>  drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 23 +++++++++++++--------
>  drivers/gpu/drm/panfrost/panfrost_perfcnt.h |  2 +-
>  5 files changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 2630c5027c63..1c67ac434e10 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -445,7 +445,7 @@ panfrost_postclose(struct drm_device *dev, struct drm_file *file)
>  {
>  	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
>  
> -	panfrost_perfcnt_close(panfrost_priv);
> +	panfrost_perfcnt_close(file);
>  	panfrost_job_close(panfrost_priv);
>  
>  	panfrost_mmu_pgtable_free(panfrost_priv);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index daf4c55a2863..92a95210a899 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -46,7 +46,7 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
>  	drm_gem_shmem_free_object(obj);
>  }
>  
> -static int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)
> +int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)
>  {
>  	int ret;
>  	size_t size = obj->size;
> @@ -85,7 +85,7 @@ static int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_p
>  	return ret;
>  }
>  
> -static void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv)
> +void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv)
>  {
>  	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
>  	struct panfrost_file_priv *priv = file_priv->driver_priv;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index 50920819cc16..4b17e7308764 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -45,6 +45,10 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv,
>  				u32 flags,
>  				uint32_t *handle);
>  
> +int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv);
> +void panfrost_gem_close(struct drm_gem_object *obj,
> +			struct drm_file *file_priv);
> +
>  void panfrost_gem_shrinker_init(struct drm_device *dev);
>  void panfrost_gem_shrinker_cleanup(struct drm_device *dev);
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index 2dba192bf198..2c04e858c50a 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> @@ -67,9 +67,10 @@ static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev)
>  }
>  
>  static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
> -					  struct panfrost_file_priv *user,
> +					  struct drm_file *file_priv,
>  					  unsigned int counterset)
>  {
> +	struct panfrost_file_priv *user = file_priv->driver_priv;
>  	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
>  	struct drm_gem_shmem_object *bo;
>  	u32 cfg;
> @@ -91,14 +92,14 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  	perfcnt->bo = to_panfrost_bo(&bo->base);
>  
>  	/* Map the perfcnt buf in the address space attached to file_priv. */
> -	ret = panfrost_mmu_map(perfcnt->bo);
> +	ret = panfrost_gem_open(&perfcnt->bo->base.base, file_priv);
>  	if (ret)
>  		goto err_put_bo;
>  
>  	perfcnt->buf = drm_gem_shmem_vmap(&bo->base);
>  	if (IS_ERR(perfcnt->buf)) {
>  		ret = PTR_ERR(perfcnt->buf);
> -		goto err_put_bo;
> +		goto err_close_bo;
>  	}
>  
>  	/*
> @@ -157,14 +158,17 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  
>  err_vunmap:
>  	drm_gem_shmem_vunmap(&perfcnt->bo->base.base, perfcnt->buf);
> +err_close_bo:
> +	panfrost_gem_close(&perfcnt->bo->base.base, file_priv);
>  err_put_bo:
>  	drm_gem_object_put_unlocked(&bo->base);
>  	return ret;
>  }
>  
>  static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
> -					   struct panfrost_file_priv *user)
> +					   struct drm_file *file_priv)
>  {
> +	struct panfrost_file_priv *user = file_priv->driver_priv;
>  	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
>  
>  	if (user != perfcnt->user)
> @@ -180,6 +184,7 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
>  	perfcnt->user = NULL;
>  	drm_gem_shmem_vunmap(&perfcnt->bo->base.base, perfcnt->buf);
>  	perfcnt->buf = NULL;
> +	panfrost_gem_close(&perfcnt->bo->base.base, file_priv);
>  	drm_gem_object_put_unlocked(&perfcnt->bo->base.base);
>  	perfcnt->bo = NULL;
>  	pm_runtime_mark_last_busy(pfdev->dev);
> @@ -191,7 +196,6 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
>  int panfrost_ioctl_perfcnt_enable(struct drm_device *dev, void *data,
>  				  struct drm_file *file_priv)
>  {
> -	struct panfrost_file_priv *pfile = file_priv->driver_priv;
>  	struct panfrost_device *pfdev = dev->dev_private;
>  	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
>  	struct drm_panfrost_perfcnt_enable *req = data;
> @@ -207,10 +211,10 @@ int panfrost_ioctl_perfcnt_enable(struct drm_device *dev, void *data,
>  
>  	mutex_lock(&perfcnt->lock);
>  	if (req->enable)
> -		ret = panfrost_perfcnt_enable_locked(pfdev, pfile,
> +		ret = panfrost_perfcnt_enable_locked(pfdev, file_priv,
>  						     req->counterset);
>  	else
> -		ret = panfrost_perfcnt_disable_locked(pfdev, pfile);
> +		ret = panfrost_perfcnt_disable_locked(pfdev, file_priv);
>  	mutex_unlock(&perfcnt->lock);
>  
>  	return ret;
> @@ -248,15 +252,16 @@ int panfrost_ioctl_perfcnt_dump(struct drm_device *dev, void *data,
>  	return ret;
>  }
>  
> -void panfrost_perfcnt_close(struct panfrost_file_priv *pfile)
> +void panfrost_perfcnt_close(struct drm_file *file_priv)
>  {
> +	struct panfrost_file_priv *pfile = file_priv->driver_priv;
>  	struct panfrost_device *pfdev = pfile->pfdev;
>  	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
>  
>  	pm_runtime_get_sync(pfdev->dev);
>  	mutex_lock(&perfcnt->lock);
>  	if (perfcnt->user == pfile)
> -		panfrost_perfcnt_disable_locked(pfdev, pfile);
> +		panfrost_perfcnt_disable_locked(pfdev, file_priv);
>  	mutex_unlock(&perfcnt->lock);
>  	pm_runtime_mark_last_busy(pfdev->dev);
>  	pm_runtime_put_autosuspend(pfdev->dev);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.h b/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
> index 13b8fdaa1b43..8bbcf5f5fb33 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
> @@ -9,7 +9,7 @@ void panfrost_perfcnt_sample_done(struct panfrost_device *pfdev);
>  void panfrost_perfcnt_clean_cache_done(struct panfrost_device *pfdev);
>  int panfrost_perfcnt_init(struct panfrost_device *pfdev);
>  void panfrost_perfcnt_fini(struct panfrost_device *pfdev);
> -void panfrost_perfcnt_close(struct panfrost_file_priv *pfile);
> +void panfrost_perfcnt_close(struct drm_file *file_priv);
>  int panfrost_ioctl_perfcnt_enable(struct drm_device *dev, void *data,
>  				  struct drm_file *file_priv);
>  int panfrost_ioctl_perfcnt_dump(struct drm_device *dev, void *data,
> 

Re: [PATCH 1/8] drm/panfrost: Make panfrost_job_run() return an ERR_PTR() instead of NULL

[Steven Price]

On 29/11/2019 14:31, Boris Brezillon wrote:
> On Fri, 29 Nov 2019 14:19:50 +0000
> Steven Price <steven.price@arm.com> wrote:
> 
>> On 29/11/2019 13:59, Boris Brezillon wrote:
>>> If we don't do that, dma_fence_set_error() complains (called from
>>> drm_sched_main()).
>>>
>>> Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
>>
>> This might be worth doing, but actually it's not Panfrost that is broken
>> it's the callers, see [1] and [2]. So I don't think we want the
>> Fixes/stable tag.
> 
> Okay.
> 
>>
>> [1] https://patchwork.kernel.org/patch/11218399/
>> [2] https://patchwork.kernel.org/patch/11267073/
>>
>>> ---
>>>  drivers/gpu/drm/panfrost/panfrost_job.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
>>> index 21f34d44aac2..cdd9448fbbdd 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
>>> @@ -328,13 +328,13 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
>>>  	struct dma_fence *fence = NULL;
>>>  
>>>  	if (unlikely(job->base.s_fence->finished.error))
>>> -		return NULL;
>>> +		return ERR_PTR(job->base.s_fence->finished.error);
> 
> Hm, so we can keep the return NULL here if [1] is applied (the error
> is preserved), but I'm not sure it's clearer that way.
> 
>>>  
>>>  	pfdev->jobs[slot] = job;
>>>  
>>>  	fence = panfrost_fence_create(pfdev, slot);
>>>  	if (IS_ERR(fence))
>>> -		return NULL;
>>> +		return ERR_PTR(-ENOMEM);  
> 
> This one should be fixed though, otherwise the error is never updated,
> so I'm wondering if it doesn't deserve a Fixes tag in the end...

Good point, although this can't be back-ported before [3] (well unless
that commit is considered stable material too), so this is only really
relevant for v5.4. But worth fixing there.

[3] 167bf96014a0 ("drm/sched: Set error to s_fence if HW job submission
failed.")

Steve

>>
>> Why override the error from panfrost_fence_create? In this case we can just:
>>
>> 	return fence;
> 
> Indeed.
> 
>>
>> Steve
>>
>>>  
>>>  	if (job->done_fence)
>>>  		dma_fence_put(job->done_fence);
>>>   
>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 2/8] drm/panfrost: Fix a race in panfrost_ioctl_madvise()

[Steven Price]

On 29/11/2019 14:33, Boris Brezillon wrote:
> On Fri, 29 Nov 2019 14:24:48 +0000
> Steven Price <steven.price@arm.com> wrote:
> 
>> On 29/11/2019 13:59, Boris Brezillon wrote:
>>> If 2 threads change the MADVISE property of the same BO in parallel we
>>> might end up with an shmem->madv value that's inconsistent with the
>>> presence of the BO in the shrinker list.  
>>
>> I'm a bit worried from the point of view of user space sanity that you
>> observed this - but clearly the kernel should be robust!
> 
> It's not something I observed, just found the race by inspecting the
> code, and I thought it was worth fixing it.

Good! ;) Your cover letter referring to a "debug session" made me think
you'd actually hit all these.

Steve

>>
>>>
>>> The easiest solution to fix that is to protect the
>>> drm_gem_shmem_madvise() call with the shrinker lock.
>>>
>>> Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
>>
>> Reviewed-by: Steven Price <steven.price@arm.com>
> 
> Thanks.
> 
>>
>>> ---
>>>  drivers/gpu/drm/panfrost/panfrost_drv.c | 9 ++++-----
>>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
>>> index f21bc8a7ee3a..efc0a24d1f4c 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
>>> @@ -347,20 +347,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>>>  		return -ENOENT;
>>>  	}
>>>  
>>> +	mutex_lock(&pfdev->shrinker_lock);
>>>  	args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);
>>>  
>>>  	if (args->retained) {
>>>  		struct panfrost_gem_object *bo = to_panfrost_bo(gem_obj);
>>>  
>>> -		mutex_lock(&pfdev->shrinker_lock);
>>> -
>>>  		if (args->madv == PANFROST_MADV_DONTNEED)
>>> -			list_add_tail(&bo->base.madv_list, &pfdev->shrinker_list);
>>> +			list_add_tail(&bo->base.madv_list,
>>> +				      &pfdev->shrinker_list);
>>>  		else if (args->madv == PANFROST_MADV_WILLNEED)
>>>  			list_del_init(&bo->base.madv_list);
>>> -
>>> -		mutex_unlock(&pfdev->shrinker_lock);
>>>  	}
>>> +	mutex_unlock(&pfdev->shrinker_lock);
>>>  
>>>  	drm_gem_object_put_unlocked(gem_obj);
>>>  	return 0;
>>>   
>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 6/8] drm/panfrost: Make sure imported/exported BOs are never purged

[Steven Price]

On 29/11/2019 13:59, Boris Brezillon wrote:
> We don't want imported/exported BOs to be purges, as those are shared

s/purges/purged/

> with other processes that might still use them. We should also refuse
> to export a BO if it's been marked purgeable or has already been purged.
> 
> Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 19 ++++++++++++++++-
>  drivers/gpu/drm/panfrost/panfrost_gem.c | 27 +++++++++++++++++++++++++
>  2 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 1c67ac434e10..751df975534f 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -343,6 +343,7 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  	struct drm_panfrost_madvise *args = data;
>  	struct panfrost_device *pfdev = dev->dev_private;
>  	struct drm_gem_object *gem_obj;
> +	int ret;
>  
>  	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
>  	if (!gem_obj) {
> @@ -350,6 +351,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  		return -ENOENT;
>  	}
>  
> +	/*
> +	 * We don't want to mark exported/imported BOs as purgeable: we're not
> +	 * the only owner in that case.
> +	 */
> +	mutex_lock(&dev->object_name_lock);
> +	if (gem_obj->dma_buf)
> +		ret = -EINVAL;
> +	else
> +		ret = 0;
> +
> +	if (ret)
> +		goto out_unlock_object_name;
> +

This might be better by setting ret = 0 at the beginning of the
function. This can then be re-written as the more normal:

	if (gem_obj->dma_buf) {
		ret = -EINVAL;
		goto out_unlock_object_name;
	]

>  	mutex_lock(&pfdev->shrinker_lock);
>  	args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);
>  
> @@ -364,8 +378,11 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  	}
>  	mutex_unlock(&pfdev->shrinker_lock);
>  
> +out_unlock_object_name:
> +	mutex_unlock(&dev->object_name_lock);
> +
>  	drm_gem_object_put_unlocked(gem_obj);
> -	return 0;
> +	return ret;
>  }
>  
>  int panfrost_unstable_ioctl_check(void)
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 92a95210a899..31d6417dd21c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -99,6 +99,32 @@ void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv)
>  	spin_unlock(&priv->mm_lock);
>  }
>  
> +static struct dma_buf *
> +panfrost_gem_export(struct drm_gem_object *obj, int flags)
> +{
> +	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
> +	int ret;
> +
> +	/*
> +	 * We must make sure the BO has not been marked purgeable/purged before
> +	 * adding the mapping.
> +	 * Note that we don't need to protect this test with a lock because
> +	 * &drm_gem_object_funcs.export() is called with
> +	 * &drm_device.object_lock held, and panfrost_ioctl_madvise() takes
> +	 * this lock before calling drm_gem_shmem_madvise() (the function that
> +	 * modifies bo->base.madv).
> +	 */
> +	if (bo->base.madv == PANFROST_MADV_WILLNEED)
> +		ret = -EINVAL;
> +	else
> +		ret = 0;
> +
> +	if (ret)
> +		return ERR_PTR(ret);

No need for ret here:

	if (bo->base.madv == PANFROST_MADV_WILLNEED)
		return ERR_PTR(-EINVAL);

Steve

> +
> +	return drm_gem_prime_export(obj, flags);
> +}
> +
>  static int panfrost_gem_pin(struct drm_gem_object *obj)
>  {
>  	if (to_panfrost_bo(obj)->is_heap)
> @@ -112,6 +138,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
>  	.open = panfrost_gem_open,
>  	.close = panfrost_gem_close,
>  	.print_info = drm_gem_shmem_print_info,
> +	.export = panfrost_gem_export,
>  	.pin = panfrost_gem_pin,
>  	.unpin = drm_gem_shmem_unpin,
>  	.get_sg_table = drm_gem_shmem_get_sg_table,
> 

Re: [PATCH 6/8] drm/panfrost: Make sure imported/exported BOs are never purged

[Boris Brezillon]

On Fri, 29 Nov 2019 14:45:41 +0000
Steven Price <steven.price@arm.com> wrote:

> On 29/11/2019 13:59, Boris Brezillon wrote:
> > We don't want imported/exported BOs to be purges, as those are shared  
> 
> s/purges/purged/
> 
> > with other processes that might still use them. We should also refuse
> > to export a BO if it's been marked purgeable or has already been purged.
> > 
> > Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_drv.c | 19 ++++++++++++++++-
> >  drivers/gpu/drm/panfrost/panfrost_gem.c | 27 +++++++++++++++++++++++++
> >  2 files changed, 45 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index 1c67ac434e10..751df975534f 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -343,6 +343,7 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> >  	struct drm_panfrost_madvise *args = data;
> >  	struct panfrost_device *pfdev = dev->dev_private;
> >  	struct drm_gem_object *gem_obj;
> > +	int ret;
> >  
> >  	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
> >  	if (!gem_obj) {
> > @@ -350,6 +351,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> >  		return -ENOENT;
> >  	}
> >  
> > +	/*
> > +	 * We don't want to mark exported/imported BOs as purgeable: we're not
> > +	 * the only owner in that case.
> > +	 */
> > +	mutex_lock(&dev->object_name_lock);
> > +	if (gem_obj->dma_buf)
> > +		ret = -EINVAL;
> > +	else
> > +		ret = 0;
> > +
> > +	if (ret)
> > +		goto out_unlock_object_name;
> > +  
> 
> This might be better by setting ret = 0 at the beginning of the
> function. This can then be re-written as the more normal:
> 
> 	if (gem_obj->dma_buf) {
> 		ret = -EINVAL;
> 		goto out_unlock_object_name;
> 	]

Agreed.

> 
> >  	mutex_lock(&pfdev->shrinker_lock);
> >  	args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);
> >  
> > @@ -364,8 +378,11 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> >  	}
> >  	mutex_unlock(&pfdev->shrinker_lock);
> >  
> > +out_unlock_object_name:
> > +	mutex_unlock(&dev->object_name_lock);
> > +
> >  	drm_gem_object_put_unlocked(gem_obj);
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  int panfrost_unstable_ioctl_check(void)
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > index 92a95210a899..31d6417dd21c 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > @@ -99,6 +99,32 @@ void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv)
> >  	spin_unlock(&priv->mm_lock);
> >  }
> >  
> > +static struct dma_buf *
> > +panfrost_gem_export(struct drm_gem_object *obj, int flags)
> > +{
> > +	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
> > +	int ret;
> > +
> > +	/*
> > +	 * We must make sure the BO has not been marked purgeable/purged before
> > +	 * adding the mapping.
> > +	 * Note that we don't need to protect this test with a lock because
> > +	 * &drm_gem_object_funcs.export() is called with
> > +	 * &drm_device.object_lock held, and panfrost_ioctl_madvise() takes
> > +	 * this lock before calling drm_gem_shmem_madvise() (the function that
> > +	 * modifies bo->base.madv).
> > +	 */
> > +	if (bo->base.madv == PANFROST_MADV_WILLNEED)
> > +		ret = -EINVAL;
> > +	else
> > +		ret = 0;
> > +
> > +	if (ret)
> > +		return ERR_PTR(ret);  
> 
> No need for ret here:
> 
> 	if (bo->base.madv == PANFROST_MADV_WILLNEED)
> 		return ERR_PTR(-EINVAL);

This if/else section was previously surrounded by a lock/unlock, hence
the convoluted logic. I'll rework as suggested.

Thanks.

Re: [PATCH 7/8] drm/panfrost: Add the panfrost_gem_mapping concept

[Steven Price]

On 29/11/2019 13:59, Boris Brezillon wrote:
> With the introduction of per-FD address space, the same BO can be mapped
> in different address space if the BO is globally visible (GEM_FLINK)
> and opened in different context. The current implementation does not
> take case into account, and attaches the mapping directly to the
> panfrost_gem_object.
> 
> Let's create a panfrost_gem_mapping struct and allow multiple mappings
> per BO.
> 
> The mappings are refcounted, which helps solve another problem where
> mappings were teared down (GEM handle closed by userspace) while GPU
> jobs accessing those BOs were still in-flight. Jobs now keep a
> reference on the mappings they use.
> 
> Fixes: a5efb4c9a562 ("drm/panfrost: Restructure the GEM object creation")
> Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Looks good, some minor comments inline.

> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c       |  92 +++++++++++-
>  drivers/gpu/drm/panfrost/panfrost_gem.c       | 140 +++++++++++++++---
>  drivers/gpu/drm/panfrost/panfrost_gem.h       |  41 ++++-
>  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |   4 +-
>  drivers/gpu/drm/panfrost/panfrost_job.c       |  13 +-
>  drivers/gpu/drm/panfrost/panfrost_job.h       |   1 +
>  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  61 ++++----
>  drivers/gpu/drm/panfrost/panfrost_mmu.h       |   6 +-
>  drivers/gpu/drm/panfrost/panfrost_perfcnt.c   |  34 +++--
>  9 files changed, 318 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 751df975534f..b406b5243b40 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -78,8 +78,10 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
>  static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
>  		struct drm_file *file)
>  {
> +	struct panfrost_file_priv *priv = file->driver_priv;
>  	struct panfrost_gem_object *bo;
>  	struct drm_panfrost_create_bo *args = data;
> +	struct panfrost_gem_mapping *mapping;
>  
>  	if (!args->size || args->pad ||
>  	    (args->flags & ~(PANFROST_BO_NOEXEC | PANFROST_BO_HEAP)))
> @@ -95,7 +97,14 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
>  	if (IS_ERR(bo))
>  		return PTR_ERR(bo);
>  
> -	args->offset = bo->node.start << PAGE_SHIFT;
> +	mapping = panfrost_gem_mapping_get(bo, priv);
> +	if (!mapping) {
> +		drm_gem_object_put_unlocked(&bo->base.base);
> +		return -EINVAL;
> +	}
> +
> +	args->offset = mapping->mmnode.start << PAGE_SHIFT;
> +	panfrost_gem_mapping_put(mapping);
>  
>  	return 0;
>  }
> @@ -119,6 +128,11 @@ panfrost_lookup_bos(struct drm_device *dev,
>  		  struct drm_panfrost_submit *args,
>  		  struct panfrost_job *job)
>  {
> +	struct panfrost_file_priv *priv = file_priv->driver_priv;
> +	struct panfrost_gem_object *bo;
> +	unsigned int i;
> +	int ret;
> +
>  	job->bo_count = args->bo_handle_count;
>  
>  	if (!job->bo_count)
> @@ -130,9 +144,32 @@ panfrost_lookup_bos(struct drm_device *dev,
>  	if (!job->implicit_fences)
>  		return -ENOMEM;
>  
> -	return drm_gem_objects_lookup(file_priv,
> -				      (void __user *)(uintptr_t)args->bo_handles,
> -				      job->bo_count, &job->bos);
> +	ret = drm_gem_objects_lookup(file_priv,
> +				     (void __user *)(uintptr_t)args->bo_handles,
> +				     job->bo_count, &job->bos);
> +	if (ret)
> +		return ret;
> +
> +	job->mappings = kvmalloc_array(job->bo_count,
> +				       sizeof(struct panfrost_gem_mapping *),
> +				       GFP_KERNEL | __GFP_ZERO);
> +	if (!job->mappings)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < job->bo_count; i++) {
> +		struct panfrost_gem_mapping *mapping;
> +
> +		bo = to_panfrost_bo(job->bos[i]);
> +		mapping = panfrost_gem_mapping_get(bo, priv);
> +		if (!mapping) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		job->mappings[i] = mapping;
> +	}
> +
> +	return ret;
>  }
>  
>  /**
> @@ -320,7 +357,9 @@ static int panfrost_ioctl_mmap_bo(struct drm_device *dev, void *data,
>  static int panfrost_ioctl_get_bo_offset(struct drm_device *dev, void *data,
>  			    struct drm_file *file_priv)
>  {
> +	struct panfrost_file_priv *priv = file_priv->driver_priv;
>  	struct drm_panfrost_get_bo_offset *args = data;
> +	struct panfrost_gem_mapping *mapping;
>  	struct drm_gem_object *gem_obj;
>  	struct panfrost_gem_object *bo;
>  
> @@ -331,18 +370,25 @@ static int panfrost_ioctl_get_bo_offset(struct drm_device *dev, void *data,
>  	}
>  	bo = to_panfrost_bo(gem_obj);
>  
> -	args->offset = bo->node.start << PAGE_SHIFT;
> -
> +	mapping = panfrost_gem_mapping_get(bo, priv);
>  	drm_gem_object_put_unlocked(gem_obj);
> +
> +	if (!mapping)
> +		return -EINVAL;
> +
> +	args->offset = mapping->mmnode.start << PAGE_SHIFT;
> +	panfrost_gem_mapping_put(mapping);
>  	return 0;
>  }
>  
>  static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  				  struct drm_file *file_priv)
>  {
> +	struct panfrost_file_priv *priv = file_priv->driver_priv;
>  	struct drm_panfrost_madvise *args = data;
>  	struct panfrost_device *pfdev = dev->dev_private;
>  	struct drm_gem_object *gem_obj;
> +	struct panfrost_gem_object *bo;
>  	int ret;
>  
>  	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
> @@ -364,18 +410,48 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  	if (ret)
>  		goto out_unlock_object_name;
>  
> +	bo = to_panfrost_bo(gem_obj);
> +
>  	mutex_lock(&pfdev->shrinker_lock);
> +	mutex_lock(&bo->mappings.lock);
> +	if (args->madv == PANFROST_MADV_DONTNEED) {
> +		struct panfrost_gem_mapping *first, *last;
> +
> +		first = list_first_entry(&bo->mappings.list,
> +					 struct panfrost_gem_mapping,
> +					 node);
> +		last = list_last_entry(&bo->mappings.list,
> +				       struct panfrost_gem_mapping,
> +				       node);
> +
> +		/*
> +		 * If we want to mark the BO purgeable, there must be only one
> +		 * user: the caller FD.
> +		 * We could do something smarter and mark the BO purgeable only
> +		 * when all its users have marked it purgeable, but globally
> +		 * visible/shared BOs are likely to never be marked purgeable
> +		 * anyway, so let's not bother.
> +		 */
> +		if (first != last || WARN_ON_ONCE(first->mmu != &priv->mmu)) {
> +			ret = -EINVAL;
> +			goto out_unlock_mappings;
> +		}
> +	}
> +
>  	args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);
>  
>  	if (args->retained) {
> -		struct panfrost_gem_object *bo = to_panfrost_bo(gem_obj);
> -
>  		if (args->madv == PANFROST_MADV_DONTNEED)
>  			list_add_tail(&bo->base.madv_list,
>  				      &pfdev->shrinker_list);
>  		else if (args->madv == PANFROST_MADV_WILLNEED)
>  			list_del_init(&bo->base.madv_list);
>  	}
> +
> +	ret = 0;

NIT: This is unnecessary - ret will already be 0.

> +
> +out_unlock_mappings:
> +	mutex_unlock(&bo->mappings.lock);
>  	mutex_unlock(&pfdev->shrinker_lock);
>  
>  out_unlock_object_name:
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 31d6417dd21c..5a2c463c45d3 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -29,6 +29,12 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
>  	list_del_init(&bo->base.madv_list);
>  	mutex_unlock(&pfdev->shrinker_lock);
>  
> +	/*
> +	 * If we still have mappings attached to the BO, there's a problem in
> +	 * our refcounting.
> +	 */
> +	WARN_ON_ONCE(!list_empty(&bo->mappings.list));
> +
>  	if (bo->sgts) {
>  		int i;
>  		int n_sgt = bo->base.base.size / SZ_2M;
> @@ -46,6 +52,70 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
>  	drm_gem_shmem_free_object(obj);
>  }
>  
> +struct panfrost_gem_mapping *
> +panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
> +			 struct panfrost_file_priv *priv)
> +{
> +	struct panfrost_gem_mapping *mapping = NULL, *iter;
> +
> +	mutex_lock(&bo->mappings.lock);
> +	list_for_each_entry(iter, &bo->mappings.list, node) {
> +		if (iter->mmu == &priv->mmu) {
> +			kref_get(&iter->refcount);
> +			mapping = iter;

Add a "break" here, or a WARN_ON(mapping) if you don't trust that
there's only ever one mapping matching the mmu... ;)

> +		}
> +	}
> +	mutex_unlock(&bo->mappings.lock);
> +
> +	return mapping;
> +}
> +
> +static void
> +panfrost_gem_teardown_mapping(struct panfrost_gem_mapping *mapping)
> +{
> +	struct panfrost_file_priv *priv;
> +
> +	if (mapping->active)
> +		panfrost_mmu_unmap(mapping);
> +
> +	priv = container_of(mapping->mmu, struct panfrost_file_priv, mmu);
> +	spin_lock(&priv->mm_lock);
> +	if (drm_mm_node_allocated(&mapping->mmnode))
> +		drm_mm_remove_node(&mapping->mmnode);
> +	spin_unlock(&priv->mm_lock);
> +}
> +
> +static void panfrost_gem_mapping_release(struct kref *kref)
> +{
> +	struct panfrost_gem_mapping *mapping;
> +	struct panfrost_file_priv *priv;
> +
> +	mapping = container_of(kref, struct panfrost_gem_mapping, refcount);
> +	priv = container_of(mapping->mmu, struct panfrost_file_priv, mmu);

'priv' isn't used

> +
> +	panfrost_gem_teardown_mapping(mapping);
> +	drm_gem_object_put_unlocked(&mapping->obj->base.base);
> +	kfree(mapping);
> +}
> +
> +void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping)
> +{
> +	if (!mapping)
> +		return;
> +
> +	kref_put(&mapping->refcount, panfrost_gem_mapping_release);
> +}
> +
> +void panfrost_gem_teardown_mappings(struct panfrost_gem_object *bo)
> +{
> +	struct panfrost_gem_mapping *mapping;
> +
> +	mutex_lock(&bo->mappings.lock);
> +	list_for_each_entry(mapping, &bo->mappings.list, node)
> +		panfrost_gem_teardown_mapping(mapping);
> +	mutex_unlock(&bo->mappings.lock);
> +}
> +
>  int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)
>  {
>  	int ret;
> @@ -54,6 +124,16 @@ int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)
>  	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
>  	unsigned long color = bo->noexec ? PANFROST_BO_NOEXEC : 0;
>  	struct panfrost_file_priv *priv = file_priv->driver_priv;
> +	struct panfrost_gem_mapping *mapping;
> +
> +	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
> +	if (!mapping)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&mapping->node);
> +	kref_init(&mapping->refcount);
> +	drm_gem_object_get(obj);
> +	mapping->obj = bo;
>  
>  	/*
>  	 * Executable buffers cannot cross a 16MB boundary as the program
> @@ -66,37 +146,61 @@ int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)
>  	else
>  		align = size >= SZ_2M ? SZ_2M >> PAGE_SHIFT : 0;
>  
> -	bo->mmu = &priv->mmu;
> +	mapping->mmu = &priv->mmu;
>  	spin_lock(&priv->mm_lock);
> -	ret = drm_mm_insert_node_generic(&priv->mm, &bo->node,
> +	ret = drm_mm_insert_node_generic(&priv->mm, &mapping->mmnode,
>  					 size >> PAGE_SHIFT, align, color, 0);
>  	spin_unlock(&priv->mm_lock);
>  	if (ret)
> -		return ret;
> +		goto err;
>  
>  	if (!bo->is_heap) {
> -		ret = panfrost_mmu_map(bo);
> -		if (ret) {
> -			spin_lock(&priv->mm_lock);
> -			drm_mm_remove_node(&bo->node);
> -			spin_unlock(&priv->mm_lock);
> -		}
> +		ret = panfrost_mmu_map(mapping);
> +		if (ret)
> +			goto err;
>  	}
> +
> +	mutex_lock(&obj->dev->object_name_lock);
> +	/*
> +	 * We must make sure the BO has not been marked purgeable before
> +	 * adding the mapping.
> +	 */
> +	if (bo->base.madv == PANFROST_MADV_WILLNEED) {
> +		mutex_lock(&bo->mappings.lock);
> +		list_add_tail(&mapping->node, &bo->mappings.list);
> +		mutex_unlock(&bo->mappings.lock);
> +	} else {
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&obj->dev->object_name_lock);
> +
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	panfrost_gem_mapping_put(mapping);
>  	return ret;

That can be simplified to:

err:
	if (ret)
		panfrost_gem_mapping_put(mapping);
	return ret;

>  }
>  
>  void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv)
>  {
> -	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
>  	struct panfrost_file_priv *priv = file_priv->driver_priv;
> +	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
> +	struct panfrost_gem_mapping *mapping = NULL, *iter;
> +
> +	mutex_lock(&bo->mappings.lock);
> +	list_for_each_entry(iter, &bo->mappings.list, node) {
> +		if (iter->mmu == &priv->mmu) {
> +			mapping = iter;
> +			list_del(&iter->node);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&bo->mappings.lock);
>  
> -	if (bo->is_mapped)
> -		panfrost_mmu_unmap(bo);
> -
> -	spin_lock(&priv->mm_lock);
> -	if (drm_mm_node_allocated(&bo->node))
> -		drm_mm_remove_node(&bo->node);
> -	spin_unlock(&priv->mm_lock);
> +	panfrost_gem_mapping_put(mapping);
>  }
>  
>  static struct dma_buf *
> @@ -163,6 +267,8 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t
>  	if (!obj)
>  		return NULL;
>  
> +	INIT_LIST_HEAD(&obj->mappings.list);
> +	mutex_init(&obj->mappings.lock);
>  	obj->base.base.funcs = &panfrost_gem_funcs;
>  
>  	return &obj->base.base;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index 4b17e7308764..ca1bc9019600 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -13,23 +13,46 @@ struct panfrost_gem_object {
>  	struct drm_gem_shmem_object base;
>  	struct sg_table *sgts;
>  
> -	struct panfrost_mmu *mmu;
> -	struct drm_mm_node node;
> -	bool is_mapped		:1;
> +	/*
> +	 * Use a list for now. If searching a mapping ever becomes the
> +	 * bottleneck, we should consider using an RB-tree, or even better,
> +	 * let the core store drm_gem_object_mapping entries (where we
> +	 * could place driver specific data) instead of drm_gem_object ones
> +	 * in its drm_file->object_idr table.
> +	 *
> +	 * struct drm_gem_object_mapping {
> +	 *	struct drm_gem_object *obj;
> +	 *	void *driver_priv;
> +	 * };
> +	 */
> +	struct {
> +		struct list_head list;
> +		struct mutex lock;
> +	} mappings;
> +
>  	bool noexec		:1;
>  	bool is_heap		:1;
>  };
>  
> +struct panfrost_gem_mapping {
> +	struct list_head node;
> +	struct kref refcount;
> +	struct panfrost_gem_object *obj;
> +	struct drm_mm_node mmnode;
> +	struct panfrost_mmu *mmu;
> +	bool active		:1;
> +};
> +
>  static inline
>  struct  panfrost_gem_object *to_panfrost_bo(struct drm_gem_object *obj)
>  {
>  	return container_of(to_drm_gem_shmem_obj(obj), struct panfrost_gem_object, base);
>  }
>  
> -static inline
> -struct  panfrost_gem_object *drm_mm_node_to_panfrost_bo(struct drm_mm_node *node)
> +static inline struct panfrost_gem_mapping *
> +drm_mm_node_to_panfrost_mapping(struct drm_mm_node *node)
>  {
> -	return container_of(node, struct panfrost_gem_object, node);
> +	return container_of(node, struct panfrost_gem_mapping, mmnode);
>  }
>  
>  struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t size);
> @@ -49,6 +72,12 @@ int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv);
>  void panfrost_gem_close(struct drm_gem_object *obj,
>  			struct drm_file *file_priv);
>  
> +struct panfrost_gem_mapping *
> +panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
> +			 struct panfrost_file_priv *priv);
> +void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping);
> +void panfrost_gem_teardown_mappings(struct panfrost_gem_object *bo);
> +
>  void panfrost_gem_shrinker_init(struct drm_device *dev);
>  void panfrost_gem_shrinker_cleanup(struct drm_device *dev);
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> index 458f0fa68111..b36df326c860 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> @@ -39,11 +39,13 @@ panfrost_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc
>  static bool panfrost_gem_purge(struct drm_gem_object *obj)
>  {
>  	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> +	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
> +

NIT: Extra blank line

Steve

>  
>  	if (!mutex_trylock(&shmem->pages_lock))
>  		return false;
>  
> -	panfrost_mmu_unmap(to_panfrost_bo(obj));
> +	panfrost_gem_teardown_mappings(bo);
>  	drm_gem_shmem_purge_locked(obj);
>  
>  	mutex_unlock(&shmem->pages_lock);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index cdd9448fbbdd..c85d45be3b5e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -269,9 +269,20 @@ static void panfrost_job_cleanup(struct kref *ref)
>  	dma_fence_put(job->done_fence);
>  	dma_fence_put(job->render_done_fence);
>  
> -	if (job->bos) {
> +	if (job->mappings) {
>  		for (i = 0; i < job->bo_count; i++)
> +			panfrost_gem_mapping_put(job->mappings[i]);
> +		kvfree(job->mappings);
> +	}
> +
> +	if (job->bos) {
> +		struct panfrost_gem_object *bo;
> +
> +		for (i = 0; i < job->bo_count; i++) {
> +			bo = to_panfrost_bo(job->bos[i]);
>  			drm_gem_object_put_unlocked(job->bos[i]);
> +		}
> +
>  		kvfree(job->bos);
>  	}
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
> index 62454128a792..bbd3ba97ff67 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.h
> @@ -32,6 +32,7 @@ struct panfrost_job {
>  
>  	/* Exclusive fences we have taken from the BOs to wait for */
>  	struct dma_fence **implicit_fences;
> +	struct panfrost_gem_mapping **mappings;
>  	struct drm_gem_object **bos;
>  	u32 bo_count;
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index a3ed64a1f15e..763cfca886a7 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -269,14 +269,15 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
>  	return 0;
>  }
>  
> -int panfrost_mmu_map(struct panfrost_gem_object *bo)
> +int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
>  {
> +	struct panfrost_gem_object *bo = mapping->obj;
>  	struct drm_gem_object *obj = &bo->base.base;
>  	struct panfrost_device *pfdev = to_panfrost_device(obj->dev);
>  	struct sg_table *sgt;
>  	int prot = IOMMU_READ | IOMMU_WRITE;
>  
> -	if (WARN_ON(bo->is_mapped))
> +	if (WARN_ON(mapping->active))
>  		return 0;
>  
>  	if (bo->noexec)
> @@ -286,25 +287,28 @@ int panfrost_mmu_map(struct panfrost_gem_object *bo)
>  	if (WARN_ON(IS_ERR(sgt)))
>  		return PTR_ERR(sgt);
>  
> -	mmu_map_sg(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT, prot, sgt);
> -	bo->is_mapped = true;
> +	mmu_map_sg(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT,
> +		   prot, sgt);
> +	mapping->active = true;
>  
>  	return 0;
>  }
>  
> -void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
> +void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping)
>  {
> +	struct panfrost_gem_object *bo = mapping->obj;
>  	struct drm_gem_object *obj = &bo->base.base;
>  	struct panfrost_device *pfdev = to_panfrost_device(obj->dev);
> -	struct io_pgtable_ops *ops = bo->mmu->pgtbl_ops;
> -	u64 iova = bo->node.start << PAGE_SHIFT;
> -	size_t len = bo->node.size << PAGE_SHIFT;
> +	struct io_pgtable_ops *ops = mapping->mmu->pgtbl_ops;
> +	u64 iova = mapping->mmnode.start << PAGE_SHIFT;
> +	size_t len = mapping->mmnode.size << PAGE_SHIFT;
>  	size_t unmapped_len = 0;
>  
> -	if (WARN_ON(!bo->is_mapped))
> +	if (WARN_ON(!mapping->active))
>  		return;
>  
> -	dev_dbg(pfdev->dev, "unmap: as=%d, iova=%llx, len=%zx", bo->mmu->as, iova, len);
> +	dev_dbg(pfdev->dev, "unmap: as=%d, iova=%llx, len=%zx",
> +		mapping->mmu->as, iova, len);
>  
>  	while (unmapped_len < len) {
>  		size_t unmapped_page;
> @@ -318,8 +322,9 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>  		unmapped_len += pgsize;
>  	}
>  
> -	panfrost_mmu_flush_range(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT, len);
> -	bo->is_mapped = false;
> +	panfrost_mmu_flush_range(pfdev, mapping->mmu,
> +				 mapping->mmnode.start << PAGE_SHIFT, len);
> +	mapping->active = false;
>  }
>  
>  static void mmu_tlb_inv_context_s1(void *cookie)
> @@ -394,10 +399,10 @@ void panfrost_mmu_pgtable_free(struct panfrost_file_priv *priv)
>  	free_io_pgtable_ops(mmu->pgtbl_ops);
>  }
>  
> -static struct panfrost_gem_object *
> -addr_to_drm_mm_node(struct panfrost_device *pfdev, int as, u64 addr)
> +static struct panfrost_gem_mapping *
> +addr_to_mapping(struct panfrost_device *pfdev, int as, u64 addr)
>  {
> -	struct panfrost_gem_object *bo = NULL;
> +	struct panfrost_gem_mapping *mapping = NULL;
>  	struct panfrost_file_priv *priv;
>  	struct drm_mm_node *node;
>  	u64 offset = addr >> PAGE_SHIFT;
> @@ -418,8 +423,9 @@ addr_to_drm_mm_node(struct panfrost_device *pfdev, int as, u64 addr)
>  	drm_mm_for_each_node(node, &priv->mm) {
>  		if (offset >= node->start &&
>  		    offset < (node->start + node->size)) {
> -			bo = drm_mm_node_to_panfrost_bo(node);
> -			drm_gem_object_get(&bo->base.base);
> +			mapping = drm_mm_node_to_panfrost_mapping(node);
> +
> +			kref_get(&mapping->refcount);
>  			break;
>  		}
>  	}
> @@ -427,7 +433,7 @@ addr_to_drm_mm_node(struct panfrost_device *pfdev, int as, u64 addr)
>  	spin_unlock(&priv->mm_lock);
>  out:
>  	spin_unlock(&pfdev->as_lock);
> -	return bo;
> +	return mapping;
>  }
>  
>  #define NUM_FAULT_PAGES (SZ_2M / PAGE_SIZE)
> @@ -436,28 +442,30 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>  				       u64 addr)
>  {
>  	int ret, i;
> +	struct panfrost_gem_mapping *bomapping;
>  	struct panfrost_gem_object *bo;
>  	struct address_space *mapping;
>  	pgoff_t page_offset;
>  	struct sg_table *sgt;
>  	struct page **pages;
>  
> -	bo = addr_to_drm_mm_node(pfdev, as, addr);
> -	if (!bo)
> +	bomapping = addr_to_mapping(pfdev, as, addr);
> +	if (!bomapping)
>  		return -ENOENT;
>  
> +	bo = bomapping->obj;
>  	if (!bo->is_heap) {
>  		dev_WARN(pfdev->dev, "matching BO is not heap type (GPU VA = %llx)",
> -			 bo->node.start << PAGE_SHIFT);
> +			 bomapping->mmnode.start << PAGE_SHIFT);
>  		ret = -EINVAL;
>  		goto err_bo;
>  	}
> -	WARN_ON(bo->mmu->as != as);
> +	WARN_ON(bomapping->mmu->as != as);
>  
>  	/* Assume 2MB alignment and size multiple */
>  	addr &= ~((u64)SZ_2M - 1);
>  	page_offset = addr >> PAGE_SHIFT;
> -	page_offset -= bo->node.start;
> +	page_offset -= bomapping->mmnode.start;
>  
>  	mutex_lock(&bo->base.pages_lock);
>  
> @@ -509,13 +517,14 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>  		goto err_map;
>  	}
>  
> -	mmu_map_sg(pfdev, bo->mmu, addr, IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
> +	mmu_map_sg(pfdev, bomapping->mmu, addr,
> +		   IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
>  
> -	bo->is_mapped = true;
> +	bomapping->active = true;
>  
>  	dev_dbg(pfdev->dev, "mapped page fault @ AS%d %llx", as, addr);
>  
> -	drm_gem_object_put_unlocked(&bo->base.base);
> +	panfrost_gem_mapping_put(bomapping);
>  
>  	return 0;
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.h b/drivers/gpu/drm/panfrost/panfrost_mmu.h
> index 7c5b6775ae23..44fc2edf63ce 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.h
> @@ -4,12 +4,12 @@
>  #ifndef __PANFROST_MMU_H__
>  #define __PANFROST_MMU_H__
>  
> -struct panfrost_gem_object;
> +struct panfrost_gem_mapping;
>  struct panfrost_file_priv;
>  struct panfrost_mmu;
>  
> -int panfrost_mmu_map(struct panfrost_gem_object *bo);
> -void panfrost_mmu_unmap(struct panfrost_gem_object *bo);
> +int panfrost_mmu_map(struct panfrost_gem_mapping *mapping);
> +void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping);
>  
>  int panfrost_mmu_init(struct panfrost_device *pfdev);
>  void panfrost_mmu_fini(struct panfrost_device *pfdev);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index 2c04e858c50a..684820448be3 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> @@ -25,7 +25,7 @@
>  #define V4_SHADERS_PER_COREGROUP	4
>  
>  struct panfrost_perfcnt {
> -	struct panfrost_gem_object *bo;
> +	struct panfrost_gem_mapping *mapping;
>  	size_t bosize;
>  	void *buf;
>  	struct panfrost_file_priv *user;
> @@ -49,7 +49,7 @@ static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev)
>  	int ret;
>  
>  	reinit_completion(&pfdev->perfcnt->dump_comp);
> -	gpuva = pfdev->perfcnt->bo->node.start << PAGE_SHIFT;
> +	gpuva = pfdev->perfcnt->mapping->mmnode.start << PAGE_SHIFT;
>  	gpu_write(pfdev, GPU_PERFCNT_BASE_LO, gpuva);
>  	gpu_write(pfdev, GPU_PERFCNT_BASE_HI, gpuva >> 32);
>  	gpu_write(pfdev, GPU_INT_CLEAR,
> @@ -89,17 +89,22 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  	if (IS_ERR(bo))
>  		return PTR_ERR(bo);
>  
> -	perfcnt->bo = to_panfrost_bo(&bo->base);
> -
>  	/* Map the perfcnt buf in the address space attached to file_priv. */
> -	ret = panfrost_gem_open(&perfcnt->bo->base.base, file_priv);
> +	ret = panfrost_gem_open(&bo->base, file_priv);
>  	if (ret)
>  		goto err_put_bo;
>  
> +	perfcnt->mapping = panfrost_gem_mapping_get(to_panfrost_bo(&bo->base),
> +						    user);
> +	if (!perfcnt->mapping) {
> +		ret = -EINVAL;
> +		goto err_close_bo;
> +	}
> +
>  	perfcnt->buf = drm_gem_shmem_vmap(&bo->base);
>  	if (IS_ERR(perfcnt->buf)) {
>  		ret = PTR_ERR(perfcnt->buf);
> -		goto err_close_bo;
> +		goto err_put_mapping;
>  	}
>  
>  	/*
> @@ -154,12 +159,17 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
>  		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
>  
> +	/* The BO ref is retained by the mapping. */
> +	drm_gem_object_put_unlocked(&bo->base);
> +
>  	return 0;
>  
>  err_vunmap:
> -	drm_gem_shmem_vunmap(&perfcnt->bo->base.base, perfcnt->buf);
> +	drm_gem_shmem_vunmap(&bo->base, perfcnt->buf);
> +err_put_mapping:
> +	panfrost_gem_mapping_put(perfcnt->mapping);
>  err_close_bo:
> -	panfrost_gem_close(&perfcnt->bo->base.base, file_priv);
> +	panfrost_gem_close(&bo->base, file_priv);
>  err_put_bo:
>  	drm_gem_object_put_unlocked(&bo->base);
>  	return ret;
> @@ -182,11 +192,11 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
>  		  GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF));
>  
>  	perfcnt->user = NULL;
> -	drm_gem_shmem_vunmap(&perfcnt->bo->base.base, perfcnt->buf);
> +	drm_gem_shmem_vunmap(&perfcnt->mapping->obj->base.base, perfcnt->buf);
>  	perfcnt->buf = NULL;
> -	panfrost_gem_close(&perfcnt->bo->base.base, file_priv);
> -	drm_gem_object_put_unlocked(&perfcnt->bo->base.base);
> -	perfcnt->bo = NULL;
> +	panfrost_gem_close(&perfcnt->mapping->obj->base.base, file_priv);
> +	panfrost_gem_mapping_put(perfcnt->mapping);
> +	perfcnt->mapping = NULL;
>  	pm_runtime_mark_last_busy(pfdev->dev);
>  	pm_runtime_put_autosuspend(pfdev->dev);
>  
> 

Re: [PATCH 8/8] drm/panfrost: Make sure the shrinker does not reclaim referenced BOs

[Steven Price]

On 29/11/2019 13:59, Boris Brezillon wrote:
> Userspace might tag a BO purgeable while it's still referenced by GPU
> jobs. We need to make sure the shrinker does not purge such BOs until
> all jobs referencing it are finished.
> 
> Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

I'm happy with this, but I would also argue that it was previously user
space's job not to mark a BO purgeable while it's in use by the GPU.
This is in some ways an ABI change so we should be sure this is
something that we want to support "forever" more. But if Mesa has
'always' been assuming this behaviour we might as well match.

Reviewed-by: Steven Price <steven.price@arm.com>

Steve

> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c          | 1 +
>  drivers/gpu/drm/panfrost/panfrost_gem.h          | 6 ++++++
>  drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c | 2 ++
>  drivers/gpu/drm/panfrost/panfrost_job.c          | 7 ++++++-
>  4 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index b406b5243b40..297c0e7304d2 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -166,6 +166,7 @@ panfrost_lookup_bos(struct drm_device *dev,
>  			break;
>  		}
>  
> +		atomic_inc(&bo->gpu_usecount);
>  		job->mappings[i] = mapping;
>  	}
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index ca1bc9019600..b3517ff9630c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -30,6 +30,12 @@ struct panfrost_gem_object {
>  		struct mutex lock;
>  	} mappings;
>  
> +	/*
> +	 * Count the number of jobs referencing this BO so we don't let the
> +	 * shrinker reclaim this object prematurely.
> +	 */
> +	atomic_t gpu_usecount;
> +
>  	bool noexec		:1;
>  	bool is_heap		:1;
>  };
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> index b36df326c860..288e46c40673 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> @@ -41,6 +41,8 @@ static bool panfrost_gem_purge(struct drm_gem_object *obj)
>  	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>  	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
>  
> +	if (atomic_read(&bo->gpu_usecount))
> +		return false;
>  
>  	if (!mutex_trylock(&shmem->pages_lock))
>  		return false;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index c85d45be3b5e..2b12aa87ff32 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -270,8 +270,13 @@ static void panfrost_job_cleanup(struct kref *ref)
>  	dma_fence_put(job->render_done_fence);
>  
>  	if (job->mappings) {
> -		for (i = 0; i < job->bo_count; i++)
> +		for (i = 0; i < job->bo_count; i++) {
> +			if (!job->mappings[i])
> +				break;
> +
> +			atomic_dec(&job->mappings[i]->obj->gpu_usecount);
>  			panfrost_gem_mapping_put(job->mappings[i]);
> +		}
>  		kvfree(job->mappings);
>  	}
>  
> 

Re: [PATCH 8/8] drm/panfrost: Make sure the shrinker does not reclaim referenced BOs

[Boris Brezillon]

On Fri, 29 Nov 2019 15:48:15 +0000
Steven Price <steven.price@arm.com> wrote:

> On 29/11/2019 13:59, Boris Brezillon wrote:
> > Userspace might tag a BO purgeable while it's still referenced by GPU
> > jobs. We need to make sure the shrinker does not purge such BOs until
> > all jobs referencing it are finished.
> > 
> > Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> 
> I'm happy with this, but I would also argue that it was previously user
> space's job not to mark a BO purgeable while it's in use by the GPU.

I was not aware of this design choice.

> This is in some ways an ABI change so we should be sure this is
> something that we want to support "forever" more.

Well, in that case, the ABI change is bringing extra robustness, with
AFAICT, no downside for those that were taking care of that in
userspace.

> But if Mesa has
> 'always' been assuming this behaviour we might as well match.

I think so, and VC4 seems to have the same expectations.

> 
> Reviewed-by: Steven Price <steven.price@arm.com>

Thanks for the review.

Re: [PATCH 8/8] drm/panfrost: Make sure the shrinker does not reclaim referenced BOs

[Steven Price]

On 29/11/2019 16:07, Boris Brezillon wrote:
> On Fri, 29 Nov 2019 15:48:15 +0000
> Steven Price <steven.price@arm.com> wrote:
> 
>> On 29/11/2019 13:59, Boris Brezillon wrote:
>>> Userspace might tag a BO purgeable while it's still referenced by GPU
>>> jobs. We need to make sure the shrinker does not purge such BOs until
>>> all jobs referencing it are finished.
>>>
>>> Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
>>
>> I'm happy with this, but I would also argue that it was previously user
>> space's job not to mark a BO purgeable while it's in use by the GPU.
> 
> I was not aware of this design choice.

When I was maintaining mali_kbase I would have said no to this ;) But
thankfully I've moved on! I'm not sure whether anyone actually made a
design choice for Panfrost here.

>> This is in some ways an ABI change so we should be sure this is
>> something that we want to support "forever" more.
> 
> Well, in that case, the ABI change is bringing extra robustness, with
> AFAICT, no downside for those that were taking care of that in
> userspace.

Yes, there's no downside for user space - this is just giving user space
extra freedom.

>> But if Mesa has
>> 'always' been assuming this behaviour we might as well match.
> 
> I think so, and VC4 seems to have the same expectations.

This seems like enough justification to me.

Steve

>>
>> Reviewed-by: Steven Price <steven.price@arm.com>
> 
> Thanks for the review.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 1/8] drm/panfrost: Make panfrost_job_run() return an ERR_PTR() instead of NULL

[Boris Brezillon]

On Fri, 29 Nov 2019 14:38:50 +0000
Steven Price <steven.price@arm.com> wrote:

> On 29/11/2019 14:31, Boris Brezillon wrote:
> > On Fri, 29 Nov 2019 14:19:50 +0000
> > Steven Price <steven.price@arm.com> wrote:
> >   
> >> On 29/11/2019 13:59, Boris Brezillon wrote:  
> >>> If we don't do that, dma_fence_set_error() complains (called from
> >>> drm_sched_main()).
> >>>
> >>> Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
> >>> Cc: <stable@vger.kernel.org>
> >>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>    
> >>
> >> This might be worth doing, but actually it's not Panfrost that is broken
> >> it's the callers, see [1] and [2]. So I don't think we want the
> >> Fixes/stable tag.  
> > 
> > Okay.
> >   
> >>
> >> [1] https://patchwork.kernel.org/patch/11218399/
> >> [2] https://patchwork.kernel.org/patch/11267073/
> >>  
> >>> ---
> >>>  drivers/gpu/drm/panfrost/panfrost_job.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> >>> index 21f34d44aac2..cdd9448fbbdd 100644
> >>> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> >>> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> >>> @@ -328,13 +328,13 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
> >>>  	struct dma_fence *fence = NULL;
> >>>  
> >>>  	if (unlikely(job->base.s_fence->finished.error))
> >>> -		return NULL;
> >>> +		return ERR_PTR(job->base.s_fence->finished.error);  
> > 
> > Hm, so we can keep the return NULL here if [1] is applied (the error
> > is preserved), but I'm not sure it's clearer that way.
> >   
> >>>  
> >>>  	pfdev->jobs[slot] = job;
> >>>  
> >>>  	fence = panfrost_fence_create(pfdev, slot);
> >>>  	if (IS_ERR(fence))
> >>> -		return NULL;
> >>> +		return ERR_PTR(-ENOMEM);    
> > 
> > This one should be fixed though, otherwise the error is never updated,
> > so I'm wondering if it doesn't deserve a Fixes tag in the end...  
> 
> Good point, although this can't be back-ported before [3] (well unless
> that commit is considered stable material too), so this is only really
> relevant for v5.4. But worth fixing there.

IIRc, such constraints can be specified with:

Cc: <stable@vger.kernel.org> # v5.4+

Anyway, I'll drop this patch from the series and repost a new version
once [1] has landed.

Thanks for the heads up.

Boris

Re: [PATCH 2/8] drm/panfrost: Fix a race in panfrost_ioctl_madvise()

[Daniel Vetter]

On Fri, Nov 29, 2019 at 02:40:34PM +0000, Steven Price wrote:
> On 29/11/2019 14:33, Boris Brezillon wrote:
> > On Fri, 29 Nov 2019 14:24:48 +0000
> > Steven Price <steven.price@arm.com> wrote:
> > 
> >> On 29/11/2019 13:59, Boris Brezillon wrote:
> >>> If 2 threads change the MADVISE property of the same BO in parallel we
> >>> might end up with an shmem->madv value that's inconsistent with the
> >>> presence of the BO in the shrinker list.  
> >>
> >> I'm a bit worried from the point of view of user space sanity that you
> >> observed this - but clearly the kernel should be robust!
> > 
> > It's not something I observed, just found the race by inspecting the
> > code, and I thought it was worth fixing it.
> 
> Good! ;) Your cover letter referring to a "debug session" made me think
> you'd actually hit all these.

Time for some igt to make sure this is actually correct?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 6/8] drm/panfrost: Make sure imported/exported BOs are never purged

[Daniel Vetter]

On Fri, Nov 29, 2019 at 02:59:06PM +0100, Boris Brezillon wrote:
> We don't want imported/exported BOs to be purges, as those are shared
> with other processes that might still use them. We should also refuse
> to export a BO if it's been marked purgeable or has already been purged.
> 
> Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 19 ++++++++++++++++-
>  drivers/gpu/drm/panfrost/panfrost_gem.c | 27 +++++++++++++++++++++++++
>  2 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 1c67ac434e10..751df975534f 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -343,6 +343,7 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  	struct drm_panfrost_madvise *args = data;
>  	struct panfrost_device *pfdev = dev->dev_private;
>  	struct drm_gem_object *gem_obj;
> +	int ret;
>  
>  	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
>  	if (!gem_obj) {
> @@ -350,6 +351,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  		return -ENOENT;
>  	}
>  
> +	/*
> +	 * We don't want to mark exported/imported BOs as purgeable: we're not
> +	 * the only owner in that case.
> +	 */
> +	mutex_lock(&dev->object_name_lock);

Kinda not awesome that you have to take this core lock here and encumber
core drm locking with random driver stuff.

Can't this be solved with your own locking only and some reasonable
ordering of checks? big locks because it's easy is endless long-term pain.

Also exporting purgeable objects is kinda a userspace bug, all you have to
do is not oops in dma_buf_attachment_map. No need to prevent the damage
here imo.
-Daniel

> +	if (gem_obj->dma_buf)
> +		ret = -EINVAL;
> +	else
> +		ret = 0;
> +
> +	if (ret)
> +		goto out_unlock_object_name;
> +
>  	mutex_lock(&pfdev->shrinker_lock);
>  	args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);
>  
> @@ -364,8 +378,11 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  	}
>  	mutex_unlock(&pfdev->shrinker_lock);
>  
> +out_unlock_object_name:
> +	mutex_unlock(&dev->object_name_lock);
> +
>  	drm_gem_object_put_unlocked(gem_obj);
> -	return 0;
> +	return ret;
>  }
>  
>  int panfrost_unstable_ioctl_check(void)
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 92a95210a899..31d6417dd21c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -99,6 +99,32 @@ void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv)
>  	spin_unlock(&priv->mm_lock);
>  }
>  
> +static struct dma_buf *
> +panfrost_gem_export(struct drm_gem_object *obj, int flags)
> +{
> +	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
> +	int ret;
> +
> +	/*
> +	 * We must make sure the BO has not been marked purgeable/purged before
> +	 * adding the mapping.
> +	 * Note that we don't need to protect this test with a lock because
> +	 * &drm_gem_object_funcs.export() is called with
> +	 * &drm_device.object_lock held, and panfrost_ioctl_madvise() takes
> +	 * this lock before calling drm_gem_shmem_madvise() (the function that
> +	 * modifies bo->base.madv).
> +	 */
> +	if (bo->base.madv == PANFROST_MADV_WILLNEED)
> +		ret = -EINVAL;
> +	else
> +		ret = 0;
> +
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return drm_gem_prime_export(obj, flags);
> +}
> +
>  static int panfrost_gem_pin(struct drm_gem_object *obj)
>  {
>  	if (to_panfrost_bo(obj)->is_heap)
> @@ -112,6 +138,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
>  	.open = panfrost_gem_open,
>  	.close = panfrost_gem_close,
>  	.print_info = drm_gem_shmem_print_info,
> +	.export = panfrost_gem_export,
>  	.pin = panfrost_gem_pin,
>  	.unpin = drm_gem_shmem_unpin,
>  	.get_sg_table = drm_gem_shmem_get_sg_table,
> -- 
> 2.23.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 7/8] drm/panfrost: Add the panfrost_gem_mapping concept

[Daniel Vetter]

On Fri, Nov 29, 2019 at 02:59:07PM +0100, Boris Brezillon wrote:
> With the introduction of per-FD address space, the same BO can be mapped
> in different address space if the BO is globally visible (GEM_FLINK)

Also dma-buf self-imports for wayland/dri3 ...

> and opened in different context. The current implementation does not
> take case into account, and attaches the mapping directly to the
> panfrost_gem_object.
> 
> Let's create a panfrost_gem_mapping struct and allow multiple mappings
> per BO.
> 
> The mappings are refcounted, which helps solve another problem where
> mappings were teared down (GEM handle closed by userspace) while GPU
> jobs accessing those BOs were still in-flight. Jobs now keep a
> reference on the mappings they use.

uh what.

tbh this sounds bad enough (as in how did a desktop on panfrost ever work)
that I think you really want a few igts to test this stuff.
-Daniel

> 
> Fixes: a5efb4c9a562 ("drm/panfrost: Restructure the GEM object creation")
> Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c       |  92 +++++++++++-
>  drivers/gpu/drm/panfrost/panfrost_gem.c       | 140 +++++++++++++++---
>  drivers/gpu/drm/panfrost/panfrost_gem.h       |  41 ++++-
>  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |   4 +-
>  drivers/gpu/drm/panfrost/panfrost_job.c       |  13 +-
>  drivers/gpu/drm/panfrost/panfrost_job.h       |   1 +
>  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  61 ++++----
>  drivers/gpu/drm/panfrost/panfrost_mmu.h       |   6 +-
>  drivers/gpu/drm/panfrost/panfrost_perfcnt.c   |  34 +++--
>  9 files changed, 318 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 751df975534f..b406b5243b40 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -78,8 +78,10 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
>  static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
>  		struct drm_file *file)
>  {
> +	struct panfrost_file_priv *priv = file->driver_priv;
>  	struct panfrost_gem_object *bo;
>  	struct drm_panfrost_create_bo *args = data;
> +	struct panfrost_gem_mapping *mapping;
>  
>  	if (!args->size || args->pad ||
>  	    (args->flags & ~(PANFROST_BO_NOEXEC | PANFROST_BO_HEAP)))
> @@ -95,7 +97,14 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
>  	if (IS_ERR(bo))
>  		return PTR_ERR(bo);
>  
> -	args->offset = bo->node.start << PAGE_SHIFT;
> +	mapping = panfrost_gem_mapping_get(bo, priv);
> +	if (!mapping) {
> +		drm_gem_object_put_unlocked(&bo->base.base);
> +		return -EINVAL;
> +	}
> +
> +	args->offset = mapping->mmnode.start << PAGE_SHIFT;
> +	panfrost_gem_mapping_put(mapping);
>  
>  	return 0;
>  }
> @@ -119,6 +128,11 @@ panfrost_lookup_bos(struct drm_device *dev,
>  		  struct drm_panfrost_submit *args,
>  		  struct panfrost_job *job)
>  {
> +	struct panfrost_file_priv *priv = file_priv->driver_priv;
> +	struct panfrost_gem_object *bo;
> +	unsigned int i;
> +	int ret;
> +
>  	job->bo_count = args->bo_handle_count;
>  
>  	if (!job->bo_count)
> @@ -130,9 +144,32 @@ panfrost_lookup_bos(struct drm_device *dev,
>  	if (!job->implicit_fences)
>  		return -ENOMEM;
>  
> -	return drm_gem_objects_lookup(file_priv,
> -				      (void __user *)(uintptr_t)args->bo_handles,
> -				      job->bo_count, &job->bos);
> +	ret = drm_gem_objects_lookup(file_priv,
> +				     (void __user *)(uintptr_t)args->bo_handles,
> +				     job->bo_count, &job->bos);
> +	if (ret)
> +		return ret;
> +
> +	job->mappings = kvmalloc_array(job->bo_count,
> +				       sizeof(struct panfrost_gem_mapping *),
> +				       GFP_KERNEL | __GFP_ZERO);
> +	if (!job->mappings)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < job->bo_count; i++) {
> +		struct panfrost_gem_mapping *mapping;
> +
> +		bo = to_panfrost_bo(job->bos[i]);
> +		mapping = panfrost_gem_mapping_get(bo, priv);
> +		if (!mapping) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		job->mappings[i] = mapping;
> +	}
> +
> +	return ret;
>  }
>  
>  /**
> @@ -320,7 +357,9 @@ static int panfrost_ioctl_mmap_bo(struct drm_device *dev, void *data,
>  static int panfrost_ioctl_get_bo_offset(struct drm_device *dev, void *data,
>  			    struct drm_file *file_priv)
>  {
> +	struct panfrost_file_priv *priv = file_priv->driver_priv;
>  	struct drm_panfrost_get_bo_offset *args = data;
> +	struct panfrost_gem_mapping *mapping;
>  	struct drm_gem_object *gem_obj;
>  	struct panfrost_gem_object *bo;
>  
> @@ -331,18 +370,25 @@ static int panfrost_ioctl_get_bo_offset(struct drm_device *dev, void *data,
>  	}
>  	bo = to_panfrost_bo(gem_obj);
>  
> -	args->offset = bo->node.start << PAGE_SHIFT;
> -
> +	mapping = panfrost_gem_mapping_get(bo, priv);
>  	drm_gem_object_put_unlocked(gem_obj);
> +
> +	if (!mapping)
> +		return -EINVAL;
> +
> +	args->offset = mapping->mmnode.start << PAGE_SHIFT;
> +	panfrost_gem_mapping_put(mapping);
>  	return 0;
>  }
>  
>  static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  				  struct drm_file *file_priv)
>  {
> +	struct panfrost_file_priv *priv = file_priv->driver_priv;
>  	struct drm_panfrost_madvise *args = data;
>  	struct panfrost_device *pfdev = dev->dev_private;
>  	struct drm_gem_object *gem_obj;
> +	struct panfrost_gem_object *bo;
>  	int ret;
>  
>  	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
> @@ -364,18 +410,48 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  	if (ret)
>  		goto out_unlock_object_name;
>  
> +	bo = to_panfrost_bo(gem_obj);
> +
>  	mutex_lock(&pfdev->shrinker_lock);
> +	mutex_lock(&bo->mappings.lock);
> +	if (args->madv == PANFROST_MADV_DONTNEED) {
> +		struct panfrost_gem_mapping *first, *last;
> +
> +		first = list_first_entry(&bo->mappings.list,
> +					 struct panfrost_gem_mapping,
> +					 node);
> +		last = list_last_entry(&bo->mappings.list,
> +				       struct panfrost_gem_mapping,
> +				       node);
> +
> +		/*
> +		 * If we want to mark the BO purgeable, there must be only one
> +		 * user: the caller FD.
> +		 * We could do something smarter and mark the BO purgeable only
> +		 * when all its users have marked it purgeable, but globally
> +		 * visible/shared BOs are likely to never be marked purgeable
> +		 * anyway, so let's not bother.
> +		 */
> +		if (first != last || WARN_ON_ONCE(first->mmu != &priv->mmu)) {
> +			ret = -EINVAL;
> +			goto out_unlock_mappings;
> +		}
> +	}
> +
>  	args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);
>  
>  	if (args->retained) {
> -		struct panfrost_gem_object *bo = to_panfrost_bo(gem_obj);
> -
>  		if (args->madv == PANFROST_MADV_DONTNEED)
>  			list_add_tail(&bo->base.madv_list,
>  				      &pfdev->shrinker_list);
>  		else if (args->madv == PANFROST_MADV_WILLNEED)
>  			list_del_init(&bo->base.madv_list);
>  	}
> +
> +	ret = 0;
> +
> +out_unlock_mappings:
> +	mutex_unlock(&bo->mappings.lock);
>  	mutex_unlock(&pfdev->shrinker_lock);
>  
>  out_unlock_object_name:
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 31d6417dd21c..5a2c463c45d3 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -29,6 +29,12 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
>  	list_del_init(&bo->base.madv_list);
>  	mutex_unlock(&pfdev->shrinker_lock);
>  
> +	/*
> +	 * If we still have mappings attached to the BO, there's a problem in
> +	 * our refcounting.
> +	 */
> +	WARN_ON_ONCE(!list_empty(&bo->mappings.list));
> +
>  	if (bo->sgts) {
>  		int i;
>  		int n_sgt = bo->base.base.size / SZ_2M;
> @@ -46,6 +52,70 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
>  	drm_gem_shmem_free_object(obj);
>  }
>  
> +struct panfrost_gem_mapping *
> +panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
> +			 struct panfrost_file_priv *priv)
> +{
> +	struct panfrost_gem_mapping *mapping = NULL, *iter;
> +
> +	mutex_lock(&bo->mappings.lock);
> +	list_for_each_entry(iter, &bo->mappings.list, node) {
> +		if (iter->mmu == &priv->mmu) {
> +			kref_get(&iter->refcount);
> +			mapping = iter;
> +		}
> +	}
> +	mutex_unlock(&bo->mappings.lock);
> +
> +	return mapping;
> +}
> +
> +static void
> +panfrost_gem_teardown_mapping(struct panfrost_gem_mapping *mapping)
> +{
> +	struct panfrost_file_priv *priv;
> +
> +	if (mapping->active)
> +		panfrost_mmu_unmap(mapping);
> +
> +	priv = container_of(mapping->mmu, struct panfrost_file_priv, mmu);
> +	spin_lock(&priv->mm_lock);
> +	if (drm_mm_node_allocated(&mapping->mmnode))
> +		drm_mm_remove_node(&mapping->mmnode);
> +	spin_unlock(&priv->mm_lock);
> +}
> +
> +static void panfrost_gem_mapping_release(struct kref *kref)
> +{
> +	struct panfrost_gem_mapping *mapping;
> +	struct panfrost_file_priv *priv;
> +
> +	mapping = container_of(kref, struct panfrost_gem_mapping, refcount);
> +	priv = container_of(mapping->mmu, struct panfrost_file_priv, mmu);
> +
> +	panfrost_gem_teardown_mapping(mapping);
> +	drm_gem_object_put_unlocked(&mapping->obj->base.base);
> +	kfree(mapping);
> +}
> +
> +void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping)
> +{
> +	if (!mapping)
> +		return;
> +
> +	kref_put(&mapping->refcount, panfrost_gem_mapping_release);
> +}
> +
> +void panfrost_gem_teardown_mappings(struct panfrost_gem_object *bo)
> +{
> +	struct panfrost_gem_mapping *mapping;
> +
> +	mutex_lock(&bo->mappings.lock);
> +	list_for_each_entry(mapping, &bo->mappings.list, node)
> +		panfrost_gem_teardown_mapping(mapping);
> +	mutex_unlock(&bo->mappings.lock);
> +}
> +
>  int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)
>  {
>  	int ret;
> @@ -54,6 +124,16 @@ int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)
>  	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
>  	unsigned long color = bo->noexec ? PANFROST_BO_NOEXEC : 0;
>  	struct panfrost_file_priv *priv = file_priv->driver_priv;
> +	struct panfrost_gem_mapping *mapping;
> +
> +	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
> +	if (!mapping)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&mapping->node);
> +	kref_init(&mapping->refcount);
> +	drm_gem_object_get(obj);
> +	mapping->obj = bo;
>  
>  	/*
>  	 * Executable buffers cannot cross a 16MB boundary as the program
> @@ -66,37 +146,61 @@ int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)
>  	else
>  		align = size >= SZ_2M ? SZ_2M >> PAGE_SHIFT : 0;
>  
> -	bo->mmu = &priv->mmu;
> +	mapping->mmu = &priv->mmu;
>  	spin_lock(&priv->mm_lock);
> -	ret = drm_mm_insert_node_generic(&priv->mm, &bo->node,
> +	ret = drm_mm_insert_node_generic(&priv->mm, &mapping->mmnode,
>  					 size >> PAGE_SHIFT, align, color, 0);
>  	spin_unlock(&priv->mm_lock);
>  	if (ret)
> -		return ret;
> +		goto err;
>  
>  	if (!bo->is_heap) {
> -		ret = panfrost_mmu_map(bo);
> -		if (ret) {
> -			spin_lock(&priv->mm_lock);
> -			drm_mm_remove_node(&bo->node);
> -			spin_unlock(&priv->mm_lock);
> -		}
> +		ret = panfrost_mmu_map(mapping);
> +		if (ret)
> +			goto err;
>  	}
> +
> +	mutex_lock(&obj->dev->object_name_lock);
> +	/*
> +	 * We must make sure the BO has not been marked purgeable before
> +	 * adding the mapping.
> +	 */
> +	if (bo->base.madv == PANFROST_MADV_WILLNEED) {
> +		mutex_lock(&bo->mappings.lock);
> +		list_add_tail(&mapping->node, &bo->mappings.list);
> +		mutex_unlock(&bo->mappings.lock);
> +	} else {
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&obj->dev->object_name_lock);
> +
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	panfrost_gem_mapping_put(mapping);
>  	return ret;
>  }
>  
>  void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv)
>  {
> -	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
>  	struct panfrost_file_priv *priv = file_priv->driver_priv;
> +	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
> +	struct panfrost_gem_mapping *mapping = NULL, *iter;
> +
> +	mutex_lock(&bo->mappings.lock);
> +	list_for_each_entry(iter, &bo->mappings.list, node) {
> +		if (iter->mmu == &priv->mmu) {
> +			mapping = iter;
> +			list_del(&iter->node);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&bo->mappings.lock);
>  
> -	if (bo->is_mapped)
> -		panfrost_mmu_unmap(bo);
> -
> -	spin_lock(&priv->mm_lock);
> -	if (drm_mm_node_allocated(&bo->node))
> -		drm_mm_remove_node(&bo->node);
> -	spin_unlock(&priv->mm_lock);
> +	panfrost_gem_mapping_put(mapping);
>  }
>  
>  static struct dma_buf *
> @@ -163,6 +267,8 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t
>  	if (!obj)
>  		return NULL;
>  
> +	INIT_LIST_HEAD(&obj->mappings.list);
> +	mutex_init(&obj->mappings.lock);
>  	obj->base.base.funcs = &panfrost_gem_funcs;
>  
>  	return &obj->base.base;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index 4b17e7308764..ca1bc9019600 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -13,23 +13,46 @@ struct panfrost_gem_object {
>  	struct drm_gem_shmem_object base;
>  	struct sg_table *sgts;
>  
> -	struct panfrost_mmu *mmu;
> -	struct drm_mm_node node;
> -	bool is_mapped		:1;
> +	/*
> +	 * Use a list for now. If searching a mapping ever becomes the
> +	 * bottleneck, we should consider using an RB-tree, or even better,
> +	 * let the core store drm_gem_object_mapping entries (where we
> +	 * could place driver specific data) instead of drm_gem_object ones
> +	 * in its drm_file->object_idr table.
> +	 *
> +	 * struct drm_gem_object_mapping {
> +	 *	struct drm_gem_object *obj;
> +	 *	void *driver_priv;
> +	 * };
> +	 */
> +	struct {
> +		struct list_head list;
> +		struct mutex lock;
> +	} mappings;
> +
>  	bool noexec		:1;
>  	bool is_heap		:1;
>  };
>  
> +struct panfrost_gem_mapping {
> +	struct list_head node;
> +	struct kref refcount;
> +	struct panfrost_gem_object *obj;
> +	struct drm_mm_node mmnode;
> +	struct panfrost_mmu *mmu;
> +	bool active		:1;
> +};
> +
>  static inline
>  struct  panfrost_gem_object *to_panfrost_bo(struct drm_gem_object *obj)
>  {
>  	return container_of(to_drm_gem_shmem_obj(obj), struct panfrost_gem_object, base);
>  }
>  
> -static inline
> -struct  panfrost_gem_object *drm_mm_node_to_panfrost_bo(struct drm_mm_node *node)
> +static inline struct panfrost_gem_mapping *
> +drm_mm_node_to_panfrost_mapping(struct drm_mm_node *node)
>  {
> -	return container_of(node, struct panfrost_gem_object, node);
> +	return container_of(node, struct panfrost_gem_mapping, mmnode);
>  }
>  
>  struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t size);
> @@ -49,6 +72,12 @@ int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv);
>  void panfrost_gem_close(struct drm_gem_object *obj,
>  			struct drm_file *file_priv);
>  
> +struct panfrost_gem_mapping *
> +panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
> +			 struct panfrost_file_priv *priv);
> +void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping);
> +void panfrost_gem_teardown_mappings(struct panfrost_gem_object *bo);
> +
>  void panfrost_gem_shrinker_init(struct drm_device *dev);
>  void panfrost_gem_shrinker_cleanup(struct drm_device *dev);
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> index 458f0fa68111..b36df326c860 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> @@ -39,11 +39,13 @@ panfrost_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc
>  static bool panfrost_gem_purge(struct drm_gem_object *obj)
>  {
>  	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> +	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
> +
>  
>  	if (!mutex_trylock(&shmem->pages_lock))
>  		return false;
>  
> -	panfrost_mmu_unmap(to_panfrost_bo(obj));
> +	panfrost_gem_teardown_mappings(bo);
>  	drm_gem_shmem_purge_locked(obj);
>  
>  	mutex_unlock(&shmem->pages_lock);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index cdd9448fbbdd..c85d45be3b5e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -269,9 +269,20 @@ static void panfrost_job_cleanup(struct kref *ref)
>  	dma_fence_put(job->done_fence);
>  	dma_fence_put(job->render_done_fence);
>  
> -	if (job->bos) {
> +	if (job->mappings) {
>  		for (i = 0; i < job->bo_count; i++)
> +			panfrost_gem_mapping_put(job->mappings[i]);
> +		kvfree(job->mappings);
> +	}
> +
> +	if (job->bos) {
> +		struct panfrost_gem_object *bo;
> +
> +		for (i = 0; i < job->bo_count; i++) {
> +			bo = to_panfrost_bo(job->bos[i]);
>  			drm_gem_object_put_unlocked(job->bos[i]);
> +		}
> +
>  		kvfree(job->bos);
>  	}
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
> index 62454128a792..bbd3ba97ff67 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.h
> @@ -32,6 +32,7 @@ struct panfrost_job {
>  
>  	/* Exclusive fences we have taken from the BOs to wait for */
>  	struct dma_fence **implicit_fences;
> +	struct panfrost_gem_mapping **mappings;
>  	struct drm_gem_object **bos;
>  	u32 bo_count;
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index a3ed64a1f15e..763cfca886a7 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -269,14 +269,15 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
>  	return 0;
>  }
>  
> -int panfrost_mmu_map(struct panfrost_gem_object *bo)
> +int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
>  {
> +	struct panfrost_gem_object *bo = mapping->obj;
>  	struct drm_gem_object *obj = &bo->base.base;
>  	struct panfrost_device *pfdev = to_panfrost_device(obj->dev);
>  	struct sg_table *sgt;
>  	int prot = IOMMU_READ | IOMMU_WRITE;
>  
> -	if (WARN_ON(bo->is_mapped))
> +	if (WARN_ON(mapping->active))
>  		return 0;
>  
>  	if (bo->noexec)
> @@ -286,25 +287,28 @@ int panfrost_mmu_map(struct panfrost_gem_object *bo)
>  	if (WARN_ON(IS_ERR(sgt)))
>  		return PTR_ERR(sgt);
>  
> -	mmu_map_sg(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT, prot, sgt);
> -	bo->is_mapped = true;
> +	mmu_map_sg(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT,
> +		   prot, sgt);
> +	mapping->active = true;
>  
>  	return 0;
>  }
>  
> -void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
> +void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping)
>  {
> +	struct panfrost_gem_object *bo = mapping->obj;
>  	struct drm_gem_object *obj = &bo->base.base;
>  	struct panfrost_device *pfdev = to_panfrost_device(obj->dev);
> -	struct io_pgtable_ops *ops = bo->mmu->pgtbl_ops;
> -	u64 iova = bo->node.start << PAGE_SHIFT;
> -	size_t len = bo->node.size << PAGE_SHIFT;
> +	struct io_pgtable_ops *ops = mapping->mmu->pgtbl_ops;
> +	u64 iova = mapping->mmnode.start << PAGE_SHIFT;
> +	size_t len = mapping->mmnode.size << PAGE_SHIFT;
>  	size_t unmapped_len = 0;
>  
> -	if (WARN_ON(!bo->is_mapped))
> +	if (WARN_ON(!mapping->active))
>  		return;
>  
> -	dev_dbg(pfdev->dev, "unmap: as=%d, iova=%llx, len=%zx", bo->mmu->as, iova, len);
> +	dev_dbg(pfdev->dev, "unmap: as=%d, iova=%llx, len=%zx",
> +		mapping->mmu->as, iova, len);
>  
>  	while (unmapped_len < len) {
>  		size_t unmapped_page;
> @@ -318,8 +322,9 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>  		unmapped_len += pgsize;
>  	}
>  
> -	panfrost_mmu_flush_range(pfdev, bo->mmu, bo->node.start << PAGE_SHIFT, len);
> -	bo->is_mapped = false;
> +	panfrost_mmu_flush_range(pfdev, mapping->mmu,
> +				 mapping->mmnode.start << PAGE_SHIFT, len);
> +	mapping->active = false;
>  }
>  
>  static void mmu_tlb_inv_context_s1(void *cookie)
> @@ -394,10 +399,10 @@ void panfrost_mmu_pgtable_free(struct panfrost_file_priv *priv)
>  	free_io_pgtable_ops(mmu->pgtbl_ops);
>  }
>  
> -static struct panfrost_gem_object *
> -addr_to_drm_mm_node(struct panfrost_device *pfdev, int as, u64 addr)
> +static struct panfrost_gem_mapping *
> +addr_to_mapping(struct panfrost_device *pfdev, int as, u64 addr)
>  {
> -	struct panfrost_gem_object *bo = NULL;
> +	struct panfrost_gem_mapping *mapping = NULL;
>  	struct panfrost_file_priv *priv;
>  	struct drm_mm_node *node;
>  	u64 offset = addr >> PAGE_SHIFT;
> @@ -418,8 +423,9 @@ addr_to_drm_mm_node(struct panfrost_device *pfdev, int as, u64 addr)
>  	drm_mm_for_each_node(node, &priv->mm) {
>  		if (offset >= node->start &&
>  		    offset < (node->start + node->size)) {
> -			bo = drm_mm_node_to_panfrost_bo(node);
> -			drm_gem_object_get(&bo->base.base);
> +			mapping = drm_mm_node_to_panfrost_mapping(node);
> +
> +			kref_get(&mapping->refcount);
>  			break;
>  		}
>  	}
> @@ -427,7 +433,7 @@ addr_to_drm_mm_node(struct panfrost_device *pfdev, int as, u64 addr)
>  	spin_unlock(&priv->mm_lock);
>  out:
>  	spin_unlock(&pfdev->as_lock);
> -	return bo;
> +	return mapping;
>  }
>  
>  #define NUM_FAULT_PAGES (SZ_2M / PAGE_SIZE)
> @@ -436,28 +442,30 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>  				       u64 addr)
>  {
>  	int ret, i;
> +	struct panfrost_gem_mapping *bomapping;
>  	struct panfrost_gem_object *bo;
>  	struct address_space *mapping;
>  	pgoff_t page_offset;
>  	struct sg_table *sgt;
>  	struct page **pages;
>  
> -	bo = addr_to_drm_mm_node(pfdev, as, addr);
> -	if (!bo)
> +	bomapping = addr_to_mapping(pfdev, as, addr);
> +	if (!bomapping)
>  		return -ENOENT;
>  
> +	bo = bomapping->obj;
>  	if (!bo->is_heap) {
>  		dev_WARN(pfdev->dev, "matching BO is not heap type (GPU VA = %llx)",
> -			 bo->node.start << PAGE_SHIFT);
> +			 bomapping->mmnode.start << PAGE_SHIFT);
>  		ret = -EINVAL;
>  		goto err_bo;
>  	}
> -	WARN_ON(bo->mmu->as != as);
> +	WARN_ON(bomapping->mmu->as != as);
>  
>  	/* Assume 2MB alignment and size multiple */
>  	addr &= ~((u64)SZ_2M - 1);
>  	page_offset = addr >> PAGE_SHIFT;
> -	page_offset -= bo->node.start;
> +	page_offset -= bomapping->mmnode.start;
>  
>  	mutex_lock(&bo->base.pages_lock);
>  
> @@ -509,13 +517,14 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>  		goto err_map;
>  	}
>  
> -	mmu_map_sg(pfdev, bo->mmu, addr, IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
> +	mmu_map_sg(pfdev, bomapping->mmu, addr,
> +		   IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
>  
> -	bo->is_mapped = true;
> +	bomapping->active = true;
>  
>  	dev_dbg(pfdev->dev, "mapped page fault @ AS%d %llx", as, addr);
>  
> -	drm_gem_object_put_unlocked(&bo->base.base);
> +	panfrost_gem_mapping_put(bomapping);
>  
>  	return 0;
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.h b/drivers/gpu/drm/panfrost/panfrost_mmu.h
> index 7c5b6775ae23..44fc2edf63ce 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.h
> @@ -4,12 +4,12 @@
>  #ifndef __PANFROST_MMU_H__
>  #define __PANFROST_MMU_H__
>  
> -struct panfrost_gem_object;
> +struct panfrost_gem_mapping;
>  struct panfrost_file_priv;
>  struct panfrost_mmu;
>  
> -int panfrost_mmu_map(struct panfrost_gem_object *bo);
> -void panfrost_mmu_unmap(struct panfrost_gem_object *bo);
> +int panfrost_mmu_map(struct panfrost_gem_mapping *mapping);
> +void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping);
>  
>  int panfrost_mmu_init(struct panfrost_device *pfdev);
>  void panfrost_mmu_fini(struct panfrost_device *pfdev);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index 2c04e858c50a..684820448be3 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> @@ -25,7 +25,7 @@
>  #define V4_SHADERS_PER_COREGROUP	4
>  
>  struct panfrost_perfcnt {
> -	struct panfrost_gem_object *bo;
> +	struct panfrost_gem_mapping *mapping;
>  	size_t bosize;
>  	void *buf;
>  	struct panfrost_file_priv *user;
> @@ -49,7 +49,7 @@ static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev)
>  	int ret;
>  
>  	reinit_completion(&pfdev->perfcnt->dump_comp);
> -	gpuva = pfdev->perfcnt->bo->node.start << PAGE_SHIFT;
> +	gpuva = pfdev->perfcnt->mapping->mmnode.start << PAGE_SHIFT;
>  	gpu_write(pfdev, GPU_PERFCNT_BASE_LO, gpuva);
>  	gpu_write(pfdev, GPU_PERFCNT_BASE_HI, gpuva >> 32);
>  	gpu_write(pfdev, GPU_INT_CLEAR,
> @@ -89,17 +89,22 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  	if (IS_ERR(bo))
>  		return PTR_ERR(bo);
>  
> -	perfcnt->bo = to_panfrost_bo(&bo->base);
> -
>  	/* Map the perfcnt buf in the address space attached to file_priv. */
> -	ret = panfrost_gem_open(&perfcnt->bo->base.base, file_priv);
> +	ret = panfrost_gem_open(&bo->base, file_priv);
>  	if (ret)
>  		goto err_put_bo;
>  
> +	perfcnt->mapping = panfrost_gem_mapping_get(to_panfrost_bo(&bo->base),
> +						    user);
> +	if (!perfcnt->mapping) {
> +		ret = -EINVAL;
> +		goto err_close_bo;
> +	}
> +
>  	perfcnt->buf = drm_gem_shmem_vmap(&bo->base);
>  	if (IS_ERR(perfcnt->buf)) {
>  		ret = PTR_ERR(perfcnt->buf);
> -		goto err_close_bo;
> +		goto err_put_mapping;
>  	}
>  
>  	/*
> @@ -154,12 +159,17 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
>  		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
>  
> +	/* The BO ref is retained by the mapping. */
> +	drm_gem_object_put_unlocked(&bo->base);
> +
>  	return 0;
>  
>  err_vunmap:
> -	drm_gem_shmem_vunmap(&perfcnt->bo->base.base, perfcnt->buf);
> +	drm_gem_shmem_vunmap(&bo->base, perfcnt->buf);
> +err_put_mapping:
> +	panfrost_gem_mapping_put(perfcnt->mapping);
>  err_close_bo:
> -	panfrost_gem_close(&perfcnt->bo->base.base, file_priv);
> +	panfrost_gem_close(&bo->base, file_priv);
>  err_put_bo:
>  	drm_gem_object_put_unlocked(&bo->base);
>  	return ret;
> @@ -182,11 +192,11 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
>  		  GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF));
>  
>  	perfcnt->user = NULL;
> -	drm_gem_shmem_vunmap(&perfcnt->bo->base.base, perfcnt->buf);
> +	drm_gem_shmem_vunmap(&perfcnt->mapping->obj->base.base, perfcnt->buf);
>  	perfcnt->buf = NULL;
> -	panfrost_gem_close(&perfcnt->bo->base.base, file_priv);
> -	drm_gem_object_put_unlocked(&perfcnt->bo->base.base);
> -	perfcnt->bo = NULL;
> +	panfrost_gem_close(&perfcnt->mapping->obj->base.base, file_priv);
> +	panfrost_gem_mapping_put(perfcnt->mapping);
> +	perfcnt->mapping = NULL;
>  	pm_runtime_mark_last_busy(pfdev->dev);
>  	pm_runtime_put_autosuspend(pfdev->dev);
>  
> -- 
> 2.23.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 6/8] drm/panfrost: Make sure imported/exported BOs are never purged

[Boris Brezillon]

On Fri, 29 Nov 2019 21:12:13 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Nov 29, 2019 at 02:59:06PM +0100, Boris Brezillon wrote:
> > We don't want imported/exported BOs to be purges, as those are shared
> > with other processes that might still use them. We should also refuse
> > to export a BO if it's been marked purgeable or has already been purged.
> > 
> > Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_drv.c | 19 ++++++++++++++++-
> >  drivers/gpu/drm/panfrost/panfrost_gem.c | 27 +++++++++++++++++++++++++
> >  2 files changed, 45 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index 1c67ac434e10..751df975534f 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -343,6 +343,7 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> >  	struct drm_panfrost_madvise *args = data;
> >  	struct panfrost_device *pfdev = dev->dev_private;
> >  	struct drm_gem_object *gem_obj;
> > +	int ret;
> >  
> >  	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
> >  	if (!gem_obj) {
> > @@ -350,6 +351,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> >  		return -ENOENT;
> >  	}
> >  
> > +	/*
> > +	 * We don't want to mark exported/imported BOs as purgeable: we're not
> > +	 * the only owner in that case.
> > +	 */
> > +	mutex_lock(&dev->object_name_lock);  
> 
> Kinda not awesome that you have to take this core lock here and encumber
> core drm locking with random driver stuff.

Looks like drm_gem_shmem_is_purgeable() already does the !imported &&
!exported check. For the imported case, I think we're good, since
userspace can't change the madv value before ->import_attach has been
set. For the exporter case, we need to make sure there's no race
between the export and madvise operations, which I can probably do from
the gem->export() hook by taking the shrinker or bo->mappings lock.

> 
> Can't this be solved with your own locking only and some reasonable
> ordering of checks? big locks because it's easy is endless long-term pain.
> 
> Also exporting purgeable objects is kinda a userspace bug, all you have to
> do is not oops in dma_buf_attachment_map. No need to prevent the damage
> here imo.

I feel like making sure an exported BO can't be purged or a purged BO
can't be exported would be much simpler than making sure all importers
are ready to have the sgt freed.

Re: [PATCH 7/8] drm/panfrost: Add the panfrost_gem_mapping concept

[Boris Brezillon]

On Fri, 29 Nov 2019 21:14:59 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Nov 29, 2019 at 02:59:07PM +0100, Boris Brezillon wrote:
> > With the introduction of per-FD address space, the same BO can be mapped
> > in different address space if the BO is globally visible (GEM_FLINK)  
> 
> Also dma-buf self-imports for wayland/dri3 ...

Indeed, I'll extend the commit message to mention that case.

> 
> > and opened in different context. The current implementation does not
> > take case into account, and attaches the mapping directly to the
> > panfrost_gem_object.
> > 
> > Let's create a panfrost_gem_mapping struct and allow multiple mappings
> > per BO.
> > 
> > The mappings are refcounted, which helps solve another problem where
> > mappings were teared down (GEM handle closed by userspace) while GPU
> > jobs accessing those BOs were still in-flight. Jobs now keep a
> > reference on the mappings they use.  
> 
> uh what.
> 
> tbh this sounds bad enough (as in how did a desktop on panfrost ever work)

Well, we didn't discover this problem until recently because:

1/ We have a BO cache in mesa, and until recently, this cache could
only grow (no entry eviction and no MADVISE support), meaning that BOs
were staying around forever until the app was killed.

2/ Mappings were teared down at BO destruction time before commit
a5efb4c9a562 ("drm/panfrost: Restructure the GEM object creation"), and
jobs are retaining references to all the BO they access.

3/ The mesa driver was serializing GPU jobs, and only releasing the BO
reference when the job was done (wait on the completion fence). This
has recently been changed, and now BOs are returned to the cache as
soon as the job has been submitted to the kernel. When that
happens,those BOs are marked purgeable which means the kernel can
reclaim them when it's under memory pressure.

So yes, kernel 5.4 with a recent mesa version is currently subject to
GPU page-fault storms when the system starts reclaiming memory.

> that I think you really want a few igts to test this stuff.

I'll see what I can come up with (not sure how to easily detect
pagefaults from userspace).

Re: [PATCH 2/8] drm/panfrost: Fix a race in panfrost_ioctl_madvise()

[Boris Brezillon]

On Fri, 29 Nov 2019 21:07:33 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Nov 29, 2019 at 02:40:34PM +0000, Steven Price wrote:
> > On 29/11/2019 14:33, Boris Brezillon wrote:  
> > > On Fri, 29 Nov 2019 14:24:48 +0000
> > > Steven Price <steven.price@arm.com> wrote:
> > >   
> > >> On 29/11/2019 13:59, Boris Brezillon wrote:  
> > >>> If 2 threads change the MADVISE property of the same BO in parallel we
> > >>> might end up with an shmem->madv value that's inconsistent with the
> > >>> presence of the BO in the shrinker list.    
> > >>
> > >> I'm a bit worried from the point of view of user space sanity that you
> > >> observed this - but clearly the kernel should be robust!  
> > > 
> > > It's not something I observed, just found the race by inspecting the
> > > code, and I thought it was worth fixing it.  
> > 
> > Good! ;) Your cover letter referring to a "debug session" made me think
> > you'd actually hit all these.  
> 
> Time for some igt to make sure this is actually correct?

That's not something I can easily trigger without instrumenting the
code: I need 2 threads doing MADVISE with 2 different values,
and there has to be a context switch between the
drm_gem_shmem_madvise() call and the mutex_lock(shrinker_lock) one. And
last but not least, I'll need a way to report such inconsistencies to
userspace (trace?).

Re: [PATCH 6/8] drm/panfrost: Make sure imported/exported BOs are never purged

[Daniel Vetter]

On Fri, Nov 29, 2019 at 10:09:24PM +0100, Boris Brezillon wrote:
> On Fri, 29 Nov 2019 21:12:13 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Fri, Nov 29, 2019 at 02:59:06PM +0100, Boris Brezillon wrote:
> > > We don't want imported/exported BOs to be purges, as those are shared
> > > with other processes that might still use them. We should also refuse
> > > to export a BO if it's been marked purgeable or has already been purged.
> > > 
> > > Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > ---
> > >  drivers/gpu/drm/panfrost/panfrost_drv.c | 19 ++++++++++++++++-
> > >  drivers/gpu/drm/panfrost/panfrost_gem.c | 27 +++++++++++++++++++++++++
> > >  2 files changed, 45 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > index 1c67ac434e10..751df975534f 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > @@ -343,6 +343,7 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> > >  	struct drm_panfrost_madvise *args = data;
> > >  	struct panfrost_device *pfdev = dev->dev_private;
> > >  	struct drm_gem_object *gem_obj;
> > > +	int ret;
> > >  
> > >  	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
> > >  	if (!gem_obj) {
> > > @@ -350,6 +351,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> > >  		return -ENOENT;
> > >  	}
> > >  
> > > +	/*
> > > +	 * We don't want to mark exported/imported BOs as purgeable: we're not
> > > +	 * the only owner in that case.
> > > +	 */
> > > +	mutex_lock(&dev->object_name_lock);  
> > 
> > Kinda not awesome that you have to take this core lock here and encumber
> > core drm locking with random driver stuff.
> 
> Looks like drm_gem_shmem_is_purgeable() already does the !imported &&
> !exported check. For the imported case, I think we're good, since
> userspace can't change the madv value before ->import_attach has been
> set. For the exporter case, we need to make sure there's no race
> between the export and madvise operations, which I can probably do from
> the gem->export() hook by taking the shrinker or bo->mappings lock.
> 
> > 
> > Can't this be solved with your own locking only and some reasonable
> > ordering of checks? big locks because it's easy is endless long-term pain.
> > 
> > Also exporting purgeable objects is kinda a userspace bug, all you have to
> > do is not oops in dma_buf_attachment_map. No need to prevent the damage
> > here imo.
> 
> I feel like making sure an exported BO can't be purged or a purged BO
> can't be exported would be much simpler than making sure all importers
> are ready to have the sgt freed.

If you free the sgt while someone is using it, that's kinda a different
bug I think. You already have a pages refcount, that should be enough to
prevent this?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 7/8] drm/panfrost: Add the panfrost_gem_mapping concept

[Daniel Vetter]

On Fri, Nov 29, 2019 at 10:36:29PM +0100, Boris Brezillon wrote:
> On Fri, 29 Nov 2019 21:14:59 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Fri, Nov 29, 2019 at 02:59:07PM +0100, Boris Brezillon wrote:
> > > With the introduction of per-FD address space, the same BO can be mapped
> > > in different address space if the BO is globally visible (GEM_FLINK)  
> > 
> > Also dma-buf self-imports for wayland/dri3 ...
> 
> Indeed, I'll extend the commit message to mention that case.
> 
> > 
> > > and opened in different context. The current implementation does not
> > > take case into account, and attaches the mapping directly to the
> > > panfrost_gem_object.
> > > 
> > > Let's create a panfrost_gem_mapping struct and allow multiple mappings
> > > per BO.
> > > 
> > > The mappings are refcounted, which helps solve another problem where
> > > mappings were teared down (GEM handle closed by userspace) while GPU
> > > jobs accessing those BOs were still in-flight. Jobs now keep a
> > > reference on the mappings they use.  
> > 
> > uh what.
> > 
> > tbh this sounds bad enough (as in how did a desktop on panfrost ever work)
> 
> Well, we didn't discover this problem until recently because:
> 
> 1/ We have a BO cache in mesa, and until recently, this cache could
> only grow (no entry eviction and no MADVISE support), meaning that BOs
> were staying around forever until the app was killed.

Uh, so where was the userspace when we merged this?

> 2/ Mappings were teared down at BO destruction time before commit
> a5efb4c9a562 ("drm/panfrost: Restructure the GEM object creation"), and
> jobs are retaining references to all the BO they access.
> 
> 3/ The mesa driver was serializing GPU jobs, and only releasing the BO
> reference when the job was done (wait on the completion fence). This
> has recently been changed, and now BOs are returned to the cache as
> soon as the job has been submitted to the kernel. When that
> happens,those BOs are marked purgeable which means the kernel can
> reclaim them when it's under memory pressure.
> 
> So yes, kernel 5.4 with a recent mesa version is currently subject to
> GPU page-fault storms when the system starts reclaiming memory.
> 
> > that I think you really want a few igts to test this stuff.
> 
> I'll see what I can come up with (not sure how to easily detect
> pagefaults from userspace).

The dumb approach we do is just thrash memory and check nothing has blown
up (which the runner does by looking at the dmesg and a few proc files).
If you run that on a kernel with all debugging enabled, it's pretty good
at catching issues.

For added nastiness lots of interrupts to check error paths/syscall
restarting, and at the end of the testcase, some sanity check that all the
bo still contain what you think they should contain.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 7/8] drm/panfrost: Add the panfrost_gem_mapping concept

[Boris Brezillon]

On Mon, 2 Dec 2019 09:55:32 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Nov 29, 2019 at 10:36:29PM +0100, Boris Brezillon wrote:
> > On Fri, 29 Nov 2019 21:14:59 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >   
> > > On Fri, Nov 29, 2019 at 02:59:07PM +0100, Boris Brezillon wrote:  
> > > > With the introduction of per-FD address space, the same BO can be mapped
> > > > in different address space if the BO is globally visible (GEM_FLINK)    
> > > 
> > > Also dma-buf self-imports for wayland/dri3 ...  
> > 
> > Indeed, I'll extend the commit message to mention that case.
> >   
> > >   
> > > > and opened in different context. The current implementation does not
> > > > take case into account, and attaches the mapping directly to the
> > > > panfrost_gem_object.
> > > > 
> > > > Let's create a panfrost_gem_mapping struct and allow multiple mappings
> > > > per BO.
> > > > 
> > > > The mappings are refcounted, which helps solve another problem where
> > > > mappings were teared down (GEM handle closed by userspace) while GPU
> > > > jobs accessing those BOs were still in-flight. Jobs now keep a
> > > > reference on the mappings they use.    
> > > 
> > > uh what.
> > > 
> > > tbh this sounds bad enough (as in how did a desktop on panfrost ever work)  
> > 
> > Well, we didn't discover this problem until recently because:
> > 
> > 1/ We have a BO cache in mesa, and until recently, this cache could
> > only grow (no entry eviction and no MADVISE support), meaning that BOs
> > were staying around forever until the app was killed.  
> 
> Uh, so where was the userspace when we merged this?

Well, userspace was there, it's just that we probably didn't stress
the implementation as it should have been when doing the changes
described in #1, #2 and 3. 

> 
> > 2/ Mappings were teared down at BO destruction time before commit
> > a5efb4c9a562 ("drm/panfrost: Restructure the GEM object creation"), and
> > jobs are retaining references to all the BO they access.
> > 
> > 3/ The mesa driver was serializing GPU jobs, and only releasing the BO
> > reference when the job was done (wait on the completion fence). This
> > has recently been changed, and now BOs are returned to the cache as
> > soon as the job has been submitted to the kernel. When that
> > happens,those BOs are marked purgeable which means the kernel can
> > reclaim them when it's under memory pressure.
> > 
> > So yes, kernel 5.4 with a recent mesa version is currently subject to
> > GPU page-fault storms when the system starts reclaiming memory.
> >   
> > > that I think you really want a few igts to test this stuff.  
> > 
> > I'll see what I can come up with (not sure how to easily detect
> > pagefaults from userspace).  
> 
> The dumb approach we do is just thrash memory and check nothing has blown
> up (which the runner does by looking at the dmesg and a few proc files).
> If you run that on a kernel with all debugging enabled, it's pretty good
> at catching issues.

We could also check the fence state (assuming it's signaled with an
error, which I'm not sure is the case right now).

> 
> For added nastiness lots of interrupts to check error paths/syscall
> restarting, and at the end of the testcase, some sanity check that all the
> bo still contain what you think they should contain.

Okay, but that requires a GPU job (vertex or fragment shader) touching
a BO. Apparently we haven't done that for panfrost IGT tests yet, and
I'm not sure how to approach that. Should we manually forge a cmdstream
and submit it?

Re: [PATCH 7/8] drm/panfrost: Add the panfrost_gem_mapping concept

[Daniel Vetter]

On Mon, Dec 2, 2019 at 10:13 AM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Mon, 2 Dec 2019 09:55:32 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Fri, Nov 29, 2019 at 10:36:29PM +0100, Boris Brezillon wrote:
> > > On Fri, 29 Nov 2019 21:14:59 +0100
> > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > > On Fri, Nov 29, 2019 at 02:59:07PM +0100, Boris Brezillon wrote:
> > > > > With the introduction of per-FD address space, the same BO can be mapped
> > > > > in different address space if the BO is globally visible (GEM_FLINK)
> > > >
> > > > Also dma-buf self-imports for wayland/dri3 ...
> > >
> > > Indeed, I'll extend the commit message to mention that case.
> > >
> > > >
> > > > > and opened in different context. The current implementation does not
> > > > > take case into account, and attaches the mapping directly to the
> > > > > panfrost_gem_object.
> > > > >
> > > > > Let's create a panfrost_gem_mapping struct and allow multiple mappings
> > > > > per BO.
> > > > >
> > > > > The mappings are refcounted, which helps solve another problem where
> > > > > mappings were teared down (GEM handle closed by userspace) while GPU
> > > > > jobs accessing those BOs were still in-flight. Jobs now keep a
> > > > > reference on the mappings they use.
> > > >
> > > > uh what.
> > > >
> > > > tbh this sounds bad enough (as in how did a desktop on panfrost ever work)
> > >
> > > Well, we didn't discover this problem until recently because:
> > >
> > > 1/ We have a BO cache in mesa, and until recently, this cache could
> > > only grow (no entry eviction and no MADVISE support), meaning that BOs
> > > were staying around forever until the app was killed.
> >
> > Uh, so where was the userspace when we merged this?
>
> Well, userspace was there, it's just that we probably didn't stress
> the implementation as it should have been when doing the changes
> described in #1, #2 and 3.
>
> >
> > > 2/ Mappings were teared down at BO destruction time before commit
> > > a5efb4c9a562 ("drm/panfrost: Restructure the GEM object creation"), and
> > > jobs are retaining references to all the BO they access.
> > >
> > > 3/ The mesa driver was serializing GPU jobs, and only releasing the BO
> > > reference when the job was done (wait on the completion fence). This
> > > has recently been changed, and now BOs are returned to the cache as
> > > soon as the job has been submitted to the kernel. When that
> > > happens,those BOs are marked purgeable which means the kernel can
> > > reclaim them when it's under memory pressure.
> > >
> > > So yes, kernel 5.4 with a recent mesa version is currently subject to
> > > GPU page-fault storms when the system starts reclaiming memory.
> > >
> > > > that I think you really want a few igts to test this stuff.
> > >
> > > I'll see what I can come up with (not sure how to easily detect
> > > pagefaults from userspace).
> >
> > The dumb approach we do is just thrash memory and check nothing has blown
> > up (which the runner does by looking at the dmesg and a few proc files).
> > If you run that on a kernel with all debugging enabled, it's pretty good
> > at catching issues.
>
> We could also check the fence state (assuming it's signaled with an
> error, which I'm not sure is the case right now).
>
> >
> > For added nastiness lots of interrupts to check error paths/syscall
> > restarting, and at the end of the testcase, some sanity check that all the
> > bo still contain what you think they should contain.
>
> Okay, but that requires a GPU job (vertex or fragment shader) touching
> a BO. Apparently we haven't done that for panfrost IGT tests yet, and
> I'm not sure how to approach that. Should we manually forge a cmdstream
> and submit it?

Yeah that's what we do all the time in i915 igts. Usually a simple
commandstream dword write (if you have that somewhere) is good enough
for tests. We also have a 2d blitter engine, plus a library for
issuing copies using the rendercopy.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/8] drm/panfrost: Make sure imported/exported BOs are never purged

[Boris Brezillon]

On Mon, 2 Dec 2019 09:52:43 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Nov 29, 2019 at 10:09:24PM +0100, Boris Brezillon wrote:
> > On Fri, 29 Nov 2019 21:12:13 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >   
> > > On Fri, Nov 29, 2019 at 02:59:06PM +0100, Boris Brezillon wrote:  
> > > > We don't want imported/exported BOs to be purges, as those are shared
> > > > with other processes that might still use them. We should also refuse
> > > > to export a BO if it's been marked purgeable or has already been purged.
> > > > 
> > > > Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> > > > Cc: <stable@vger.kernel.org>
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > > ---
> > > >  drivers/gpu/drm/panfrost/panfrost_drv.c | 19 ++++++++++++++++-
> > > >  drivers/gpu/drm/panfrost/panfrost_gem.c | 27 +++++++++++++++++++++++++
> > > >  2 files changed, 45 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > index 1c67ac434e10..751df975534f 100644
> > > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > @@ -343,6 +343,7 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> > > >  	struct drm_panfrost_madvise *args = data;
> > > >  	struct panfrost_device *pfdev = dev->dev_private;
> > > >  	struct drm_gem_object *gem_obj;
> > > > +	int ret;
> > > >  
> > > >  	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
> > > >  	if (!gem_obj) {
> > > > @@ -350,6 +351,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> > > >  		return -ENOENT;
> > > >  	}
> > > >  
> > > > +	/*
> > > > +	 * We don't want to mark exported/imported BOs as purgeable: we're not
> > > > +	 * the only owner in that case.
> > > > +	 */
> > > > +	mutex_lock(&dev->object_name_lock);    
> > > 
> > > Kinda not awesome that you have to take this core lock here and encumber
> > > core drm locking with random driver stuff.  
> > 
> > Looks like drm_gem_shmem_is_purgeable() already does the !imported &&
> > !exported check. For the imported case, I think we're good, since
> > userspace can't change the madv value before ->import_attach has been
> > set. For the exporter case, we need to make sure there's no race
> > between the export and madvise operations, which I can probably do from
> > the gem->export() hook by taking the shrinker or bo->mappings lock.

Okay, I tried that, and I actually need an extra
panfrost_gem_object->exported field that's set to true from the
->export() hook with the mappings lock held, otherwise the code is
still racy (->dma_buf is assigned after ->export() returns).

> >   
> > > 
> > > Can't this be solved with your own locking only and some reasonable
> > > ordering of checks? big locks because it's easy is endless long-term pain.
> > > 
> > > Also exporting purgeable objects is kinda a userspace bug, all you have to
> > > do is not oops in dma_buf_attachment_map. No need to prevent the damage
> > > here imo.  
> > 
> > I feel like making sure an exported BO can't be purged or a purged BO
> > can't be exported would be much simpler than making sure all importers
> > are ready to have the sgt freed.  
> 
> If you free the sgt while someone is using it, that's kinda a different
> bug I think. You already have a pages refcount, that should be enough to
> prevent this?

My bad, I thought drm_gem_shmem_get_pages_sgt() was used as the
->get_sg_table() implem, but we actually use
drm_gem_shmem_get_sg_table() which allocates a new sgt. I still need to
make sure we're safe against sgt destruction in panfrost.

Re: [PATCH 8/8] drm/panfrost: Make sure the shrinker does not reclaim referenced BOs

[Robin Murphy]

On 29/11/2019 1:59 pm, Boris Brezillon wrote:
> Userspace might tag a BO purgeable while it's still referenced by GPU
> jobs. We need to make sure the shrinker does not purge such BOs until
> all jobs referencing it are finished.

Nit: for extra robustness, perhaps it's worth using the refcount_t API 
rather than bare atomic_t?

Robin.

> Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>   drivers/gpu/drm/panfrost/panfrost_drv.c          | 1 +
>   drivers/gpu/drm/panfrost/panfrost_gem.h          | 6 ++++++
>   drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c | 2 ++
>   drivers/gpu/drm/panfrost/panfrost_job.c          | 7 ++++++-
>   4 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index b406b5243b40..297c0e7304d2 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -166,6 +166,7 @@ panfrost_lookup_bos(struct drm_device *dev,
>   			break;
>   		}
>   
> +		atomic_inc(&bo->gpu_usecount);
>   		job->mappings[i] = mapping;
>   	}
>   
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index ca1bc9019600..b3517ff9630c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -30,6 +30,12 @@ struct panfrost_gem_object {
>   		struct mutex lock;
>   	} mappings;
>   
> +	/*
> +	 * Count the number of jobs referencing this BO so we don't let the
> +	 * shrinker reclaim this object prematurely.
> +	 */
> +	atomic_t gpu_usecount;
> +
>   	bool noexec		:1;
>   	bool is_heap		:1;
>   };
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> index b36df326c860..288e46c40673 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> @@ -41,6 +41,8 @@ static bool panfrost_gem_purge(struct drm_gem_object *obj)
>   	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>   	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
>   
> +	if (atomic_read(&bo->gpu_usecount))
> +		return false;
>   
>   	if (!mutex_trylock(&shmem->pages_lock))
>   		return false;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index c85d45be3b5e..2b12aa87ff32 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -270,8 +270,13 @@ static void panfrost_job_cleanup(struct kref *ref)
>   	dma_fence_put(job->render_done_fence);
>   
>   	if (job->mappings) {
> -		for (i = 0; i < job->bo_count; i++)
> +		for (i = 0; i < job->bo_count; i++) {
> +			if (!job->mappings[i])
> +				break;
> +
> +			atomic_dec(&job->mappings[i]->obj->gpu_usecount);
>   			panfrost_gem_mapping_put(job->mappings[i]);
> +		}
>   		kvfree(job->mappings);
>   	}
>   
> 

Re: [PATCH 8/8] drm/panfrost: Make sure the shrinker does not reclaim referenced BOs

[Boris Brezillon]

On Mon, 2 Dec 2019 12:50:20 +0000
Robin Murphy <robin.murphy@arm.com> wrote:

> On 29/11/2019 1:59 pm, Boris Brezillon wrote:
> > Userspace might tag a BO purgeable while it's still referenced by GPU
> > jobs. We need to make sure the shrinker does not purge such BOs until
> > all jobs referencing it are finished.  
> 
> Nit: for extra robustness, perhaps it's worth using the refcount_t API 
> rather than bare atomic_t?

I considered doing that. The problem is, we start counting from 0, not
1, and the refcount API assumes counters start at 0, and should never
see a 0 -> 1 transition. I guess we could do

	if (refcount_inc_not_zero()) {
		...
	} else {
		refcount_set(1);
		...
	}

so we at least get the integer overflow/underflow protection.

Anyway, I'm reworking the gem_shmem code so we can refcount the sgt
users (I actually re-use the page_use_count counter and turn into a
refcount_t so we don't need to take the lock if it's > 0). With this
change I think I won't need the gpu_usecount.

Re: [PATCH 7/8] drm/panfrost: Add the panfrost_gem_mapping concept

[Steven Price]

On 02/12/2019 09:44, Daniel Vetter wrote:
> On Mon, Dec 2, 2019 at 10:13 AM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
>>
>> On Mon, 2 Dec 2019 09:55:32 +0100
>> Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>>> On Fri, Nov 29, 2019 at 10:36:29PM +0100, Boris Brezillon wrote:
>>>> On Fri, 29 Nov 2019 21:14:59 +0100
>>>> Daniel Vetter <daniel@ffwll.ch> wrote:
>>>>
>>>>> On Fri, Nov 29, 2019 at 02:59:07PM +0100, Boris Brezillon wrote:
>>>>>> With the introduction of per-FD address space, the same BO can be mapped
>>>>>> in different address space if the BO is globally visible (GEM_FLINK)
>>>>>
>>>>> Also dma-buf self-imports for wayland/dri3 ...
>>>>
>>>> Indeed, I'll extend the commit message to mention that case.
>>>>
>>>>>
>>>>>> and opened in different context. The current implementation does not
>>>>>> take case into account, and attaches the mapping directly to the
>>>>>> panfrost_gem_object.
>>>>>>
>>>>>> Let's create a panfrost_gem_mapping struct and allow multiple mappings
>>>>>> per BO.
>>>>>>
>>>>>> The mappings are refcounted, which helps solve another problem where
>>>>>> mappings were teared down (GEM handle closed by userspace) while GPU
>>>>>> jobs accessing those BOs were still in-flight. Jobs now keep a
>>>>>> reference on the mappings they use.
>>>>>
>>>>> uh what.
>>>>>
>>>>> tbh this sounds bad enough (as in how did a desktop on panfrost ever work)
>>>>
>>>> Well, we didn't discover this problem until recently because:
>>>>
>>>> 1/ We have a BO cache in mesa, and until recently, this cache could
>>>> only grow (no entry eviction and no MADVISE support), meaning that BOs
>>>> were staying around forever until the app was killed.
>>>
>>> Uh, so where was the userspace when we merged this?
>>
>> Well, userspace was there, it's just that we probably didn't stress
>> the implementation as it should have been when doing the changes
>> described in #1, #2 and 3.
>>
>>>
>>>> 2/ Mappings were teared down at BO destruction time before commit
>>>> a5efb4c9a562 ("drm/panfrost: Restructure the GEM object creation"), and
>>>> jobs are retaining references to all the BO they access.
>>>>
>>>> 3/ The mesa driver was serializing GPU jobs, and only releasing the BO
>>>> reference when the job was done (wait on the completion fence). This
>>>> has recently been changed, and now BOs are returned to the cache as
>>>> soon as the job has been submitted to the kernel. When that
>>>> happens,those BOs are marked purgeable which means the kernel can
>>>> reclaim them when it's under memory pressure.
>>>>
>>>> So yes, kernel 5.4 with a recent mesa version is currently subject to
>>>> GPU page-fault storms when the system starts reclaiming memory.
>>>>
>>>>> that I think you really want a few igts to test this stuff.
>>>>
>>>> I'll see what I can come up with (not sure how to easily detect
>>>> pagefaults from userspace).
>>>
>>> The dumb approach we do is just thrash memory and check nothing has blown
>>> up (which the runner does by looking at the dmesg and a few proc files).
>>> If you run that on a kernel with all debugging enabled, it's pretty good
>>> at catching issues.
>>
>> We could also check the fence state (assuming it's signaled with an
>> error, which I'm not sure is the case right now).
>>
>>>
>>> For added nastiness lots of interrupts to check error paths/syscall
>>> restarting, and at the end of the testcase, some sanity check that all the
>>> bo still contain what you think they should contain.
>>
>> Okay, but that requires a GPU job (vertex or fragment shader) touching
>> a BO. Apparently we haven't done that for panfrost IGT tests yet, and
>> I'm not sure how to approach that. Should we manually forge a cmdstream
>> and submit it?
> 
> Yeah that's what we do all the time in i915 igts. Usually a simple
> commandstream dword write (if you have that somewhere) is good enough
> for tests. We also have a 2d blitter engine, plus a library for
> issuing copies using the rendercopy.

Midgard has a "write value" job (or "set value" as Panfrost calls it).
See the "infinite job" test I submitted for IGT [1] for an example where
the job descriptor (of another job) is being modified. Although I don't
think that has actually been merged into IGT yet?

[1]
https://lists.freedesktop.org/archives/igt-dev/2019-September/016251.html

Steve

Re: [PATCH 2/8] drm/panfrost: Fix a race in panfrost_ioctl_madvise()

[Rob Herring]

On Fri, Nov 29, 2019 at 8:33 AM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Fri, 29 Nov 2019 14:24:48 +0000
> Steven Price <steven.price@arm.com> wrote:
>
> > On 29/11/2019 13:59, Boris Brezillon wrote:
> > > If 2 threads change the MADVISE property of the same BO in parallel we
> > > might end up with an shmem->madv value that's inconsistent with the
> > > presence of the BO in the shrinker list.
> >
> > I'm a bit worried from the point of view of user space sanity that you
> > observed this - but clearly the kernel should be robust!
>
> It's not something I observed, just found the race by inspecting the
> code, and I thought it was worth fixing it.

I'm not so sure there's a race. If there is, we still check madv value
when purging, so it would be harmless even if the state is
inconsistent.

> > > The easiest solution to fix that is to protect the
> > > drm_gem_shmem_madvise() call with the shrinker lock.
> > >
> > > Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> >
> > Reviewed-by: Steven Price <steven.price@arm.com>
>
> Thanks.
>
> >
> > > ---
> > >  drivers/gpu/drm/panfrost/panfrost_drv.c | 9 ++++-----
> > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > index f21bc8a7ee3a..efc0a24d1f4c 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > @@ -347,20 +347,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> > >             return -ENOENT;
> > >     }
> > >
> > > +   mutex_lock(&pfdev->shrinker_lock);
> > >     args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);

This means we now hold the shrinker_lock while we take the pages_lock.
Is lockdep happy with this change? I suspect not given all the fun I
had getting lockdep happy.

Rob

Re: [PATCH 2/8] drm/panfrost: Fix a race in panfrost_ioctl_madvise()

[Boris Brezillon]

On Thu, 5 Dec 2019 17:08:02 -0600
Rob Herring <robh+dt@kernel.org> wrote:

> On Fri, Nov 29, 2019 at 8:33 AM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > On Fri, 29 Nov 2019 14:24:48 +0000
> > Steven Price <steven.price@arm.com> wrote:
> >  
> > > On 29/11/2019 13:59, Boris Brezillon wrote:  
> > > > If 2 threads change the MADVISE property of the same BO in parallel we
> > > > might end up with an shmem->madv value that's inconsistent with the
> > > > presence of the BO in the shrinker list.  
> > >
> > > I'm a bit worried from the point of view of user space sanity that you
> > > observed this - but clearly the kernel should be robust!  
> >
> > It's not something I observed, just found the race by inspecting the
> > code, and I thought it was worth fixing it.  
> 
> I'm not so sure there's a race.

I'm pretty sure there's one:

T0				T1

lock(pages)
madv = 1
unlock(pages)

				lock(pages)
				madv = 0
				unlock(pages)

				lock(shrinker)
				remove_from_list(bo)
				unlock(shrinker)

lock(shrinker)
add_to_list(bo)
unlock(shrinker)

You end up with madv = 0 and the BO is added to the list.

> If there is, we still check madv value
> when purging, so it would be harmless even if the state is
> inconsistent.

Indeed. Note that you could also have this other situation where the BO
is marked purgeable but not present in the list. In that case it will
never be purged, but it's kinda user space fault anyway. I agree, none
of this problems are critical, and I'm fine leaving it unfixed as long
as it's documented somewhere that the race exist and is harmless.

> 
> > > > The easiest solution to fix that is to protect the
> > > > drm_gem_shmem_madvise() call with the shrinker lock.
> > > >
> > > > Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> > > > Cc: <stable@vger.kernel.org>
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> > >
> > > Reviewed-by: Steven Price <steven.price@arm.com>  
> >
> > Thanks.
> >  
> > >  
> > > > ---
> > > >  drivers/gpu/drm/panfrost/panfrost_drv.c | 9 ++++-----
> > > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > index f21bc8a7ee3a..efc0a24d1f4c 100644
> > > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > @@ -347,20 +347,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> > > >             return -ENOENT;
> > > >     }
> > > >
> > > > +   mutex_lock(&pfdev->shrinker_lock);
> > > >     args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);  
> 
> This means we now hold the shrinker_lock while we take the pages_lock.
> Is lockdep happy with this change? I suspect not given all the fun I
> had getting lockdep happy.

I have tested with lockdep enabled and it's all good from lockdep PoV
because the locks are taken in the same order in the madvise() and
schinker_scan() path (first the shrinker lock, then the pages lock).

Note that patch 7 introduces a deadlock in the shrinker path, but this
is unrelated to this shrinker lock being taken earlier in madvise
(drm_gem_put_pages() is called while the pages lock is already held).

Re: [PATCH 2/8] drm/panfrost: Fix a race in panfrost_ioctl_madvise()

[Boris Brezillon]

On Fri, 6 Dec 2019 08:53:27 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Thu, 5 Dec 2019 17:08:02 -0600
> Rob Herring <robh+dt@kernel.org> wrote:
> 
> > On Fri, Nov 29, 2019 at 8:33 AM Boris Brezillon
> > <boris.brezillon@collabora.com> wrote:  
> > >
> > > On Fri, 29 Nov 2019 14:24:48 +0000
> > > Steven Price <steven.price@arm.com> wrote:
> > >    
> > > > On 29/11/2019 13:59, Boris Brezillon wrote:    
> > > > > If 2 threads change the MADVISE property of the same BO in parallel we
> > > > > might end up with an shmem->madv value that's inconsistent with the
> > > > > presence of the BO in the shrinker list.    
> > > >
> > > > I'm a bit worried from the point of view of user space sanity that you
> > > > observed this - but clearly the kernel should be robust!    
> > >
> > > It's not something I observed, just found the race by inspecting the
> > > code, and I thought it was worth fixing it.    
> > 
> > I'm not so sure there's a race.  
> 
> I'm pretty sure there's one:
> 
> T0				T1
> 
> lock(pages)
> madv = 1
> unlock(pages)
> 
> 				lock(pages)
> 				madv = 0
> 				unlock(pages)
> 
> 				lock(shrinker)
> 				remove_from_list(bo)
> 				unlock(shrinker)
> 
> lock(shrinker)
> add_to_list(bo)
> unlock(shrinker)
> 
> You end up with madv = 0 and the BO is added to the list.
> 
> > If there is, we still check madv value
> > when purging, so it would be harmless even if the state is
> > inconsistent.  
> 
> Indeed. Note that you could also have this other situation where the BO
> is marked purgeable but not present in the list. In that case it will
> never be purged, but it's kinda user space fault anyway. I agree, none
> of this problems are critical, and I'm fine leaving it unfixed as long
> as it's documented somewhere that the race exist and is harmless.
> 
> >   
> > > > > The easiest solution to fix that is to protect the
> > > > > drm_gem_shmem_madvise() call with the shrinker lock.
> > > > >
> > > > > Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> > > > > Cc: <stable@vger.kernel.org>
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>    
> > > >
> > > > Reviewed-by: Steven Price <steven.price@arm.com>    
> > >
> > > Thanks.
> > >    
> > > >    
> > > > > ---
> > > > >  drivers/gpu/drm/panfrost/panfrost_drv.c | 9 ++++-----
> > > > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > > index f21bc8a7ee3a..efc0a24d1f4c 100644
> > > > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > > @@ -347,20 +347,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> > > > >             return -ENOENT;
> > > > >     }
> > > > >
> > > > > +   mutex_lock(&pfdev->shrinker_lock);
> > > > >     args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);    
> > 
> > This means we now hold the shrinker_lock while we take the pages_lock.
> > Is lockdep happy with this change? I suspect not given all the fun I
> > had getting lockdep happy.  
> 
> I have tested with lockdep enabled and it's all good from lockdep PoV
> because the locks are taken in the same order in the madvise() and
> schinker_scan() path (first the shrinker lock, then the pages lock).
> 
> Note that patch 7 introduces a deadlock in the shrinker path, but this
> is unrelated to this shrinker lock being taken earlier in madvise
> (drm_gem_put_pages() is called while the pages lock is already held).

My bad, there's no deadlock in this version, because we don't use
->pages_use_count to retain the page table (we just use a gpu_usecount
in patch 8 to prevent the purge). But I started working on a version
that uses ->pages_use_count instead of introducing yet another
refcount, and in this version I take/release a ref on the page table in
the mmu_map()/mmu_unmap() path. This causes a deadlock when GEM mappings
are teared down by the shrinker logic (because the pages lock is already
taken in panfrost_gem_purge())...