From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Flush the pending flips on the CRTC before modification Date: Fri, 28 Sep 2012 13:45:03 +0300 Message-ID: <20120928104503.GN19732@intel.com> References: <1348777558-17881-1-git-send-email-chris@chris-wilson.co.uk> <20120928100551.GM19732@intel.com> <275ffc$6pg4hg@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 mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 254B19E844 for ; Fri, 28 Sep 2012 03:45:07 -0700 (PDT) Content-Disposition: inline In-Reply-To: <275ffc$6pg4hg@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, Sep 28, 2012 at 11:22:45AM +0100, Chris Wilson wrote: > On Fri, 28 Sep 2012 13:05:51 +0300, Ville Syrj=E4l=E4 wrote: > > On Thu, Sep 27, 2012 at 09:25:58PM +0100, Chris Wilson wrote: > > > This was meant to be the purpose of the > > > intel_crtc_wait_for_pending_flips() function which is called whilst > > > preparing the CRTC for a modeset or before disabling. However, as Vil= le > > > Syrjala pointed out, we set the pending flip notification on the old > > > framebuffer that is no longer attached to the CRTC by the time we come > > > to flush the pending operations. Instead, we can simply wait on the > > > pending unpin work to be finished on this CRTC, knowning that the > > > hardware has therefore finished modifying the registers, before proce= eding > > > with our direct access. > > > = > > > Signed-off-by: Chris Wilson > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++++++++++++-- > > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > = > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i= 915/intel_display.c > > > index a262326..39df185 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -2896,15 +2896,36 @@ static void ironlake_fdi_disable(struct drm_c= rtc *crtc) > > > udelay(100); > > > } > > > = > > > +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); > > = > > The locking looks pointless here. > = > It does rather. Being pedagogical we should probably leave a mb of some > sort in there... > = > pending =3D to_intel_crtc(crtc)->unpin_work !=3D NULL; > smp_rmb(); > = > with the existing spin_lock providing the necessary barriers before the > wake_up(); IIRC wake_up()/wait_event() already have the necessary barriers. And based on a quick glance Documentation/memory-barriers.txt seems to agree with me. -- = Ville Syrj=E4l=E4 Intel OTC