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

From: Ander Conselvan De Oliveira <conselvan2@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw
Date: Wed, 14 Oct 2015 11:21:46 +0300	[thread overview]
Message-ID: <1444810906.2636.4.camel@gmail.com> (raw)
In-Reply-To: <20151013140759.GG26718@phenom.ffwll.local>

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.

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