[PATCH 1/8] drm: add mmap() to drm_gem_object_funcs

[PATCH 1/8] drm: add mmap() to drm_gem_object_funcs

[Gerd Hoffmann]

drm_gem_object_funcs->vm_ops alone can't handle everything which needs
to be done for mmap(), tweaking vm_flags for example.  So add a new
mmap() callback to drm_gem_object_funcs where this code can go to.

Note that the vm_ops field is not used in case the mmap callback is
presnt, it is expected that the callback sets vma->vm_ops instead.

drm_gem_mmap_obj() will use the new callback for object specific mmap
setup.  With this in place the need for driver-speific fops->mmap
callbacks goes away, drm_gem_mmap can be hooked instead.

drm_gem_prime_mmap() will use the new callback too to just mmap gem
objects directly instead of jumping though loops to make
drm_gem_object_lookup() and fops->mmap work.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/drm/drm_gem.h       | 14 ++++++++++++++
 drivers/gpu/drm/drm_gem.c   | 27 ++++++++++++++++++---------
 drivers/gpu/drm/drm_prime.c |  9 +++++++++
 3 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 6aaba14f5972..e71f75a2ab57 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -150,6 +150,20 @@ struct drm_gem_object_funcs {
 	 */
 	void (*vunmap)(struct drm_gem_object *obj, void *vaddr);
 
+	/**
+	 * @mmap:
+	 *
+	 * Handle mmap() of the gem object, setup vma accordingly.
+	 *
+	 * This callback is optional.
+	 *
+	 * The callback is used by by both drm_gem_mmap_obj() and
+	 * drm_gem_prime_mmap().  When @mmap is present @vm_ops is not
+	 * used, the @mmap callback must set vma->vm_ops instead.
+	 *
+	 */
+	int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma);
+
 	/**
 	 * @vm_ops:
 	 *
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 6854f5867d51..56f42e0f2584 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1099,22 +1099,31 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 		     struct vm_area_struct *vma)
 {
 	struct drm_device *dev = obj->dev;
+	int ret;
 
 	/* Check for valid size. */
 	if (obj_size < vma->vm_end - vma->vm_start)
 		return -EINVAL;
 
-	if (obj->funcs && obj->funcs->vm_ops)
-		vma->vm_ops = obj->funcs->vm_ops;
-	else if (dev->driver->gem_vm_ops)
-		vma->vm_ops = dev->driver->gem_vm_ops;
-	else
-		return -EINVAL;
+	if (obj->funcs && obj->funcs->mmap) {
+		ret = obj->funcs->mmap(obj, vma);
+		if (ret)
+			return ret;
+		WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));
+	} else {
+		if (obj->funcs && obj->funcs->vm_ops)
+			vma->vm_ops = obj->funcs->vm_ops;
+		else if (dev->driver->gem_vm_ops)
+			vma->vm_ops = dev->driver->gem_vm_ops;
+		else
+			return -EINVAL;
+
+		vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
+		vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
+		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
+	}
 
-	vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
 	vma->vm_private_data = obj;
-	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
-	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
 
 	/* Take a ref for this mapping of the object, so that the fault
 	 * handler can dereference the mmap offset's pointer to the object.
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 0a2316e0e812..0814211b0f3f 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -713,6 +713,15 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
 	struct file *fil;
 	int ret;
 
+	if (obj->funcs && obj->funcs->mmap) {
+		ret = obj->funcs->mmap(obj, vma);
+		if (ret)
+			return ret;
+		vma->vm_private_data = obj;
+		drm_gem_object_get(obj);
+		return 0;
+	}
+
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	fil = kzalloc(sizeof(*fil), GFP_KERNEL);
 	if (!priv || !fil) {
-- 
2.18.1

[PATCH 2/8] drm/shmem: switch shmem helper to &drm_gem_object_funcs.mmap

[Gerd Hoffmann]

Switch gem shmem helper to the new mmap() workflow,
from &gem_driver.fops.mmap to &drm_gem_object_funcs.mmap.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/drm/drm_gem_shmem_helper.h      |  6 ++----
 drivers/gpu/drm/drm_gem_shmem_helper.c  | 26 ++++++++-----------------
 drivers/gpu/drm/panfrost/panfrost_gem.c |  2 +-
 drivers/gpu/drm/v3d/v3d_bo.c            |  2 +-
 drivers/gpu/drm/virtio/virtgpu_object.c |  2 +-
 5 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index 01f514521687..d89f2116c8ab 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -111,7 +111,7 @@ struct drm_gem_shmem_object {
 		.poll		= drm_poll,\
 		.read		= drm_read,\
 		.llseek		= noop_llseek,\
-		.mmap		= drm_gem_shmem_mmap, \
+		.mmap		= drm_gem_mmap, \
 	}
 
 struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t size);
@@ -143,9 +143,7 @@ drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
 int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
 			      struct drm_mode_create_dumb *args);
 
-int drm_gem_shmem_mmap(struct file *filp, struct vm_area_struct *vma);
-
-extern const struct vm_operations_struct drm_gem_shmem_vm_ops;
+int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
 
 void drm_gem_shmem_print_info(struct drm_printer *p, unsigned int indent,
 			      const struct drm_gem_object *obj);
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index f5918707672f..a104140154bb 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -32,7 +32,7 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
 	.get_sg_table = drm_gem_shmem_get_sg_table,
 	.vmap = drm_gem_shmem_vmap,
 	.vunmap = drm_gem_shmem_vunmap,
-	.vm_ops = &drm_gem_shmem_vm_ops,
+	.mmap = drm_gem_shmem_mmap,
 };
 
 /**
@@ -505,39 +505,30 @@ static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
 	drm_gem_vm_close(vma);
 }
 
-const struct vm_operations_struct drm_gem_shmem_vm_ops = {
+static const struct vm_operations_struct drm_gem_shmem_vm_ops = {
 	.fault = drm_gem_shmem_fault,
 	.open = drm_gem_shmem_vm_open,
 	.close = drm_gem_shmem_vm_close,
 };
-EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
 
 /**
  * drm_gem_shmem_mmap - Memory-map a shmem GEM object
- * @filp: File object
+ * @obj: gem object
  * @vma: VMA for the area to be mapped
  *
  * This function implements an augmented version of the GEM DRM file mmap
  * operation for shmem objects. Drivers which employ the shmem helpers should
- * use this function as their &file_operations.mmap handler in the DRM device file's
- * file_operations structure.
- *
- * Instead of directly referencing this function, drivers should use the
- * DEFINE_DRM_GEM_SHMEM_FOPS() macro.
+ * use this function as their &drm_gem_object_funcs.mmap handler.
  *
  * Returns:
  * 0 on success or a negative error code on failure.
  */
-int drm_gem_shmem_mmap(struct file *filp, struct vm_area_struct *vma)
+int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
 {
 	struct drm_gem_shmem_object *shmem;
 	int ret;
 
-	ret = drm_gem_mmap(filp, vma);
-	if (ret)
-		return ret;
-
-	shmem = to_drm_gem_shmem_obj(vma->vm_private_data);
+	shmem = to_drm_gem_shmem_obj(obj);
 
 	ret = drm_gem_shmem_get_pages(shmem);
 	if (ret) {
@@ -545,9 +536,8 @@ int drm_gem_shmem_mmap(struct file *filp, struct vm_area_struct *vma)
 		return ret;
 	}
 
-	/* VM_PFNMAP was set by drm_gem_mmap() */
-	vma->vm_flags &= ~VM_PFNMAP;
-	vma->vm_flags |= VM_MIXEDMAP;
+	vma->vm_flags |= (VM_MIXEDMAP|VM_DONTEXPAND);
+	vma->vm_ops = &drm_gem_shmem_vm_ops;
 
 	/* Remove the fake offset */
 	vma->vm_pgoff -= drm_vma_node_start(&shmem->base.vma_node);
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index acb07fe06580..deca0c30bbd4 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -112,7 +112,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
 	.get_sg_table = drm_gem_shmem_get_sg_table,
 	.vmap = drm_gem_shmem_vmap,
 	.vunmap = drm_gem_shmem_vunmap,
-	.vm_ops = &drm_gem_shmem_vm_ops,
+	.mmap = drm_gem_shmem_mmap,
 };
 
 /**
diff --git a/drivers/gpu/drm/v3d/v3d_bo.c b/drivers/gpu/drm/v3d/v3d_bo.c
index a22b75a3a533..edd299ab53d8 100644
--- a/drivers/gpu/drm/v3d/v3d_bo.c
+++ b/drivers/gpu/drm/v3d/v3d_bo.c
@@ -58,7 +58,7 @@ static const struct drm_gem_object_funcs v3d_gem_funcs = {
 	.get_sg_table = drm_gem_shmem_get_sg_table,
 	.vmap = drm_gem_shmem_vmap,
 	.vunmap = drm_gem_shmem_vunmap,
-	.vm_ops = &drm_gem_shmem_vm_ops,
+	.mmap = drm_gem_shmem_mmap,
 };
 
 /* gem_create_object function for allocating a BO struct and doing
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index 69a3d310ff70..017a9e0fc3bb 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -86,7 +86,7 @@ static const struct drm_gem_object_funcs virtio_gpu_gem_funcs = {
 	.get_sg_table = drm_gem_shmem_get_sg_table,
 	.vmap = drm_gem_shmem_vmap,
 	.vunmap = drm_gem_shmem_vunmap,
-	.vm_ops = &drm_gem_shmem_vm_ops,
+	.mmap = &drm_gem_shmem_mmap,
 };
 
 struct drm_gem_object *virtio_gpu_create_object(struct drm_device *dev,
-- 
2.18.1

[PATCH 3/8] drm/shmem: drop DEFINE_DRM_GEM_SHMEM_FOPS

[Gerd Hoffmann]

DEFINE_DRM_GEM_SHMEM_FOPS is identical
to DEFINE_DRM_GEM_FOPS now, drop it.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/drm/drm_gem_shmem_helper.h      | 26 -------------------------
 drivers/gpu/drm/cirrus/cirrus.c         |  2 +-
 drivers/gpu/drm/panfrost/panfrost_drv.c |  2 +-
 drivers/gpu/drm/tiny/gm12u320.c         |  2 +-
 drivers/gpu/drm/v3d/v3d_drv.c           |  2 +-
 drivers/gpu/drm/virtio/virtgpu_drv.c    |  2 +-
 6 files changed, 5 insertions(+), 31 deletions(-)

diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index d89f2116c8ab..6748379a0b44 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -88,32 +88,6 @@ struct drm_gem_shmem_object {
 #define to_drm_gem_shmem_obj(obj) \
 	container_of(obj, struct drm_gem_shmem_object, base)
 
-/**
- * DEFINE_DRM_GEM_SHMEM_FOPS() - Macro to generate file operations for shmem drivers
- * @name: name for the generated structure
- *
- * This macro autogenerates a suitable &struct file_operations for shmem based
- * drivers, which can be assigned to &drm_driver.fops. Note that this structure
- * cannot be shared between drivers, because it contains a reference to the
- * current module using THIS_MODULE.
- *
- * Note that the declaration is already marked as static - if you need a
- * non-static version of this you're probably doing it wrong and will break the
- * THIS_MODULE reference by accident.
- */
-#define DEFINE_DRM_GEM_SHMEM_FOPS(name) \
-	static const struct file_operations name = {\
-		.owner		= THIS_MODULE,\
-		.open		= drm_open,\
-		.release	= drm_release,\
-		.unlocked_ioctl	= drm_ioctl,\
-		.compat_ioctl	= drm_compat_ioctl,\
-		.poll		= drm_poll,\
-		.read		= drm_read,\
-		.llseek		= noop_llseek,\
-		.mmap		= drm_gem_mmap, \
-	}
-
 struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t size);
 void drm_gem_shmem_free_object(struct drm_gem_object *obj);
 
diff --git a/drivers/gpu/drm/cirrus/cirrus.c b/drivers/gpu/drm/cirrus/cirrus.c
index 89d9e6fdeb8c..7d08d067e1a4 100644
--- a/drivers/gpu/drm/cirrus/cirrus.c
+++ b/drivers/gpu/drm/cirrus/cirrus.c
@@ -510,7 +510,7 @@ static void cirrus_mode_config_init(struct cirrus_device *cirrus)
 
 /* ------------------------------------------------------------------ */
 
-DEFINE_DRM_GEM_SHMEM_FOPS(cirrus_fops);
+DEFINE_DRM_GEM_FOPS(cirrus_fops);
 
 static struct drm_driver cirrus_driver = {
 	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index d74442d71048..a78ce21594d7 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -470,7 +470,7 @@ static const struct drm_ioctl_desc panfrost_drm_driver_ioctls[] = {
 	PANFROST_IOCTL(MADVISE,		madvise,	DRM_RENDER_ALLOW),
 };
 
-DEFINE_DRM_GEM_SHMEM_FOPS(panfrost_drm_driver_fops);
+DEFINE_DRM_GEM_FOPS(panfrost_drm_driver_fops);
 
 /*
  * Panfrost driver version:
diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
index 03d0e2df6774..94fb1f593564 100644
--- a/drivers/gpu/drm/tiny/gm12u320.c
+++ b/drivers/gpu/drm/tiny/gm12u320.c
@@ -649,7 +649,7 @@ static void gm12u320_driver_release(struct drm_device *dev)
 	kfree(gm12u320);
 }
 
-DEFINE_DRM_GEM_SHMEM_FOPS(gm12u320_fops);
+DEFINE_DRM_GEM_FOPS(gm12u320_fops);
 
 static struct drm_driver gm12u320_drm_driver = {
 	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 3506ae2723ae..03e4fbe1b92b 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -169,7 +169,7 @@ v3d_postclose(struct drm_device *dev, struct drm_file *file)
 	kfree(v3d_priv);
 }
 
-DEFINE_DRM_GEM_SHMEM_FOPS(v3d_drm_fops);
+DEFINE_DRM_GEM_FOPS(v3d_drm_fops);
 
 /* DRM_AUTH is required on SUBMIT_CL for now, while we don't have GMP
  * protection between clients.  Note that render nodes would be be
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index a5cb58754f7d..8dee698c90ff 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -184,7 +184,7 @@ MODULE_AUTHOR("Dave Airlie <airlied@redhat.com>");
 MODULE_AUTHOR("Gerd Hoffmann <kraxel@redhat.com>");
 MODULE_AUTHOR("Alon Levy");
 
-DEFINE_DRM_GEM_SHMEM_FOPS(virtio_gpu_driver_fops);
+DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
 
 static struct drm_driver driver = {
 	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC,
-- 
2.18.1

[PATCH 4/8] drm/ttm: factor out ttm_bo_mmap_vma_setup

[Gerd Hoffmann]

Factor out ttm vma setup to a new function.  Reduces
code duplication a bit and allows to implement
&drm_gem_object_funcs.mmap in gem ttm helpers.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/drm/ttm/ttm_bo_api.h    |  8 ++++++
 drivers/gpu/drm/ttm/ttm_bo_vm.c | 47 ++++++++++++++++++---------------
 2 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 43c4929a2171..88c652f49602 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -734,6 +734,14 @@ int ttm_fbdev_mmap(struct vm_area_struct *vma, struct ttm_buffer_object *bo);
 int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
 		struct ttm_bo_device *bdev);
 
+/**
+ * ttm_bo_mmap_vma_setup - initialize vma for ttm bo mmap
+ *
+ * @bo: The buffer object.
+ * @vma: vma as input from the mmap method.
+ */
+void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_struct *vma);
+
 void *ttm_kmap_atomic_prot(struct page *page, pgprot_t prot);
 
 void ttm_kunmap_atomic_prot(void *addr, pgprot_t prot);
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index 4aa007edffb0..7c0e85c10e0e 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -426,6 +426,29 @@ static struct ttm_buffer_object *ttm_bo_vm_lookup(struct ttm_bo_device *bdev,
 	return bo;
 }
 
+void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_struct *vma)
+{
+	vma->vm_ops = &ttm_bo_vm_ops;
+
+	/*
+	 * Note: We're transferring the bo reference to
+	 * vma->vm_private_data here.
+	 */
+
+	vma->vm_private_data = bo;
+
+	/*
+	 * We'd like to use VM_PFNMAP on shared mappings, where
+	 * (vma->vm_flags & VM_SHARED) != 0, for performance reasons,
+	 * but for some reason VM_PFNMAP + x86 PAT + write-combine is very
+	 * bad for performance. Until that has been sorted out, use
+	 * VM_MIXEDMAP on all mappings. See freedesktop.org bug #75719
+	 */
+	vma->vm_flags |= VM_MIXEDMAP;
+	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
+}
+EXPORT_SYMBOL(ttm_bo_mmap_vma_setup);
+
 int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
 		struct ttm_bo_device *bdev)
 {
@@ -449,24 +472,7 @@ int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
 	if (unlikely(ret != 0))
 		goto out_unref;
 
-	vma->vm_ops = &ttm_bo_vm_ops;
-
-	/*
-	 * Note: We're transferring the bo reference to
-	 * vma->vm_private_data here.
-	 */
-
-	vma->vm_private_data = bo;
-
-	/*
-	 * We'd like to use VM_PFNMAP on shared mappings, where
-	 * (vma->vm_flags & VM_SHARED) != 0, for performance reasons,
-	 * but for some reason VM_PFNMAP + x86 PAT + write-combine is very
-	 * bad for performance. Until that has been sorted out, use
-	 * VM_MIXEDMAP on all mappings. See freedesktop.org bug #75719
-	 */
-	vma->vm_flags |= VM_MIXEDMAP;
-	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
+	ttm_bo_mmap_vma_setup(bo, vma);
 	return 0;
 out_unref:
 	ttm_bo_put(bo);
@@ -481,10 +487,7 @@ int ttm_fbdev_mmap(struct vm_area_struct *vma, struct ttm_buffer_object *bo)
 
 	ttm_bo_get(bo);
 
-	vma->vm_ops = &ttm_bo_vm_ops;
-	vma->vm_private_data = bo;
-	vma->vm_flags |= VM_MIXEDMAP;
-	vma->vm_flags |= VM_IO | VM_DONTEXPAND;
+	ttm_bo_mmap_vma_setup(bo, vma);
 	return 0;
 }
 EXPORT_SYMBOL(ttm_fbdev_mmap);
-- 
2.18.1

[PATCH 5/8] drm/ttm: add drm_gem_ttm_mmap()

[Gerd Hoffmann]

Add helper function to mmap ttm bo's using &drm_gem_object_funcs.mmap().

Note that with this code path access verification is done by
drm_gem_mmap() (which calls drm_vma_node_is_allowed(()).
The &ttm_bo_driver.verify_access() callback is is not used.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/drm/drm_gem_ttm_helper.h     |  2 ++
 drivers/gpu/drm/drm_gem_ttm_helper.c | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/drm/drm_gem_ttm_helper.h b/include/drm/drm_gem_ttm_helper.h
index 6268f89c5a48..118cef76f84f 100644
--- a/include/drm/drm_gem_ttm_helper.h
+++ b/include/drm/drm_gem_ttm_helper.h
@@ -15,5 +15,7 @@
 
 void drm_gem_ttm_print_info(struct drm_printer *p, unsigned int indent,
 			    const struct drm_gem_object *gem);
+int drm_gem_ttm_mmap(struct drm_gem_object *gem,
+		     struct vm_area_struct *vma);
 
 #endif
diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
index 9a4bafcf20df..34ce6cf78b35 100644
--- a/drivers/gpu/drm/drm_gem_ttm_helper.c
+++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
@@ -52,5 +52,24 @@ void drm_gem_ttm_print_info(struct drm_printer *p, unsigned int indent,
 }
 EXPORT_SYMBOL(drm_gem_ttm_print_info);
 
+/**
+ * drm_gem_ttm_mmap() - mmap &ttm_buffer_object
+ * @gem: GEM object.
+ * @vma: vm area.
+ *
+ * This function can be used as &drm_gem_object_funcs.mmap
+ * callback.
+ */
+int drm_gem_ttm_mmap(struct drm_gem_object *gem,
+		     struct vm_area_struct *vma)
+{
+	struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
+
+	ttm_bo_get(bo);
+	ttm_bo_mmap_vma_setup(bo, vma);
+	return 0;
+}
+EXPORT_SYMBOL(drm_gem_ttm_mmap);
+
 MODULE_DESCRIPTION("DRM gem ttm helpers");
 MODULE_LICENSE("GPL");
-- 
2.18.1

[PATCH 6/8] drm/vram: switch vram helper to &drm_gem_object_funcs.mmap()

[Gerd Hoffmann]

Wire up the new drm_gem_ttm_mmap() helper function,
use generic drm_gem_mmap for &fops.mmap and
delete dead drm_vram_mm_file_operations_mmap().

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/drm/drm_gem_vram_helper.h     |  9 +------
 drivers/gpu/drm/drm_gem_vram_helper.c | 34 +--------------------------
 2 files changed, 2 insertions(+), 41 deletions(-)

diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index 9aaef4f8c327..9d5526650291 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -180,13 +180,6 @@ struct drm_vram_mm *drm_vram_helper_alloc_mm(
 	struct drm_device *dev, uint64_t vram_base, size_t vram_size);
 void drm_vram_helper_release_mm(struct drm_device *dev);
 
-/*
- * Helpers for &struct file_operations
- */
-
-int drm_vram_mm_file_operations_mmap(
-	struct file *filp, struct vm_area_struct *vma);
-
 /**
  * define DRM_VRAM_MM_FILE_OPERATIONS - default callback functions for \
 	&struct file_operations
@@ -200,7 +193,7 @@ int drm_vram_mm_file_operations_mmap(
 	.poll		= drm_poll, \
 	.unlocked_ioctl = drm_ioctl, \
 	.compat_ioctl	= drm_compat_ioctl, \
-	.mmap		= drm_vram_mm_file_operations_mmap, \
+	.mmap		= drm_gem_mmap, \
 	.open		= drm_open, \
 	.release	= drm_release \
 
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 7bee80c6b6e8..e100b97ea6e3 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -681,6 +681,7 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs = {
 	.unpin	= drm_gem_vram_object_unpin,
 	.vmap	= drm_gem_vram_object_vmap,
 	.vunmap	= drm_gem_vram_object_vunmap,
+	.mmap   = drm_gem_ttm_mmap,
 	.print_info = drm_gem_ttm_print_info,
 };
 
@@ -915,12 +916,6 @@ static void drm_vram_mm_cleanup(struct drm_vram_mm *vmm)
 	ttm_bo_device_release(&vmm->bdev);
 }
 
-static int drm_vram_mm_mmap(struct file *filp, struct vm_area_struct *vma,
-			    struct drm_vram_mm *vmm)
-{
-	return ttm_bo_mmap(filp, vma, &vmm->bdev);
-}
-
 /*
  * Helpers for integration with struct drm_device
  */
@@ -976,30 +971,3 @@ void drm_vram_helper_release_mm(struct drm_device *dev)
 	dev->vram_mm = NULL;
 }
 EXPORT_SYMBOL(drm_vram_helper_release_mm);
-
-/*
- * Helpers for &struct file_operations
- */
-
-/**
- * drm_vram_mm_file_operations_mmap() - \
-	Implements &struct file_operations.mmap()
- * @filp:	the mapping's file structure
- * @vma:	the mapping's memory area
- *
- * Returns:
- * 0 on success, or
- * a negative error code otherwise.
- */
-int drm_vram_mm_file_operations_mmap(
-	struct file *filp, struct vm_area_struct *vma)
-{
-	struct drm_file *file_priv = filp->private_data;
-	struct drm_device *dev = file_priv->minor->dev;
-
-	if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
-		return -EINVAL;
-
-	return drm_vram_mm_mmap(filp, vma, dev->vram_mm);
-}
-EXPORT_SYMBOL(drm_vram_mm_file_operations_mmap);
-- 
2.18.1

[PATCH 7/8] drm/vram: drop verify_access

[Gerd Hoffmann]

Not needed any more.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index e100b97ea6e3..42ee80414273 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -469,13 +469,6 @@ static void drm_gem_vram_bo_driver_evict_flags(struct drm_gem_vram_object *gbo,
 	*pl = gbo->placement;
 }
 
-static int drm_gem_vram_bo_driver_verify_access(struct drm_gem_vram_object *gbo,
-						struct file *filp)
-{
-	return drm_vma_node_verify_access(&gbo->bo.base.vma_node,
-					  filp->private_data);
-}
-
 static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo,
 					       bool evict,
 					       struct ttm_mem_reg *new_mem)
@@ -767,20 +760,6 @@ static void bo_driver_evict_flags(struct ttm_buffer_object *bo,
 	drm_gem_vram_bo_driver_evict_flags(gbo, placement);
 }
 
-static int bo_driver_verify_access(struct ttm_buffer_object *bo,
-				   struct file *filp)
-{
-	struct drm_gem_vram_object *gbo;
-
-	/* TTM may pass BOs that are not GEM VRAM BOs. */
-	if (!drm_is_gem_vram(bo))
-		return -EINVAL;
-
-	gbo = drm_gem_vram_of_bo(bo);
-
-	return drm_gem_vram_bo_driver_verify_access(gbo, filp);
-}
-
 static void bo_driver_move_notify(struct ttm_buffer_object *bo,
 				  bool evict,
 				  struct ttm_mem_reg *new_mem)
@@ -837,7 +816,6 @@ static struct ttm_bo_driver bo_driver = {
 	.init_mem_type = bo_driver_init_mem_type,
 	.eviction_valuable = ttm_bo_eviction_valuable,
 	.evict_flags = bo_driver_evict_flags,
-	.verify_access = bo_driver_verify_access,
 	.move_notify = bo_driver_move_notify,
 	.io_mem_reserve = bo_driver_io_mem_reserve,
 	.io_mem_free = bo_driver_io_mem_free,
-- 
2.18.1

[PATCH 8/8] drm/vram: drop DRM_VRAM_MM_FILE_OPERATIONS

[Gerd Hoffmann]

Not needed any more because we don't have vram specific fops
any more.  DEFINE_DRM_GEM_FOPS() can be used instead.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/drm/drm_gem_vram_helper.h             | 18 ----
 include/drm/drm_vram_mm_helper.h              | 82 +++++++++++++++++++
 drivers/gpu/drm/ast/ast_drv.c                 |  5 +-
 drivers/gpu/drm/bochs/bochs_drv.c             |  5 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  5 +-
 drivers/gpu/drm/mgag200/mgag200_drv.c         |  5 +-
 drivers/gpu/drm/vboxvideo/vbox_drv.c          |  5 +-
 7 files changed, 87 insertions(+), 38 deletions(-)
 create mode 100644 include/drm/drm_vram_mm_helper.h

diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index 9d5526650291..3503ff784803 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -180,22 +180,4 @@ struct drm_vram_mm *drm_vram_helper_alloc_mm(
 	struct drm_device *dev, uint64_t vram_base, size_t vram_size);
 void drm_vram_helper_release_mm(struct drm_device *dev);
 
-/**
- * define DRM_VRAM_MM_FILE_OPERATIONS - default callback functions for \
-	&struct file_operations
- *
- * Drivers that use VRAM MM can use this macro to initialize
- * &struct file_operations with default functions.
- */
-#define DRM_VRAM_MM_FILE_OPERATIONS \
-	.llseek		= no_llseek, \
-	.read		= drm_read, \
-	.poll		= drm_poll, \
-	.unlocked_ioctl = drm_ioctl, \
-	.compat_ioctl	= drm_compat_ioctl, \
-	.mmap		= drm_gem_mmap, \
-	.open		= drm_open, \
-	.release	= drm_release \
-
-
 #endif
diff --git a/include/drm/drm_vram_mm_helper.h b/include/drm/drm_vram_mm_helper.h
new file mode 100644
index 000000000000..a47b49adba62
--- /dev/null
+++ b/include/drm/drm_vram_mm_helper.h
@@ -0,0 +1,82 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef DRM_VRAM_MM_HELPER_H
+#define DRM_VRAM_MM_HELPER_H
+
+#include <drm/drm_file.h>
+#include <drm/drm_ioctl.h>
+#include <drm/ttm/ttm_bo_driver.h>
+
+struct drm_device;
+
+/**
+ * struct drm_vram_mm_funcs - Callback functions for &struct drm_vram_mm
+ * @evict_flags:	Provides an implementation for struct \
+	&ttm_bo_driver.evict_flags
+ * @move_notify:	Provides an implementation for
+ *			struct &ttm_bo_driver.move_notify
+ *
+ * These callback function integrate VRAM MM with TTM buffer objects. New
+ * functions can be added if necessary.
+ */
+struct drm_vram_mm_funcs {
+	void (*evict_flags)(struct ttm_buffer_object *bo,
+			    struct ttm_placement *placement);
+	void (*move_notify)(struct ttm_buffer_object *bo, bool evict,
+			    struct ttm_mem_reg *new_mem);
+};
+
+/**
+ * struct drm_vram_mm - An instance of VRAM MM
+ * @vram_base:	Base address of the managed video memory
+ * @vram_size:	Size of the managed video memory in bytes
+ * @bdev:	The TTM BO device.
+ * @funcs:	TTM BO functions
+ *
+ * The fields &struct drm_vram_mm.vram_base and
+ * &struct drm_vram_mm.vrm_size are managed by VRAM MM, but are
+ * available for public read access. Use the field
+ * &struct drm_vram_mm.bdev to access the TTM BO device.
+ */
+struct drm_vram_mm {
+	uint64_t vram_base;
+	size_t vram_size;
+
+	struct ttm_bo_device bdev;
+
+	const struct drm_vram_mm_funcs *funcs;
+};
+
+/**
+ * drm_vram_mm_of_bdev() - \
+	Returns the container of type &struct ttm_bo_device for field bdev.
+ * @bdev:	the TTM BO device
+ *
+ * Returns:
+ * The containing instance of &struct drm_vram_mm
+ */
+static inline struct drm_vram_mm *drm_vram_mm_of_bdev(
+	struct ttm_bo_device *bdev)
+{
+	return container_of(bdev, struct drm_vram_mm, bdev);
+}
+
+int drm_vram_mm_debugfs_init(struct drm_minor *minor);
+int drm_vram_mm_init(struct drm_vram_mm *vmm, struct drm_device *dev,
+		     uint64_t vram_base, size_t vram_size,
+		     const struct drm_vram_mm_funcs *funcs);
+void drm_vram_mm_cleanup(struct drm_vram_mm *vmm);
+
+int drm_vram_mm_mmap(struct file *filp, struct vm_area_struct *vma,
+		     struct drm_vram_mm *vmm);
+
+/*
+ * Helpers for integration with struct drm_device
+ */
+
+struct drm_vram_mm *drm_vram_helper_alloc_mm(
+	struct drm_device *dev, uint64_t vram_base, size_t vram_size,
+	const struct drm_vram_mm_funcs *funcs);
+void drm_vram_helper_release_mm(struct drm_device *dev);
+
+#endif
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index e0e8770462bc..1f17794b0890 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -200,10 +200,7 @@ static struct pci_driver ast_pci_driver = {
 	.driver.pm = &ast_pm_ops,
 };
 
-static const struct file_operations ast_fops = {
-	.owner = THIS_MODULE,
-	DRM_VRAM_MM_FILE_OPERATIONS
-};
+DEFINE_DRM_GEM_FOPS(ast_fops);
 
 static struct drm_driver driver = {
 	.driver_features = DRIVER_MODESET | DRIVER_GEM,
diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
index 3b9b0d9bbc14..10460878414e 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -58,10 +58,7 @@ static int bochs_load(struct drm_device *dev)
 	return ret;
 }
 
-static const struct file_operations bochs_fops = {
-	.owner		= THIS_MODULE,
-	DRM_VRAM_MM_FILE_OPERATIONS
-};
+DEFINE_DRM_GEM_FOPS(bochs_fops);
 
 static struct drm_driver bochs_driver = {
 	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index f5b35fdef6f3..b6fdac91e502 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -26,10 +26,7 @@
 #include "hibmc_drm_drv.h"
 #include "hibmc_drm_regs.h"
 
-static const struct file_operations hibmc_fops = {
-	.owner		= THIS_MODULE,
-	DRM_VRAM_MM_FILE_OPERATIONS
-};
+DEFINE_DRM_GEM_FOPS(hibmc_fops);
 
 static irqreturn_t hibmc_drm_interrupt(int irq, void *arg)
 {
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 4f9df3b93598..397f8b0a9af8 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -58,10 +58,7 @@ static void mga_pci_remove(struct pci_dev *pdev)
 	drm_put_dev(dev);
 }
 
-static const struct file_operations mgag200_driver_fops = {
-	.owner = THIS_MODULE,
-	DRM_VRAM_MM_FILE_OPERATIONS
-};
+DEFINE_DRM_GEM_FOPS(mgag200_driver_fops);
 
 static struct drm_driver driver = {
 	.driver_features = DRIVER_GEM | DRIVER_MODESET,
diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c
index 862db495d111..0c37032c8b65 100644
--- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
+++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
@@ -189,10 +189,7 @@ static struct pci_driver vbox_pci_driver = {
 #endif
 };
 
-static const struct file_operations vbox_fops = {
-	.owner = THIS_MODULE,
-	DRM_VRAM_MM_FILE_OPERATIONS
-};
+DEFINE_DRM_GEM_FOPS(vbox_fops);
 
 static struct drm_driver driver = {
 	.driver_features =
-- 
2.18.1

Re: [PATCH 4/8] drm/ttm: factor out ttm_bo_mmap_vma_setup

[Thomas Zimmermann]

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

Hi

Am 13.09.19 um 14:29 schrieb Gerd Hoffmann:
> Factor out ttm vma setup to a new function.  Reduces
> code duplication a bit and allows to implement
> &drm_gem_object_funcs.mmap in gem ttm helpers.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/ttm/ttm_bo_api.h    |  8 ++++++
>  drivers/gpu/drm/ttm/ttm_bo_vm.c | 47 ++++++++++++++++++---------------
>  2 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 43c4929a2171..88c652f49602 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -734,6 +734,14 @@ int ttm_fbdev_mmap(struct vm_area_struct *vma, struct ttm_buffer_object *bo);
>  int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
>  		struct ttm_bo_device *bdev);
>  
> +/**
> + * ttm_bo_mmap_vma_setup - initialize vma for ttm bo mmap
> + *
> + * @bo: The buffer object.
> + * @vma: vma as input from the mmap method.
> + */
> +void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_struct *vma);
> +
>  void *ttm_kmap_atomic_prot(struct page *page, pgprot_t prot);
>  
>  void ttm_kunmap_atomic_prot(void *addr, pgprot_t prot);
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index 4aa007edffb0..7c0e85c10e0e 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -426,6 +426,29 @@ static struct ttm_buffer_object *ttm_bo_vm_lookup(struct ttm_bo_device *bdev,
>  	return bo;
>  }
>  
> +void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_struct *vma)
> +{
> +	vma->vm_ops = &ttm_bo_vm_ops;
> +
> +	/*
> +	 * Note: We're transferring the bo reference to
> +	 * vma->vm_private_data here.
> +	 */
> +
> +	vma->vm_private_data = bo;
> +
> +	/*
> +	 * We'd like to use VM_PFNMAP on shared mappings, where
> +	 * (vma->vm_flags & VM_SHARED) != 0, for performance reasons,
> +	 * but for some reason VM_PFNMAP + x86 PAT + write-combine is very
> +	 * bad for performance. Until that has been sorted out, use
> +	 * VM_MIXEDMAP on all mappings. See freedesktop.org bug #75719
> +	 */
> +	vma->vm_flags |= VM_MIXEDMAP;
> +	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
> +}
> +EXPORT_SYMBOL(ttm_bo_mmap_vma_setup);
> +
>  int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
>  		struct ttm_bo_device *bdev)
>  {
> @@ -449,24 +472,7 @@ int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
>  	if (unlikely(ret != 0))
>  		goto out_unref;
>  
> -	vma->vm_ops = &ttm_bo_vm_ops;
> -
> -	/*
> -	 * Note: We're transferring the bo reference to
> -	 * vma->vm_private_data here.
> -	 */
> -
> -	vma->vm_private_data = bo;
> -
> -	/*
> -	 * We'd like to use VM_PFNMAP on shared mappings, where
> -	 * (vma->vm_flags & VM_SHARED) != 0, for performance reasons,
> -	 * but for some reason VM_PFNMAP + x86 PAT + write-combine is very
> -	 * bad for performance. Until that has been sorted out, use
> -	 * VM_MIXEDMAP on all mappings. See freedesktop.org bug #75719
> -	 */
> -	vma->vm_flags |= VM_MIXEDMAP;
> -	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
> +	ttm_bo_mmap_vma_setup(bo, vma);
>  	return 0;
>  out_unref:
>  	ttm_bo_put(bo);
> @@ -481,10 +487,7 @@ int ttm_fbdev_mmap(struct vm_area_struct *vma, struct ttm_buffer_object *bo)
>  
>  	ttm_bo_get(bo);
>  
> -	vma->vm_ops = &ttm_bo_vm_ops;
> -	vma->vm_private_data = bo;
> -	vma->vm_flags |= VM_MIXEDMAP;
> -	vma->vm_flags |= VM_IO | VM_DONTEXPAND;
> +	ttm_bo_mmap_vma_setup(bo, vma);
Just double-checking:  ttm_bo_mmap_vma_setup() will set VM_DONTDUMP in
vm_flags. Is that OK?

Best regards
Thomas

>  	return 0;
>  }
>  EXPORT_SYMBOL(ttm_fbdev_mmap);
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


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

Re: [PATCH 5/8] drm/ttm: add drm_gem_ttm_mmap()

[Thomas Zimmermann]

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

Hi

Am 13.09.19 um 14:29 schrieb Gerd Hoffmann:
> Add helper function to mmap ttm bo's using &drm_gem_object_funcs.mmap().
> 
> Note that with this code path access verification is done by
> drm_gem_mmap() (which calls drm_vma_node_is_allowed(()).
> The &ttm_bo_driver.verify_access() callback is is not used.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_gem_ttm_helper.h     |  2 ++
>  drivers/gpu/drm/drm_gem_ttm_helper.c | 19 +++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/include/drm/drm_gem_ttm_helper.h b/include/drm/drm_gem_ttm_helper.h
> index 6268f89c5a48..118cef76f84f 100644
> --- a/include/drm/drm_gem_ttm_helper.h
> +++ b/include/drm/drm_gem_ttm_helper.h
> @@ -15,5 +15,7 @@
>  
>  void drm_gem_ttm_print_info(struct drm_printer *p, unsigned int indent,
>  			    const struct drm_gem_object *gem);
> +int drm_gem_ttm_mmap(struct drm_gem_object *gem,
> +		     struct vm_area_struct *vma);
>  
>  #endif
> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
> index 9a4bafcf20df..34ce6cf78b35 100644
> --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
> @@ -52,5 +52,24 @@ void drm_gem_ttm_print_info(struct drm_printer *p, unsigned int indent,
>  }
>  EXPORT_SYMBOL(drm_gem_ttm_print_info);
>  
> +/**
> + * drm_gem_ttm_mmap() - mmap &ttm_buffer_object
> + * @gem: GEM object.
> + * @vma: vm area.
> + *
> + * This function can be used as &drm_gem_object_funcs.mmap
> + * callback.
> + */
> +int drm_gem_ttm_mmap(struct drm_gem_object *gem,
> +		     struct vm_area_struct *vma)
> +{
> +	struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
> +
> +	ttm_bo_get(bo);
> +	ttm_bo_mmap_vma_setup(bo, vma);
> +	return 0;
> +}

This function looks like ttm_fbdev_mmap(). Can you use that instead?

Best regards
Thomas

> +EXPORT_SYMBOL(drm_gem_ttm_mmap);
> +
>  MODULE_DESCRIPTION("DRM gem ttm helpers");
>  MODULE_LICENSE("GPL");
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


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

Re: [PATCH 4/8] drm/ttm: factor out ttm_bo_mmap_vma_setup

[Thomas Zimmermann]

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

Hi

Am 13.09.19 um 14:29 schrieb Gerd Hoffmann:
> Factor out ttm vma setup to a new function.  Reduces
> code duplication a bit and allows to implement
> &drm_gem_object_funcs.mmap in gem ttm helpers.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/ttm/ttm_bo_api.h    |  8 ++++++
>  drivers/gpu/drm/ttm/ttm_bo_vm.c | 47 ++++++++++++++++++---------------
>  2 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 43c4929a2171..88c652f49602 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -734,6 +734,14 @@ int ttm_fbdev_mmap(struct vm_area_struct *vma, struct ttm_buffer_object *bo);
>  int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
>  		struct ttm_bo_device *bdev);
>  
> +/**
> + * ttm_bo_mmap_vma_setup - initialize vma for ttm bo mmap
> + *
> + * @bo: The buffer object.
> + * @vma: vma as input from the mmap method.
> + */
> +void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_struct *vma);
> +
>  void *ttm_kmap_atomic_prot(struct page *page, pgprot_t prot);
>  
>  void ttm_kunmap_atomic_prot(void *addr, pgprot_t prot);
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index 4aa007edffb0..7c0e85c10e0e 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -426,6 +426,29 @@ static struct ttm_buffer_object *ttm_bo_vm_lookup(struct ttm_bo_device *bdev,
>  	return bo;
>  }
>  
> +void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_struct *vma)
> +{
> +	vma->vm_ops = &ttm_bo_vm_ops;
> +
> +	/*
> +	 * Note: We're transferring the bo reference to
> +	 * vma->vm_private_data here.
> +	 */
> +
> +	vma->vm_private_data = bo;
> +
> +	/*
> +	 * We'd like to use VM_PFNMAP on shared mappings, where
> +	 * (vma->vm_flags & VM_SHARED) != 0, for performance reasons,
> +	 * but for some reason VM_PFNMAP + x86 PAT + write-combine is very
> +	 * bad for performance. Until that has been sorted out, use
> +	 * VM_MIXEDMAP on all mappings. See freedesktop.org bug #75719
> +	 */
> +	vma->vm_flags |= VM_MIXEDMAP;
> +	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
> +}
> +EXPORT_SYMBOL(ttm_bo_mmap_vma_setup);

To me, this function looks like an internal helper that should rather
remain internal. As mentioned in my reply to patch 5, maybe re-using
ttm_fbdev_mmap() could help.

Best regards
Thomas

> +
>  int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
>  		struct ttm_bo_device *bdev)
>  {
> @@ -449,24 +472,7 @@ int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
>  	if (unlikely(ret != 0))
>  		goto out_unref;
>  
> -	vma->vm_ops = &ttm_bo_vm_ops;
> -
> -	/*
> -	 * Note: We're transferring the bo reference to
> -	 * vma->vm_private_data here.
> -	 */
> -
> -	vma->vm_private_data = bo;
> -
> -	/*
> -	 * We'd like to use VM_PFNMAP on shared mappings, where
> -	 * (vma->vm_flags & VM_SHARED) != 0, for performance reasons,
> -	 * but for some reason VM_PFNMAP + x86 PAT + write-combine is very
> -	 * bad for performance. Until that has been sorted out, use
> -	 * VM_MIXEDMAP on all mappings. See freedesktop.org bug #75719
> -	 */
> -	vma->vm_flags |= VM_MIXEDMAP;
> -	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
> +	ttm_bo_mmap_vma_setup(bo, vma);
>  	return 0;
>  out_unref:
>  	ttm_bo_put(bo);
> @@ -481,10 +487,7 @@ int ttm_fbdev_mmap(struct vm_area_struct *vma, struct ttm_buffer_object *bo)
>  
>  	ttm_bo_get(bo);
>  
> -	vma->vm_ops = &ttm_bo_vm_ops;
> -	vma->vm_private_data = bo;
> -	vma->vm_flags |= VM_MIXEDMAP;
> -	vma->vm_flags |= VM_IO | VM_DONTEXPAND;
> +	ttm_bo_mmap_vma_setup(bo, vma);
>  	return 0;
>  }
>  EXPORT_SYMBOL(ttm_fbdev_mmap);
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


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

Re: [PATCH 6/8] drm/vram: switch vram helper to &drm_gem_object_funcs.mmap()

[Thomas Zimmermann]

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

Hi

Am 13.09.19 um 14:29 schrieb Gerd Hoffmann:
> Wire up the new drm_gem_ttm_mmap() helper function,
> use generic drm_gem_mmap for &fops.mmap and
> delete dead drm_vram_mm_file_operations_mmap().
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>  include/drm/drm_gem_vram_helper.h     |  9 +------
>  drivers/gpu/drm/drm_gem_vram_helper.c | 34 +--------------------------
>  2 files changed, 2 insertions(+), 41 deletions(-)
> 
> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> index 9aaef4f8c327..9d5526650291 100644
> --- a/include/drm/drm_gem_vram_helper.h
> +++ b/include/drm/drm_gem_vram_helper.h
> @@ -180,13 +180,6 @@ struct drm_vram_mm *drm_vram_helper_alloc_mm(
>  	struct drm_device *dev, uint64_t vram_base, size_t vram_size);
>  void drm_vram_helper_release_mm(struct drm_device *dev);
>  
> -/*
> - * Helpers for &struct file_operations
> - */
> -
> -int drm_vram_mm_file_operations_mmap(
> -	struct file *filp, struct vm_area_struct *vma);
> -
>  /**
>   * define DRM_VRAM_MM_FILE_OPERATIONS - default callback functions for \
>  	&struct file_operations
> @@ -200,7 +193,7 @@ int drm_vram_mm_file_operations_mmap(
>  	.poll		= drm_poll, \
>  	.unlocked_ioctl = drm_ioctl, \
>  	.compat_ioctl	= drm_compat_ioctl, \
> -	.mmap		= drm_vram_mm_file_operations_mmap, \
> +	.mmap		= drm_gem_mmap, \
>  	.open		= drm_open, \
>  	.release	= drm_release \
>  
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index 7bee80c6b6e8..e100b97ea6e3 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -681,6 +681,7 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs = {
>  	.unpin	= drm_gem_vram_object_unpin,
>  	.vmap	= drm_gem_vram_object_vmap,
>  	.vunmap	= drm_gem_vram_object_vunmap,
> +	.mmap   = drm_gem_ttm_mmap,
>  	.print_info = drm_gem_ttm_print_info,
>  };
>  
> @@ -915,12 +916,6 @@ static void drm_vram_mm_cleanup(struct drm_vram_mm *vmm)
>  	ttm_bo_device_release(&vmm->bdev);
>  }
>  
> -static int drm_vram_mm_mmap(struct file *filp, struct vm_area_struct *vma,
> -			    struct drm_vram_mm *vmm)
> -{
> -	return ttm_bo_mmap(filp, vma, &vmm->bdev);
> -}
> -
>  /*
>   * Helpers for integration with struct drm_device
>   */
> @@ -976,30 +971,3 @@ void drm_vram_helper_release_mm(struct drm_device *dev)
>  	dev->vram_mm = NULL;
>  }
>  EXPORT_SYMBOL(drm_vram_helper_release_mm);
> -
> -/*
> - * Helpers for &struct file_operations
> - */
> -
> -/**
> - * drm_vram_mm_file_operations_mmap() - \
> -	Implements &struct file_operations.mmap()
> - * @filp:	the mapping's file structure
> - * @vma:	the mapping's memory area
> - *
> - * Returns:
> - * 0 on success, or
> - * a negative error code otherwise.
> - */
> -int drm_vram_mm_file_operations_mmap(
> -	struct file *filp, struct vm_area_struct *vma)
> -{
> -	struct drm_file *file_priv = filp->private_data;
> -	struct drm_device *dev = file_priv->minor->dev;
> -
> -	if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
> -		return -EINVAL;
> -
> -	return drm_vram_mm_mmap(filp, vma, dev->vram_mm);
> -}
> -EXPORT_SYMBOL(drm_vram_mm_file_operations_mmap);
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


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

Re: [PATCH 7/8] drm/vram: drop verify_access

[Thomas Zimmermann]

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

Hi

Am 13.09.19 um 14:29 schrieb Gerd Hoffmann:
> Not needed any more.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>  drivers/gpu/drm/drm_gem_vram_helper.c | 22 ----------------------
>  1 file changed, 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index e100b97ea6e3..42ee80414273 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -469,13 +469,6 @@ static void drm_gem_vram_bo_driver_evict_flags(struct drm_gem_vram_object *gbo,
>  	*pl = gbo->placement;
>  }
>  
> -static int drm_gem_vram_bo_driver_verify_access(struct drm_gem_vram_object *gbo,
> -						struct file *filp)
> -{
> -	return drm_vma_node_verify_access(&gbo->bo.base.vma_node,
> -					  filp->private_data);
> -}
> -
>  static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo,
>  					       bool evict,
>  					       struct ttm_mem_reg *new_mem)
> @@ -767,20 +760,6 @@ static void bo_driver_evict_flags(struct ttm_buffer_object *bo,
>  	drm_gem_vram_bo_driver_evict_flags(gbo, placement);
>  }
>  
> -static int bo_driver_verify_access(struct ttm_buffer_object *bo,
> -				   struct file *filp)
> -{
> -	struct drm_gem_vram_object *gbo;
> -
> -	/* TTM may pass BOs that are not GEM VRAM BOs. */
> -	if (!drm_is_gem_vram(bo))
> -		return -EINVAL;
> -
> -	gbo = drm_gem_vram_of_bo(bo);
> -
> -	return drm_gem_vram_bo_driver_verify_access(gbo, filp);
> -}
> -
>  static void bo_driver_move_notify(struct ttm_buffer_object *bo,
>  				  bool evict,
>  				  struct ttm_mem_reg *new_mem)
> @@ -837,7 +816,6 @@ static struct ttm_bo_driver bo_driver = {
>  	.init_mem_type = bo_driver_init_mem_type,
>  	.eviction_valuable = ttm_bo_eviction_valuable,
>  	.evict_flags = bo_driver_evict_flags,
> -	.verify_access = bo_driver_verify_access,
>  	.move_notify = bo_driver_move_notify,
>  	.io_mem_reserve = bo_driver_io_mem_reserve,
>  	.io_mem_free = bo_driver_io_mem_free,
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


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

Re: [PATCH 8/8] drm/vram: drop DRM_VRAM_MM_FILE_OPERATIONS

[Thomas Zimmermann]

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



Am 13.09.19 um 14:29 schrieb Gerd Hoffmann:
> Not needed any more because we don't have vram specific fops
> any more.  DEFINE_DRM_GEM_FOPS() can be used instead.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_gem_vram_helper.h             | 18 ----
>  include/drm/drm_vram_mm_helper.h              | 82 +++++++++++++++++++
>  drivers/gpu/drm/ast/ast_drv.c                 |  5 +-
>  drivers/gpu/drm/bochs/bochs_drv.c             |  5 +-
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  5 +-
>  drivers/gpu/drm/mgag200/mgag200_drv.c         |  5 +-
>  drivers/gpu/drm/vboxvideo/vbox_drv.c          |  5 +-
>  7 files changed, 87 insertions(+), 38 deletions(-)
>  create mode 100644 include/drm/drm_vram_mm_helper.h
> 
> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> index 9d5526650291..3503ff784803 100644
> --- a/include/drm/drm_gem_vram_helper.h
> +++ b/include/drm/drm_gem_vram_helper.h
> @@ -180,22 +180,4 @@ struct drm_vram_mm *drm_vram_helper_alloc_mm(
>  	struct drm_device *dev, uint64_t vram_base, size_t vram_size);
>  void drm_vram_helper_release_mm(struct drm_device *dev);
>  
> -/**
> - * define DRM_VRAM_MM_FILE_OPERATIONS - default callback functions for \
> -	&struct file_operations
> - *
> - * Drivers that use VRAM MM can use this macro to initialize
> - * &struct file_operations with default functions.
> - */
> -#define DRM_VRAM_MM_FILE_OPERATIONS \
> -	.llseek		= no_llseek, \
> -	.read		= drm_read, \
> -	.poll		= drm_poll, \
> -	.unlocked_ioctl = drm_ioctl, \
> -	.compat_ioctl	= drm_compat_ioctl, \
> -	.mmap		= drm_gem_mmap, \
> -	.open		= drm_open, \
> -	.release	= drm_release \
> -
> -
>  #endif
> diff --git a/include/drm/drm_vram_mm_helper.h b/include/drm/drm_vram_mm_helper.h
> new file mode 100644

Please rebase onto the latest drm-tip. This entire file has been removed
in a recent patch.

With this change applied:

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> index 000000000000..a47b49adba62
> --- /dev/null
> +++ b/include/drm/drm_vram_mm_helper.h
> @@ -0,0 +1,82 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef DRM_VRAM_MM_HELPER_H
> +#define DRM_VRAM_MM_HELPER_H
> +
> +#include <drm/drm_file.h>
> +#include <drm/drm_ioctl.h>
> +#include <drm/ttm/ttm_bo_driver.h>
> +
> +struct drm_device;
> +
> +/**
> + * struct drm_vram_mm_funcs - Callback functions for &struct drm_vram_mm
> + * @evict_flags:	Provides an implementation for struct \
> +	&ttm_bo_driver.evict_flags
> + * @move_notify:	Provides an implementation for
> + *			struct &ttm_bo_driver.move_notify
> + *
> + * These callback function integrate VRAM MM with TTM buffer objects. New
> + * functions can be added if necessary.
> + */
> +struct drm_vram_mm_funcs {
> +	void (*evict_flags)(struct ttm_buffer_object *bo,
> +			    struct ttm_placement *placement);
> +	void (*move_notify)(struct ttm_buffer_object *bo, bool evict,
> +			    struct ttm_mem_reg *new_mem);
> +};
> +
> +/**
> + * struct drm_vram_mm - An instance of VRAM MM
> + * @vram_base:	Base address of the managed video memory
> + * @vram_size:	Size of the managed video memory in bytes
> + * @bdev:	The TTM BO device.
> + * @funcs:	TTM BO functions
> + *
> + * The fields &struct drm_vram_mm.vram_base and
> + * &struct drm_vram_mm.vrm_size are managed by VRAM MM, but are
> + * available for public read access. Use the field
> + * &struct drm_vram_mm.bdev to access the TTM BO device.
> + */
> +struct drm_vram_mm {
> +	uint64_t vram_base;
> +	size_t vram_size;
> +
> +	struct ttm_bo_device bdev;
> +
> +	const struct drm_vram_mm_funcs *funcs;
> +};
> +
> +/**
> + * drm_vram_mm_of_bdev() - \
> +	Returns the container of type &struct ttm_bo_device for field bdev.
> + * @bdev:	the TTM BO device
> + *
> + * Returns:
> + * The containing instance of &struct drm_vram_mm
> + */
> +static inline struct drm_vram_mm *drm_vram_mm_of_bdev(
> +	struct ttm_bo_device *bdev)
> +{
> +	return container_of(bdev, struct drm_vram_mm, bdev);
> +}
> +
> +int drm_vram_mm_debugfs_init(struct drm_minor *minor);
> +int drm_vram_mm_init(struct drm_vram_mm *vmm, struct drm_device *dev,
> +		     uint64_t vram_base, size_t vram_size,
> +		     const struct drm_vram_mm_funcs *funcs);
> +void drm_vram_mm_cleanup(struct drm_vram_mm *vmm);
> +
> +int drm_vram_mm_mmap(struct file *filp, struct vm_area_struct *vma,
> +		     struct drm_vram_mm *vmm);
> +
> +/*
> + * Helpers for integration with struct drm_device
> + */
> +
> +struct drm_vram_mm *drm_vram_helper_alloc_mm(
> +	struct drm_device *dev, uint64_t vram_base, size_t vram_size,
> +	const struct drm_vram_mm_funcs *funcs);
> +void drm_vram_helper_release_mm(struct drm_device *dev);
> +
> +#endif
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index e0e8770462bc..1f17794b0890 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -200,10 +200,7 @@ static struct pci_driver ast_pci_driver = {
>  	.driver.pm = &ast_pm_ops,
>  };
>  
> -static const struct file_operations ast_fops = {
> -	.owner = THIS_MODULE,
> -	DRM_VRAM_MM_FILE_OPERATIONS
> -};
> +DEFINE_DRM_GEM_FOPS(ast_fops);
>  
>  static struct drm_driver driver = {
>  	.driver_features = DRIVER_MODESET | DRIVER_GEM,
> diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
> index 3b9b0d9bbc14..10460878414e 100644
> --- a/drivers/gpu/drm/bochs/bochs_drv.c
> +++ b/drivers/gpu/drm/bochs/bochs_drv.c
> @@ -58,10 +58,7 @@ static int bochs_load(struct drm_device *dev)
>  	return ret;
>  }
>  
> -static const struct file_operations bochs_fops = {
> -	.owner		= THIS_MODULE,
> -	DRM_VRAM_MM_FILE_OPERATIONS
> -};
> +DEFINE_DRM_GEM_FOPS(bochs_fops);
>  
>  static struct drm_driver bochs_driver = {
>  	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index f5b35fdef6f3..b6fdac91e502 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -26,10 +26,7 @@
>  #include "hibmc_drm_drv.h"
>  #include "hibmc_drm_regs.h"
>  
> -static const struct file_operations hibmc_fops = {
> -	.owner		= THIS_MODULE,
> -	DRM_VRAM_MM_FILE_OPERATIONS
> -};
> +DEFINE_DRM_GEM_FOPS(hibmc_fops);
>  
>  static irqreturn_t hibmc_drm_interrupt(int irq, void *arg)
>  {
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 4f9df3b93598..397f8b0a9af8 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -58,10 +58,7 @@ static void mga_pci_remove(struct pci_dev *pdev)
>  	drm_put_dev(dev);
>  }
>  
> -static const struct file_operations mgag200_driver_fops = {
> -	.owner = THIS_MODULE,
> -	DRM_VRAM_MM_FILE_OPERATIONS
> -};
> +DEFINE_DRM_GEM_FOPS(mgag200_driver_fops);
>  
>  static struct drm_driver driver = {
>  	.driver_features = DRIVER_GEM | DRIVER_MODESET,
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> index 862db495d111..0c37032c8b65 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> @@ -189,10 +189,7 @@ static struct pci_driver vbox_pci_driver = {
>  #endif
>  };
>  
> -static const struct file_operations vbox_fops = {
> -	.owner = THIS_MODULE,
> -	DRM_VRAM_MM_FILE_OPERATIONS
> -};
> +DEFINE_DRM_GEM_FOPS(vbox_fops);
>  
>  static struct drm_driver driver = {
>  	.driver_features =
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


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

Re: [PATCH 2/8] drm/shmem: switch shmem helper to &drm_gem_object_funcs.mmap

[Steven Price]

On 13/09/2019 13:29, Gerd Hoffmann wrote:
> Switch gem shmem helper to the new mmap() workflow,
> from &gem_driver.fops.mmap to &drm_gem_object_funcs.mmap.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_gem_shmem_helper.h      |  6 ++----
>  drivers/gpu/drm/drm_gem_shmem_helper.c  | 26 ++++++++-----------------
>  drivers/gpu/drm/panfrost/panfrost_gem.c |  2 +-
>  drivers/gpu/drm/v3d/v3d_bo.c            |  2 +-
>  drivers/gpu/drm/virtio/virtgpu_object.c |  2 +-
>  5 files changed, 13 insertions(+), 25 deletions(-)
> 
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index 01f514521687..d89f2116c8ab 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -111,7 +111,7 @@ struct drm_gem_shmem_object {
>  		.poll		= drm_poll,\
>  		.read		= drm_read,\
>  		.llseek		= noop_llseek,\
> -		.mmap		= drm_gem_shmem_mmap, \
> +		.mmap		= drm_gem_mmap, \
>  	}
>  
>  struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t size);
> @@ -143,9 +143,7 @@ drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
>  int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>  			      struct drm_mode_create_dumb *args);
>  
> -int drm_gem_shmem_mmap(struct file *filp, struct vm_area_struct *vma);
> -
> -extern const struct vm_operations_struct drm_gem_shmem_vm_ops;
> +int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
>  
>  void drm_gem_shmem_print_info(struct drm_printer *p, unsigned int indent,
>  			      const struct drm_gem_object *obj);
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index f5918707672f..a104140154bb 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -32,7 +32,7 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
>  	.get_sg_table = drm_gem_shmem_get_sg_table,
>  	.vmap = drm_gem_shmem_vmap,
>  	.vunmap = drm_gem_shmem_vunmap,
> -	.vm_ops = &drm_gem_shmem_vm_ops,
> +	.mmap = drm_gem_shmem_mmap,
>  };
>  
>  /**
> @@ -505,39 +505,30 @@ static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
>  	drm_gem_vm_close(vma);
>  }
>  
> -const struct vm_operations_struct drm_gem_shmem_vm_ops = {
> +static const struct vm_operations_struct drm_gem_shmem_vm_ops = {
>  	.fault = drm_gem_shmem_fault,
>  	.open = drm_gem_shmem_vm_open,
>  	.close = drm_gem_shmem_vm_close,
>  };
> -EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
>  
>  /**
>   * drm_gem_shmem_mmap - Memory-map a shmem GEM object
> - * @filp: File object
> + * @obj: gem object
>   * @vma: VMA for the area to be mapped
>   *
>   * This function implements an augmented version of the GEM DRM file mmap
>   * operation for shmem objects. Drivers which employ the shmem helpers should
> - * use this function as their &file_operations.mmap handler in the DRM device file's
> - * file_operations structure.
> - *
> - * Instead of directly referencing this function, drivers should use the
> - * DEFINE_DRM_GEM_SHMEM_FOPS() macro.
> + * use this function as their &drm_gem_object_funcs.mmap handler.
>   *
>   * Returns:
>   * 0 on success or a negative error code on failure.
>   */
> -int drm_gem_shmem_mmap(struct file *filp, struct vm_area_struct *vma)
> +int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>  {
>  	struct drm_gem_shmem_object *shmem;
>  	int ret;
>  
> -	ret = drm_gem_mmap(filp, vma);
> -	if (ret)
> -		return ret;
> -
> -	shmem = to_drm_gem_shmem_obj(vma->vm_private_data);
> +	shmem = to_drm_gem_shmem_obj(obj);
>  
>  	ret = drm_gem_shmem_get_pages(shmem);
>  	if (ret) {
> @@ -545,9 +536,8 @@ int drm_gem_shmem_mmap(struct file *filp, struct vm_area_struct *vma)
>  		return ret;
>  	}
>  
> -	/* VM_PFNMAP was set by drm_gem_mmap() */
> -	vma->vm_flags &= ~VM_PFNMAP;
> -	vma->vm_flags |= VM_MIXEDMAP;
> +	vma->vm_flags |= (VM_MIXEDMAP|VM_DONTEXPAND);

I'm finding this a bit hard to follow - but I think here we've lost
VM_IO and VM_DONTDUMP which used to be set by drm_gem_mmap().

Also it looks like nothing is fiddling vma->vm_page_prot anymore.

Steve

> +	vma->vm_ops = &drm_gem_shmem_vm_ops;
>  
>  	/* Remove the fake offset */
>  	vma->vm_pgoff -= drm_vma_node_start(&shmem->base.vma_node);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index acb07fe06580..deca0c30bbd4 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -112,7 +112,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
>  	.get_sg_table = drm_gem_shmem_get_sg_table,
>  	.vmap = drm_gem_shmem_vmap,
>  	.vunmap = drm_gem_shmem_vunmap,
> -	.vm_ops = &drm_gem_shmem_vm_ops,
> +	.mmap = drm_gem_shmem_mmap,
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/v3d/v3d_bo.c b/drivers/gpu/drm/v3d/v3d_bo.c
> index a22b75a3a533..edd299ab53d8 100644
> --- a/drivers/gpu/drm/v3d/v3d_bo.c
> +++ b/drivers/gpu/drm/v3d/v3d_bo.c
> @@ -58,7 +58,7 @@ static const struct drm_gem_object_funcs v3d_gem_funcs = {
>  	.get_sg_table = drm_gem_shmem_get_sg_table,
>  	.vmap = drm_gem_shmem_vmap,
>  	.vunmap = drm_gem_shmem_vunmap,
> -	.vm_ops = &drm_gem_shmem_vm_ops,
> +	.mmap = drm_gem_shmem_mmap,
>  };
>  
>  /* gem_create_object function for allocating a BO struct and doing
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
> index 69a3d310ff70..017a9e0fc3bb 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -86,7 +86,7 @@ static const struct drm_gem_object_funcs virtio_gpu_gem_funcs = {
>  	.get_sg_table = drm_gem_shmem_get_sg_table,
>  	.vmap = drm_gem_shmem_vmap,
>  	.vunmap = drm_gem_shmem_vunmap,
> -	.vm_ops = &drm_gem_shmem_vm_ops,
> +	.mmap = &drm_gem_shmem_mmap,
>  };
>  
>  struct drm_gem_object *virtio_gpu_create_object(struct drm_device *dev,
> 

Re: [PATCH 3/8] drm/shmem: drop DEFINE_DRM_GEM_SHMEM_FOPS

[Rob Herring]

On Fri, Sep 13, 2019 at 7:29 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>

Version? Pretty sure this is not v1.

> DEFINE_DRM_GEM_SHMEM_FOPS is identical
> to DEFINE_DRM_GEM_FOPS now, drop it.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_gem_shmem_helper.h      | 26 -------------------------
>  drivers/gpu/drm/cirrus/cirrus.c         |  2 +-
>  drivers/gpu/drm/panfrost/panfrost_drv.c |  2 +-
>  drivers/gpu/drm/tiny/gm12u320.c         |  2 +-
>  drivers/gpu/drm/v3d/v3d_drv.c           |  2 +-
>  drivers/gpu/drm/virtio/virtgpu_drv.c    |  2 +-
>  6 files changed, 5 insertions(+), 31 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/8] drm/shmem: switch shmem helper to &drm_gem_object_funcs.mmap

[Gerd Hoffmann]

  Hi,

> > -	/* VM_PFNMAP was set by drm_gem_mmap() */
> > -	vma->vm_flags &= ~VM_PFNMAP;
> > -	vma->vm_flags |= VM_MIXEDMAP;
> > +	vma->vm_flags |= (VM_MIXEDMAP|VM_DONTEXPAND);
> 
> I'm finding this a bit hard to follow - but I think here we've lost
> VM_IO and VM_DONTDUMP which used to be set by drm_gem_mmap().

Yep.  Intentional, but I think I better split that off to a separate
patch with a commit message explaining things.

> Also it looks like nothing is fiddling vma->vm_page_prot anymore.

Oops, that one was not intentional.  Will fix.

cheers,
  Gerd

Re: [PATCH 3/8] drm/shmem: drop DEFINE_DRM_GEM_SHMEM_FOPS

[Gerd Hoffmann]

On Mon, Sep 16, 2019 at 05:07:14PM -0500, Rob Herring wrote:
> On Fri, Sep 13, 2019 at 7:29 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> 
> Version? Pretty sure this is not v1.

Yep, was posted as part of a longer series before.

Splitted the long series into multiple smaller ones by cherry-picking
patches into new branches, which re-started the numbering.

Sorry for the confusion,
  Gerd

Re: [PATCH 4/8] drm/ttm: factor out ttm_bo_mmap_vma_setup

[Gerd Hoffmann]

On Fri, Sep 13, 2019 at 02:56:09PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 13.09.19 um 14:29 schrieb Gerd Hoffmann:
> > Factor out ttm vma setup to a new function.  Reduces
> > code duplication a bit and allows to implement
> > &drm_gem_object_funcs.mmap in gem ttm helpers.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  include/drm/ttm/ttm_bo_api.h    |  8 ++++++
> >  drivers/gpu/drm/ttm/ttm_bo_vm.c | 47 ++++++++++++++++++---------------
> >  2 files changed, 33 insertions(+), 22 deletions(-)
> > 
> > diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> > index 43c4929a2171..88c652f49602 100644
> > --- a/include/drm/ttm/ttm_bo_api.h
> > +++ b/include/drm/ttm/ttm_bo_api.h
> > @@ -734,6 +734,14 @@ int ttm_fbdev_mmap(struct vm_area_struct *vma, struct ttm_buffer_object *bo);
> >  int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
> >  		struct ttm_bo_device *bdev);
> >  
> > +/**
> > + * ttm_bo_mmap_vma_setup - initialize vma for ttm bo mmap
> > + *
> > + * @bo: The buffer object.
> > + * @vma: vma as input from the mmap method.
> > + */
> > +void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_struct *vma);
> > +
> >  void *ttm_kmap_atomic_prot(struct page *page, pgprot_t prot);
> >  
> >  void ttm_kunmap_atomic_prot(void *addr, pgprot_t prot);
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> > index 4aa007edffb0..7c0e85c10e0e 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> > @@ -426,6 +426,29 @@ static struct ttm_buffer_object *ttm_bo_vm_lookup(struct ttm_bo_device *bdev,
> >  	return bo;
> >  }
> >  
> > +void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_struct *vma)
> > +{
> > +	vma->vm_ops = &ttm_bo_vm_ops;
> > +
> > +	/*
> > +	 * Note: We're transferring the bo reference to
> > +	 * vma->vm_private_data here.
> > +	 */
> > +
> > +	vma->vm_private_data = bo;
> > +
> > +	/*
> > +	 * We'd like to use VM_PFNMAP on shared mappings, where
> > +	 * (vma->vm_flags & VM_SHARED) != 0, for performance reasons,
> > +	 * but for some reason VM_PFNMAP + x86 PAT + write-combine is very
> > +	 * bad for performance. Until that has been sorted out, use
> > +	 * VM_MIXEDMAP on all mappings. See freedesktop.org bug #75719
> > +	 */
> > +	vma->vm_flags |= VM_MIXEDMAP;
> > +	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
> > +}
> > +EXPORT_SYMBOL(ttm_bo_mmap_vma_setup);
> > +
> >  int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
> >  		struct ttm_bo_device *bdev)
> >  {
> > @@ -449,24 +472,7 @@ int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
> >  	if (unlikely(ret != 0))
> >  		goto out_unref;
> >  
> > -	vma->vm_ops = &ttm_bo_vm_ops;
> > -
> > -	/*
> > -	 * Note: We're transferring the bo reference to
> > -	 * vma->vm_private_data here.
> > -	 */
> > -
> > -	vma->vm_private_data = bo;
> > -
> > -	/*
> > -	 * We'd like to use VM_PFNMAP on shared mappings, where
> > -	 * (vma->vm_flags & VM_SHARED) != 0, for performance reasons,
> > -	 * but for some reason VM_PFNMAP + x86 PAT + write-combine is very
> > -	 * bad for performance. Until that has been sorted out, use
> > -	 * VM_MIXEDMAP on all mappings. See freedesktop.org bug #75719
> > -	 */
> > -	vma->vm_flags |= VM_MIXEDMAP;
> > -	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
> > +	ttm_bo_mmap_vma_setup(bo, vma);
> >  	return 0;
> >  out_unref:
> >  	ttm_bo_put(bo);
> > @@ -481,10 +487,7 @@ int ttm_fbdev_mmap(struct vm_area_struct *vma, struct ttm_buffer_object *bo)
> >  
> >  	ttm_bo_get(bo);
> >  
> > -	vma->vm_ops = &ttm_bo_vm_ops;
> > -	vma->vm_private_data = bo;
> > -	vma->vm_flags |= VM_MIXEDMAP;
> > -	vma->vm_flags |= VM_IO | VM_DONTEXPAND;
> > +	ttm_bo_mmap_vma_setup(bo, vma);
> Just double-checking:  ttm_bo_mmap_vma_setup() will set VM_DONTDUMP in
> vm_flags. Is that OK?

Should be ok, according to daniel vetter this most likely dates back to
the days where drivers exposed hardware registers mmap()able object.

But doing this as separate patch is probably a good idea ...

cheers,
  Gerd

Re: [PATCH 4/8] drm/ttm: factor out ttm_bo_mmap_vma_setup

[Gerd Hoffmann]

On Fri, Sep 13, 2019 at 03:05:34PM +0200, Thomas Zimmermann wrote:

> > +void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_struct *vma)
> > +{
> > +	vma->vm_ops = &ttm_bo_vm_ops;
> > +
> > +	/*
> > +	 * Note: We're transferring the bo reference to
> > +	 * vma->vm_private_data here.
> > +	 */
> > +
> > +	vma->vm_private_data = bo;
> > +
> > +	/*
> > +	 * We'd like to use VM_PFNMAP on shared mappings, where
> > +	 * (vma->vm_flags & VM_SHARED) != 0, for performance reasons,
> > +	 * but for some reason VM_PFNMAP + x86 PAT + write-combine is very
> > +	 * bad for performance. Until that has been sorted out, use
> > +	 * VM_MIXEDMAP on all mappings. See freedesktop.org bug #75719
> > +	 */
> > +	vma->vm_flags |= VM_MIXEDMAP;
> > +	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
> > +}
> > +EXPORT_SYMBOL(ttm_bo_mmap_vma_setup);
> 
> To me, this function looks like an internal helper that should rather
> remain internal.

Well, I'm moving that to a helper exactly to avoid drm gem ttm helpers
messing with ttm internals.  To not them initialize vm_flags for
example, and to avoid exporting ttm_bo_vm_ops.  Also to make sure ttm bo
vma's are initialized the same way no matter which code path was taken
to mmap the object.

> As mentioned in my reply to patch 5, maybe re-using
> ttm_fbdev_mmap() could help.

No, the check in that function prevents that from working.

cheers,
  Gerd

Re: [PATCH 8/8] drm/vram: drop DRM_VRAM_MM_FILE_OPERATIONS

[Gerd Hoffmann]

> >  include/drm/drm_vram_mm_helper.h              | 82 +++++++++++++++++++

> > diff --git a/include/drm/drm_vram_mm_helper.h b/include/drm/drm_vram_mm_helper.h
> > new file mode 100644
> 
> Please rebase onto the latest drm-tip. This entire file has been removed
> in a recent patch.

I did rebase already, then re-added the file by mistake.  Didn't pay
enough attention while solving the conflict.  Fixed now.

cheers,
  Gerd

Re: [PATCH 4/8] drm/ttm: factor out ttm_bo_mmap_vma_setup

[Thomas Zimmermann]

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

Hi Gerd

Am 17.09.19 um 10:34 schrieb Gerd Hoffmann:
> On Fri, Sep 13, 2019 at 03:05:34PM +0200, Thomas Zimmermann wrote:
> 
>>> +void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_struct *vma)
>>> +{
>>> +	vma->vm_ops = &ttm_bo_vm_ops;
>>> +
>>> +	/*
>>> +	 * Note: We're transferring the bo reference to
>>> +	 * vma->vm_private_data here.
>>> +	 */
>>> +
>>> +	vma->vm_private_data = bo;
>>> +
>>> +	/*
>>> +	 * We'd like to use VM_PFNMAP on shared mappings, where
>>> +	 * (vma->vm_flags & VM_SHARED) != 0, for performance reasons,
>>> +	 * but for some reason VM_PFNMAP + x86 PAT + write-combine is very
>>> +	 * bad for performance. Until that has been sorted out, use
>>> +	 * VM_MIXEDMAP on all mappings. See freedesktop.org bug #75719
>>> +	 */
>>> +	vma->vm_flags |= VM_MIXEDMAP;
>>> +	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
>>> +}
>>> +EXPORT_SYMBOL(ttm_bo_mmap_vma_setup);
>>
>> To me, this function looks like an internal helper that should rather
>> remain internal.
> 
> Well, I'm moving that to a helper exactly to avoid drm gem ttm helpers
> messing with ttm internals.  To not them initialize vm_flags for
> example, and to avoid exporting ttm_bo_vm_ops.  Also to make sure ttm bo
> vma's are initialized the same way no matter which code path was taken
> to mmap the object.

It may not be worth blocking on this, so

  Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

But I still think it's not a good interface because it exposes internal
details.

Please consider another idea: how about splitting off the ttm_bo_get()
and vma-flags setup of ttm_fbdev_mmap() into a separate function, like this:

void ttm_bo_mmap_refed(vma, bo)
{
	ttm_bo_get(bo)
	ttm_bo_mmap_vma_setup(vma);
}
EXPORT_SYMBOL(ttm_bo_mmap_refed)

int ttm_fbdev_mmap(vma, bo)
{
        if (vma->vm_pgoff != 0)
                return -EACCES;
	ttm_bo_mmap_refed(vma, bo);
	return 0;
}

That would allow to keep _vma_setup() an internal function.

ttm_fbdev_mmap() sounds like it is only for fbdev and the only user is
amdgpu. Can it be moved out of ttm entirely?

Best regards
Thomas

>> As mentioned in my reply to patch 5, maybe re-using
>> ttm_fbdev_mmap() could help.
> 
> No, the check in that function prevents that from working.
> 
> cheers,
>   Gerd
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


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

Re: [PATCH 4/8] drm/ttm: factor out ttm_bo_mmap_vma_setup

[Gerd Hoffmann]

> It may not be worth blocking on this, so
> 
>   Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> But I still think it's not a good interface because it exposes internal
> details.
> 
> Please consider another idea: how about splitting off the ttm_bo_get()
> and vma-flags setup of ttm_fbdev_mmap() into a separate function, like this:
> 
> void ttm_bo_mmap_refed(vma, bo)
> {
> 	ttm_bo_get(bo)
> 	ttm_bo_mmap_vma_setup(vma);
> }
> EXPORT_SYMBOL(ttm_bo_mmap_refed)

ttm_bo_mmap_refed and ttm_bo_mmap_vma_setup are almost identical ...

But, yes, moving the ttm_bo_get call to ttm_bo_mmap_vma_setup probably
makes sense and hides this little detail to the outside.

> ttm_fbdev_mmap() sounds like it is only for fbdev and the only user is
> amdgpu. Can it be moved out of ttm entirely?

Exporting ttm_bo_mmap_vma_setup() allows to do that ;)

cheers,
  Gerd

Re: [PATCH 1/8] drm: add mmap() to drm_gem_object_funcs

[Daniel Vetter]

On Fri, Sep 13, 2019 at 02:29:01PM +0200, Gerd Hoffmann wrote:
> drm_gem_object_funcs->vm_ops alone can't handle everything which needs
> to be done for mmap(), tweaking vm_flags for example.  So add a new
> mmap() callback to drm_gem_object_funcs where this code can go to.
> 
> Note that the vm_ops field is not used in case the mmap callback is
> presnt, it is expected that the callback sets vma->vm_ops instead.
> 
> drm_gem_mmap_obj() will use the new callback for object specific mmap
> setup.  With this in place the need for driver-speific fops->mmap
> callbacks goes away, drm_gem_mmap can be hooked instead.
> 
> drm_gem_prime_mmap() will use the new callback too to just mmap gem
> objects directly instead of jumping though loops to make
> drm_gem_object_lookup() and fops->mmap work.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_gem.h       | 14 ++++++++++++++
>  drivers/gpu/drm/drm_gem.c   | 27 ++++++++++++++++++---------
>  drivers/gpu/drm/drm_prime.c |  9 +++++++++
>  3 files changed, 41 insertions(+), 9 deletions(-)
> 
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 6aaba14f5972..e71f75a2ab57 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -150,6 +150,20 @@ struct drm_gem_object_funcs {
>  	 */
>  	void (*vunmap)(struct drm_gem_object *obj, void *vaddr);
>  
> +	/**
> +	 * @mmap:
> +	 *
> +	 * Handle mmap() of the gem object, setup vma accordingly.
> +	 *
> +	 * This callback is optional.
> +	 *
> +	 * The callback is used by by both drm_gem_mmap_obj() and
> +	 * drm_gem_prime_mmap().  When @mmap is present @vm_ops is not
> +	 * used, the @mmap callback must set vma->vm_ops instead.
> +	 *
> +	 */
> +	int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma);
> +
>  	/**
>  	 * @vm_ops:
>  	 *
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 6854f5867d51..56f42e0f2584 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1099,22 +1099,31 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  		     struct vm_area_struct *vma)
>  {
>  	struct drm_device *dev = obj->dev;
> +	int ret;
>  
>  	/* Check for valid size. */
>  	if (obj_size < vma->vm_end - vma->vm_start)
>  		return -EINVAL;
>  
> -	if (obj->funcs && obj->funcs->vm_ops)
> -		vma->vm_ops = obj->funcs->vm_ops;
> -	else if (dev->driver->gem_vm_ops)
> -		vma->vm_ops = dev->driver->gem_vm_ops;
> -	else
> -		return -EINVAL;
> +	if (obj->funcs && obj->funcs->mmap) {
> +		ret = obj->funcs->mmap(obj, vma);
> +		if (ret)
> +			return ret;
> +		WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));

Yeah I think checking for VM_DONTEXPAND is even better than checking for
just VM_SPECIAL.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> +	} else {
> +		if (obj->funcs && obj->funcs->vm_ops)
> +			vma->vm_ops = obj->funcs->vm_ops;
> +		else if (dev->driver->gem_vm_ops)
> +			vma->vm_ops = dev->driver->gem_vm_ops;
> +		else
> +			return -EINVAL;
> +
> +		vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
> +		vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> +		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> +	}
>  
> -	vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
>  	vma->vm_private_data = obj;
> -	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> -	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
>  
>  	/* Take a ref for this mapping of the object, so that the fault
>  	 * handler can dereference the mmap offset's pointer to the object.
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 0a2316e0e812..0814211b0f3f 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -713,6 +713,15 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>  	struct file *fil;
>  	int ret;
>  
> +	if (obj->funcs && obj->funcs->mmap) {
> +		ret = obj->funcs->mmap(obj, vma);
> +		if (ret)
> +			return ret;
> +		vma->vm_private_data = obj;
> +		drm_gem_object_get(obj);
> +		return 0;
> +	}
> +
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>  	fil = kzalloc(sizeof(*fil), GFP_KERNEL);
>  	if (!priv || !fil) {
> -- 
> 2.18.1
> 

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