On Mon, 13 Jan 2014 17:55:27 -0200 Paulo Zanoni <przanoni@xxxxxxxxx> wrote: > 2014/1/8 Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>: > > No idea if this is correct or not, all the WRPLL programming is new to > > me. Paulo, can you take a look? At least it doesn't complain on my BDW > > here... > > As a side note: can't we add some debugfs interface that, when read, > would trigger a HW state readout, and then would compare it against > what we have? It would be the perfect thing to validate patches like > the one you wrote... And it would make QA easier (just write a test > case that sets modes and reads the debugfs interface! Or just modify > kms_setmode/testdisplay to do it everywhere). > > Some comments: > > - I guess the commit message could be improved :) > - We probably need to do something about the VGA output, which uses > the old intel_crt encoder instead of intel_digital_port. If this patch > doesn't regress anything, we could do it on a separate patch. I'm not using intel_digital_port here though, or is that what ddi_get_encoder_port needs? If so then yeah I guess I'll need to add specific VGA support somehow. > - Don't we also need to patch intel_pipe_config_compare() so it checks > the now-used adjusted_mode.crtc_clock? Yeah, it's there, just needs to come out from under its !HAS_DDI rock. > - Based on that, don't we also need HW readout support for port_clock? Ah yeah that needs to be set too. > - You should probably compare your patch against "[PATCH 2/2] > drm/i915: add fast boot support for Haswell" sent by Furquan Shaikh in > August, and also check the reviews we gave to it. Just checked (I had totally forgotten that), I think his version is a little nicer, but I didn't see an update. Furquan, were you planning to re-submit for inclusion? > - I tried booting it on my HSW machine that only has an eDP output, > and the driver doesn't load: > > I get: > > [ 62.635700] WARNING: CPU: 0 PID: 1120 at > drivers/gpu/drm/i915/intel_ddi.c:431 > intel_ddi_get_crtc_encoder+0x95/0xa0 [i915]() > [ 62.635703] 0 encoders on crtc for pipe A Hm what would cause that? Why would we not have an encoder assigned? > > Then later I see: > > [ 62.636200] kernel BUG at drivers/gpu/drm/i915/intel_ddi.c:433! > [ 62.636249] invalid opcode: 0000 [#1] SMP > > Which is the "BUG_ON(ret == NULL);" line > > Please notice that I'm not using the i915_fastboot command line argument. > > - > > - ironlake_get_fdi_m_n_config(crtc, pipe_config); > > } > > > > + ironlake_get_fdi_m_n_config(crtc, pipe_config); > > This is a little confusing, probably wrong, because FDI is only used > when has_pch_encoder is true, but you're calling this code on all > cases. > > Function ironlake_get_fdi_m_n_config() calls > intel_cpu_transcoder_get_m_n() for eDP/DP/HDMI. > However, we have encoder->get_config which calls > intel_ddi_get_config() which calls intel_dp_get_m_n(), which will call > intel_cpu_transcoder_get_m_n() again on the same cases. So we call the > same thing twice. Confusing I agree, but I think we set the CPU m/n values for eDP/DP/HDMI on HSW+? If so, we need these values to figure out the actual mode clock. I think the redundant call is ok, but I'll check other platforms. > I would imagine you probably don't need to move this line above, but I > may be wrong. If something is wrong, you probably need to fix some > other code that's assuming that "gen5+ DP m/n registers are on the > PCH" or something like that. We mainly need to get the values early on to calculate the pixel clock, so maybe the later call can be dropped. -- Jesse Barnes, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
