From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [PATCH v3 12/32] drm/exynos: Split manager/display/subdrv Date: Sun, 10 Nov 2013 22:09:14 +0100 Message-ID: <4195025.7qusjSGvgG@flatron> References: <1383063198-10526-1-git-send-email-seanpaul@chromium.org> <1383063198-10526-13-git-send-email-seanpaul@chromium.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f41.google.com (mail-ee0-f41.google.com [74.125.83.41]) by gabe.freedesktop.org (Postfix) with ESMTP id D5943FA61D for ; Sun, 10 Nov 2013 22:18:05 -0800 (PST) Received: by mail-ee0-f41.google.com with SMTP id e53so2218574eek.28 for ; Sun, 10 Nov 2013 22:18:05 -0800 (PST) In-Reply-To: <1383063198-10526-13-git-send-email-seanpaul@chromium.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: Sean Paul Cc: dri-devel@lists.freedesktop.org, marcheu@chromium.org List-Id: dri-devel@lists.freedesktop.org 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 > --- > > 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 > #include 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