Review request to DRM Driver for Samsung SoC Exynos4210.

[Inki Dae]

Hello Rob.
Thank you for your comments.

> -----Original Message-----
> From: robdclark at gmail.com [mailto:robdclark at gmail.com] On Behalf Of Rob
> Clark
> Sent: Wednesday, August 31, 2011 7:16 AM
> To: Inki Dae
> Cc: Dave Airlie; eric.y.miao at gmail.com; sw0312.kim at samsung.com; dri-
> devel at lists.freedesktop.org; kyungmin.park at 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 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()?
> 
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);