[PATCH v4 1/3] drm/prime: Pass the right module owner through to dma_buf_export()

[PATCH v4 1/3] drm/prime: Pass the right module owner through to dma_buf_export()

[Chris Wilson]

dma_buf_export() adds a reference to the owning module to the dmabuf (to
prevent the driver from being unloaded whilst a third party still refers
to the dmabuf). However, drm_gem_prime_export() was passing its own
THIS_MODULE (i.e. drm.ko) rather than the driver. Extract the right
owner from the device->fops instead.

v2: Use C99 initializers to zero out unset elements of
dma_buf_export_info
v3: Extract the right module from dev->fops.

Testcase: igt/vgem_basic/unload
Reported-by: Petri Latvala <petri.latvala@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Petri Latvala <petri.latvala@intel.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: stable@vger.kernel.org
Tested-by: Petri Latvala <petri.latvala@intel.com>
---
 drivers/gpu/drm/drm_prime.c | 17 ++++++++++-------
 include/drm/drmP.h          |  3 ++-
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 57201d68cf61..80907b34d857 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -397,14 +397,17 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
  * using the PRIME helpers.
  */
 struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
-				     struct drm_gem_object *obj, int flags)
+				     struct drm_gem_object *obj,
+				     int flags)
 {
-	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
-
-	exp_info.ops = &drm_gem_prime_dmabuf_ops;
-	exp_info.size = obj->size;
-	exp_info.flags = flags;
-	exp_info.priv = obj;
+	struct dma_buf_export_info exp_info = {
+		.exp_name = KBUILD_MODNAME, /* white lie for debug */
+		.owner = dev->driver->fops->owner,
+		.ops = &drm_gem_prime_dmabuf_ops,
+		.size = obj->size,
+		.flags = flags,
+		.priv = obj,
+	};
 
 	if (dev->driver->gem_prime_res_obj)
 		exp_info.resv = dev->driver->gem_prime_res_obj(obj);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 0e99669159c1..81fcd553edf7 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1012,7 +1012,8 @@ static inline int drm_debugfs_remove_files(const struct drm_info_list *files,
 #endif
 
 extern struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
-		struct drm_gem_object *obj, int flags);
+					    struct drm_gem_object *obj,
+					    int flags);
 extern int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 		struct drm_file *file_priv, uint32_t handle, uint32_t flags,
 		int *prime_fd);
-- 
2.9.3

[PATCH v4 2/3] drm/prime: Take a ref on the drm_dev when exporting a dma_buf

[Chris Wilson]

dma_buf may live a long time, longer than the last direct user of the
driver. We already hold a reference to the owner module (that prevents
the object code from disappearing), but there is no reference to the
drm_dev - so the pointers to the driver backend themselves may vanish.

v2: Resist temptation to fix the bug in armada_gem.c not setting the
correct flags on the exported dma-buf (it should pass the flags through
and not be arbitrarily setting O_RDWR).

Use a common wrapper for exporting the dmabuf and acquiring the
reference to the drm_device.

Testcase: igt/vgem_basic/unload
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Petri Latvala <petri.latvala@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
Tested-by: Petri Latvala <petri.latvala@intel.com>
---
 drivers/gpu/drm/armada/armada_gem.c    |  2 +-
 drivers/gpu/drm/drm_prime.c            | 30 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_gem_dmabuf.c |  2 +-
 drivers/gpu/drm/tegra/gem.c            |  2 +-
 drivers/gpu/drm/udl/udl_dmabuf.c       |  2 +-
 include/drm/drmP.h                     |  4 ++++
 6 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
index cb8f0347b934..a5e428d27d2f 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -547,7 +547,7 @@ armada_gem_prime_export(struct drm_device *dev, struct drm_gem_object *obj,
 	exp_info.flags = O_RDWR;
 	exp_info.priv = obj;
 
-	return dma_buf_export(&exp_info);
+	return drm_gem_dmabuf_export(dev, &exp_info);
 }
 
 struct drm_gem_object *
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 80907b34d857..875df8d719fb 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -284,18 +284,46 @@ static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
 }
 
 /**
+ * drm_gem_dmabuf_export - dma_buf export implementation for GEM
+ * @dma_buf: buffer to be exported
+ *
+ * This wraps dma_buf_export() for use by generic GEM drivers that are using
+ * drm_gem_dmabuf_release(). In addition to calling dma_buf_export(), we take
+ * a reference to the drm_device which is released by drm_gem_dmabuf_release().
+ *
+ * Returns the new dmabuf.
+ */
+struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev,
+				      struct dma_buf_export_info *exp_info)
+{
+	struct dma_buf *dma_buf;
+
+	dma_buf = dma_buf_export(exp_info);
+	if (!IS_ERR(dma_buf))
+		drm_dev_ref(dev);
+
+	return dma_buf;
+}
+EXPORT_SYMBOL(drm_gem_dmabuf_export);
+
+/**
  * drm_gem_dmabuf_release - dma_buf release implementation for GEM
  * @dma_buf: buffer to be released
  *
  * Generic release function for dma_bufs exported as PRIME buffers. GEM drivers
  * must use this in their dma_buf ops structure as the release callback.
+ * drm_gem_dmabuf_release() should be used in conjunction with
+ * drm_gem_dmabuf_export().
  */
 void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
 {
 	struct drm_gem_object *obj = dma_buf->priv;
+	struct drm_device *dev = obj->dev;
 
 	/* drop the reference on the export fd holds */
 	drm_gem_object_unreference_unlocked(obj);
+
+	drm_dev_unref(dev);
 }
 EXPORT_SYMBOL(drm_gem_dmabuf_release);
 
@@ -412,7 +440,7 @@ struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
 	if (dev->driver->gem_prime_res_obj)
 		exp_info.resv = dev->driver->gem_prime_res_obj(obj);
 
-	return dma_buf_export(&exp_info);
+	return drm_gem_dmabuf_export(dev, &exp_info);
 }
 EXPORT_SYMBOL(drm_gem_prime_export);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 10265bb35604..97c9d68b45df 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -283,7 +283,7 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
 			return ERR_PTR(ret);
 	}
 
-	dma_buf = dma_buf_export(&exp_info);
+	dma_buf = drm_gem_dmabuf_export(dev, &exp_info);
 	if (IS_ERR(dma_buf))
 		return dma_buf;
 
diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
index aa60d9909ea2..95e622e31931 100644
--- a/drivers/gpu/drm/tegra/gem.c
+++ b/drivers/gpu/drm/tegra/gem.c
@@ -613,7 +613,7 @@ struct dma_buf *tegra_gem_prime_export(struct drm_device *drm,
 	exp_info.flags = flags;
 	exp_info.priv = gem;
 
-	return dma_buf_export(&exp_info);
+	return drm_gem_dmabuf_export(drm, &exp_info);
 }
 
 struct drm_gem_object *tegra_gem_prime_import(struct drm_device *drm,
diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c
index e2243edd1ce3..ac90ffdb5912 100644
--- a/drivers/gpu/drm/udl/udl_dmabuf.c
+++ b/drivers/gpu/drm/udl/udl_dmabuf.c
@@ -209,7 +209,7 @@ struct dma_buf *udl_gem_prime_export(struct drm_device *dev,
 	exp_info.flags = flags;
 	exp_info.priv = obj;
 
-	return dma_buf_export(&exp_info);
+	return drm_gem_dmabuf_export(dev, &exp_info);
 }
 
 static int udl_prime_create(struct drm_device *dev,
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 81fcd553edf7..672644031bd5 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1011,6 +1011,8 @@ static inline int drm_debugfs_remove_files(const struct drm_info_list *files,
 }
 #endif
 
+struct dma_buf_export_info;
+
 extern struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
 					    struct drm_gem_object *obj,
 					    int flags);
@@ -1021,6 +1023,8 @@ extern struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
 		struct dma_buf *dma_buf);
 extern int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 		struct drm_file *file_priv, int prime_fd, uint32_t *handle);
+struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev,
+				      struct dma_buf_export_info *exp_info);
 extern void drm_gem_dmabuf_release(struct dma_buf *dma_buf);
 
 extern int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
-- 
2.9.3

[PATCH v4 3/3] drm/vgem: Keep a reference to the dmabuf when mmaped

[Chris Wilson]

In order to keep the dmabuf alive whilst the mmap is, we need to hold a
reference to the dmabuf and not the backing object. This is important as
the dmabuf not only keeps the object alive, but also the device so that

	dmabuf = vgem_create_dmabuf();
	ptr = mmap(... dmabuf ...);
	close(dmabuf);

persists across module-unload as well as device closure.

Testcase: igt/vgem_basic/unload
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Petri Latvala <petri.latvala@intel.com>
---
 drivers/gpu/drm/vgem/vgem_drv.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index f36c14729b55..74a83e41efa9 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -198,7 +198,6 @@ static struct drm_ioctl_desc vgem_ioctls[] = {
 
 static int vgem_mmap(struct file *filp, struct vm_area_struct *vma)
 {
-	unsigned long flags = vma->vm_flags;
 	int ret;
 
 	ret = drm_gem_mmap(filp, vma);
@@ -208,7 +207,7 @@ static int vgem_mmap(struct file *filp, struct vm_area_struct *vma)
 	/* Keep the WC mmaping set by drm_gem_mmap() but our pages
 	 * are ordinary and not special.
 	 */
-	vma->vm_flags = flags | VM_DONTEXPAND | VM_DONTDUMP;
+	vma->vm_flags &= ~(VM_IO | VM_PFNMAP);
 	return 0;
 }
 
@@ -281,21 +280,11 @@ static int vgem_prime_mmap(struct drm_gem_object *obj,
 {
 	int ret;
 
-	if (obj->size < vma->vm_end - vma->vm_start)
-		return -EINVAL;
-
-	if (!obj->filp)
-		return -ENODEV;
-
-	ret = obj->filp->f_op->mmap(obj->filp, vma);
+	ret = drm_gem_mmap_obj(obj, obj->size, vma);
 	if (ret)
 		return ret;
 
-	fput(vma->vm_file);
-	vma->vm_file = get_file(obj->filp);
-	vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
-	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
-
+	vma->vm_flags &= ~(VM_IO | VM_PFNMAP);
 	return 0;
 }
 
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/3] drm/prime: Pass the right module owner through to dma_buf_export()

[Petri Latvala]

For the series:

Reviewed-by: Petri Latvala <petri.latvala@intel.com>

On 10/05/2016 03:21 PM, Chris Wilson wrote:
> dma_buf_export() adds a reference to the owning module to the dmabuf (to
> prevent the driver from being unloaded whilst a third party still refers
> to the dmabuf). However, drm_gem_prime_export() was passing its own
> THIS_MODULE (i.e. drm.ko) rather than the driver. Extract the right
> owner from the device->fops instead.
>
> v2: Use C99 initializers to zero out unset elements of
> dma_buf_export_info
> v3: Extract the right module from dev->fops.
>
> Testcase: igt/vgem_basic/unload
> Reported-by: Petri Latvala <petri.latvala@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Petri Latvala <petri.latvala@intel.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: stable@vger.kernel.org
> Tested-by: Petri Latvala <petri.latvala@intel.com>
> ---
>   drivers/gpu/drm/drm_prime.c | 17 ++++++++++-------
>   include/drm/drmP.h          |  3 ++-
>   2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 57201d68cf61..80907b34d857 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -397,14 +397,17 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
>    * using the PRIME helpers.
>    */
>   struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
> -				     struct drm_gem_object *obj, int flags)
> +				     struct drm_gem_object *obj,
> +				     int flags)
>   {
> -	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> -
> -	exp_info.ops = &drm_gem_prime_dmabuf_ops;
> -	exp_info.size = obj->size;
> -	exp_info.flags = flags;
> -	exp_info.priv = obj;
> +	struct dma_buf_export_info exp_info = {
> +		.exp_name = KBUILD_MODNAME, /* white lie for debug */
> +		.owner = dev->driver->fops->owner,
> +		.ops = &drm_gem_prime_dmabuf_ops,
> +		.size = obj->size,
> +		.flags = flags,
> +		.priv = obj,
> +	};
>   
>   	if (dev->driver->gem_prime_res_obj)
>   		exp_info.resv = dev->driver->gem_prime_res_obj(obj);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 0e99669159c1..81fcd553edf7 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1012,7 +1012,8 @@ static inline int drm_debugfs_remove_files(const struct drm_info_list *files,
>   #endif
>   
>   extern struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
> -		struct drm_gem_object *obj, int flags);
> +					    struct drm_gem_object *obj,
> +					    int flags);
>   extern int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>   		struct drm_file *file_priv, uint32_t handle, uint32_t flags,
>   		int *prime_fd);

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx