This series:
1. Adds common drm-shmem memory shrinker
2. Enables shrinker for VirtIO-GPU driver
3. Switches Panfrost driver to the common shrinker
Related patches:
Mesa: https://gitlab.freedesktop.org/digetx/mesa/-/commits/virgl-madvise
igt: https://gitlab.freedesktop.org/digetx/igt-gpu-tools/-/commits/virtio-madvise
https://gitlab.freedesktop.org/digetx/igt-gpu-tools/-/commits/panfrost-madvise
I'll upstream Mesa and igt patches once kernel part will be merged.
Changelog:
v8: - Rebased on top of recent linux-next that now has dma-buf locking
convention patches merged, which was blocking shmem shrinker before.
- Shmem shrinker now uses new drm_gem_lru helper.
- Dropped Steven Price t-b from the Panfrost patch because code
changed significantly since v6 and should be re-tested.
v7: - dma-buf locking convention
v6: https://lore.kernel.org/dri-devel/[email protected]/
Dmitry Osipenko (7):
drm/msm/gem: Prevent blocking within shrinker loop
drm/shmem-helper: Don't use vmap_use_count for dma-bufs
drm/shmem-helper: Switch to reservation lock
drm/shmem-helper: Add memory shrinker
drm/gem: Add drm_gem_pin_unlocked()
drm/virtio: Support memory shrinking
drm/panfrost: Switch to generic memory shrinker
drivers/gpu/drm/drm_gem.c | 38 +-
drivers/gpu/drm/drm_gem_shmem_helper.c | 648 ++++++++++++++----
drivers/gpu/drm/lima/lima_gem.c | 8 +-
drivers/gpu/drm/msm/msm_gem_shrinker.c | 8 +-
drivers/gpu/drm/panfrost/Makefile | 1 -
drivers/gpu/drm/panfrost/panfrost_device.h | 4 -
drivers/gpu/drm/panfrost/panfrost_drv.c | 26 +-
drivers/gpu/drm/panfrost/panfrost_gem.c | 33 +-
drivers/gpu/drm/panfrost/panfrost_gem.h | 9 -
.../gpu/drm/panfrost/panfrost_gem_shrinker.c | 122 ----
drivers/gpu/drm/panfrost/panfrost_job.c | 18 +-
drivers/gpu/drm/panfrost/panfrost_mmu.c | 19 +-
drivers/gpu/drm/virtio/virtgpu_drv.h | 18 +-
drivers/gpu/drm/virtio/virtgpu_gem.c | 55 ++
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 37 +
drivers/gpu/drm/virtio/virtgpu_kms.c | 9 +
drivers/gpu/drm/virtio/virtgpu_object.c | 138 +++-
drivers/gpu/drm/virtio/virtgpu_plane.c | 22 +-
drivers/gpu/drm/virtio/virtgpu_vq.c | 40 ++
include/drm/drm_device.h | 4 +
include/drm/drm_gem.h | 7 +-
include/drm/drm_gem_shmem_helper.h | 99 ++-
include/uapi/drm/virtgpu_drm.h | 14 +
23 files changed, 1003 insertions(+), 374 deletions(-)
delete mode 100644 drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
--
2.37.3
Replace Panfrost's custom memory shrinker with a common drm-shmem
memory shrinker.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/gpu/drm/panfrost/Makefile | 1 -
drivers/gpu/drm/panfrost/panfrost_device.h | 4 -
drivers/gpu/drm/panfrost/panfrost_drv.c | 19 +--
drivers/gpu/drm/panfrost/panfrost_gem.c | 33 +++--
drivers/gpu/drm/panfrost/panfrost_gem.h | 9 --
.../gpu/drm/panfrost/panfrost_gem_shrinker.c | 129 ------------------
drivers/gpu/drm/panfrost/panfrost_job.c | 18 ++-
7 files changed, 42 insertions(+), 171 deletions(-)
delete mode 100644 drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile
index 7da2b3f02ed9..11622e22cf15 100644
--- a/drivers/gpu/drm/panfrost/Makefile
+++ b/drivers/gpu/drm/panfrost/Makefile
@@ -5,7 +5,6 @@ panfrost-y := \
panfrost_device.o \
panfrost_devfreq.o \
panfrost_gem.o \
- panfrost_gem_shrinker.o \
panfrost_gpu.o \
panfrost_job.o \
panfrost_mmu.o \
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 8b25278f34c8..fe04b21fc044 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -115,10 +115,6 @@ struct panfrost_device {
atomic_t pending;
} reset;
- struct mutex shrinker_lock;
- struct list_head shrinker_list;
- struct shrinker shrinker;
-
struct panfrost_devfreq pfdevfreq;
};
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 94b8e6de34b8..fe78d5c75abf 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -160,7 +160,6 @@ panfrost_lookup_bos(struct drm_device *dev,
break;
}
- atomic_inc(&bo->gpu_usecount);
job->mappings[i] = mapping;
}
@@ -392,7 +391,6 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
{
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 = 0;
@@ -409,7 +407,6 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
if (ret)
goto out_put_object;
- mutex_lock(&pfdev->shrinker_lock);
mutex_lock(&bo->mappings.lock);
if (args->madv == PANFROST_MADV_DONTNEED) {
struct panfrost_gem_mapping *first;
@@ -435,17 +432,8 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
args->retained = drm_gem_shmem_madvise(&bo->base, args->madv);
- if (args->retained) {
- if (args->madv == PANFROST_MADV_DONTNEED)
- list_move_tail(&bo->base.madv_list,
- &pfdev->shrinker_list);
- else if (args->madv == PANFROST_MADV_WILLNEED)
- list_del_init(&bo->base.madv_list);
- }
-
out_unlock_mappings:
mutex_unlock(&bo->mappings.lock);
- mutex_unlock(&pfdev->shrinker_lock);
dma_resv_unlock(bo->base.base.resv);
out_put_object:
drm_gem_object_put(gem_obj);
@@ -577,9 +565,6 @@ static int panfrost_probe(struct platform_device *pdev)
ddev->dev_private = pfdev;
pfdev->ddev = ddev;
- mutex_init(&pfdev->shrinker_lock);
- INIT_LIST_HEAD(&pfdev->shrinker_list);
-
err = panfrost_device_init(pfdev);
if (err) {
if (err != -EPROBE_DEFER)
@@ -601,7 +586,7 @@ static int panfrost_probe(struct platform_device *pdev)
if (err < 0)
goto err_out1;
- panfrost_gem_shrinker_init(ddev);
+ drm_gem_shmem_shrinker_register(ddev, "panfrost-shrinker");
return 0;
@@ -619,8 +604,8 @@ static int panfrost_remove(struct platform_device *pdev)
struct panfrost_device *pfdev = platform_get_drvdata(pdev);
struct drm_device *ddev = pfdev->ddev;
+ drm_gem_shmem_shrinker_unregister(ddev);
drm_dev_unregister(ddev);
- panfrost_gem_shrinker_cleanup(ddev);
pm_runtime_get_sync(pfdev->dev);
pm_runtime_disable(pfdev->dev);
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 293e799e2fe8..f1436405e3a0 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -19,16 +19,6 @@ 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 we still have mappings attached to the BO, there's a problem in
* our refcounting.
@@ -209,6 +199,25 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
.vm_ops = &drm_gem_shmem_vm_ops,
};
+static int panfrost_shmem_evict(struct drm_gem_shmem_object *shmem)
+{
+ struct panfrost_gem_object *bo = to_panfrost_bo(&shmem->base);
+
+ if (!drm_gem_shmem_is_purgeable(shmem))
+ return -EOPNOTSUPP;
+
+ if (!mutex_trylock(&bo->mappings.lock))
+ return -EBUSY;
+
+ panfrost_gem_teardown_mappings_locked(bo);
+
+ drm_gem_shmem_purge(shmem);
+
+ mutex_unlock(&bo->mappings.lock);
+
+ return 0;
+}
+
/**
* panfrost_gem_create_object - Implementation of driver->gem_create_object.
* @dev: DRM device
@@ -230,6 +239,7 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t
mutex_init(&obj->mappings.lock);
obj->base.base.funcs = &panfrost_gem_funcs;
obj->base.map_wc = !pfdev->coherent;
+ obj->base.evict = panfrost_shmem_evict;
return &obj->base.base;
}
@@ -266,6 +276,9 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv,
if (ret)
return ERR_PTR(ret);
+ if (!bo->is_heap)
+ drm_gem_shmem_set_purgeable(shmem);
+
return bo;
}
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
index 8088d5fd8480..09da064f1c07 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
@@ -30,12 +30,6 @@ 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;
};
@@ -84,7 +78,4 @@ panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping);
void panfrost_gem_teardown_mappings_locked(struct panfrost_gem_object *bo);
-void panfrost_gem_shrinker_init(struct drm_device *dev);
-void panfrost_gem_shrinker_cleanup(struct drm_device *dev);
-
#endif /* __PANFROST_GEM_H__ */
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
deleted file mode 100644
index 865a989d67c8..000000000000
--- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
+++ /dev/null
@@ -1,129 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/* Copyright (C) 2019 Arm Ltd.
- *
- * Based on msm_gem_freedreno.c:
- * Copyright (C) 2016 Red Hat
- * Author: Rob Clark <[email protected]>
- */
-
-#include <linux/list.h>
-
-#include <drm/drm_device.h>
-#include <drm/drm_gem_shmem_helper.h>
-
-#include "panfrost_device.h"
-#include "panfrost_gem.h"
-#include "panfrost_mmu.h"
-
-static bool panfrost_gem_shmem_is_purgeable(struct drm_gem_shmem_object *shmem)
-{
- return (shmem->madv > 0) &&
- !shmem->pages_pin_count && shmem->sgt &&
- !shmem->base.dma_buf && !shmem->base.import_attach;
-}
-
-static unsigned long
-panfrost_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
-{
- struct panfrost_device *pfdev =
- container_of(shrinker, struct panfrost_device, shrinker);
- struct drm_gem_shmem_object *shmem;
- unsigned long count = 0;
-
- if (!mutex_trylock(&pfdev->shrinker_lock))
- return 0;
-
- list_for_each_entry(shmem, &pfdev->shrinker_list, madv_list) {
- if (panfrost_gem_shmem_is_purgeable(shmem))
- count += shmem->base.size >> PAGE_SHIFT;
- }
-
- mutex_unlock(&pfdev->shrinker_lock);
-
- return count;
-}
-
-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);
- bool ret = false;
-
- if (atomic_read(&bo->gpu_usecount))
- return false;
-
- if (!mutex_trylock(&bo->mappings.lock))
- return false;
-
- if (!dma_resv_trylock(shmem->base.resv))
- goto unlock_mappings;
-
- panfrost_gem_teardown_mappings_locked(bo);
- drm_gem_shmem_purge(&bo->base);
- ret = true;
-
- dma_resv_unlock(shmem->base.resv);
-
-unlock_mappings:
- mutex_unlock(&bo->mappings.lock);
- return ret;
-}
-
-static unsigned long
-panfrost_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
-{
- struct panfrost_device *pfdev =
- container_of(shrinker, struct panfrost_device, shrinker);
- struct drm_gem_shmem_object *shmem, *tmp;
- unsigned long freed = 0;
-
- if (!mutex_trylock(&pfdev->shrinker_lock))
- return SHRINK_STOP;
-
- list_for_each_entry_safe(shmem, tmp, &pfdev->shrinker_list, madv_list) {
- if (freed >= sc->nr_to_scan)
- break;
- if (drm_gem_shmem_is_purgeable(shmem) &&
- panfrost_gem_purge(&shmem->base)) {
- freed += shmem->base.size >> PAGE_SHIFT;
- list_del_init(&shmem->madv_list);
- }
- }
-
- mutex_unlock(&pfdev->shrinker_lock);
-
- if (freed > 0)
- pr_info_ratelimited("Purging %lu bytes\n", freed << PAGE_SHIFT);
-
- return freed;
-}
-
-/**
- * panfrost_gem_shrinker_init - Initialize panfrost shrinker
- * @dev: DRM device
- *
- * This function registers and sets up the panfrost shrinker.
- */
-void panfrost_gem_shrinker_init(struct drm_device *dev)
-{
- struct panfrost_device *pfdev = dev->dev_private;
- pfdev->shrinker.count_objects = panfrost_gem_shrinker_count;
- pfdev->shrinker.scan_objects = panfrost_gem_shrinker_scan;
- pfdev->shrinker.seeks = DEFAULT_SEEKS;
- WARN_ON(register_shrinker(&pfdev->shrinker, "drm-panfrost"));
-}
-
-/**
- * panfrost_gem_shrinker_cleanup - Clean up panfrost shrinker
- * @dev: DRM device
- *
- * This function unregisters the panfrost shrinker.
- */
-void panfrost_gem_shrinker_cleanup(struct drm_device *dev)
-{
- struct panfrost_device *pfdev = dev->dev_private;
-
- if (pfdev->shrinker.nr_deferred) {
- unregister_shrinker(&pfdev->shrinker);
- }
-}
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index dbc597ab46fb..98d9751d2b2c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -272,6 +272,19 @@ static void panfrost_attach_object_fences(struct drm_gem_object **bos,
dma_resv_add_fence(bos[i]->resv, fence, DMA_RESV_USAGE_WRITE);
}
+static int panfrost_objects_prepare(struct drm_gem_object **bos, int bo_count)
+{
+ struct panfrost_gem_object *bo;
+ int ret = 0;
+
+ while (!ret && bo_count--) {
+ bo = to_panfrost_bo(bos[bo_count]);
+ ret = bo->base.madv ? -ENOMEM : 0;
+ }
+
+ return ret;
+}
+
int panfrost_job_push(struct panfrost_job *job)
{
struct panfrost_device *pfdev = job->pfdev;
@@ -283,6 +296,10 @@ int panfrost_job_push(struct panfrost_job *job)
if (ret)
return ret;
+ ret = panfrost_objects_prepare(job->bos, job->bo_count);
+ if (ret)
+ goto unlock;
+
mutex_lock(&pfdev->sched_lock);
drm_sched_job_arm(&job->base);
@@ -324,7 +341,6 @@ static void panfrost_job_cleanup(struct kref *ref)
if (!job->mappings[i])
break;
- atomic_dec(&job->mappings[i]->obj->gpu_usecount);
panfrost_gem_mapping_put(job->mappings[i]);
}
kvfree(job->mappings);
--
2.37.3
Replace all drm-shmem locks with a GEM reservation lock. This makes locks
consistent with dma-buf locking convention where importers are responsible
for holding reservation lock for all operations performed over dma-bufs,
preventing deadlock between dma-buf importers and exporters.
Suggested-by: Daniel Vetter <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 184 +++++++-----------
drivers/gpu/drm/lima/lima_gem.c | 8 +-
drivers/gpu/drm/panfrost/panfrost_drv.c | 7 +-
.../gpu/drm/panfrost/panfrost_gem_shrinker.c | 6 +-
drivers/gpu/drm/panfrost/panfrost_mmu.c | 19 +-
include/drm/drm_gem_shmem_helper.h | 14 +-
6 files changed, 95 insertions(+), 143 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 801033b48893..d6e62f228989 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -86,8 +86,6 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
if (ret)
goto err_release;
- mutex_init(&shmem->pages_lock);
- mutex_init(&shmem->vmap_lock);
INIT_LIST_HEAD(&shmem->madv_list);
if (!private) {
@@ -139,11 +137,13 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
{
struct drm_gem_object *obj = &shmem->base;
- WARN_ON(shmem->vmap_use_count);
-
if (obj->import_attach) {
drm_prime_gem_destroy(obj, shmem->sgt);
} else {
+ dma_resv_lock(shmem->base.resv, NULL);
+
+ WARN_ON(shmem->vmap_use_count);
+
if (shmem->sgt) {
dma_unmap_sgtable(obj->dev->dev, shmem->sgt,
DMA_BIDIRECTIONAL, 0);
@@ -152,18 +152,18 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
}
if (shmem->pages)
drm_gem_shmem_put_pages(shmem);
- }
- WARN_ON(shmem->pages_use_count);
+ WARN_ON(shmem->pages_use_count);
+
+ dma_resv_unlock(shmem->base.resv);
+ }
drm_gem_object_release(obj);
- mutex_destroy(&shmem->pages_lock);
- mutex_destroy(&shmem->vmap_lock);
kfree(shmem);
}
EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
-static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
+static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
{
struct drm_gem_object *obj = &shmem->base;
struct page **pages;
@@ -194,35 +194,17 @@ static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
}
/*
- * drm_gem_shmem_get_pages - Allocate backing pages for a shmem GEM object
+ * drm_gem_shmem_put_pages - Decrease use count on the backing pages for a shmem GEM object
* @shmem: shmem GEM object
*
- * This function makes sure that backing pages exists for the shmem GEM object
- * and increases the use count.
- *
- * Returns:
- * 0 on success or a negative error code on failure.
+ * This function decreases the use count and puts the backing pages when use drops to zero.
*/
-int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
-{
- int ret;
-
- WARN_ON(shmem->base.import_attach);
-
- ret = mutex_lock_interruptible(&shmem->pages_lock);
- if (ret)
- return ret;
- ret = drm_gem_shmem_get_pages_locked(shmem);
- mutex_unlock(&shmem->pages_lock);
-
- return ret;
-}
-EXPORT_SYMBOL(drm_gem_shmem_get_pages);
-
-static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem)
+void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
{
struct drm_gem_object *obj = &shmem->base;
+ dma_resv_assert_held(shmem->base.resv);
+
if (WARN_ON_ONCE(!shmem->pages_use_count))
return;
@@ -239,19 +221,6 @@ static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem)
shmem->pages_mark_accessed_on_put);
shmem->pages = NULL;
}
-
-/*
- * drm_gem_shmem_put_pages - Decrease use count on the backing pages for a shmem GEM object
- * @shmem: shmem GEM object
- *
- * This function decreases the use count and puts the backing pages when use drops to zero.
- */
-void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
-{
- mutex_lock(&shmem->pages_lock);
- drm_gem_shmem_put_pages_locked(shmem);
- mutex_unlock(&shmem->pages_lock);
-}
EXPORT_SYMBOL(drm_gem_shmem_put_pages);
/**
@@ -266,6 +235,8 @@ EXPORT_SYMBOL(drm_gem_shmem_put_pages);
*/
int drm_gem_shmem_pin(struct drm_gem_shmem_object *shmem)
{
+ dma_resv_assert_held(shmem->base.resv);
+
WARN_ON(shmem->base.import_attach);
return drm_gem_shmem_get_pages(shmem);
@@ -281,14 +252,31 @@ EXPORT_SYMBOL(drm_gem_shmem_pin);
*/
void drm_gem_shmem_unpin(struct drm_gem_shmem_object *shmem)
{
+ dma_resv_assert_held(shmem->base.resv);
+
WARN_ON(shmem->base.import_attach);
drm_gem_shmem_put_pages(shmem);
}
EXPORT_SYMBOL(drm_gem_shmem_unpin);
-static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem,
- struct iosys_map *map)
+/*
+ * drm_gem_shmem_vmap - Create a virtual mapping for a shmem GEM object
+ * @shmem: shmem GEM object
+ * @map: Returns the kernel virtual address of the SHMEM GEM object's backing
+ * store.
+ *
+ * This function makes sure that a contiguous kernel virtual address mapping
+ * exists for the buffer backing the shmem GEM object. It hides the differences
+ * between dma-buf imported and natively allocated objects.
+ *
+ * Acquired mappings should be cleaned up by calling drm_gem_shmem_vunmap().
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
+ struct iosys_map *map)
{
struct drm_gem_object *obj = &shmem->base;
int ret = 0;
@@ -304,6 +292,8 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem,
} else {
pgprot_t prot = PAGE_KERNEL;
+ dma_resv_assert_held(shmem->base.resv);
+
if (shmem->vmap_use_count++ > 0) {
iosys_map_set_vaddr(map, shmem->vaddr);
return 0;
@@ -338,45 +328,30 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem,
return ret;
}
+EXPORT_SYMBOL(drm_gem_shmem_vmap);
/*
- * drm_gem_shmem_vmap - Create a virtual mapping for a shmem GEM object
+ * drm_gem_shmem_vunmap - Unmap a virtual mapping for a shmem GEM object
* @shmem: shmem GEM object
- * @map: Returns the kernel virtual address of the SHMEM GEM object's backing
- * store.
- *
- * This function makes sure that a contiguous kernel virtual address mapping
- * exists for the buffer backing the shmem GEM object. It hides the differences
- * between dma-buf imported and natively allocated objects.
+ * @map: Kernel virtual address where the SHMEM GEM object was mapped
*
- * Acquired mappings should be cleaned up by calling drm_gem_shmem_vunmap().
+ * This function cleans up a kernel virtual address mapping acquired by
+ * drm_gem_shmem_vmap(). The mapping is only removed when the use count drops to
+ * zero.
*
- * Returns:
- * 0 on success or a negative error code on failure.
+ * This function hides the differences between dma-buf imported and natively
+ * allocated objects.
*/
-int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
- struct iosys_map *map)
-{
- int ret;
-
- ret = mutex_lock_interruptible(&shmem->vmap_lock);
- if (ret)
- return ret;
- ret = drm_gem_shmem_vmap_locked(shmem, map);
- mutex_unlock(&shmem->vmap_lock);
-
- return ret;
-}
-EXPORT_SYMBOL(drm_gem_shmem_vmap);
-
-static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
- struct iosys_map *map)
+void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
+ struct iosys_map *map)
{
struct drm_gem_object *obj = &shmem->base;
if (obj->import_attach) {
dma_buf_vunmap(obj->import_attach->dmabuf, map);
} else {
+ dma_resv_assert_held(shmem->base.resv);
+
if (WARN_ON_ONCE(!shmem->vmap_use_count))
return;
@@ -389,26 +364,6 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
shmem->vaddr = NULL;
}
-
-/*
- * drm_gem_shmem_vunmap - Unmap a virtual mapping for a shmem GEM object
- * @shmem: shmem GEM object
- * @map: Kernel virtual address where the SHMEM GEM object was mapped
- *
- * This function cleans up a kernel virtual address mapping acquired by
- * drm_gem_shmem_vmap(). The mapping is only removed when the use count drops to
- * zero.
- *
- * This function hides the differences between dma-buf imported and natively
- * allocated objects.
- */
-void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
- struct iosys_map *map)
-{
- mutex_lock(&shmem->vmap_lock);
- drm_gem_shmem_vunmap_locked(shmem, map);
- mutex_unlock(&shmem->vmap_lock);
-}
EXPORT_SYMBOL(drm_gem_shmem_vunmap);
static struct drm_gem_shmem_object *
@@ -441,24 +396,24 @@ drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
*/
int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv)
{
- mutex_lock(&shmem->pages_lock);
+ dma_resv_assert_held(shmem->base.resv);
if (shmem->madv >= 0)
shmem->madv = madv;
madv = shmem->madv;
- mutex_unlock(&shmem->pages_lock);
-
return (madv >= 0);
}
EXPORT_SYMBOL(drm_gem_shmem_madvise);
-void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem)
+void drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem)
{
struct drm_gem_object *obj = &shmem->base;
struct drm_device *dev = obj->dev;
+ dma_resv_assert_held(shmem->base.resv);
+
WARN_ON(!drm_gem_shmem_is_purgeable(shmem));
dma_unmap_sgtable(dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0);
@@ -466,7 +421,7 @@ void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem)
kfree(shmem->sgt);
shmem->sgt = NULL;
- drm_gem_shmem_put_pages_locked(shmem);
+ drm_gem_shmem_put_pages(shmem);
shmem->madv = -1;
@@ -482,17 +437,6 @@ void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem)
invalidate_mapping_pages(file_inode(obj->filp)->i_mapping, 0, (loff_t)-1);
}
-EXPORT_SYMBOL(drm_gem_shmem_purge_locked);
-
-bool drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem)
-{
- if (!mutex_trylock(&shmem->pages_lock))
- return false;
- drm_gem_shmem_purge_locked(shmem);
- mutex_unlock(&shmem->pages_lock);
-
- return true;
-}
EXPORT_SYMBOL(drm_gem_shmem_purge);
/**
@@ -548,7 +492,7 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
/* We don't use vmf->pgoff since that has the fake offset */
page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
- mutex_lock(&shmem->pages_lock);
+ dma_resv_lock(shmem->base.resv, NULL);
if (page_offset >= num_pages ||
WARN_ON_ONCE(!shmem->pages) ||
@@ -560,7 +504,7 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
ret = vmf_insert_pfn(vma, vmf->address, page_to_pfn(page));
}
- mutex_unlock(&shmem->pages_lock);
+ dma_resv_unlock(shmem->base.resv);
return ret;
}
@@ -573,8 +517,10 @@ static void drm_gem_shmem_vm_open(struct vm_area_struct *vma)
WARN_ON(shmem->base.import_attach);
+ dma_resv_lock(shmem->base.resv, NULL);
ret = drm_gem_shmem_get_pages(shmem);
WARN_ON_ONCE(ret != 0);
+ dma_resv_unlock(shmem->base.resv);
drm_gem_vm_open(vma);
}
@@ -584,7 +530,10 @@ static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
struct drm_gem_object *obj = vma->vm_private_data;
struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
+ dma_resv_lock(shmem->base.resv, NULL);
drm_gem_shmem_put_pages(shmem);
+ dma_resv_unlock(shmem->base.resv);
+
drm_gem_vm_close(vma);
}
@@ -619,7 +568,10 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
return dma_buf_mmap(obj->dma_buf, vma, 0);
}
+ dma_resv_lock(shmem->base.resv, NULL);
ret = drm_gem_shmem_get_pages(shmem);
+ dma_resv_unlock(shmem->base.resv);
+
if (ret) {
drm_gem_vm_close(vma);
return ret;
@@ -707,9 +659,11 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
WARN_ON(obj->import_attach);
+ dma_resv_lock(shmem->base.resv, NULL);
+
ret = drm_gem_shmem_get_pages(shmem);
if (ret)
- return ERR_PTR(ret);
+ goto err_unlock;
sgt = drm_gem_shmem_get_sg_table(shmem);
if (IS_ERR(sgt)) {
@@ -723,6 +677,8 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
shmem->sgt = sgt;
+ dma_resv_unlock(shmem->base.resv);
+
return sgt;
err_free_sgt:
@@ -730,6 +686,8 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
kfree(sgt);
err_put_pages:
drm_gem_shmem_put_pages(shmem);
+err_unlock:
+ dma_resv_unlock(shmem->base.resv);
return ERR_PTR(ret);
}
EXPORT_SYMBOL_GPL(drm_gem_shmem_get_pages_sgt);
diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c
index 0f1ca0b0db49..5008f0c2428f 100644
--- a/drivers/gpu/drm/lima/lima_gem.c
+++ b/drivers/gpu/drm/lima/lima_gem.c
@@ -34,7 +34,7 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
new_size = min(new_size, bo->base.base.size);
- mutex_lock(&bo->base.pages_lock);
+ dma_resv_lock(bo->base.base.resv, NULL);
if (bo->base.pages) {
pages = bo->base.pages;
@@ -42,7 +42,7 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
pages = kvmalloc_array(bo->base.base.size >> PAGE_SHIFT,
sizeof(*pages), GFP_KERNEL | __GFP_ZERO);
if (!pages) {
- mutex_unlock(&bo->base.pages_lock);
+ dma_resv_unlock(bo->base.base.resv);
return -ENOMEM;
}
@@ -56,13 +56,13 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
struct page *page = shmem_read_mapping_page(mapping, i);
if (IS_ERR(page)) {
- mutex_unlock(&bo->base.pages_lock);
+ dma_resv_unlock(bo->base.base.resv);
return PTR_ERR(page);
}
pages[i] = page;
}
- mutex_unlock(&bo->base.pages_lock);
+ dma_resv_unlock(bo->base.base.resv);
ret = sg_alloc_table_from_pages(&sgt, pages, i, 0,
new_size, GFP_KERNEL);
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 2fa5afe21288..94b8e6de34b8 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -405,6 +405,10 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
bo = to_panfrost_bo(gem_obj);
+ ret = dma_resv_lock_interruptible(bo->base.base.resv, NULL);
+ if (ret)
+ goto out_put_object;
+
mutex_lock(&pfdev->shrinker_lock);
mutex_lock(&bo->mappings.lock);
if (args->madv == PANFROST_MADV_DONTNEED) {
@@ -442,7 +446,8 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
out_unlock_mappings:
mutex_unlock(&bo->mappings.lock);
mutex_unlock(&pfdev->shrinker_lock);
-
+ dma_resv_unlock(bo->base.base.resv);
+out_put_object:
drm_gem_object_put(gem_obj);
return ret;
}
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
index bf0170782f25..6a71a2555f85 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
@@ -48,14 +48,14 @@ static bool panfrost_gem_purge(struct drm_gem_object *obj)
if (!mutex_trylock(&bo->mappings.lock))
return false;
- if (!mutex_trylock(&shmem->pages_lock))
+ if (!dma_resv_trylock(shmem->base.resv))
goto unlock_mappings;
panfrost_gem_teardown_mappings_locked(bo);
- drm_gem_shmem_purge_locked(&bo->base);
+ drm_gem_shmem_purge(&bo->base);
ret = true;
- mutex_unlock(&shmem->pages_lock);
+ dma_resv_unlock(shmem->base.resv);
unlock_mappings:
mutex_unlock(&bo->mappings.lock);
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index e246d914e7f6..0d5a75b23ed2 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -434,6 +434,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
struct panfrost_gem_mapping *bomapping;
struct panfrost_gem_object *bo;
struct address_space *mapping;
+ struct drm_gem_object *obj;
pgoff_t page_offset;
struct sg_table *sgt;
struct page **pages;
@@ -456,15 +457,16 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
page_offset = addr >> PAGE_SHIFT;
page_offset -= bomapping->mmnode.start;
- mutex_lock(&bo->base.pages_lock);
+ obj = &bo->base.base;
+
+ dma_resv_lock(obj->resv, NULL);
if (!bo->base.pages) {
bo->sgts = kvmalloc_array(bo->base.base.size / SZ_2M,
sizeof(struct sg_table), GFP_KERNEL | __GFP_ZERO);
if (!bo->sgts) {
- mutex_unlock(&bo->base.pages_lock);
ret = -ENOMEM;
- goto err_bo;
+ goto err_unlock;
}
pages = kvmalloc_array(bo->base.base.size >> PAGE_SHIFT,
@@ -472,9 +474,8 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
if (!pages) {
kvfree(bo->sgts);
bo->sgts = NULL;
- mutex_unlock(&bo->base.pages_lock);
ret = -ENOMEM;
- goto err_bo;
+ goto err_unlock;
}
bo->base.pages = pages;
bo->base.pages_use_count = 1;
@@ -482,7 +483,6 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
pages = bo->base.pages;
if (pages[page_offset]) {
/* Pages are already mapped, bail out. */
- mutex_unlock(&bo->base.pages_lock);
goto out;
}
}
@@ -493,14 +493,11 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
for (i = page_offset; i < page_offset + NUM_FAULT_PAGES; i++) {
pages[i] = shmem_read_mapping_page(mapping, i);
if (IS_ERR(pages[i])) {
- mutex_unlock(&bo->base.pages_lock);
ret = PTR_ERR(pages[i]);
goto err_pages;
}
}
- mutex_unlock(&bo->base.pages_lock);
-
sgt = &bo->sgts[page_offset / (SZ_2M / PAGE_SIZE)];
ret = sg_alloc_table_from_pages(sgt, pages + page_offset,
NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL);
@@ -519,6 +516,8 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
dev_dbg(pfdev->dev, "mapped page fault @ AS%d %llx", as, addr);
out:
+ dma_resv_unlock(obj->resv);
+
panfrost_gem_mapping_put(bomapping);
return 0;
@@ -527,6 +526,8 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
sg_free_table(sgt);
err_pages:
drm_gem_shmem_put_pages(&bo->base);
+err_unlock:
+ dma_resv_unlock(obj->resv);
err_bo:
panfrost_gem_mapping_put(bomapping);
return ret;
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index a2201b2488c5..3b055d238584 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -26,11 +26,6 @@ struct drm_gem_shmem_object {
*/
struct drm_gem_object base;
- /**
- * @pages_lock: Protects the page table and use count
- */
- struct mutex pages_lock;
-
/**
* @pages: Page table
*/
@@ -79,11 +74,6 @@ struct drm_gem_shmem_object {
*/
struct sg_table *sgt;
- /**
- * @vmap_lock: Protects the vmap address and use count
- */
- struct mutex vmap_lock;
-
/**
* @vaddr: Kernel virtual address of the backing memory
*/
@@ -109,7 +99,6 @@ struct drm_gem_shmem_object {
struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t size);
void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem);
-int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem);
void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem);
int drm_gem_shmem_pin(struct drm_gem_shmem_object *shmem);
void drm_gem_shmem_unpin(struct drm_gem_shmem_object *shmem);
@@ -128,8 +117,7 @@ static inline bool drm_gem_shmem_is_purgeable(struct drm_gem_shmem_object *shmem
!shmem->base.dma_buf && !shmem->base.import_attach;
}
-void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem);
-bool drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem);
+void drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem);
struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem);
struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem);
--
2.37.3
DMA-buf core has its own refcounting of vmaps, use it instead of drm-shmem
counting. This change prepares drm-shmem for addition of memory shrinker
support where drm-shmem will use a single dma-buf reservation lock for
all operations performed over dma-bufs.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 35 +++++++++++++++-----------
1 file changed, 20 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 35138f8a375c..801033b48893 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -293,24 +293,22 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem,
struct drm_gem_object *obj = &shmem->base;
int ret = 0;
- if (shmem->vmap_use_count++ > 0) {
- iosys_map_set_vaddr(map, shmem->vaddr);
- return 0;
- }
-
if (obj->import_attach) {
ret = dma_buf_vmap(obj->import_attach->dmabuf, map);
if (!ret) {
if (WARN_ON(map->is_iomem)) {
dma_buf_vunmap(obj->import_attach->dmabuf, map);
- ret = -EIO;
- goto err_put_pages;
+ return -EIO;
}
- shmem->vaddr = map->vaddr;
}
} else {
pgprot_t prot = PAGE_KERNEL;
+ if (shmem->vmap_use_count++ > 0) {
+ iosys_map_set_vaddr(map, shmem->vaddr);
+ return 0;
+ }
+
ret = drm_gem_shmem_get_pages(shmem);
if (ret)
goto err_zero_use;
@@ -376,15 +374,15 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
{
struct drm_gem_object *obj = &shmem->base;
- if (WARN_ON_ONCE(!shmem->vmap_use_count))
- return;
-
- if (--shmem->vmap_use_count > 0)
- return;
-
if (obj->import_attach) {
dma_buf_vunmap(obj->import_attach->dmabuf, map);
} else {
+ if (WARN_ON_ONCE(!shmem->vmap_use_count))
+ return;
+
+ if (--shmem->vmap_use_count > 0)
+ return;
+
vunmap(shmem->vaddr);
drm_gem_shmem_put_pages(shmem);
}
@@ -646,7 +644,14 @@ void drm_gem_shmem_print_info(const struct drm_gem_shmem_object *shmem,
struct drm_printer *p, unsigned int indent)
{
drm_printf_indent(p, indent, "pages_use_count=%u\n", shmem->pages_use_count);
- drm_printf_indent(p, indent, "vmap_use_count=%u\n", shmem->vmap_use_count);
+
+ if (shmem->base.import_attach)
+ drm_printf_indent(p, indent, "vmap_use_count=%u\n",
+ shmem->base.dma_buf->vmapping_counter);
+ else
+ drm_printf_indent(p, indent, "vmap_use_count=%u\n",
+ shmem->vmap_use_count);
+
drm_printf_indent(p, indent, "vaddr=%p\n", shmem->vaddr);
}
EXPORT_SYMBOL(drm_gem_shmem_print_info);
--
2.37.3
Hi,
I have a few comments that are not really something for this patch. I'll
comment it here anyway to have them posted.
Am 06.11.22 um 00:27 schrieb Dmitry Osipenko:
> DMA-buf core has its own refcounting of vmaps, use it instead of drm-shmem
> counting. This change prepares drm-shmem for addition of memory shrinker
> support where drm-shmem will use a single dma-buf reservation lock for
> all operations performed over dma-bufs.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c | 35 +++++++++++++++-----------
> 1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 35138f8a375c..801033b48893 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -293,24 +293,22 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem,
> struct drm_gem_object *obj = &shmem->base;
> int ret = 0;
>
> - if (shmem->vmap_use_count++ > 0) {
> - iosys_map_set_vaddr(map, shmem->vaddr);
> - return 0;
> - }
> -
> if (obj->import_attach) {
We have a number of such branches in various memory managers. At some
point we should think about something like a GEM DMA-BUF object (or at
least some helpers) that represents an imported buffer. Such a ting
might be useful to avoid the duplication and branching that's going on
here and in other functions.
> ret = dma_buf_vmap(obj->import_attach->dmabuf, map);
> if (!ret) {
> if (WARN_ON(map->is_iomem)) {
I think it's OK to drop this test for is_iomem. It was useful when we
did not distinguish between I/O and system memory in most of our
helpers. But that should have been resolved by now and whatever code
still depends on it should be updated accordingly. All helpers use
iosys_map functionality to access the memory.
> dma_buf_vunmap(obj->import_attach->dmabuf, map);
> - ret = -EIO;
> - goto err_put_pages;
> + return -EIO;
> }
> - shmem->vaddr = map->vaddr;
To resolve the is_iomap issue, we'd have to store vaddr as struct
iosysm_map value.
> }
> } else {
> pgprot_t prot = PAGE_KERNEL;
>
> + if (shmem->vmap_use_count++ > 0) {
> + iosys_map_set_vaddr(map, shmem->vaddr);
> + return 0;
> + }
> +
> ret = drm_gem_shmem_get_pages(shmem);
> if (ret)
> goto err_zero_use;
> @@ -376,15 +374,15 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
> {
> struct drm_gem_object *obj = &shmem->base;
>
> - if (WARN_ON_ONCE(!shmem->vmap_use_count))
> - return;
> -
> - if (--shmem->vmap_use_count > 0)
> - return;
> -
> if (obj->import_attach) {
> dma_buf_vunmap(obj->import_attach->dmabuf, map);
> } else {
> + if (WARN_ON_ONCE(!shmem->vmap_use_count))
> + return;
> +
> + if (--shmem->vmap_use_count > 0)
> + return;
> +
> vunmap(shmem->vaddr);
> drm_gem_shmem_put_pages(shmem);
> }
> @@ -646,7 +644,14 @@ void drm_gem_shmem_print_info(const struct drm_gem_shmem_object *shmem,
> struct drm_printer *p, unsigned int indent)
> {
> drm_printf_indent(p, indent, "pages_use_count=%u\n", shmem->pages_use_count);
> - drm_printf_indent(p, indent, "vmap_use_count=%u\n", shmem->vmap_use_count);
> +
> + if (shmem->base.import_attach)
> + drm_printf_indent(p, indent, "vmap_use_count=%u\n",
> + shmem->base.dma_buf->vmapping_counter);
> + else
> + drm_printf_indent(p, indent, "vmap_use_count=%u\n",
> + shmem->vmap_use_count);
> +
Here's another case where a GEM DMA-BUF object might become helpful.
Best regards
Thomas
> drm_printf_indent(p, indent, "vaddr=%p\n", shmem->vaddr);
> }
> EXPORT_SYMBOL(drm_gem_shmem_print_info);
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
On 05/11/2022 23:27, Dmitry Osipenko wrote:
> Replace Panfrost's custom memory shrinker with a common drm-shmem
> memory shrinker.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
Sadly this triggers GPU faults under memory pressure - it looks
suspiciously like mappings are being freed while the jobs are still running.
I'm not sure I understand how the generic shrinker replicates the
"gpu_usecount" atomic that Panfrost currently has, and I'm wondering if
that's the cause?
Also just reverting this commit (so just patches 1-6) I can't actually
get Panfrost to purge any memory. So I don't think the changes (most
likely in patch 4) are quite right either.
At the moment I don't have the time to investigate in detail. But if
you've any ideas for something specific I should look at I can run more
testing.
Steve
> ---
> drivers/gpu/drm/panfrost/Makefile | 1 -
> drivers/gpu/drm/panfrost/panfrost_device.h | 4 -
> drivers/gpu/drm/panfrost/panfrost_drv.c | 19 +--
> drivers/gpu/drm/panfrost/panfrost_gem.c | 33 +++--
> drivers/gpu/drm/panfrost/panfrost_gem.h | 9 --
> .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 129 ------------------
> drivers/gpu/drm/panfrost/panfrost_job.c | 18 ++-
> 7 files changed, 42 insertions(+), 171 deletions(-)
> delete mode 100644 drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
>
> diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile
> index 7da2b3f02ed9..11622e22cf15 100644
> --- a/drivers/gpu/drm/panfrost/Makefile
> +++ b/drivers/gpu/drm/panfrost/Makefile
> @@ -5,7 +5,6 @@ panfrost-y := \
> panfrost_device.o \
> panfrost_devfreq.o \
> panfrost_gem.o \
> - panfrost_gem_shrinker.o \
> panfrost_gpu.o \
> panfrost_job.o \
> panfrost_mmu.o \
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 8b25278f34c8..fe04b21fc044 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -115,10 +115,6 @@ struct panfrost_device {
> atomic_t pending;
> } reset;
>
> - struct mutex shrinker_lock;
> - struct list_head shrinker_list;
> - struct shrinker shrinker;
> -
> struct panfrost_devfreq pfdevfreq;
> };
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 94b8e6de34b8..fe78d5c75abf 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -160,7 +160,6 @@ panfrost_lookup_bos(struct drm_device *dev,
> break;
> }
>
> - atomic_inc(&bo->gpu_usecount);
> job->mappings[i] = mapping;
> }
>
> @@ -392,7 +391,6 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> {
> 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 = 0;
> @@ -409,7 +407,6 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> if (ret)
> goto out_put_object;
>
> - mutex_lock(&pfdev->shrinker_lock);
> mutex_lock(&bo->mappings.lock);
> if (args->madv == PANFROST_MADV_DONTNEED) {
> struct panfrost_gem_mapping *first;
> @@ -435,17 +432,8 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>
> args->retained = drm_gem_shmem_madvise(&bo->base, args->madv);
>
> - if (args->retained) {
> - if (args->madv == PANFROST_MADV_DONTNEED)
> - list_move_tail(&bo->base.madv_list,
> - &pfdev->shrinker_list);
> - else if (args->madv == PANFROST_MADV_WILLNEED)
> - list_del_init(&bo->base.madv_list);
> - }
> -
> out_unlock_mappings:
> mutex_unlock(&bo->mappings.lock);
> - mutex_unlock(&pfdev->shrinker_lock);
> dma_resv_unlock(bo->base.base.resv);
> out_put_object:
> drm_gem_object_put(gem_obj);
> @@ -577,9 +565,6 @@ static int panfrost_probe(struct platform_device *pdev)
> ddev->dev_private = pfdev;
> pfdev->ddev = ddev;
>
> - mutex_init(&pfdev->shrinker_lock);
> - INIT_LIST_HEAD(&pfdev->shrinker_list);
> -
> err = panfrost_device_init(pfdev);
> if (err) {
> if (err != -EPROBE_DEFER)
> @@ -601,7 +586,7 @@ static int panfrost_probe(struct platform_device *pdev)
> if (err < 0)
> goto err_out1;
>
> - panfrost_gem_shrinker_init(ddev);
> + drm_gem_shmem_shrinker_register(ddev, "panfrost-shrinker");
>
> return 0;
>
> @@ -619,8 +604,8 @@ static int panfrost_remove(struct platform_device *pdev)
> struct panfrost_device *pfdev = platform_get_drvdata(pdev);
> struct drm_device *ddev = pfdev->ddev;
>
> + drm_gem_shmem_shrinker_unregister(ddev);
> drm_dev_unregister(ddev);
> - panfrost_gem_shrinker_cleanup(ddev);
>
> pm_runtime_get_sync(pfdev->dev);
> pm_runtime_disable(pfdev->dev);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 293e799e2fe8..f1436405e3a0 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -19,16 +19,6 @@ 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 we still have mappings attached to the BO, there's a problem in
> * our refcounting.
> @@ -209,6 +199,25 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
> .vm_ops = &drm_gem_shmem_vm_ops,
> };
>
> +static int panfrost_shmem_evict(struct drm_gem_shmem_object *shmem)
> +{
> + struct panfrost_gem_object *bo = to_panfrost_bo(&shmem->base);
> +
> + if (!drm_gem_shmem_is_purgeable(shmem))
> + return -EOPNOTSUPP;
> +
> + if (!mutex_trylock(&bo->mappings.lock))
> + return -EBUSY;
> +
> + panfrost_gem_teardown_mappings_locked(bo);
> +
> + drm_gem_shmem_purge(shmem);
> +
> + mutex_unlock(&bo->mappings.lock);
> +
> + return 0;
> +}
> +
> /**
> * panfrost_gem_create_object - Implementation of driver->gem_create_object.
> * @dev: DRM device
> @@ -230,6 +239,7 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t
> mutex_init(&obj->mappings.lock);
> obj->base.base.funcs = &panfrost_gem_funcs;
> obj->base.map_wc = !pfdev->coherent;
> + obj->base.evict = panfrost_shmem_evict;
>
> return &obj->base.base;
> }
> @@ -266,6 +276,9 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv,
> if (ret)
> return ERR_PTR(ret);
>
> + if (!bo->is_heap)
> + drm_gem_shmem_set_purgeable(shmem);
> +
> return bo;
> }
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index 8088d5fd8480..09da064f1c07 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -30,12 +30,6 @@ 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;
> };
> @@ -84,7 +78,4 @@ panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
> void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping);
> void panfrost_gem_teardown_mappings_locked(struct panfrost_gem_object *bo);
>
> -void panfrost_gem_shrinker_init(struct drm_device *dev);
> -void panfrost_gem_shrinker_cleanup(struct drm_device *dev);
> -
> #endif /* __PANFROST_GEM_H__ */
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> deleted file mode 100644
> index 865a989d67c8..000000000000
> --- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> +++ /dev/null
> @@ -1,129 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/* Copyright (C) 2019 Arm Ltd.
> - *
> - * Based on msm_gem_freedreno.c:
> - * Copyright (C) 2016 Red Hat
> - * Author: Rob Clark <[email protected]>
> - */
> -
> -#include <linux/list.h>
> -
> -#include <drm/drm_device.h>
> -#include <drm/drm_gem_shmem_helper.h>
> -
> -#include "panfrost_device.h"
> -#include "panfrost_gem.h"
> -#include "panfrost_mmu.h"
> -
> -static bool panfrost_gem_shmem_is_purgeable(struct drm_gem_shmem_object *shmem)
> -{
> - return (shmem->madv > 0) &&
> - !shmem->pages_pin_count && shmem->sgt &&
> - !shmem->base.dma_buf && !shmem->base.import_attach;
> -}
> -
> -static unsigned long
> -panfrost_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
> -{
> - struct panfrost_device *pfdev =
> - container_of(shrinker, struct panfrost_device, shrinker);
> - struct drm_gem_shmem_object *shmem;
> - unsigned long count = 0;
> -
> - if (!mutex_trylock(&pfdev->shrinker_lock))
> - return 0;
> -
> - list_for_each_entry(shmem, &pfdev->shrinker_list, madv_list) {
> - if (panfrost_gem_shmem_is_purgeable(shmem))
> - count += shmem->base.size >> PAGE_SHIFT;
> - }
> -
> - mutex_unlock(&pfdev->shrinker_lock);
> -
> - return count;
> -}
> -
> -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);
> - bool ret = false;
> -
> - if (atomic_read(&bo->gpu_usecount))
> - return false;
> -
> - if (!mutex_trylock(&bo->mappings.lock))
> - return false;
> -
> - if (!dma_resv_trylock(shmem->base.resv))
> - goto unlock_mappings;
> -
> - panfrost_gem_teardown_mappings_locked(bo);
> - drm_gem_shmem_purge(&bo->base);
> - ret = true;
> -
> - dma_resv_unlock(shmem->base.resv);
> -
> -unlock_mappings:
> - mutex_unlock(&bo->mappings.lock);
> - return ret;
> -}
> -
> -static unsigned long
> -panfrost_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
> -{
> - struct panfrost_device *pfdev =
> - container_of(shrinker, struct panfrost_device, shrinker);
> - struct drm_gem_shmem_object *shmem, *tmp;
> - unsigned long freed = 0;
> -
> - if (!mutex_trylock(&pfdev->shrinker_lock))
> - return SHRINK_STOP;
> -
> - list_for_each_entry_safe(shmem, tmp, &pfdev->shrinker_list, madv_list) {
> - if (freed >= sc->nr_to_scan)
> - break;
> - if (drm_gem_shmem_is_purgeable(shmem) &&
> - panfrost_gem_purge(&shmem->base)) {
> - freed += shmem->base.size >> PAGE_SHIFT;
> - list_del_init(&shmem->madv_list);
> - }
> - }
> -
> - mutex_unlock(&pfdev->shrinker_lock);
> -
> - if (freed > 0)
> - pr_info_ratelimited("Purging %lu bytes\n", freed << PAGE_SHIFT);
> -
> - return freed;
> -}
> -
> -/**
> - * panfrost_gem_shrinker_init - Initialize panfrost shrinker
> - * @dev: DRM device
> - *
> - * This function registers and sets up the panfrost shrinker.
> - */
> -void panfrost_gem_shrinker_init(struct drm_device *dev)
> -{
> - struct panfrost_device *pfdev = dev->dev_private;
> - pfdev->shrinker.count_objects = panfrost_gem_shrinker_count;
> - pfdev->shrinker.scan_objects = panfrost_gem_shrinker_scan;
> - pfdev->shrinker.seeks = DEFAULT_SEEKS;
> - WARN_ON(register_shrinker(&pfdev->shrinker, "drm-panfrost"));
> -}
> -
> -/**
> - * panfrost_gem_shrinker_cleanup - Clean up panfrost shrinker
> - * @dev: DRM device
> - *
> - * This function unregisters the panfrost shrinker.
> - */
> -void panfrost_gem_shrinker_cleanup(struct drm_device *dev)
> -{
> - struct panfrost_device *pfdev = dev->dev_private;
> -
> - if (pfdev->shrinker.nr_deferred) {
> - unregister_shrinker(&pfdev->shrinker);
> - }
> -}
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index dbc597ab46fb..98d9751d2b2c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -272,6 +272,19 @@ static void panfrost_attach_object_fences(struct drm_gem_object **bos,
> dma_resv_add_fence(bos[i]->resv, fence, DMA_RESV_USAGE_WRITE);
> }
>
> +static int panfrost_objects_prepare(struct drm_gem_object **bos, int bo_count)
> +{
> + struct panfrost_gem_object *bo;
> + int ret = 0;
> +
> + while (!ret && bo_count--) {
> + bo = to_panfrost_bo(bos[bo_count]);
> + ret = bo->base.madv ? -ENOMEM : 0;
> + }
> +
> + return ret;
> +}
> +
> int panfrost_job_push(struct panfrost_job *job)
> {
> struct panfrost_device *pfdev = job->pfdev;
> @@ -283,6 +296,10 @@ int panfrost_job_push(struct panfrost_job *job)
> if (ret)
> return ret;
>
> + ret = panfrost_objects_prepare(job->bos, job->bo_count);
> + if (ret)
> + goto unlock;
> +
> mutex_lock(&pfdev->sched_lock);
> drm_sched_job_arm(&job->base);
>
> @@ -324,7 +341,6 @@ static void panfrost_job_cleanup(struct kref *ref)
> if (!job->mappings[i])
> break;
>
> - atomic_dec(&job->mappings[i]->obj->gpu_usecount);
> panfrost_gem_mapping_put(job->mappings[i]);
> }
> kvfree(job->mappings);
Hello Steve,
On 11/14/22 19:54, Steven Price wrote:
> On 05/11/2022 23:27, Dmitry Osipenko wrote:
>> Replace Panfrost's custom memory shrinker with a common drm-shmem
>> memory shrinker.
>>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>
> Sadly this triggers GPU faults under memory pressure - it looks
> suspiciously like mappings are being freed while the jobs are still running.
>
> I'm not sure I understand how the generic shrinker replicates the
> "gpu_usecount" atomic that Panfrost currently has, and I'm wondering if
> that's the cause?
>
> Also just reverting this commit (so just patches 1-6) I can't actually
> get Panfrost to purge any memory. So I don't think the changes (most
> likely in patch 4) are quite right either.
>
> At the moment I don't have the time to investigate in detail. But if
> you've any ideas for something specific I should look at I can run more
> testing.
Thank you for the testing! It just occurred to me that the shrinker
callback lost the dma_resv_test_signaled() in comparison to the previous
versions of this patchset. It appeared to me that the drm_gem_lru now
checks whether reservation is busy, but it doesn't.
I saw a similar page faults once in a while when was testing the
Panfrost driver, but then couldn't reproduce the faults after applying
the IOMMU unmap range fix that Robin made recently.
I'll re-add the dma_resv_test_signaled() in v9, it was a luck that I
didn't hit it much during my testing.
--
Best regards,
Dmitry