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

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

Hi Sascha,

On Wednesday 30 May 2012 18:28:12 Sascha Hauer wrote:
> 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 ;)

I wouldn't claim to be an expert on this matter :-) As far as I know, PFNMAP 
means that the mapping refers to physical memory with no struct page 
associated with it (that could be I/O memory, DRAM reserved at boot time, ... 
basically any memory outside of the system memory resources controlled by the 
kernel page allocator). MIXEDMAP means that the mapping refers to memory that 
contains both PFNMAP regions and non-PFNMAP regions. I would be surprised if 
dma_alloc_writecombine() returned such a mix. On the other hand the memory it 
returns might be PFNMAP or non-PFNMAP depending on the underneath allocator, 
maybe that's why VM_MIXEDMAP was used.

I'd appreciate an expert's opinion on this :-)

> > > +
> > > +	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.

-- 
Regards,

Laurent Pinchart