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

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

On Tuesday 26 of November 2013 10:00:13 Olof Johansson wrote:
> On Tue, Nov 12, 2013 at 10:35 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > 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.
> 
> More like code written in the same style as x86 DRM stuff, where
> they're not used to overabstracting things from day one to make things
> generic instead of supporting the only known chip combination to date.

No, not really. They don't have any modularity on x86. Just a graphics
card with everything on it, so they can freely do such things, as they
don't have to account for all we need to account for on ARM platforms.

Also, I wouldn't call making a driver usable and useful for more than
just one fixed platform "overabstracting"...

> 
> 
> >> 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.
> 
> On the other hand, there's a chance more abstraction than that is
> needed when the second or third chip is added. Until then, there's
> little point in doing premature work on abstracting too much of it
> out. I'd rather see the code go in sooner and iterate in the tree.

First of all, this is heavily Chromebook specific and that's not the only
Exynos-based platform using Exynos DRM. In fact, before conversion to DT,
we had full support for parallel and DSI displays on several Exynos4 based
boards.

I really regret that we allowed for such regression back then, but now
the only thing we can do about it is to at least not make a step backwards
with this and make DRM bridge something useful (or at least usable).

Best regards,
Tomasz