From mboxrd@z Thu Jan 1 00:00:00 1970 From: Inki Dae Subject: RE: Review request to DRM Driver for Samsung SoC Exynos4210. Date: Wed, 31 Aug 2011 13:36:14 +0900 Message-ID: <000a01cc6797$84368570$8ca39050$%dae@samsung.com> References: <001501cc6636$cca3b650$65eb22f0$%dae@samsung.com> <001301cc66d5$8992fac0$9cb8f040$%dae@samsung.com> <002d01cc6711$a869c230$f93d4690$%dae@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mailout1.samsung.com (mailout1.samsung.com [203.254.224.24]) by gabe.freedesktop.org (Postfix) with ESMTP id AF6E29E8F9 for ; Tue, 30 Aug 2011 21:36:51 -0700 (PDT) Received: from epcpsbgm1.samsung.com (mailout1.samsung.com [203.254.224.24]) by mailout1.samsung.com (Oracle Communications Messaging Exchange Server 7u4-19.01 64bit (built Sep 7 2010)) with ESMTP id <0LQR003SEZF78SF0@mailout1.samsung.com> for dri-devel@lists.freedesktop.org; Wed, 31 Aug 2011 13:36:37 +0900 (KST) Received: from TNRNDGASPAPP1.tn.corp.samsungelectronics.net ([165.213.149.150]) by mmp2.samsung.com (iPlanet Messaging Server 5.2 Patch 2 (built Jul 14 2004)) with ESMTPA id <0LQR001MWZH178@mmp2.samsung.com> for dri-devel@lists.freedesktop.org; Wed, 31 Aug 2011 13:36:37 +0900 (KST) In-reply-to: Content-language: ko 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: 'Rob Clark' Cc: eric.y.miao@gmail.com, kyungmin.park@samsung.com, dri-devel@lists.freedesktop.org, sw0312.kim@samsung.com List-Id: dri-devel@lists.freedesktop.org Hello Rob. Thank you for your comments. > -----Original Message----- > From: robdclark@gmail.com [mailto:robdclark@gmail.com] On Behalf Of Rob > Clark > Sent: Wednesday, August 31, 2011 7:16 AM > To: Inki Dae > Cc: Dave Airlie; eric.y.miao@gmail.com; sw0312.kim@samsung.com; dri- > devel@lists.freedesktop.org; kyungmin.park@samsung.com > Subject: Re: Review request to DRM Driver for Samsung SoC Exynos4210. > > On Tue, Aug 30, 2011 at 7:38 AM, Inki Dae wrote: > > > >> > Basically gem_dumb_* and gem_* are same operation. The difference > >> between > >> > them is that gem_dumb_ needs framebuffer information such width, > height > >> and > >> > bpp to allocate buffer and gem_ needs only size. in case of gem_dumb_, > >> size > >> > would be calculated at kernel side(at samsung_drm_gem_dumb_create). > do > >> you > >> > think it's better using only gem_dumb_? if so, I will remove > >> > gem_create_ioctl, gem_map_offset_ioctl and gem_mmap functions. > >> > >> I think using the dumb ioctls initially is a good plan, esp as you > >> have no tiling or acceleration support, the idea > >> behind the dumb ioctls is to give splash screens and maybe write a > >> generic X.org driver in the future that can just do sw rendering. > >> > >> Like at some point I forsee you needing driver specific ioctls for > >> alloc/mapping, I'd just rather wait until you have some userspace > >> available to use them that we can validate them with. > >> > > Thank you for your pointing out. I will remove SAMSUNG_GEM_MAP_OFFSET > and > > SAMSUNG_GEM_MAP ioctls except SAMSUNG_GEM_CREATE and SAMSUNG_GEM_MMAP > ioctls > > because these are duplicated with dumb_*. and for alloc/mapping you > > mentioned above, we have already tested them through user application. > > > > This is example code in user level: > > > > /* allocation. */ > > gem.size = 1024 * 600 * 4; > > ioctl(fd, DRM_IOCTL_SAMSUNG_GEM_CREATE, &gem); > > > > /* user space mapping. */ > > map.handle = gem.handle > > map.offset = 0; > > map.size = gem.size; > > ioctl(fd, DRM_IOCTL_SAMSUNG_GEM_MMAP, &map); > > Inki, any reason not to just pass the offset (which looks like you get > from _MMAP ioctl) back to mmap() syscall? Rather than having an ioctl > that calls do_mmap()? > This is samsung specific ioctls and this feature simplifies to map user space to physical memory. the offset would be sent to driver by user and then samsung_drm_gem_mmap_buffer gets the offset value through vma->vm_pgoff after do_mmap -> do_mmap_pgoff -> mmap_region -> file->f_op->mmap. the offset is physical memory offset to be mapped like this. pfn = (samsung_gem_obj->entry->paddr + vma->vm_pgoff) >> PAGE_SHIFT; remap_pfn_range(..., pfn, ...); if this feature has some possibility to jeopardize drm framework in any case, then I will remove this feature. otherwise I'd like to use one. and I915 also use same feature. you can refer to libdrm/tests/gem_mmap.c for application and linux/drivers/gpu/drm/i915/i915_gem.c for device driver. if there are any points I missed, give me any comments or pointing out please. > Actually, it may even be enough to combine GEM_CREATE and GEM_MMAP > ioctls.. (currently in OMAP DRM driver I have them combined). I > *think* (and someone correct me if I am wrong), the only reason to > have separate ioctl to get mmap offset is to allow for separate > coherent and non-coherent mappings in same process. And this would > only work on ARM if you had a proper GART that you were mapping > through, otherwise it would be aliased virtual mappings of same > physical page. > Yes, we already have the feature you said above also. SAMSUNG_GEM_MAP_OFFSET ioctl is that. but as you know, this feature is duplicated with dumb_* and Dave pointed out before. only the difference between them is to use size only or buffer information such as width, height and bpp to allocate buffer. and so I am going to remove this feature. how do you think about that? I wonder your opinions. > I think that it would be possible to add another ioctl later to > generate additional offsets if we ever had hw where this made sense. > Until then a single GEM_CREATE that returns a handle and offset (plus > GEM_INFO that gives you a way to get the offset in a different > process) would be sufficient. > Thank you for your comments again. :) > BR, > -R > > > > > /* clear buffer. */ > > memset(map.mapped, 0x0, map.size); > > > > drmModeAddFB(fd, width, height, 32, 32, stride, map.handle, &fb_id); > > > > drmModeSetCrtc(fd, ....); > > > > /* release. */ > > munmap(map.mmaped, map.size); > > > > gem_close.handle = gem.handle; > > ioctl(fd, DRM_IOCTL_GEM_CLOSE, &gem_close);