Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753363AbbGAO76 (ORCPT ); Wed, 1 Jul 2015 10:59:58 -0400 Received: from mail-wi0-f173.google.com ([209.85.212.173]:33106 "EHLO mail-wi0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753055AbbGAO7t (ORCPT ); Wed, 1 Jul 2015 10:59:49 -0400 Date: Wed, 1 Jul 2015 17:02:37 +0200 From: Daniel Vetter To: Ander Conselvan De Oliveira Cc: Jani Nikula , Daniel Vetter , intel-gfx , Linux Kernel Mailing List , DRI mailing list , Daniel Vetter , Linus Torvalds Subject: Re: [Intel-gfx] [PATCH] drm/i915: Clear pipe's pll hw state in hsw_dp_set_ddi_pll_sel() Message-ID: <20150701150237.GX23343@phenom.ffwll.local> Mail-Followup-To: Ander Conselvan De Oliveira , Jani Nikula , intel-gfx , Linux Kernel Mailing List , DRI mailing list , Daniel Vetter , Linus Torvalds References: <878ub1wdur.fsf@intel.com> <1435669838-24747-1-git-send-email-ander.conselvan.de.oliveira@intel.com> <876165wbol.fsf@intel.com> <20150630142256.GZ30960@phenom.ffwll.local> <87pp4d2ogi.fsf@intel.com> <1435762446.2645.11.camel@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1435762446.2645.11.camel@gmail.com> X-Operating-System: Linux phenom 4.0.0-rc3+ User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5358 Lines: 110 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 wrote: > > > On Tue, Jun 30, 2015 at 04:47:06PM +0300, Jani Nikula wrote: > > >> On Tue, 30 Jun 2015, Ander Conselvan de Oliveira 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 > > >> > 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 > > >> > > >> Reported-by: Linus Torvalds > > >> Tested-by: Jani Nikula > > > > > > 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 > > > 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 > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/