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

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