From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Flush outstanding unpin tasks before pageflipping Date: Thu, 1 Nov 2012 16:29:35 +0100 Message-ID: <20121101152935.GU5755@phenom.ffwll.local> References: <1351761986-27982-1-git-send-email-chris@chris-wilson.co.uk> <20121101080759.43d90dd3@jbarnes-desktop> <275ffc$77mr8c@fmsmga002.fm.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f177.google.com (mail-ea0-f177.google.com [209.85.215.177]) by gabe.freedesktop.org (Postfix) with ESMTP id 41B719E75A for ; Thu, 1 Nov 2012 08:28:29 -0700 (PDT) Received: by mail-ea0-f177.google.com with SMTP id n13so1039200eaa.36 for ; Thu, 01 Nov 2012 08:28:28 -0700 (PDT) Content-Disposition: inline In-Reply-To: <275ffc$77mr8c@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 Thu, Nov 01, 2012 at 03:18:46PM +0000, Chris Wilson wrote: > On Thu, 1 Nov 2012 08:07:59 -0700, Jesse Barnes wrote: > > 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). > > The limit on 2 is due to the limited resolution of pincount. Hence my > earlier fear for your async flip patch. I think for asyn flips we simply need to have a real flip queue in our code, instead of abusing the implicit list in the workqueue code ... One other thing is that with async flips we don't have a natural limit on the number of pinned framebuffers any more, which means we can easily exhaust all mappable GTT space. Hence we need to integrate that new, explicit flip queue into our eviction code, too. For now I'm rather happy with the flush_wq ducttape presented here ;-) Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch