[PATCH] DRM: add drm gem CMA helper

[PATCH] 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>
---

Lars-Peter, please let me know if this fits your needs or if you are missing
something.

 drivers/gpu/drm/Kconfig              |    6 +
 drivers/gpu/drm/Makefile             |    1 +
 drivers/gpu/drm/drm_gem_cma_helper.c |  321 ++++++++++++++++++++++++++++++++++
 include/drm/drm_gem_cma_helper.h     |   47 +++++
 4 files changed, 375 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..75534ff
--- /dev/null
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -0,0 +1,321 @@
+/*
+ * 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 convert_to_vm_err_msg(int msg)
+{
+	unsigned int out_msg;
+
+	switch (msg) {
+	case 0:
+	case -ERESTARTSYS:
+	case -EINTR:
+		out_msg = VM_FAULT_NOPAGE;
+		break;
+
+	case -ENOMEM:
+		out_msg = VM_FAULT_OOM;
+		break;
+
+	default:
+		out_msg = VM_FAULT_SIGBUS;
+		break;
+	}
+
+	return out_msg;
+}
+
+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_obj *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_obj* on success or ERR_PTR values
+ * on failure.
+ */
+struct drm_gem_cma_obj *drm_gem_cma_create(struct drm_device *drm,
+		unsigned int size)
+{
+	struct drm_gem_cma_obj *cma_obj;
+	struct drm_gem_object *gem_obj;
+	int ret;
+
+	size = roundup(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,
+			(dma_addr_t *)&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_obj* on success or ERR_PTR values
+ * on failure.
+ */
+struct drm_gem_cma_obj *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_obj *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);
+}
+
+static int drm_gem_cma_mmap_buffer(struct file *filp,
+		struct vm_area_struct *vma)
+{
+	struct drm_gem_object *gem_obj = filp->private_data;
+	struct drm_gem_cma_obj *cma_obj = to_dma_alloc_gem_obj(gem_obj);
+	unsigned long pfn, vm_size;
+
+	vma->vm_flags |= VM_IO | VM_RESERVED;
+
+	vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
+	vma->vm_file = filp;
+
+	vm_size = vma->vm_end - vma->vm_start;
+
+	/* check if user-requested size is valid. */
+	if (vm_size > gem_obj->size)
+		return -EINVAL;
+
+	/*
+	 * get page frame number to physical memory to be mapped
+	 * to user space.
+	 */
+	pfn = cma_obj->paddr >> PAGE_SHIFT;
+
+	if (remap_pfn_range(vma, vma->vm_start, pfn, vm_size,
+				vma->vm_page_prot)) {
+		dev_err(gem_obj->dev->dev, "failed to remap pfn range.\n");
+		return -EAGAIN;
+	}
+
+	return 0;
+}
+
+static const struct file_operations drm_gem_cma_fops = {
+	.mmap = drm_gem_cma_mmap_buffer,
+};
+
+/*
+ * 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_obj *cma_obj;
+
+	if (gem_obj->map_list.map)
+		drm_gem_free_mmap_offset(gem_obj);
+
+	drm_gem_object_release(gem_obj);
+
+	cma_obj = to_dma_alloc_gem_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_obj *cma_obj;
+
+	if (args->pitch < args->width * args->bpp >> 3)
+		args->pitch = args->width * args->bpp >> 3;
+
+	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_cma_obj *cma_obj;
+	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;
+	}
+
+	cma_obj = to_dma_alloc_gem_obj(gem_obj);
+
+	*offset = get_gem_mmap_offset(&cma_obj->base);
+
+	drm_gem_object_unreference(gem_obj);
+
+	mutex_unlock(&drm->struct_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_map_offset);
+
+/*
+ * drm_gem_cma_fault - (struct vm_operations_struct)->fault callback function
+ */
+int drm_gem_cma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
+{
+	struct drm_gem_object *gem_obj = vma->vm_private_data;
+	struct drm_gem_cma_obj *cma_obj = to_dma_alloc_gem_obj(gem_obj);
+	struct drm_device *dev = gem_obj->dev;
+	unsigned long pfn;
+	pgoff_t page_offset;
+	int ret;
+
+	page_offset = ((unsigned long)vmf->virtual_address -
+			vma->vm_start) >> PAGE_SHIFT;
+
+	mutex_lock(&dev->struct_mutex);
+
+	pfn = (cma_obj->paddr >> PAGE_SHIFT) + page_offset;
+
+	ret = vm_insert_mixed(vma, (unsigned long)vmf->virtual_address, pfn);
+
+	mutex_unlock(&dev->struct_mutex);
+
+	return convert_to_vm_err_msg(ret);
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_fault);
+
+/*
+ * drm_gem_cma_mmap - (struct file_operation)->mmap callback function
+ */
+int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+	int ret;
+
+	ret = drm_gem_mmap(filp, vma);
+	if (ret)
+		return ret;
+
+	vma->vm_flags &= ~VM_PFNMAP;
+	vma->vm_flags |= VM_MIXEDMAP;
+
+	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..53b007c
--- /dev/null
+++ b/include/drm/drm_gem_cma_helper.h
@@ -0,0 +1,47 @@
+#ifndef __DRM_GEM_DMA_ALLOC_HELPER_H__
+#define __DRM_GEM_DMA_ALLOC_HELPER_H__
+
+struct drm_gem_cma_obj {
+	struct drm_gem_object base;
+	dma_addr_t paddr;
+	void __iomem *vaddr;
+};
+
+#define to_dma_alloc_gem_obj(x) \
+	container_of(x,	struct drm_gem_cma_obj, 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_obj *drm_gem_cma_create(struct drm_device *drm,
+		unsigned int size);
+
+struct drm_gem_cma_obj *drm_gem_cma_create_with_handle(
+		struct drm_file *file_priv,
+		struct drm_device *drm, unsigned int size,
+		unsigned int *handle);
+
+#endif
-- 
1.7.10

Re: [PATCH] DRM: add drm gem CMA helper

[Lars-Peter Clausen]

On 05/29/2012 04:10 PM, 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>
> ---
> 
> Lars-Peter, please let me know if this fits your needs or if you are missing
> something.
> 

Awesome :) The overall structure looks basically like what I had, so I can
just use these functions as drop-in replacement. I had some minor
differences in the implementation though. Comments inline.

>  drivers/gpu/drm/Kconfig              |    6 +
>  drivers/gpu/drm/Makefile             |    1 +
>  drivers/gpu/drm/drm_gem_cma_helper.c |  321 ++++++++++++++++++++++++++++++++++
>  include/drm/drm_gem_cma_helper.h     |   47 +++++
>  4 files changed, 375 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

This shouldn't have a help text as it should be selected by the driver and
not by the user. Also the 'depends on DRM' can go away, since it becomes
meaningless if the symbol is not user-selectable.

> +
>  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..75534ff
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -0,0 +1,321 @@
> [...]
> +/*
> + * drm_gem_cma_create - allocate an object with the given size
> + *
> + * returns a struct drm_gem_cma_obj* on success or ERR_PTR values
> + * on failure.
> + */
> +struct drm_gem_cma_obj *drm_gem_cma_create(struct drm_device *drm,
> +		unsigned int size)
> +{
> +	struct drm_gem_cma_obj *cma_obj;
> +	struct drm_gem_object *gem_obj;
> +	int ret;
> +
> +	size = roundup(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,
> +			(dma_addr_t *)&cma_obj->paddr,

The cast shouldn't be necessary.


? [...]
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_cma_create);
>[...]
> +
> +static int drm_gem_cma_mmap_buffer(struct file *filp,
> +		struct vm_area_struct *vma)
> +{
> +	struct drm_gem_object *gem_obj = filp->private_data;
> +	struct drm_gem_cma_obj *cma_obj = to_dma_alloc_gem_obj(gem_obj);
> +	unsigned long pfn, vm_size;
> +
> +	vma->vm_flags |= VM_IO | VM_RESERVED;
> +
> +	vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> +	vma->vm_file = filp;
> +
> +	vm_size = vma->vm_end - vma->vm_start;
> +
> +	/* check if user-requested size is valid. */
> +	if (vm_size > gem_obj->size)
> +		return -EINVAL;
> +
> +	/*
> +	 * get page frame number to physical memory to be mapped
> +	 * to user space.
> +	 */
> +	pfn = cma_obj->paddr >> PAGE_SHIFT;
> +
> +	if (remap_pfn_range(vma, vma->vm_start, pfn, vm_size,
> +				vma->vm_page_prot)) {
> +		dev_err(gem_obj->dev->dev, "failed to remap pfn range.\n");
> +		return -EAGAIN;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct file_operations drm_gem_cma_fops = {
> +	.mmap = drm_gem_cma_mmap_buffer,
> +};

This and the function above seem to be unused. I think it's a relict from
the old Exynos code.

> [...]
> +/*
> + * 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_obj *cma_obj;
> +
> +	if (args->pitch < args->width * args->bpp >> 3)
> +		args->pitch = args->width * args->bpp >> 3;

I used DIV_ROUND_UP(args->bpp, 8) here. I think it makes more sense for the
generic case.

> +
> +	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_cma_obj *cma_obj;
> +	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;
> +	}
> +
> +	cma_obj = to_dma_alloc_gem_obj(gem_obj);
> +
> +	*offset = get_gem_mmap_offset(&cma_obj->base);

Just get_gem_mmap_offset(gem_obj), also means the to_dma_alloc_gem_obj can
go away.

> +
> +	drm_gem_object_unreference(gem_obj);
> +
> +	mutex_unlock(&drm->struct_mutex);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_map_offset);
> +
> +/*
> + * drm_gem_cma_fault - (struct vm_operations_struct)->fault callback function
> + */
> +int drm_gem_cma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> +{
> +	struct drm_gem_object *gem_obj = vma->vm_private_data;
> +	struct drm_gem_cma_obj *cma_obj = to_dma_alloc_gem_obj(gem_obj);
> +	struct drm_device *dev = gem_obj->dev;
> +	unsigned long pfn;
> +	pgoff_t page_offset;
> +	int ret;
> +
> +	page_offset = ((unsigned long)vmf->virtual_address -
> +			vma->vm_start) >> PAGE_SHIFT;
> +
> +	mutex_lock(&dev->struct_mutex);
> +
> +	pfn = (cma_obj->paddr >> PAGE_SHIFT) + page_offset;
> +
> +	ret = vm_insert_mixed(vma, (unsigned long)vmf->virtual_address, pfn);
> +
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return convert_to_vm_err_msg(ret);
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_cma_fault);
> +

Do you think it makes sense to have generic vm_operations struct as well, I
think it will look the same for most drivers:

struct vm_operations_struct drm_gem_cma_vm_ops = {
    .fault = drm_gem_cma_fault,
    .open = drm_gem_vm_open,
    .close = drm_gem_vm_close,
};

> +/*
> + * drm_gem_cma_mmap - (struct file_operation)->mmap callback function
> + */
> +int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
> +{
> +	int ret;
> +
> +	ret = drm_gem_mmap(filp, vma);
> +	if (ret)
> +		return ret;
> +
> +	vma->vm_flags &= ~VM_PFNMAP;
> +	vma->vm_flags |= VM_MIXEDMAP;
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);

This might be worth being made a more generic helper function, since we are
not the only ones who want a mixed mapping. But that can wait for later.

> +
> +/*
> + * 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..53b007c
> --- /dev/null
> +++ b/include/drm/drm_gem_cma_helper.h
> @@ -0,0 +1,47 @@
> +#ifndef __DRM_GEM_DMA_ALLOC_HELPER_H__
> +#define __DRM_GEM_DMA_ALLOC_HELPER_H__
> +
> +struct drm_gem_cma_obj {
> +	struct drm_gem_object base;
> +	dma_addr_t paddr;
> +	void __iomem *vaddr;

__iomem?

> +};
> +
> +#define to_dma_alloc_gem_obj(x) \
> +	container_of(x,	struct drm_gem_cma_obj, 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_obj *drm_gem_cma_create(struct drm_device *drm,
> +		unsigned int size);
> +
> +struct drm_gem_cma_obj *drm_gem_cma_create_with_handle(
> +		struct drm_file *file_priv,
> +		struct drm_device *drm, unsigned int size,
> +		unsigned int *handle);
> +
> +#endif

Thanks for taking care of this,
- Lars

Re: [PATCH] DRM: add drm gem CMA helper

[Lars-Peter Clausen]

On 05/29/2012 04:46 PM, Lars-Peter Clausen wrote:
> On 05/29/2012 04:10 PM, 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>
>> ---
>>
>> Lars-Peter, please let me know if this fits your needs or if you are missing
>> something.
>>
> 
> Awesome :) The overall structure looks basically like what I had, so I can
> just use these functions as drop-in replacement. I had some minor
> differences in the implementation though. Comments inline.

Did some initial testing and it seems to work fine :)

> 
>>  drivers/gpu/drm/Kconfig              |    6 +
>>  drivers/gpu/drm/Makefile             |    1 +
>>  drivers/gpu/drm/drm_gem_cma_helper.c |  321 ++++++++++++++++++++++++++++++++++
>>  include/drm/drm_gem_cma_helper.h     |   47 +++++
>>  4 files changed, 375 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
> 
> This shouldn't have a help text as it should be selected by the driver and
> not by the user. Also the 'depends on DRM' can go away, since it becomes
> meaningless if the symbol is not user-selectable.
> 

Sorry, ignore that. I though it would appear in menuconfig if it had an help
text. But it needs to have a title. The "depends on" can still go away though.

>> +
>> +#define to_dma_alloc_gem_obj(x) \
>> +	container_of(x,	struct drm_gem_cma_obj, base)

This looks like it has been missed during some renaming. It should probably
be "to_drm_gem_cma_obj". And maybe make it an inline function.

Thanks,
- Lars

Re: [PATCH] DRM: add drm gem CMA helper

[Sascha Hauer]

Hi Lars,

Thanks for your quick comments.

On Tue, May 29, 2012 at 04:46:36PM +0200, Lars-Peter Clausen wrote:
> On 05/29/2012 04:10 PM, Sascha Hauer wrote:
> > 
> > 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
> 
> This shouldn't have a help text as it should be selected by the driver and
> not by the user. Also the 'depends on DRM' can go away, since it becomes
> meaningless if the symbol is not user-selectable.

The other helpers also have a depends on DRM in them. You are right,
it's quite meaningless. The only advantage I can think of is that it
would produce a 'has unmet direct dependencies' if someone outside DRM
would select this. I'll drop it.

> > +
> > +static const struct file_operations drm_gem_cma_fops = {
> > +	.mmap = drm_gem_cma_mmap_buffer,
> > +};
> 
> This and the function above seem to be unused. I think it's a relict from
> the old Exynos code.

Indeed. I wonder why my compiler hasn't warned me about this. Will
remove.

> 
> Do you think it makes sense to have generic vm_operations struct as well, I
> think it will look the same for most drivers:
> 
> struct vm_operations_struct drm_gem_cma_vm_ops = {
>     .fault = drm_gem_cma_fault,
>     .open = drm_gem_vm_open,
>     .close = drm_gem_vm_close,
> };

As we both can make use of this I'll add it. People who need something
else can still add their own vm_operations_struct.

I integrated your other comments, will repost tomorrow unless there are
more comments on this.

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 |

[PATCH 1/2] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

This patchset introduces a set of helper function for implementing the KMS
framebuffer layer for drivers which use the drm gem CMA helper function.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/gpu/drm/Kconfig             |   11 ++
 drivers/gpu/drm/Makefile            |    1 +
 drivers/gpu/drm/drm_fb_cma_helper.c |  359 +++++++++++++++++++++++++++++++++++
 include/drm/drm_fb_cma_helper.h     |   24 +++
 4 files changed, 395 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
 create mode 100644 include/drm/drm_fb_cma_helper.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 6b6e519..0050caa 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -41,6 +41,17 @@ config DRM_GEM_CMA_HELPER
 	help
 	  Choose this if you need the GEM cma helper functions
 
+config DRM_KMS_CMA_HELPER
+	tristate
+	select DRM_GEM_CMA_HELPER
+	select DRM_KMS_HELPER
+	select FB_SYS_FILLRECT
+	select FB_SYS_COPYAREA
+	select FB_SYS_IMAGEBLIT
+	help
+	  Choose this if you need the KMS 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 2c882af..8b332fe 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -18,6 +18,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
 
 drm_kms_helper-y := drm_fb_helper.o drm_crtc_helper.o drm_dp_i2c_helper.o
+drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
new file mode 100644
index 0000000..f3eb805
--- /dev/null
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -0,0 +1,359 @@
+/*
+ * drm kms/fb cma (contiguous memory allocator) helper functions
+ *
+ * Copyright (C) 2012 Analog Device Inc.
+ *   Author: Lars-Peter Clausen <lars@metafoo.de>
+ *
+ * Based on udl_fbdev.c
+ *  Copyright (C) 2012 Red Hat
+ *
+ * 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_crtc.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_fb_cma_helper.h>
+#include <linux/module.h>
+
+struct drm_fb_cma {
+	struct drm_framebuffer	fb;
+	struct drm_gem_cma_obj	*obj;
+};
+
+struct drm_fbdev_cma {
+	struct drm_fb_helper	fb_helper;
+	struct drm_fb_cma	*fb;
+};
+
+static inline struct drm_fbdev_cma *to_fbdev_cma(struct drm_fb_helper *helper)
+{
+	return container_of(helper, struct drm_fbdev_cma, fb_helper);
+}
+
+static inline struct drm_fb_cma *to_fb_cma(struct drm_framebuffer *fb)
+{
+	return container_of(fb, struct drm_fb_cma, fb);
+}
+
+static void drm_fb_cma_destroy(struct drm_framebuffer *fb)
+{
+	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
+
+	if (fb_cma->obj)
+		drm_gem_object_unreference_unlocked(&fb_cma->obj->base);
+
+	drm_framebuffer_cleanup(fb);
+	kfree(fb_cma);
+}
+
+static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
+	struct drm_file *file_priv, unsigned int *handle)
+{
+	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
+
+	return drm_gem_handle_create(file_priv,
+			&fb_cma->obj->base, handle);
+}
+
+static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
+	.destroy	= drm_fb_cma_destroy,
+	.create_handle	= drm_fb_cma_create_handle,
+};
+
+static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
+	struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_cma_obj *obj)
+{
+	struct drm_fb_cma *fb_cma;
+	int ret;
+
+	fb_cma = kzalloc(sizeof(*fb_cma), GFP_KERNEL);
+	if (!fb_cma)
+		return ERR_PTR(-ENOMEM);
+
+	ret = drm_framebuffer_init(dev, &fb_cma->fb, &drm_fb_cma_funcs);
+	if (ret) {
+		dev_err(dev->dev, "Failed to initalize framebuffer: %d\n", ret);
+		return ERR_PTR(ret);
+	}
+
+	drm_helper_mode_fill_fb_struct(&fb_cma->fb, mode_cmd);
+
+	fb_cma->obj = obj;
+
+	return fb_cma;
+}
+
+/**
+ * drm_fb_cma_create() - (struct drm_mode_config_funcs *)->fb_create callback function
+ */
+struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
+	struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	struct drm_fb_cma *fb_cma;
+	struct drm_gem_object *obj;
+
+	obj = drm_gem_object_lookup(dev, file_priv, mode_cmd->handles[0]);
+	if (!obj) {
+		dev_err(dev->dev, "Failed to lookup GEM object\n");
+		return ERR_PTR(-ENXIO);
+	}
+
+	fb_cma = drm_fb_cma_alloc(dev, mode_cmd, to_drm_gem_cma_obj(obj));
+	if (IS_ERR(fb_cma)) {
+		drm_gem_object_unreference_unlocked(obj);
+		return ERR_CAST(fb_cma);
+	}
+
+	return &fb_cma->fb;
+}
+EXPORT_SYMBOL_GPL(drm_fb_cma_create);
+
+/**
+ * drm_fb_cma_get_gem_obj() - Get CMA GEM object for framebuffer
+ * @fb: The framebuffer
+ *
+ * Return the CMA GEM object for given framebuffer.
+ *
+ * This function will usually be called from the CRTC callback functions.
+ */
+struct drm_gem_cma_obj *drm_fb_cma_get_gem_obj(struct drm_framebuffer *fb)
+{
+	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
+
+	return fb_cma->obj;
+}
+EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_obj);
+
+static struct fb_ops drm_fbdev_cma_ops = {
+	.owner		= THIS_MODULE,
+	.fb_fillrect	= sys_fillrect,
+	.fb_copyarea	= sys_copyarea,
+	.fb_imageblit	= sys_imageblit,
+	.fb_check_var	= drm_fb_helper_check_var,
+	.fb_set_par	= drm_fb_helper_set_par,
+	.fb_blank	= drm_fb_helper_blank,
+	.fb_pan_display	= drm_fb_helper_pan_display,
+	.fb_setcmap	= drm_fb_helper_setcmap,
+};
+
+static int drm_fbdev_cma_create(struct drm_fb_helper *helper,
+	struct drm_fb_helper_surface_size *sizes)
+{
+	struct drm_fbdev_cma *fbdev_cma = to_fbdev_cma(helper);
+	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
+	struct drm_device *dev = helper->dev;
+	struct drm_gem_cma_obj *obj;
+	struct drm_framebuffer *fb;
+	unsigned int bytes_per_pixel;
+	unsigned long offset;
+	struct fb_info *fbi;
+	size_t size;
+	int ret;
+
+	DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d\n",
+			sizes->surface_width, sizes->surface_height,
+			sizes->surface_bpp);
+
+	bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
+
+	mode_cmd.width = sizes->surface_width;
+	mode_cmd.height = sizes->surface_height;
+	mode_cmd.pitches[0] = sizes->surface_width * bytes_per_pixel;
+	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
+		sizes->surface_depth);
+
+	size = mode_cmd.pitches[0] * mode_cmd.height;
+	obj = drm_gem_cma_create(dev, size);
+	if (!obj)
+		return -ENOMEM;
+
+	fbi = framebuffer_alloc(0, dev->dev);
+	if (!fbi) {
+		dev_err(dev->dev, "Failed to allocate framebuffer info.\n");
+		ret = -ENOMEM;
+		goto err_drm_gem_cma_free_object;
+	}
+
+	fbdev_cma->fb = drm_fb_cma_alloc(dev, &mode_cmd, obj);
+	if (IS_ERR(fbdev_cma->fb)) {
+		dev_err(dev->dev, "Failed to allocate DRM framebuffer.\n");
+		ret = PTR_ERR(fbdev_cma->fb);
+		goto err_framebuffer_release;
+	}
+
+	fb = &fbdev_cma->fb->fb;
+	helper->fb = fb;
+	helper->fbdev = fbi;
+
+	fbi->par = helper;
+	fbi->flags = FBINFO_FLAG_DEFAULT;
+	fbi->fbops = &drm_fbdev_cma_ops;
+
+	ret = fb_alloc_cmap(&fbi->cmap, 256, 0);
+	if (ret) {
+		dev_err(dev->dev, "Failed to allocate color map.\n");
+		goto err_drm_fb_cma_destroy;
+	}
+
+	drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->depth);
+	drm_fb_helper_fill_var(fbi, helper, fb->width, fb->height);
+
+	offset = fbi->var.xoffset * bytes_per_pixel;
+	offset += fbi->var.yoffset * fb->pitches[0];
+
+	dev->mode_config.fb_base = (resource_size_t)obj->paddr;
+	fbi->screen_base = obj->vaddr + offset;
+	fbi->fix.smem_start = (unsigned long)(obj->paddr + offset);
+	fbi->screen_size = size;
+	fbi->fix.smem_len = size;
+
+	return 0;
+
+err_drm_fb_cma_destroy:
+	drm_fb_cma_destroy(fb);
+err_framebuffer_release:
+	framebuffer_release(fbi);
+err_drm_gem_cma_free_object:
+	drm_gem_cma_free_object(&obj->base);
+	return ret;
+}
+
+static int drm_fbdev_cma_probe(struct drm_fb_helper *helper,
+	struct drm_fb_helper_surface_size *sizes)
+{
+	int ret = 0;
+
+	if (!helper->fb) {
+		ret = drm_fbdev_cma_create(helper, sizes);
+		if (ret < 0)
+			return ret;
+		ret = 1;
+	}
+
+	return ret;
+}
+
+static struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = {
+	.fb_probe = drm_fbdev_cma_probe,
+};
+
+/**
+ * drm_fbdev_cma_init() - Allocate and initlaize a drm_fbdev_cma struct
+ * @dev: DRM device
+ * @prefered_bpp: Prefered bits per pixel for the device
+ * @num_crtc: Number of CRTCs
+ * @max_conn_count: Maximum number of connectors
+ *
+ * Returns a newly allocated drm_fbdev_cma struct or a ERR_PTR.
+ */
+struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, unsigned int prefered_bpp,
+	unsigned int num_crtc, unsigned int max_conn_count)
+{
+	struct drm_fbdev_cma *fbdev_cma;
+	struct drm_fb_helper *helper;
+	int ret;
+
+	fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL);
+	if (!fbdev_cma) {
+		dev_err(dev->dev, "Failed to allocate drm fbdev.\n");
+		return ERR_PTR(-ENOMEM);
+	}
+
+	fbdev_cma->fb_helper.funcs = &drm_fb_cma_helper_funcs;
+	helper = &fbdev_cma->fb_helper;
+
+	ret = drm_fb_helper_init(dev, helper, num_crtc, max_conn_count);
+	if (ret < 0) {
+		dev_err(dev->dev, "Failed to initialize drm fb helper.\n");
+		goto err_free;
+	}
+
+	ret = drm_fb_helper_single_add_all_connectors(helper);
+	if (ret < 0) {
+		dev_err(dev->dev, "Failed to add connectors.\n");
+		goto err_drm_fb_helper_fini;
+
+	}
+
+	ret = drm_fb_helper_initial_config(helper, prefered_bpp);
+	if (ret < 0) {
+		dev_err(dev->dev, "Failed to set inital hw configuration.\n");
+		goto err_drm_fb_helper_fini;
+	}
+
+	return fbdev_cma;
+
+err_drm_fb_helper_fini:
+	drm_fb_helper_fini(helper);
+err_free:
+	kfree(fbdev_cma);
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(drm_fbdev_cma_init);
+
+/**
+ * drm_fbdev_cma_fini() - Free drm_fbdev_cma struct
+ * @fbdev_cma: The drm_fbdev_cma struct
+ */
+void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma)
+{
+	if (fbdev_cma->fb_helper.fbdev) {
+		struct fb_info *info;
+		int ret;
+
+		info = fbdev_cma->fb_helper.fbdev;
+		ret = unregister_framebuffer(info);
+		if (ret < 0)
+			DRM_DEBUG_KMS("failed unregister_framebuffer()\n");
+
+		if (info->cmap.len)
+			fb_dealloc_cmap(&info->cmap);
+
+		framebuffer_release(info);
+	}
+
+	if (fbdev_cma->fb)
+		drm_fb_cma_destroy(&fbdev_cma->fb->fb);
+
+	drm_fb_helper_fini(&fbdev_cma->fb_helper);
+	kfree(fbdev_cma);
+}
+EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini);
+
+/**
+ * drm_fbdev_cma_restore_mode() - Restores inital framebuffer mode
+ * @fbdev_cma: The drm_fbdev_cma struct, may be NULL
+ *
+ * This function is usually called from the DRM drivers lastclose callback.
+ */
+void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma)
+{
+	if (fbdev_cma)
+		drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper);
+}
+EXPORT_SYMBOL_GPL(drm_fbdev_cma_restore_mode);
+
+/**
+ * drm_fbdev_cma_hotplug_event() - Poll for hotpulug events
+ * @fbdev_cma: The drm_fbdev_cma struct, may be NULL
+ *
+ * This function is usually called from the DRM drivers output_poll_changed
+ * callback.
+ */
+void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma)
+{
+	if (fbdev_cma)
+		drm_fb_helper_hotplug_event(&fbdev_cma->fb_helper);
+}
+EXPORT_SYMBOL_GPL(drm_fbdev_cma_hotplug_event);
diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
new file mode 100644
index 0000000..dc90e26
--- /dev/null
+++ b/include/drm/drm_fb_cma_helper.h
@@ -0,0 +1,24 @@
+#ifndef __DRM_FB_CMA_HELPER_H__
+#define __DRM_FB_CMA_HELPER_H__
+
+struct drm_fbdev_cma;
+struct drm_gem_cma_obj;
+
+struct drm_framebuffer;
+struct drm_device;
+struct drm_file;
+struct drm_mode_fb_cmd2;
+
+struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, unsigned int prefered_bpp,
+	unsigned int num_crtc, unsigned int max_conn_count);
+void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma);
+void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
+void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
+
+struct drm_gem_cma_obj *drm_fb_cma_get_gem_obj(struct drm_framebuffer *fb);
+
+struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
+	struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd);
+
+#endif
+
-- 
1.7.10

[PATCH 2/2] DRM: imx: Use new fb cma helper functions

[Lars-Peter Clausen]

---
Sascha, this is against your inital patch and I suppose it does not apply
cleanly anymore to your current tree, but it shouldn't be to hard to make the
changes manually. Can you test whether the generic cma fb helper functions works
for you?
---
 drivers/gpu/drm/imx/Kconfig         |    1 +
 drivers/gpu/drm/imx/Makefile        |    2 +-
 drivers/gpu/drm/imx/imx-drm-core.c  |   13 +-
 drivers/gpu/drm/imx/imx-fb.c        |  137 +--------------------
 drivers/gpu/drm/imx/imx-fbdev.c     |  228 ++---------------------------------
 drivers/gpu/drm/imx/imx-lcdc-crtc.c |   10 +-
 6 files changed, 24 insertions(+), 367 deletions(-)

diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
index 5fc3a44..6b5e3ef 100644
--- a/drivers/gpu/drm/imx/Kconfig
+++ b/drivers/gpu/drm/imx/Kconfig
@@ -5,6 +5,7 @@ config DRM_IMX
 
 config DRM_IMX_FB_HELPER
 	tristate "provide legacy framebuffer /dev/fb0"
+	select DRM_KMS_CMA_HELPER
 	depends on DRM_IMX
 
 config DRM_IMX_LCDC
diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
index 0f7c038..6c7dd2d 100644
--- a/drivers/gpu/drm/imx/Makefile
+++ b/drivers/gpu/drm/imx/Makefile
@@ -1,5 +1,5 @@
 
-imxdrm-objs := imx-drm-core.o imx-fb.o imx-gem.o
+imxdrm-objs := imx-drm-core.o imx-fb.o
 
 obj-$(CONFIG_DRM_IMX) += imxdrm.o
 
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 29f5f10..68cf4da 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -22,6 +22,7 @@
 #include <linux/fb.h>
 #include <asm/fb.h>
 #include <linux/module.h>
+#include <drm/drm_gem_cma_helper.h>
 
 #include "imx-drm.h"
 
@@ -149,7 +150,7 @@ static void imx_drm_disable_vblank(struct drm_device *drm, int crtc)
 }
 
 static struct vm_operations_struct imx_drm_gem_vm_ops = {
-	.fault = imx_drm_gem_fault,
+	.fault = drm_gem_cma_fault,
 	.open = drm_gem_vm_open,
 	.close = drm_gem_vm_close,
 };
@@ -159,7 +160,7 @@ static const struct file_operations imx_drm_driver_fops = {
 	.open = drm_open,
 	.release = drm_release,
 	.unlocked_ioctl = drm_ioctl,
-	.mmap = imx_drm_gem_mmap,
+	.mmap = drm_gem_cma_mmap,
 	.poll = drm_poll,
 	.fasync = drm_fasync,
 	.read = drm_read,
@@ -646,11 +647,11 @@ static struct drm_driver imx_drm_driver = {
 	.unload			= imx_drm_driver_unload,
 	.firstopen		= imx_drm_driver_firstopen,
 	.lastclose		= imx_drm_driver_lastclose,
-	.gem_free_object	= imx_drm_gem_free_object,
+	.gem_free_object	= drm_gem_cma_free_object,
 	.gem_vm_ops		= &imx_drm_gem_vm_ops,
-	.dumb_create		= imx_drm_gem_dumb_create,
-	.dumb_map_offset	= imx_drm_gem_dumb_map_offset,
-	.dumb_destroy		= imx_drm_gem_dumb_destroy,
+	.dumb_create		= drm_gem_cma_dumb_create,
+	.dumb_map_offset	= drm_gem_cma_dumb_map_offset,
+	.dumb_destroy		= drm_gem_cma_dumb_destroy,
 
 	.get_vblank_counter	= drm_vblank_count,
 	.enable_vblank		= imx_drm_enable_vblank,
diff --git a/drivers/gpu/drm/imx/imx-fb.c b/drivers/gpu/drm/imx/imx-fb.c
index 5a08c86..dde738b 100644
--- a/drivers/gpu/drm/imx/imx-fb.c
+++ b/drivers/gpu/drm/imx/imx-fb.c
@@ -21,145 +21,12 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_cma_helper.h>
 
 #include "imx-drm.h"
 
-#define to_imx_drm_fb(x)	container_of(x, struct imx_drm_fb, fb)
-
-/*
- * imx specific framebuffer structure.
- *
- * @fb: drm framebuffer obejct.
- * @imx_drm_gem_obj: drm ec specific gem object containing a gem object.
- * @entry: pointer to ec drm buffer entry object.
- *	- containing only the information to physically continuous memory
- *	region allocated at default framebuffer creation.
- */
-struct imx_drm_fb {
-	struct drm_framebuffer		fb;
-	struct imx_drm_gem_obj	*imx_drm_gem_obj;
-	struct imx_drm_buf_entry	*entry;
-};
-
-static void imx_drm_fb_destroy(struct drm_framebuffer *fb)
-{
-	struct imx_drm_fb *imx_drm_fb = to_imx_drm_fb(fb);
-
-	drm_framebuffer_cleanup(fb);
-
-	/*
-	 * default framebuffer has no gem object so
-	 * a buffer of the default framebuffer should be released at here.
-	 */
-	if (!imx_drm_fb->imx_drm_gem_obj && imx_drm_fb->entry)
-		imx_drm_buf_destroy(fb->dev, imx_drm_fb->entry);
-
-	kfree(imx_drm_fb);
-}
-
-static int imx_drm_fb_create_handle(struct drm_framebuffer *fb,
-		struct drm_file *file_priv, unsigned int *handle)
-{
-	struct imx_drm_fb *imx_drm_fb = to_imx_drm_fb(fb);
-
-	return drm_gem_handle_create(file_priv,
-			&imx_drm_fb->imx_drm_gem_obj->base, handle);
-}
-
-static struct drm_framebuffer_funcs imx_drm_fb_funcs = {
-	.destroy	= imx_drm_fb_destroy,
-	.create_handle	= imx_drm_fb_create_handle,
-};
-
-static struct drm_framebuffer *imx_drm_fb_create(struct drm_device *dev,
-		struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd)
-{
-	struct imx_drm_fb *imx_drm_fb;
-	struct drm_framebuffer *fb;
-	struct drm_gem_object *obj;
-	unsigned int size;
-	int ret;
-	u32 bpp, depth;
-
-	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp);
-
-	mode_cmd->pitches[0] = max(mode_cmd->pitches[0],
-			mode_cmd->width * (bpp >> 3));
-
-	dev_dbg(dev->dev, "drm fb create(%dx%d)\n",
-			mode_cmd->width, mode_cmd->height);
-
-	imx_drm_fb = kzalloc(sizeof(*imx_drm_fb), GFP_KERNEL);
-	if (!imx_drm_fb) {
-		dev_err(dev->dev, "failed to allocate drm framebuffer.\n");
-		return ERR_PTR(-ENOMEM);
-	}
-
-	fb = &imx_drm_fb->fb;
-	ret = drm_framebuffer_init(dev, fb, &imx_drm_fb_funcs);
-	if (ret) {
-		dev_err(dev->dev, "failed to initialize framebuffer.\n");
-		goto err_init;
-	}
-
-	dev_dbg(dev->dev, "create: fb id: %d\n", fb->base.id);
-
-	size = mode_cmd->pitches[0] * mode_cmd->height;
-
-	/*
-	 * without file_priv we are called from imx_drm_fbdev_create in which
-	 * case we only create a framebuffer without a handle.
-	 */
-	if (!file_priv) {
-		struct imx_drm_buf_entry *entry;
-
-		entry = imx_drm_buf_create(dev, size);
-		if (IS_ERR(entry)) {
-			ret = PTR_ERR(entry);
-			goto err_buffer;
-		}
-
-		imx_drm_fb->entry = entry;
-	} else {
-		obj = drm_gem_object_lookup(dev, file_priv, mode_cmd->handles[0]);
-		if (!obj) {
-			ret = -EINVAL;
-			goto err_buffer;
-		}
-
-		imx_drm_fb->imx_drm_gem_obj = to_imx_drm_gem_obj(obj);
-
-		drm_gem_object_unreference_unlocked(obj);
-
-		imx_drm_fb->entry = imx_drm_fb->imx_drm_gem_obj->entry;
-	}
-
-	drm_helper_mode_fill_fb_struct(fb, mode_cmd);
-
-	return fb;
-
-err_buffer:
-	drm_framebuffer_cleanup(fb);
-
-err_init:
-	kfree(imx_drm_fb);
-
-	return ERR_PTR(ret);
-}
-
-struct imx_drm_buf_entry *imx_drm_fb_get_buf(struct drm_framebuffer *fb)
-{
-	struct imx_drm_fb *imx_drm_fb = to_imx_drm_fb(fb);
-	struct imx_drm_buf_entry *entry;
-
-	entry = imx_drm_fb->entry;
-
-	return entry;
-}
-EXPORT_SYMBOL_GPL(imx_drm_fb_get_buf);
-
 static struct drm_mode_config_funcs imx_drm_mode_config_funcs = {
-	.fb_create = imx_drm_fb_create,
+	.fb_create = drm_fb_cma_create,
 };
 
 void imx_drm_mode_config_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/imx/imx-fbdev.c b/drivers/gpu/drm/imx/imx-fbdev.c
index f038797..a0e7ccd 100644
--- a/drivers/gpu/drm/imx/imx-fbdev.c
+++ b/drivers/gpu/drm/imx/imx-fbdev.c
@@ -20,229 +20,14 @@
 #include <linux/module.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
-#include <drm/drm_fb_helper.h>
-#include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_cma_helper.h>
 
 #include "imx-drm.h"
 
 #define MAX_CONNECTOR		4
 #define PREFERRED_BPP		16
 
-static struct fb_ops imx_drm_fb_ops = {
-	.owner		= THIS_MODULE,
-	.fb_fillrect	= cfb_fillrect,
-	.fb_copyarea	= cfb_copyarea,
-	.fb_imageblit	= cfb_imageblit,
-	.fb_check_var	= drm_fb_helper_check_var,
-	.fb_set_par	= drm_fb_helper_set_par,
-	.fb_blank	= drm_fb_helper_blank,
-	.fb_pan_display	= drm_fb_helper_pan_display,
-	.fb_setcmap	= drm_fb_helper_setcmap,
-};
-
-static int imx_drm_fbdev_update(struct drm_fb_helper *helper,
-				     struct drm_framebuffer *fb,
-				     unsigned int fb_width,
-				     unsigned int fb_height)
-{
-	struct fb_info *fbi = helper->fbdev;
-	struct drm_device *drm = helper->dev;
-	struct imx_drm_buf_entry *entry;
-	unsigned int size = fb_width * fb_height * (fb->bits_per_pixel >> 3);
-	unsigned long offset;
-
-	drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->depth);
-	drm_fb_helper_fill_var(fbi, helper, fb_width, fb_height);
-
-	entry = imx_drm_fb_get_buf(fb);
-	if (!entry) {
-		dev_dbg(drm->dev, "entry is null.\n");
-		return -EFAULT;
-	}
-
-	offset = fbi->var.xoffset * (fb->bits_per_pixel >> 3);
-	offset += fbi->var.yoffset * fb->pitches[0];
-
-	drm->mode_config.fb_base = entry->paddr;
-	fbi->screen_base = entry->vaddr + offset;
-	fbi->fix.smem_start = entry->paddr + offset;
-	fbi->screen_size = size;
-	fbi->fix.smem_len = size;
-	fbi->flags |= FBINFO_CAN_FORCE_OUTPUT;
-
-	return 0;
-}
-
-static int imx_drm_fbdev_create(struct drm_fb_helper *helper,
-				    struct drm_fb_helper_surface_size *sizes)
-{
-	struct drm_device *drm = helper->dev;
-	struct fb_info *fbi;
-	struct drm_framebuffer *fb;
-	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
-	struct platform_device *pdev = drm->platformdev;
-	int ret;
-
-	dev_dbg(drm->dev, "surface width(%d), height(%d) and bpp(%d\n",
-			sizes->surface_width, sizes->surface_height,
-			sizes->surface_bpp);
-
-	mode_cmd.width = sizes->surface_width;
-	mode_cmd.height = sizes->surface_height;
-	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
-			sizes->surface_depth);
-
-	mutex_lock(&drm->struct_mutex);
-
-	fbi = framebuffer_alloc(0, &pdev->dev);
-	if (!fbi) {
-		ret = -ENOMEM;
-		goto err_fb_alloc;
-	}
-
-	fb = drm->mode_config.funcs->fb_create(drm, NULL, &mode_cmd);
-	if (IS_ERR(fb)) {
-		dev_err(drm->dev, "failed to create drm framebuffer.\n");
-		ret = PTR_ERR(fb);
-		goto err_fb_create;
-	}
-
-	helper->fb = fb;
-	helper->fbdev = fbi;
-
-	fbi->par = helper;
-	fbi->flags = FBINFO_FLAG_DEFAULT;
-	fbi->fbops = &imx_drm_fb_ops;
-
-	ret = fb_alloc_cmap(&fbi->cmap, 256, 0);
-	if (ret)
-		goto err_alloc_cmap;
-
-	ret = imx_drm_fbdev_update(helper, helper->fb, sizes->fb_width,
-			sizes->fb_height);
-	if (ret)
-		goto err_fbdev_update;
-
-	mutex_unlock(&drm->struct_mutex);
-
-	return 0;
-
-err_fbdev_update:
-	fb_dealloc_cmap(&fbi->cmap);
-
-err_alloc_cmap:
-	fb->funcs->destroy(fb);
-
-err_fb_create:
-	framebuffer_release(fbi);
-
-err_fb_alloc:
-	mutex_unlock(&drm->struct_mutex);
-
-	return ret;
-}
-
-static int imx_drm_fbdev_probe(struct drm_fb_helper *helper,
-				   struct drm_fb_helper_surface_size *sizes)
-{
-	int ret;
-
-	BUG_ON(helper->fb);
-
-	ret = imx_drm_fbdev_create(helper, sizes);
-	if (ret) {
-		dev_err(helper->dev->dev, "creating fbdev failed with %d\n", ret);
-		return ret;
-	}
-
-	/*
-	 * fb_helper expects a value more than 1 if succeed
-	 * because register_framebuffer() should be called.
-	 */
-	return 1;
-}
-
-static struct drm_fb_helper_funcs imx_drm_fb_helper_funcs = {
-	.fb_probe = imx_drm_fbdev_probe,
-};
-
-static struct drm_fb_helper *imx_drm_fbdev_init(struct drm_device *drm,
-		int preferred_bpp)
-{
-	struct drm_fb_helper *helper;
-	unsigned int num_crtc;
-	int ret;
-
-	helper = kzalloc(sizeof(*helper), GFP_KERNEL);
-	if (!helper)
-		return NULL;
-
-	helper->funcs = &imx_drm_fb_helper_funcs;
-
-	num_crtc = drm->mode_config.num_crtc;
-
-	ret = drm_fb_helper_init(drm, helper, num_crtc, MAX_CONNECTOR);
-	if (ret) {
-		dev_err(drm->dev, "initializing drm fb helper failed with %d\n",
-				ret);
-		goto err_init;
-	}
-
-	ret = drm_fb_helper_single_add_all_connectors(helper);
-	if (ret) {
-		dev_err(drm->dev, "registering drm_fb_helper_connector failed with %d\n",
-				ret);
-		goto err_setup;
-
-	}
-
-	ret = drm_fb_helper_initial_config(helper, preferred_bpp);
-	if (ret) {
-		dev_err(drm->dev, "initial config failed with %d\n", ret);
-		goto err_setup;
-	}
-
-	return helper;
-
-err_setup:
-	drm_fb_helper_fini(helper);
-
-err_init:
-	kfree(helper);
-
-	return NULL;
-}
-
-static void imx_drm_fbdev_fini(struct drm_fb_helper *helper)
-{
-	struct imx_drm_buf_entry *entry;
-
-	if (helper->fbdev) {
-		struct fb_info *info;
-		int ret;
-
-		info = helper->fbdev;
-		ret = unregister_framebuffer(info);
-		if (ret)
-			dev_err(helper->dev->dev, "unregister_framebuffer failed with %d\n",
-					ret);
-
-		if (info->cmap.len)
-			fb_dealloc_cmap(&info->cmap);
-
-		entry = imx_drm_fb_get_buf(helper->fb);
-
-		imx_drm_buf_destroy(helper->dev, entry);
-
-		framebuffer_release(info);
-	}
-
-	drm_fb_helper_fini(helper);
-
-	kfree(helper);
-}
-
-static struct drm_fb_helper *imx_fb_helper;
+static struct drm_fbdev_cma *fbdev_cma;
 
 static int __init imx_fb_helper_init(void)
 {
@@ -252,10 +37,11 @@ static int __init imx_fb_helper_init(void)
 	if (!drm)
 		return -EINVAL;
 
-	imx_fb_helper = imx_drm_fbdev_init(drm, PREFERRED_BPP);
-	if (!imx_fb_helper) {
+	fbdev_cma = drm_fbdev_cma_init(drm, PREFERRED_BPP, drm->mode_config.num_crtc, MAX_CONNECTOR);
+
+	if (IS_ERR(fbdev_cma)) {
 		imx_drm_device_put();
-		return -EINVAL;
+		return PTR_ERR(fbdev_cma);
 	}
 
 	return 0;
@@ -263,7 +49,7 @@ static int __init imx_fb_helper_init(void)
 
 static void __exit imx_fb_helper_exit(void)
 {
-	imx_drm_fbdev_fini(imx_fb_helper);
+	drm_fbdev_cma_fini(fbdev_cma);
 	imx_drm_device_put();
 }
 
diff --git a/drivers/gpu/drm/imx/imx-lcdc-crtc.c b/drivers/gpu/drm/imx/imx-lcdc-crtc.c
index e77c015..eb066df 100644
--- a/drivers/gpu/drm/imx/imx-lcdc-crtc.c
+++ b/drivers/gpu/drm/imx/imx-lcdc-crtc.c
@@ -18,6 +18,8 @@
 #include <drm/drmP.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_fb_cma_helper.h>
 #include <linux/fb.h>
 #include <linux/clk.h>
 #include <asm/fb.h>
@@ -192,15 +194,15 @@ static int imx_drm_crtc_set(struct drm_crtc *crtc,
 static int imx_drm_set_base(struct drm_crtc *crtc, int x, int y)
 {
 	struct imx_crtc *imx_crtc = to_imx_crtc(crtc);
-	struct imx_drm_buf_entry *entry;
+	struct drm_gem_cma_obj *obj;
 	struct drm_framebuffer *fb = crtc->fb;
 	unsigned long phys;
 
-	entry = imx_drm_fb_get_buf(fb);
-	if (!entry)
+	obj = drm_fb_cma_get_gem_obj(fb);
+	if (!obj)
 		return -EFAULT;
 
-	phys = entry->paddr;
+	phys = obj->paddr;
 	phys += x * (fb->bits_per_pixel >> 3);
 	phys += y * fb->pitches[0];
 
-- 
1.7.10

Re: [PATCH 1/2] DRM: Add DRM kms/fb cma helper

[Sascha Hauer]

On Tue, May 29, 2012 at 08:20:35PM +0200, Lars-Peter Clausen wrote:
> This patchset introduces a set of helper function for implementing the KMS
> framebuffer layer for drivers which use the drm gem CMA helper function.

I just integrated this into my series. Works great, thanks.
Would be great to have this mainline.

Only some nitpicking and one missing kfree below.

Sascha

> +static void drm_fb_cma_destroy(struct drm_framebuffer *fb)
> +{
> +	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
> +
> +	if (fb_cma->obj)
> +		drm_gem_object_unreference_unlocked(&fb_cma->obj->base);
> +
> +	drm_framebuffer_cleanup(fb);
> +	kfree(fb_cma);
> +}
> +
> +static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
> +	struct drm_file *file_priv, unsigned int *handle)
> +{
> +	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
> +
> +	return drm_gem_handle_create(file_priv,
> +			&fb_cma->obj->base, handle);
> +}
> +
> +static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
> +	.destroy	= drm_fb_cma_destroy,
> +	.create_handle	= drm_fb_cma_create_handle,
> +};
> +
> +static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
> +	struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_cma_obj *obj)
> +{
> +	struct drm_fb_cma *fb_cma;
> +	int ret;
> +
> +	fb_cma = kzalloc(sizeof(*fb_cma), GFP_KERNEL);
> +	if (!fb_cma)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ret = drm_framebuffer_init(dev, &fb_cma->fb, &drm_fb_cma_funcs);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to initalize framebuffer: %d\n", ret);

s/initalize/initialize/

kfree(fb_cma)?

> +		return ERR_PTR(ret);
> +	}
> +
> +
> +/**
> + * drm_fbdev_cma_init() - Allocate and initlaize a drm_fbdev_cma struct

s/initlaize/initialize/

> + * @dev: DRM device
> + * @prefered_bpp: Prefered bits per pixel for the device

s/prefered/preferred/

> + * @num_crtc: Number of CRTCs
> + * @max_conn_count: Maximum number of connectors
> + *
> + * Returns a newly allocated drm_fbdev_cma struct or a ERR_PTR.
> + */

-- 
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] DRM: add drm gem CMA helper

[Laurent Pinchart]

Hi Sascha,

Thank you for the patch. I've successfully tested the helper with the new SH 
Mobile DRM driver. Just a couple of comments below in addition to Lars' 
comments (this is not a full review, just details that caught my attention 
when comparing the code with my implementation, and trying to use it).

On Tuesday 29 May 2012 16:10:27 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>
> ---
> 
> Lars-Peter, please let me know if this fits your needs or if you are missing
> something.
> 
>  drivers/gpu/drm/Kconfig              |    6 +
>  drivers/gpu/drm/Makefile             |    1 +
>  drivers/gpu/drm/drm_gem_cma_helper.c |  321 +++++++++++++++++++++++++++++++
>  include/drm/drm_gem_cma_helper.h     |   47 +++++
>  4 files changed, 375 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_gem_cma_helper.c
>  create mode 100644 include/drm/drm_gem_cma_helper.h

[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..75534ff
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -0,0 +1,321 @@

[snip]

> +/*
> + * drm_gem_cma_create - allocate an object with the given size
> + *
> + * returns a struct drm_gem_cma_obj* on success or ERR_PTR values
> + * on failure.
> + */
> +struct drm_gem_cma_obj *drm_gem_cma_create(struct drm_device *drm,
> +		unsigned int size)
> +{
> +	struct drm_gem_cma_obj *cma_obj;
> +	struct drm_gem_object *gem_obj;
> +	int ret;
> +
> +	size = roundup(size, PAGE_SIZE);

As PAGE_SIZE is guaranteed to be a power of two, you can use round_up, which 
should be faster.

> +
> +	cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL);
> +	if (!cma_obj)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cma_obj->vaddr = dma_alloc_writecombine(drm->dev, size,
> +			(dma_addr_t *)&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);

[snip]

> +/*
> + * drm_gem_cma_mmap - (struct file_operation)->mmap callback function
> + */
> +int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
> +{
> +	int ret;
> +
> +	ret = drm_gem_mmap(filp, vma);
> +	if (ret)
> +		return ret;
> +
> +	vma->vm_flags &= ~VM_PFNMAP;
> +	vma->vm_flags |= VM_MIXEDMAP;

Why is this a mixed map ?

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);

My implementation maps the whole buffer in one go at mmap time, not page by 
page at page fault time. Isn't that more efficient when mapping frame buffer 
memory ?

[snip]

> diff --git a/include/drm/drm_gem_cma_helper.h
> b/include/drm/drm_gem_cma_helper.h new file mode 100644
> index 0000000..53b007c
> --- /dev/null
> +++ b/include/drm/drm_gem_cma_helper.h
> @@ -0,0 +1,47 @@
> +#ifndef __DRM_GEM_DMA_ALLOC_HELPER_H__
> +#define __DRM_GEM_DMA_ALLOC_HELPER_H__

Should this be __DRM_GEM_CMA_HELPER_H__ ?

> +
> +struct drm_gem_cma_obj {
> +	struct drm_gem_object base;
> +	dma_addr_t paddr;
> +	void __iomem *vaddr;
> +};

All drm objects end with _object, would it be better to rename this to struct 
drm_gem_cma_object ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] DRM: add drm gem CMA helper

[Sascha Hauer]

On Wed, May 30, 2012 at 05:40:13PM +0200, Laurent Pinchart wrote:
> Hi Sascha,
> 
> Thank you for the patch. I've successfully tested the helper with the new SH 
> Mobile DRM driver. Just a couple of comments below in addition to Lars' 
> comments (this is not a full review, just details that caught my attention 
> when comparing the code with my implementation, and trying to use it).
> 
> > +/*
> > + * drm_gem_cma_mmap - (struct file_operation)->mmap callback function
> > + */
> > +int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
> > +{
> > +	int ret;
> > +
> > +	ret = drm_gem_mmap(filp, vma);
> > +	if (ret)
> > +		return ret;
> > +
> > +	vma->vm_flags &= ~VM_PFNMAP;
> > +	vma->vm_flags |= VM_MIXEDMAP;
> 
> Why is this a mixed map ?

Honestly, I don't know. This is copied from the exynos driver. I don't
know much about these flags, so if you think something else is better
here than you're probably right ;)

> 
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
> 
> My implementation maps the whole buffer in one go at mmap time, not page by 
> page at page fault time. Isn't that more efficient when mapping frame buffer 
> memory ?

I remember Alan has mentioned this in an earlier review. I'll update
the patch accordingly.

I will fix this and the other things you mentioned and repost.

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] DRM: add drm gem CMA helper

[Laurent Pinchart]

Hi Sascha,

On Wednesday 30 May 2012 18:28:12 Sascha Hauer wrote:
> On Wed, May 30, 2012 at 05:40:13PM +0200, Laurent Pinchart wrote:
> > Hi Sascha,
> > 
> > Thank you for the patch. I've successfully tested the helper with the new
> > SH Mobile DRM driver. Just a couple of comments below in addition to
> > Lars' comments (this is not a full review, just details that caught my
> > attention when comparing the code with my implementation, and trying to
> > use it).
> > > +/*
> > > + * drm_gem_cma_mmap - (struct file_operation)->mmap callback function
> > > + */
> > > +int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = drm_gem_mmap(filp, vma);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	vma->vm_flags &= ~VM_PFNMAP;
> > > +	vma->vm_flags |= VM_MIXEDMAP;
> > 
> > Why is this a mixed map ?
> 
> Honestly, I don't know. This is copied from the exynos driver. I don't
> know much about these flags, so if you think something else is better
> here than you're probably right ;)

I wouldn't claim to be an expert on this matter :-) As far as I know, PFNMAP 
means that the mapping refers to physical memory with no struct page 
associated with it (that could be I/O memory, DRAM reserved at boot time, ... 
basically any memory outside of the system memory resources controlled by the 
kernel page allocator). MIXEDMAP means that the mapping refers to memory that 
contains both PFNMAP regions and non-PFNMAP regions. I would be surprised if 
dma_alloc_writecombine() returned such a mix. On the other hand the memory it 
returns might be PFNMAP or non-PFNMAP depending on the underneath allocator, 
maybe that's why VM_MIXEDMAP was used.

I'd appreciate an expert's opinion on this :-)

> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
> > 
> > My implementation maps the whole buffer in one go at mmap time, not page
> > by page at page fault time. Isn't that more efficient when mapping frame
> > buffer memory ?
> 
> I remember Alan has mentioned this in an earlier review. I'll update the
> patch accordingly.
> 
> I will fix this and the other things you mentioned and repost.

-- 
Regards,

Laurent Pinchart

[PATCH] 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. The code technically does
not depend on CMA as the backend allocator, the name has been chosen
because cma makes for a nice, short but still descriptive function
prefix.

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

changes since v2:
- make drm_gem_cma_create_with_handle static
- use DIV_ROUND_UP(args->width * args->bpp, 8) to calculate the pitch
- make drm_gem_cma_vm_ops const
- add missing #include <linux/export.h>
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

 drivers/gpu/drm/Kconfig              |    6 +
 drivers/gpu/drm/Makefile             |    1 +
 drivers/gpu/drm/drm_gem_cma_helper.c |  250 ++++++++++++++++++++++++++++++++++
 include/drm/drm_gem_cma_helper.h     |   44 ++++++
 4 files changed, 301 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 dc5df12..8f362ec 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 0487ff6..8759cda 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..417f45e5
--- /dev/null
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -0,0 +1,250 @@
+/*
+ * 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 <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/mutex.h>
+#include <linux/export.h>
+#include <linux/dma-mapping.h>
+
+#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.
+ */
+static 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);
+
+const 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..f0f6b1a
--- /dev/null
+++ b/include/drm/drm_gem_cma_helper.h
@@ -0,0 +1,44 @@
+#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);
+
+/* 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);
+
+extern const struct vm_operations_struct drm_gem_cma_vm_ops;
+
+#endif /* __DRM_GEM_CMA_HELPER_H__ */
-- 
1.7.10

Re: [PATCH] DRM: add drm gem CMA helper

[Laurent Pinchart]

Hi Sascha,

Thanks for the patch.

On Wednesday 27 June 2012 15:00:05 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. The code technically does
> not depend on CMA as the backend allocator, the name has been chosen
> because cma makes for a nice, short but still descriptive function
> prefix.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> 
> changes since v2:
> - make drm_gem_cma_create_with_handle static
> - use DIV_ROUND_UP(args->width * args->bpp, 8) to calculate the pitch

[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..417f45e5
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -0,0 +1,250 @@

[snip]

> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/export.h>
> +#include <linux/dma-mapping.h>

Nitpicking, I usually sort headers alphabetically, that helps locating 
duplicates quickly. It's not mandatory though.

[snip]

> +/*
> + * 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);

It's nice to mention in the changelog that you fixed the problem, but it would 
be even nicer to really fix it ;-)

> +
> +	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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH] DRM: add drm gem CMA helper

[Sascha Hauer]

On Wed, Jun 27, 2012 at 03:20:27PM +0200, Laurent Pinchart wrote:
> Hi Sascha,
> 
> Thanks for the patch.
> 
> On Wednesday 27 June 2012 15:00:05 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. The code technically does
> > not depend on CMA as the backend allocator, the name has been chosen
> > because cma makes for a nice, short but still descriptive function
> > prefix.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> > 
> > changes since v2:
> > - make drm_gem_cma_create_with_handle static
> > - use DIV_ROUND_UP(args->width * args->bpp, 8) to calculate the pitch
> 
> [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..417f45e5
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> > @@ -0,0 +1,250 @@
> 
> [snip]
> 
> > +#include <linux/mm.h>
> > +#include <linux/slab.h>
> > +#include <linux/mutex.h>
> > +#include <linux/export.h>
> > +#include <linux/dma-mapping.h>
> 
> Nitpicking, I usually sort headers alphabetically, that helps locating 
> duplicates quickly. It's not mandatory though.
> 
> [snip]
> 
> > +/*
> > + * 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);
> 
> It's nice to mention in the changelog that you fixed the problem, but it would 
> be even nicer to really fix it ;-)

Damn ;)

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 |