* [PATCH 1/3] drm: Add dma_dev in struct drm_device
2018-10-26 7:22 [PATCH 0/3] Mediatek drm driver use drm_gem_cma_object instead of mtk_drm_gem_obj CK Hu
@ 2018-10-26 7:22 ` CK Hu
2018-10-26 7:22 ` [PATCH 2/3] drm: Add drm_gem_cma_dumb_create_no_kmap() helper function CK Hu
2018-10-26 7:22 ` [PATCH 3/3] drm/mediatek: Use drm_gem_cma_object instead of mtk_drm_gem_obj CK Hu
2 siblings, 0 replies; 9+ messages in thread
From: CK Hu @ 2018-10-26 7:22 UTC (permalink / raw)
To: Daniel Vetter, David Airlie, Gustavo Padovan, Maarten Lankhorst,
Sean Paul, CK Hu, Philipp Zabel
Cc: Matthias Brugger, linux-kernel, dri-devel, linux-arm-kernel,
linux-mediatek, srv_heupstream
For drm device which has sub device, the dma function may be in the sub
device rather than it, so the dma_dev is the device which has dma
function and use this device for dma operation.
Signed-off-by: CK Hu <ck.hu@mediatek.com>
---
drivers/gpu/drm/drm_gem_cma_helper.c | 19 ++++++++++++++-----
include/drm/drm_device.h | 1 +
2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 80a5115..0ba2c2a 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -101,6 +101,7 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
size_t size)
{
struct drm_gem_cma_object *cma_obj;
+ struct device *dev = drm->dma_dev ? drm->dma_dev : drm->dev;
int ret;
size = round_up(size, PAGE_SIZE);
@@ -109,10 +110,10 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
if (IS_ERR(cma_obj))
return cma_obj;
- cma_obj->vaddr = dma_alloc_wc(drm->dev, size, &cma_obj->paddr,
+ cma_obj->vaddr = dma_alloc_wc(dev, size, &cma_obj->paddr,
GFP_KERNEL | __GFP_NOWARN);
if (!cma_obj->vaddr) {
- dev_dbg(drm->dev, "failed to allocate buffer with size %zu\n",
+ dev_dbg(dev, "failed to allocate buffer with size %zu\n",
size);
ret = -ENOMEM;
goto error;
@@ -182,11 +183,14 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
{
struct drm_gem_cma_object *cma_obj;
+ struct device *dev;
cma_obj = to_drm_gem_cma_obj(gem_obj);
if (cma_obj->vaddr) {
- dma_free_wc(gem_obj->dev->dev, cma_obj->base.size,
+ dev = gem_obj->dev->dma_dev ?
+ gem_obj->dev->dma_dev : gem_obj->dev->dev;
+ dma_free_wc(dev, cma_obj->base.size,
cma_obj->vaddr, cma_obj->paddr);
} else if (gem_obj->import_attach) {
drm_prime_gem_destroy(gem_obj, cma_obj->sgt);
@@ -274,6 +278,7 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
struct vm_area_struct *vma)
{
int ret;
+ struct device *dev;
/*
* Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
@@ -283,7 +288,9 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
vma->vm_flags &= ~VM_PFNMAP;
vma->vm_pgoff = 0;
- ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr,
+ dev = cma_obj->base.dev->dma_dev ?
+ cma_obj->base.dev->dma_dev : cma_obj->base.dev->dev;
+ ret = dma_mmap_wc(dev, vma, cma_obj->vaddr,
cma_obj->paddr, vma->vm_end - vma->vm_start);
if (ret)
drm_gem_vm_close(vma);
@@ -432,13 +439,15 @@ struct sg_table *drm_gem_cma_prime_get_sg_table(struct drm_gem_object *obj)
{
struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
struct sg_table *sgt;
+ struct device *dev;
int ret;
sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
if (!sgt)
return NULL;
- ret = dma_get_sgtable(obj->dev->dev, sgt, cma_obj->vaddr,
+ dev = obj->dev->dma_dev ? obj->dev->dma_dev : obj->dev->dev;
+ ret = dma_get_sgtable(dev, sgt, cma_obj->vaddr,
cma_obj->paddr, obj->size);
if (ret < 0)
goto out;
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index f9c6e0e..ede9228 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -113,6 +113,7 @@ struct drm_device {
/** \name DMA support */
/*@{ */
+ struct device *dma_dev; /**< Device structure of dma device */
struct drm_device_dma *dma; /**< Optional pointer for DMA support */
/*@} */
--
1.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/3] drm: Add drm_gem_cma_dumb_create_no_kmap() helper function
2018-10-26 7:22 [PATCH 0/3] Mediatek drm driver use drm_gem_cma_object instead of mtk_drm_gem_obj CK Hu
2018-10-26 7:22 ` [PATCH 1/3] drm: Add dma_dev in struct drm_device CK Hu
@ 2018-10-26 7:22 ` CK Hu
2018-10-26 7:22 ` [PATCH 3/3] drm/mediatek: Use drm_gem_cma_object instead of mtk_drm_gem_obj CK Hu
2 siblings, 0 replies; 9+ messages in thread
From: CK Hu @ 2018-10-26 7:22 UTC (permalink / raw)
To: Daniel Vetter, David Airlie, Gustavo Padovan, Maarten Lankhorst,
Sean Paul, CK Hu, Philipp Zabel
Cc: Matthias Brugger, linux-kernel, dri-devel, linux-arm-kernel,
linux-mediatek, srv_heupstream
For iommu-supporting device, mapping kernel virtual address would reduce
free virtual memory area, and kernel usually need not using this virtual
address, so add drm_gem_cma_dumb_create_no_kmap() to create cma dumb
without mapping kernel virtual address.
Signed-off-by: CK Hu <ck.hu@mediatek.com>
---
drivers/gpu/drm/drm_gem_cma_helper.c | 99 +++++++++++++++++++++++++++++-------
include/drm/drm_gem_cma_helper.h | 7 +++
2 files changed, 88 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 0ba2c2a..c8e0e8e 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -85,20 +85,23 @@
}
/**
- * drm_gem_cma_create - allocate an object with the given size
+ * drm_gem_cma_create_kmap - allocate an object with the given size and
+ * map kernel virtual address.
* @drm: DRM device
* @size: size of the object to allocate
+ * @alloc_kmap: dma allocation with kernel mapping
*
- * This function creates a CMA GEM object and allocates a contiguous chunk of
- * memory as backing store. The backing memory has the writecombine attribute
- * set.
+ * This function creates a CMA GEM object and allocates a memory as
+ * backing store. The backing memory has the writecombine attribute
+ * set. If alloc_kmap is true, the backing memory also has the kernel mapping
+ * attribute set.
*
* Returns:
* A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative
* error code on failure.
*/
-struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
- size_t size)
+static struct drm_gem_cma_object *
+drm_gem_cma_create_kmap(struct drm_device *drm, size_t size, bool alloc_kmap)
{
struct drm_gem_cma_object *cma_obj;
struct device *dev = drm->dma_dev ? drm->dma_dev : drm->dev;
@@ -110,21 +113,48 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
if (IS_ERR(cma_obj))
return cma_obj;
- cma_obj->vaddr = dma_alloc_wc(dev, size, &cma_obj->paddr,
- GFP_KERNEL | __GFP_NOWARN);
- if (!cma_obj->vaddr) {
+ cma_obj->dma_attrs = DMA_ATTR_WRITE_COMBINE;
+ if (!alloc_kmap)
+ cma_obj->dma_attrs |= DMA_ATTR_NO_KERNEL_MAPPING;
+
+ cma_obj->cookie = dma_alloc_attrs(dev, size, &cma_obj->paddr,
+ GFP_KERNEL | __GFP_NOWARN,
+ cma_obj->dma_attrs);
+ if (!cma_obj->cookie) {
dev_dbg(dev, "failed to allocate buffer with size %zu\n",
size);
ret = -ENOMEM;
goto error;
}
+ if (alloc_kmap)
+ cma_obj->vaddr = cma_obj->cookie;
+
return cma_obj;
error:
drm_gem_object_put_unlocked(&cma_obj->base);
return ERR_PTR(ret);
}
+
+/**
+ * drm_gem_cma_create - allocate an object with the given size
+ * @drm: DRM device
+ * @size: size of the object to allocate
+ *
+ * This function creates a CMA GEM object and allocates a contiguous chunk of
+ * memory as backing store. The backing memory has the writecombine attribute
+ * set.
+ *
+ * Returns:
+ * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative
+ * error code on failure.
+ */
+struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
+ size_t size)
+{
+ return drm_gem_cma_create_kmap(drm, size, true);
+}
EXPORT_SYMBOL_GPL(drm_gem_cma_create);
/**
@@ -146,13 +176,13 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
static struct drm_gem_cma_object *
drm_gem_cma_create_with_handle(struct drm_file *file_priv,
struct drm_device *drm, size_t size,
- uint32_t *handle)
+ uint32_t *handle, bool alloc_kmap)
{
struct drm_gem_cma_object *cma_obj;
struct drm_gem_object *gem_obj;
int ret;
- cma_obj = drm_gem_cma_create(drm, size);
+ cma_obj = drm_gem_cma_create_kmap(drm, size, alloc_kmap);
if (IS_ERR(cma_obj))
return cma_obj;
@@ -187,11 +217,12 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
cma_obj = to_drm_gem_cma_obj(gem_obj);
- if (cma_obj->vaddr) {
+ if (cma_obj->cookie) {
dev = gem_obj->dev->dma_dev ?
gem_obj->dev->dma_dev : gem_obj->dev->dev;
- dma_free_wc(dev, cma_obj->base.size,
- cma_obj->vaddr, cma_obj->paddr);
+ dma_free_attrs(dev, cma_obj->base.size,
+ cma_obj->cookie, cma_obj->paddr,
+ cma_obj->dma_attrs);
} else if (gem_obj->import_attach) {
drm_prime_gem_destroy(gem_obj, cma_obj->sgt);
}
@@ -230,7 +261,7 @@ int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
args->size = args->pitch * args->height;
cma_obj = drm_gem_cma_create_with_handle(file_priv, drm, args->size,
- &args->handle);
+ &args->handle, true);
return PTR_ERR_OR_ZERO(cma_obj);
}
EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create_internal);
@@ -263,11 +294,43 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
args->size = args->pitch * args->height;
cma_obj = drm_gem_cma_create_with_handle(file_priv, drm, args->size,
- &args->handle);
+ &args->handle, true);
return PTR_ERR_OR_ZERO(cma_obj);
}
EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create);
+/**
+ * drm_gem_cma_dumb_create_no_kmap - create a dumb buffer object without
+ * kernel mapping
+ * @file_priv: DRM file-private structure to create the dumb buffer for
+ * @drm: DRM device
+ * @args: IOCTL data
+ *
+ * This function computes the pitch of the dumb buffer and rounds it up to an
+ * integer number of bytes per pixel. Drivers for hardware that doesn't have
+ * any additional restrictions on the pitch can directly use this function as
+ * their &drm_driver.dumb_create callback.
+ *
+ * For hardware with additional restrictions, don't use this function.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_gem_cma_dumb_create_no_kmap(struct drm_file *file_priv,
+ struct drm_device *drm,
+ struct drm_mode_create_dumb *args)
+{
+ struct drm_gem_cma_object *cma_obj;
+
+ args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+ args->size = args->pitch * args->height;
+
+ cma_obj = drm_gem_cma_create_with_handle(file_priv, drm, args->size,
+ &args->handle, false);
+ return PTR_ERR_OR_ZERO(cma_obj);
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create_no_kmap);
+
const struct vm_operations_struct drm_gem_cma_vm_ops = {
.open = drm_gem_vm_open,
.close = drm_gem_vm_close,
@@ -290,7 +353,7 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
dev = cma_obj->base.dev->dma_dev ?
cma_obj->base.dev->dma_dev : cma_obj->base.dev->dev;
- ret = dma_mmap_wc(dev, vma, cma_obj->vaddr,
+ ret = dma_mmap_wc(dev, vma, cma_obj->cookie,
cma_obj->paddr, vma->vm_end - vma->vm_start);
if (ret)
drm_gem_vm_close(vma);
@@ -447,7 +510,7 @@ struct sg_table *drm_gem_cma_prime_get_sg_table(struct drm_gem_object *obj)
return NULL;
dev = obj->dev->dma_dev ? obj->dev->dma_dev : obj->dev->dev;
- ret = dma_get_sgtable(dev, sgt, cma_obj->vaddr,
+ ret = dma_get_sgtable(dev, sgt, cma_obj->cookie,
cma_obj->paddr, obj->size);
if (ret < 0)
goto out;
diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
index 1977714..5164925 100644
--- a/include/drm/drm_gem_cma_helper.h
+++ b/include/drm/drm_gem_cma_helper.h
@@ -20,7 +20,9 @@ struct drm_gem_cma_object {
struct sg_table *sgt;
/* For objects with DMA memory allocated by GEM CMA */
+ void *cookie;
void *vaddr;
+ unsigned long dma_attrs;
};
#define to_drm_gem_cma_obj(gem_obj) \
@@ -73,6 +75,11 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
struct drm_device *drm,
struct drm_mode_create_dumb *args);
+/* create memory region for DRM framebuffer */
+int drm_gem_cma_dumb_create_no_kmap(struct drm_file *file_priv,
+ struct drm_device *drm,
+ struct drm_mode_create_dumb *args);
+
/* set vm_flags and we can change the VM attribute to other one at here */
int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma);
--
1.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 3/3] drm/mediatek: Use drm_gem_cma_object instead of mtk_drm_gem_obj
2018-10-26 7:22 [PATCH 0/3] Mediatek drm driver use drm_gem_cma_object instead of mtk_drm_gem_obj CK Hu
2018-10-26 7:22 ` [PATCH 1/3] drm: Add dma_dev in struct drm_device CK Hu
2018-10-26 7:22 ` [PATCH 2/3] drm: Add drm_gem_cma_dumb_create_no_kmap() helper function CK Hu
@ 2018-10-26 7:22 ` CK Hu
2018-10-26 10:21 ` Daniel Vetter
2 siblings, 1 reply; 9+ messages in thread
From: CK Hu @ 2018-10-26 7:22 UTC (permalink / raw)
To: Daniel Vetter, David Airlie, Gustavo Padovan, Maarten Lankhorst,
Sean Paul, CK Hu, Philipp Zabel
Cc: Matthias Brugger, linux-kernel, dri-devel, linux-arm-kernel,
linux-mediatek, srv_heupstream
After adding dma_dev in struct drm_device and
drm_gem_cma_dumb_create_no_kmap(), drm_gem_cma_object could replace
mtk_drm_gem_obj, so use drm_gem_cma_object instead of mtk_drm_gem_obj to
reduce redundant code.
Signed-off-by: CK Hu <ck.hu@mediatek.com>
---
drivers/gpu/drm/mediatek/Makefile | 1 -
drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 -
drivers/gpu/drm/mediatek/mtk_drm_drv.c | 15 +-
drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 -
drivers/gpu/drm/mediatek/mtk_drm_fb.c | 1 -
drivers/gpu/drm/mediatek/mtk_drm_gem.c | 243 -------------------------------
drivers/gpu/drm/mediatek/mtk_drm_gem.h | 56 -------
drivers/gpu/drm/mediatek/mtk_drm_plane.c | 8 +-
8 files changed, 11 insertions(+), 315 deletions(-)
delete mode 100644 drivers/gpu/drm/mediatek/mtk_drm_gem.c
delete mode 100644 drivers/gpu/drm/mediatek/mtk_drm_gem.h
diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile
index ce83c39..c4fa126 100644
--- a/drivers/gpu/drm/mediatek/Makefile
+++ b/drivers/gpu/drm/mediatek/Makefile
@@ -7,7 +7,6 @@ mediatek-drm-y := mtk_disp_color.o \
mtk_drm_ddp_comp.o \
mtk_drm_drv.o \
mtk_drm_fb.o \
- mtk_drm_gem.o \
mtk_drm_plane.o \
mtk_dsi.o \
mtk_mipi_tx.o \
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 92ecb9b..534c22a 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -24,7 +24,6 @@
#include "mtk_drm_crtc.h"
#include "mtk_drm_ddp.h"
#include "mtk_drm_ddp_comp.h"
-#include "mtk_drm_gem.h"
#include "mtk_drm_plane.h"
/**
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 47ec604..306ba29 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -30,7 +30,6 @@
#include "mtk_drm_ddp_comp.h"
#include "mtk_drm_drv.h"
#include "mtk_drm_fb.h"
-#include "mtk_drm_gem.h"
#define DRIVER_NAME "mediatek"
#define DRIVER_DESC "Mediatek SoC DRM"
@@ -282,7 +281,7 @@ static int mtk_drm_kms_init(struct drm_device *drm)
goto err_component_unbind;
}
- private->dma_dev = &pdev->dev;
+ drm->dma_dev = &pdev->dev;
/*
* We don't use the drm_irq_install() helpers provided by the DRM
@@ -320,7 +319,7 @@ static void mtk_drm_kms_deinit(struct drm_device *drm)
.open = drm_open,
.release = drm_release,
.unlocked_ioctl = drm_ioctl,
- .mmap = mtk_drm_gem_mmap,
+ .mmap = drm_gem_cma_mmap,
.poll = drm_poll,
.read = drm_read,
.compat_ioctl = drm_compat_ioctl,
@@ -330,17 +329,17 @@ static void mtk_drm_kms_deinit(struct drm_device *drm)
.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME |
DRIVER_ATOMIC,
- .gem_free_object_unlocked = mtk_drm_gem_free_object,
+ .gem_free_object_unlocked = drm_gem_cma_free_object,
.gem_vm_ops = &drm_gem_cma_vm_ops,
- .dumb_create = mtk_drm_gem_dumb_create,
+ .dumb_create = drm_gem_cma_dumb_create_no_kmap,
.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
.gem_prime_export = drm_gem_prime_export,
.gem_prime_import = drm_gem_prime_import,
- .gem_prime_get_sg_table = mtk_gem_prime_get_sg_table,
- .gem_prime_import_sg_table = mtk_gem_prime_import_sg_table,
- .gem_prime_mmap = mtk_drm_gem_mmap_buf,
+ .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
+ .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
+ .gem_prime_mmap = drm_gem_cma_prime_mmap,
.fops = &mtk_drm_fops,
.name = DRIVER_NAME,
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
index ecc00ca..cbbe63b 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
@@ -41,7 +41,6 @@ struct mtk_mmsys_driver_data {
struct mtk_drm_private {
struct drm_device *drm;
- struct device *dma_dev;
unsigned int num_pipes;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.c b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
index be5f6f1..45c22aa 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_fb.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
@@ -21,7 +21,6 @@
#include "mtk_drm_drv.h"
#include "mtk_drm_fb.h"
-#include "mtk_drm_gem.h"
static const struct drm_framebuffer_funcs mtk_drm_fb_funcs = {
.create_handle = drm_gem_fb_create_handle,
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
deleted file mode 100644
index 259b7b0..0000000
--- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
+++ /dev/null
@@ -1,243 +0,0 @@
-/*
- * Copyright (c) 2015 MediaTek Inc.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- */
-
-#include <drm/drmP.h>
-#include <drm/drm_gem.h>
-#include <linux/dma-buf.h>
-
-#include "mtk_drm_drv.h"
-#include "mtk_drm_gem.h"
-
-static struct mtk_drm_gem_obj *mtk_drm_gem_init(struct drm_device *dev,
- unsigned long size)
-{
- struct mtk_drm_gem_obj *mtk_gem_obj;
- int ret;
-
- size = round_up(size, PAGE_SIZE);
-
- mtk_gem_obj = kzalloc(sizeof(*mtk_gem_obj), GFP_KERNEL);
- if (!mtk_gem_obj)
- return ERR_PTR(-ENOMEM);
-
- ret = drm_gem_object_init(dev, &mtk_gem_obj->base, size);
- if (ret < 0) {
- DRM_ERROR("failed to initialize gem object\n");
- kfree(mtk_gem_obj);
- return ERR_PTR(ret);
- }
-
- return mtk_gem_obj;
-}
-
-struct mtk_drm_gem_obj *mtk_drm_gem_create(struct drm_device *dev,
- size_t size, bool alloc_kmap)
-{
- struct mtk_drm_private *priv = dev->dev_private;
- struct mtk_drm_gem_obj *mtk_gem;
- struct drm_gem_object *obj;
- int ret;
-
- mtk_gem = mtk_drm_gem_init(dev, size);
- if (IS_ERR(mtk_gem))
- return ERR_CAST(mtk_gem);
-
- obj = &mtk_gem->base;
-
- mtk_gem->dma_attrs = DMA_ATTR_WRITE_COMBINE;
-
- if (!alloc_kmap)
- mtk_gem->dma_attrs |= DMA_ATTR_NO_KERNEL_MAPPING;
-
- mtk_gem->cookie = dma_alloc_attrs(priv->dma_dev, obj->size,
- &mtk_gem->dma_addr, GFP_KERNEL,
- mtk_gem->dma_attrs);
- if (!mtk_gem->cookie) {
- DRM_ERROR("failed to allocate %zx byte dma buffer", obj->size);
- ret = -ENOMEM;
- goto err_gem_free;
- }
-
- if (alloc_kmap)
- mtk_gem->kvaddr = mtk_gem->cookie;
-
- DRM_DEBUG_DRIVER("cookie = %p dma_addr = %pad size = %zu\n",
- mtk_gem->cookie, &mtk_gem->dma_addr,
- size);
-
- return mtk_gem;
-
-err_gem_free:
- drm_gem_object_release(obj);
- kfree(mtk_gem);
- return ERR_PTR(ret);
-}
-
-void mtk_drm_gem_free_object(struct drm_gem_object *obj)
-{
- struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj);
- struct mtk_drm_private *priv = obj->dev->dev_private;
-
- if (mtk_gem->sg)
- drm_prime_gem_destroy(obj, mtk_gem->sg);
- else
- dma_free_attrs(priv->dma_dev, obj->size, mtk_gem->cookie,
- mtk_gem->dma_addr, mtk_gem->dma_attrs);
-
- /* release file pointer to gem object. */
- drm_gem_object_release(obj);
-
- kfree(mtk_gem);
-}
-
-int mtk_drm_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev,
- struct drm_mode_create_dumb *args)
-{
- struct mtk_drm_gem_obj *mtk_gem;
- int ret;
-
- args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
- args->size = args->pitch * args->height;
-
- mtk_gem = mtk_drm_gem_create(dev, args->size, false);
- if (IS_ERR(mtk_gem))
- return PTR_ERR(mtk_gem);
-
- /*
- * allocate a id of idr table where the obj is registered
- * and handle has the id what user can see.
- */
- ret = drm_gem_handle_create(file_priv, &mtk_gem->base, &args->handle);
- if (ret)
- goto err_handle_create;
-
- /* drop reference from allocate - handle holds it now. */
- drm_gem_object_put_unlocked(&mtk_gem->base);
-
- return 0;
-
-err_handle_create:
- mtk_drm_gem_free_object(&mtk_gem->base);
- return ret;
-}
-
-static int mtk_drm_gem_object_mmap(struct drm_gem_object *obj,
- struct vm_area_struct *vma)
-
-{
- int ret;
- struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj);
- struct mtk_drm_private *priv = obj->dev->dev_private;
-
- /*
- * dma_alloc_attrs() allocated a struct page table for mtk_gem, so clear
- * VM_PFNMAP flag that was set by drm_gem_mmap_obj()/drm_gem_mmap().
- */
- vma->vm_flags &= ~VM_PFNMAP;
- vma->vm_pgoff = 0;
-
- ret = dma_mmap_attrs(priv->dma_dev, vma, mtk_gem->cookie,
- mtk_gem->dma_addr, obj->size, mtk_gem->dma_attrs);
- if (ret)
- drm_gem_vm_close(vma);
-
- return ret;
-}
-
-int mtk_drm_gem_mmap_buf(struct drm_gem_object *obj, struct vm_area_struct *vma)
-{
- int ret;
-
- ret = drm_gem_mmap_obj(obj, obj->size, vma);
- if (ret)
- return ret;
-
- return mtk_drm_gem_object_mmap(obj, vma);
-}
-
-int mtk_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
-{
- struct drm_gem_object *obj;
- int ret;
-
- ret = drm_gem_mmap(filp, vma);
- if (ret)
- return ret;
-
- obj = vma->vm_private_data;
-
- return mtk_drm_gem_object_mmap(obj, vma);
-}
-
-/*
- * Allocate a sg_table for this GEM object.
- * Note: Both the table's contents, and the sg_table itself must be freed by
- * the caller.
- * Returns a pointer to the newly allocated sg_table, or an ERR_PTR() error.
- */
-struct sg_table *mtk_gem_prime_get_sg_table(struct drm_gem_object *obj)
-{
- struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj);
- struct mtk_drm_private *priv = obj->dev->dev_private;
- struct sg_table *sgt;
- int ret;
-
- sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
- if (!sgt)
- return ERR_PTR(-ENOMEM);
-
- ret = dma_get_sgtable_attrs(priv->dma_dev, sgt, mtk_gem->cookie,
- mtk_gem->dma_addr, obj->size,
- mtk_gem->dma_attrs);
- if (ret) {
- DRM_ERROR("failed to allocate sgt, %d\n", ret);
- kfree(sgt);
- return ERR_PTR(ret);
- }
-
- return sgt;
-}
-
-struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device *dev,
- struct dma_buf_attachment *attach, struct sg_table *sg)
-{
- struct mtk_drm_gem_obj *mtk_gem;
- int ret;
- struct scatterlist *s;
- unsigned int i;
- dma_addr_t expected;
-
- mtk_gem = mtk_drm_gem_init(dev, attach->dmabuf->size);
-
- if (IS_ERR(mtk_gem))
- return ERR_CAST(mtk_gem);
-
- expected = sg_dma_address(sg->sgl);
- for_each_sg(sg->sgl, s, sg->nents, i) {
- if (sg_dma_address(s) != expected) {
- DRM_ERROR("sg_table is not contiguous");
- ret = -EINVAL;
- goto err_gem_free;
- }
- expected = sg_dma_address(s) + sg_dma_len(s);
- }
-
- mtk_gem->dma_addr = sg_dma_address(sg->sgl);
- mtk_gem->sg = sg;
-
- return &mtk_gem->base;
-
-err_gem_free:
- kfree(mtk_gem);
- return ERR_PTR(ret);
-}
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.h b/drivers/gpu/drm/mediatek/mtk_drm_gem.h
deleted file mode 100644
index 534639b..0000000
--- a/drivers/gpu/drm/mediatek/mtk_drm_gem.h
+++ /dev/null
@@ -1,56 +0,0 @@
-/*
- * Copyright (c) 2015 MediaTek Inc.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- */
-
-#ifndef _MTK_DRM_GEM_H_
-#define _MTK_DRM_GEM_H_
-
-#include <drm/drm_gem.h>
-
-/*
- * mtk drm buffer structure.
- *
- * @base: a gem object.
- * - a new handle to this gem object would be created
- * by drm_gem_handle_create().
- * @cookie: the return value of dma_alloc_attrs(), keep it for dma_free_attrs()
- * @kvaddr: kernel virtual address of gem buffer.
- * @dma_addr: dma address of gem buffer.
- * @dma_attrs: dma attributes of gem buffer.
- *
- * P.S. this object would be transferred to user as kms_bo.handle so
- * user can access the buffer through kms_bo.handle.
- */
-struct mtk_drm_gem_obj {
- struct drm_gem_object base;
- void *cookie;
- void *kvaddr;
- dma_addr_t dma_addr;
- unsigned long dma_attrs;
- struct sg_table *sg;
-};
-
-#define to_mtk_gem_obj(x) container_of(x, struct mtk_drm_gem_obj, base)
-
-void mtk_drm_gem_free_object(struct drm_gem_object *gem);
-struct mtk_drm_gem_obj *mtk_drm_gem_create(struct drm_device *dev, size_t size,
- bool alloc_kmap);
-int mtk_drm_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev,
- struct drm_mode_create_dumb *args);
-int mtk_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
-int mtk_drm_gem_mmap_buf(struct drm_gem_object *obj,
- struct vm_area_struct *vma);
-struct sg_table *mtk_gem_prime_get_sg_table(struct drm_gem_object *obj);
-struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device *dev,
- struct dma_buf_attachment *attach, struct sg_table *sg);
-
-#endif
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index f7e6aa1..62de9d5 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -15,13 +15,13 @@
#include <drm/drmP.h>
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
+#include <drm/drm_gem_cma_helper.h>
#include <drm/drm_plane_helper.h>
#include "mtk_drm_crtc.h"
#include "mtk_drm_ddp_comp.h"
#include "mtk_drm_drv.h"
#include "mtk_drm_fb.h"
-#include "mtk_drm_gem.h"
#include "mtk_drm_plane.h"
static const u32 formats[] = {
@@ -115,7 +115,7 @@ static void mtk_plane_atomic_update(struct drm_plane *plane,
struct drm_crtc *crtc = plane->state->crtc;
struct drm_framebuffer *fb = plane->state->fb;
struct drm_gem_object *gem;
- struct mtk_drm_gem_obj *mtk_gem;
+ struct drm_gem_cma_object *cma_obj;
unsigned int pitch, format;
dma_addr_t addr;
@@ -123,8 +123,8 @@ static void mtk_plane_atomic_update(struct drm_plane *plane,
return;
gem = fb->obj[0];
- mtk_gem = to_mtk_gem_obj(gem);
- addr = mtk_gem->dma_addr;
+ cma_obj = to_drm_gem_cma_obj(gem);
+ addr = cma_obj->paddr;
pitch = fb->pitches[0];
format = fb->format->format;
--
1.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] drm/mediatek: Use drm_gem_cma_object instead of mtk_drm_gem_obj
2018-10-26 7:22 ` [PATCH 3/3] drm/mediatek: Use drm_gem_cma_object instead of mtk_drm_gem_obj CK Hu
@ 2018-10-26 10:21 ` Daniel Vetter
2018-10-29 3:11 ` CK Hu
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2018-10-26 10:21 UTC (permalink / raw)
To: CK Hu
Cc: Daniel Vetter, David Airlie, Gustavo Padovan, Maarten Lankhorst,
Sean Paul, Philipp Zabel, Matthias Brugger, linux-kernel,
dri-devel, linux-arm-kernel, linux-mediatek, srv_heupstream
On Fri, Oct 26, 2018 at 03:22:03PM +0800, CK Hu wrote:
> After adding dma_dev in struct drm_device and
> drm_gem_cma_dumb_create_no_kmap(), drm_gem_cma_object could replace
> mtk_drm_gem_obj, so use drm_gem_cma_object instead of mtk_drm_gem_obj to
> reduce redundant code.
>
> Signed-off-by: CK Hu <ck.hu@mediatek.com>
A few questions/thoughts:
- Why do you need both drm_device->dev and drm_device->dma_dev? Can't you
just register the drm_device with the right struct device?
- You don't use drm_gem_prime_import_dev, so prime import isn't using the
right device either.
- exynos seems to have the same or at least similar issue, stronger case
for your patches if you can solve both.
- I'd start out with using struct drm_gem_cma_object in mtk (similar to
what vc4 does), and then reusing as much as possible of the existing
helpers. And then looking later on what's still left (like the support
for leaving out the virtual mapping).
-Daniel
> ---
> drivers/gpu/drm/mediatek/Makefile | 1 -
> drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 -
> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 15 +-
> drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 -
> drivers/gpu/drm/mediatek/mtk_drm_fb.c | 1 -
> drivers/gpu/drm/mediatek/mtk_drm_gem.c | 243 -------------------------------
> drivers/gpu/drm/mediatek/mtk_drm_gem.h | 56 -------
> drivers/gpu/drm/mediatek/mtk_drm_plane.c | 8 +-
> 8 files changed, 11 insertions(+), 315 deletions(-)
> delete mode 100644 drivers/gpu/drm/mediatek/mtk_drm_gem.c
> delete mode 100644 drivers/gpu/drm/mediatek/mtk_drm_gem.h
>
> diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile
> index ce83c39..c4fa126 100644
> --- a/drivers/gpu/drm/mediatek/Makefile
> +++ b/drivers/gpu/drm/mediatek/Makefile
> @@ -7,7 +7,6 @@ mediatek-drm-y := mtk_disp_color.o \
> mtk_drm_ddp_comp.o \
> mtk_drm_drv.o \
> mtk_drm_fb.o \
> - mtk_drm_gem.o \
> mtk_drm_plane.o \
> mtk_dsi.o \
> mtk_mipi_tx.o \
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 92ecb9b..534c22a 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -24,7 +24,6 @@
> #include "mtk_drm_crtc.h"
> #include "mtk_drm_ddp.h"
> #include "mtk_drm_ddp_comp.h"
> -#include "mtk_drm_gem.h"
> #include "mtk_drm_plane.h"
>
> /**
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 47ec604..306ba29 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -30,7 +30,6 @@
> #include "mtk_drm_ddp_comp.h"
> #include "mtk_drm_drv.h"
> #include "mtk_drm_fb.h"
> -#include "mtk_drm_gem.h"
>
> #define DRIVER_NAME "mediatek"
> #define DRIVER_DESC "Mediatek SoC DRM"
> @@ -282,7 +281,7 @@ static int mtk_drm_kms_init(struct drm_device *drm)
> goto err_component_unbind;
> }
>
> - private->dma_dev = &pdev->dev;
> + drm->dma_dev = &pdev->dev;
>
> /*
> * We don't use the drm_irq_install() helpers provided by the DRM
> @@ -320,7 +319,7 @@ static void mtk_drm_kms_deinit(struct drm_device *drm)
> .open = drm_open,
> .release = drm_release,
> .unlocked_ioctl = drm_ioctl,
> - .mmap = mtk_drm_gem_mmap,
> + .mmap = drm_gem_cma_mmap,
> .poll = drm_poll,
> .read = drm_read,
> .compat_ioctl = drm_compat_ioctl,
> @@ -330,17 +329,17 @@ static void mtk_drm_kms_deinit(struct drm_device *drm)
> .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME |
> DRIVER_ATOMIC,
>
> - .gem_free_object_unlocked = mtk_drm_gem_free_object,
> + .gem_free_object_unlocked = drm_gem_cma_free_object,
> .gem_vm_ops = &drm_gem_cma_vm_ops,
> - .dumb_create = mtk_drm_gem_dumb_create,
> + .dumb_create = drm_gem_cma_dumb_create_no_kmap,
>
> .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> .gem_prime_export = drm_gem_prime_export,
> .gem_prime_import = drm_gem_prime_import,
> - .gem_prime_get_sg_table = mtk_gem_prime_get_sg_table,
> - .gem_prime_import_sg_table = mtk_gem_prime_import_sg_table,
> - .gem_prime_mmap = mtk_drm_gem_mmap_buf,
> + .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
> + .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
> + .gem_prime_mmap = drm_gem_cma_prime_mmap,
> .fops = &mtk_drm_fops,
>
> .name = DRIVER_NAME,
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> index ecc00ca..cbbe63b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> @@ -41,7 +41,6 @@ struct mtk_mmsys_driver_data {
>
> struct mtk_drm_private {
> struct drm_device *drm;
> - struct device *dma_dev;
>
> unsigned int num_pipes;
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.c b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
> index be5f6f1..45c22aa 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_fb.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
> @@ -21,7 +21,6 @@
>
> #include "mtk_drm_drv.h"
> #include "mtk_drm_fb.h"
> -#include "mtk_drm_gem.h"
>
> static const struct drm_framebuffer_funcs mtk_drm_fb_funcs = {
> .create_handle = drm_gem_fb_create_handle,
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> deleted file mode 100644
> index 259b7b0..0000000
> --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> +++ /dev/null
> @@ -1,243 +0,0 @@
> -/*
> - * Copyright (c) 2015 MediaTek Inc.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> - * GNU General Public License for more details.
> - */
> -
> -#include <drm/drmP.h>
> -#include <drm/drm_gem.h>
> -#include <linux/dma-buf.h>
> -
> -#include "mtk_drm_drv.h"
> -#include "mtk_drm_gem.h"
> -
> -static struct mtk_drm_gem_obj *mtk_drm_gem_init(struct drm_device *dev,
> - unsigned long size)
> -{
> - struct mtk_drm_gem_obj *mtk_gem_obj;
> - int ret;
> -
> - size = round_up(size, PAGE_SIZE);
> -
> - mtk_gem_obj = kzalloc(sizeof(*mtk_gem_obj), GFP_KERNEL);
> - if (!mtk_gem_obj)
> - return ERR_PTR(-ENOMEM);
> -
> - ret = drm_gem_object_init(dev, &mtk_gem_obj->base, size);
> - if (ret < 0) {
> - DRM_ERROR("failed to initialize gem object\n");
> - kfree(mtk_gem_obj);
> - return ERR_PTR(ret);
> - }
> -
> - return mtk_gem_obj;
> -}
> -
> -struct mtk_drm_gem_obj *mtk_drm_gem_create(struct drm_device *dev,
> - size_t size, bool alloc_kmap)
> -{
> - struct mtk_drm_private *priv = dev->dev_private;
> - struct mtk_drm_gem_obj *mtk_gem;
> - struct drm_gem_object *obj;
> - int ret;
> -
> - mtk_gem = mtk_drm_gem_init(dev, size);
> - if (IS_ERR(mtk_gem))
> - return ERR_CAST(mtk_gem);
> -
> - obj = &mtk_gem->base;
> -
> - mtk_gem->dma_attrs = DMA_ATTR_WRITE_COMBINE;
> -
> - if (!alloc_kmap)
> - mtk_gem->dma_attrs |= DMA_ATTR_NO_KERNEL_MAPPING;
> -
> - mtk_gem->cookie = dma_alloc_attrs(priv->dma_dev, obj->size,
> - &mtk_gem->dma_addr, GFP_KERNEL,
> - mtk_gem->dma_attrs);
> - if (!mtk_gem->cookie) {
> - DRM_ERROR("failed to allocate %zx byte dma buffer", obj->size);
> - ret = -ENOMEM;
> - goto err_gem_free;
> - }
> -
> - if (alloc_kmap)
> - mtk_gem->kvaddr = mtk_gem->cookie;
> -
> - DRM_DEBUG_DRIVER("cookie = %p dma_addr = %pad size = %zu\n",
> - mtk_gem->cookie, &mtk_gem->dma_addr,
> - size);
> -
> - return mtk_gem;
> -
> -err_gem_free:
> - drm_gem_object_release(obj);
> - kfree(mtk_gem);
> - return ERR_PTR(ret);
> -}
> -
> -void mtk_drm_gem_free_object(struct drm_gem_object *obj)
> -{
> - struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj);
> - struct mtk_drm_private *priv = obj->dev->dev_private;
> -
> - if (mtk_gem->sg)
> - drm_prime_gem_destroy(obj, mtk_gem->sg);
> - else
> - dma_free_attrs(priv->dma_dev, obj->size, mtk_gem->cookie,
> - mtk_gem->dma_addr, mtk_gem->dma_attrs);
> -
> - /* release file pointer to gem object. */
> - drm_gem_object_release(obj);
> -
> - kfree(mtk_gem);
> -}
> -
> -int mtk_drm_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev,
> - struct drm_mode_create_dumb *args)
> -{
> - struct mtk_drm_gem_obj *mtk_gem;
> - int ret;
> -
> - args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> - args->size = args->pitch * args->height;
> -
> - mtk_gem = mtk_drm_gem_create(dev, args->size, false);
> - if (IS_ERR(mtk_gem))
> - return PTR_ERR(mtk_gem);
> -
> - /*
> - * allocate a id of idr table where the obj is registered
> - * and handle has the id what user can see.
> - */
> - ret = drm_gem_handle_create(file_priv, &mtk_gem->base, &args->handle);
> - if (ret)
> - goto err_handle_create;
> -
> - /* drop reference from allocate - handle holds it now. */
> - drm_gem_object_put_unlocked(&mtk_gem->base);
> -
> - return 0;
> -
> -err_handle_create:
> - mtk_drm_gem_free_object(&mtk_gem->base);
> - return ret;
> -}
> -
> -static int mtk_drm_gem_object_mmap(struct drm_gem_object *obj,
> - struct vm_area_struct *vma)
> -
> -{
> - int ret;
> - struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj);
> - struct mtk_drm_private *priv = obj->dev->dev_private;
> -
> - /*
> - * dma_alloc_attrs() allocated a struct page table for mtk_gem, so clear
> - * VM_PFNMAP flag that was set by drm_gem_mmap_obj()/drm_gem_mmap().
> - */
> - vma->vm_flags &= ~VM_PFNMAP;
> - vma->vm_pgoff = 0;
> -
> - ret = dma_mmap_attrs(priv->dma_dev, vma, mtk_gem->cookie,
> - mtk_gem->dma_addr, obj->size, mtk_gem->dma_attrs);
> - if (ret)
> - drm_gem_vm_close(vma);
> -
> - return ret;
> -}
> -
> -int mtk_drm_gem_mmap_buf(struct drm_gem_object *obj, struct vm_area_struct *vma)
> -{
> - int ret;
> -
> - ret = drm_gem_mmap_obj(obj, obj->size, vma);
> - if (ret)
> - return ret;
> -
> - return mtk_drm_gem_object_mmap(obj, vma);
> -}
> -
> -int mtk_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> -{
> - struct drm_gem_object *obj;
> - int ret;
> -
> - ret = drm_gem_mmap(filp, vma);
> - if (ret)
> - return ret;
> -
> - obj = vma->vm_private_data;
> -
> - return mtk_drm_gem_object_mmap(obj, vma);
> -}
> -
> -/*
> - * Allocate a sg_table for this GEM object.
> - * Note: Both the table's contents, and the sg_table itself must be freed by
> - * the caller.
> - * Returns a pointer to the newly allocated sg_table, or an ERR_PTR() error.
> - */
> -struct sg_table *mtk_gem_prime_get_sg_table(struct drm_gem_object *obj)
> -{
> - struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj);
> - struct mtk_drm_private *priv = obj->dev->dev_private;
> - struct sg_table *sgt;
> - int ret;
> -
> - sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> - if (!sgt)
> - return ERR_PTR(-ENOMEM);
> -
> - ret = dma_get_sgtable_attrs(priv->dma_dev, sgt, mtk_gem->cookie,
> - mtk_gem->dma_addr, obj->size,
> - mtk_gem->dma_attrs);
> - if (ret) {
> - DRM_ERROR("failed to allocate sgt, %d\n", ret);
> - kfree(sgt);
> - return ERR_PTR(ret);
> - }
> -
> - return sgt;
> -}
> -
> -struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device *dev,
> - struct dma_buf_attachment *attach, struct sg_table *sg)
> -{
> - struct mtk_drm_gem_obj *mtk_gem;
> - int ret;
> - struct scatterlist *s;
> - unsigned int i;
> - dma_addr_t expected;
> -
> - mtk_gem = mtk_drm_gem_init(dev, attach->dmabuf->size);
> -
> - if (IS_ERR(mtk_gem))
> - return ERR_CAST(mtk_gem);
> -
> - expected = sg_dma_address(sg->sgl);
> - for_each_sg(sg->sgl, s, sg->nents, i) {
> - if (sg_dma_address(s) != expected) {
> - DRM_ERROR("sg_table is not contiguous");
> - ret = -EINVAL;
> - goto err_gem_free;
> - }
> - expected = sg_dma_address(s) + sg_dma_len(s);
> - }
> -
> - mtk_gem->dma_addr = sg_dma_address(sg->sgl);
> - mtk_gem->sg = sg;
> -
> - return &mtk_gem->base;
> -
> -err_gem_free:
> - kfree(mtk_gem);
> - return ERR_PTR(ret);
> -}
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.h b/drivers/gpu/drm/mediatek/mtk_drm_gem.h
> deleted file mode 100644
> index 534639b..0000000
> --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.h
> +++ /dev/null
> @@ -1,56 +0,0 @@
> -/*
> - * Copyright (c) 2015 MediaTek Inc.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> - * GNU General Public License for more details.
> - */
> -
> -#ifndef _MTK_DRM_GEM_H_
> -#define _MTK_DRM_GEM_H_
> -
> -#include <drm/drm_gem.h>
> -
> -/*
> - * mtk drm buffer structure.
> - *
> - * @base: a gem object.
> - * - a new handle to this gem object would be created
> - * by drm_gem_handle_create().
> - * @cookie: the return value of dma_alloc_attrs(), keep it for dma_free_attrs()
> - * @kvaddr: kernel virtual address of gem buffer.
> - * @dma_addr: dma address of gem buffer.
> - * @dma_attrs: dma attributes of gem buffer.
> - *
> - * P.S. this object would be transferred to user as kms_bo.handle so
> - * user can access the buffer through kms_bo.handle.
> - */
> -struct mtk_drm_gem_obj {
> - struct drm_gem_object base;
> - void *cookie;
> - void *kvaddr;
> - dma_addr_t dma_addr;
> - unsigned long dma_attrs;
> - struct sg_table *sg;
> -};
> -
> -#define to_mtk_gem_obj(x) container_of(x, struct mtk_drm_gem_obj, base)
> -
> -void mtk_drm_gem_free_object(struct drm_gem_object *gem);
> -struct mtk_drm_gem_obj *mtk_drm_gem_create(struct drm_device *dev, size_t size,
> - bool alloc_kmap);
> -int mtk_drm_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev,
> - struct drm_mode_create_dumb *args);
> -int mtk_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
> -int mtk_drm_gem_mmap_buf(struct drm_gem_object *obj,
> - struct vm_area_struct *vma);
> -struct sg_table *mtk_gem_prime_get_sg_table(struct drm_gem_object *obj);
> -struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device *dev,
> - struct dma_buf_attachment *attach, struct sg_table *sg);
> -
> -#endif
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index f7e6aa1..62de9d5 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -15,13 +15,13 @@
> #include <drm/drmP.h>
> #include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> #include <drm/drm_plane_helper.h>
>
> #include "mtk_drm_crtc.h"
> #include "mtk_drm_ddp_comp.h"
> #include "mtk_drm_drv.h"
> #include "mtk_drm_fb.h"
> -#include "mtk_drm_gem.h"
> #include "mtk_drm_plane.h"
>
> static const u32 formats[] = {
> @@ -115,7 +115,7 @@ static void mtk_plane_atomic_update(struct drm_plane *plane,
> struct drm_crtc *crtc = plane->state->crtc;
> struct drm_framebuffer *fb = plane->state->fb;
> struct drm_gem_object *gem;
> - struct mtk_drm_gem_obj *mtk_gem;
> + struct drm_gem_cma_object *cma_obj;
> unsigned int pitch, format;
> dma_addr_t addr;
>
> @@ -123,8 +123,8 @@ static void mtk_plane_atomic_update(struct drm_plane *plane,
> return;
>
> gem = fb->obj[0];
> - mtk_gem = to_mtk_gem_obj(gem);
> - addr = mtk_gem->dma_addr;
> + cma_obj = to_drm_gem_cma_obj(gem);
> + addr = cma_obj->paddr;
> pitch = fb->pitches[0];
> format = fb->format->format;
>
> --
> 1.9.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] drm/mediatek: Use drm_gem_cma_object instead of mtk_drm_gem_obj
2018-10-26 10:21 ` Daniel Vetter
@ 2018-10-29 3:11 ` CK Hu
2018-10-29 9:16 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: CK Hu @ 2018-10-29 3:11 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, David Airlie, Gustavo Padovan, Maarten Lankhorst,
Sean Paul, Philipp Zabel, Matthias Brugger, linux-kernel,
dri-devel, linux-arm-kernel, linux-mediatek, srv_heupstream
Hi,Daniel:
On Fri, 2018-10-26 at 12:21 +0200, Daniel Vetter wrote:
> On Fri, Oct 26, 2018 at 03:22:03PM +0800, CK Hu wrote:
> > After adding dma_dev in struct drm_device and
> > drm_gem_cma_dumb_create_no_kmap(), drm_gem_cma_object could replace
> > mtk_drm_gem_obj, so use drm_gem_cma_object instead of mtk_drm_gem_obj to
> > reduce redundant code.
> >
> > Signed-off-by: CK Hu <ck.hu@mediatek.com>
>
> A few questions/thoughts:
>
> - Why do you need both drm_device->dev and drm_device->dma_dev? Can't you
> just register the drm_device with the right struct device?
>
In [1], mmsys is the drm driver and ovl0 and ovl1 is the sub device
which has dma function.
In this drm, there are two crtc and each one is comprised of many
component.
This is an example of mt8173:
crtc0: ovl0, color0, aal, od, rdma0, ufoe, dsi0
crtc1: ovl1, color1, gamma, rdma1, dpi0
In the device node of ovl0 and ovl1, there is a 'iommus' parameter in
it, so use dma_alloc_xxx() and dma_map_xxx() with that device would get
iova rather than pa. I don't think it's a good idea to register ovl0 or
ovl1 as drm device because each one is just a component in a pipeline.
mmsys controls the clock and routing of multi-media system which include
this drm system, so it's better to register mmsys as drm device. Maybe
we could move 'iommus' parameter from ovl device to mmsys device, so the
dma device changes from ovl device to mmsys device. I'm not sure this
would be a good choice, how do you think?
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v4.19
> - You don't use drm_gem_prime_import_dev, so prime import isn't using the
> right device either.
Yes, you are right. I'm not familiar with whore drm core, so I start to
modify what Mediatek drm use. But this function still works for the drm
device that itself is dma device. If one day there is a drm device which
itself is not a dma device and need this function, send a patch to
modify this function and test it with that drm device. If you want me to
modify all in advance, I'm ok but need others to test it because
Mediatek drm driver does not use them.
>
> - exynos seems to have the same or at least similar issue, stronger case
> for your patches if you can solve both.
I'm still Mediatek's employee. If I modify other company's driver and it
is not a MUST-BE for Mediatek, Mediatek may think I give contribution to
other company. So I've better not to modify exynos driver.
> - I'd start out with using struct drm_gem_cma_object in mtk (similar to
> what vc4 does), and then reusing as much as possible of the existing
> helpers. And then looking later on what's still left (like the support
> for leaving out the virtual mapping).
I'm not clear what vc4 does. It looks like that you want me to redefine
mtk_drm_gem_obj based on drm_gem_cma_object. So it would be like
struct mtk_drm_gem_obj {
struct drm_gem_cma_object base;
void *cookie;
unsigned long dma_attrs;
};
I could try to modify as this and see what have left.
Regards,
CK
>
> -Daniel
>
[Snip...]
> > ---
> > --
> > 1.9.1
> >
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] drm/mediatek: Use drm_gem_cma_object instead of mtk_drm_gem_obj
2018-10-29 3:11 ` CK Hu
@ 2018-10-29 9:16 ` Daniel Vetter
2018-10-30 6:54 ` CK Hu
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2018-10-29 9:16 UTC (permalink / raw)
To: CK Hu
Cc: Daniel Vetter, Daniel Vetter, David Airlie, Gustavo Padovan,
Maarten Lankhorst, Sean Paul, Philipp Zabel, Matthias Brugger,
linux-kernel, dri-devel, linux-arm-kernel, linux-mediatek,
srv_heupstream
On Mon, Oct 29, 2018 at 11:11:16AM +0800, CK Hu wrote:
> Hi,Daniel:
>
> On Fri, 2018-10-26 at 12:21 +0200, Daniel Vetter wrote:
> > On Fri, Oct 26, 2018 at 03:22:03PM +0800, CK Hu wrote:
> > > After adding dma_dev in struct drm_device and
> > > drm_gem_cma_dumb_create_no_kmap(), drm_gem_cma_object could replace
> > > mtk_drm_gem_obj, so use drm_gem_cma_object instead of mtk_drm_gem_obj to
> > > reduce redundant code.
> > >
> > > Signed-off-by: CK Hu <ck.hu@mediatek.com>
> >
> > A few questions/thoughts:
> >
> > - Why do you need both drm_device->dev and drm_device->dma_dev? Can't you
> > just register the drm_device with the right struct device?
> >
>
> In [1], mmsys is the drm driver and ovl0 and ovl1 is the sub device
> which has dma function.
> In this drm, there are two crtc and each one is comprised of many
> component.
> This is an example of mt8173:
>
> crtc0: ovl0, color0, aal, od, rdma0, ufoe, dsi0
> crtc1: ovl1, color1, gamma, rdma1, dpi0
>
> In the device node of ovl0 and ovl1, there is a 'iommus' parameter in
> it, so use dma_alloc_xxx() and dma_map_xxx() with that device would get
> iova rather than pa. I don't think it's a good idea to register ovl0 or
> ovl1 as drm device because each one is just a component in a pipeline.
> mmsys controls the clock and routing of multi-media system which include
> this drm system, so it's better to register mmsys as drm device. Maybe
> we could move 'iommus' parameter from ovl device to mmsys device, so the
> dma device changes from ovl device to mmsys device. I'm not sure this
> would be a good choice, how do you think?
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v4.19
Ah ok. But if you have 2 blocks that make up the overall drm device, why
don't you need to switch at runtime between them? I.e. buffer allocated
for crtc0 needs to be dma-mapped to crtc0, buffer allocated to crtc1 needs
to be dma-mapped on crtc1?
And if they're both the exact same iommu, then imo it would make indeed
sense to move the iommu attribute up. Since your current code cant'
actually handle truly separate dma-mappings. And neither can your patch
series here handled separate iommu for crtc0 and crtc1.
> > - You don't use drm_gem_prime_import_dev, so prime import isn't using the
> > right device either.
>
> Yes, you are right. I'm not familiar with whore drm core, so I start to
> modify what Mediatek drm use. But this function still works for the drm
> device that itself is dma device. If one day there is a drm device which
> itself is not a dma device and need this function, send a patch to
> modify this function and test it with that drm device. If you want me to
> modify all in advance, I'm ok but need others to test it because
> Mediatek drm driver does not use them.
I meant to say that mediatek should use drm_gem_prime_import_dev, but
currently isn't using that. And your patch series here doesn't fix that
either. So there's more bugs left in this area.
> > - exynos seems to have the same or at least similar issue, stronger case
> > for your patches if you can solve both.
>
> I'm still Mediatek's employee. If I modify other company's driver and it
> is not a MUST-BE for Mediatek, Mediatek may think I give contribution to
> other company. So I've better not to modify exynos driver.
This isn't how upstream works :-)
> > - I'd start out with using struct drm_gem_cma_object in mtk (similar to
> > what vc4 does), and then reusing as much as possible of the existing
> > helpers. And then looking later on what's still left (like the support
> > for leaving out the virtual mapping).
>
> I'm not clear what vc4 does. It looks like that you want me to redefine
> mtk_drm_gem_obj based on drm_gem_cma_object. So it would be like
>
> struct mtk_drm_gem_obj {
> struct drm_gem_cma_object base;
> void *cookie;
> unsigned long dma_attrs;
> };
>
> I could try to modify as this and see what have left.
Yup, that's my suggestion. Then we can look at what mtk can use unchanged
from the core helpers. And what would need to change and so better
evaluate whether it makes sense to do that.
I still think just moving the iommu is probably best.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] drm/mediatek: Use drm_gem_cma_object instead of mtk_drm_gem_obj
2018-10-29 9:16 ` Daniel Vetter
@ 2018-10-30 6:54 ` CK Hu
2018-10-30 9:02 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: CK Hu @ 2018-10-30 6:54 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, David Airlie, Gustavo Padovan, Maarten Lankhorst,
Sean Paul, Philipp Zabel, Matthias Brugger, linux-kernel,
dri-devel, linux-arm-kernel, linux-mediatek, srv_heupstream
Hi, Daniel:
On Mon, 2018-10-29 at 10:16 +0100, Daniel Vetter wrote:
> On Mon, Oct 29, 2018 at 11:11:16AM +0800, CK Hu wrote:
> > Hi,Daniel:
> >
> > On Fri, 2018-10-26 at 12:21 +0200, Daniel Vetter wrote:
> > > On Fri, Oct 26, 2018 at 03:22:03PM +0800, CK Hu wrote:
> > > > After adding dma_dev in struct drm_device and
> > > > drm_gem_cma_dumb_create_no_kmap(), drm_gem_cma_object could replace
> > > > mtk_drm_gem_obj, so use drm_gem_cma_object instead of mtk_drm_gem_obj to
> > > > reduce redundant code.
> > > >
> > > > Signed-off-by: CK Hu <ck.hu@mediatek.com>
> > >
> > > A few questions/thoughts:
> > >
> > > - Why do you need both drm_device->dev and drm_device->dma_dev? Can't you
> > > just register the drm_device with the right struct device?
> > >
> >
> > In [1], mmsys is the drm driver and ovl0 and ovl1 is the sub device
> > which has dma function.
> > In this drm, there are two crtc and each one is comprised of many
> > component.
> > This is an example of mt8173:
> >
> > crtc0: ovl0, color0, aal, od, rdma0, ufoe, dsi0
> > crtc1: ovl1, color1, gamma, rdma1, dpi0
> >
> > In the device node of ovl0 and ovl1, there is a 'iommus' parameter in
> > it, so use dma_alloc_xxx() and dma_map_xxx() with that device would get
> > iova rather than pa. I don't think it's a good idea to register ovl0 or
> > ovl1 as drm device because each one is just a component in a pipeline.
> > mmsys controls the clock and routing of multi-media system which include
> > this drm system, so it's better to register mmsys as drm device. Maybe
> > we could move 'iommus' parameter from ovl device to mmsys device, so the
> > dma device changes from ovl device to mmsys device. I'm not sure this
> > would be a good choice, how do you think?
> >
> > [1]
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v4.19
>
> Ah ok. But if you have 2 blocks that make up the overall drm device, why
> don't you need to switch at runtime between them? I.e. buffer allocated
> for crtc0 needs to be dma-mapped to crtc0, buffer allocated to crtc1 needs
> to be dma-mapped on crtc1?
>
> And if they're both the exact same iommu, then imo it would make indeed
> sense to move the iommu attribute up. Since your current code cant'
> actually handle truly separate dma-mappings. And neither can your patch
> series here handled separate iommu for crtc0 and crtc1.
Yes, they're the exact same iommu. So I would move iommu attribute up.
>
> > > - You don't use drm_gem_prime_import_dev, so prime import isn't using the
> > > right device either.
> >
> > Yes, you are right. I'm not familiar with whore drm core, so I start to
> > modify what Mediatek drm use. But this function still works for the drm
> > device that itself is dma device. If one day there is a drm device which
> > itself is not a dma device and need this function, send a patch to
> > modify this function and test it with that drm device. If you want me to
> > modify all in advance, I'm ok but need others to test it because
> > Mediatek drm driver does not use them.
>
> I meant to say that mediatek should use drm_gem_prime_import_dev, but
> currently isn't using that. And your patch series here doesn't fix that
> either. So there's more bugs left in this area.
Great, you find a bug. My test only include export but not import. This
would take time to generate import-test environment.
>
> > > - exynos seems to have the same or at least similar issue, stronger case
> > > for your patches if you can solve both.
> >
> > I'm still Mediatek's employee. If I modify other company's driver and it
> > is not a MUST-BE for Mediatek, Mediatek may think I give contribution to
> > other company. So I've better not to modify exynos driver.
>
> This isn't how upstream works :-)
OK, because now I would not modify drm core, I would focus on Mediatek
drm driver first. If the modification of exynos driver is easy, I could
try. But if the modification of exynos is huge, I suggest that someone
who is familiar with exynos driver and have exynos platform to do it.
Regards,
CK
>
> > > - I'd start out with using struct drm_gem_cma_object in mtk (similar to
> > > what vc4 does), and then reusing as much as possible of the existing
> > > helpers. And then looking later on what's still left (like the support
> > > for leaving out the virtual mapping).
> >
> > I'm not clear what vc4 does. It looks like that you want me to redefine
> > mtk_drm_gem_obj based on drm_gem_cma_object. So it would be like
> >
> > struct mtk_drm_gem_obj {
> > struct drm_gem_cma_object base;
> > void *cookie;
> > unsigned long dma_attrs;
> > };
> >
> > I could try to modify as this and see what have left.
>
> Yup, that's my suggestion. Then we can look at what mtk can use unchanged
> from the core helpers. And what would need to change and so better
> evaluate whether it makes sense to do that.
>
> I still think just moving the iommu is probably best.
> -Daniel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] drm/mediatek: Use drm_gem_cma_object instead of mtk_drm_gem_obj
2018-10-30 6:54 ` CK Hu
@ 2018-10-30 9:02 ` Daniel Vetter
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2018-10-30 9:02 UTC (permalink / raw)
To: CK Hu
Cc: Daniel Vetter, Daniel Vetter, David Airlie, Gustavo Padovan,
Maarten Lankhorst, Sean Paul, Philipp Zabel, Matthias Brugger,
linux-kernel, dri-devel, linux-arm-kernel, linux-mediatek,
srv_heupstream
On Tue, Oct 30, 2018 at 02:54:31PM +0800, CK Hu wrote:
> Hi, Daniel:
>
> On Mon, 2018-10-29 at 10:16 +0100, Daniel Vetter wrote:
> > On Mon, Oct 29, 2018 at 11:11:16AM +0800, CK Hu wrote:
> > > Hi,Daniel:
> > >
> > > On Fri, 2018-10-26 at 12:21 +0200, Daniel Vetter wrote:
> > > > On Fri, Oct 26, 2018 at 03:22:03PM +0800, CK Hu wrote:
> > > > > After adding dma_dev in struct drm_device and
> > > > > drm_gem_cma_dumb_create_no_kmap(), drm_gem_cma_object could replace
> > > > > mtk_drm_gem_obj, so use drm_gem_cma_object instead of mtk_drm_gem_obj to
> > > > > reduce redundant code.
> > > > >
> > > > > Signed-off-by: CK Hu <ck.hu@mediatek.com>
> > > >
> > > > A few questions/thoughts:
> > > >
> > > > - Why do you need both drm_device->dev and drm_device->dma_dev? Can't you
> > > > just register the drm_device with the right struct device?
> > > >
> > >
> > > In [1], mmsys is the drm driver and ovl0 and ovl1 is the sub device
> > > which has dma function.
> > > In this drm, there are two crtc and each one is comprised of many
> > > component.
> > > This is an example of mt8173:
> > >
> > > crtc0: ovl0, color0, aal, od, rdma0, ufoe, dsi0
> > > crtc1: ovl1, color1, gamma, rdma1, dpi0
> > >
> > > In the device node of ovl0 and ovl1, there is a 'iommus' parameter in
> > > it, so use dma_alloc_xxx() and dma_map_xxx() with that device would get
> > > iova rather than pa. I don't think it's a good idea to register ovl0 or
> > > ovl1 as drm device because each one is just a component in a pipeline.
> > > mmsys controls the clock and routing of multi-media system which include
> > > this drm system, so it's better to register mmsys as drm device. Maybe
> > > we could move 'iommus' parameter from ovl device to mmsys device, so the
> > > dma device changes from ovl device to mmsys device. I'm not sure this
> > > would be a good choice, how do you think?
> > >
> > > [1]
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v4.19
> >
> > Ah ok. But if you have 2 blocks that make up the overall drm device, why
> > don't you need to switch at runtime between them? I.e. buffer allocated
> > for crtc0 needs to be dma-mapped to crtc0, buffer allocated to crtc1 needs
> > to be dma-mapped on crtc1?
> >
> > And if they're both the exact same iommu, then imo it would make indeed
> > sense to move the iommu attribute up. Since your current code cant'
> > actually handle truly separate dma-mappings. And neither can your patch
> > series here handled separate iommu for crtc0 and crtc1.
>
> Yes, they're the exact same iommu. So I would move iommu attribute up.
>
> >
> > > > - You don't use drm_gem_prime_import_dev, so prime import isn't using the
> > > > right device either.
> > >
> > > Yes, you are right. I'm not familiar with whore drm core, so I start to
> > > modify what Mediatek drm use. But this function still works for the drm
> > > device that itself is dma device. If one day there is a drm device which
> > > itself is not a dma device and need this function, send a patch to
> > > modify this function and test it with that drm device. If you want me to
> > > modify all in advance, I'm ok but need others to test it because
> > > Mediatek drm driver does not use them.
> >
> > I meant to say that mediatek should use drm_gem_prime_import_dev, but
> > currently isn't using that. And your patch series here doesn't fix that
> > either. So there's more bugs left in this area.
>
> Great, you find a bug. My test only include export but not import. This
> would take time to generate import-test environment.
igt has a bunch of import tests for display iirc, using vgem.
> > > > - exynos seems to have the same or at least similar issue, stronger case
> > > > for your patches if you can solve both.
> > >
> > > I'm still Mediatek's employee. If I modify other company's driver and it
> > > is not a MUST-BE for Mediatek, Mediatek may think I give contribution to
> > > other company. So I've better not to modify exynos driver.
> >
> > This isn't how upstream works :-)
>
> OK, because now I would not modify drm core, I would focus on Mediatek
> drm driver first. If the modification of exynos driver is easy, I could
> try. But if the modification of exynos is huge, I suggest that someone
> who is familiar with exynos driver and have exynos platform to do it.
Since we now clarified that you only have 1 iommu (and that it's kinda
misplaced in the DT) I think exonys is a different case. This was more
about your original patch series, which looked like exynos could need too.
-Daniel
>
> Regards,
> CK
> >
> > > > - I'd start out with using struct drm_gem_cma_object in mtk (similar to
> > > > what vc4 does), and then reusing as much as possible of the existing
> > > > helpers. And then looking later on what's still left (like the support
> > > > for leaving out the virtual mapping).
> > >
> > > I'm not clear what vc4 does. It looks like that you want me to redefine
> > > mtk_drm_gem_obj based on drm_gem_cma_object. So it would be like
> > >
> > > struct mtk_drm_gem_obj {
> > > struct drm_gem_cma_object base;
> > > void *cookie;
> > > unsigned long dma_attrs;
> > > };
> > >
> > > I could try to modify as this and see what have left.
> >
> > Yup, that's my suggestion. Then we can look at what mtk can use unchanged
> > from the core helpers. And what would need to change and so better
> > evaluate whether it makes sense to do that.
> >
> > I still think just moving the iommu is probably best.
> > -Daniel
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 9+ messages in thread