On Sat, Oct 20, 2012 at 12:00 AM, Daniel Vetter <daniel at ffwll.ch> wrote: > On Fri, Oct 19, 2012 at 11:19 PM, Paulo Zanoni <przanoni at gmail.com> wrote: >> - Daniel suggested to not convert some of the code that touches FDI and >> the PCH transcoder but I did not remove these chunks since we're >> currently running this code on Haswell even when not using VGA, so if we >> don't convert the code to cpu_transcoder we might start poking the wrong >> pipes and messing a lot of things. I really don't think we should delay >> Haswell enablement even more based on cleanups we did not even write >> yet. I've been maintaining these Haswell patches for a long time, I'd >> love to get rid of them. When we do the VGA cleanups we will be able to >> fix what we need to fix. > > Hm, where are we calling into fdi/pch code if the connector is not > VGA? I've thought we've taken care of all these things by now ... Ok, on irc Paulo clarified that in ironlake_crtc_disable we still call some of the fdi and (pch) transcoder functions. Now for most cases I'd simply call this a bug - we need to teach the code some smarts to check whether any of the encoders are on the pch and only disable things if that's the case. But even without that consideration I don't think it makes sense to to the s/pipe/cpu_transcoder transformation on these functions: - At most we should talk about a separate pch transcoder (to make the vga encoder work on cpu_pipe != PIPE_A). The current code works somewhat since there's only one pch transcoder on LPT, and by fixing VGA to pipe A we make sure we don't try to use a non-existing pch transcoder. - We don't have an eDP transcoder on any pch device. So mapping to the transcoder enum makes even less sense. My proposal: - ditch the s/pipe/cpu_transcoder/ that touch the fdi/pch transcoder stuff from this patch series. - when fixing up hsw vga support, first rename the current transcoder functions to pch_transcoder, to make clear what they're dealing with - add a intel_crtc->pch_transcoder, which for all platforms but hsw would be the same as intel_crtc->pipe, switch all pch code over to use that - add an encoder->is_pch_encoder field, and wrap up all the fdi/pch stuff with checks for this to avoid calling code that we should call. We probably should keep around all the asserts that check whether things are properly disabled. - fix the pch_transcoder field on hsw to PIPE_A, since that's what we'll ever use. Depending upon what the code looks like, we could also create haswell specific crtc_enable/disable functions and maybe even move all the fdi/pch_transcoder stuff into the a hsw specific crt encoder (since it's the only odd thing out). Comments? Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
