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

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

Hi Sascha,

On Tuesday 05 June 2012 09:49:48 Sascha Hauer wrote:
> 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.

Yes I was thinking mainly about YUV, although I'm not sure whether we already 
have formats where this could happen. It won't hurt to be prepared for the 
future though :-)

> Integrated the rest of you comments.

-- 
Regards,

Laurent Pinchart