[Intel-gfx] [PATCH 46/66] drm/i915: Move hsw_fdi_link_train into intel_crt.c

[Daniel Vetter]

On Tue, May 27, 2014 at 10:31:43AM -0700, Jesse Barnes wrote:
> On Thu, 22 May 2014 23:57:02 +0200
> Daniel Vetter <daniel at ffwll.ch> wrote:
> 
> > On Thu, May 22, 2014 at 05:28:05PM -0300, Paulo Zanoni wrote:
> > > 2014-04-24 18:55 GMT-03:00 Daniel Vetter <daniel.vetter at ffwll.ch>:
> > > > The pch encoder case really isn't anything generic on hsw:
> > > > - It's for the vga port only and
> > > > - the vga port does only exist on some hsw platforms.
> > > >
> > > > Imo it helps the generic code flow a lot if we shovel all this into
> > > > hsw specific enable/disable hooks. A bonus is that some of our largest
> > > > files (intel_ddi.c and intel_display.c) will lose a pile of really big
> > > > functions.
> > > >
> > > > Step one is to move the fdi link training code.
> > > 
> > > I don't think that helps much, since the other fdi link train code is
> > > still at intel_display.c, and even if the only thing that needs fdi
> > > link training on HSW is CRT, fdi link training is really _not_ a CRT
> > > thing. So IMHO we're breaking an abstraction here by putting fdi link
> > > training in CRT code. Also, the fact that HSW+ won't be using
> > > dev_priv->display.fdi_link_train will makes things even more confusing
> > > for people reading our code.
> > > 
> > > I'd really prefer if we merge
> > > http://patchwork.freedesktop.org/patch/24019/ instead. Or something
> > > based on that.
> > 
> > Well my idea with all this was that even on hsw vga on the pch is a bit
> > the special case, and on bdw+ it's completely gone. So taking this out of
> > the common modeset code should help bdw+ a bit since now the modeset
> > sequence and our code really nicely align.
> > 
> > Also with ilk-ivb having the fdi code in the crtc functions made a lot of
> > sense since the big crossbar was in the pch. So doing everything up to the
> > pch crossbar, including all the fdi handling in crtc code, and everything
> > after that in encoder callbacks.
> > 
> > But on hsw+ we have the big crossbar on the cpu side, between the
> > transcoders and the ddi ports. So from a functional pov it makes much more
> > sense imo to have all the pipe/transcoder code in the crtc, and all the
> > ddi handling code in encoder callbacks. The pch and fdi link that hang off
> > ddi port E work more like a drm_bridge (but we don't need that since the
> > lpt pch will never be used outside of intel platforms).
> > 
> > So with this example, grouping all the fdi code together only groups
> > functions by their name. But moving the hsw fdi stuff into the crt encoder
> > groups the code by the behaviour of the hardware. And that's massively
> > different betweent ivb and hsw.
> > 
> > I hope this explains a bit my thinking here.
> 
> I think I agree with Paulo here.  FDI happens to be only used for VGA
> on HSW, but it's theoretically possible to use it for other stuff on
> the PCH side, so keeping it under has_pch_encoder in the platform
> independent code paths makes sense.

There is simply no silicon with anything but a VGA encoder on the pch. And
even though the hw engineers have kept all the register at the same place,
there's also no cross bar any more, and also no 2nd pipe. The lpt pch
really is just a fancy transcoder, and imo should be treated like that.

> But a nice compromise would be to split out the FDI code as Paulo
> suggested; that would get it out of intel_display.c and maybe make for
> less confusion.
> 
> On top of that, we could export the FDI training stuff from there and
> push the training calls as needed into the ports, like you've done with
> the CRT here, but overall I think the training code itself should live
> separately from the port code, since who knows what funky things may be
> coming.

VGA is dead, also fdi seems to have gone. The way to do transcoders for
specific produts seems to either be with (e)DP or with mipi.

I'm not against extracting all the fdi code into an intel_fdi.c file
really, but since there's not a lot of shared code I don't really see the
point all that much. For me the metric to measure whether a new source
file works is the number of exports and interactions between different
pieces of code.

But like you've said, this code shuffling here doesn't really prevent
anyone from doing intel_pch.c ..
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch