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:52:07 +0100 Message-ID: <20121126215207.GG32309@phenom.ffwll.local> References: <1352121653-30904-1-git-send-email-ville.syrjala@linux.intel.com> <20121126212128.GF32309@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ee0-f49.google.com (mail-ee0-f49.google.com [74.125.83.49]) by gabe.freedesktop.org (Postfix) with ESMTP id 7EADCE5C7B for ; Mon, 26 Nov 2012 13:50:44 -0800 (PST) Received: by mail-ee0-f49.google.com with SMTP id c4so4843604eek.36 for ; Mon, 26 Nov 2012 13:50:43 -0800 (PST) Content-Disposition: inline In-Reply-To: <20121126212128.GF32309@phenom.ffwll.local> 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 26, 2012 at 10:21:28PM +0100, Daniel Vetter wrote: > On Mon, Nov 05, 2012 at 03:20:53PM +0200, ville.syrjala@linux.intel.com w= rote: > > 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/i91= 5/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 d= rm_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 *= crtc) > > +{ > > + 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 ... Ok, the patch is actually correct, I just couldn't read C code for a moment. Still I think a v3 with the superflous wait_event in finish_fb ditched would look nice. Now Chris reminded me that you have a few other patches around this in the "drm/i915: i915_gem_execbuffer_wait_for_flips and other flip stuff" thread. And I have a few reset state transition improvements in the pipeline for 3.9 at http://cgit.freedesktop.org/~danvet/drm/log/?h=3Drobustify-reset-transitions Can you please rebase that entire patch series on top of those patches? For patch 3 I think we should go with option b) and outright kill the wait-for-flips in the execbuf ioctl. Maybe harrass Jesse and Kristian for a formal ack on that one. Also, if you can throw an additional patch on top to properly clear any outstanding flips when the gpu hangs, that would be awesome - since currently pageflips will get busted if the gpu hangs. And if you could throw a testcase in for fun ... Cheers, Daniel PS: The testcase should be easy to pull of by adapting ZZ_hangman with a different workload. Like flip_test ... -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch