[PATCH v2] DRM: add drm gem cma helper

[PATCH v2] DRM: add drm gem cma helper

[Sascha Hauer]

Many embedded drm devices do not have a IOMMU and no dedicated
memory for graphics. These devices use cma (Contiguous Memory
Allocator) backed graphics memory. This patch provides helper
functions to be able to share the code.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---

changes since v1:

- map whole buffer at mmap time, not page by page at fault time
- several cleanups as suggested by Lars-Peter Clausen and Laurent Pinchart

I currently do a remap_pfn_range after drm_gem_mmap, so we release dev->struct_mutex
in between. I don't know if this is correct or if we have to hold the lock over
the whole process.

 drivers/gpu/drm/Kconfig              |    6 +
 drivers/gpu/drm/Makefile             |    1 +
 drivers/gpu/drm/drm_gem_cma_helper.c |  243 ++++++++++++++++++++++++++++++++++
 include/drm/drm_gem_cma_helper.h     |   52 ++++++++
 4 files changed, 302 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_gem_cma_helper.c
 create mode 100644 include/drm/drm_gem_cma_helper.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index e354bc0..f62717e 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -53,6 +53,12 @@ config DRM_TTM
 	  GPU memory types. Will be enabled automatically if a device driver
 	  uses it.
 
+config DRM_GEM_CMA_HELPER
+	tristate
+	depends on DRM
+	help
+	  Choose this if you need the GEM cma helper functions
+
 config DRM_TDFX
 	tristate "3dfx Banshee/Voodoo3+"
 	depends on DRM && PCI
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index c20da5b..9a0d98a 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -15,6 +15,7 @@ drm-y       :=	drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
 		drm_trace_points.o drm_global.o drm_prime.o
 
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
+drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
 
 drm-usb-y   := drm_usb.o
 
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
new file mode 100644
index 0000000..d8c0dc7
--- /dev/null
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -0,0 +1,243 @@
+/*
+ * drm gem cma (contiguous memory allocator) helper functions
+ *
+ * Copyright (C) 2012 Sascha Hauer, Pengutronix
+ *
+ * Based on Samsung Exynos code
+ *
+ * Copyright (c) 2011 Samsung Electronics Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * 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.h>
+#include <drm/drm_gem_cma_helper.h>
+
+static unsigned int get_gem_mmap_offset(struct drm_gem_object *obj)
+{
+	return (unsigned int)obj->map_list.hash.key << PAGE_SHIFT;
+}
+
+static void drm_gem_cma_buf_destroy(struct drm_device *drm,
+		struct drm_gem_cma_object *cma_obj)
+{
+	dma_free_writecombine(drm->dev, cma_obj->base.size, cma_obj->vaddr,
+			cma_obj->paddr);
+}
+
+/*
+ * drm_gem_cma_create - allocate an object with the given size
+ *
+ * returns a struct drm_gem_cma_object* on success or ERR_PTR values
+ * on failure.
+ */
+struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
+		unsigned int size)
+{
+	struct drm_gem_cma_object *cma_obj;
+	struct drm_gem_object *gem_obj;
+	int ret;
+
+	size = round_up(size, PAGE_SIZE);
+
+	cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL);
+	if (!cma_obj)
+		return ERR_PTR(-ENOMEM);
+
+	cma_obj->vaddr = dma_alloc_writecombine(drm->dev, size,
+			&cma_obj->paddr, GFP_KERNEL | __GFP_NOWARN);
+	if (!cma_obj->vaddr) {
+		dev_err(drm->dev, "failed to allocate buffer with size %d\n", size);
+		ret = -ENOMEM;
+		goto err_dma_alloc;
+	}
+
+	gem_obj = &cma_obj->base;
+
+	ret = drm_gem_object_init(drm, gem_obj, size);
+	if (ret)
+		goto err_obj_init;
+
+	ret = drm_gem_create_mmap_offset(gem_obj);
+	if (ret)
+		goto err_create_mmap_offset;
+
+	return cma_obj;
+
+err_create_mmap_offset:
+	drm_gem_object_release(gem_obj);
+
+err_obj_init:
+	drm_gem_cma_buf_destroy(drm, cma_obj);
+
+err_dma_alloc:
+	kfree(cma_obj);
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_create);
+
+/*
+ * drm_gem_cma_create_with_handle - allocate an object with the given
+ * size and create a gem handle on it
+ *
+ * returns a struct drm_gem_cma_object* on success or ERR_PTR values
+ * on failure.
+ */
+struct drm_gem_cma_object *drm_gem_cma_create_with_handle(
+		struct drm_file *file_priv,
+		struct drm_device *drm, unsigned int size,
+		unsigned int *handle)
+{
+	struct drm_gem_cma_object *cma_obj;
+	struct drm_gem_object *gem_obj;
+	int ret;
+
+	cma_obj = drm_gem_cma_create(drm, size);
+	if (IS_ERR(cma_obj))
+		return cma_obj;
+
+	gem_obj = &cma_obj->base;
+
+	/*
+	 * 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, gem_obj, handle);
+	if (ret)
+		goto err_handle_create;
+
+	/* drop reference from allocate - handle holds it now. */
+	drm_gem_object_unreference_unlocked(gem_obj);
+
+	return cma_obj;
+
+err_handle_create:
+	drm_gem_cma_free_object(gem_obj);
+
+	return ERR_PTR(ret);
+}
+
+/*
+ * drm_gem_cma_free_object - (struct drm_driver)->gem_free_object callback
+ * function
+ */
+void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
+{
+	struct drm_gem_cma_object *cma_obj;
+
+	if (gem_obj->map_list.map)
+		drm_gem_free_mmap_offset(gem_obj);
+
+	drm_gem_object_release(gem_obj);
+
+	cma_obj = to_drm_gem_cma_obj(gem_obj);
+
+	drm_gem_cma_buf_destroy(gem_obj->dev, cma_obj);
+
+	kfree(cma_obj);
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
+
+/*
+ * drm_gem_cma_dumb_create - (struct drm_driver)->dumb_create callback
+ * function
+ *
+ * This aligns the pitch and size arguments to the minimum required. wrap
+ * this into your own function if you need bigger alignment.
+ */
+int drm_gem_cma_dumb_create(struct drm_file *file_priv,
+		struct drm_device *dev, struct drm_mode_create_dumb *args)
+{
+	struct drm_gem_cma_object *cma_obj;
+
+	if (args->pitch < args->width * DIV_ROUND_UP(args->bpp, 8))
+		args->pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
+
+	if (args->size < args->pitch * args->height)
+		args->size = args->pitch * args->height;
+
+	cma_obj = drm_gem_cma_create_with_handle(file_priv, dev,
+			args->size, &args->handle);
+	if (IS_ERR(cma_obj))
+		return PTR_ERR(cma_obj);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create);
+
+/*
+ * drm_gem_cma_dumb_map_offset - (struct drm_driver)->dumb_map_offset callback
+ * function
+ */
+int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
+		struct drm_device *drm, uint32_t handle, uint64_t *offset)
+{
+	struct drm_gem_object *gem_obj;
+
+	mutex_lock(&drm->struct_mutex);
+
+	gem_obj = drm_gem_object_lookup(drm, file_priv, handle);
+	if (!gem_obj) {
+		dev_err(drm->dev, "failed to lookup gem object\n");
+		mutex_unlock(&drm->struct_mutex);
+		return -EINVAL;
+	}
+
+	*offset = get_gem_mmap_offset(gem_obj);
+
+	drm_gem_object_unreference(gem_obj);
+
+	mutex_unlock(&drm->struct_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_map_offset);
+
+struct vm_operations_struct drm_gem_cma_vm_ops = {
+	.open = drm_gem_vm_open,
+	.close = drm_gem_vm_close,
+};
+EXPORT_SYMBOL_GPL(drm_gem_cma_vm_ops);
+
+/*
+ * drm_gem_cma_mmap - (struct file_operation)->mmap callback function
+ */
+int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+	struct drm_gem_object *gem_obj;
+	struct drm_gem_cma_object *cma_obj;
+	int ret;
+
+	ret = drm_gem_mmap(filp, vma);
+	if (ret)
+		return ret;
+
+	gem_obj = vma->vm_private_data;
+	cma_obj = to_drm_gem_cma_obj(gem_obj);
+
+	ret = remap_pfn_range(vma, vma->vm_start, cma_obj->paddr >> PAGE_SHIFT,
+			vma->vm_end - vma->vm_start, vma->vm_page_prot);
+	if (ret)
+		drm_gem_vm_close(vma);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
+
+/*
+ * drm_gem_cma_dumb_destroy - (struct drm_driver)->dumb_destroy callback function
+ */
+int drm_gem_cma_dumb_destroy(struct drm_file *file_priv,
+		struct drm_device *drm, unsigned int handle)
+{
+	return drm_gem_handle_delete(file_priv, handle);
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_destroy);
diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
new file mode 100644
index 0000000..c4ed90b
--- /dev/null
+++ b/include/drm/drm_gem_cma_helper.h
@@ -0,0 +1,52 @@
+#ifndef __DRM_GEM_CMA_HELPER_H__
+#define __DRM_GEM_CMA_HELPER_H__
+
+struct drm_gem_cma_object {
+	struct drm_gem_object base;
+	dma_addr_t paddr;
+	void *vaddr;
+};
+
+static inline struct drm_gem_cma_object *
+to_drm_gem_cma_obj(struct drm_gem_object *gem_obj)
+{
+	return container_of(gem_obj, struct drm_gem_cma_object, base);
+}
+
+/* free gem object. */
+void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
+
+/* create memory region for drm framebuffer. */
+int drm_gem_cma_dumb_create(struct drm_file *file_priv,
+		struct drm_device *drm, struct drm_mode_create_dumb *args);
+
+/* map memory region for drm framebuffer to user space. */
+int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
+		struct drm_device *drm, uint32_t handle, uint64_t *offset);
+
+/* page fault handler and mmap fault address(virtual) to physical memory. */
+int drm_gem_cma_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 drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma);
+
+/*
+ * destroy memory region allocated.
+ *	- a gem handle and physical memory region pointed by a gem object
+ *	would be released by drm_gem_handle_delete().
+ */
+int drm_gem_cma_dumb_destroy(struct drm_file *file_priv,
+		struct drm_device *drm, unsigned int handle);
+
+/* allocate physical memory. */
+struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
+		unsigned int size);
+
+struct drm_gem_cma_object *drm_gem_cma_create_with_handle(
+		struct drm_file *file_priv,
+		struct drm_device *drm, unsigned int size,
+		unsigned int *handle);
+
+extern struct vm_operations_struct drm_gem_cma_vm_ops;
+
+#endif /* __DRM_GEM_CMA_HELPER_H__ */
-- 
1.7.10

Re: [PATCH v2] DRM: add drm gem cma helper

[Laurent Pinchart]

Hi Sascha,

Thanks for the patch. Here's a bit more extensive review.

On Thursday 31 May 2012 10:08:54 Sascha Hauer wrote:
> Many embedded drm devices do not have a IOMMU and no dedicated
> memory for graphics. These devices use cma (Contiguous Memory
> Allocator) backed graphics memory. This patch provides helper
> functions to be able to share the code.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> 
> changes since v1:
> 
> - map whole buffer at mmap time, not page by page at fault time
> - several cleanups as suggested by Lars-Peter Clausen and Laurent Pinchart
> 
> I currently do a remap_pfn_range after drm_gem_mmap, so we release
> dev->struct_mutex in between. I don't know if this is correct or if we have
> to hold the lock over the whole process.

drm_gem_mmap() takes a reference on the GEM object so I don't think we'll have 
any issue there, but please don't take my word for it. dev->struct_mutex is 
the large mutex I still need to document, I don't know what it protects 
exactly.

>  drivers/gpu/drm/Kconfig              |    6 +
>  drivers/gpu/drm/Makefile             |    1 +
>  drivers/gpu/drm/drm_gem_cma_helper.c |  243 +++++++++++++++++++++++++++++++
>  include/drm/drm_gem_cma_helper.h     |   52 ++++++++
>  4 files changed, 302 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_gem_cma_helper.c
>  create mode 100644 include/drm/drm_gem_cma_helper.h
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e354bc0..f62717e 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -53,6 +53,12 @@ config DRM_TTM
>  	  GPU memory types. Will be enabled automatically if a device driver
>  	  uses it.
> 
> +config DRM_GEM_CMA_HELPER
> +	tristate
> +	depends on DRM
> +	help
> +	  Choose this if you need the GEM cma helper functions

I would put CMA in uppercase, but that's just nitpicking.

BTW this helper is not strictly dedicated to CMA. It uses the DMA API to 
allocate memory, without caring about the underlying allocator.

> +
>  config DRM_TDFX
>  	tristate "3dfx Banshee/Voodoo3+"
>  	depends on DRM && PCI

[snip]

> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
> b/drivers/gpu/drm/drm_gem_cma_helper.c new file mode 100644
> index 0000000..d8c0dc7
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -0,0 +1,243 @@
> +/*
> + * drm gem cma (contiguous memory allocator) helper functions
> + *
> + * Copyright (C) 2012 Sascha Hauer, Pengutronix
> + *
> + * Based on Samsung Exynos code
> + *
> + * Copyright (c) 2011 Samsung Electronics Co., Ltd.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * 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.h>
> +#include <drm/drm_gem_cma_helper.h>
> +
> +static unsigned int get_gem_mmap_offset(struct drm_gem_object *obj)
> +{
> +	return (unsigned int)obj->map_list.hash.key << PAGE_SHIFT;
> +}
> +
> +static void drm_gem_cma_buf_destroy(struct drm_device *drm,
> +		struct drm_gem_cma_object *cma_obj)
> +{
> +	dma_free_writecombine(drm->dev, cma_obj->base.size, cma_obj->vaddr,
> +			cma_obj->paddr);
> +}
> +
> +/*
> + * drm_gem_cma_create - allocate an object with the given size
> + *
> + * returns a struct drm_gem_cma_object* on success or ERR_PTR values
> + * on failure.
> + */
> +struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> +		unsigned int size)
> +{
> +	struct drm_gem_cma_object *cma_obj;
> +	struct drm_gem_object *gem_obj;
> +	int ret;
> +
> +	size = round_up(size, PAGE_SIZE);
> +
> +	cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL);
> +	if (!cma_obj)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cma_obj->vaddr = dma_alloc_writecombine(drm->dev, size,
> +			&cma_obj->paddr, GFP_KERNEL | __GFP_NOWARN);
> +	if (!cma_obj->vaddr) {
> +		dev_err(drm->dev, "failed to allocate buffer with size %d\n", size);
> +		ret = -ENOMEM;
> +		goto err_dma_alloc;
> +	}
> +
> +	gem_obj = &cma_obj->base;
> +
> +	ret = drm_gem_object_init(drm, gem_obj, size);
> +	if (ret)
> +		goto err_obj_init;
> +
> +	ret = drm_gem_create_mmap_offset(gem_obj);
> +	if (ret)
> +		goto err_create_mmap_offset;
> +
> +	return cma_obj;
> +
> +err_create_mmap_offset:
> +	drm_gem_object_release(gem_obj);
> +
> +err_obj_init:
> +	drm_gem_cma_buf_destroy(drm, cma_obj);
> +
> +err_dma_alloc:
> +	kfree(cma_obj);
> +
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_cma_create);
> +
> +/*
> + * drm_gem_cma_create_with_handle - allocate an object with the given
> + * size and create a gem handle on it
> + *
> + * returns a struct drm_gem_cma_object* on success or ERR_PTR values
> + * on failure.
> + */
> +struct drm_gem_cma_object *drm_gem_cma_create_with_handle(
> +		struct drm_file *file_priv,
> +		struct drm_device *drm, unsigned int size,
> +		unsigned int *handle)

Shouldn't this function be static ?

> +{
> +	struct drm_gem_cma_object *cma_obj;
> +	struct drm_gem_object *gem_obj;
> +	int ret;
> +
> +	cma_obj = drm_gem_cma_create(drm, size);
> +	if (IS_ERR(cma_obj))
> +		return cma_obj;
> +
> +	gem_obj = &cma_obj->base;
> +
> +	/*
> +	 * 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, gem_obj, handle);
> +	if (ret)
> +		goto err_handle_create;
> +
> +	/* drop reference from allocate - handle holds it now. */
> +	drm_gem_object_unreference_unlocked(gem_obj);
> +
> +	return cma_obj;
> +
> +err_handle_create:
> +	drm_gem_cma_free_object(gem_obj);
> +
> +	return ERR_PTR(ret);
> +}
> +
> +/*
> + * drm_gem_cma_free_object - (struct drm_driver)->gem_free_object callback
> + * function
> + */
> +void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
> +{
> +	struct drm_gem_cma_object *cma_obj;
> +
> +	if (gem_obj->map_list.map)
> +		drm_gem_free_mmap_offset(gem_obj);
> +
> +	drm_gem_object_release(gem_obj);
> +
> +	cma_obj = to_drm_gem_cma_obj(gem_obj);
> +
> +	drm_gem_cma_buf_destroy(gem_obj->dev, cma_obj);
> +
> +	kfree(cma_obj);
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
> +
> +/*
> + * drm_gem_cma_dumb_create - (struct drm_driver)->dumb_create callback
> + * function
> + *
> + * This aligns the pitch and size arguments to the minimum required. wrap
> + * this into your own function if you need bigger alignment.
> + */
> +int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> +		struct drm_device *dev, struct drm_mode_create_dumb *args)
> +{
> +	struct drm_gem_cma_object *cma_obj;
> +
> +	if (args->pitch < args->width * DIV_ROUND_UP(args->bpp, 8))
> +		args->pitch = args->width * DIV_ROUND_UP(args->bpp, 8);

Shouldn't this be DIV_ROUND_UP(args->width * args->bpp, 8) ? Not all formats 
might need to pad pixels to an integer number of bytes.

> +
> +	if (args->size < args->pitch * args->height)
> +		args->size = args->pitch * args->height;
> +
> +	cma_obj = drm_gem_cma_create_with_handle(file_priv, dev,
> +			args->size, &args->handle);
> +	if (IS_ERR(cma_obj))
> +		return PTR_ERR(cma_obj);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create);
> +
> +/*
> + * drm_gem_cma_dumb_map_offset - (struct drm_driver)->dumb_map_offset
> callback + * function
> + */
> +int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
> +		struct drm_device *drm, uint32_t handle, uint64_t *offset)
> +{
> +	struct drm_gem_object *gem_obj;
> +
> +	mutex_lock(&drm->struct_mutex);
> +
> +	gem_obj = drm_gem_object_lookup(drm, file_priv, handle);
> +	if (!gem_obj) {
> +		dev_err(drm->dev, "failed to lookup gem object\n");
> +		mutex_unlock(&drm->struct_mutex);
> +		return -EINVAL;
> +	}
> +
> +	*offset = get_gem_mmap_offset(gem_obj);
> +
> +	drm_gem_object_unreference(gem_obj);
> +
> +	mutex_unlock(&drm->struct_mutex);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_map_offset);
> +
> +struct vm_operations_struct drm_gem_cma_vm_ops = {

Should this be const ?

> +	.open = drm_gem_vm_open,
> +	.close = drm_gem_vm_close,
> +};
> +EXPORT_SYMBOL_GPL(drm_gem_cma_vm_ops);
> +
> +/*
> + * drm_gem_cma_mmap - (struct file_operation)->mmap callback function
> + */
> +int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
> +{
> +	struct drm_gem_object *gem_obj;
> +	struct drm_gem_cma_object *cma_obj;
> +	int ret;
> +
> +	ret = drm_gem_mmap(filp, vma);
> +	if (ret)
> +		return ret;
> +
> +	gem_obj = vma->vm_private_data;
> +	cma_obj = to_drm_gem_cma_obj(gem_obj);
> +
> +	ret = remap_pfn_range(vma, vma->vm_start, cma_obj->paddr >> PAGE_SHIFT,
> +			vma->vm_end - vma->vm_start, vma->vm_page_prot);
> +	if (ret)
> +		drm_gem_vm_close(vma);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
> +
> +/*
> + * drm_gem_cma_dumb_destroy - (struct drm_driver)->dumb_destroy callback
> function + */
> +int drm_gem_cma_dumb_destroy(struct drm_file *file_priv,
> +		struct drm_device *drm, unsigned int handle)
> +{
> +	return drm_gem_handle_delete(file_priv, handle);
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_destroy);
> diff --git a/include/drm/drm_gem_cma_helper.h
> b/include/drm/drm_gem_cma_helper.h new file mode 100644
> index 0000000..c4ed90b
> --- /dev/null
> +++ b/include/drm/drm_gem_cma_helper.h
> @@ -0,0 +1,52 @@
> +#ifndef __DRM_GEM_CMA_HELPER_H__
> +#define __DRM_GEM_CMA_HELPER_H__
> +
> +struct drm_gem_cma_object {
> +	struct drm_gem_object base;
> +	dma_addr_t paddr;
> +	void *vaddr;
> +};
> +
> +static inline struct drm_gem_cma_object *
> +to_drm_gem_cma_obj(struct drm_gem_object *gem_obj)
> +{
> +	return container_of(gem_obj, struct drm_gem_cma_object, base);
> +}
> +
> +/* free gem object. */
> +void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
> +
> +/* create memory region for drm framebuffer. */
> +int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> +		struct drm_device *drm, struct drm_mode_create_dumb *args);
> +
> +/* map memory region for drm framebuffer to user space. */
> +int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
> +		struct drm_device *drm, uint32_t handle, uint64_t *offset);
> +
> +/* page fault handler and mmap fault address(virtual) to physical memory.
> */ +int drm_gem_cma_fault(struct vm_area_struct *vma, struct vm_fault
> *vmf);

That function has been removed.

> +
> +/* 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);
> +
> +/*
> + * destroy memory region allocated.
> + *	- a gem handle and physical memory region pointed by a gem object
> + *	would be released by drm_gem_handle_delete().
> + */
> +int drm_gem_cma_dumb_destroy(struct drm_file *file_priv,
> +		struct drm_device *drm, unsigned int handle);
> +
> +/* allocate physical memory. */
> +struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> +		unsigned int size);
> +
> +struct drm_gem_cma_object *drm_gem_cma_create_with_handle(
> +		struct drm_file *file_priv,
> +		struct drm_device *drm, unsigned int size,
> +		unsigned int *handle);
> +
> +extern struct vm_operations_struct drm_gem_cma_vm_ops;
> +
> +#endif /* __DRM_GEM_CMA_HELPER_H__ */

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] DRM: add drm gem cma helper

[InKi Dae]

Hi Sascha,

it's good try to avoid demand paging way by page fault handler.
actually, we don't need demand paging way on non-iommu system.  I
looked into your previous patch and I realized that patch was based on
old exynos driver. now patch looks almostly good to me and below is my
comments minor.

Thanks,
Inki Dae

2012/5/31 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Sascha,
>
> Thanks for the patch. Here's a bit more extensive review.
>
> On Thursday 31 May 2012 10:08:54 Sascha Hauer wrote:
>> Many embedded drm devices do not have a IOMMU and no dedicated
>> memory for graphics. These devices use cma (Contiguous Memory
>> Allocator) backed graphics memory. This patch provides helper
>> functions to be able to share the code.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> ---
>>
>> changes since v1:
>>
>> - map whole buffer at mmap time, not page by page at fault time
>> - several cleanups as suggested by Lars-Peter Clausen and Laurent Pinchart
>>
>> I currently do a remap_pfn_range after drm_gem_mmap, so we release
>> dev->struct_mutex in between. I don't know if this is correct or if we have
>> to hold the lock over the whole process.
>
> drm_gem_mmap() takes a reference on the GEM object so I don't think we'll have
> any issue there, but please don't take my word for it. dev->struct_mutex is
> the large mutex I still need to document, I don't know what it protects
> exactly.
>
>>  drivers/gpu/drm/Kconfig              |    6 +
>>  drivers/gpu/drm/Makefile             |    1 +
>>  drivers/gpu/drm/drm_gem_cma_helper.c |  243 +++++++++++++++++++++++++++++++
>>  include/drm/drm_gem_cma_helper.h     |   52 ++++++++
>>  4 files changed, 302 insertions(+)
>>  create mode 100644 drivers/gpu/drm/drm_gem_cma_helper.c
>>  create mode 100644 include/drm/drm_gem_cma_helper.h
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index e354bc0..f62717e 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -53,6 +53,12 @@ config DRM_TTM
>>         GPU memory types. Will be enabled automatically if a device driver
>>         uses it.
>>
>> +config DRM_GEM_CMA_HELPER
>> +     tristate
>> +     depends on DRM
>> +     help
>> +       Choose this if you need the GEM cma helper functions
>
> I would put CMA in uppercase, but that's just nitpicking.
>
> BTW this helper is not strictly dedicated to CMA. It uses the DMA API to
> allocate memory, without caring about the underlying allocator.
>
>> +
>>  config DRM_TDFX
>>       tristate "3dfx Banshee/Voodoo3+"
>>       depends on DRM && PCI
>
> [snip]
>
>> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
>> b/drivers/gpu/drm/drm_gem_cma_helper.c new file mode 100644
>> index 0000000..d8c0dc7
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
>> @@ -0,0 +1,243 @@
>> +/*
>> + * drm gem cma (contiguous memory allocator) helper functions
>> + *
>> + * Copyright (C) 2012 Sascha Hauer, Pengutronix
>> + *
>> + * Based on Samsung Exynos code
>> + *
>> + * Copyright (c) 2011 Samsung Electronics Co., Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version 2
>> + * of the License, or (at your option) any later version.
>> + * 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.h>
>> +#include <drm/drm_gem_cma_helper.h>
>> +
>> +static unsigned int get_gem_mmap_offset(struct drm_gem_object *obj)
>> +{
>> +     return (unsigned int)obj->map_list.hash.key << PAGE_SHIFT;
>> +}
>> +
>> +static void drm_gem_cma_buf_destroy(struct drm_device *drm,
>> +             struct drm_gem_cma_object *cma_obj)
>> +{
>> +     dma_free_writecombine(drm->dev, cma_obj->base.size, cma_obj->vaddr,
>> +                     cma_obj->paddr);
>> +}
>> +
>> +/*
>> + * drm_gem_cma_create - allocate an object with the given size
>> + *
>> + * returns a struct drm_gem_cma_object* on success or ERR_PTR values
>> + * on failure.
>> + */
>> +struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
>> +             unsigned int size)
>> +{
>> +     struct drm_gem_cma_object *cma_obj;
>> +     struct drm_gem_object *gem_obj;
>> +     int ret;
>> +
>> +     size = round_up(size, PAGE_SIZE);
>> +
>> +     cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL);
>> +     if (!cma_obj)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     cma_obj->vaddr = dma_alloc_writecombine(drm->dev, size,
>> +                     &cma_obj->paddr, GFP_KERNEL | __GFP_NOWARN);

how about calling drm_gem_cma_buf_allocate() instead of
dma_alloc_wirtecombine() for consistency in using dma api so its call
flow would be "dma_gem_cma_buf_allocate() ->
dma_alloc_writecombine()".

>> +     if (!cma_obj->vaddr) {
>> +             dev_err(drm->dev, "failed to allocate buffer with size %d\n", size);
>> +             ret = -ENOMEM;
>> +             goto err_dma_alloc;
>> +     }
>> +
>> +     gem_obj = &cma_obj->base;
>> +
>> +     ret = drm_gem_object_init(drm, gem_obj, size);
>> +     if (ret)
>> +             goto err_obj_init;
>> +
>> +     ret = drm_gem_create_mmap_offset(gem_obj);
>> +     if (ret)
>> +             goto err_create_mmap_offset;
>> +
>> +     return cma_obj;
>> +
>> +err_create_mmap_offset:
>> +     drm_gem_object_release(gem_obj);
>> +
>> +err_obj_init:
>> +     drm_gem_cma_buf_destroy(drm, cma_obj);
>> +
>> +err_dma_alloc:
>> +     kfree(cma_obj);
>> +
>> +     return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(drm_gem_cma_create);
>> +
>> +/*
>> + * drm_gem_cma_create_with_handle - allocate an object with the given
>> + * size and create a gem handle on it
>> + *
>> + * returns a struct drm_gem_cma_object* on success or ERR_PTR values
>> + * on failure.
>> + */
>> +struct drm_gem_cma_object *drm_gem_cma_create_with_handle(
>> +             struct drm_file *file_priv,
>> +             struct drm_device *drm, unsigned int size,
>> +             unsigned int *handle)
>
> Shouldn't this function be static ?
>
>> +{
>> +     struct drm_gem_cma_object *cma_obj;
>> +     struct drm_gem_object *gem_obj;
>> +     int ret;
>> +
>> +     cma_obj = drm_gem_cma_create(drm, size);
>> +     if (IS_ERR(cma_obj))
>> +             return cma_obj;
>> +
>> +     gem_obj = &cma_obj->base;
>> +
>> +     /*
>> +      * 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, gem_obj, handle);
>> +     if (ret)
>> +             goto err_handle_create;
>> +
>> +     /* drop reference from allocate - handle holds it now. */
>> +     drm_gem_object_unreference_unlocked(gem_obj);
>> +
>> +     return cma_obj;
>> +
>> +err_handle_create:
>> +     drm_gem_cma_free_object(gem_obj);
>> +
>> +     return ERR_PTR(ret);
>> +}
>> +
>> +/*
>> + * drm_gem_cma_free_object - (struct drm_driver)->gem_free_object callback
>> + * function
>> + */
>> +void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
>> +{
>> +     struct drm_gem_cma_object *cma_obj;
>> +
>> +     if (gem_obj->map_list.map)
>> +             drm_gem_free_mmap_offset(gem_obj);
>> +
>> +     drm_gem_object_release(gem_obj);
>> +
>> +     cma_obj = to_drm_gem_cma_obj(gem_obj);
>> +
>> +     drm_gem_cma_buf_destroy(gem_obj->dev, cma_obj);
>> +
>> +     kfree(cma_obj);
>> +}
>> +EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
>> +
>> +/*
>> + * drm_gem_cma_dumb_create - (struct drm_driver)->dumb_create callback
>> + * function
>> + *
>> + * This aligns the pitch and size arguments to the minimum required. wrap
>> + * this into your own function if you need bigger alignment.
>> + */
>> +int drm_gem_cma_dumb_create(struct drm_file *file_priv,
>> +             struct drm_device *dev, struct drm_mode_create_dumb *args)
>> +{
>> +     struct drm_gem_cma_object *cma_obj;
>> +
>> +     if (args->pitch < args->width * DIV_ROUND_UP(args->bpp, 8))
>> +             args->pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
>
> Shouldn't this be DIV_ROUND_UP(args->width * args->bpp, 8) ? Not all formats
> might need to pad pixels to an integer number of bytes.
>
>> +
>> +     if (args->size < args->pitch * args->height)
>> +             args->size = args->pitch * args->height;
>> +
>> +     cma_obj = drm_gem_cma_create_with_handle(file_priv, dev,
>> +                     args->size, &args->handle);
>> +     if (IS_ERR(cma_obj))
>> +             return PTR_ERR(cma_obj);
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create);
>> +
>> +/*
>> + * drm_gem_cma_dumb_map_offset - (struct drm_driver)->dumb_map_offset
>> callback + * function
>> + */
>> +int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
>> +             struct drm_device *drm, uint32_t handle, uint64_t *offset)
>> +{
>> +     struct drm_gem_object *gem_obj;
>> +
>> +     mutex_lock(&drm->struct_mutex);
>> +
>> +     gem_obj = drm_gem_object_lookup(drm, file_priv, handle);
>> +     if (!gem_obj) {
>> +             dev_err(drm->dev, "failed to lookup gem object\n");
>> +             mutex_unlock(&drm->struct_mutex);
>> +             return -EINVAL;
>> +     }
>> +
>> +     *offset = get_gem_mmap_offset(gem_obj);
>> +
>> +     drm_gem_object_unreference(gem_obj);
>> +
>> +     mutex_unlock(&drm->struct_mutex);
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_map_offset);
>> +
>> +struct vm_operations_struct drm_gem_cma_vm_ops = {
>
> Should this be const ?
>
>> +     .open = drm_gem_vm_open,
>> +     .close = drm_gem_vm_close,
>> +};
>> +EXPORT_SYMBOL_GPL(drm_gem_cma_vm_ops);
>> +
>> +/*
>> + * drm_gem_cma_mmap - (struct file_operation)->mmap callback function
>> + */
>> +int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
>> +{
>> +     struct drm_gem_object *gem_obj;
>> +     struct drm_gem_cma_object *cma_obj;
>> +     int ret;
>> +
>> +     ret = drm_gem_mmap(filp, vma);
>> +     if (ret)
>> +             return ret;
>> +
>> +     gem_obj = vma->vm_private_data;
>> +     cma_obj = to_drm_gem_cma_obj(gem_obj);

it's likely to need checking if user space size is valid or not here.
like this;
if (vma->end - vma->start > gem_obj->size) {
          error message;
         and some exceptions;
}

>> +
>> +     ret = remap_pfn_range(vma, vma->vm_start, cma_obj->paddr >> PAGE_SHIFT,
>> +                     vma->vm_end - vma->vm_start, vma->vm_page_prot);
>> +     if (ret)
>> +             drm_gem_vm_close(vma);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
>> +
>> +/*
>> + * drm_gem_cma_dumb_destroy - (struct drm_driver)->dumb_destroy callback
>> function + */
>> +int drm_gem_cma_dumb_destroy(struct drm_file *file_priv,
>> +             struct drm_device *drm, unsigned int handle)
>> +{
>> +     return drm_gem_handle_delete(file_priv, handle);
>> +}
>> +EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_destroy);
>> diff --git a/include/drm/drm_gem_cma_helper.h
>> b/include/drm/drm_gem_cma_helper.h new file mode 100644
>> index 0000000..c4ed90b
>> --- /dev/null
>> +++ b/include/drm/drm_gem_cma_helper.h
>> @@ -0,0 +1,52 @@
>> +#ifndef __DRM_GEM_CMA_HELPER_H__
>> +#define __DRM_GEM_CMA_HELPER_H__
>> +
>> +struct drm_gem_cma_object {
>> +     struct drm_gem_object base;
>> +     dma_addr_t paddr;
>> +     void *vaddr;
>> +};
>> +
>> +static inline struct drm_gem_cma_object *
>> +to_drm_gem_cma_obj(struct drm_gem_object *gem_obj)
>> +{
>> +     return container_of(gem_obj, struct drm_gem_cma_object, base);
>> +}
>> +
>> +/* free gem object. */
>> +void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
>> +
>> +/* create memory region for drm framebuffer. */
>> +int drm_gem_cma_dumb_create(struct drm_file *file_priv,
>> +             struct drm_device *drm, struct drm_mode_create_dumb *args);
>> +
>> +/* map memory region for drm framebuffer to user space. */
>> +int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
>> +             struct drm_device *drm, uint32_t handle, uint64_t *offset);
>> +
>> +/* page fault handler and mmap fault address(virtual) to physical memory.
>> */ +int drm_gem_cma_fault(struct vm_area_struct *vma, struct vm_fault
>> *vmf);
>
> That function has been removed.
>
>> +
>> +/* 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);
>> +
>> +/*
>> + * destroy memory region allocated.
>> + *   - a gem handle and physical memory region pointed by a gem object
>> + *   would be released by drm_gem_handle_delete().
>> + */
>> +int drm_gem_cma_dumb_destroy(struct drm_file *file_priv,
>> +             struct drm_device *drm, unsigned int handle);
>> +
>> +/* allocate physical memory. */
>> +struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
>> +             unsigned int size);
>> +
>> +struct drm_gem_cma_object *drm_gem_cma_create_with_handle(
>> +             struct drm_file *file_priv,
>> +             struct drm_device *drm, unsigned int size,
>> +             unsigned int *handle);
>> +
>> +extern struct vm_operations_struct drm_gem_cma_vm_ops;
>> +
>> +#endif /* __DRM_GEM_CMA_HELPER_H__ */
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] DRM: add drm gem cma helper

[Lars-Peter Clausen]

On 05/31/2012 10:08 AM, Sascha Hauer wrote:
> [...]
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
b/drivers/gpu/drm/drm_gem_cma_helper.c
> new file mode 100644
> index 0000000..d8c0dc7
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -0,0 +1,243 @@
> +/*
> + * drm gem cma (contiguous memory allocator) helper functions
> + *
> + * Copyright (C) 2012 Sascha Hauer, Pengutronix
> + *
> + * Based on Samsung Exynos code
> + *
> + * Copyright (c) 2011 Samsung Electronics Co., Ltd.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * 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.h>
> +#include <drm/drm_gem_cma_helper.h>

Sorry, forgot to mention this during the v1 review. This needs
'#include <linux/export.h>' for EXPORT_SYMBOL_GPL. I get compile errors
otherwise. Maybe it also makes sense to include linux/slab.h,
linux/dma-mapping.h, linux/mm.h and linux/mutex.h since those are only
implicitly included right now.

- Lars

Re: [PATCH v2] DRM: add drm gem cma helper

[Sascha Hauer]

On Thu, May 31, 2012 at 11:36:15AM +0200, Laurent Pinchart wrote:
> Hi Sascha,
> 
> > +	depends on DRM
> > +	help
> > +	  Choose this if you need the GEM cma helper functions
> 
> I would put CMA in uppercase, but that's just nitpicking.
> 
> BTW this helper is not strictly dedicated to CMA. It uses the DMA API to 
> allocate memory, without caring about the underlying allocator.

Yes, I know. It's just that 'CMA' is short and expresses very much what
I mean. I first had 'dma_alloc' instead, but this makes the function
names quite long.

> > +
> > +/*
> > + * drm_gem_cma_dumb_create - (struct drm_driver)->dumb_create callback
> > + * function
> > + *
> > + * This aligns the pitch and size arguments to the minimum required. wrap
> > + * this into your own function if you need bigger alignment.
> > + */
> > +int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> > +		struct drm_device *dev, struct drm_mode_create_dumb *args)
> > +{
> > +	struct drm_gem_cma_object *cma_obj;
> > +
> > +	if (args->pitch < args->width * DIV_ROUND_UP(args->bpp, 8))
> > +		args->pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
> 
> Shouldn't this be DIV_ROUND_UP(args->width * args->bpp, 8) ? Not all formats 
> might need to pad pixels to an integer number of bytes.

Are you thinking about YUV formats? I can't imagine RGB formats where
this might be an issue.

Integrated the rest of you comments.

Thanks
 Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] DRM: add drm gem cma helper

[Sascha Hauer]

On Fri, Jun 01, 2012 at 12:29:47AM +0900, InKi Dae wrote:
> Hi Sascha,
> 
> >> +struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> >> +             unsigned int size)
> >> +{
> >> +     struct drm_gem_cma_object *cma_obj;
> >> +     struct drm_gem_object *gem_obj;
> >> +     int ret;
> >> +
> >> +     size = round_up(size, PAGE_SIZE);
> >> +
> >> +     cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL);
> >> +     if (!cma_obj)
> >> +             return ERR_PTR(-ENOMEM);
> >> +
> >> +     cma_obj->vaddr = dma_alloc_writecombine(drm->dev, size,
> >> +                     &cma_obj->paddr, GFP_KERNEL | __GFP_NOWARN);
> 
> how about calling drm_gem_cma_buf_allocate() instead of
> dma_alloc_wirtecombine() for consistency in using dma api so its call
> flow would be "dma_gem_cma_buf_allocate() ->
> dma_alloc_writecombine()".

What do you mean? There is no drm_gem_cma_buf_allocate() function.

> >> +     struct drm_gem_object *gem_obj;
> >> +     struct drm_gem_cma_object *cma_obj;
> >> +     int ret;
> >> +
> >> +     ret = drm_gem_mmap(filp, vma);
> >> +     if (ret)
> >> +             return ret;
> >> +
> >> +     gem_obj = vma->vm_private_data;
> >> +     cma_obj = to_drm_gem_cma_obj(gem_obj);
> 
> it's likely to need checking if user space size is valid or not here.
> like this;
> if (vma->end - vma->start > gem_obj->size) {
>           error message;
>          and some exceptions;
> }

This is already done in drm_gem_mmap().

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] DRM: add drm gem cma helper

[Laurent Pinchart]

Hi Sascha,

On Tuesday 05 June 2012 09:49:48 Sascha Hauer wrote:
> On Thu, May 31, 2012 at 11:36:15AM +0200, Laurent Pinchart wrote:
> > Hi Sascha,
> > 
> > > +	depends on DRM
> > > +	help
> > > +	  Choose this if you need the GEM cma helper functions
> > 
> > I would put CMA in uppercase, but that's just nitpicking.
> > 
> > BTW this helper is not strictly dedicated to CMA. It uses the DMA API to
> > allocate memory, without caring about the underlying allocator.
> 
> Yes, I know. It's just that 'CMA' is short and expresses very much what
> I mean. I first had 'dma_alloc' instead, but this makes the function
> names quite long.
>
> > > +
> > > +/*
> > > + * drm_gem_cma_dumb_create - (struct drm_driver)->dumb_create callback
> > > + * function
> > > + *
> > > + * This aligns the pitch and size arguments to the minimum required.
> > > wrap
> > > + * this into your own function if you need bigger alignment.
> > > + */
> > > +int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> > > +		struct drm_device *dev, struct drm_mode_create_dumb *args)
> > > +{
> > > +	struct drm_gem_cma_object *cma_obj;
> > > +
> > > +	if (args->pitch < args->width * DIV_ROUND_UP(args->bpp, 8))
> > > +		args->pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
> > 
> > Shouldn't this be DIV_ROUND_UP(args->width * args->bpp, 8) ? Not all
> > formats might need to pad pixels to an integer number of bytes.
> 
> Are you thinking about YUV formats? I can't imagine RGB formats where
> this might be an issue.

Yes I was thinking mainly about YUV, although I'm not sure whether we already 
have formats where this could happen. It won't hurt to be prepared for the 
future though :-)

> Integrated the rest of you comments.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] DRM: add drm gem cma helper

[InKi Dae]

2012/6/5 Sascha Hauer <s.hauer@pengutronix.de>:
> On Fri, Jun 01, 2012 at 12:29:47AM +0900, InKi Dae wrote:
>> Hi Sascha,
>>
>> >> +struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
>> >> +             unsigned int size)
>> >> +{
>> >> +     struct drm_gem_cma_object *cma_obj;
>> >> +     struct drm_gem_object *gem_obj;
>> >> +     int ret;
>> >> +
>> >> +     size = round_up(size, PAGE_SIZE);
>> >> +
>> >> +     cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL);
>> >> +     if (!cma_obj)
>> >> +             return ERR_PTR(-ENOMEM);
>> >> +
>> >> +     cma_obj->vaddr = dma_alloc_writecombine(drm->dev, size,
>> >> +                     &cma_obj->paddr, GFP_KERNEL | __GFP_NOWARN);
>>
>> how about calling drm_gem_cma_buf_allocate() instead of
>> dma_alloc_wirtecombine() for consistency in using dma api so its call
>> flow would be "dma_gem_cma_buf_allocate() ->
>> dma_alloc_writecombine()".
>
> What do you mean? There is no drm_gem_cma_buf_allocate() function.
>

I mean have a pair with drm_gem_cma_buf_destroy() otherwise how about
removing dem_gem_cma_buf_destroy() and calling dma_free_writecombine()
directly? drm_gem_cma_buf_destroy() is just a wrapper to
dma_free_writecombine().

>> >> +     struct drm_gem_object *gem_obj;
>> >> +     struct drm_gem_cma_object *cma_obj;
>> >> +     int ret;
>> >> +
>> >> +     ret = drm_gem_mmap(filp, vma);
>> >> +     if (ret)
>> >> +             return ret;
>> >> +
>> >> +     gem_obj = vma->vm_private_data;
>> >> +     cma_obj = to_drm_gem_cma_obj(gem_obj);
>>
>> it's likely to need checking if user space size is valid or not here.
>> like this;
>> if (vma->end - vma->start > gem_obj->size) {
>>           error message;
>>          and some exceptions;
>> }
>
> This is already done in drm_gem_mmap().
>
> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] DRM: add drm gem cma helper

[Sascha Hauer]

On Tue, Jun 05, 2012 at 10:54:10PM +0900, InKi Dae wrote:
> 2012/6/5 Sascha Hauer <s.hauer@pengutronix.de>:
> > On Fri, Jun 01, 2012 at 12:29:47AM +0900, InKi Dae wrote:
> >> Hi Sascha,
> >>
> >> >> +struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> >> >> +             unsigned int size)
> >> >> +{
> >> >> +     struct drm_gem_cma_object *cma_obj;
> >> >> +     struct drm_gem_object *gem_obj;
> >> >> +     int ret;
> >> >> +
> >> >> +     size = round_up(size, PAGE_SIZE);
> >> >> +
> >> >> +     cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL);
> >> >> +     if (!cma_obj)
> >> >> +             return ERR_PTR(-ENOMEM);
> >> >> +
> >> >> +     cma_obj->vaddr = dma_alloc_writecombine(drm->dev, size,
> >> >> +                     &cma_obj->paddr, GFP_KERNEL | __GFP_NOWARN);
> >>
> >> how about calling drm_gem_cma_buf_allocate() instead of
> >> dma_alloc_wirtecombine() for consistency in using dma api so its call
> >> flow would be "dma_gem_cma_buf_allocate() ->
> >> dma_alloc_writecombine()".
> >
> > What do you mean? There is no drm_gem_cma_buf_allocate() function.
> >
> 
> I mean have a pair with drm_gem_cma_buf_destroy() otherwise how about
> removing dem_gem_cma_buf_destroy() and calling dma_free_writecombine()
> directly? drm_gem_cma_buf_destroy() is just a wrapper to
> dma_free_writecombine().

Ok, then I'll remove drm_gem_cma_buf_destroy() and use
dma_free_writecombine() directly instead.


Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] DRM: add drm gem cma helper

[Laurent Pinchart]

Hi Sascha,

On Thursday 31 May 2012 10:08:54 Sascha Hauer wrote:
> Many embedded drm devices do not have a IOMMU and no dedicated
> memory for graphics. These devices use cma (Contiguous Memory
> Allocator) backed graphics memory. This patch provides helper
> functions to be able to share the code.

Do you plan to post v3 in the near future ? I'm preparing the next version of 
the Renesas Mobile DRM driver, which will depend on this patch (as well as 
Lars-Peter's kms/fb cma helper).

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] DRM: add drm gem cma helper

[Sascha Hauer]

Hi Laurent,

On Wed, Jun 27, 2012 at 01:38:39PM +0200, Laurent Pinchart wrote:
> Hi Sascha,
> 
> On Thursday 31 May 2012 10:08:54 Sascha Hauer wrote:
> > Many embedded drm devices do not have a IOMMU and no dedicated
> > memory for graphics. These devices use cma (Contiguous Memory
> > Allocator) backed graphics memory. This patch provides helper
> > functions to be able to share the code.
> 
> Do you plan to post v3 in the near future ?

Just did that.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |