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

[Lars-Peter Clausen <lars@metafoo.de>]

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