[PATCH 00/10] drm/exynos: fix vidi driver and gem module

[PATCH 00/10] drm/exynos: fix vidi driver and gem module

[Inki Dae]

these patch sets fix some issues to gem and vidi driver and
include the following:
- fixes cache issue according to gpu operation
  . shmem_read_mapping_page_gfp function first tries to allocate pages
    from page cache and if the allocation is done from page cache
    the the pages could have valid cache line so cpu may read garbage data
    from cache after gpu operation is completed.
- use edid data from user propely and add some exception codes
- fixes releasing issue to exported gem buffer into dmabuf
- and code clean

Thanks.

Inki Dae (10):
  drm/exynos: removed unnecessary declaration.
  drm/exynos: set edid fake data only for test.
  drm/exynos: check if raw edid data is fake or not for test
  drm/exynos: fixed edid data setting at vidi connection request
  drm/exynos: fixed build warning.
  drm/exynos: use alloc_page() to allocate pages.
  drm/exynos: set buffer type from exporter.
  drm/exynos: do not release memory region from exporter.
  drm/exynos: removed unnecessary variable
  drm/exynos: consider memory releasing to exported gem buffer into
    dmabuf

 drivers/gpu/drm/exynos/exynos_drm_core.c   |    5 ---
 drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |   27 ++++++++++----
 drivers/gpu/drm/exynos/exynos_drm_drv.c    |    1 +
 drivers/gpu/drm/exynos/exynos_drm_gem.c    |   40 +++++++++++++++------
 drivers/gpu/drm/exynos/exynos_drm_gem.h    |    4 ++
 drivers/gpu/drm/exynos/exynos_drm_vidi.c   |   53 +++++++++++++++++++++------
 6 files changed, 95 insertions(+), 35 deletions(-)

-- 
1.7.4.1

[PATCH 01/10] drm/exynos: removed unnecessary declaration.

[Inki Dae]

Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_vidi.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index 7b9c153..5640d57 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -85,8 +85,6 @@ static const char fake_edid_info[] = {
 	0x00, 0x00, 0x00, 0x06
 };
 
-static void vidi_fake_vblank_handler(struct work_struct *work);
-
 static bool vidi_display_is_connected(struct device *dev)
 {
 	struct vidi_context *ctx = get_vidi_context(dev);
-- 
1.7.4.1

[PATCH 02/10] drm/exynos: set edid fake data only for test.

[Inki Dae]

Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_vidi.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index 5640d57..88dae6b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -529,6 +529,10 @@ static int vidi_store_connection(struct device *dev,
 	if (ctx->connected > 1)
 		return -EINVAL;
 
+	/* use fake edid data for test. */
+	if (!ctx->raw_edid)
+		ctx->raw_edid = (struct edid *)fake_edid_info;
+
 	DRM_DEBUG_KMS("requested connection.\n");
 
 	drm_helper_hpd_irq_event(ctx->subdrv.drm_dev);
@@ -612,9 +616,6 @@ static int __devinit vidi_probe(struct platform_device *pdev)
 
 	INIT_WORK(&ctx->work, vidi_fake_vblank_handler);
 
-	/* for test */
-	ctx->raw_edid = (struct edid *)fake_edid_info;
-
 	subdrv = &ctx->subdrv;
 	subdrv->dev = dev;
 	subdrv->manager = &vidi_manager;
-- 
1.7.4.1

[PATCH 03/10] drm/exynos: check if raw edid data is fake or not for test

[Inki Dae]

if raw edid data isn't same as fake data then it can't be tested.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_vidi.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index 88dae6b..dbbf2fc 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -533,6 +533,12 @@ static int vidi_store_connection(struct device *dev,
 	if (!ctx->raw_edid)
 		ctx->raw_edid = (struct edid *)fake_edid_info;
 
+	/* if raw_edid isn't same as fake data then it can't be tested. */
+	if (ctx->raw_edid != (struct edid *)fake_edid_info) {
+		DRM_DEBUG_KMS("edid data is not fake data.\n");
+		return -EINVAL;
+	}
+
 	DRM_DEBUG_KMS("requested connection.\n");
 
 	drm_helper_hpd_irq_event(ctx->subdrv.drm_dev);
-- 
1.7.4.1

[PATCH 04/10] drm/exynos: fixed edid data setting at vidi connection request

[Inki Dae]

edid data from user should be allocated and copied into vidi context and also
freed with disconnection.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_vidi.c |   38 ++++++++++++++++++++++++-----
 1 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index dbbf2fc..1d7d030 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -557,6 +557,8 @@ int vidi_connection_ioctl(struct drm_device *drm_dev, void *data,
 	struct exynos_drm_manager *manager;
 	struct exynos_drm_display_ops *display_ops;
 	struct drm_exynos_vidi_connection *vidi = data;
+	struct edid *raw_edid;
+	int edid_len;
 
 	DRM_DEBUG_KMS("%s\n", __FILE__);
 
@@ -565,11 +567,6 @@ int vidi_connection_ioctl(struct drm_device *drm_dev, void *data,
 		return -EINVAL;
 	}
 
-	if (!vidi->edid) {
-		DRM_DEBUG_KMS("edid data is null.\n");
-		return -EINVAL;
-	}
-
 	if (vidi->connection > 1) {
 		DRM_DEBUG_KMS("connection should be 0 or 1.\n");
 		return -EINVAL;
@@ -596,8 +593,30 @@ int vidi_connection_ioctl(struct drm_device *drm_dev, void *data,
 		return -EINVAL;
 	}
 
-	if (vidi->connection)
-		ctx->raw_edid = (struct edid *)vidi->edid;
+	if (vidi->connection) {
+		if (!vidi->edid) {
+			DRM_DEBUG_KMS("edid data is null.\n");
+			return -EINVAL;
+		}
+		raw_edid = (struct edid *)vidi->edid;
+		edid_len = (1 + raw_edid->extensions) * EDID_LENGTH;
+		ctx->raw_edid = kzalloc(edid_len, GFP_KERNEL);
+		if (!ctx->raw_edid) {
+			DRM_DEBUG_KMS("failed to allocate raw_edid.\n");
+			return -ENOMEM;
+		}
+		memcpy(ctx->raw_edid, raw_edid, edid_len);
+	} else {
+		/*
+		 * with connection = 0, free raw_edid
+		 * only if raw edid data isn't same as fake data.
+		 */
+		if (ctx->raw_edid && ctx->raw_edid !=
+				(struct edid *)fake_edid_info) {
+			kfree(ctx->raw_edid);
+			ctx->raw_edid = NULL;
+		}
+	}
 
 	ctx->connected = vidi->connection;
 	drm_helper_hpd_irq_event(ctx->subdrv.drm_dev);
@@ -649,6 +668,11 @@ static int __devexit vidi_remove(struct platform_device *pdev)
 
 	exynos_drm_subdrv_unregister(&ctx->subdrv);
 
+	if (ctx->raw_edid != (struct edid *)fake_edid_info) {
+		kfree(ctx->raw_edid);
+		ctx->raw_edid = NULL;
+	}
+
 	kfree(ctx);
 
 	return 0;
-- 
1.7.4.1

[PATCH 05/10] drm/exynos: fixed build warning.

[Inki Dae]

Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_vidi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index 1d7d030..bb1550c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -598,7 +598,7 @@ int vidi_connection_ioctl(struct drm_device *drm_dev, void *data,
 			DRM_DEBUG_KMS("edid data is null.\n");
 			return -EINVAL;
 		}
-		raw_edid = (struct edid *)vidi->edid;
+		raw_edid = (struct edid *)(uint32_t)vidi->edid;
 		edid_len = (1 + raw_edid->extensions) * EDID_LENGTH;
 		ctx->raw_edid = kzalloc(edid_len, GFP_KERNEL);
 		if (!ctx->raw_edid) {
-- 
1.7.4.1

[PATCH 06/10] drm/exynos: use alloc_page() to allocate pages.

[Inki Dae]

shmem_read_mapping_page_gfp() first tries to allocate pages from page cache
so if pages are allocated from page cache then these pages could have
valid cache line. after that cpu may read garbage data from cache
once gpu operation is completed with allocated pages. so with this patch,
Non-contiguous memory allocation request allocates pages from highmem
through alloc_page() with GFP_HIGHUSER_MOVABLE.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_gem.c |   14 +++-----------
 1 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index 5c8b683..c29c02d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -99,25 +99,17 @@ out:
 struct page **exynos_gem_get_pages(struct drm_gem_object *obj,
 						gfp_t gfpmask)
 {
-	struct inode *inode;
-	struct address_space *mapping;
 	struct page *p, **pages;
 	int i, npages;
 
-	/* This is the shared memory object that backs the GEM resource */
-	inode = obj->filp->f_path.dentry->d_inode;
-	mapping = inode->i_mapping;
-
 	npages = obj->size >> PAGE_SHIFT;
 
 	pages = drm_malloc_ab(npages, sizeof(struct page *));
 	if (pages == NULL)
 		return ERR_PTR(-ENOMEM);
 
-	gfpmask |= mapping_gfp_mask(mapping);
-
 	for (i = 0; i < npages; i++) {
-		p = shmem_read_mapping_page_gfp(mapping, i, gfpmask);
+		p = alloc_page(gfpmask);
 		if (IS_ERR(p))
 			goto fail;
 		pages[i] = p;
@@ -127,7 +119,7 @@ struct page **exynos_gem_get_pages(struct drm_gem_object *obj,
 
 fail:
 	while (i--)
-		page_cache_release(pages[i]);
+		__free_page(pages[i]);
 
 	drm_free_large(pages);
 	return ERR_PTR(PTR_ERR(p));
@@ -189,7 +181,7 @@ static int exynos_drm_gem_get_pages(struct drm_gem_object *obj)
 		return -EINVAL;
 	}
 
-	pages = exynos_gem_get_pages(obj, GFP_KERNEL);
+	pages = exynos_gem_get_pages(obj, GFP_HIGHUSER_MOVABLE);
 	if (IS_ERR(pages)) {
 		DRM_ERROR("failed to get pages.\n");
 		return PTR_ERR(pages);
-- 
1.7.4.1

[PATCH 07/10] drm/exynos: set buffer type from exporter.

[Inki Dae]

when fd is imported to gem, whether the memory type from exporter
is contigous or not should be set to gem flag so that drm-based
driver can aware of the memory type.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
index 2749092..38544c1 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
@@ -25,6 +25,7 @@
 
 #include "drmP.h"
 #include "drm.h"
+#include "exynos_drm.h"
 #include "exynos_drm_drv.h"
 #include "exynos_drm_gem.h"
 
@@ -186,7 +187,7 @@ struct drm_gem_object *exynos_dmabuf_prime_import(struct drm_device *drm_dev,
 	struct exynos_drm_gem_obj *exynos_gem_obj;
 	struct exynos_drm_gem_buf *buffer;
 	struct page *page;
-	int ret, i = 0;
+	int ret;
 
 	DRM_DEBUG_PRIME("%s\n", __FILE__);
 
@@ -236,13 +237,25 @@ struct drm_gem_object *exynos_dmabuf_prime_import(struct drm_device *drm_dev,
 	}
 
 	sgl = sgt->sgl;
-	buffer->dma_addr = sg_dma_address(sgl);
 
-	while (i < sgt->nents) {
-		buffer->pages[i] = sg_page(sgl);
-		buffer->size += sg_dma_len(sgl);
-		sgl = sg_next(sgl);
-		i++;
+	if (sgt->nents == 1) {
+		buffer->dma_addr = sg_dma_address(sgt->sgl);
+		buffer->size = sg_dma_len(sgt->sgl);
+
+		/* always physically continuous memory if sgt->nents is 1. */
+		exynos_gem_obj->flags |= EXYNOS_BO_CONTIG;
+	} else {
+		unsigned int i = 0;
+
+		buffer->dma_addr = sg_dma_address(sgl);
+		while (i < sgt->nents) {
+			buffer->pages[i] = sg_page(sgl);
+			buffer->size += sg_dma_len(sgl);
+			sgl = sg_next(sgl);
+			i++;
+		}
+
+		exynos_gem_obj->flags |= EXYNOS_BO_NONCONTIG;
 	}
 
 	exynos_gem_obj->buffer = buffer;
-- 
1.7.4.1

[PATCH 08/10] drm/exynos: do not release memory region from exporter.

[Inki Dae]

the region should be released by exporter once dmabuf's refcount becomes 0.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_gem.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index c29c02d..411d82b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -283,11 +283,21 @@ void exynos_drm_gem_destroy(struct exynos_drm_gem_obj *exynos_gem_obj)
 	if (!buf->pages)
 		return;
 
+	/*
+	 * do not release memory region from exporter.
+	 *
+	 * the region will be released by exporter
+	 * once dmabuf's refcount becomes 0.
+	 */
+	if (obj->import_attach)
+		goto out;
+
 	if (exynos_gem_obj->flags & EXYNOS_BO_NONCONTIG)
 		exynos_drm_gem_put_pages(obj);
 	else
 		exynos_drm_free_buf(obj->dev, exynos_gem_obj->flags, buf);
 
+out:
 	exynos_drm_fini_buf(obj->dev, buf);
 	exynos_gem_obj->buffer = NULL;
 
-- 
1.7.4.1

[PATCH 09/10] drm/exynos: removed unnecessary variable

[Inki Dae]

Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_core.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c b/drivers/gpu/drm/exynos/exynos_drm_core.c
index eaf630d..84dd099 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_core.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_core.c
@@ -33,7 +33,6 @@
 #include "exynos_drm_fbdev.h"
 
 static LIST_HEAD(exynos_drm_subdrv_list);
-static struct drm_device *drm_dev;
 
 static int exynos_drm_subdrv_probe(struct drm_device *dev,
 					struct exynos_drm_subdrv *subdrv)
@@ -120,8 +119,6 @@ int exynos_drm_device_register(struct drm_device *dev)
 	if (!dev)
 		return -EINVAL;
 
-	drm_dev = dev;
-
 	list_for_each_entry_safe(subdrv, n, &exynos_drm_subdrv_list, list) {
 		subdrv->drm_dev = dev;
 		err = exynos_drm_subdrv_probe(dev, subdrv);
@@ -149,8 +146,6 @@ int exynos_drm_device_unregister(struct drm_device *dev)
 	list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list)
 		exynos_drm_subdrv_remove(dev, subdrv);
 
-	drm_dev = NULL;
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(exynos_drm_device_unregister);
-- 
1.7.4.1

[PATCH 10/10] drm/exynos: consider memory releasing to exported gem buffer into dmabuf

[Inki Dae]

exported gem buffer into dmabuf should be released when a gem relese is
requested by user. with dma_buf_put() call, if file->f_count is 0 then
a release callback of exynos gem module(exporter) will be called to release
its own gem buffer.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c |    1 +
 drivers/gpu/drm/exynos/exynos_drm_gem.c |   16 ++++++++++++++++
 drivers/gpu/drm/exynos/exynos_drm_gem.h |    4 ++++
 3 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index d6de2e0..1501dd2 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -258,6 +258,7 @@ static struct drm_driver exynos_drm_driver = {
 	.gem_init_object	= exynos_drm_gem_init_object,
 	.gem_free_object	= exynos_drm_gem_free_object,
 	.gem_vm_ops		= &exynos_drm_gem_vm_ops,
+	.gem_close_object	= exynos_drm_gem_close_object,
 	.dumb_create		= exynos_drm_gem_dumb_create,
 	.dumb_map_offset	= exynos_drm_gem_dumb_map_offset,
 	.dumb_destroy		= exynos_drm_gem_dumb_destroy,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index 411d82b..5ca8641 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -27,6 +27,7 @@
 #include "drm.h"
 
 #include <linux/shmem_fs.h>
+#include <linux/dma-buf.h>
 #include <drm/exynos_drm.h>
 
 #include "exynos_drm_drv.h"
@@ -749,6 +750,21 @@ int exynos_drm_gem_dumb_destroy(struct drm_file *file_priv,
 	return 0;
 }
 
+void exynos_drm_gem_close_object(struct drm_gem_object *obj,
+				struct drm_file *file)
+{
+	DRM_DEBUG_KMS("%s\n", __FILE__);
+
+	/*
+	 * exported dmabuf should be released when a gem is released by user.
+	 * with dma_buf_put() call, if file->f_count is 0 then a release
+	 * callback of gem module(exporter) will be called to release
+	 * its own gem buffer.
+	 */
+	if (obj->export_dma_buf)
+		dma_buf_put(obj->export_dma_buf);
+}
+
 int exynos_drm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
 	struct drm_gem_object *obj = vma->vm_private_data;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h
index 14d038b..4f1ba1a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
@@ -162,4 +162,8 @@ int exynos_drm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
 /* set vm_flags and we can change the vm attribute to other one at here. */
 int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
 
+/* do extra release exynos gem module needs when gem close is called. */
+void exynos_drm_gem_close_object(struct drm_gem_object *obj,
+				struct drm_file *file);
+
 #endif
-- 
1.7.4.1

Re: [PATCH 10/10] drm/exynos: consider memory releasing to exported gem buffer into dmabuf

[Rob Clark]

On Wed, Jun 27, 2012 at 3:03 AM, Inki Dae <inki.dae@samsung.com> wrote:
> exported gem buffer into dmabuf should be released when a gem relese is
> requested by user. with dma_buf_put() call, if file->f_count is 0 then
> a release callback of exynos gem module(exporter) will be called to release
> its own gem buffer.
>
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c |    1 +
>  drivers/gpu/drm/exynos/exynos_drm_gem.c |   16 ++++++++++++++++
>  drivers/gpu/drm/exynos/exynos_drm_gem.h |    4 ++++
>  3 files changed, 21 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index d6de2e0..1501dd2 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -258,6 +258,7 @@ static struct drm_driver exynos_drm_driver = {
>        .gem_init_object        = exynos_drm_gem_init_object,
>        .gem_free_object        = exynos_drm_gem_free_object,
>        .gem_vm_ops             = &exynos_drm_gem_vm_ops,
> +       .gem_close_object       = exynos_drm_gem_close_object,
>        .dumb_create            = exynos_drm_gem_dumb_create,
>        .dumb_map_offset        = exynos_drm_gem_dumb_map_offset,
>        .dumb_destroy           = exynos_drm_gem_dumb_destroy,
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index 411d82b..5ca8641 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -27,6 +27,7 @@
>  #include "drm.h"
>
>  #include <linux/shmem_fs.h>
> +#include <linux/dma-buf.h>
>  #include <drm/exynos_drm.h>
>
>  #include "exynos_drm_drv.h"
> @@ -749,6 +750,21 @@ int exynos_drm_gem_dumb_destroy(struct drm_file *file_priv,
>        return 0;
>  }
>
> +void exynos_drm_gem_close_object(struct drm_gem_object *obj,
> +                               struct drm_file *file)
> +{
> +       DRM_DEBUG_KMS("%s\n", __FILE__);
> +
> +       /*
> +        * exported dmabuf should be released when a gem is released by user.
> +        * with dma_buf_put() call, if file->f_count is 0 then a release
> +        * callback of gem module(exporter) will be called to release
> +        * its own gem buffer.
> +        */
> +       if (obj->export_dma_buf)
> +               dma_buf_put(obj->export_dma_buf);

this doesn't seem quite right to me..  although I think the dmabuf
release fxn needs to NULL out the obj->export_dma_buf.

If your GEM obj is getting released before your dmabuf then there is
something going wrong with the ref cnt'ing.

BR,
-R

> +}
> +
>  int exynos_drm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>  {
>        struct drm_gem_object *obj = vma->vm_private_data;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> index 14d038b..4f1ba1a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> @@ -162,4 +162,8 @@ int exynos_drm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
>  /* set vm_flags and we can change the vm attribute to other one at here. */
>  int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
>
> +/* do extra release exynos gem module needs when gem close is called. */
> +void exynos_drm_gem_close_object(struct drm_gem_object *obj,
> +                               struct drm_file *file);
> +
>  #endif
> --
> 1.7.4.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 10/10] drm/exynos: consider memory releasing to exported gem buffer into dmabuf

[Inki Dae]

Hi Rob,

> -----Original Message-----
> From: robdclark@gmail.com [mailto:robdclark@gmail.com] On Behalf Of Rob
> Clark
> Sent: Wednesday, June 27, 2012 9:20 PM
> To: Inki Dae
> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
> kyungmin.park@samsung.com; sw0312.kim@samsung.com
> Subject: Re: [PATCH 10/10] drm/exynos: consider memory releasing to
> exported gem buffer into dmabuf
> 
> On Wed, Jun 27, 2012 at 3:03 AM, Inki Dae <inki.dae@samsung.com> wrote:
> > exported gem buffer into dmabuf should be released when a gem relese is
> > requested by user. with dma_buf_put() call, if file->f_count is 0 then
> > a release callback of exynos gem module(exporter) will be called to
> release
> > its own gem buffer.
> >
> > Signed-off-by: Inki Dae <inki.dae@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_drv.c |    1 +
> >  drivers/gpu/drm/exynos/exynos_drm_gem.c |   16 ++++++++++++++++
> >  drivers/gpu/drm/exynos/exynos_drm_gem.h |    4 ++++
> >  3 files changed, 21 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > index d6de2e0..1501dd2 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > @@ -258,6 +258,7 @@ static struct drm_driver exynos_drm_driver = {
> >        .gem_init_object        = exynos_drm_gem_init_object,
> >        .gem_free_object        = exynos_drm_gem_free_object,
> >        .gem_vm_ops             = &exynos_drm_gem_vm_ops,
> > +       .gem_close_object       = exynos_drm_gem_close_object,
> >        .dumb_create            = exynos_drm_gem_dumb_create,
> >        .dumb_map_offset        = exynos_drm_gem_dumb_map_offset,
> >        .dumb_destroy           = exynos_drm_gem_dumb_destroy,
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > index 411d82b..5ca8641 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > @@ -27,6 +27,7 @@
> >  #include "drm.h"
> >
> >  #include <linux/shmem_fs.h>
> > +#include <linux/dma-buf.h>
> >  #include <drm/exynos_drm.h>
> >
> >  #include "exynos_drm_drv.h"
> > @@ -749,6 +750,21 @@ int exynos_drm_gem_dumb_destroy(struct drm_file
> *file_priv,
> >        return 0;
> >  }
> >
> > +void exynos_drm_gem_close_object(struct drm_gem_object *obj,
> > +                               struct drm_file *file)
> > +{
> > +       DRM_DEBUG_KMS("%s\n", __FILE__);
> > +
> > +       /*
> > +        * exported dmabuf should be released when a gem is released by
> user.
> > +        * with dma_buf_put() call, if file->f_count is 0 then a release
> > +        * callback of gem module(exporter) will be called to release
> > +        * its own gem buffer.
> > +        */
> > +       if (obj->export_dma_buf)
> > +               dma_buf_put(obj->export_dma_buf);
> 
> this doesn't seem quite right to me..  although I think the dmabuf
> release fxn needs to NULL out the obj->export_dma_buf.
> 
> If your GEM obj is getting released before your dmabuf then there is
> something going wrong with the ref cnt'ing.
> 
> BR,
> -R
> 

Could you look into below steps? I understood gem and dmabuf life cycle like below so thought we needs this patch. with this patch, the gem object isn't getting released before dmabuf and the gem will be released by dma_buf_put(). if there is my missing point then please give me any comment.

Reference count(step number)
====================================
gem object1   gem object2       dmabuf
------------------------------------
    1(1)
    2(2)                               1(2)
                     1(3)              2(3)
                     0(4)              1(4)
    1(5)                               0(5)
    0(5)
====================================

1. create gem1
2. export the gem1 into dmabuf
3. import the dmabuf into gem2
4. close gem2
5. close gem1


Thanks,
Inki Dae

> > +}
> > +
> >  int exynos_drm_gem_fault(struct vm_area_struct *vma, struct vm_fault
> *vmf)
> >  {
> >        struct drm_gem_object *obj = vma->vm_private_data;
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> > index 14d038b..4f1ba1a 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> > @@ -162,4 +162,8 @@ int exynos_drm_gem_fault(struct vm_area_struct *vma,
> struct vm_fault *vmf);
> >  /* set vm_flags and we can change the vm attribute to other one at here.
> */
> >  int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
> >
> > +/* do extra release exynos gem module needs when gem close is called.
> */
> > +void exynos_drm_gem_close_object(struct drm_gem_object *obj,
> > +                               struct drm_file *file);
> > +
> >  #endif
> > --
> > 1.7.4.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] drm/exynos: consider memory releasing to exported gem buffer into dmabuf

[Daniel Vetter]

On Wed, Jun 27, 2012 at 09:44:15PM +0900, Inki Dae wrote:
> Hi Rob,
> 
> > -----Original Message-----
> > From: robdclark@gmail.com [mailto:robdclark@gmail.com] On Behalf Of Rob
> > Clark
> > Sent: Wednesday, June 27, 2012 9:20 PM
> > To: Inki Dae
> > Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
> > kyungmin.park@samsung.com; sw0312.kim@samsung.com
> > Subject: Re: [PATCH 10/10] drm/exynos: consider memory releasing to
> > exported gem buffer into dmabuf
> > 
> > On Wed, Jun 27, 2012 at 3:03 AM, Inki Dae <inki.dae@samsung.com> wrote:
> > > exported gem buffer into dmabuf should be released when a gem relese is
> > > requested by user. with dma_buf_put() call, if file->f_count is 0 then
> > > a release callback of exynos gem module(exporter) will be called to
> > release
> > > its own gem buffer.
> > >
> > > Signed-off-by: Inki Dae <inki.dae@samsung.com>
> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > > ---
> > >  drivers/gpu/drm/exynos/exynos_drm_drv.c |    1 +
> > >  drivers/gpu/drm/exynos/exynos_drm_gem.c |   16 ++++++++++++++++
> > >  drivers/gpu/drm/exynos/exynos_drm_gem.h |    4 ++++
> > >  3 files changed, 21 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > > index d6de2e0..1501dd2 100644
> > > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > > @@ -258,6 +258,7 @@ static struct drm_driver exynos_drm_driver = {
> > >        .gem_init_object        = exynos_drm_gem_init_object,
> > >        .gem_free_object        = exynos_drm_gem_free_object,
> > >        .gem_vm_ops             = &exynos_drm_gem_vm_ops,
> > > +       .gem_close_object       = exynos_drm_gem_close_object,
> > >        .dumb_create            = exynos_drm_gem_dumb_create,
> > >        .dumb_map_offset        = exynos_drm_gem_dumb_map_offset,
> > >        .dumb_destroy           = exynos_drm_gem_dumb_destroy,
> > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > > index 411d82b..5ca8641 100644
> > > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > > @@ -27,6 +27,7 @@
> > >  #include "drm.h"
> > >
> > >  #include <linux/shmem_fs.h>
> > > +#include <linux/dma-buf.h>
> > >  #include <drm/exynos_drm.h>
> > >
> > >  #include "exynos_drm_drv.h"
> > > @@ -749,6 +750,21 @@ int exynos_drm_gem_dumb_destroy(struct drm_file
> > *file_priv,
> > >        return 0;
> > >  }
> > >
> > > +void exynos_drm_gem_close_object(struct drm_gem_object *obj,
> > > +                               struct drm_file *file)
> > > +{
> > > +       DRM_DEBUG_KMS("%s\n", __FILE__);
> > > +
> > > +       /*
> > > +        * exported dmabuf should be released when a gem is released by
> > user.
> > > +        * with dma_buf_put() call, if file->f_count is 0 then a release
> > > +        * callback of gem module(exporter) will be called to release
> > > +        * its own gem buffer.
> > > +        */
> > > +       if (obj->export_dma_buf)
> > > +               dma_buf_put(obj->export_dma_buf);
> > 
> > this doesn't seem quite right to me..  although I think the dmabuf
> > release fxn needs to NULL out the obj->export_dma_buf.
> > 
> > If your GEM obj is getting released before your dmabuf then there is
> > something going wrong with the ref cnt'ing.
> > 
> > BR,
> > -R
> > 
> 
> Could you look into below steps? I understood gem and dmabuf life cycle like below so thought we needs this patch. with this patch, the gem object isn't getting released before dmabuf and the gem will be released by dma_buf_put(). if there is my missing point then please give me any comment.
> 
> Reference count(step number)
> ====================================
> gem object1   gem object2       dmabuf
> ------------------------------------
>     1(1)
>     2(2)                               1(2)
>                      1(3)              2(3)
>                      0(4)              1(4)
>     1(5)                               0(5)
>     0(5)
> ====================================
> 
> 1. create gem1
> 2. export the gem1 into dmabuf
> 3. import the dmabuf into gem2
> 4. close gem2
> 5. close gem1

Step 5 looks wrong, simply closing the gem object 1 (in the exporting
driver) can't change the reference count of the dmabuf object.

Furthermore the dmabuf object _must_ be able to outlive the object it's
been created from. E.g. when you use dma-buf fd handles to facilitate
cross-process buffer sharing (maybe even on the same drm device, i.e. not
necessarily across devices), process A exports the dmabuf and passes the
fd and, process B imports it. Currently with dri2/gem_flink process A
needs to keep the gem object around until process B has confirmed that it
has imported the object, which is really ugly. But because fds themselves
hold a reference, this is not required for dma-buf cross-process sharing.

But if you break that (whith something like this patch) that won't work.
Long story short, your description above is missing step 6:

6. close dma-buf fd

> Reference count(step number)
> ====================================
> gem object1   gem object2       dmabuf
> ------------------------------------
>     1(1)
>     2(2)                               1(2)
>                      1(3)              2(3)
>                      0(4)              1(4)
>     1(5)                               1(5)
>                                        0(6)
> ====================================

Only then may the object's backing storage be freed.

Cheers, Daniel

PS: There's the funny thing what happens when you import a dma-buf into
the same gem/drm device: Then the driver _must_ _not_ create a new gem
object, but instead must increment the reference count of the underlying
gem object and create a new fd name for that underlying gem object. The
driver can check for this case by comparing the dma-buf ops and priv
fields.
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

RE: [PATCH 10/10] drm/exynos: consider memory releasing to exported gem buffer into dmabuf

[Inki Dae]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Wednesday, June 27, 2012 9:55 PM
> To: Inki Dae
> Cc: 'Rob Clark'; kyungmin.park@samsung.com; sw0312.kim@samsung.com; dri-
> devel@lists.freedesktop.org
> Subject: Re: [PATCH 10/10] drm/exynos: consider memory releasing to
> exported gem buffer into dmabuf
> 
> On Wed, Jun 27, 2012 at 09:44:15PM +0900, Inki Dae wrote:
> > Hi Rob,
> >
> > > -----Original Message-----
> > > From: robdclark@gmail.com [mailto:robdclark@gmail.com] On Behalf Of
> Rob
> > > Clark
> > > Sent: Wednesday, June 27, 2012 9:20 PM
> > > To: Inki Dae
> > > Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
> > > kyungmin.park@samsung.com; sw0312.kim@samsung.com
> > > Subject: Re: [PATCH 10/10] drm/exynos: consider memory releasing to
> > > exported gem buffer into dmabuf
> > >
> > > On Wed, Jun 27, 2012 at 3:03 AM, Inki Dae <inki.dae@samsung.com>
wrote:
> > > > exported gem buffer into dmabuf should be released when a gem relese
> is
> > > > requested by user. with dma_buf_put() call, if file->f_count is 0
> then
> > > > a release callback of exynos gem module(exporter) will be called to
> > > release
> > > > its own gem buffer.
> > > >
> > > > Signed-off-by: Inki Dae <inki.dae@samsung.com>
> > > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > > > ---
> > > >  drivers/gpu/drm/exynos/exynos_drm_drv.c |    1 +
> > > >  drivers/gpu/drm/exynos/exynos_drm_gem.c |   16 ++++++++++++++++
> > > >  drivers/gpu/drm/exynos/exynos_drm_gem.h |    4 ++++
> > > >  3 files changed, 21 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > > b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > > > index d6de2e0..1501dd2 100644
> > > > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > > > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > > > @@ -258,6 +258,7 @@ static struct drm_driver exynos_drm_driver = {
> > > >        .gem_init_object        = exynos_drm_gem_init_object,
> > > >        .gem_free_object        = exynos_drm_gem_free_object,
> > > >        .gem_vm_ops             = &exynos_drm_gem_vm_ops,
> > > > +       .gem_close_object       = exynos_drm_gem_close_object,
> > > >        .dumb_create            = exynos_drm_gem_dumb_create,
> > > >        .dumb_map_offset        = exynos_drm_gem_dumb_map_offset,
> > > >        .dumb_destroy           = exynos_drm_gem_dumb_destroy,
> > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > > b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > > > index 411d82b..5ca8641 100644
> > > > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > > > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > > > @@ -27,6 +27,7 @@
> > > >  #include "drm.h"
> > > >
> > > >  #include <linux/shmem_fs.h>
> > > > +#include <linux/dma-buf.h>
> > > >  #include <drm/exynos_drm.h>
> > > >
> > > >  #include "exynos_drm_drv.h"
> > > > @@ -749,6 +750,21 @@ int exynos_drm_gem_dumb_destroy(struct drm_file
> > > *file_priv,
> > > >        return 0;
> > > >  }
> > > >
> > > > +void exynos_drm_gem_close_object(struct drm_gem_object *obj,
> > > > +                               struct drm_file *file)
> > > > +{
> > > > +       DRM_DEBUG_KMS("%s\n", __FILE__);
> > > > +
> > > > +       /*
> > > > +        * exported dmabuf should be released when a gem is released
> by
> > > user.
> > > > +        * with dma_buf_put() call, if file->f_count is 0 then a
> release
> > > > +        * callback of gem module(exporter) will be called to
release
> > > > +        * its own gem buffer.
> > > > +        */
> > > > +       if (obj->export_dma_buf)
> > > > +               dma_buf_put(obj->export_dma_buf);
> > >
> > > this doesn't seem quite right to me..  although I think the dmabuf
> > > release fxn needs to NULL out the obj->export_dma_buf.
> > >
> > > If your GEM obj is getting released before your dmabuf then there is
> > > something going wrong with the ref cnt'ing.
> > >
> > > BR,
> > > -R
> > >
> >
> > Could you look into below steps? I understood gem and dmabuf life cycle
> like below so thought we needs this patch. with this patch, the gem object
> isn't getting released before dmabuf and the gem will be released by
> dma_buf_put(). if there is my missing point then please give me any
> comment.
> >
> > Reference count(step number)
> > ====================================
> > gem object1   gem object2       dmabuf
> > ------------------------------------
> >     1(1)
> >     2(2)                               1(2)
> >                      1(3)              2(3)
> >                      0(4)              1(4)
> >     1(5)                               0(5)
> >     0(5)
> > ====================================
> >
> > 1. create gem1
> > 2. export the gem1 into dmabuf
> > 3. import the dmabuf into gem2
> > 4. close gem2
> > 5. close gem1
> 
> Step 5 looks wrong, simply closing the gem object 1 (in the exporting
> driver) can't change the reference count of the dmabuf object.
> 
> Furthermore the dmabuf object _must_ be able to outlive the object it's
> been created from. E.g. when you use dma-buf fd handles to facilitate
> cross-process buffer sharing (maybe even on the same drm device, i.e. not
> necessarily across devices), process A exports the dmabuf and passes the
> fd and, process B imports it. Currently with dri2/gem_flink process A
> needs to keep the gem object around until process B has confirmed that it
> has imported the object, which is really ugly. But because fds themselves

Ok, I understood. with this patch, process B can't import the gem if process
A already released the gem before that. as you mentioned, I missed step 6.
thanks for your comment.

> hold a reference, this is not required for dma-buf cross-process sharing.
> 
> But if you break that (whith something like this patch) that won't work.
> Long story short, your description above is missing step 6:
> 
> 6. close dma-buf fd
> 
> > Reference count(step number)
> > ====================================
> > gem object1   gem object2       dmabuf
> > ------------------------------------
> >     1(1)
> >     2(2)                               1(2)
> >                      1(3)              2(3)
> >                      0(4)              1(4)
> >     1(5)                               1(5)
> >                                        0(6)
> > ====================================
> 
> Only then may the object's backing storage be freed.
> 
> Cheers, Daniel
> 
> PS: There's the funny thing what happens when you import a dma-buf into
> the same gem/drm device: Then the driver _must_ _not_ create a new gem
> object, but instead must increment the reference count of the underlying
> gem object and create a new fd name for that underlying gem object. The
> driver can check for this case by comparing the dma-buf ops and priv
> fields.

Above case was just simple example. actually our driver checks for that
case.

Thanks,
Inki Dae

> --
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48