[PATCH v2] DRM: Add DRM kms/fb cma helper

[PATCH v2] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

This patch 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>

---
Changes since v1:
	* Some spelling fixes
	* Add missing kfree in drm_fb_cma_alloc error path
	* Add multi-plane support. drm_fb_cma_get_gem_obj now takes a second
	  parameter to specify the plane.
---
 drivers/gpu/drm/Kconfig             |   11 +
 drivers/gpu/drm/Makefile            |    1 +
 drivers/gpu/drm/drm_fb_cma_helper.c |  384 +++++++++++++++++++++++++++++++++++
 include/drm/drm_fb_cma_helper.h     |   27 +++
 4 files changed, 423 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 a78dfbe..e7c0a3d 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 6e9e948..5dcb1a5 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..8821a98
--- /dev/null
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -0,0 +1,384 @@
+/*
+ * 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[4];
+};
+
+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);
+	int i;
+
+	for (i = 0; i < 4; i++) {
+		if (fb_cma->obj[i])
+			drm_gem_object_unreference_unlocked(&fb_cma->obj[i]->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[0]->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,
+	unsigned int num_planes)
+{
+	struct drm_fb_cma *fb_cma;
+	int ret;
+	int i;
+
+	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);
+		kfree(fb_cma);
+		return ERR_PTR(ret);
+	}
+
+	drm_helper_mode_fill_fb_struct(&fb_cma->fb, mode_cmd);
+
+	for (i = 0; i < num_planes; i++)
+		fb_cma->obj[i] = obj[i];
+
+	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_cma_obj *objs[4];
+	struct drm_gem_object *obj;
+	int ret;
+	int i;
+
+	for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) {
+		obj = drm_gem_object_lookup(dev, file_priv, mode_cmd->handles[i]);
+		if (!obj) {
+			dev_err(dev->dev, "Failed to lookup GEM object\n");
+			ret = -ENXIO;
+			goto err_gem_object_unreference;
+		}
+		objs[i] = to_drm_gem_cma_obj(obj);
+	}
+
+	fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i);
+	if (IS_ERR(fb_cma)) {
+		ret = PTR_ERR(fb_cma);
+		goto err_gem_object_unreference;
+	}
+
+	return &fb_cma->fb;
+
+err_gem_object_unreference:
+	for (i--; i >= 0; i--)
+		drm_gem_object_unreference_unlocked(&objs[i]->base);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(drm_fb_cma_create);
+
+/**
+ * drm_fb_cma_get_gem_obj() - Get CMA GEM object for framebuffer
+ * @fb: The framebuffer
+ * @plane: Which plane
+ *
+ * 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,
+	unsigned int plane)
+{
+	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
+
+	if (plane >= 4)
+		return NULL;
+
+	return fb_cma->obj[plane];
+}
+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, 1);
+	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 initializes a drm_fbdev_cma struct
+ * @dev: DRM device
+ * @preferred_bpp: Preferred 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 preferred_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, preferred_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 initial 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..7317beb
--- /dev/null
+++ b/include/drm/drm_fb_cma_helper.h
@@ -0,0 +1,27 @@
+#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 preferred_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_framebuffer *drm_fb_cma_create(struct drm_device *dev,
+	struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd);
+
+struct drm_gem_cma_obj *drm_fb_cma_get_gem_obj(struct drm_framebuffer *fb,
+	unsigned int plane);
+
+#endif
+
-- 
1.7.10

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

[Sascha Hauer]

On Wed, May 30, 2012 at 05:30:15PM +0200, Lars-Peter Clausen wrote:
> This patch 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>
> 
> ---
> Changes since v1:
> 	* Some spelling fixes
> 	* Add missing kfree in drm_fb_cma_alloc error path
> 	* Add multi-plane support. drm_fb_cma_get_gem_obj now takes a second
> 	  parameter to specify the plane.
> ---
>  drivers/gpu/drm/Kconfig             |   11 +
>  drivers/gpu/drm/Makefile            |    1 +
>  drivers/gpu/drm/drm_fb_cma_helper.c |  384 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_fb_cma_helper.h     |   27 +++
>  4 files changed, 423 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 a78dfbe..e7c0a3d 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 6e9e948..5dcb1a5 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..8821a98
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -0,0 +1,384 @@
> +/*
> + * 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[4];

Can we have a define for this magic '4'? It is used several times in
this patch.

> +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,
> +	unsigned int num_planes)
> +{
> +	struct drm_fb_cma *fb_cma;
> +	int ret;
> +	int i;
> +
> +	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);
> +		kfree(fb_cma);
> +		return ERR_PTR(ret);
> +	}
> +
> +	drm_helper_mode_fill_fb_struct(&fb_cma->fb, mode_cmd);
> +
> +	for (i = 0; i < num_planes; i++)
> +		fb_cma->obj[i] = obj[i];

Check for valid num_planes before this loop?


Note I have renamed struct drm_gem_cma_obj to struct drm_gem_cma_object
in the patch I just sent.

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 kms/fb cma helper

[Lars-Peter Clausen]

On 05/31/2012 10:13 AM, Sascha Hauer wrote:
> On Wed, May 30, 2012 at 05:30:15PM +0200, Lars-Peter Clausen wrote:
>> This patch 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>
>>
>> ---
>> Changes since v1:
>> 	* Some spelling fixes
>> 	* Add missing kfree in drm_fb_cma_alloc error path
>> 	* Add multi-plane support. drm_fb_cma_get_gem_obj now takes a second
>> 	  parameter to specify the plane.
>> ---
>>  drivers/gpu/drm/Kconfig             |   11 +
>>  drivers/gpu/drm/Makefile            |    1 +
>>  drivers/gpu/drm/drm_fb_cma_helper.c |  384 +++++++++++++++++++++++++++++++++++
>>  include/drm/drm_fb_cma_helper.h     |   27 +++
>>  4 files changed, 423 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 a78dfbe..e7c0a3d 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 6e9e948..5dcb1a5 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..8821a98
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> @@ -0,0 +1,384 @@
>> +/*
>> + * 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[4];
> 
> Can we have a define for this magic '4'? It is used several times in
> this patch.

Yes, had the same though. The magic 4 is actually used all through all of DRM.
Something like '#define DRM_MAX_PLANES 4' probably makes sense.

> 
>> +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,
>> +	unsigned int num_planes)
>> +{
>> +	struct drm_fb_cma *fb_cma;
>> +	int ret;
>> +	int i;
>> +
>> +	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);
>> +		kfree(fb_cma);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	drm_helper_mode_fill_fb_struct(&fb_cma->fb, mode_cmd);
>> +
>> +	for (i = 0; i < num_planes; i++)
>> +		fb_cma->obj[i] = obj[i];
> 
> Check for valid num_planes before this loop?
> 

Hm, I think the callers already take care of this. drm_format_num_planes will
always return a valid number and the other caller passes 1 unconditionally.

> 
> Note I have renamed struct drm_gem_cma_obj to struct drm_gem_cma_object
> in the patch I just sent.
> 

Do you think it makes sense for you to carry this patch as part of your iMX DRM
series?

Thanks,
- Lars

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

[Sascha Hauer]

On Thu, May 31, 2012 at 11:34:37AM +0200, Lars-Peter Clausen wrote:
> >> +	drm_helper_mode_fill_fb_struct(&fb_cma->fb, mode_cmd);
> >> +
> >> +	for (i = 0; i < num_planes; i++)
> >> +		fb_cma->obj[i] = obj[i];
> > 
> > Check for valid num_planes before this loop?
> > 
> 
> Hm, I think the callers already take care of this. drm_format_num_planes will
> always return a valid number and the other caller passes 1 unconditionally.

As long as the array always is big enough to hold the maximum number of
planes I think it's fine. However, if there is some format with 5 planes
(don't know if there is, I only know yuv with multiple planes) it's not
obvious that this code does not support this.

> 
> > 
> > Note I have renamed struct drm_gem_cma_obj to struct drm_gem_cma_object
> > in the patch I just sent.
> > 
> 
> Do you think it makes sense for you to carry this patch as part of your iMX DRM
> series?

I can carry this, but it could be that Laurent has his driver in an
acceptable state earlier. Let's see.

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 kms/fb cma helper

[Lars-Peter Clausen]

On 05/31/2012 08:11 PM, Sascha Hauer wrote:
> On Thu, May 31, 2012 at 11:34:37AM +0200, Lars-Peter Clausen wrote:
>>>> +	drm_helper_mode_fill_fb_struct(&fb_cma->fb, mode_cmd);
>>>> +
>>>> +	for (i = 0; i < num_planes; i++)
>>>> +		fb_cma->obj[i] = obj[i];
>>>
>>> Check for valid num_planes before this loop?
>>>
>>
>> Hm, I think the callers already take care of this. drm_format_num_planes will
>> always return a valid number and the other caller passes 1 unconditionally.
> 
> As long as the array always is big enough to hold the maximum number of
> planes I think it's fine. However, if there is some format with 5 planes
> (don't know if there is, I only know yuv with multiple planes) it's not
> obvious that this code does not support this.

The DRM ABI limits the number of planes to 4. But adding a global
DRM_MAX_PLANES is probably a good idea to avoid surprises if this ever changes.