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