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

[Olof Johansson]

On Tue, Nov 12, 2013 at 10:35 AM, Tomasz Figa <tomasz.figa at 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 at 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 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...
>> >
>>
>> 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.


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


-Olof