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(-)
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);
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;
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);
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;
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) {
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);
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;
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;
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);
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
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
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 byuser.
* 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
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 byuser.
* 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)====================================
- create gem1
- export the gem1 into dmabuf
- import the dmabuf into gem2
- close gem2
- 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.
-----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 releasedby
user.
* with dma_buf_put() call, if file->f_count is 0 then arelease
* 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)====================================
- create gem1
- export the gem1 into dmabuf
- import the dmabuf into gem2
- close gem2
- 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:
- 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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Inki Dae -
Rob Clark