[Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Daniel Vetter]

On Wed, Oct 14, 2015 at 11:21:46AM +0300, Ander Conselvan De Oliveira wrote:
> On Tue, 2015-10-13 at 16:08 +0200, Daniel Vetter wrote:
> > On Tue, Oct 13, 2015 at 04:00:37PM +0200, Maarten Lankhorst wrote:
> > > Op 13-10-15 om 15:58 schreef Daniel Vetter:
> > > > On Tue, Oct 13, 2015 at 03:43:28PM +0200, Maarten Lankhorst wrote:
> > > > > Op 13-10-15 om 15:35 schreef Daniel Vetter:
> > > > > > On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst wrote:
> > > > > > > Op 23-09-15 om 17:34 schreef Gabriel Feceoru:
> > > > > > > > Using 2 connectors (DVI and VGA) will cause wrpll to be set for
> > > > > > > > INTEL_OUTPUT_HDMI but never reset if switching to INTEL_OUTPUT_VGA
> > > > > > > > 
> > > > > > > > Supresses errors like these:
> > > > > > > > [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in dpll_hw_state.wrpll
> > > > > > > > 
> > > > > > > Looks like a good idea to always zero it.
> > > > > > Except that we still have a bunch of cases where we recompute clock state
> > > > > > but only partially. Can we just move them all up into a common place
> > > > > > please? That would also catch cases where we simply forget to fill this
> > > > > > out at all.
> > > > > > 
> > > > > > One case I noticed is edp in skl_ddi_pll_select, but there's probably
> > > > > > more.
> > > > > > 
> > > > > Something like below, with all the memsets for dpll_hw_state removed?
> > > > I think this will blow up since we recompute clock state only when
> > > > needs_modeset is true. So needs a bit more intelligence in deciding when
> > > > to clear it I think.
> > > Oops you're right. Maybe intel_modeset_clear_plls because that's where all the clock state
> > > belongs?
> > 
> > Yeah that might be an even better place, in the loop after the continue;
> > statement.
> 
> The reason I didn't put the memset there in the first place was the way we calculate plls for DP
> with DDI platforms. In that case, ddi_pll_sel is setup from the encoder_config instead of
> compute_clock, so a memset ends up clearing the new pll config.

Hm, I forgot about this split totally. And there seems to be a giant mess
going on here:

In our top-level intel_atomic_check we have 4 parts to compute state:
1. drm_atomic_helper_check_modeset
2. intel_modeset_pipe_config
3. intel_modeset_checks
4. drm_atomic_helper_check_planes

We recalculate clocks (by calling dev_priv->display.crtc_compute_clock)
in 1., way ahead of anything else in intel_crtc_atomic_check. That looks
very suspcious since it means only very later on (in the loop that does
2.) do we even decide whether we need to do a full modeset or not.

So what I had in mind is that we clear clocks in
intel_modeset_pipe_config, before we call any of the callbacks. That makes
sure that when we decided to do a modeset, we do recompute the clocks
correctly. First step would be to order 1 and 2 correctly I guess.

Next up is 3, that one just has a confusing name - it doesn't clear PLL
state, but updates the global pll state if necessary. So should perhaps be
renamed to intel_modeset_compute_shared_dpll or similar.

Related, there's also the scaler code in step 1, which is definitely
bonkers since that's done before we recompute the config in step 3. But
that's an unrelated issue really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch