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

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

On Tue, 2015-11-10 at 14:53 +0200, Jani Nikula wrote:
> On Wed, 14 Oct 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Oct 14, 2015 at 04:58:55PM +0300, Ander Conselvan De Oliveira wrote:
> > > On Wed, 2015-10-14 at 14:44 +0200, Daniel Vetter wrote:
> > > > 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.
> > > 
> > > I had a suspicion this would interact badly with how we "cancel" the
> > > modeset if the pipe config
> > > didn't changed, just after the call to intel_modeset_pipe_config(). It
> > > turns out there's an issue
> > > there already.
> > > 
> > > There are two possibilities for the dpll_hw_state value after the new
> > > pipe_config is calculated. It
> > > may have the new values already for DP in HSW/BDW and eDP in SKL or it may
> > > still have the old value.
> > > In the latter case the new value is only calculated in .crtc_clock(),
> > > after we already compared the
> > > old and new configs and may have decided to skip the modeset.
> > > 
> > > But doing the memset() in intel_modeset_pipe_config() would be find as
> > > long as we don't change our
> > > minds about doing a modeset later.
> > 
> > It's more annoying since my analysis is all wrong: intel_crtc_atomic_check
> > is called from drm_atomic_helper_check_planes, i.e. step 4 not step 1.
> > It'll all work out I think if we memset it in intel_modeset_pipe_config.
> > The caveat is that we need to move the clock recomputation into
> > intel_modeset_pipe_config too (which is better, since then we'll have more
> > accurate state to decided whether we'll fastboot or not).
> > 
> > And then intel_modeset_clear_plls would really just update the global pll
> > setup (and would be really good to rename it to
> > intel_modeset_compute_shared_dpll or whatever).
> > 
> > Thoughts?
> 
> Ander, Maarten, where are we with this? Is it horribly wrong to merge
> the original patch in this ever-growing and diverging thread?

I think the patch as is will cause problems with DP, since we might clear the
pll selection made in hsw_dp_set_ddi_pll_sel(). I think the easy fix
disregarding the discussion in this thread is to drop another memset in
 intel_crt_compute_config(). Like this

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index b84aaa0..ad099f3 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -278,6 +278,9 @@ static bool intel_crt_compute_config(struct intel_encoder
*encoder,
 
        /* FDI must always be 2.7 GHz */
        if (HAS_DDI(dev)) {
+               memset(&pipe_config->dpll_hw_state, 0,
+                      sizeof(pipe_config->dpll_hw_state));
+
                pipe_config->ddi_pll_sel = PORT_CLK_SEL_SPLL;
                pipe_config->port_clock = 135000 * 2;
        }

Ander
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx