From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Move vblank enable earlier and disable later Date: Thu, 14 Aug 2014 21:25:20 +0200 Message-ID: <20140814192520.GC10500@phenom.ffwll.local> References: <1408043077-12002-1-git-send-email-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-we0-f180.google.com (mail-we0-f180.google.com [74.125.82.180]) by gabe.freedesktop.org (Postfix) with ESMTP id D41AE6E07A for ; Thu, 14 Aug 2014 12:25:09 -0700 (PDT) Received: by mail-we0-f180.google.com with SMTP id w61so1531892wes.11 for ; Thu, 14 Aug 2014 12:25:09 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1408043077-12002-1-git-send-email-ville.syrjala@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: ville.syrjala@linux.intel.com Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Aug 14, 2014 at 10:04:37PM +0300, ville.syrjala@linux.intel.com wro= te: > From: Ville Syrj=E4l=E4 > = > We changed to an interrupt based vblank wait (as opposed to polling) > in: > commit 44bd93a3d367913d883be6abba9a6e51a53c4e90 > Author: Daniel Vetter > Date: Fri Jul 25 23:36:44 2014 +0200 > = > drm/i915: Use generic vblank wait > = > However we already had vblank waits on the wrong side of > drm_vblank_{on,off}() calls due to various workarounds, so now we get > a warning more or less every time we do a modeset, and we fail to > wait for the vblank like we should. > = > Move the drm_vblank_{on,off}() calls back out from > intel_crtc_{enable,disable}_planes() so that all of these vblank waits > return to proper operation. Also move the cxsr wait a bit earlier so > that we can keep the encoder disable after we've turned off vblanks. > Moving stuff out from the plane enable/disable functions seems > preferrable to moving the workaround stuff in since the workarounds are > required only on specific platforms. > = > While at it switch over to the drm_crtc_ variants of the vblank on/off > functions. > = > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=3D82525 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=3D82490 > Cc: Daniel Vetter > Signed-off-by: Ville Syrj=E4l=E4 I've dropped the offending patch temporarily and picked up yours here into my atomic branch - currently there's too many things unmerged to get this in. Thanks anyway for the patch. > --- > drivers/gpu/drm/i915/intel_display.c | 35 ++++++++++++++++++++++++------= ----- > 1 file changed, 24 insertions(+), 11 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= intel_display.c > index 3813526..970c2ab 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3845,10 +3845,6 @@ static void intel_crtc_enable_planes(struct drm_cr= tc *crtc) > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > int pipe =3D intel_crtc->pipe; > = > - assert_vblank_disabled(crtc); > - > - drm_vblank_on(dev, pipe); > - > intel_enable_primary_hw_plane(crtc->primary, crtc); > intel_enable_planes(crtc); > intel_crtc_update_cursor(crtc, true); > @@ -3894,10 +3890,6 @@ static void intel_crtc_disable_planes(struct drm_c= rtc *crtc) > * consider this a flip to a NULL plane. > */ > intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe)); > - > - drm_vblank_off(dev, pipe); > - > - assert_vblank_disabled(crtc); > } > = > static void ironlake_crtc_enable(struct drm_crtc *crtc) > @@ -3967,6 +3959,9 @@ static void ironlake_crtc_enable(struct drm_crtc *c= rtc) > if (HAS_PCH_CPT(dev)) > cpt_verify_modeset(dev, intel_crtc->pipe); > = > + assert_vblank_disabled(crtc); > + drm_crtc_vblank_on(crtc); > + > intel_crtc_enable_planes(crtc); > } > = > @@ -4074,6 +4069,9 @@ static void haswell_crtc_enable(struct drm_crtc *cr= tc) > intel_opregion_notify_encoder(encoder, true); > } > = > + assert_vblank_disabled(crtc); > + drm_crtc_vblank_on(crtc); > + > /* If we change the relative order between pipe/planes enabling, we need > * to change the workaround. */ > haswell_mode_set_planes_workaround(intel_crtc); > @@ -4109,6 +4107,9 @@ static void ironlake_crtc_disable(struct drm_crtc *= crtc) > = > intel_crtc_disable_planes(crtc); > = > + drm_crtc_vblank_off(crtc); > + assert_vblank_disabled(crtc); > + > for_each_encoder_on_crtc(dev, crtc, encoder) > encoder->disable(encoder); > = > @@ -4175,6 +4176,9 @@ static void haswell_crtc_disable(struct drm_crtc *c= rtc) > = > intel_crtc_disable_planes(crtc); > = > + drm_crtc_vblank_off(crtc); > + assert_vblank_disabled(crtc); > + > for_each_encoder_on_crtc(dev, crtc, encoder) { > intel_opregion_notify_encoder(encoder, false); > encoder->disable(encoder); > @@ -4638,6 +4642,9 @@ static void valleyview_crtc_enable(struct drm_crtc = *crtc) > for_each_encoder_on_crtc(dev, crtc, encoder) > encoder->enable(encoder); > = > + assert_vblank_disabled(crtc); > + drm_crtc_vblank_on(crtc); > + > intel_crtc_enable_planes(crtc); > = > /* Underruns don't raise interrupts, so check manually. */ > @@ -4695,6 +4702,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) > for_each_encoder_on_crtc(dev, crtc, encoder) > encoder->enable(encoder); > = > + assert_vblank_disabled(crtc); > + drm_crtc_vblank_on(crtc); > + > intel_crtc_enable_planes(crtc); > = > /* > @@ -4758,9 +4768,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > intel_set_memory_cxsr(dev_priv, false); > intel_crtc_disable_planes(crtc); > = > - for_each_encoder_on_crtc(dev, crtc, encoder) > - encoder->disable(encoder); > - > /* > * On gen2 planes are double buffered but the pipe isn't, so we must > * wait for planes to fully turn off before disabling the pipe. > @@ -4769,6 +4776,12 @@ static void i9xx_crtc_disable(struct drm_crtc *crt= c) > */ > intel_wait_for_vblank(dev, pipe); > = > + drm_crtc_vblank_off(crtc); > + assert_vblank_disabled(crtc); > + > + for_each_encoder_on_crtc(dev, crtc, encoder) > + encoder->disable(encoder); > + > intel_disable_pipe(dev_priv, pipe); > = > i9xx_pfit_disable(intel_crtc); > -- = > 1.8.5.5 > = -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch