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

[Tomasz Figa <tomasz.figa@gmail.com>]

On Tuesday 12 of November 2013 12:51:11 Sean Paul wrote:
> 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.

I was afraid of this. Well, I guess nothing can be done about it then.

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

Sorry, this might have been a bit too harsh, but just imagine myself doing
my regular patch review round, hoping (as usual) to see only code that is
not less than great, while suddenly stumbling upon a line of code that
I would expect at most in some vendor tree or maybe several years ago in
sources of a 2.4 kernel.

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

Well, I should say that Laurent would probably have something to say about
this, but I understand that the really good solution need to take some
time to be developed, so...

What about at least creating a minimal, temporary abstraction layer that
would provide all you originally needed from drm/bridge/ptn3460.h, but in
a bridge chip agnostic way?

I believe we already have established Device Tree bindings for video
interfaces (used currently by V4L) and you can safely use them to bind
your video devices together. Based on this assumption, you can implement
a local parser for a subset of those bindings that is enough to achieve
everything you need - bind a bridge to a video output of the SoC.

As for your explicit need to include headers of particular bridges,
looking at your Chromium kernel sources[1] I can see that all you need
is an init routine for the bridge. A rewritten find_bridge() that would
instead take some generic cookie of a device driving a connector could
return a generic (exynos_)drm_bridge cookie that could be further used
to call functions like (exynos_)drm_bridge_init(), which would then
use the cookie to call appropriate bridge-specific init function.

Of course, as a temporary solution, this can be made Exynos-specific,
but this won't add dependencies on particular bridge chips to Exynos DRM
core.

[1] http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel-next.git;a=blob;f=drivers/gpu/drm/exynos/exynos_drm_drv.c;h=099baa836784ea5d0eff8c2189abab35a3b342c5;hb=refs/heads/chromeos-3.8

Best regards,
Tomasz