[PATCHv3 0/3] dma_buf import support for vgem

[PATCHv3 0/3] dma_buf import support for vgem

[Laura Abbott]

Hi,

This is v3 of the series to add dma_buf import functions for vgem.
This is mostly a rebase to drm-misc/drm-misc-next with a fixup of
the resulting conflicts. More details can be found on the individual
patches.

Thanks,
Laura

Laura Abbott (3):
  drm/vgem: Add a dummy platform device
  drm/prime: Introduce drm_gem_prime_import_platform
  drm/vgem: Enable dmabuf import interfaces

 drivers/gpu/drm/drm_prime.c     |  49 ++++++++++---
 drivers/gpu/drm/vgem/vgem_drv.c | 150 +++++++++++++++++++++++++++++++---------
 drivers/gpu/drm/vgem/vgem_drv.h |   2 +
 include/drm/drmP.h              |   2 +
 include/drm/drm_prime.h         |   4 ++
 5 files changed, 164 insertions(+), 43 deletions(-)

-- 
2.7.4

[PATCHv3 1/3] drm/vgem: Add a dummy platform device

[Laura Abbott]

The vgem driver is currently registered independent of any actual
device. Some usage of the dmabuf APIs require an actual device structure
to do anything. Register a dummy platform device for use with dmabuf.

Cc: intel-gfx@lists.freedesktop.org
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: No changes
---
 drivers/gpu/drm/vgem/vgem_drv.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 9fee38a..727eed2 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -335,11 +335,20 @@ static int __init vgem_init(void)
 	int ret;
 
 	vgem_device = drm_dev_alloc(&vgem_driver, NULL);
-	if (IS_ERR(vgem_device)) {
-		ret = PTR_ERR(vgem_device);
+	if (IS_ERR(vgem_device))
+		return PTR_ERR(vgem_device);
+
+	vgem_device->platformdev = platform_device_register_simple("vgem",
+					-1, NULL, 0);
+
+	if (!vgem_device->platformdev) {
+		ret = -ENODEV;
 		goto out;
 	}
 
+	dma_coerce_mask_and_coherent(&vgem_device->platformdev->dev,
+					DMA_BIT_MASK(64));
+
 	ret  = drm_dev_register(vgem_device, 0);
 	if (ret)
 		goto out_unref;
@@ -347,13 +356,15 @@ static int __init vgem_init(void)
 	return 0;
 
 out_unref:
-	drm_dev_unref(vgem_device);
+	platform_device_unregister(vgem_device->platformdev);
 out:
+	drm_dev_unref(vgem_device);
 	return ret;
 }
 
 static void __exit vgem_exit(void)
 {
+	platform_device_unregister(vgem_device->platformdev);
 	drm_dev_unregister(vgem_device);
 	drm_dev_unref(vgem_device);
 }
-- 
2.7.4

[PATCHv3 2/3] drm/prime: Introduce drm_gem_prime_import_platform

[Laura Abbott]

The existing drm_gem_prime_import function uses the underlying
struct device of a drm_device for attaching to a dma_buf. Some drivers
(notably vgem) may not have an underlying device structure. Offer
an alternate function to attach using a platform device associated
with drm_device.

Cc: intel-gfx@lists.freedesktop.org
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: Rebase to drm-misc-next. Prototype moved to a new header file. Comments
added for new function. Brought back drm_device->platformdev as it had been
removed in 76adb460fd93 ("drm: Remove the struct drm_device platformdev field").
I'm not entirely thrilled about this since the platformdev removal was good
cleanup and this feels like a small step backwards. I don't know of a better
approach though.
---
 drivers/gpu/drm/drm_prime.c | 49 +++++++++++++++++++++++++++++++++++----------
 include/drm/drmP.h          |  2 ++
 include/drm/drm_prime.h     |  4 ++++
 3 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 9fb65b7..a557a4b 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -594,16 +594,9 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
 
-/**
- * drm_gem_prime_import - helper library implementation of the import callback
- * @dev: drm_device to import into
- * @dma_buf: dma-buf object to import
- *
- * This is the implementation of the gem_prime_import functions for GEM drivers
- * using the PRIME helpers.
- */
-struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
-					    struct dma_buf *dma_buf)
+static struct drm_gem_object *__drm_gem_prime_import(struct drm_device *dev,
+					    struct dma_buf *dma_buf,
+					    struct device *attach_dev)
 {
 	struct dma_buf_attachment *attach;
 	struct sg_table *sgt;
@@ -625,7 +618,7 @@ struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
 	if (!dev->driver->gem_prime_import_sg_table)
 		return ERR_PTR(-EINVAL);
 
-	attach = dma_buf_attach(dma_buf, dev->dev);
+	attach = dma_buf_attach(dma_buf, attach_dev);
 	if (IS_ERR(attach))
 		return ERR_CAST(attach);
 
@@ -655,9 +648,43 @@ struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
 
 	return ERR_PTR(ret);
 }
+
+/**
+ * drm_gem_prime_import - helper library implementation of the import callback
+ * @dev: drm_device to import into
+ * @dma_buf: dma-buf object to import
+ *
+ * This is the implementation of the gem_prime_import functions for GEM drivers
+ * using the PRIME helpers.
+ */
+struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
+					    struct dma_buf *dma_buf)
+{
+	return __drm_gem_prime_import(dev, dma_buf, dev->dev);
+}
 EXPORT_SYMBOL(drm_gem_prime_import);
 
 /**
+ * drm_gem_prime_import_platform - alternate implementation of the import callback
+ * @dev: drm_device to import into
+ * @dma_buf: dma-buf object to import
+ *
+ * This is identical to drm_gem_prime_import except the device used for dma_buf
+ * attachment is an internal platform device instead of the standard device
+ * structure. The use of this function should be limited to drivers that do not
+ * set up an underlying device structure.
+ */
+struct drm_gem_object *drm_gem_prime_import_platform(struct drm_device *dev,
+					    struct dma_buf *dma_buf)
+{
+	if (WARN_ON_ONCE(!dev->platformdev))
+		return NULL;
+
+	return __drm_gem_prime_import(dev, dma_buf, &dev->platformdev->dev);
+}
+EXPORT_SYMBOL(drm_gem_prime_import_platform);
+
+/**
  * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers
  * @dev: dev to export the buffer from
  * @file_priv: drm file-private structure
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index e1daa4f..086daee 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -439,6 +439,8 @@ struct drm_device {
 	struct pci_controller *hose;
 #endif
 
+	/**< Platform device for drivers that do not use the standard device */
+	struct platform_device *platformdev;
 	struct virtio_device *virtdev;
 
 	struct drm_sg_mem *sg;	/**< Scatter gather memory */
diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
index 0b2a235..9fe39f8 100644
--- a/include/drm/drm_prime.h
+++ b/include/drm/drm_prime.h
@@ -65,6 +65,10 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 			       int *prime_fd);
 struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
 					    struct dma_buf *dma_buf);
+
+struct drm_gem_object *drm_gem_prime_import_platform(struct drm_device *dev,
+						     struct dma_buf *dma_buf);
+
 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,
-- 
2.7.4

[PATCHv3 3/3] drm/vgem: Enable dmabuf import interfaces

[Laura Abbott]

Enable the GEM dma-buf import interfaces in addition to the export
interfaces. This lets vgem be used as a test source for other allocators
(e.g. Ion).

Cc: intel-gfx@lists.freedesktop.org
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: Minor fixes suggested by Chris Wilson
---
 drivers/gpu/drm/vgem/vgem_drv.c | 133 +++++++++++++++++++++++++++++++---------
 drivers/gpu/drm/vgem/vgem_drv.h |   2 +
 2 files changed, 106 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 727eed2..c254c80 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -34,6 +34,9 @@
 #include <linux/ramfs.h>
 #include <linux/shmem_fs.h>
 #include <linux/dma-buf.h>
+
+#include <drm/drmP.h>
+
 #include "vgem_drv.h"
 
 #define DRIVER_NAME	"vgem"
@@ -46,6 +49,11 @@ static void vgem_gem_free_object(struct drm_gem_object *obj)
 {
 	struct drm_vgem_gem_object *vgem_obj = to_vgem_bo(obj);
 
+	drm_free_large(vgem_obj->pages);
+
+	if (obj->import_attach)
+		drm_prime_gem_destroy(obj, vgem_obj->table);
+
 	drm_gem_object_release(obj);
 	kfree(vgem_obj);
 }
@@ -56,26 +64,49 @@ static int vgem_gem_fault(struct vm_fault *vmf)
 	struct drm_vgem_gem_object *obj = vma->vm_private_data;
 	/* We don't use vmf->pgoff since that has the fake offset */
 	unsigned long vaddr = vmf->address;
-	struct page *page;
-
-	page = shmem_read_mapping_page(file_inode(obj->base.filp)->i_mapping,
-				       (vaddr - vma->vm_start) >> PAGE_SHIFT);
-	if (!IS_ERR(page)) {
-		vmf->page = page;
-		return 0;
-	} else switch (PTR_ERR(page)) {
-		case -ENOSPC:
-		case -ENOMEM:
-			return VM_FAULT_OOM;
-		case -EBUSY:
-			return VM_FAULT_RETRY;
-		case -EFAULT:
-		case -EINVAL:
-			return VM_FAULT_SIGBUS;
-		default:
-			WARN_ON_ONCE(PTR_ERR(page));
-			return VM_FAULT_SIGBUS;
+	int ret;
+	loff_t num_pages;
+	pgoff_t page_offset;
+	page_offset = (vaddr - vma->vm_start) >> PAGE_SHIFT;
+
+	num_pages = DIV_ROUND_UP(obj->base.size, PAGE_SIZE);
+
+	if (page_offset > num_pages)
+		return VM_FAULT_SIGBUS;
+
+	if (obj->pages) {
+		get_page(obj->pages[page_offset]);
+		vmf->page = obj->pages[page_offset];
+		ret = 0;
+	} else {
+		struct page *page;
+
+		page = shmem_read_mapping_page(
+					file_inode(obj->base.filp)->i_mapping,
+					page_offset);
+		if (!IS_ERR(page)) {
+			vmf->page = page;
+			ret = 0;
+		} else switch (PTR_ERR(page)) {
+			case -ENOSPC:
+			case -ENOMEM:
+				ret = VM_FAULT_OOM;
+				break;
+			case -EBUSY:
+				ret = VM_FAULT_RETRY;
+				break;
+			case -EFAULT:
+			case -EINVAL:
+				ret = VM_FAULT_SIGBUS;
+				break;
+			default:
+				WARN_ON(PTR_ERR(page));
+				ret = VM_FAULT_SIGBUS;
+				break;
+		}
+
 	}
+	return ret;
 }
 
 static const struct vm_operations_struct vgem_gem_vm_ops = {
@@ -112,12 +143,8 @@ static void vgem_postclose(struct drm_device *dev, struct drm_file *file)
 	kfree(vfile);
 }
 
-/* ioctls */
-
-static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
-					      struct drm_file *file,
-					      unsigned int *handle,
-					      unsigned long size)
+static struct drm_vgem_gem_object *__vgem_gem_create(struct drm_device *dev,
+						unsigned long size)
 {
 	struct drm_vgem_gem_object *obj;
 	int ret;
@@ -127,8 +154,31 @@ static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
 		return ERR_PTR(-ENOMEM);
 
 	ret = drm_gem_object_init(dev, &obj->base, roundup(size, PAGE_SIZE));
-	if (ret)
-		goto err_free;
+	if (ret) {
+		kfree(obj);
+		return ERR_PTR(ret);
+	}
+
+	return obj;
+}
+
+static void __vgem_gem_destroy(struct drm_vgem_gem_object *obj)
+{
+	drm_gem_object_release(&obj->base);
+	kfree(obj);
+}
+
+static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
+					      struct drm_file *file,
+					      unsigned int *handle,
+					      unsigned long size)
+{
+	struct drm_vgem_gem_object *obj;
+	int ret;
+
+	obj = __vgem_gem_create(dev, size);
+	if (IS_ERR(obj))
+		return ERR_CAST(obj);
 
 	ret = drm_gem_handle_create(file, &obj->base, handle);
 	drm_gem_object_unreference_unlocked(&obj->base);
@@ -137,9 +187,8 @@ static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
 
 	return &obj->base;
 
-err_free:
-	kfree(obj);
 err:
+	__vgem_gem_destroy(obj);
 	return ERR_PTR(ret);
 }
 
@@ -256,6 +305,29 @@ static struct sg_table *vgem_prime_get_sg_table(struct drm_gem_object *obj)
 	return st;
 }
 
+static struct drm_gem_object *vgem_prime_import_sg_table(struct drm_device *dev,
+			struct dma_buf_attachment *attach, struct sg_table *sg)
+{
+	struct drm_vgem_gem_object *obj;
+	int npages;
+
+	obj = __vgem_gem_create(dev, attach->dmabuf->size);
+	if (IS_ERR(obj))
+		return ERR_CAST(obj);
+
+	npages = PAGE_ALIGN(attach->dmabuf->size) / PAGE_SIZE;
+
+	obj->table = sg;
+	obj->pages = drm_malloc_ab(npages, sizeof(struct page *));
+	if (!obj->pages) {
+		__vgem_gem_destroy(obj);
+		return ERR_PTR(-ENOMEM);
+	}
+	drm_prime_sg_to_page_addr_arrays(obj->table, obj->pages, NULL,
+					npages);
+	return &obj->base;
+}
+
 static void *vgem_prime_vmap(struct drm_gem_object *obj)
 {
 	long n_pages = obj->size >> PAGE_SHIFT;
@@ -314,8 +386,11 @@ static struct drm_driver vgem_driver = {
 	.dumb_map_offset		= vgem_gem_dumb_map,
 
 	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
+	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
 	.gem_prime_pin = vgem_prime_pin,
+	.gem_prime_import = drm_gem_prime_import_platform,
 	.gem_prime_export = drm_gem_prime_export,
+	.gem_prime_import_sg_table = vgem_prime_import_sg_table,
 	.gem_prime_get_sg_table = vgem_prime_get_sg_table,
 	.gem_prime_vmap = vgem_prime_vmap,
 	.gem_prime_vunmap = vgem_prime_vunmap,
diff --git a/drivers/gpu/drm/vgem/vgem_drv.h b/drivers/gpu/drm/vgem/vgem_drv.h
index cb59c7a..1aae014 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.h
+++ b/drivers/gpu/drm/vgem/vgem_drv.h
@@ -43,6 +43,8 @@ struct vgem_file {
 #define to_vgem_bo(x) container_of(x, struct drm_vgem_gem_object, base)
 struct drm_vgem_gem_object {
 	struct drm_gem_object base;
+	struct page **pages;
+	struct sg_table *table;
 };
 
 int vgem_fence_open(struct vgem_file *file);
-- 
2.7.4

Re: [PATCHv3 2/3] drm/prime: Introduce drm_gem_prime_import_platform

[Chris Wilson]

On Tue, May 02, 2017 at 10:02:07AM -0700, Laura Abbott wrote:
> The existing drm_gem_prime_import function uses the underlying
> struct device of a drm_device for attaching to a dma_buf. Some drivers
> (notably vgem) may not have an underlying device structure. Offer
> an alternate function to attach using a platform device associated
> with drm_device.
> 
> Cc: intel-gfx@lists.freedesktop.org
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v3: Rebase to drm-misc-next. Prototype moved to a new header file. Comments
> added for new function. Brought back drm_device->platformdev as it had been
> removed in 76adb460fd93 ("drm: Remove the struct drm_device platformdev field").
> I'm not entirely thrilled about this since the platformdev removal was good
> cleanup and this feels like a small step backwards. I don't know of a better
> approach though.
> ---
>  drivers/gpu/drm/drm_prime.c | 49 +++++++++++++++++++++++++++++++++++----------
>  include/drm/drmP.h          |  2 ++
>  include/drm/drm_prime.h     |  4 ++++
>  3 files changed, 44 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 9fb65b7..a557a4b 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -594,16 +594,9 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
>  
> -/**
> - * drm_gem_prime_import - helper library implementation of the import callback
> - * @dev: drm_device to import into
> - * @dma_buf: dma-buf object to import
> - *
> - * This is the implementation of the gem_prime_import functions for GEM drivers
> - * using the PRIME helpers.
> - */
> -struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
> -					    struct dma_buf *dma_buf)
> +static struct drm_gem_object *__drm_gem_prime_import(struct drm_device *dev,
> +					    struct dma_buf *dma_buf,
> +					    struct device *attach_dev)
>  {
>  	struct dma_buf_attachment *attach;
>  	struct sg_table *sgt;
> @@ -625,7 +618,7 @@ struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
>  	if (!dev->driver->gem_prime_import_sg_table)
>  		return ERR_PTR(-EINVAL);
>  
> -	attach = dma_buf_attach(dma_buf, dev->dev);
> +	attach = dma_buf_attach(dma_buf, attach_dev);
>  	if (IS_ERR(attach))
>  		return ERR_CAST(attach);
>  
> @@ -655,9 +648,43 @@ struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
>  
>  	return ERR_PTR(ret);
>  }
> +
> +/**
> + * drm_gem_prime_import - helper library implementation of the import callback
> + * @dev: drm_device to import into
> + * @dma_buf: dma-buf object to import
> + *
> + * This is the implementation of the gem_prime_import functions for GEM drivers
> + * using the PRIME helpers.
> + */
> +struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
> +					    struct dma_buf *dma_buf)
> +{
> +	return __drm_gem_prime_import(dev, dma_buf, dev->dev);
> +}
>  EXPORT_SYMBOL(drm_gem_prime_import);
>  
>  /**
> + * drm_gem_prime_import_platform - alternate implementation of the import callback
> + * @dev: drm_device to import into
> + * @dma_buf: dma-buf object to import
> + *
> + * This is identical to drm_gem_prime_import except the device used for dma_buf
> + * attachment is an internal platform device instead of the standard device
> + * structure. The use of this function should be limited to drivers that do not
> + * set up an underlying device structure.
> + */
> +struct drm_gem_object *drm_gem_prime_import_platform(struct drm_device *dev,

Simpler soluation will be for the caller to provide the platformdev?

That works nicely for the vgem case, I think.

> +					    struct dma_buf *dma_buf)
> +{
> +	if (WARN_ON_ONCE(!dev->platformdev))
> +		return NULL;
> +
> +	return __drm_gem_prime_import(dev, dma_buf, &dev->platformdev->dev);
> +}
> +EXPORT_SYMBOL(drm_gem_prime_import_platform);

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCHv3 2/3] drm/prime: Introduce drm_gem_prime_import_platform

[Daniel Vetter]

On Tue, May 02, 2017 at 09:22:13PM +0100, Chris Wilson wrote:
> On Tue, May 02, 2017 at 10:02:07AM -0700, Laura Abbott wrote:
> >  /**
> > + * drm_gem_prime_import_platform - alternate implementation of the import callback
> > + * @dev: drm_device to import into
> > + * @dma_buf: dma-buf object to import
> > + *
> > + * This is identical to drm_gem_prime_import except the device used for dma_buf
> > + * attachment is an internal platform device instead of the standard device
> > + * structure. The use of this function should be limited to drivers that do not
> > + * set up an underlying device structure.
> > + */
> > +struct drm_gem_object *drm_gem_prime_import_platform(struct drm_device *dev,
> 
> Simpler soluation will be for the caller to provide the platformdev?
> 
> That works nicely for the vgem case, I think.

Yeah looking at this again, do we really need this patch? Couldn't we
instead change patch 1 to first allocate the fake platform device, then
pass that to drm_dev_alloc (instead of NULL like we do now)?

That way no resurrection of drm_device.platform_dev is needed (and I'd
really like this zombie to stay dead on 2nd thought).

Sry about this yet-another-round review :-/
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCHv3 2/3] drm/prime: Introduce drm_gem_prime_import_platform

[Laura Abbott]

On 05/03/2017 12:39 AM, Daniel Vetter wrote:
> On Tue, May 02, 2017 at 09:22:13PM +0100, Chris Wilson wrote:
>> On Tue, May 02, 2017 at 10:02:07AM -0700, Laura Abbott wrote:
>>>  /**
>>> + * drm_gem_prime_import_platform - alternate implementation of the import callback
>>> + * @dev: drm_device to import into
>>> + * @dma_buf: dma-buf object to import
>>> + *
>>> + * This is identical to drm_gem_prime_import except the device used for dma_buf
>>> + * attachment is an internal platform device instead of the standard device
>>> + * structure. The use of this function should be limited to drivers that do not
>>> + * set up an underlying device structure.
>>> + */
>>> +struct drm_gem_object *drm_gem_prime_import_platform(struct drm_device *dev,
>>
>> Simpler soluation will be for the caller to provide the platformdev?
>>
>> That works nicely for the vgem case, I think.
> 
> Yeah looking at this again, do we really need this patch? Couldn't we
> instead change patch 1 to first allocate the fake platform device, then
> pass that to drm_dev_alloc (instead of NULL like we do now)?
> 

That was what I proposed in the first version and it was rejected.
It's useful to have at least one driver with a NULL device for testing
edge cases.

> That way no resurrection of drm_device.platform_dev is needed (and I'd
> really like this zombie to stay dead on 2nd thought).
> 

I had a hunch this would be unpopular but I figured it was worth a
shot. I think an even cleaner solution is to allow passing of any
struct device. I'll see about reworking this.

> Sry about this yet-another-round review :-/
> -Daniel
> 

Thanks for your patience.

Laura

Re: [PATCHv3 2/3] drm/prime: Introduce drm_gem_prime_import_platform

[Daniel Vetter]

On Wed, May 03, 2017 at 07:40:51AM -0700, Laura Abbott wrote:
> On 05/03/2017 12:39 AM, Daniel Vetter wrote:
> > On Tue, May 02, 2017 at 09:22:13PM +0100, Chris Wilson wrote:
> >> On Tue, May 02, 2017 at 10:02:07AM -0700, Laura Abbott wrote:
> >>>  /**
> >>> + * drm_gem_prime_import_platform - alternate implementation of the import callback
> >>> + * @dev: drm_device to import into
> >>> + * @dma_buf: dma-buf object to import
> >>> + *
> >>> + * This is identical to drm_gem_prime_import except the device used for dma_buf
> >>> + * attachment is an internal platform device instead of the standard device
> >>> + * structure. The use of this function should be limited to drivers that do not
> >>> + * set up an underlying device structure.
> >>> + */
> >>> +struct drm_gem_object *drm_gem_prime_import_platform(struct drm_device *dev,
> >>
> >> Simpler soluation will be for the caller to provide the platformdev?
> >>
> >> That works nicely for the vgem case, I think.
> > 
> > Yeah looking at this again, do we really need this patch? Couldn't we
> > instead change patch 1 to first allocate the fake platform device, then
> > pass that to drm_dev_alloc (instead of NULL like we do now)?
> > 
> 
> That was what I proposed in the first version and it was rejected.
> It's useful to have at least one driver with a NULL device for testing
> edge cases.

Oh drat :( I'd say dropping the coverage for NULL testing is ok, there's
no other driver than vgem using this. And now that we have vgem dma-buf
(or will, soonish) I'd expect that the same will hold for vkms, if it ever
happens.
-Daniel

> > That way no resurrection of drm_device.platform_dev is needed (and I'd
> > really like this zombie to stay dead on 2nd thought).
> > 
> 
> I had a hunch this would be unpopular but I figured it was worth a
> shot. I think an even cleaner solution is to allow passing of any
> struct device. I'll see about reworking this.
> 
> > Sry about this yet-another-round review :-/
> > -Daniel
> > 
> 
> Thanks for your patience.
> 
> Laura
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCHv3 2/3] drm/prime: Introduce drm_gem_prime_import_platform

[Chris Wilson]

On Wed, May 03, 2017 at 05:07:03PM +0200, Daniel Vetter wrote:
> On Wed, May 03, 2017 at 07:40:51AM -0700, Laura Abbott wrote:
> > On 05/03/2017 12:39 AM, Daniel Vetter wrote:
> > > On Tue, May 02, 2017 at 09:22:13PM +0100, Chris Wilson wrote:
> > >> On Tue, May 02, 2017 at 10:02:07AM -0700, Laura Abbott wrote:
> > >>>  /**
> > >>> + * drm_gem_prime_import_platform - alternate implementation of the import callback
> > >>> + * @dev: drm_device to import into
> > >>> + * @dma_buf: dma-buf object to import
> > >>> + *
> > >>> + * This is identical to drm_gem_prime_import except the device used for dma_buf
> > >>> + * attachment is an internal platform device instead of the standard device
> > >>> + * structure. The use of this function should be limited to drivers that do not
> > >>> + * set up an underlying device structure.
> > >>> + */
> > >>> +struct drm_gem_object *drm_gem_prime_import_platform(struct drm_device *dev,
> > >>
> > >> Simpler soluation will be for the caller to provide the platformdev?
> > >>
> > >> That works nicely for the vgem case, I think.
> > > 
> > > Yeah looking at this again, do we really need this patch? Couldn't we
> > > instead change patch 1 to first allocate the fake platform device, then
> > > pass that to drm_dev_alloc (instead of NULL like we do now)?
> > > 
> > 
> > That was what I proposed in the first version and it was rejected.
> > It's useful to have at least one driver with a NULL device for testing
> > edge cases.
> 
> Oh drat :( I'd say dropping the coverage for NULL testing is ok, there's
> no other driver than vgem using this. And now that we have vgem dma-buf
> (or will, soonish) I'd expect that the same will hold for vkms, if it ever
> happens.

This series creates vgem->platformdev which we can just pass to
drm_gem_prime_import_platform() (or equivalent drm_gem_prime function that
takes an explicit dev). It was a bit of a surprise that import_platform
didn't take the platformdev after going to the trouble of creating
vgem->platformdev.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre