[Intel-gfx] [PATCH] drm/i915/skl: Handle eDP from generic crtc_compute_clock vfunc

[Damien Lespiau]

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 at intel.com>
> > 
> > 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 at intel.com>
> > Cc: Damien Lespiau <damien.lespiau at intel.com>
> > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> 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 at intel.com>
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?

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().

-- 
Damien