Re: [PATCH 46/66] drm/i915: Move hsw_fdi_link_train into intel_crt.c

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Thu, 22 May 2014 23:57:02 +0200
Daniel Vetter <daniel@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@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.

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.

-- 
Jesse Barnes, Intel Open Source Technology Center