From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sascha Hauer Subject: Re: [PATCH] DRM: add drm gem CMA helper Date: Wed, 30 May 2012 18:28:12 +0200 Message-ID: <20120530162812.GL30400@pengutronix.de> References: <1338300627-2176-1-git-send-email-s.hauer@pengutronix.de> <7829358.GjfEKl07XY@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [92.198.50.35]) by gabe.freedesktop.org (Postfix) with ESMTP id 83558A0993 for ; Wed, 30 May 2012 09:28:17 -0700 (PDT) Content-Disposition: inline In-Reply-To: <7829358.GjfEKl07XY@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Laurent Pinchart Cc: kernel@pengutronix.de, dri-devel@lists.freedesktop.org, Rob Clark List-Id: dri-devel@lists.freedesktop.org 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 |