From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Wait for pending flips to complete before tearing down the encoders Date: Thu, 14 Feb 2013 20:15:10 +0200 Message-ID: <20130214181509.GY9135@intel.com> References: <1360779408-21705-1-git-send-email-chris@chris-wilson.co.uk> <20130214141850.GW9135@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id BD023E5DC5 for ; Thu, 14 Feb 2013 10:15:13 -0800 (PST) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Feb 14, 2013 at 04:53:57PM +0100, Daniel Vetter wrote: > On Thu, Feb 14, 2013 at 3:18 PM, Ville Syrj=E4l=E4 > wrote: > > On Wed, Feb 13, 2013 at 06:16:48PM +0000, Chris Wilson wrote: > >> If we start disabling the encoders, there is a potential for a pending > >> flip to never occur - and so we will end up waiting indefinitely. > > > > To me that would indicate that the encoder disable hooks either kill > > the vblank signal or the entire pixel clock, so that the flip never > > completes. > > > > But if that is the case, then shouldn't we also disable all planes and > > indeed the whole pipe before calling the encoder disabled hooks? > = > I think the current code is actually correct, since for all the output > ports where disabling the port kills the frame start signal (and so > the vblank counter afaik) we only disable the port in the post_disable > hook. That's hsw ddi and cpu eDP. Hmm. But then how do you explain the bug this is supposed to fix? > But I think it's generally a nicer control flow when we block out & > sync with vblank/pipe users before we start to turn things off. Note > that concurrent calls to the wait vblank ioctl can still race since > that doesn't grab the crtc lock and so could sneak in between the > vblank_off call and us actually disabling the hw pipe. But since X is > single-threaded I don't care one iota about this race for now ;-) Sure we can do the change just for being nicer, but I'd like to understand why it apparently fixes a real issue w/ flips not completing... Didn't we also have some bug about pipe disable timing out? That also smells like the same problem. > -Daniel > = > > > >> > >> v2: Also pre-emptively perform the drm_vblank_off() before switching o= ff > >> the encoders. > >> > >> References: https://bugs.launchpad.net/ubuntu/+source/xserver-xorg-vid= eo-intel/+bug/1097315 > >> Cc: Timo Aaltonen > >> Cc: Daniel Vetter > >> Signed-off-by: Chris Wilson > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 17 +++++++++-------- > >> 1 file changed, 9 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i9= 15/intel_display.c > >> index da1ad9c..15cc838 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -3486,15 +3486,15 @@ static void ironlake_crtc_disable(struct drm_c= rtc *crtc) > >> int plane =3D intel_crtc->plane; > >> u32 reg, temp; > >> > >> - > >> if (!intel_crtc->active) > >> return; > >> > >> + intel_crtc_wait_for_pending_flips(crtc); > >> + drm_vblank_off(dev, pipe); > >> + > >> for_each_encoder_on_crtc(dev, crtc, encoder) > >> encoder->disable(encoder); > >> > >> - intel_crtc_wait_for_pending_flips(crtc); > >> - drm_vblank_off(dev, pipe); > >> intel_crtc_update_cursor(crtc, false); > >> > >> intel_disable_plane(dev_priv, plane, pipe); > >> @@ -3570,13 +3570,14 @@ static void haswell_crtc_disable(struct drm_cr= tc *crtc) > >> if (!intel_crtc->active) > >> return; > >> > >> + intel_crtc_wait_for_pending_flips(crtc); > >> + drm_vblank_off(dev, pipe); > >> + > >> is_pch_port =3D haswell_crtc_driving_pch(crtc); > >> > >> for_each_encoder_on_crtc(dev, crtc, encoder) > >> encoder->disable(encoder); > >> > >> - intel_crtc_wait_for_pending_flips(crtc); > >> - drm_vblank_off(dev, pipe); > >> intel_crtc_update_cursor(crtc, false); > >> > >> intel_disable_plane(dev_priv, plane, pipe); > >> @@ -3687,16 +3688,16 @@ static void i9xx_crtc_disable(struct drm_crtc = *crtc) > >> int pipe =3D intel_crtc->pipe; > >> int plane =3D intel_crtc->plane; > >> > >> - > >> if (!intel_crtc->active) > >> return; > >> > >> + intel_crtc_wait_for_pending_flips(crtc); > >> + drm_vblank_off(dev, pipe); > >> + > >> for_each_encoder_on_crtc(dev, crtc, encoder) > >> encoder->disable(encoder); > >> > >> /* Give the overlay scaler a chance to disable if it's on this p= ipe */ > >> - intel_crtc_wait_for_pending_flips(crtc); > >> - drm_vblank_off(dev, pipe); > >> intel_crtc_dpms_overlay(intel_crtc, false); > >> intel_crtc_update_cursor(crtc, false); > >> > >> -- > >> 1.7.10.4 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrj=E4l=E4 > > Intel OTC > = > = > = > -- = > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- = Ville Syrj=E4l=E4 Intel OTC