From 61b6b9ee3b87604ae2682f1bb823973adec609fb Mon Sep 17 00:00:00 2001
From: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Date: Sat, 21 Oct 2023 20:01:37 +0300
Subject: [PATCH] drm/shmem-helper: Change sgt allocation policy
In a preparation to addition of drm-shmem memory shrinker support, change
the SGT allocation policy in this way:
1. SGT can be allocated only if shmem pages are pinned at the
time of allocation, otherwise allocation fails.
2. Drivers must ensure that pages are pinned during the time of SGT usage
and should get new SGT if pages were unpinned.
This new policy is required by the shrinker because it will move pages
to/from SWAP unless pages are pinned, invalidating SGT pointer once pages
are relocated.
Previous patches prepared drivers to the new policy.
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 55 ++++++++++++----------
drivers/gpu/drm/tests/drm_gem_shmem_test.c | 15 ++++++
2 files changed, 44 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 2adf386a38d1f..c0ab13e627a1e 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -133,6 +133,14 @@ drm_gem_shmem_free_pages(struct drm_gem_shmem_object *shmem)
{
struct drm_gem_object *obj = &shmem->base;
+ if (shmem->sgt) {
+ dma_unmap_sgtable(obj->dev->dev, shmem->sgt,
+ DMA_BIDIRECTIONAL, 0);
+ sg_free_table(shmem->sgt);
+ kfree(shmem->sgt);
+ shmem->sgt = NULL;
+ }
+
#ifdef CONFIG_X86
if (shmem->map_wc)
set_pages_array_wb(shmem->pages, obj->size >> PAGE_SHIFT);
@@ -176,24 +184,12 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
{
struct drm_gem_object *obj = &shmem->base;
- if (drm_gem_is_imported(obj)) {
+ if (drm_gem_is_imported(obj))
drm_prime_gem_destroy(obj, shmem->sgt);
- } else {
- drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count));
- if (shmem->sgt) {
- dma_unmap_sgtable(obj->dev->dev, shmem->sgt,
- DMA_BIDIRECTIONAL, 0);
- sg_free_table(shmem->sgt);
- kfree(shmem->sgt);
- }
- if (shmem->pages &&
- refcount_dec_and_test(&shmem->pages_use_count))
- drm_gem_shmem_free_pages(shmem);
-
- drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count));
- drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count));
- }
+ drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count));
+ drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count));
+ drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count));
drm_gem_object_release(obj);
kfree(shmem);
@@ -755,6 +751,9 @@ struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem)
drm_WARN_ON(obj->dev, drm_gem_is_imported(obj));
+ if (drm_WARN_ON(obj->dev, !shmem->pages))
+ return ERR_PTR(-ENOMEM);
+
return drm_prime_pages_to_sg(obj->dev, shmem->pages, obj->size >> PAGE_SHIFT);
}
EXPORT_SYMBOL_GPL(drm_gem_shmem_get_sg_table);
@@ -770,15 +769,10 @@ static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_
drm_WARN_ON(obj->dev, drm_gem_is_imported(obj));
- ret = drm_gem_shmem_get_pages_locked(shmem);
- if (ret)
- return ERR_PTR(ret);
-
sgt = drm_gem_shmem_get_sg_table(shmem);
- if (IS_ERR(sgt)) {
- ret = PTR_ERR(sgt);
- goto err_put_pages;
- }
+ if (IS_ERR(sgt))
+ return sgt;
+
/* Map the pages for use by the h/w. */
ret = dma_map_sgtable(obj->dev->dev, sgt, DMA_BIDIRECTIONAL, 0);
if (ret)
@@ -791,8 +785,6 @@ static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_
err_free_sgt:
sg_free_table(sgt);
kfree(sgt);
-err_put_pages:
- drm_gem_shmem_put_pages_locked(shmem);
return ERR_PTR(ret);
}
@@ -809,6 +801,17 @@ static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_
* and difference between dma-buf imported and natively allocated objects.
* drm_gem_shmem_get_sg_table() should not be directly called by drivers.
*
+ * Drivers should adhere to these SGT usage rules:
+ *
+ * 1. SGT should be allocated only if shmem pages are pinned at the
+ * time of allocation, otherwise allocation will fail.
+ *
+ * 2. Drivers should ensure that pages are pinned during the time of
+ * SGT usage and should get new SGT if pages were unpinned.
+ *
+ * Drivers don't own returned SGT and must take care of the SGT pointer
+ * lifetime. SGT is valid as long as GEM pages that backing SGT are pinned.
+ *
* Returns:
* A pointer to the scatter/gather table of pinned pages or errno on failure.
*/
diff --git a/drivers/gpu/drm/tests/drm_gem_shmem_test.c b/drivers/gpu/drm/tests/drm_gem_shmem_test.c
index 81cadaecdd4f6..f1a8ffb9dc266 100644
--- a/drivers/gpu/drm/tests/drm_gem_shmem_test.c
+++ b/drivers/gpu/drm/tests/drm_gem_shmem_test.c
@@ -34,6 +34,9 @@ KUNIT_DEFINE_ACTION_WRAPPER(sg_free_table_wrapper, sg_free_table,
KUNIT_DEFINE_ACTION_WRAPPER(drm_gem_shmem_free_wrapper, drm_gem_shmem_free,
struct drm_gem_shmem_object *);
+KUNIT_DEFINE_ACTION_WRAPPER(drm_gem_shmem_put_pages_wrapper, drm_gem_shmem_put_pages,
+ struct drm_gem_shmem_object *);
+
/*
* Test creating a shmem GEM object backed by shmem buffer. The test
* case succeeds if the GEM object is successfully allocated with the
@@ -247,6 +250,12 @@ static void drm_gem_shmem_test_get_sg_table(struct kunit *test)
ret = kunit_add_action_or_reset(test, drm_gem_shmem_free_wrapper, shmem);
KUNIT_ASSERT_EQ(test, ret, 0);
+ ret = drm_gem_shmem_get_pages(shmem);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ ret = kunit_add_action_or_reset(test, drm_gem_shmem_put_pages_wrapper, shmem);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
/* The scatter/gather table will be freed by drm_gem_shmem_free */
sgt = drm_gem_shmem_get_pages_sgt(shmem);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sgt);
@@ -322,6 +331,12 @@ static void drm_gem_shmem_test_purge(struct kunit *test)
ret = drm_gem_shmem_madvise_locked(shmem, 1);
KUNIT_EXPECT_TRUE(test, ret);
+ ret = drm_gem_shmem_get_pages(shmem);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ ret = kunit_add_action_or_reset(test, drm_gem_shmem_put_pages_wrapper, shmem);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
/* The scatter/gather table will be freed by drm_gem_shmem_free */
sgt = drm_gem_shmem_get_pages_sgt(shmem);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sgt);
--
GitLab