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

From: Sean Paul <seanpaul@chromium.org>
To: Tomasz Figa <tomasz.figa@gmail.com>
Cc: dri-devel <dri-devel@lists.freedesktop.org>,
	"Stéphane Marchesin" <marcheu@chromium.org>
Subject: Re: [PATCH v3 12/32] drm/exynos: Split manager/display/subdrv
Date: Tue, 12 Nov 2013 12:51:11 -0500	[thread overview]
Message-ID: <CAOw6vbKfXJaouyFC84BWYa86Oygdkfo1=5F80bzAWHQuE_ttqA@mail.gmail.com> (raw)
In-Reply-To: <4195025.7qusjSGvgG@flatron>

On Sun, Nov 10, 2013 at 4:09 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> 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@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...
>

I've distilled it down as much as possible. Unfortunately the
encoder/crtc were fused pretty tightly and the effects of that are
far-reaching.

> 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).
>

Well, that was blunt. Unfortunately, I don't know how else to do this
other than this broken, unreasonable and insane way.

This has already been discussed at length in a few other places.
Without some help from the drm layer, I don't see any other way to do
this than to enumerate the possible bridges in each drm driver.

I based the current set off the "Add some missing bits for
exynos5250-snow" series I sent up a little while ago. This set was
sent to the relevant dt people, acked by Olof, and there are no
outstanding review comments. Since it's required for me to test on
Real Hardware, I assumed this was an appropriate base.

If you think this is _broken_, unreasonable, and insane, I'd love to
hear an alternative.

Sean

> Otherwise, this patch seems reasonable (except its size).
>
> Best regards,
> Tomasz
>