From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH v2] drm/i915: Wait for pending flips in intel_pipe_set_base() Date: Mon, 26 Nov 2012 22:21:28 +0100 Message-ID: <20121126212128.GF32309@phenom.ffwll.local> References: <1352121653-30904-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-ea0-f177.google.com (mail-ea0-f177.google.com [209.85.215.177]) by gabe.freedesktop.org (Postfix) with ESMTP id 663EFE5C49 for ; Mon, 26 Nov 2012 13:20:05 -0800 (PST) Received: by mail-ea0-f177.google.com with SMTP id c10so2800022eaa.36 for ; Mon, 26 Nov 2012 13:20:04 -0800 (PST) Content-Disposition: inline In-Reply-To: <1352121653-30904-1-git-send-email-ville.syrjala@linux.intel.com> 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: ville.syrjala@linux.intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Mon, Nov 05, 2012 at 03:20:53PM +0200, ville.syrjala@linux.intel.com wro= te: > From: Ville Syrj=E4l=E4 > = > intel_pipe_set_base() never actually waited for any pending page flips > on the CRTC. It looks like it tried to, by calling intel_finish_fb() on > the current front buffer. But the pending flips were actually tracked > in the BO of the previous front buffer, so the call to intel_finish_fb() > never did anything useful. > = > intel_crtc_wait_for_pending_flips() is the current _working_ way to wait > for pending page flips. So use it in intel_pipe_set_base() too. Some > refactoring was necessary to avoid locking struct_mutex twice. > = > Signed-off-by: Ville Syrj=E4l=E4 > --- > v2: Shuffle the code around so that intel_crtc_wait_for_pending_flips() > just wraps intel_crtc_wait_for_pending_flips_locked(). Sorry for the long delay in looking at this. One bikeshed here: I prefer the patch changelog before the sob lines so that it gets included in the commit message - most often it's rather interesting read, especially for patches that take a few revisions to get right. More substantial comment below. > drivers/gpu/drm/i915/intel_display.c | 76 ++++++++++++++++++----------= ----- > 1 files changed, 41 insertions(+), 35 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= intel_display.c > index 1a38267..a18e6e6 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2228,6 +2228,46 @@ static void intel_crtc_update_sarea_pos(struct drm= _crtc *crtc, int x, int y) > } > } > = > +static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc) > +{ > + struct drm_device *dev =3D crtc->dev; > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + unsigned long flags; > + bool pending; > + > + if (atomic_read(&dev_priv->mm.wedged)) > + return false; > + > + spin_lock_irqsave(&dev->event_lock, flags); > + pending =3D to_intel_crtc(crtc)->unpin_work !=3D NULL; > + spin_unlock_irqrestore(&dev->event_lock, flags); > + > + return pending; > +} > + > +static void intel_crtc_wait_for_pending_flips_locked(struct drm_crtc *cr= tc) > +{ > + struct drm_device *dev =3D crtc->dev; > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + > + if (crtc->fb =3D=3D NULL) > + return; > + > + wait_event(dev_priv->pending_flip_queue, > + !intel_crtc_has_pending_flip(crtc)); I think we also need to add a dev_priv->mm.wedged check here, since the gpu might die and never execute the pageflip. Otoh we don't complete any pageflips that never executed due to a gpu hang, so maybe also add a big FIXME. But with the wedged check we should at least not hang in an non-interruptible wait. The other thing is that the wait_even in finish_fb is not superflous, since we should never see a framebuffer with pending flips for _this_ crtc (it could have a pending flip on another crtc). So I think that code in finish_fb should die, leaving just the comment and the finish_gpu call. Cheers, Daniel PS: Testcase would be awesome, but I have no ideas beyond what we already have in flip_test unfortunately ... -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch