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 inki.dae@samsung.com 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);