[RESEND PATCH 0/3] drm/{exynos,xen}: Implement gem_prime_mmap with drm_gem_prime_mmap()

[RESEND PATCH 0/3] drm/{exynos,xen}: Implement gem_prime_mmap with drm_gem_prime_mmap()

[Thomas Zimmermann]

(Resending the patchset from [1]. Most drivers have already been updated and
only two drivers are left.)

Replace all remaining implementations of struct drm_driver.gem_prime_mmap
with use drm_gem_prime_mmap(). For each affected driver, put the mmap code
into struct drm_gem_object_funcs.mmap. With the latter change in place,
create struct file_operations via DEFINE_DRM_GEM_FOPS().

As next steps, drivers can be converted to use drm_gem_prime_mmap() and
drm_gem_mmap() (e.g., Tegra). The default mmap code in drm_gem_prime_mmap()
can be pushed into affected drivers or a helper function. The gem_prime_mmap
hook can probably be removed at some point.

[1] https://lore.kernel.org/dri-devel/20210609112012.10019-1-tzimmermann@suse.de/

Thomas Zimmermann (3):
  drm/exynox: Implement mmap as GEM object function
  drm/xen: Implement mmap as GEM object function
  drm: Update documentation and TODO of gem_prime_mmap hook

 Documentation/gpu/todo.rst                |  11 ---
 drivers/gpu/drm/exynos/exynos_drm_drv.c   |  13 +--
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |  20 +---
 drivers/gpu/drm/exynos/exynos_drm_gem.c   |  43 ++-------
 drivers/gpu/drm/exynos/exynos_drm_gem.h   |   5 -
 drivers/gpu/drm/xen/xen_drm_front.c       |  16 +---
 drivers/gpu/drm/xen/xen_drm_front_gem.c   | 108 +++++++++-------------
 drivers/gpu/drm/xen/xen_drm_front_gem.h   |   7 --
 include/drm/drm_drv.h                     |  11 ++-
 9 files changed, 64 insertions(+), 170 deletions(-)


base-commit: 215295e7b0a3deb2015c6d6b343b319e4f6d9a1d
prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
--
2.33.1

[PATCH 1/3] drm/exynox: Implement mmap as GEM object function

[Thomas Zimmermann]

Moving the driver-specific mmap code into a GEM object function allows
for using DRM helpers for various mmap callbacks.

The respective exynos functions are being removed. The file_operations
structure exynos_drm_driver_fops is now being created by the helper macro
DEFINE_DRM_GEM_FOPS().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c   | 13 ++-----
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 20 ++---------
 drivers/gpu/drm/exynos/exynos_drm_gem.c   | 43 +++++------------------
 drivers/gpu/drm/exynos/exynos_drm_gem.h   |  5 ---
 4 files changed, 13 insertions(+), 68 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index d8f1cf4d6b69..9743b6b17447 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -102,16 +102,7 @@ static const struct drm_ioctl_desc exynos_ioctls[] = {
 			DRM_RENDER_ALLOW),
 };
 
-static const struct file_operations exynos_drm_driver_fops = {
-	.owner		= THIS_MODULE,
-	.open		= drm_open,
-	.mmap		= exynos_drm_gem_mmap,
-	.poll		= drm_poll,
-	.read		= drm_read,
-	.unlocked_ioctl	= drm_ioctl,
-	.compat_ioctl = drm_compat_ioctl,
-	.release	= drm_release,
-};
+DEFINE_DRM_GEM_FOPS(exynos_drm_driver_fops);
 
 static const struct drm_driver exynos_drm_driver = {
 	.driver_features	= DRIVER_MODESET | DRIVER_GEM
@@ -124,7 +115,7 @@ static const struct drm_driver exynos_drm_driver = {
 	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
 	.gem_prime_import	= exynos_drm_gem_prime_import,
 	.gem_prime_import_sg_table	= exynos_drm_gem_prime_import_sg_table,
-	.gem_prime_mmap		= exynos_drm_gem_prime_mmap,
+	.gem_prime_mmap		= drm_gem_prime_mmap,
 	.ioctls			= exynos_ioctls,
 	.num_ioctls		= ARRAY_SIZE(exynos_ioctls),
 	.fops			= &exynos_drm_driver_fops,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index 5147f5929be7..02c97b9ca926 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -15,6 +15,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_prime.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/exynos_drm.h>
 
@@ -39,25 +40,8 @@ static int exynos_drm_fb_mmap(struct fb_info *info,
 	struct drm_fb_helper *helper = info->par;
 	struct exynos_drm_fbdev *exynos_fbd = to_exynos_fbdev(helper);
 	struct exynos_drm_gem *exynos_gem = exynos_fbd->exynos_gem;
-	unsigned long vm_size;
-	int ret;
-
-	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
-
-	vm_size = vma->vm_end - vma->vm_start;
-
-	if (vm_size > exynos_gem->size)
-		return -EINVAL;
 
-	ret = dma_mmap_attrs(to_dma_dev(helper->dev), vma, exynos_gem->cookie,
-			     exynos_gem->dma_addr, exynos_gem->size,
-			     exynos_gem->dma_attrs);
-	if (ret < 0) {
-		DRM_DEV_ERROR(to_dma_dev(helper->dev), "failed to mmap.\n");
-		return ret;
-	}
-
-	return 0;
+	return drm_gem_prime_mmap(&exynos_gem->base, vma);
 }
 
 static const struct fb_ops exynos_drm_fb_ops = {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index 4396224227d1..c4b63902ee7a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -17,6 +17,8 @@
 #include "exynos_drm_drv.h"
 #include "exynos_drm_gem.h"
 
+static int exynos_drm_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
+
 static int exynos_drm_alloc_buf(struct exynos_drm_gem *exynos_gem, bool kvmap)
 {
 	struct drm_device *dev = exynos_gem->base.dev;
@@ -135,6 +137,7 @@ static const struct vm_operations_struct exynos_drm_gem_vm_ops = {
 static const struct drm_gem_object_funcs exynos_drm_gem_object_funcs = {
 	.free = exynos_drm_gem_free_object,
 	.get_sg_table = exynos_drm_gem_prime_get_sg_table,
+	.mmap = exynos_drm_gem_mmap,
 	.vm_ops = &exynos_drm_gem_vm_ops,
 };
 
@@ -354,12 +357,16 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
 	return 0;
 }
 
-static int exynos_drm_gem_mmap_obj(struct drm_gem_object *obj,
-				   struct vm_area_struct *vma)
+static int exynos_drm_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
 {
 	struct exynos_drm_gem *exynos_gem = to_exynos_gem(obj);
 	int ret;
 
+	if (obj->import_attach)
+		return dma_buf_mmap(obj->dma_buf, vma, 0);
+
+	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
+
 	DRM_DEV_DEBUG_KMS(to_dma_dev(obj->dev), "flags = 0x%x\n",
 			  exynos_gem->flags);
 
@@ -385,26 +392,6 @@ static int exynos_drm_gem_mmap_obj(struct drm_gem_object *obj,
 	return ret;
 }
 
-int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
-{
-	struct drm_gem_object *obj;
-	int ret;
-
-	/* set vm_area_struct. */
-	ret = drm_gem_mmap(filp, vma);
-	if (ret < 0) {
-		DRM_ERROR("failed to mmap.\n");
-		return ret;
-	}
-
-	obj = vma->vm_private_data;
-
-	if (obj->import_attach)
-		return dma_buf_mmap(obj->dma_buf, vma, 0);
-
-	return exynos_drm_gem_mmap_obj(obj, vma);
-}
-
 /* low-level interface prime helpers */
 struct drm_gem_object *exynos_drm_gem_prime_import(struct drm_device *dev,
 					    struct dma_buf *dma_buf)
@@ -466,15 +453,3 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
 	exynos_gem->sgt = sgt;
 	return &exynos_gem->base;
 }
-
-int exynos_drm_gem_prime_mmap(struct drm_gem_object *obj,
-			      struct vm_area_struct *vma)
-{
-	int ret;
-
-	ret = drm_gem_mmap_obj(obj, obj->size, vma);
-	if (ret < 0)
-		return ret;
-
-	return exynos_drm_gem_mmap_obj(obj, vma);
-}
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h
index a23272fb96fb..79d7e1a87419 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
@@ -96,9 +96,6 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
 			       struct drm_device *dev,
 			       struct drm_mode_create_dumb *args);
 
-/* set vm_flags and we can change the vm attribute to other one at here. */
-int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
-
 /* low-level interface prime helpers */
 struct drm_gem_object *exynos_drm_gem_prime_import(struct drm_device *dev,
 					    struct dma_buf *dma_buf);
@@ -107,7 +104,5 @@ struct drm_gem_object *
 exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
 				     struct dma_buf_attachment *attach,
 				     struct sg_table *sgt);
-int exynos_drm_gem_prime_mmap(struct drm_gem_object *obj,
-			      struct vm_area_struct *vma);
 
 #endif
-- 
2.33.1

[PATCH 2/3] drm/xen: Implement mmap as GEM object function

[Thomas Zimmermann]

Moving the driver-specific mmap code into a GEM object function allows
for using DRM helpers for various mmap callbacks.

The respective xen functions are being removed. The file_operations
structure fops is now being created by the helper macro
DEFINE_DRM_GEM_FOPS().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/xen/xen_drm_front.c     |  16 +---
 drivers/gpu/drm/xen/xen_drm_front_gem.c | 108 +++++++++---------------
 drivers/gpu/drm/xen/xen_drm_front_gem.h |   7 --
 3 files changed, 44 insertions(+), 87 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front.c b/drivers/gpu/drm/xen/xen_drm_front.c
index 9f14d99c763c..434064c820e8 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.c
+++ b/drivers/gpu/drm/xen/xen_drm_front.c
@@ -469,19 +469,7 @@ static void xen_drm_drv_release(struct drm_device *dev)
 	kfree(drm_info);
 }
 
-static const struct file_operations xen_drm_dev_fops = {
-	.owner          = THIS_MODULE,
-	.open           = drm_open,
-	.release        = drm_release,
-	.unlocked_ioctl = drm_ioctl,
-#ifdef CONFIG_COMPAT
-	.compat_ioctl   = drm_compat_ioctl,
-#endif
-	.poll           = drm_poll,
-	.read           = drm_read,
-	.llseek         = no_llseek,
-	.mmap           = xen_drm_front_gem_mmap,
-};
+DEFINE_DRM_GEM_FOPS(xen_drm_dev_fops);
 
 static const struct drm_driver xen_drm_driver = {
 	.driver_features           = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
@@ -489,7 +477,7 @@ static const struct drm_driver xen_drm_driver = {
 	.prime_handle_to_fd        = drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle        = drm_gem_prime_fd_to_handle,
 	.gem_prime_import_sg_table = xen_drm_front_gem_import_sg_table,
-	.gem_prime_mmap            = xen_drm_front_gem_prime_mmap,
+	.gem_prime_mmap            = drm_gem_prime_mmap,
 	.dumb_create               = xen_drm_drv_dumb_create,
 	.fops                      = &xen_drm_dev_fops,
 	.name                      = "xendrm-du",
diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index b293c67230ef..dd358ba2bf8e 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -57,6 +57,47 @@ static void gem_free_pages_array(struct xen_gem_object *xen_obj)
 	xen_obj->pages = NULL;
 }
 
+static int xen_drm_front_gem_object_mmap(struct drm_gem_object *gem_obj,
+					 struct vm_area_struct *vma)
+{
+	struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
+	int ret;
+
+	vma->vm_ops = gem_obj->funcs->vm_ops;
+
+	/*
+	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
+	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
+	 * the whole buffer.
+	 */
+	vma->vm_flags &= ~VM_PFNMAP;
+	vma->vm_flags |= VM_MIXEDMAP;
+	vma->vm_pgoff = 0;
+
+	/*
+	 * According to Xen on ARM ABI (xen/include/public/arch-arm.h):
+	 * all memory which is shared with other entities in the system
+	 * (including the hypervisor and other guests) must reside in memory
+	 * which is mapped as Normal Inner Write-Back Outer Write-Back
+	 * Inner-Shareable.
+	 */
+	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
+
+	/*
+	 * vm_operations_struct.fault handler will be called if CPU access
+	 * to VM is here. For GPUs this isn't the case, because CPU  doesn't
+	 * touch the memory. Insert pages now, so both CPU and GPU are happy.
+	 *
+	 * FIXME: as we insert all the pages now then no .fault handler must
+	 * be called, so don't provide one
+	 */
+	ret = vm_map_pages(vma, xen_obj->pages, xen_obj->num_pages);
+	if (ret < 0)
+		DRM_ERROR("Failed to map pages into vma: %d\n", ret);
+
+	return ret;
+}
+
 static const struct vm_operations_struct xen_drm_drv_vm_ops = {
 	.open           = drm_gem_vm_open,
 	.close          = drm_gem_vm_close,
@@ -67,6 +108,7 @@ static const struct drm_gem_object_funcs xen_drm_front_gem_object_funcs = {
 	.get_sg_table = xen_drm_front_gem_get_sg_table,
 	.vmap = xen_drm_front_gem_prime_vmap,
 	.vunmap = xen_drm_front_gem_prime_vunmap,
+	.mmap = xen_drm_front_gem_object_mmap,
 	.vm_ops = &xen_drm_drv_vm_ops,
 };
 
@@ -238,58 +280,6 @@ xen_drm_front_gem_import_sg_table(struct drm_device *dev,
 	return &xen_obj->base;
 }
 
-static int gem_mmap_obj(struct xen_gem_object *xen_obj,
-			struct vm_area_struct *vma)
-{
-	int ret;
-
-	/*
-	 * clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
-	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
-	 * the whole buffer.
-	 */
-	vma->vm_flags &= ~VM_PFNMAP;
-	vma->vm_flags |= VM_MIXEDMAP;
-	vma->vm_pgoff = 0;
-	/*
-	 * According to Xen on ARM ABI (xen/include/public/arch-arm.h):
-	 * all memory which is shared with other entities in the system
-	 * (including the hypervisor and other guests) must reside in memory
-	 * which is mapped as Normal Inner Write-Back Outer Write-Back
-	 * Inner-Shareable.
-	 */
-	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
-
-	/*
-	 * vm_operations_struct.fault handler will be called if CPU access
-	 * to VM is here. For GPUs this isn't the case, because CPU
-	 * doesn't touch the memory. Insert pages now, so both CPU and GPU are
-	 * happy.
-	 * FIXME: as we insert all the pages now then no .fault handler must
-	 * be called, so don't provide one
-	 */
-	ret = vm_map_pages(vma, xen_obj->pages, xen_obj->num_pages);
-	if (ret < 0)
-		DRM_ERROR("Failed to map pages into vma: %d\n", ret);
-
-	return ret;
-}
-
-int xen_drm_front_gem_mmap(struct file *filp, struct vm_area_struct *vma)
-{
-	struct xen_gem_object *xen_obj;
-	struct drm_gem_object *gem_obj;
-	int ret;
-
-	ret = drm_gem_mmap(filp, vma);
-	if (ret < 0)
-		return ret;
-
-	gem_obj = vma->vm_private_data;
-	xen_obj = to_xen_gem_obj(gem_obj);
-	return gem_mmap_obj(xen_obj, vma);
-}
-
 int xen_drm_front_gem_prime_vmap(struct drm_gem_object *gem_obj, struct dma_buf_map *map)
 {
 	struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
@@ -313,17 +303,3 @@ void xen_drm_front_gem_prime_vunmap(struct drm_gem_object *gem_obj,
 {
 	vunmap(map->vaddr);
 }
-
-int xen_drm_front_gem_prime_mmap(struct drm_gem_object *gem_obj,
-				 struct vm_area_struct *vma)
-{
-	struct xen_gem_object *xen_obj;
-	int ret;
-
-	ret = drm_gem_mmap_obj(gem_obj, gem_obj->size, vma);
-	if (ret < 0)
-		return ret;
-
-	xen_obj = to_xen_gem_obj(gem_obj);
-	return gem_mmap_obj(xen_obj, vma);
-}
diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.h b/drivers/gpu/drm/xen/xen_drm_front_gem.h
index a4e67d0a149c..eaea470f7001 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.h
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.h
@@ -15,9 +15,7 @@ struct dma_buf_attachment;
 struct dma_buf_map;
 struct drm_device;
 struct drm_gem_object;
-struct file;
 struct sg_table;
-struct vm_area_struct;
 
 struct drm_gem_object *xen_drm_front_gem_create(struct drm_device *dev,
 						size_t size);
@@ -33,15 +31,10 @@ struct page **xen_drm_front_gem_get_pages(struct drm_gem_object *obj);
 
 void xen_drm_front_gem_free_object_unlocked(struct drm_gem_object *gem_obj);
 
-int xen_drm_front_gem_mmap(struct file *filp, struct vm_area_struct *vma);
-
 int xen_drm_front_gem_prime_vmap(struct drm_gem_object *gem_obj,
 				 struct dma_buf_map *map);
 
 void xen_drm_front_gem_prime_vunmap(struct drm_gem_object *gem_obj,
 				    struct dma_buf_map *map);
 
-int xen_drm_front_gem_prime_mmap(struct drm_gem_object *gem_obj,
-				 struct vm_area_struct *vma);
-
 #endif /* __XEN_DRM_FRONT_GEM_H */
-- 
2.33.1

[PATCH 3/3] drm: Update documentation and TODO of gem_prime_mmap hook

[Thomas Zimmermann]

The hook gem_prime_mmap in struct drm_driver is deprecated. Document
the new requirements.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 Documentation/gpu/todo.rst | 11 -----------
 include/drm/drm_drv.h      | 11 +++++++----
 2 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 60d1d7ee0719..6593c4c4006e 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -268,17 +268,6 @@ Contact: Daniel Vetter
 
 Level: Intermediate
 
-Clean up mmap forwarding
-------------------------
-
-A lot of drivers forward gem mmap calls to dma-buf mmap for imported buffers.
-And also a lot of them forward dma-buf mmap to the gem mmap implementations.
-There's drm_gem_prime_mmap() for this now, but still needs to be rolled out.
-
-Contact: Daniel Vetter
-
-Level: Intermediate
-
 Generic fbdev defio support
 ---------------------------
 
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 0cd95953cdf5..a84eb4028e5b 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -345,11 +345,14 @@ struct drm_driver {
 	 * mmap hook for GEM drivers, used to implement dma-buf mmap in the
 	 * PRIME helpers.
 	 *
-	 * FIXME: There's way too much duplication going on here, and also moved
-	 * to &drm_gem_object_funcs.
+	 * This hook only exists for historical reasons. Drivers must use
+	 * drm_gem_prime_mmap() to implement it.
+	 *
+	 * FIXME: Convert all drivers to implement mmap in struct
+	 * &drm_gem_object_funcs and inline drm_gem_prime_mmap() into
+	 * its callers. This hook should be removed afterwards.
 	 */
-	int (*gem_prime_mmap)(struct drm_gem_object *obj,
-				struct vm_area_struct *vma);
+	int (*gem_prime_mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma);
 
 	/**
 	 * @dumb_create:
-- 
2.33.1

Re: [PATCH 2/3] drm/xen: Implement mmap as GEM object function

[Oleksandr Andrushchenko]

Hi, Thomas!

On 08.11.21 12:28, Thomas Zimmermann wrote:
> Moving the driver-specific mmap code into a GEM object function allows
> for using DRM helpers for various mmap callbacks.
>
> The respective xen functions are being removed. The file_operations
> structure fops is now being created by the helper macro
> DEFINE_DRM_GEM_FOPS().
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
>   drivers/gpu/drm/xen/xen_drm_front.c     |  16 +---
>   drivers/gpu/drm/xen/xen_drm_front_gem.c | 108 +++++++++---------------
>   drivers/gpu/drm/xen/xen_drm_front_gem.h |   7 --
>   3 files changed, 44 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/gpu/drm/xen/xen_drm_front.c b/drivers/gpu/drm/xen/xen_drm_front.c
> index 9f14d99c763c..434064c820e8 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front.c
> @@ -469,19 +469,7 @@ static void xen_drm_drv_release(struct drm_device *dev)
>   	kfree(drm_info);
>   }
>   
> -static const struct file_operations xen_drm_dev_fops = {
> -	.owner          = THIS_MODULE,
> -	.open           = drm_open,
> -	.release        = drm_release,
> -	.unlocked_ioctl = drm_ioctl,
> -#ifdef CONFIG_COMPAT
> -	.compat_ioctl   = drm_compat_ioctl,
> -#endif
> -	.poll           = drm_poll,
> -	.read           = drm_read,
> -	.llseek         = no_llseek,
> -	.mmap           = xen_drm_front_gem_mmap,
> -};
> +DEFINE_DRM_GEM_FOPS(xen_drm_dev_fops);
>   
>   static const struct drm_driver xen_drm_driver = {
>   	.driver_features           = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> @@ -489,7 +477,7 @@ static const struct drm_driver xen_drm_driver = {
>   	.prime_handle_to_fd        = drm_gem_prime_handle_to_fd,
>   	.prime_fd_to_handle        = drm_gem_prime_fd_to_handle,
>   	.gem_prime_import_sg_table = xen_drm_front_gem_import_sg_table,
> -	.gem_prime_mmap            = xen_drm_front_gem_prime_mmap,
> +	.gem_prime_mmap            = drm_gem_prime_mmap,
>   	.dumb_create               = xen_drm_drv_dumb_create,
>   	.fops                      = &xen_drm_dev_fops,
>   	.name                      = "xendrm-du",
> diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c b/drivers/gpu/drm/xen/xen_drm_front_gem.c
> index b293c67230ef..dd358ba2bf8e 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
> @@ -57,6 +57,47 @@ static void gem_free_pages_array(struct xen_gem_object *xen_obj)
>   	xen_obj->pages = NULL;
>   }
>   
> +static int xen_drm_front_gem_object_mmap(struct drm_gem_object *gem_obj,
> +					 struct vm_area_struct *vma)
> +{
> +	struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
> +	int ret;
> +
> +	vma->vm_ops = gem_obj->funcs->vm_ops;
> +
> +	/*
> +	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
> +	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
> +	 * the whole buffer.
> +	 */
> +	vma->vm_flags &= ~VM_PFNMAP;
> +	vma->vm_flags |= VM_MIXEDMAP;
> +	vma->vm_pgoff = 0;
> +
> +	/*
> +	 * According to Xen on ARM ABI (xen/include/public/arch-arm.h):
> +	 * all memory which is shared with other entities in the system
> +	 * (including the hypervisor and other guests) must reside in memory
> +	 * which is mapped as Normal Inner Write-Back Outer Write-Back
> +	 * Inner-Shareable.
> +	 */
> +	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> +
> +	/*
> +	 * vm_operations_struct.fault handler will be called if CPU access
> +	 * to VM is here. For GPUs this isn't the case, because CPU  doesn't
> +	 * touch the memory. Insert pages now, so both CPU and GPU are happy.
> +	 *
> +	 * FIXME: as we insert all the pages now then no .fault handler must
> +	 * be called, so don't provide one
> +	 */
> +	ret = vm_map_pages(vma, xen_obj->pages, xen_obj->num_pages);
> +	if (ret < 0)
> +		DRM_ERROR("Failed to map pages into vma: %d\n", ret);
> +
> +	return ret;
> +}
> +
>   static const struct vm_operations_struct xen_drm_drv_vm_ops = {
>   	.open           = drm_gem_vm_open,
>   	.close          = drm_gem_vm_close,
> @@ -67,6 +108,7 @@ static const struct drm_gem_object_funcs xen_drm_front_gem_object_funcs = {
>   	.get_sg_table = xen_drm_front_gem_get_sg_table,
>   	.vmap = xen_drm_front_gem_prime_vmap,
>   	.vunmap = xen_drm_front_gem_prime_vunmap,
> +	.mmap = xen_drm_front_gem_object_mmap,
>   	.vm_ops = &xen_drm_drv_vm_ops,
>   };
>   
> @@ -238,58 +280,6 @@ xen_drm_front_gem_import_sg_table(struct drm_device *dev,
>   	return &xen_obj->base;
>   }
>   
> -static int gem_mmap_obj(struct xen_gem_object *xen_obj,
> -			struct vm_area_struct *vma)
> -{
> -	int ret;
> -
> -	/*
> -	 * clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
> -	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
> -	 * the whole buffer.
> -	 */
> -	vma->vm_flags &= ~VM_PFNMAP;
> -	vma->vm_flags |= VM_MIXEDMAP;
> -	vma->vm_pgoff = 0;
> -	/*
> -	 * According to Xen on ARM ABI (xen/include/public/arch-arm.h):
> -	 * all memory which is shared with other entities in the system
> -	 * (including the hypervisor and other guests) must reside in memory
> -	 * which is mapped as Normal Inner Write-Back Outer Write-Back
> -	 * Inner-Shareable.
> -	 */
> -	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> -
> -	/*
> -	 * vm_operations_struct.fault handler will be called if CPU access
> -	 * to VM is here. For GPUs this isn't the case, because CPU
> -	 * doesn't touch the memory. Insert pages now, so both CPU and GPU are
> -	 * happy.
> -	 * FIXME: as we insert all the pages now then no .fault handler must
> -	 * be called, so don't provide one
> -	 */
> -	ret = vm_map_pages(vma, xen_obj->pages, xen_obj->num_pages);
> -	if (ret < 0)
> -		DRM_ERROR("Failed to map pages into vma: %d\n", ret);
> -
> -	return ret;
> -}
> -
> -int xen_drm_front_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> -{
> -	struct xen_gem_object *xen_obj;
> -	struct drm_gem_object *gem_obj;
> -	int ret;
> -
> -	ret = drm_gem_mmap(filp, vma);
> -	if (ret < 0)
> -		return ret;
> -
> -	gem_obj = vma->vm_private_data;
> -	xen_obj = to_xen_gem_obj(gem_obj);
> -	return gem_mmap_obj(xen_obj, vma);
> -}
> -
>   int xen_drm_front_gem_prime_vmap(struct drm_gem_object *gem_obj, struct dma_buf_map *map)
>   {
>   	struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
> @@ -313,17 +303,3 @@ void xen_drm_front_gem_prime_vunmap(struct drm_gem_object *gem_obj,
>   {
>   	vunmap(map->vaddr);
>   }
> -
> -int xen_drm_front_gem_prime_mmap(struct drm_gem_object *gem_obj,
> -				 struct vm_area_struct *vma)
> -{
> -	struct xen_gem_object *xen_obj;
> -	int ret;
> -
> -	ret = drm_gem_mmap_obj(gem_obj, gem_obj->size, vma);
> -	if (ret < 0)
> -		return ret;
> -
> -	xen_obj = to_xen_gem_obj(gem_obj);
> -	return gem_mmap_obj(xen_obj, vma);
> -}
> diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.h b/drivers/gpu/drm/xen/xen_drm_front_gem.h
> index a4e67d0a149c..eaea470f7001 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front_gem.h
> +++ b/drivers/gpu/drm/xen/xen_drm_front_gem.h
> @@ -15,9 +15,7 @@ struct dma_buf_attachment;
>   struct dma_buf_map;
>   struct drm_device;
>   struct drm_gem_object;
> -struct file;
>   struct sg_table;
> -struct vm_area_struct;
>   
>   struct drm_gem_object *xen_drm_front_gem_create(struct drm_device *dev,
>   						size_t size);
> @@ -33,15 +31,10 @@ struct page **xen_drm_front_gem_get_pages(struct drm_gem_object *obj);
>   
>   void xen_drm_front_gem_free_object_unlocked(struct drm_gem_object *gem_obj);
>   
> -int xen_drm_front_gem_mmap(struct file *filp, struct vm_area_struct *vma);
> -
>   int xen_drm_front_gem_prime_vmap(struct drm_gem_object *gem_obj,
>   				 struct dma_buf_map *map);
>   
>   void xen_drm_front_gem_prime_vunmap(struct drm_gem_object *gem_obj,
>   				    struct dma_buf_map *map);
>   
> -int xen_drm_front_gem_prime_mmap(struct drm_gem_object *gem_obj,
> -				 struct vm_area_struct *vma);
> -
>   #endif /* __XEN_DRM_FRONT_GEM_H */

Re: [PATCH 2/3] drm/xen: Implement mmap as GEM object function

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 8835 bytes --]

Hi

Am 08.11.21 um 11:46 schrieb Oleksandr Andrushchenko:
> Hi, Thomas!
> 
> On 08.11.21 12:28, Thomas Zimmermann wrote:
>> Moving the driver-specific mmap code into a GEM object function allows
>> for using DRM helpers for various mmap callbacks.
>>
>> The respective xen functions are being removed. The file_operations
>> structure fops is now being created by the helper macro
>> DEFINE_DRM_GEM_FOPS().
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

That was quick! Thanks a lot.

Best regards
Thomas

>> ---
>>    drivers/gpu/drm/xen/xen_drm_front.c     |  16 +---
>>    drivers/gpu/drm/xen/xen_drm_front_gem.c | 108 +++++++++---------------
>>    drivers/gpu/drm/xen/xen_drm_front_gem.h |   7 --
>>    3 files changed, 44 insertions(+), 87 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xen/xen_drm_front.c b/drivers/gpu/drm/xen/xen_drm_front.c
>> index 9f14d99c763c..434064c820e8 100644
>> --- a/drivers/gpu/drm/xen/xen_drm_front.c
>> +++ b/drivers/gpu/drm/xen/xen_drm_front.c
>> @@ -469,19 +469,7 @@ static void xen_drm_drv_release(struct drm_device *dev)
>>    	kfree(drm_info);
>>    }
>>    
>> -static const struct file_operations xen_drm_dev_fops = {
>> -	.owner          = THIS_MODULE,
>> -	.open           = drm_open,
>> -	.release        = drm_release,
>> -	.unlocked_ioctl = drm_ioctl,
>> -#ifdef CONFIG_COMPAT
>> -	.compat_ioctl   = drm_compat_ioctl,
>> -#endif
>> -	.poll           = drm_poll,
>> -	.read           = drm_read,
>> -	.llseek         = no_llseek,
>> -	.mmap           = xen_drm_front_gem_mmap,
>> -};
>> +DEFINE_DRM_GEM_FOPS(xen_drm_dev_fops);
>>    
>>    static const struct drm_driver xen_drm_driver = {
>>    	.driver_features           = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
>> @@ -489,7 +477,7 @@ static const struct drm_driver xen_drm_driver = {
>>    	.prime_handle_to_fd        = drm_gem_prime_handle_to_fd,
>>    	.prime_fd_to_handle        = drm_gem_prime_fd_to_handle,
>>    	.gem_prime_import_sg_table = xen_drm_front_gem_import_sg_table,
>> -	.gem_prime_mmap            = xen_drm_front_gem_prime_mmap,
>> +	.gem_prime_mmap            = drm_gem_prime_mmap,
>>    	.dumb_create               = xen_drm_drv_dumb_create,
>>    	.fops                      = &xen_drm_dev_fops,
>>    	.name                      = "xendrm-du",
>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c b/drivers/gpu/drm/xen/xen_drm_front_gem.c
>> index b293c67230ef..dd358ba2bf8e 100644
>> --- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
>> +++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
>> @@ -57,6 +57,47 @@ static void gem_free_pages_array(struct xen_gem_object *xen_obj)
>>    	xen_obj->pages = NULL;
>>    }
>>    
>> +static int xen_drm_front_gem_object_mmap(struct drm_gem_object *gem_obj,
>> +					 struct vm_area_struct *vma)
>> +{
>> +	struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
>> +	int ret;
>> +
>> +	vma->vm_ops = gem_obj->funcs->vm_ops;
>> +
>> +	/*
>> +	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
>> +	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
>> +	 * the whole buffer.
>> +	 */
>> +	vma->vm_flags &= ~VM_PFNMAP;
>> +	vma->vm_flags |= VM_MIXEDMAP;
>> +	vma->vm_pgoff = 0;
>> +
>> +	/*
>> +	 * According to Xen on ARM ABI (xen/include/public/arch-arm.h):
>> +	 * all memory which is shared with other entities in the system
>> +	 * (including the hypervisor and other guests) must reside in memory
>> +	 * which is mapped as Normal Inner Write-Back Outer Write-Back
>> +	 * Inner-Shareable.
>> +	 */
>> +	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>> +
>> +	/*
>> +	 * vm_operations_struct.fault handler will be called if CPU access
>> +	 * to VM is here. For GPUs this isn't the case, because CPU  doesn't
>> +	 * touch the memory. Insert pages now, so both CPU and GPU are happy.
>> +	 *
>> +	 * FIXME: as we insert all the pages now then no .fault handler must
>> +	 * be called, so don't provide one
>> +	 */
>> +	ret = vm_map_pages(vma, xen_obj->pages, xen_obj->num_pages);
>> +	if (ret < 0)
>> +		DRM_ERROR("Failed to map pages into vma: %d\n", ret);
>> +
>> +	return ret;
>> +}
>> +
>>    static const struct vm_operations_struct xen_drm_drv_vm_ops = {
>>    	.open           = drm_gem_vm_open,
>>    	.close          = drm_gem_vm_close,
>> @@ -67,6 +108,7 @@ static const struct drm_gem_object_funcs xen_drm_front_gem_object_funcs = {
>>    	.get_sg_table = xen_drm_front_gem_get_sg_table,
>>    	.vmap = xen_drm_front_gem_prime_vmap,
>>    	.vunmap = xen_drm_front_gem_prime_vunmap,
>> +	.mmap = xen_drm_front_gem_object_mmap,
>>    	.vm_ops = &xen_drm_drv_vm_ops,
>>    };
>>    
>> @@ -238,58 +280,6 @@ xen_drm_front_gem_import_sg_table(struct drm_device *dev,
>>    	return &xen_obj->base;
>>    }
>>    
>> -static int gem_mmap_obj(struct xen_gem_object *xen_obj,
>> -			struct vm_area_struct *vma)
>> -{
>> -	int ret;
>> -
>> -	/*
>> -	 * clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
>> -	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
>> -	 * the whole buffer.
>> -	 */
>> -	vma->vm_flags &= ~VM_PFNMAP;
>> -	vma->vm_flags |= VM_MIXEDMAP;
>> -	vma->vm_pgoff = 0;
>> -	/*
>> -	 * According to Xen on ARM ABI (xen/include/public/arch-arm.h):
>> -	 * all memory which is shared with other entities in the system
>> -	 * (including the hypervisor and other guests) must reside in memory
>> -	 * which is mapped as Normal Inner Write-Back Outer Write-Back
>> -	 * Inner-Shareable.
>> -	 */
>> -	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>> -
>> -	/*
>> -	 * vm_operations_struct.fault handler will be called if CPU access
>> -	 * to VM is here. For GPUs this isn't the case, because CPU
>> -	 * doesn't touch the memory. Insert pages now, so both CPU and GPU are
>> -	 * happy.
>> -	 * FIXME: as we insert all the pages now then no .fault handler must
>> -	 * be called, so don't provide one
>> -	 */
>> -	ret = vm_map_pages(vma, xen_obj->pages, xen_obj->num_pages);
>> -	if (ret < 0)
>> -		DRM_ERROR("Failed to map pages into vma: %d\n", ret);
>> -
>> -	return ret;
>> -}
>> -
>> -int xen_drm_front_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>> -{
>> -	struct xen_gem_object *xen_obj;
>> -	struct drm_gem_object *gem_obj;
>> -	int ret;
>> -
>> -	ret = drm_gem_mmap(filp, vma);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	gem_obj = vma->vm_private_data;
>> -	xen_obj = to_xen_gem_obj(gem_obj);
>> -	return gem_mmap_obj(xen_obj, vma);
>> -}
>> -
>>    int xen_drm_front_gem_prime_vmap(struct drm_gem_object *gem_obj, struct dma_buf_map *map)
>>    {
>>    	struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
>> @@ -313,17 +303,3 @@ void xen_drm_front_gem_prime_vunmap(struct drm_gem_object *gem_obj,
>>    {
>>    	vunmap(map->vaddr);
>>    }
>> -
>> -int xen_drm_front_gem_prime_mmap(struct drm_gem_object *gem_obj,
>> -				 struct vm_area_struct *vma)
>> -{
>> -	struct xen_gem_object *xen_obj;
>> -	int ret;
>> -
>> -	ret = drm_gem_mmap_obj(gem_obj, gem_obj->size, vma);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	xen_obj = to_xen_gem_obj(gem_obj);
>> -	return gem_mmap_obj(xen_obj, vma);
>> -}
>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.h b/drivers/gpu/drm/xen/xen_drm_front_gem.h
>> index a4e67d0a149c..eaea470f7001 100644
>> --- a/drivers/gpu/drm/xen/xen_drm_front_gem.h
>> +++ b/drivers/gpu/drm/xen/xen_drm_front_gem.h
>> @@ -15,9 +15,7 @@ struct dma_buf_attachment;
>>    struct dma_buf_map;
>>    struct drm_device;
>>    struct drm_gem_object;
>> -struct file;
>>    struct sg_table;
>> -struct vm_area_struct;
>>    
>>    struct drm_gem_object *xen_drm_front_gem_create(struct drm_device *dev,
>>    						size_t size);
>> @@ -33,15 +31,10 @@ struct page **xen_drm_front_gem_get_pages(struct drm_gem_object *obj);
>>    
>>    void xen_drm_front_gem_free_object_unlocked(struct drm_gem_object *gem_obj);
>>    
>> -int xen_drm_front_gem_mmap(struct file *filp, struct vm_area_struct *vma);
>> -
>>    int xen_drm_front_gem_prime_vmap(struct drm_gem_object *gem_obj,
>>    				 struct dma_buf_map *map);
>>    
>>    void xen_drm_front_gem_prime_vunmap(struct drm_gem_object *gem_obj,
>>    				    struct dma_buf_map *map);
>>    
>> -int xen_drm_front_gem_prime_mmap(struct drm_gem_object *gem_obj,
>> -				 struct vm_area_struct *vma);
>> -
>>    #endif /* __XEN_DRM_FRONT_GEM_H */

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/3] drm/exynox: Implement mmap as GEM object function

[Daniel Vetter]

On Mon, Nov 08, 2021 at 11:28:44AM +0100, Thomas Zimmermann wrote:
> Moving the driver-specific mmap code into a GEM object function allows
> for using DRM helpers for various mmap callbacks.
> 
> The respective exynos functions are being removed. The file_operations
> structure exynos_drm_driver_fops is now being created by the helper macro
> DEFINE_DRM_GEM_FOPS().
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

s/exynox/exynos in the subject.

Patch lgtm, but would still be good if exynos maintainers would
ack/review/test it. But if you don't get anything within 2 weeks here's
my:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c   | 13 ++-----
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 20 ++---------
>  drivers/gpu/drm/exynos/exynos_drm_gem.c   | 43 +++++------------------
>  drivers/gpu/drm/exynos/exynos_drm_gem.h   |  5 ---
>  4 files changed, 13 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index d8f1cf4d6b69..9743b6b17447 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -102,16 +102,7 @@ static const struct drm_ioctl_desc exynos_ioctls[] = {
>  			DRM_RENDER_ALLOW),
>  };
>  
> -static const struct file_operations exynos_drm_driver_fops = {
> -	.owner		= THIS_MODULE,
> -	.open		= drm_open,
> -	.mmap		= exynos_drm_gem_mmap,
> -	.poll		= drm_poll,
> -	.read		= drm_read,
> -	.unlocked_ioctl	= drm_ioctl,
> -	.compat_ioctl = drm_compat_ioctl,
> -	.release	= drm_release,
> -};
> +DEFINE_DRM_GEM_FOPS(exynos_drm_driver_fops);
>  
>  static const struct drm_driver exynos_drm_driver = {
>  	.driver_features	= DRIVER_MODESET | DRIVER_GEM
> @@ -124,7 +115,7 @@ static const struct drm_driver exynos_drm_driver = {
>  	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
>  	.gem_prime_import	= exynos_drm_gem_prime_import,
>  	.gem_prime_import_sg_table	= exynos_drm_gem_prime_import_sg_table,
> -	.gem_prime_mmap		= exynos_drm_gem_prime_mmap,
> +	.gem_prime_mmap		= drm_gem_prime_mmap,
>  	.ioctls			= exynos_ioctls,
>  	.num_ioctls		= ARRAY_SIZE(exynos_ioctls),
>  	.fops			= &exynos_drm_driver_fops,
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index 5147f5929be7..02c97b9ca926 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -15,6 +15,7 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_fourcc.h>
> +#include <drm/drm_prime.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/exynos_drm.h>
>  
> @@ -39,25 +40,8 @@ static int exynos_drm_fb_mmap(struct fb_info *info,
>  	struct drm_fb_helper *helper = info->par;
>  	struct exynos_drm_fbdev *exynos_fbd = to_exynos_fbdev(helper);
>  	struct exynos_drm_gem *exynos_gem = exynos_fbd->exynos_gem;
> -	unsigned long vm_size;
> -	int ret;
> -
> -	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
> -
> -	vm_size = vma->vm_end - vma->vm_start;
> -
> -	if (vm_size > exynos_gem->size)
> -		return -EINVAL;
>  
> -	ret = dma_mmap_attrs(to_dma_dev(helper->dev), vma, exynos_gem->cookie,
> -			     exynos_gem->dma_addr, exynos_gem->size,
> -			     exynos_gem->dma_attrs);
> -	if (ret < 0) {
> -		DRM_DEV_ERROR(to_dma_dev(helper->dev), "failed to mmap.\n");
> -		return ret;
> -	}
> -
> -	return 0;
> +	return drm_gem_prime_mmap(&exynos_gem->base, vma);
>  }
>  
>  static const struct fb_ops exynos_drm_fb_ops = {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index 4396224227d1..c4b63902ee7a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -17,6 +17,8 @@
>  #include "exynos_drm_drv.h"
>  #include "exynos_drm_gem.h"
>  
> +static int exynos_drm_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
> +
>  static int exynos_drm_alloc_buf(struct exynos_drm_gem *exynos_gem, bool kvmap)
>  {
>  	struct drm_device *dev = exynos_gem->base.dev;
> @@ -135,6 +137,7 @@ static const struct vm_operations_struct exynos_drm_gem_vm_ops = {
>  static const struct drm_gem_object_funcs exynos_drm_gem_object_funcs = {
>  	.free = exynos_drm_gem_free_object,
>  	.get_sg_table = exynos_drm_gem_prime_get_sg_table,
> +	.mmap = exynos_drm_gem_mmap,
>  	.vm_ops = &exynos_drm_gem_vm_ops,
>  };
>  
> @@ -354,12 +357,16 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
>  	return 0;
>  }
>  
> -static int exynos_drm_gem_mmap_obj(struct drm_gem_object *obj,
> -				   struct vm_area_struct *vma)
> +static int exynos_drm_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>  {
>  	struct exynos_drm_gem *exynos_gem = to_exynos_gem(obj);
>  	int ret;
>  
> +	if (obj->import_attach)
> +		return dma_buf_mmap(obj->dma_buf, vma, 0);
> +
> +	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
> +
>  	DRM_DEV_DEBUG_KMS(to_dma_dev(obj->dev), "flags = 0x%x\n",
>  			  exynos_gem->flags);
>  
> @@ -385,26 +392,6 @@ static int exynos_drm_gem_mmap_obj(struct drm_gem_object *obj,
>  	return ret;
>  }
>  
> -int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> -{
> -	struct drm_gem_object *obj;
> -	int ret;
> -
> -	/* set vm_area_struct. */
> -	ret = drm_gem_mmap(filp, vma);
> -	if (ret < 0) {
> -		DRM_ERROR("failed to mmap.\n");
> -		return ret;
> -	}
> -
> -	obj = vma->vm_private_data;
> -
> -	if (obj->import_attach)
> -		return dma_buf_mmap(obj->dma_buf, vma, 0);
> -
> -	return exynos_drm_gem_mmap_obj(obj, vma);
> -}
> -
>  /* low-level interface prime helpers */
>  struct drm_gem_object *exynos_drm_gem_prime_import(struct drm_device *dev,
>  					    struct dma_buf *dma_buf)
> @@ -466,15 +453,3 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>  	exynos_gem->sgt = sgt;
>  	return &exynos_gem->base;
>  }
> -
> -int exynos_drm_gem_prime_mmap(struct drm_gem_object *obj,
> -			      struct vm_area_struct *vma)
> -{
> -	int ret;
> -
> -	ret = drm_gem_mmap_obj(obj, obj->size, vma);
> -	if (ret < 0)
> -		return ret;
> -
> -	return exynos_drm_gem_mmap_obj(obj, vma);
> -}
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> index a23272fb96fb..79d7e1a87419 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> @@ -96,9 +96,6 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
>  			       struct drm_device *dev,
>  			       struct drm_mode_create_dumb *args);
>  
> -/* set vm_flags and we can change the vm attribute to other one at here. */
> -int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
> -
>  /* low-level interface prime helpers */
>  struct drm_gem_object *exynos_drm_gem_prime_import(struct drm_device *dev,
>  					    struct dma_buf *dma_buf);
> @@ -107,7 +104,5 @@ struct drm_gem_object *
>  exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>  				     struct dma_buf_attachment *attach,
>  				     struct sg_table *sgt);
> -int exynos_drm_gem_prime_mmap(struct drm_gem_object *obj,
> -			      struct vm_area_struct *vma);
>  
>  #endif
> -- 
> 2.33.1
> 

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

Re: [PATCH 1/3] drm/exynox: Implement mmap as GEM object function

[Inki Dae]

Hi,

Really sorry for late. I saw this patch in my mailbox just before. Seems I missed it due to a typo, exynox.
I will review and apply this patch ASAP.

Thanks,
Inki Dae

21. 11. 8. 오후 7:28에 Thomas Zimmermann 이(가) 쓴 글:
> Moving the driver-specific mmap code into a GEM object function allows
> for using DRM helpers for various mmap callbacks.
> 
> The respective exynos functions are being removed. The file_operations
> structure exynos_drm_driver_fops is now being created by the helper macro
> DEFINE_DRM_GEM_FOPS().
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c   | 13 ++-----
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 20 ++---------
>  drivers/gpu/drm/exynos/exynos_drm_gem.c   | 43 +++++------------------
>  drivers/gpu/drm/exynos/exynos_drm_gem.h   |  5 ---
>  4 files changed, 13 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index d8f1cf4d6b69..9743b6b17447 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -102,16 +102,7 @@ static const struct drm_ioctl_desc exynos_ioctls[] = {
>  			DRM_RENDER_ALLOW),
>  };
>  
> -static const struct file_operations exynos_drm_driver_fops = {
> -	.owner		= THIS_MODULE,
> -	.open		= drm_open,
> -	.mmap		= exynos_drm_gem_mmap,
> -	.poll		= drm_poll,
> -	.read		= drm_read,
> -	.unlocked_ioctl	= drm_ioctl,
> -	.compat_ioctl = drm_compat_ioctl,
> -	.release	= drm_release,
> -};
> +DEFINE_DRM_GEM_FOPS(exynos_drm_driver_fops);
>  
>  static const struct drm_driver exynos_drm_driver = {
>  	.driver_features	= DRIVER_MODESET | DRIVER_GEM
> @@ -124,7 +115,7 @@ static const struct drm_driver exynos_drm_driver = {
>  	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
>  	.gem_prime_import	= exynos_drm_gem_prime_import,
>  	.gem_prime_import_sg_table	= exynos_drm_gem_prime_import_sg_table,
> -	.gem_prime_mmap		= exynos_drm_gem_prime_mmap,
> +	.gem_prime_mmap		= drm_gem_prime_mmap,
>  	.ioctls			= exynos_ioctls,
>  	.num_ioctls		= ARRAY_SIZE(exynos_ioctls),
>  	.fops			= &exynos_drm_driver_fops,
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index 5147f5929be7..02c97b9ca926 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -15,6 +15,7 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_fourcc.h>
> +#include <drm/drm_prime.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/exynos_drm.h>
>  
> @@ -39,25 +40,8 @@ static int exynos_drm_fb_mmap(struct fb_info *info,
>  	struct drm_fb_helper *helper = info->par;
>  	struct exynos_drm_fbdev *exynos_fbd = to_exynos_fbdev(helper);
>  	struct exynos_drm_gem *exynos_gem = exynos_fbd->exynos_gem;
> -	unsigned long vm_size;
> -	int ret;
> -
> -	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
> -
> -	vm_size = vma->vm_end - vma->vm_start;
> -
> -	if (vm_size > exynos_gem->size)
> -		return -EINVAL;
>  
> -	ret = dma_mmap_attrs(to_dma_dev(helper->dev), vma, exynos_gem->cookie,
> -			     exynos_gem->dma_addr, exynos_gem->size,
> -			     exynos_gem->dma_attrs);
> -	if (ret < 0) {
> -		DRM_DEV_ERROR(to_dma_dev(helper->dev), "failed to mmap.\n");
> -		return ret;
> -	}
> -
> -	return 0;
> +	return drm_gem_prime_mmap(&exynos_gem->base, vma);
>  }
>  
>  static const struct fb_ops exynos_drm_fb_ops = {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index 4396224227d1..c4b63902ee7a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -17,6 +17,8 @@
>  #include "exynos_drm_drv.h"
>  #include "exynos_drm_gem.h"
>  
> +static int exynos_drm_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
> +
>  static int exynos_drm_alloc_buf(struct exynos_drm_gem *exynos_gem, bool kvmap)
>  {
>  	struct drm_device *dev = exynos_gem->base.dev;
> @@ -135,6 +137,7 @@ static const struct vm_operations_struct exynos_drm_gem_vm_ops = {
>  static const struct drm_gem_object_funcs exynos_drm_gem_object_funcs = {
>  	.free = exynos_drm_gem_free_object,
>  	.get_sg_table = exynos_drm_gem_prime_get_sg_table,
> +	.mmap = exynos_drm_gem_mmap,
>  	.vm_ops = &exynos_drm_gem_vm_ops,
>  };
>  
> @@ -354,12 +357,16 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
>  	return 0;
>  }
>  
> -static int exynos_drm_gem_mmap_obj(struct drm_gem_object *obj,
> -				   struct vm_area_struct *vma)
> +static int exynos_drm_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>  {
>  	struct exynos_drm_gem *exynos_gem = to_exynos_gem(obj);
>  	int ret;
>  
> +	if (obj->import_attach)
> +		return dma_buf_mmap(obj->dma_buf, vma, 0);
> +
> +	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
> +
>  	DRM_DEV_DEBUG_KMS(to_dma_dev(obj->dev), "flags = 0x%x\n",
>  			  exynos_gem->flags);
>  
> @@ -385,26 +392,6 @@ static int exynos_drm_gem_mmap_obj(struct drm_gem_object *obj,
>  	return ret;
>  }
>  
> -int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> -{
> -	struct drm_gem_object *obj;
> -	int ret;
> -
> -	/* set vm_area_struct. */
> -	ret = drm_gem_mmap(filp, vma);
> -	if (ret < 0) {
> -		DRM_ERROR("failed to mmap.\n");
> -		return ret;
> -	}
> -
> -	obj = vma->vm_private_data;
> -
> -	if (obj->import_attach)
> -		return dma_buf_mmap(obj->dma_buf, vma, 0);
> -
> -	return exynos_drm_gem_mmap_obj(obj, vma);
> -}
> -
>  /* low-level interface prime helpers */
>  struct drm_gem_object *exynos_drm_gem_prime_import(struct drm_device *dev,
>  					    struct dma_buf *dma_buf)
> @@ -466,15 +453,3 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>  	exynos_gem->sgt = sgt;
>  	return &exynos_gem->base;
>  }
> -
> -int exynos_drm_gem_prime_mmap(struct drm_gem_object *obj,
> -			      struct vm_area_struct *vma)
> -{
> -	int ret;
> -
> -	ret = drm_gem_mmap_obj(obj, obj->size, vma);
> -	if (ret < 0)
> -		return ret;
> -
> -	return exynos_drm_gem_mmap_obj(obj, vma);
> -}
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> index a23272fb96fb..79d7e1a87419 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> @@ -96,9 +96,6 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
>  			       struct drm_device *dev,
>  			       struct drm_mode_create_dumb *args);
>  
> -/* set vm_flags and we can change the vm attribute to other one at here. */
> -int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
> -
>  /* low-level interface prime helpers */
>  struct drm_gem_object *exynos_drm_gem_prime_import(struct drm_device *dev,
>  					    struct dma_buf *dma_buf);
> @@ -107,7 +104,5 @@ struct drm_gem_object *
>  exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>  				     struct dma_buf_attachment *attach,
>  				     struct sg_table *sgt);
> -int exynos_drm_gem_prime_mmap(struct drm_gem_object *obj,
> -			      struct vm_area_struct *vma);
>  
>  #endif
> 

Re: [PATCH 1/3] drm/exynox: Implement mmap as GEM object function

[Inki Dae]

Hi Thomas and Daniel,

21. 11. 9. 오전 12:29에 Daniel Vetter 이(가) 쓴 글:
> On Mon, Nov 08, 2021 at 11:28:44AM +0100, Thomas Zimmermann wrote:
>> Moving the driver-specific mmap code into a GEM object function allows
>> for using DRM helpers for various mmap callbacks.
>>
>> The respective exynos functions are being removed. The file_operations
>> structure exynos_drm_driver_fops is now being created by the helper macro
>> DEFINE_DRM_GEM_FOPS().
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> s/exynox/exynos in the subject.
> 
> Patch lgtm, but would still be good if exynos maintainers would
> ack/review/test it. But if you don't get anything within 2 weeks here's

Sorry for late. Confirmed working well on Odroid board, and had a review. Applied.
https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git/log/?h=exynos-drm-next

Thanks,
Inki Dae

> my:
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_drv.c   | 13 ++-----
>>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 20 ++---------
>>  drivers/gpu/drm/exynos/exynos_drm_gem.c   | 43 +++++------------------
>>  drivers/gpu/drm/exynos/exynos_drm_gem.h   |  5 ---
>>  4 files changed, 13 insertions(+), 68 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> index d8f1cf4d6b69..9743b6b17447 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> @@ -102,16 +102,7 @@ static const struct drm_ioctl_desc exynos_ioctls[] = {
>>  			DRM_RENDER_ALLOW),
>>  };
>>  
>> -static const struct file_operations exynos_drm_driver_fops = {
>> -	.owner		= THIS_MODULE,
>> -	.open		= drm_open,
>> -	.mmap		= exynos_drm_gem_mmap,
>> -	.poll		= drm_poll,
>> -	.read		= drm_read,
>> -	.unlocked_ioctl	= drm_ioctl,
>> -	.compat_ioctl = drm_compat_ioctl,
>> -	.release	= drm_release,
>> -};
>> +DEFINE_DRM_GEM_FOPS(exynos_drm_driver_fops);
>>  
>>  static const struct drm_driver exynos_drm_driver = {
>>  	.driver_features	= DRIVER_MODESET | DRIVER_GEM
>> @@ -124,7 +115,7 @@ static const struct drm_driver exynos_drm_driver = {
>>  	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
>>  	.gem_prime_import	= exynos_drm_gem_prime_import,
>>  	.gem_prime_import_sg_table	= exynos_drm_gem_prime_import_sg_table,
>> -	.gem_prime_mmap		= exynos_drm_gem_prime_mmap,
>> +	.gem_prime_mmap		= drm_gem_prime_mmap,
>>  	.ioctls			= exynos_ioctls,
>>  	.num_ioctls		= ARRAY_SIZE(exynos_ioctls),
>>  	.fops			= &exynos_drm_driver_fops,
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
>> index 5147f5929be7..02c97b9ca926 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
>> @@ -15,6 +15,7 @@
>>  #include <drm/drm_crtc.h>
>>  #include <drm/drm_fb_helper.h>
>>  #include <drm/drm_fourcc.h>
>> +#include <drm/drm_prime.h>
>>  #include <drm/drm_probe_helper.h>
>>  #include <drm/exynos_drm.h>
>>  
>> @@ -39,25 +40,8 @@ static int exynos_drm_fb_mmap(struct fb_info *info,
>>  	struct drm_fb_helper *helper = info->par;
>>  	struct exynos_drm_fbdev *exynos_fbd = to_exynos_fbdev(helper);
>>  	struct exynos_drm_gem *exynos_gem = exynos_fbd->exynos_gem;
>> -	unsigned long vm_size;
>> -	int ret;
>> -
>> -	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
>> -
>> -	vm_size = vma->vm_end - vma->vm_start;
>> -
>> -	if (vm_size > exynos_gem->size)
>> -		return -EINVAL;
>>  
>> -	ret = dma_mmap_attrs(to_dma_dev(helper->dev), vma, exynos_gem->cookie,
>> -			     exynos_gem->dma_addr, exynos_gem->size,
>> -			     exynos_gem->dma_attrs);
>> -	if (ret < 0) {
>> -		DRM_DEV_ERROR(to_dma_dev(helper->dev), "failed to mmap.\n");
>> -		return ret;
>> -	}
>> -
>> -	return 0;
>> +	return drm_gem_prime_mmap(&exynos_gem->base, vma);
>>  }
>>  
>>  static const struct fb_ops exynos_drm_fb_ops = {
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> index 4396224227d1..c4b63902ee7a 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> @@ -17,6 +17,8 @@
>>  #include "exynos_drm_drv.h"
>>  #include "exynos_drm_gem.h"
>>  
>> +static int exynos_drm_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
>> +
>>  static int exynos_drm_alloc_buf(struct exynos_drm_gem *exynos_gem, bool kvmap)
>>  {
>>  	struct drm_device *dev = exynos_gem->base.dev;
>> @@ -135,6 +137,7 @@ static const struct vm_operations_struct exynos_drm_gem_vm_ops = {
>>  static const struct drm_gem_object_funcs exynos_drm_gem_object_funcs = {
>>  	.free = exynos_drm_gem_free_object,
>>  	.get_sg_table = exynos_drm_gem_prime_get_sg_table,
>> +	.mmap = exynos_drm_gem_mmap,
>>  	.vm_ops = &exynos_drm_gem_vm_ops,
>>  };
>>  
>> @@ -354,12 +357,16 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
>>  	return 0;
>>  }
>>  
>> -static int exynos_drm_gem_mmap_obj(struct drm_gem_object *obj,
>> -				   struct vm_area_struct *vma)
>> +static int exynos_drm_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>>  {
>>  	struct exynos_drm_gem *exynos_gem = to_exynos_gem(obj);
>>  	int ret;
>>  
>> +	if (obj->import_attach)
>> +		return dma_buf_mmap(obj->dma_buf, vma, 0);
>> +
>> +	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
>> +
>>  	DRM_DEV_DEBUG_KMS(to_dma_dev(obj->dev), "flags = 0x%x\n",
>>  			  exynos_gem->flags);
>>  
>> @@ -385,26 +392,6 @@ static int exynos_drm_gem_mmap_obj(struct drm_gem_object *obj,
>>  	return ret;
>>  }
>>  
>> -int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>> -{
>> -	struct drm_gem_object *obj;
>> -	int ret;
>> -
>> -	/* set vm_area_struct. */
>> -	ret = drm_gem_mmap(filp, vma);
>> -	if (ret < 0) {
>> -		DRM_ERROR("failed to mmap.\n");
>> -		return ret;
>> -	}
>> -
>> -	obj = vma->vm_private_data;
>> -
>> -	if (obj->import_attach)
>> -		return dma_buf_mmap(obj->dma_buf, vma, 0);
>> -
>> -	return exynos_drm_gem_mmap_obj(obj, vma);
>> -}
>> -
>>  /* low-level interface prime helpers */
>>  struct drm_gem_object *exynos_drm_gem_prime_import(struct drm_device *dev,
>>  					    struct dma_buf *dma_buf)
>> @@ -466,15 +453,3 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>>  	exynos_gem->sgt = sgt;
>>  	return &exynos_gem->base;
>>  }
>> -
>> -int exynos_drm_gem_prime_mmap(struct drm_gem_object *obj,
>> -			      struct vm_area_struct *vma)
>> -{
>> -	int ret;
>> -
>> -	ret = drm_gem_mmap_obj(obj, obj->size, vma);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	return exynos_drm_gem_mmap_obj(obj, vma);
>> -}
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h
>> index a23272fb96fb..79d7e1a87419 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
>> @@ -96,9 +96,6 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
>>  			       struct drm_device *dev,
>>  			       struct drm_mode_create_dumb *args);
>>  
>> -/* set vm_flags and we can change the vm attribute to other one at here. */
>> -int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
>> -
>>  /* low-level interface prime helpers */
>>  struct drm_gem_object *exynos_drm_gem_prime_import(struct drm_device *dev,
>>  					    struct dma_buf *dma_buf);
>> @@ -107,7 +104,5 @@ struct drm_gem_object *
>>  exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>>  				     struct dma_buf_attachment *attach,
>>  				     struct sg_table *sgt);
>> -int exynos_drm_gem_prime_mmap(struct drm_gem_object *obj,
>> -			      struct vm_area_struct *vma);
>>  
>>  #endif
>> -- 
>> 2.33.1
>>
> 

Re: [PATCH 1/3] drm/exynox: Implement mmap as GEM object function

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 8522 bytes --]

Hi

Am 09.11.21 um 10:34 schrieb Inki Dae:
> Hi Thomas and Daniel,
> 
> 21. 11. 9. 오전 12:29에 Daniel Vetter 이(가) 쓴 글:
>> On Mon, Nov 08, 2021 at 11:28:44AM +0100, Thomas Zimmermann wrote:
>>> Moving the driver-specific mmap code into a GEM object function allows
>>> for using DRM helpers for various mmap callbacks.
>>>
>>> The respective exynos functions are being removed. The file_operations
>>> structure exynos_drm_driver_fops is now being created by the helper macro
>>> DEFINE_DRM_GEM_FOPS().
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>
>> s/exynox/exynos in the subject.
>>
>> Patch lgtm, but would still be good if exynos maintainers would
>> ack/review/test it. But if you don't get anything within 2 weeks here's
> 
> Sorry for late. Confirmed working well on Odroid board, and had a review. Applied.
> https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git/log/?h=exynos-drm-next

Thanks a lot!

Best regards
Thomas

> 
> Thanks,
> Inki Dae
> 
>> my:
>>
>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>>> ---
>>>   drivers/gpu/drm/exynos/exynos_drm_drv.c   | 13 ++-----
>>>   drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 20 ++---------
>>>   drivers/gpu/drm/exynos/exynos_drm_gem.c   | 43 +++++------------------
>>>   drivers/gpu/drm/exynos/exynos_drm_gem.h   |  5 ---
>>>   4 files changed, 13 insertions(+), 68 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>> index d8f1cf4d6b69..9743b6b17447 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>> @@ -102,16 +102,7 @@ static const struct drm_ioctl_desc exynos_ioctls[] = {
>>>   			DRM_RENDER_ALLOW),
>>>   };
>>>   
>>> -static const struct file_operations exynos_drm_driver_fops = {
>>> -	.owner		= THIS_MODULE,
>>> -	.open		= drm_open,
>>> -	.mmap		= exynos_drm_gem_mmap,
>>> -	.poll		= drm_poll,
>>> -	.read		= drm_read,
>>> -	.unlocked_ioctl	= drm_ioctl,
>>> -	.compat_ioctl = drm_compat_ioctl,
>>> -	.release	= drm_release,
>>> -};
>>> +DEFINE_DRM_GEM_FOPS(exynos_drm_driver_fops);
>>>   
>>>   static const struct drm_driver exynos_drm_driver = {
>>>   	.driver_features	= DRIVER_MODESET | DRIVER_GEM
>>> @@ -124,7 +115,7 @@ static const struct drm_driver exynos_drm_driver = {
>>>   	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
>>>   	.gem_prime_import	= exynos_drm_gem_prime_import,
>>>   	.gem_prime_import_sg_table	= exynos_drm_gem_prime_import_sg_table,
>>> -	.gem_prime_mmap		= exynos_drm_gem_prime_mmap,
>>> +	.gem_prime_mmap		= drm_gem_prime_mmap,
>>>   	.ioctls			= exynos_ioctls,
>>>   	.num_ioctls		= ARRAY_SIZE(exynos_ioctls),
>>>   	.fops			= &exynos_drm_driver_fops,
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
>>> index 5147f5929be7..02c97b9ca926 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
>>> @@ -15,6 +15,7 @@
>>>   #include <drm/drm_crtc.h>
>>>   #include <drm/drm_fb_helper.h>
>>>   #include <drm/drm_fourcc.h>
>>> +#include <drm/drm_prime.h>
>>>   #include <drm/drm_probe_helper.h>
>>>   #include <drm/exynos_drm.h>
>>>   
>>> @@ -39,25 +40,8 @@ static int exynos_drm_fb_mmap(struct fb_info *info,
>>>   	struct drm_fb_helper *helper = info->par;
>>>   	struct exynos_drm_fbdev *exynos_fbd = to_exynos_fbdev(helper);
>>>   	struct exynos_drm_gem *exynos_gem = exynos_fbd->exynos_gem;
>>> -	unsigned long vm_size;
>>> -	int ret;
>>> -
>>> -	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
>>> -
>>> -	vm_size = vma->vm_end - vma->vm_start;
>>> -
>>> -	if (vm_size > exynos_gem->size)
>>> -		return -EINVAL;
>>>   
>>> -	ret = dma_mmap_attrs(to_dma_dev(helper->dev), vma, exynos_gem->cookie,
>>> -			     exynos_gem->dma_addr, exynos_gem->size,
>>> -			     exynos_gem->dma_attrs);
>>> -	if (ret < 0) {
>>> -		DRM_DEV_ERROR(to_dma_dev(helper->dev), "failed to mmap.\n");
>>> -		return ret;
>>> -	}
>>> -
>>> -	return 0;
>>> +	return drm_gem_prime_mmap(&exynos_gem->base, vma);
>>>   }
>>>   
>>>   static const struct fb_ops exynos_drm_fb_ops = {
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>> index 4396224227d1..c4b63902ee7a 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>> @@ -17,6 +17,8 @@
>>>   #include "exynos_drm_drv.h"
>>>   #include "exynos_drm_gem.h"
>>>   
>>> +static int exynos_drm_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
>>> +
>>>   static int exynos_drm_alloc_buf(struct exynos_drm_gem *exynos_gem, bool kvmap)
>>>   {
>>>   	struct drm_device *dev = exynos_gem->base.dev;
>>> @@ -135,6 +137,7 @@ static const struct vm_operations_struct exynos_drm_gem_vm_ops = {
>>>   static const struct drm_gem_object_funcs exynos_drm_gem_object_funcs = {
>>>   	.free = exynos_drm_gem_free_object,
>>>   	.get_sg_table = exynos_drm_gem_prime_get_sg_table,
>>> +	.mmap = exynos_drm_gem_mmap,
>>>   	.vm_ops = &exynos_drm_gem_vm_ops,
>>>   };
>>>   
>>> @@ -354,12 +357,16 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
>>>   	return 0;
>>>   }
>>>   
>>> -static int exynos_drm_gem_mmap_obj(struct drm_gem_object *obj,
>>> -				   struct vm_area_struct *vma)
>>> +static int exynos_drm_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>>>   {
>>>   	struct exynos_drm_gem *exynos_gem = to_exynos_gem(obj);
>>>   	int ret;
>>>   
>>> +	if (obj->import_attach)
>>> +		return dma_buf_mmap(obj->dma_buf, vma, 0);
>>> +
>>> +	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
>>> +
>>>   	DRM_DEV_DEBUG_KMS(to_dma_dev(obj->dev), "flags = 0x%x\n",
>>>   			  exynos_gem->flags);
>>>   
>>> @@ -385,26 +392,6 @@ static int exynos_drm_gem_mmap_obj(struct drm_gem_object *obj,
>>>   	return ret;
>>>   }
>>>   
>>> -int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>>> -{
>>> -	struct drm_gem_object *obj;
>>> -	int ret;
>>> -
>>> -	/* set vm_area_struct. */
>>> -	ret = drm_gem_mmap(filp, vma);
>>> -	if (ret < 0) {
>>> -		DRM_ERROR("failed to mmap.\n");
>>> -		return ret;
>>> -	}
>>> -
>>> -	obj = vma->vm_private_data;
>>> -
>>> -	if (obj->import_attach)
>>> -		return dma_buf_mmap(obj->dma_buf, vma, 0);
>>> -
>>> -	return exynos_drm_gem_mmap_obj(obj, vma);
>>> -}
>>> -
>>>   /* low-level interface prime helpers */
>>>   struct drm_gem_object *exynos_drm_gem_prime_import(struct drm_device *dev,
>>>   					    struct dma_buf *dma_buf)
>>> @@ -466,15 +453,3 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>>>   	exynos_gem->sgt = sgt;
>>>   	return &exynos_gem->base;
>>>   }
>>> -
>>> -int exynos_drm_gem_prime_mmap(struct drm_gem_object *obj,
>>> -			      struct vm_area_struct *vma)
>>> -{
>>> -	int ret;
>>> -
>>> -	ret = drm_gem_mmap_obj(obj, obj->size, vma);
>>> -	if (ret < 0)
>>> -		return ret;
>>> -
>>> -	return exynos_drm_gem_mmap_obj(obj, vma);
>>> -}
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h
>>> index a23272fb96fb..79d7e1a87419 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
>>> @@ -96,9 +96,6 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
>>>   			       struct drm_device *dev,
>>>   			       struct drm_mode_create_dumb *args);
>>>   
>>> -/* set vm_flags and we can change the vm attribute to other one at here. */
>>> -int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
>>> -
>>>   /* low-level interface prime helpers */
>>>   struct drm_gem_object *exynos_drm_gem_prime_import(struct drm_device *dev,
>>>   					    struct dma_buf *dma_buf);
>>> @@ -107,7 +104,5 @@ struct drm_gem_object *
>>>   exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>>>   				     struct dma_buf_attachment *attach,
>>>   				     struct sg_table *sgt);
>>> -int exynos_drm_gem_prime_mmap(struct drm_gem_object *obj,
>>> -			      struct vm_area_struct *vma);
>>>   
>>>   #endif
>>> -- 
>>> 2.33.1
>>>
>>

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 3/3] drm: Update documentation and TODO of gem_prime_mmap hook

[Daniel Vetter]

On Mon, Nov 08, 2021 at 11:28:46AM +0100, Thomas Zimmermann wrote:
> The hook gem_prime_mmap in struct drm_driver is deprecated. Document
> the new requirements.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  Documentation/gpu/todo.rst | 11 -----------
>  include/drm/drm_drv.h      | 11 +++++++----
>  2 files changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 60d1d7ee0719..6593c4c4006e 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -268,17 +268,6 @@ Contact: Daniel Vetter
>  
>  Level: Intermediate
>  
> -Clean up mmap forwarding
> -------------------------
> -
> -A lot of drivers forward gem mmap calls to dma-buf mmap for imported buffers.
> -And also a lot of them forward dma-buf mmap to the gem mmap implementations.
> -There's drm_gem_prime_mmap() for this now, but still needs to be rolled out.
> -
> -Contact: Daniel Vetter
> -
> -Level: Intermediate
> -
>  Generic fbdev defio support
>  ---------------------------
>  
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 0cd95953cdf5..a84eb4028e5b 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -345,11 +345,14 @@ struct drm_driver {
>  	 * mmap hook for GEM drivers, used to implement dma-buf mmap in the
>  	 * PRIME helpers.
>  	 *
> -	 * FIXME: There's way too much duplication going on here, and also moved
> -	 * to &drm_gem_object_funcs.
> +	 * This hook only exists for historical reasons. Drivers must use
> +	 * drm_gem_prime_mmap() to implement it.
> +	 *
> +	 * FIXME: Convert all drivers to implement mmap in struct
> +	 * &drm_gem_object_funcs and inline drm_gem_prime_mmap() into
> +	 * its callers. This hook should be removed afterwards.

I think at least i915 has different semantics between dma-buf mmap and gem
mmap in some cases, so this might be tricky to achieve. But I think
everywhere else this should be solid.
-Daniel

>  	 */
> -	int (*gem_prime_mmap)(struct drm_gem_object *obj,
> -				struct vm_area_struct *vma);
> +	int (*gem_prime_mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma);
>  
>  	/**
>  	 * @dumb_create:
> -- 
> 2.33.1
> 

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

Re: [PATCH 3/3] drm: Update documentation and TODO of gem_prime_mmap hook

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3061 bytes --]

Hi

Am 10.11.21 um 11:46 schrieb Daniel Vetter:
> On Mon, Nov 08, 2021 at 11:28:46AM +0100, Thomas Zimmermann wrote:
>> The hook gem_prime_mmap in struct drm_driver is deprecated. Document
>> the new requirements.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks a lot!

With the remaining patches merged, all drivers will implement 
gem_prime_mmap via drm_gem_prime_mmap() and use the GEM object's mmap 
callback for specific details.

The next step would be to move all custom implementations of 
file_operations.mmap into the GEM object's mmap and implement the 
file_operations callback with drm_mmap().

Best regards
Thomas

> 
>> ---
>>   Documentation/gpu/todo.rst | 11 -----------
>>   include/drm/drm_drv.h      | 11 +++++++----
>>   2 files changed, 7 insertions(+), 15 deletions(-)
>>
>> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
>> index 60d1d7ee0719..6593c4c4006e 100644
>> --- a/Documentation/gpu/todo.rst
>> +++ b/Documentation/gpu/todo.rst
>> @@ -268,17 +268,6 @@ Contact: Daniel Vetter
>>   
>>   Level: Intermediate
>>   
>> -Clean up mmap forwarding
>> -------------------------
>> -
>> -A lot of drivers forward gem mmap calls to dma-buf mmap for imported buffers.
>> -And also a lot of them forward dma-buf mmap to the gem mmap implementations.
>> -There's drm_gem_prime_mmap() for this now, but still needs to be rolled out.
>> -
>> -Contact: Daniel Vetter
>> -
>> -Level: Intermediate
>> -
>>   Generic fbdev defio support
>>   ---------------------------
>>   
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index 0cd95953cdf5..a84eb4028e5b 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -345,11 +345,14 @@ struct drm_driver {
>>   	 * mmap hook for GEM drivers, used to implement dma-buf mmap in the
>>   	 * PRIME helpers.
>>   	 *
>> -	 * FIXME: There's way too much duplication going on here, and also moved
>> -	 * to &drm_gem_object_funcs.
>> +	 * This hook only exists for historical reasons. Drivers must use
>> +	 * drm_gem_prime_mmap() to implement it.
>> +	 *
>> +	 * FIXME: Convert all drivers to implement mmap in struct
>> +	 * &drm_gem_object_funcs and inline drm_gem_prime_mmap() into
>> +	 * its callers. This hook should be removed afterwards.
> 
> I think at least i915 has different semantics between dma-buf mmap and gem
> mmap in some cases, so this might be tricky to achieve. But I think
> everywhere else this should be solid.
> -Daniel
> 
>>   	 */
>> -	int (*gem_prime_mmap)(struct drm_gem_object *obj,
>> -				struct vm_area_struct *vma);
>> +	int (*gem_prime_mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma);
>>   
>>   	/**
>>   	 * @dumb_create:
>> -- 
>> 2.33.1
>>
> 

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]