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