RE: Review request to DRM Driver for Samsung SoC Exynos4210.

[Kyungmin Park <kyungmin.park@samsung.com>]

CC Eric (freescale), Rob (TI) who working the DRM with other ARM SoCs.

As Dave mentioned, Please review Samsung DRM codes.

Thank you,
Kyungmin Park

-----Original Message-----
From: Dave Airlie [mailto:airlied@gmail.com] 
Sent: Monday, August 29, 2011 11:27 PM
To: Inki Dae
Cc: airlied@linux.ie; kyungmin.park@samsung.com;
dri-devel@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.

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.

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.

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,

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.

5. you probably don't need master_create/set ops.

Dave.