[Intel-gfx] [PATCH 47/66] drm/i915: Move pch fifo underrun report enabling to hsw_crt_pre_enable

[Daniel Vetter]

On Thu, May 22, 2014 at 05:38:13PM -0300, Paulo Zanoni wrote:
> 2014-04-24 18:55 GMT-03:00 Daniel Vetter <daniel.vetter at ffwll.ch>:
> > With this all the pch pre-enable work has been moved into the special
> > hsw crt encoder functions.
> 
> For the same reasons I gave in the other patches, I'm not convinced
> this is an improvement to our code. It looks like we're just breaking
> the abstraction exploiting the fact that CRT is the only FDI/PCH
> output on Haswell. Now the HSW code will be significantly different
> from the ILK/SNB/HSW code, and I really think this can be confusing to
> people reading the code. I really think we shouldn't hide things
> aren't specific to CRT inside CRT code.

See my other mail, imo this does _not_ break the abstraction, but actually
fix it. If you look at the crtc/encoder split, the thing that connects
these two is the crossbar. So we should put the code into crtc/encoder
callbacks so that roughly everything before the crossbar is in crtc
callbacks, and everything after it is in encoder callbacks.

Of course there will be some small exceptions, e.g. a lot of the ddi port
handling (which is after the crossbar) is in crtc code. But that makes imo
sense since ddi ports are so nicely unfified between dp, hdmi and fdi.

> Since on this series we're basically disagreeing on our opinions on
> which coding style is the better, I think we should ask the other
> developers to give their opinion too :)

I'm not sure more people will help here. But this is an area I'm often
struggling on review. E.g. Ben's full ppgtt patches treat the aliasing
ppgtt on gen6 in many places like a full ppgtt. Which makes sense since
they are both called ppgtt and share a lot of the same low-level code.

But from a functional pov the aliasing ppgtt is something completely
different from full ppgtt: The first is just a fancy way to write ptes,
the 2nd is fundamentally different wrt how buffer objects get mapped into
the gpu address space.

And I'm pretty sure I've made noises like this back when we've merged the
original hsw support ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch