[PATCH] drm/msm: Replace custom dumb_map_offset with generic helper

Swaraj Gaikwad posted 1 patch 19 hours ago
drivers/gpu/drm/msm/msm_drv.c |  8 +++---
drivers/gpu/drm/msm/msm_gem.c | 49 -----------------------------------
drivers/gpu/drm/msm/msm_gem.h |  3 ---
3 files changed, 5 insertions(+), 55 deletions(-)
[PATCH] drm/msm: Replace custom dumb_map_offset with generic helper
Posted by Swaraj Gaikwad 19 hours ago
The msm driver implements a custom dumb_map_offset callback. This
implementation acquires the msm_gem_lock, but the underlying
drm_gem_create_mmap_offset() function is already thread-safe regarding
the VMA offset manager (it acquires the mgr->vm_lock internally).

Switching to the generic drm_gem_dumb_map_offset() helper provides
several benefits:
1. Removes the unnecessary locking overhead (locking leftovers).
2. Adds a missing check to reject mapping of imported objects, which is
   invalid for dumb buffers.
3. Allows for the removal of the msm_gem_dumb_map_offset() wrapper and
   the msm_gem_mmap_offset() helper function.

The logic from msm_gem_mmap_offset() has been inlined into
msm_ioctl_gem_info() to maintain functionality without the separate
helper.

This addresses the TODO:
"Documentation/gpu/todo.rst: Remove custom dumb_map_offset implementations"

Signed-off-by: Swaraj Gaikwad <swarajgaikwad1925@gmail.com>
---

 Compile-tested only.

 drivers/gpu/drm/msm/msm_drv.c |  8 +++---
 drivers/gpu/drm/msm/msm_gem.c | 49 -----------------------------------
 drivers/gpu/drm/msm/msm_gem.h |  3 ---
 3 files changed, 5 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 7e977fec4100..bf20550c5814 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -588,7 +588,9 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,

 	switch (args->info) {
 	case MSM_INFO_GET_OFFSET:
-		args->value = msm_gem_mmap_offset(obj);
+		ret = drm_gem_create_mmap_offset(obj);
+		if (ret == 0)
+		    args->value = drm_vma_node_offset_addr(&obj->vma_node);
 		break;
 	case MSM_INFO_GET_IOVA:
 		ret = msm_ioctl_gem_info_iova(dev, file, obj, &args->value);
@@ -836,7 +838,7 @@ static const struct drm_driver msm_driver = {
 	.open               = msm_open,
 	.postclose          = msm_postclose,
 	.dumb_create        = msm_gem_dumb_create,
-	.dumb_map_offset    = msm_gem_dumb_map_offset,
+	.dumb_map_offset    = drm_gem_dumb_map_offset,
 	.gem_prime_import   = msm_gem_prime_import,
 	.gem_prime_import_sg_table = msm_gem_prime_import_sg_table,
 #ifdef CONFIG_DEBUG_FS
@@ -859,7 +861,7 @@ static const struct drm_driver msm_kms_driver = {
 	.open               = msm_open,
 	.postclose          = msm_postclose,
 	.dumb_create        = msm_gem_dumb_create,
-	.dumb_map_offset    = msm_gem_dumb_map_offset,
+	.dumb_map_offset    = drm_gem_dumb_map_offset,
 	.gem_prime_import_sg_table = msm_gem_prime_import_sg_table,
 #ifdef CONFIG_DEBUG_FS
 	.debugfs_init       = msm_debugfs_init,
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 017411a0bf45..da74f1413f94 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -375,34 +375,6 @@ static vm_fault_t msm_gem_fault(struct vm_fault *vmf)
 	return ret;
 }

-/** get mmap offset */
-static uint64_t mmap_offset(struct drm_gem_object *obj)
-{
-	struct drm_device *dev = obj->dev;
-	int ret;
-
-	msm_gem_assert_locked(obj);
-
-	/* Make it mmapable */
-	ret = drm_gem_create_mmap_offset(obj);
-
-	if (ret) {
-		DRM_DEV_ERROR(dev->dev, "could not allocate mmap offset\n");
-		return 0;
-	}
-
-	return drm_vma_node_offset_addr(&obj->vma_node);
-}
-
-uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj)
-{
-	uint64_t offset;
-
-	msm_gem_lock(obj);
-	offset = mmap_offset(obj);
-	msm_gem_unlock(obj);
-	return offset;
-}

 static struct drm_gpuva *lookup_vma(struct drm_gem_object *obj,
 				    struct drm_gpuvm *vm)
@@ -730,27 +702,6 @@ int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
 			MSM_BO_SCANOUT | MSM_BO_WC, &args->handle, "dumb");
 }

-int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
-		uint32_t handle, uint64_t *offset)
-{
-	struct drm_gem_object *obj;
-	int ret = 0;
-
-	/* GEM does all our handle to object mapping */
-	obj = drm_gem_object_lookup(file, handle);
-	if (obj == NULL) {
-		ret = -ENOENT;
-		goto fail;
-	}
-
-	*offset = msm_gem_mmap_offset(obj);
-
-	drm_gem_object_put(obj);
-
-fail:
-	return ret;
-}
-
 static void *get_vaddr(struct drm_gem_object *obj, unsigned madv)
 {
 	struct msm_gem_object *msm_obj = to_msm_bo(obj);
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index a4cf31853c50..92ada1d69250 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -262,7 +262,6 @@ struct msm_gem_object {
 void msm_gem_vma_get(struct drm_gem_object *obj);
 void msm_gem_vma_put(struct drm_gem_object *obj);

-uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj);
 int msm_gem_prot(struct drm_gem_object *obj);
 int msm_gem_pin_vma_locked(struct drm_gem_object *obj, struct drm_gpuva *vma);
 void msm_gem_unpin_locked(struct drm_gem_object *obj);
@@ -285,8 +284,6 @@ struct page **msm_gem_pin_pages_locked(struct drm_gem_object *obj);
 void msm_gem_unpin_pages_locked(struct drm_gem_object *obj);
 int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
 		struct drm_mode_create_dumb *args);
-int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
-		uint32_t handle, uint64_t *offset);
 void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj);
 void *msm_gem_get_vaddr(struct drm_gem_object *obj);
 void *msm_gem_get_vaddr_active(struct drm_gem_object *obj);

base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
--
2.52.0
Re: [PATCH] drm/msm: Replace custom dumb_map_offset with generic helper
Posted by Dmitry Baryshkov an hour ago
On Mon, Dec 15, 2025 at 02:28:50AM +0000, Swaraj Gaikwad wrote:
> The msm driver implements a custom dumb_map_offset callback. This
> implementation acquires the msm_gem_lock, but the underlying
> drm_gem_create_mmap_offset() function is already thread-safe regarding
> the VMA offset manager (it acquires the mgr->vm_lock internally).

Other pieces are using msm_gem_lock() / msm_gem_unlock(), which
translates to dma_resv_lock() / dma_resv_unlock(), so you need to
describe, why it's still safe wrt. that code.

> 
> Switching to the generic drm_gem_dumb_map_offset() helper provides
> several benefits:
> 1. Removes the unnecessary locking overhead (locking leftovers).
> 2. Adds a missing check to reject mapping of imported objects, which is
>    invalid for dumb buffers.
> 3. Allows for the removal of the msm_gem_dumb_map_offset() wrapper and
>    the msm_gem_mmap_offset() helper function.
> 
> The logic from msm_gem_mmap_offset() has been inlined into
> msm_ioctl_gem_info() to maintain functionality without the separate
> helper.
> 
> This addresses the TODO:
> "Documentation/gpu/todo.rst: Remove custom dumb_map_offset implementations"
> 
> Signed-off-by: Swaraj Gaikwad <swarajgaikwad1925@gmail.com>
> ---
> 
>  Compile-tested only.
> 

-- 
With best wishes
Dmitry