Re: [PATCH 4/8] drm/i915: Use local pipe_config varariable when available

From: Daniel Vetter <daniel@ffwll.ch>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Ander Conselvan de Oliveira
	<ander.conselvan.de.oliveira@intel.com>,
	intel-gfx@lists.freedesktop.org, shuang.he@linux.intel.com
Subject: Re: [PATCH 4/8] drm/i915: Use local pipe_config varariable when available
Date: Tue, 16 Dec 2014 10:25:12 +0100	[thread overview]
Message-ID: <20141216092512.GP2711@phenom.ffwll.local> (raw)
In-Reply-To: <20141215192141.GG10649@intel.com>

On Mon, Dec 15, 2014 at 09:21:41PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 15, 2014 at 10:37:57AM -0800, Matt Roper wrote:
> > On Thu, Dec 11, 2014 at 02:38:07PM +0200, Ander Conselvan de Oliveira wrote:
> > > @@ -7541,7 +7541,7 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc,
> > >  void intel_dp_get_m_n(struct intel_crtc *crtc,
> > >  		      struct intel_crtc_state *pipe_config)
> > >  {
> > > -	if (crtc->config.has_pch_encoder)
> > > +	if (pipe_config->has_pch_encoder)
> > 
> > I think this one might also change the semantics?  The callchain
> > 
> >    check_crtc_state() -> intel_{dp,ddi}_get_config() -> intel_dp_get_m_n()
> > 
> > passes down a fresh pipe_config() that isn't the same structure stored
> > in crtc->config (although it hopefully has the same values in the end).
> 
> This looks like a straight up bug fix to me. Apparently I already
> fumbled it when I introduced the DP M/N readout.
> 
> The state checker hasn't tripped on this since we've already
> written the new pipe config to crtc->config by the time we do the
> readout during modeset.
> 
> And intel_modeset_readout_hw_state() reads the state directly into
> crtc->config, so even the initial state would have come out correct.
> 
> So, while the code is wrong, doesn't look like it could have caused
> any real issues.

We also have very light coverage of direct modeset changes (i.e. without
going through off in-between). Iirc most of the igts shut down stuff in
between before setting the new modes (because that's the only way to get
somewhat reliably modesets without atomic updates). I think we should
expect more fun in this area once we have real atomic modesets.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx