On Wed, Jul 01, 2015 at 05:54:06PM +0300, Ander Conselvan De Oliveira wrote: > On Tue, 2015-06-30 at 18:41 +0300, Jani Nikula wrote: > > On Tue, 30 Jun 2015, Daniel Vetter <daniel@xxxxxxxx> wrote: > > > On Tue, Jun 30, 2015 at 04:47:06PM +0300, Jani Nikula wrote: > > >> On Tue, 30 Jun 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx> wrote: > > >> > Similarly to what is done for SKL, clear the dpll_hw_state of the pipe > > >> > config in hsw_dp_set_ddi_pll_sel(), since it main contain stale values. > > >> > That can happen if a crtc that was previously driving an HDMI connector > > >> > switches to a DP connector. In that case, the wrpll field was left with > > >> > its old value, leading to warnings like the one below: > > >> > > > >> > [drm:check_crtc_state [i915]] *ERROR* mismatch in dpll_hw_state.wrpll (expected 0xb035061f, found 0x00000000) > > >> > ------------[ cut here ]------------ > > >> > WARNING: CPU: 1 PID: 767 at drivers/gpu/drm/i915/intel_display.c:12324 check_crtc_state+0x975/0x10b0 [i915]() > > >> > pipe state doesn't match! > > >> > > > >> > This regression was indroduced in > > >> > > > >> > commit dd3cd74acf12723045a64f1f2c6298ac7b34a5d5 > > >> > Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx> > > >> > Date: Fri May 15 13:34:29 2015 +0300 > > >> > > > >> > drm/i915: Don't overwrite (e)DP PLL selection on SKL > > >> > > > >> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx> > > >> > > >> Reported-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> > > >> Tested-by: Jani Nikula <jani.nikula@xxxxxxxxx> > > > > > > Yeah makes sense as a fix for 4.2. But for 4.3 I wonder whether the > > > original commit that started this chain needs to be changed a bit: > > > > > > commit 4978cc93d9ac240b435ce60431aef24239b4c270 > > > Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx> > > > Date: Tue Apr 21 17:13:21 2015 +0300 > > > > > > drm/i915: Preserve shared DPLL information in new pipe_config > > > > > > All the trouble this caused is because it not only preserves the sharing > > > config (in crtc_state->shared_dpll) but also the ->dpll_hw_state. And I > > > think with Maarten's latest code (for 4.3) we'd just do an unconditional > > > compute_config (need it for fast pfit updates and fastboot), which means > > > the bogus values in ->dpll_hw_state aren't a problem any more since we'll > > > overwrite them again. And then we could remove that sprinkle of memsets we > > > have all over, which would be good (since the current approach is > > > obviously a bit fragile). Anyway: > > > > > > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > > > > Pushed to drm-intel-next-fixes, thanks for the patch and review. One > > down, another one left to fix. > > I made some progress on the second issue, but I'm afraid Jani might have > a found a third bug. The warning he gets happens because we try to wait > for vblanks while updating the primary plane during the modeset. At that > point, the crtc is off. The problem is in intel_check_primary_plane(), > which is called from drm_atomic_helper_check_planes(). That function > makes decisions about waiting for a vblank based on intel_crtc->active. > Since the check is called before we disable the crtcs, active might be > true, even though the plane update is done with crtcs disable. > > The patch below makes the warning go away, but I still need to figure > out how to set crtc_state->planes_changed properly if we are going down > that route. > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index dcb1d25..f14727c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12480,10 +12480,6 @@ intel_modeset_compute_config(struct drm_crtc *crtc, > > intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,"[modeset]"); > > - ret = drm_atomic_helper_check_planes(state->dev, state); > - if (ret) > - return ERR_PTR(ret); > - > return pipe_config; > } > > > The backtrace on Linus' machine is different, though. It comes from the > call to intel_crtc_disable_planes() in __intel_set_mode(). That would > indicate we have a crtc with crtc->state->enable == true but that is > actually inactive. I'm still not sure how we can get in that state. Using intel_crtc->active to precompute any kind of decisions won't work. I guess we just need to delay the decision whether to make a vblank wait or not to where we do the vblank wait, and use the (then current intel_crtc->active) there. This will be fixed properly in 4.3. I suspect Linus' backtrace is something similar - we try to precompute what needs to be updated, get it wrong and the go boom. Sprinkling an if (!intel_crtc->active) return; early return into the set_mode should help. But I haven't looked at what 4.2 looks precisely yet in this area - too much flux because of the atomic conversion. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
