Hello Rob. Thank you for your comments. > -----Original Message----- > From: robdclark@xxxxxxxxx [mailto:robdclark@xxxxxxxxx] On Behalf Of Rob > Clark > Sent: Wednesday, August 31, 2011 7:16 AM > To: Inki Dae > Cc: Dave Airlie; eric.y.miao@xxxxxxxxx; sw0312.kim@xxxxxxxxxxx; dri- > devel@xxxxxxxxxxxxxxxxxxxxx; kyungmin.park@xxxxxxxxxxx > Subject: Re: Review request to DRM Driver for Samsung SoC Exynos4210. > > On Tue, Aug 30, 2011 at 7:38 AM, Inki Dae <inki.dae@xxxxxxxxxxx> 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); _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
