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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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