On Wed, 2015-05-13 at 18:25 +0100, Damien Lespiau wrote: > On Wed, May 13, 2015 at 05:56:17PM +0100, Damien Lespiau wrote: > > On Wed, May 13, 2015 at 05:40:44PM +0100, Tvrtko Ursulin wrote: > > > From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > > > > > > Since commit 4978cc93d9ac240b435ce60431aef24239b4c270 started clearing > > > dpll state and recomputing it via crtc_compute_clock (and probably some > > > other commit which triggered pipe config checking), modesetting is now > > > constantly triggering warnings about dpll_hw_state.ctrl1 mismatch. > > > > > > Reason is crtc_compute_clock calls skl_ddi_pll_select which does not do > > > anything for eDP, leaving the ctrl1 state at the default of zero. > > > > > > This potentially hacky fix makes skl_ddi_pll_select call > > > skl_edp_set_pll_config which fixes the problem for me. > > > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > > > Cc: Damien Lespiau <damien.lespiau@xxxxxxxxx> > > > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx> > > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > Nop! (at least I really don't think so). > > > > As discussed on IRC, on DDI platforms, the (e)DP compute_config() does > > the private DPLL selection, and the ddi_pll_select() does the the same > > for shared DPLLs (I'm not saying that the end result we want, just how > > it works today). That split comes from the introduction of shared_dpll > > in the DDI PLL selection last summer. > > > > Anyway, this means that, for SKL, dpll_hw_state is touched by the > > encoder's compute_config() for (e)DP. > > > > I think we could just remove the memset() in 4978cc93 (maybe?), or try > > to unify a bit better and only have one place where we do PLL selection > > (which I assume is part of the bigger atomic plan). Not > > skl_edp_set_pll_config() in both compute_config() and ddi_pll_select() > > though. > > To be more precise: > > commit 4978cc93d9ac240b435ce60431aef24239b4c270 > Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx> > Date: Tue Apr 21 17:13:21 2015 +0300 > > drm/i915: Preserve shared DPLL information in new pipe_config > > When a new pipe_config is calculated, the fields related to shared dplls > are reset, under the assumption that they will be recalculated as part > of the modeset, which is true with the current state of the code. > > As we convert to atomic, however, it will be possible to calculate a new > pipe_config and skip the modeset. In that case, after the state swap we > still want the shared dplls to be preserved. > > Except that dpll_hw_state is not just for shared DPLLs. So maybe dpll_hw_state > shouldn't be preserved (in clear_intel_crtc_state() in the case where > shared_dpll is DPLL_ID_PRIVATE? The idea here is that when the modeset code does a flip only, we are still doing a state swap. Before the swap was in place, we simply wouldn't change the pipe_config, so the value for dpll_hw_state wouldn't change either. If there is an actual modeset these values are recalculated, so preserving the state should be harmless. > Note that ddi_pll_sel is also a field set by the DDI PLL selection code you may > want to preserve in clear_intel_crtc_state(). Yeah, I missed that one. We *definitely* want to preserve it. I'll send a patch for doing that shortly. Thanks, Ander _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
