[PATCH v3 12/32] drm/exynos: Split manager/display/subdrv

[Tomasz Figa]

Hi Sean,

On Tuesday 29 of October 2013 12:12:58 Sean Paul wrote:
> This patch splits display and manager from subdrv. The result is that
> crtc functions can directly call into manager callbacks and encoder
> functions can directly call into display callbacks. This will allow
> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
> with common code.
> 
> Signed-off-by: Sean Paul <seanpaul at chromium.org>
> ---
> 
> Changes in v2:
> 	- Pass display into display_ops instead of context
> Changes in v3:
> 	- Changed vidi args to exynos_drm_display instead of void
> 	- Moved exynos_drm_hdmi.c removal into next patch
> 
>  drivers/gpu/drm/exynos/exynos_drm_connector.c |  50 ++---
>  drivers/gpu/drm/exynos/exynos_drm_core.c      | 181 +++++++++++++-----
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 115 +++++++++---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.h      |  20 +-
>  drivers/gpu/drm/exynos/exynos_drm_drv.c       |  29 +--
>  drivers/gpu/drm/exynos/exynos_drm_drv.h       | 106 +++++++----
>  drivers/gpu/drm/exynos/exynos_drm_encoder.c   | 258 ++++----------------------
>  drivers/gpu/drm/exynos/exynos_drm_encoder.h   |  18 +-
>  drivers/gpu/drm/exynos/exynos_drm_fb.c        |   4 +-
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 161 ++++++++--------
>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c      | 211 +++++++++------------
>  drivers/gpu/drm/exynos/exynos_drm_hdmi.h      |   2 +
>  drivers/gpu/drm/exynos/exynos_drm_plane.c     |  15 +-
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c      | 129 ++++++-------
>  14 files changed, 615 insertions(+), 684 deletions(-)

This patch is really heavy, making it very hard to review. I wonder if it
couldn't really be split into several smaller patches...

Anyway, please see my comments below, for the points I haven't overlooked
due to the complexity of this patch.

> diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c b/drivers/gpu/drm/exynos/exynos_drm_core.c
> index 08ca4f9..e76098d 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_core.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c
> @@ -16,11 +16,14 @@
>  #include <drm/drmP.h>
>  #include <drm/bridge/ptn3460.h>

Huh? Including a specific bridge chip inside a generic Exynos DRM core?
I know this is not a part of this patch, but... this is _broken_.

You may want to support multiple bridge chips in Exynos DRM core and you
may also want to support PTN3460 in other DRM cores. It can't be done this
way.

This series is very nice, but the whole effect is destroyed by basing it
on top of such insanity... Please rebase it on top of something more
reasonable (preferably Inki's next branch directly).

Otherwise, this patch seems reasonable (except its size).

Best regards,
Tomasz