Re: [PATCH v2] DRM: add drm gem cma helper

[Sascha Hauer <s.hauer@pengutronix.de>]

On Thu, May 31, 2012 at 11:36:15AM +0200, Laurent Pinchart wrote:
> Hi Sascha,
> 
> > +	depends on DRM
> > +	help
> > +	  Choose this if you need the GEM cma helper functions
> 
> I would put CMA in uppercase, but that's just nitpicking.
> 
> BTW this helper is not strictly dedicated to CMA. It uses the DMA API to 
> allocate memory, without caring about the underlying allocator.

Yes, I know. It's just that 'CMA' is short and expresses very much what
I mean. I first had 'dma_alloc' instead, but this makes the function
names quite long.

> > +
> > +/*
> > + * 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);
> 
> Shouldn't this be DIV_ROUND_UP(args->width * args->bpp, 8) ? Not all formats 
> might need to pad pixels to an integer number of bytes.

Are you thinking about YUV formats? I can't imagine RGB formats where
this might be an issue.

Integrated the rest of you comments.

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 |