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? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
