Review request to DRM Driver for Samsung SoC Exynos4210.

[Rob Clark]

On Tue, Aug 30, 2011 at 7:38 AM, Inki Dae <inki.dae at 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()?

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.

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.

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);