From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH] drm/i915: Flush outstanding unpin tasks before pageflipping Date: Thu, 1 Nov 2012 08:07:59 -0700 Message-ID: <20121101080759.43d90dd3@jbarnes-desktop> References: <1351761986-27982-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from oproxy9.bluehost.com (oproxy9.bluehost.com [69.89.24.6]) by gabe.freedesktop.org (Postfix) with SMTP id 84A73A0F10 for ; Thu, 1 Nov 2012 08:07:51 -0700 (PDT) In-Reply-To: <1351761986-27982-1-git-send-email-chris@chris-wilson.co.uk> 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 Thu, 1 Nov 2012 09:26:26 +0000 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 tasks > 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=46991 > Reported-and-tested-by: Tvrtko Ursulin > Signed-off-by: Chris Wilson > --- > drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++++++------ > drivers/gpu/drm/i915/intel_drv.h | 4 +++- > 2 files changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 69b1739..800b195 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6908,14 +6908,19 @@ static void intel_unpin_work_fn(struct work_struct *__work) > { > struct intel_unpin_work *work = > container_of(__work, struct intel_unpin_work, work); > + struct drm_device *dev = 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) == 0); > + atomic_dec(&to_intel_crtc(work->crtc)->unpin_work_count); > + > kfree(work); > } > > @@ -6963,9 +6968,9 @@ static void do_intel_finish_page_flip(struct drm_device *dev, > > atomic_clear_mask(1 << intel_crtc->plane, > &obj->pending_flip.counter); > - > wake_up(&dev_priv->pending_flip_queue); > - schedule_work(&work->work); > + > + queue_work(dev_priv->wq, &work->work); > > trace_i915_flip_complete(intel_crtc->plane, work->pending_flip_obj); > } > @@ -7266,7 +7271,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > return -ENOMEM; > > work->event = event; > - work->dev = crtc->dev; > + work->crtc = crtc; > intel_fb = to_intel_framebuffer(crtc->fb); > work->old_fb_obj = intel_fb->obj; > INIT_WORK(&work->work, intel_unpin_work_fn); > @@ -7291,6 +7296,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > intel_fb = to_intel_framebuffer(fb); > obj = intel_fb->obj; > > + if (atomic_read(&intel_crtc->unpin_work_count) >= 2) > + flush_workqueue(dev_priv->wq); > + Have you by chance tested this with the async flip patch? I wonder if in that case whether 2 is too small, and something like 100 might be better (though really async flips are for cases where we can't keep up with refresh, so a small number shouldn't hurt too much there either). -- Jesse Barnes, Intel Open Source Technology Center