From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base() Date: Fri, 2 Nov 2012 19:26:37 +0200 Message-ID: <20121102172637.GV3791@intel.com> References: <1351793163-8542-1-git-send-email-ville.syrjala@linux.intel.com> <1351793163-8542-2-git-send-email-ville.syrjala@linux.intel.com> <275ffc$785jr7@fmsmga002.fm.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga14.intel.com (mga14.intel.com [143.182.124.37]) by gabe.freedesktop.org (Postfix) with ESMTP id 48A109E9FE for ; Fri, 2 Nov 2012 10:26:41 -0700 (PDT) Content-Disposition: inline In-Reply-To: <275ffc$785jr7@fmsmga002.fm.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: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Nov 02, 2012 at 03:25:59PM +0000, Chris Wilson wrote: > On Thu, 1 Nov 2012 20:06:00 +0200, ville.syrjala@linux.intel.com wrote: > > 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. > > = > > Now even the pending_flip counter is gone, so we should just > > use intel_crtc_wait_for_pending_flips(), but since we're already holding > > struct_mutex when we would call that function, we need another version > > of it, that itself doesn't lock struct_mutex. > > = > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > drivers/gpu/drm/i915/intel_display.c | 51 +++++++++++++++++++++-----= ------- > > 1 files changed, 32 insertions(+), 19 deletions(-) > > = > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i91= 5/intel_display.c > > index 1a38267..7bf4749 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2228,6 +2228,37 @@ 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) > > +{ > = > Can we rearrange this such that the waiting logic is inside _locked() > and then intel_crtc_wiat_for_pending_flips() becomes a wrapper that > acquires the struct_mutex and then calls _locked()? Just to keep the > code simpler at the expense of the pathological case. Yeah that looks doable. It does mean we'll be holding struct_mutex around the wait_event() always. As I was already doing that for the panning case, doing the same in the crtc_disable() case shouldn't be any worse. But now I started to wonder a bit about the performance implications of keeping struct_mutex locked for that long... -- = Ville Syrj=E4l=E4 Intel OTC