[RFC PATCH 0/2] dmabuf import interfaces for vgem

[RFC PATCH 0/2] dmabuf import interfaces for vgem

[Laura Abbott]

Hi,

Ion[1] is making progress towards moving out of staging. I'd like to take
advantage of the vgem driver for development of unit tests for Ion. To
do this properly, vgem needs to support importing of foreign handles. This
is an RFC to add that support. This passes the existing vgem_basic unit
test. I plan to extend that test with support for testing import as well.
RFC is for general direction and feedback since I'm not that familiar with
DRM internals.

Thanks,
Laura

[1] lkml.kernel.org/r/<1491245884-15852-1-git-send-email-labbott@redhat.com>

Laura Abbott (2):
  drm/vgem: Add a dummy platform device
  drm/vgem: Enable dmabuf import interfaces

 drivers/gpu/drm/vgem/vgem_drv.c | 147 ++++++++++++++++++++++++++++++++--------
 drivers/gpu/drm/vgem/vgem_drv.h |   3 +
 2 files changed, 122 insertions(+), 28 deletions(-)

-- 
2.7.4

[RFC PATCH 1/2] 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.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
I realize the original driver had a note about 'drop platform support'. I
strongly dislike platform devices but they are the easiest way to get a device
structure for use with APIs like dmabuf.
---
 drivers/gpu/drm/vgem/vgem_drv.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index a1f42d1..b94feef 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -329,12 +329,19 @@ static struct drm_driver vgem_driver = {
 };
 
 static struct drm_device *vgem_device;
+static struct platform_device *vgem_pdev;
 
 static int __init vgem_init(void)
 {
 	int ret;
 
-	vgem_device = drm_dev_alloc(&vgem_driver, NULL);
+	vgem_pdev = platform_device_register_simple("vgem", -1, NULL, 0);
+	if (IS_ERR(vgem_pdev))
+		return PTR_ERR(vgem_pdev);
+
+	dma_coerce_mask_and_coherent(&vgem_pdev->dev, DMA_BIT_MASK(64));
+
+	vgem_device = drm_dev_alloc(&vgem_driver, &vgem_pdev->dev);
 	if (IS_ERR(vgem_device)) {
 		ret = PTR_ERR(vgem_device);
 		goto out;
@@ -349,6 +356,8 @@ static int __init vgem_init(void)
 out_unref:
 	drm_dev_unref(vgem_device);
 out:
+	platform_device_unregister(vgem_pdev);
+
 	return ret;
 }
 
@@ -356,6 +365,7 @@ static void __exit vgem_exit(void)
 {
 	drm_dev_unregister(vgem_device);
 	drm_dev_unref(vgem_device);
+	platform_device_unregister(vgem_pdev);
 }
 
 module_init(vgem_init);
-- 
2.7.4

[RFC PATCH 2/2] 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).

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
 drivers/gpu/drm/vgem/vgem_drv.c | 135 ++++++++++++++++++++++++++++++++--------
 drivers/gpu/drm/vgem/vgem_drv.h |   3 +
 2 files changed, 111 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index b94feef..abc72e1 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"
@@ -42,40 +45,67 @@
 #define DRIVER_MAJOR	1
 #define DRIVER_MINOR	0
 
+#define VGEM_BO_IMPORT	BIT(0)
+
+void vgem_gem_put_pages(struct drm_vgem_gem_object *obj)
+{
+	drm_gem_put_pages(&obj->base, obj->pages, false, false);
+	obj->pages = NULL;
+}
+
 static void vgem_gem_free_object(struct drm_gem_object *obj)
 {
 	struct drm_vgem_gem_object *vgem_obj = to_vgem_bo(obj);
 
+	if (vgem_obj->pages) {
+		if (vgem_obj->flags & VGEM_BO_IMPORT)
+			drm_free_large(vgem_obj->pages);
+		else
+			vgem_gem_put_pages(vgem_obj);
+	}
+
+	if (obj->import_attach)
+		drm_prime_gem_destroy(obj, vgem_obj->table);
+
 	drm_gem_object_release(obj);
 	kfree(vgem_obj);
 }
 
+int vgem_gem_get_pages(struct drm_vgem_gem_object *obj)
+{
+	struct page **pages;
+
+	if (obj->pages)
+		return 0;
+
+	pages = drm_gem_get_pages(&obj->base);
+	if (IS_ERR(pages)) {
+		return PTR_ERR(pages);
+	}
+
+	obj->pages = pages;
+
+	return 0;
+}
+
 static int vgem_gem_fault(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
 	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;
+	loff_t num_pages;
+	pgoff_t page_offset;
+	page_offset = (vaddr - vma->vm_start) >> PAGE_SHIFT;
 
-	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;
-	}
+	num_pages = DIV_ROUND_UP(obj->base.size, PAGE_SIZE);
+
+	if (page_offset > num_pages)
+		return VM_FAULT_SIGBUS;
+
+	get_page(obj->pages[page_offset]);
+	vmf->page = obj->pages[page_offset];
+	return 0;
 }
 
 static const struct vm_operations_struct vgem_gem_vm_ops = {
@@ -112,7 +142,30 @@ static void vgem_preclose(struct drm_device *dev, struct drm_file *file)
 	kfree(vfile);
 }
 
-/* ioctls */
+static struct drm_vgem_gem_object *__vgem_gem_create(struct drm_device *dev,
+						unsigned long size)
+{
+	struct drm_vgem_gem_object *obj;
+	int ret;
+
+	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+	if (!obj)
+		return ERR_PTR(-ENOMEM);
+
+	ret = drm_gem_object_init(dev, &obj->base, roundup(size, PAGE_SIZE));
+	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,
@@ -122,24 +175,25 @@ static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
 	struct drm_vgem_gem_object *obj;
 	int ret;
 
-	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
-	if (!obj)
-		return ERR_PTR(-ENOMEM);
+	obj = __vgem_gem_create(dev, size);
+	if (IS_ERR(obj))
+		return ERR_CAST(obj);
 
-	ret = drm_gem_object_init(dev, &obj->base, roundup(size, PAGE_SIZE));
+	ret = vgem_gem_get_pages(obj);
 	if (ret)
 		goto err_free;
 
 	ret = drm_gem_handle_create(file, &obj->base, handle);
 	drm_gem_object_unreference_unlocked(&obj->base);
 	if (ret)
-		goto err;
+		goto err_pages;
 
 	return &obj->base;
 
+err_pages:
+	vgem_gem_put_pages(obj);
 err_free:
-	kfree(obj);
-err:
+	__vgem_gem_destroy(obj);
 	return ERR_PTR(ret);
 }
 
@@ -256,6 +310,30 @@ 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 *));
+	obj->flags = VGEM_BO_IMPORT;
+	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 +392,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,
 	.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..f5dd076 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.h
+++ b/drivers/gpu/drm/vgem/vgem_drv.h
@@ -43,6 +43,9 @@ 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;
+	unsigned long flags;
 };
 
 int vgem_fence_open(struct vgem_file *file);
-- 
2.7.4

Re: [RFC PATCH 2/2] drm/vgem: Enable dmabuf import interfaces

[Chris Wilson]

On Thu, Apr 06, 2017 at 04:18:33PM -0700, Laura Abbott wrote:
> 
> 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).
> 
> +int vgem_gem_get_pages(struct drm_vgem_gem_object *obj)
> +{
> +	struct page **pages;
> +
> +	if (obj->pages)
> +		return 0;
> +
> +	pages = drm_gem_get_pages(&obj->base);
> +	if (IS_ERR(pages)) {
> +		return PTR_ERR(pages);
> +	}
> +
> +	obj->pages = pages;

That's a significant loss in functionality (it requires the entire
object to fit into physical memory at the same time and requires a large
vmalloc for 32b systems), for what? vgem only has the ability to mmap
and export a fd -- both of which you already have if you have the dmabuf
fd. The only extra interface is the signaling, which does not yet have a
direct correspondence on dmabuf.

(An obvious way to keep both would be to move the get_pages to importing
and then differentiate in the faulthandler where to get the page from.)

Can you provide more details on how you are using vgem to justify the
changes? I'm also not particularly happy in losing testing of a virtual
platform device from our CI.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [RFC PATCH 2/2] drm/vgem: Enable dmabuf import interfaces

[Laura Abbott]

On 04/07/2017 12:39 AM, Chris Wilson wrote:
> On Thu, Apr 06, 2017 at 04:18:33PM -0700, Laura Abbott wrote:
>>
>> 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).
>>
>> +int vgem_gem_get_pages(struct drm_vgem_gem_object *obj)
>> +{
>> +	struct page **pages;
>> +
>> +	if (obj->pages)
>> +		return 0;
>> +
>> +	pages = drm_gem_get_pages(&obj->base);
>> +	if (IS_ERR(pages)) {
>> +		return PTR_ERR(pages);
>> +	}
>> +
>> +	obj->pages = pages;
> 
> That's a significant loss in functionality (it requires the entire
> object to fit into physical memory at the same time and requires a large
> vmalloc for 32b systems), for what? vgem only has the ability to mmap
> and export a fd -- both of which you already have if you have the dmabuf
> fd. The only extra interface is the signaling, which does not yet have a
> direct correspondence on dmabuf.
> 
> (An obvious way to keep both would be to move the get_pages to importing
> and then differentiate in the faulthandler where to get the page from.)
> 

Thanks for pointing this out. I'll look to keep the existing behavior.

> Can you provide more details on how you are using vgem to justify the
> changes? I'm also not particularly happy in losing testing of a virtual
> platform device from our CI.
> -Chris
> 

There exists a test module in the Ion directory
(drivers/staging/android/ion/ion_test.c) today but it's not actually
Ion specific. It registers a platform device and then provides an
ioctl interface to perform a dma_buf attach and map. I proposed
moving this to a generic dma-buf test module
(https://marc.info/?l=dri-devel&m=148952187004611&w=2) and Daniel
suggested that this was roughly what vgem was supposed to do.

Adding the import methods for vgem covers all of what the
Ion test was doing and we don't have to invent yet another ioctl
interface and framework for attaching and mapping. 

Can you clarify about what you mean about 'virtual platform device'?


Thanks,
Laura

Re: [RFC PATCH 2/2] drm/vgem: Enable dmabuf import interfaces

[Chris Wilson]

On Fri, Apr 07, 2017 at 09:18:30AM -0700, Laura Abbott wrote:
> On 04/07/2017 12:39 AM, Chris Wilson wrote:
> > On Thu, Apr 06, 2017 at 04:18:33PM -0700, Laura Abbott wrote:
> >>
> >> 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).
> >>
> >> +int vgem_gem_get_pages(struct drm_vgem_gem_object *obj)
> >> +{
> >> +	struct page **pages;
> >> +
> >> +	if (obj->pages)
> >> +		return 0;
> >> +
> >> +	pages = drm_gem_get_pages(&obj->base);
> >> +	if (IS_ERR(pages)) {
> >> +		return PTR_ERR(pages);
> >> +	}
> >> +
> >> +	obj->pages = pages;
> > 
> > That's a significant loss in functionality (it requires the entire
> > object to fit into physical memory at the same time and requires a large
> > vmalloc for 32b systems), for what? vgem only has the ability to mmap
> > and export a fd -- both of which you already have if you have the dmabuf
> > fd. The only extra interface is the signaling, which does not yet have a
> > direct correspondence on dmabuf.
> > 
> > (An obvious way to keep both would be to move the get_pages to importing
> > and then differentiate in the faulthandler where to get the page from.)
> > 
> 
> Thanks for pointing this out. I'll look to keep the existing behavior.
> 
> > Can you provide more details on how you are using vgem to justify the
> > changes? I'm also not particularly happy in losing testing of a virtual
> > platform device from our CI.
> > -Chris
> > 
> 
> There exists a test module in the Ion directory
> (drivers/staging/android/ion/ion_test.c) today but it's not actually
> Ion specific. It registers a platform device and then provides an
> ioctl interface to perform a dma_buf attach and map. I proposed
> moving this to a generic dma-buf test module
> (https://marc.info/?l=dri-devel&m=148952187004611&w=2) and Daniel
> suggested that this was roughly what vgem was supposed to do.

mmap(dma_buf_fd) gives you DMA_BUF_IOC_TEST_DMA_MAPPING, and that's the
only facility already available via vgem.

DMA_BUF_IOC_TEST_KERNEL_MAPPING would be equivalent to
read/write(dma_buf_fd).

> Adding the import methods for vgem covers all of what the
> Ion test was doing and we don't have to invent yet another ioctl
> interface and framework for attaching and mapping. 

I don't think it does :)

To muddy the waters further, I've also done something similar:
https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/selftests/mock_dmabuf.c
https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/selftests/i915_gem_dmabuf.c

But this feels like a good enough reason for implementing read/write ops
for the dma-buf fd, and then we can test the dma_buf->ops->kmap on i915
as well, directly from i915.ko. Sounds fun, I'll see if I can cook
something up - and we can then see if that suits your testing as well.
 
> Can you clarify about what you mean about 'virtual platform device'?

	vgem_device = drm_dev_alloc(&vgem_driver, NULL);

helps exercise some more corner cases of the drm core, that we have
unwittingly broken in the past.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [RFC PATCH 2/2] drm/vgem: Enable dmabuf import interfaces

[Laura Abbott]

On 04/07/2017 09:58 AM, Chris Wilson wrote:
> On Fri, Apr 07, 2017 at 09:18:30AM -0700, Laura Abbott wrote:
>> On 04/07/2017 12:39 AM, Chris Wilson wrote:
>>> On Thu, Apr 06, 2017 at 04:18:33PM -0700, Laura Abbott wrote:
>>>>
>>>> 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).
>>>>
>>>> +int vgem_gem_get_pages(struct drm_vgem_gem_object *obj)
>>>> +{
>>>> +	struct page **pages;
>>>> +
>>>> +	if (obj->pages)
>>>> +		return 0;
>>>> +
>>>> +	pages = drm_gem_get_pages(&obj->base);
>>>> +	if (IS_ERR(pages)) {
>>>> +		return PTR_ERR(pages);
>>>> +	}
>>>> +
>>>> +	obj->pages = pages;
>>>
>>> That's a significant loss in functionality (it requires the entire
>>> object to fit into physical memory at the same time and requires a large
>>> vmalloc for 32b systems), for what? vgem only has the ability to mmap
>>> and export a fd -- both of which you already have if you have the dmabuf
>>> fd. The only extra interface is the signaling, which does not yet have a
>>> direct correspondence on dmabuf.
>>>
>>> (An obvious way to keep both would be to move the get_pages to importing
>>> and then differentiate in the faulthandler where to get the page from.)
>>>
>>
>> Thanks for pointing this out. I'll look to keep the existing behavior.
>>
>>> Can you provide more details on how you are using vgem to justify the
>>> changes? I'm also not particularly happy in losing testing of a virtual
>>> platform device from our CI.
>>> -Chris
>>>
>>
>> There exists a test module in the Ion directory
>> (drivers/staging/android/ion/ion_test.c) today but it's not actually
>> Ion specific. It registers a platform device and then provides an
>> ioctl interface to perform a dma_buf attach and map. I proposed
>> moving this to a generic dma-buf test module
>> (https://marc.info/?l=dri-devel&m=148952187004611&w=2) and Daniel
>> suggested that this was roughly what vgem was supposed to do.
> 
> mmap(dma_buf_fd) gives you DMA_BUF_IOC_TEST_DMA_MAPPING, and that's the
> only facility already available via vgem.
> 

Not completely. It's possible to mmap a dma_buf fd without being
attached to any device (the source of many caching headaches). Part of
the goal is to verify that the attachment and map callbacks are
working as expected.

> DMA_BUF_IOC_TEST_KERNEL_MAPPING would be equivalent to
> read/write(dma_buf_fd).
> 
>> Adding the import methods for vgem covers all of what the
>> Ion test was doing and we don't have to invent yet another ioctl
>> interface and framework for attaching and mapping. 
> 
> I don't think it does :)
> 
> To muddy the waters further, I've also done something similar:
> https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/selftests/mock_dmabuf.c
> https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/selftests/i915_gem_dmabuf.c
> 

Those do look familiar :)

> But this feels like a good enough reason for implementing read/write ops
> for the dma-buf fd, and then we can test the dma_buf->ops->kmap on i915
> as well, directly from i915.ko. Sounds fun, I'll see if I can cook
> something up - and we can then see if that suits your testing as well.
>  
>> Can you clarify about what you mean about 'virtual platform device'?
> 
> 	vgem_device = drm_dev_alloc(&vgem_driver, NULL);
> 
> helps exercise some more corner cases of the drm core, that we have
> unwittingly broken in the past.

Okay thanks for the explanation. I was debating putting the platform
support behind a configuration option. I don't know if that would
actually solve anything or just result in another configuration that
doesn't get tested.

> -Chris
> 

Thanks,
Laura

Re: [RFC PATCH 2/2] drm/vgem: Enable dmabuf import interfaces

[Daniel Vetter]

On Fri, Apr 07, 2017 at 10:58:47AM -0700, Laura Abbott wrote:
> On 04/07/2017 09:58 AM, Chris Wilson wrote:
> > On Fri, Apr 07, 2017 at 09:18:30AM -0700, Laura Abbott wrote:
> >> On 04/07/2017 12:39 AM, Chris Wilson wrote:
> >>> On Thu, Apr 06, 2017 at 04:18:33PM -0700, Laura Abbott wrote:
> >>>>
> >>>> 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).
> >>>>
> >>>> +int vgem_gem_get_pages(struct drm_vgem_gem_object *obj)
> >>>> +{
> >>>> +	struct page **pages;
> >>>> +
> >>>> +	if (obj->pages)
> >>>> +		return 0;
> >>>> +
> >>>> +	pages = drm_gem_get_pages(&obj->base);
> >>>> +	if (IS_ERR(pages)) {
> >>>> +		return PTR_ERR(pages);
> >>>> +	}
> >>>> +
> >>>> +	obj->pages = pages;
> >>>
> >>> That's a significant loss in functionality (it requires the entire
> >>> object to fit into physical memory at the same time and requires a large
> >>> vmalloc for 32b systems), for what? vgem only has the ability to mmap
> >>> and export a fd -- both of which you already have if you have the dmabuf
> >>> fd. The only extra interface is the signaling, which does not yet have a
> >>> direct correspondence on dmabuf.
> >>>
> >>> (An obvious way to keep both would be to move the get_pages to importing
> >>> and then differentiate in the faulthandler where to get the page from.)
> >>>
> >>
> >> Thanks for pointing this out. I'll look to keep the existing behavior.
> >>
> >>> Can you provide more details on how you are using vgem to justify the
> >>> changes? I'm also not particularly happy in losing testing of a virtual
> >>> platform device from our CI.
> >>> -Chris
> >>>
> >>
> >> There exists a test module in the Ion directory
> >> (drivers/staging/android/ion/ion_test.c) today but it's not actually
> >> Ion specific. It registers a platform device and then provides an
> >> ioctl interface to perform a dma_buf attach and map. I proposed
> >> moving this to a generic dma-buf test module
> >> (https://marc.info/?l=dri-devel&m=148952187004611&w=2) and Daniel
> >> suggested that this was roughly what vgem was supposed to do.
> > 
> > mmap(dma_buf_fd) gives you DMA_BUF_IOC_TEST_DMA_MAPPING, and that's the
> > only facility already available via vgem.
> > 
> 
> Not completely. It's possible to mmap a dma_buf fd without being
> attached to any device (the source of many caching headaches). Part of
> the goal is to verify that the attachment and map callbacks are
> working as expected.
> 
> > DMA_BUF_IOC_TEST_KERNEL_MAPPING would be equivalent to
> > read/write(dma_buf_fd).
> > 
> >> Adding the import methods for vgem covers all of what the
> >> Ion test was doing and we don't have to invent yet another ioctl
> >> interface and framework for attaching and mapping. 
> > 
> > I don't think it does :)
> > 
> > To muddy the waters further, I've also done something similar:
> > https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/selftests/mock_dmabuf.c
> > https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/selftests/i915_gem_dmabuf.c
> > 
> 
> Those do look familiar :)
> 
> > But this feels like a good enough reason for implementing read/write ops
> > for the dma-buf fd, and then we can test the dma_buf->ops->kmap on i915
> > as well, directly from i915.ko. Sounds fun, I'll see if I can cook
> > something up - and we can then see if that suits your testing as well.
> >  
> >> Can you clarify about what you mean about 'virtual platform device'?
> > 
> > 	vgem_device = drm_dev_alloc(&vgem_driver, NULL);
> > 
> > helps exercise some more corner cases of the drm core, that we have
> > unwittingly broken in the past.
> 
> Okay thanks for the explanation. I was debating putting the platform
> support behind a configuration option. I don't know if that would
> actually solve anything or just result in another configuration that
> doesn't get tested.

We could do both still, the drm core doesn't care about the device, it's
only used to place it in the right directoy in sysfs (well, minus the bugs
Chris mentioned). So having a NULL device for drm_dev_alloc while still
storing that fake platform device somewhere should be all fine (if we need
that fake platform dev for dma-buf testing).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch