Re: [PATCH] drm/i915/skl: Handle eDP from generic crtc_compute_clock vfunc

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

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

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx