From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v3 2/4] drm/i915: Wait for pending flips in intel_pipe_set_base() Date: Thu, 29 Nov 2012 13:26:28 +0200 Message-ID: <20121129112628.GE21547@intel.com> References: <1354041298-18898-1-git-send-email-ville.syrjala@linux.intel.com> <1354041298-18898-3-git-send-email-ville.syrjala@linux.intel.com> <20121128205118.GB3202@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 61F88E6546 for ; Thu, 29 Nov 2012 03:26:47 -0800 (PST) Content-Disposition: inline In-Reply-To: <20121128205118.GB3202@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: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, Nov 28, 2012 at 09:51:18PM +0100, Daniel Vetter wrote: > On Tue, Nov 27, 2012 at 08:34:56PM +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. > > = > > v2: Shuffle the code around so that intel_crtc_wait_for_pending_flips() > > just wraps intel_crtc_wait_for_pending_flips_locked(). > > = > > v3: Kill leftover wait_event() in intel_finish_fb() > > = > > Signed-off-by: Ville Syrj=E4l=E4 > > Reviewed-by: Chris Wilson > > --- > > drivers/gpu/drm/i915/intel_display.c | 80 +++++++++++++++++---------= ------- > > 1 files changed, 41 insertions(+), 39 deletions(-) > > = > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i91= 5/intel_display.c > > index 8c2d810..ea710af 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2238,10 +2238,6 @@ intel_finish_fb(struct drm_framebuffer *old_fb) > > bool was_interruptible =3D dev_priv->mm.interruptible; > > int ret; > > = > > - wait_event(dev_priv->pending_flip_queue, > > - i915_reset_in_progress(&dev_priv->gpu_error) || > > - atomic_read(&obj->pending_flip) =3D=3D 0); > > - > > /* Big Hammer, we also need to ensure that any pending > > * MI_WAIT_FOR_EVENT inside a user batch buffer on the > > * current scanout is retired before unpinning the old > > @@ -2284,6 +2280,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 (i915_reset_in_progress(&dev_priv->gpu_error)) > > + return false; > = > This check is not enough, since a full gpu reset might have happened while > we didn't look. Which means that we'll still be waiting forever. To really > close all gaps and races I think we need to full multi-state transistions > like it's already implemented in __wait_seqno (minus the first kick, since > no one is holding the dev->struct_mutex while waiting for a pageflip to > complete). So > - we need a reset_counter here like in wait_seqno > - need to reset the unpin_work state (and fire off any pending drm events > while at it) to unblock kernel waiters and userspace > = > Also: igt test highly preferred ;-) Best would be to convert the hangman > test over to the subtest infrastructure (maybe easier when first ported > python and using argpars, but I don't mind bash's getopt support) and then > adding a new subtestcase which exercises flips vs. hangs. I'm going to plead innocent on this one. I just copied code around, but you wrote it ;) -- = Ville Syrj=E4l=E4 Intel OTC