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

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

On Wed, May 30, 2012 at 05:40:13PM +0200, Laurent Pinchart wrote:
> Hi Sascha,
> 
> Thank you for the patch. I've successfully tested the helper with the new SH 
> Mobile DRM driver. Just a couple of comments below in addition to Lars' 
> comments (this is not a full review, just details that caught my attention 
> when comparing the code with my implementation, and trying to use it).
> 
> > +/*
> > + * drm_gem_cma_mmap - (struct file_operation)->mmap callback function
> > + */
> > +int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
> > +{
> > +	int ret;
> > +
> > +	ret = drm_gem_mmap(filp, vma);
> > +	if (ret)
> > +		return ret;
> > +
> > +	vma->vm_flags &= ~VM_PFNMAP;
> > +	vma->vm_flags |= VM_MIXEDMAP;
> 
> Why is this a mixed map ?

Honestly, I don't know. This is copied from the exynos driver. I don't
know much about these flags, so if you think something else is better
here than you're probably right ;)

> 
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
> 
> My implementation maps the whole buffer in one go at mmap time, not page by 
> page at page fault time. Isn't that more efficient when mapping frame buffer 
> memory ?

I remember Alan has mentioned this in an earlier review. I'll update
the patch accordingly.

I will fix this and the other things you mentioned and repost.

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 |