Review request to DRM Driver for Samsung SoC Exynos4210.

[Inki Dae]

Hello Dave.
Thank you for your comments. Below is my answers and questions.

> -----Original Message-----
> From: Dave Airlie [mailto:airlied at gmail.com]
> Sent: Monday, August 29, 2011 11:27 PM
> To: Inki Dae
> Cc: airlied at linux.ie; kyungmin.park at samsung.com; dri-
> devel at lists.freedesktop.org
> Subject: Re: Review request to DRM Driver for Samsung SoC Exynos4210.
> 
> > I had sent DRM Driver for Samsung SoC Exynos4210 over the three times.
> >
> > But I didn't received any comments from you.
> 
> I was sort of hoping someone else would take a look at these ARM
> drivers before me, it might be worth getting some inter-company review
> between the vendors submitting drm components as I'm guessing its
> going to be a lot of the same thing.
> 
> But I've done a quick review and some things that stick out.
> 
> 1. include/drm/samsung_drm.h should only contain public information
> for use on the userspace ioctl interface, it shouldn't contain any
> kernel private data structures, they should be in
> drivers/gpu/drm/samsung/samsung_drv.h or something very similiar.
> 
Ok, I will move kernel private data structures of include/drm/samsung_drm.h
to drivers/gpu/drm/samsung/any header file.

> 2. I'm not sure why samsung_drm_fn_encoder exists, it looks like from
> the crtc mode set functions you call the encoder mode set functions,
> is it not possible to use the helper drm_crtc_helper_set_mode so that
> the crtc mode set is called then the encoder ones without having the
> explicit calls? If not please either document it or point at the
> explaination I missed.
> 
For generic usage of crtc, we got only encoder has display controller(or
H/Ws) specific codes. 
as you pointed out, I will check how we use drm_crtc_helper_set_mode instead
of samsung_drm_fb_encoder because drm_crtc_helper_set_mode calls not only
crtc but also encoder's callbacks and in addition, we faced with vblank
issue. drm framework has only one irq handler but in embedded system at
least Samsung SoC, display controller and hdmi have independent their own
irq. so I got crtc has common vblank operations such as enable_vblank and
disable_vblank and each encoder's function to be called in accordance with
activated crtc. if you think this way is wrong or not good way then do you
have any good ideas to resolve this issue without Samsung_drm_fn_encoder,
the ideas that crtc could access to encoder.? 

> 3. Not terribly urgent but is samsung the best name for this? what
> happens if you bring out another chip, I also think there is a lot of
> samsung_drm_ in function names that seems not that useful. dropping
> _drm_ might help.
> 
As you know, Samsung SoCs  have a variety of prefixes such as s3c24xx,
s3c64xx, s3c, s5p, s5pv210(=s5pc110) and exynos4210(=s5pc210, =s5pv310).
except old SoCs, our drm driver would support only exynos4210 and later. so
we used samsung_ as prefix to represent them. but if you think this prefix
is not proper name then we would try to consider other names. And _drm_
would help to debug. for instance, we could know who is  current function's
owner.

> 4 ioctls: drm_samsung_gem_map_off needs explicit padding before the
> 64-bit, drm_samsung_gem_mmap needs explicit padding before the
> 64-bit,, though I'm not sure you need these ioctls all now that the
> dumb interface is upstream,
> 
I am  afraid I don't understand what you mean fully. Could you please give
me more details about "drm_samsung_gem_map_off needs explicit padding before
the 64-bit, drm_samsung_gem_mmap needs explicit padding before the
64-bit,,"? as DRM, I am a novice. drm_samsung_gem_map_off is used to get
offset that actually this is hash key that this key is used to get a gem
object corresponding to it at drm_gem_mmap(). and the gem object is
transferred to page fault handler(samsung_drm_gem_fault) with
vma->vm_private_data containing it. At this time, page fault handler gets a
gem object from vma object and then maps user space to physical memory.
call patch to drm_gem_mmap is as the following : mmap system call ->
samsung_drm_gem_mmap -> drm_gem_mmap
if the comments above are wrong way then feel free to give me your advices
and I will be pleased.

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.

> what is usr_addr in the gem create ioctl for? this seems wrong, it
> also looks too short for 64-bit addresses, but passing in userspace
> addr to kernel is generally not a great plan.
> 
This is my mistake. I will remove usr_addr from drm_samsung_ge_create
structure. this variable isn't used anywhere.

> 5. you probably don't need master_create/set ops.
> 
For h/w lock support, I think these operations should be used. If
unnecessary, I will remove these operations. and could you please explain
the purpose of drm_master?. I didn't understand the master fully. I'd try to
find how we could use the master feature after understanding.


> Dave.

Thank yo for your comments again. it's been very helpful

Best Regards
Inki Dae.