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 outstanding unpin tasks before pageflipping Date: Fri, 28 Sep 2012 15:20:53 +0300 Message-ID: <20120928122053.GQ19732@intel.com> References: <1348831796-22631-1-git-send-email-chris@chris-wilson.co.uk> <20120928120501.GO19732@intel.com> <453bf0$5t4oh8@azsmga001.ch.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 979F89EEF1 for ; Fri, 28 Sep 2012 05:20:56 -0700 (PDT) Content-Disposition: inline In-Reply-To: <453bf0$5t4oh8@azsmga001.ch.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 01:07:59PM +0100, Chris Wilson wrote: > On Fri, 28 Sep 2012 15:05:01 +0300, Ville Syrj=E4l=E4 wrote: > > On Fri, Sep 28, 2012 at 12:29:56PM +0100, Chris Wilson wrote: > > > If we accumulate unpin tasks because we are pageflipping faster than = the > > > system can schedule its workers, we can effectively create a > > > pin-leak. The solution taken here is to limit the number of unpin tas= ks > > > we have per-crtc and to flush those outstanding tasks if we accumulate > > > too many. This should prevent any jitter in the normal case, and also > > > prevent the hang if we should run too fast. > > > = > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=3D46991 > > > Signed-off-by: Chris Wilson > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 20 +++++++++++++++----- > > > drivers/gpu/drm/i915/intel_drv.h | 4 +++- > > > 2 files changed, 18 insertions(+), 6 deletions(-) > > > = > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i= 915/intel_display.c > > > index 04407fd..14f1b51 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -6310,14 +6310,19 @@ static void intel_unpin_work_fn(struct work_s= truct *__work) > > > { > > > struct intel_unpin_work *work =3D > > > container_of(__work, struct intel_unpin_work, work); > > > + struct drm_device *dev =3D work->crtc->dev; > > > = > > > - mutex_lock(&work->dev->struct_mutex); > > > + mutex_lock(&dev->struct_mutex); > > > intel_unpin_fb_obj(work->old_fb_obj); > > > drm_gem_object_unreference(&work->pending_flip_obj->base); > > > drm_gem_object_unreference(&work->old_fb_obj->base); > > > = > > > - intel_update_fbc(work->dev); > > > - mutex_unlock(&work->dev->struct_mutex); > > > + intel_update_fbc(dev); > > > + mutex_unlock(&dev->struct_mutex); > > > + > > > + BUG_ON(atomic_read(&to_intel_crtc(work->crtc)->unpin_work_count) = =3D=3D 0); > > > + atomic_dec(&to_intel_crtc(work->crtc)->unpin_work_count); > > = > > AFAICS you always have struct_mutex locked in the relevant functions, > > so no need for an atomic variable. > = > It's not in every case, since we need to do the flush without holding > the lock, we have the choice of making this variable atomic, or taking > and dropping the lock. Obviously I choose the former. Ah right. I missed the conditional flush. -- = Ville Syrj=E4l=E4 Intel OTC