| Message ID | 1348831796-22631-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > 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/i915/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_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); AFAICS you always have struct_mutex locked in the relevant functions, so no need for an atomic variable.
On Fri, Sep 28, 2012 at 01:07:59PM +0100, Chris Wilson wrote: > On Fri, 28 Sep 2012 15:05:01 +0300, Ville Syrjälä <ville.syrjala@linux.intel.com> 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 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 > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > --- > > > 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/i915/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_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); > > > > 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.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/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_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); } @@ -6389,7 +6394,7 @@ static void do_intel_finish_page_flip(struct drm_device *dev, if (atomic_read(&obj->pending_flip) == 0) 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); } @@ -6690,7 +6695,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); @@ -6715,6 +6720,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); + ret = i915_mutex_lock_interruptible(dev); if (ret) goto cleanup; @@ -6733,6 +6741,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, * the flip occurs and the object is no longer visible. */ atomic_add(1 << intel_crtc->plane, &work->old_fb_obj->pending_flip); + atomic_inc(&intel_crtc->unpin_work_count); ret = dev_priv->display.queue_flip(dev, crtc, fb, obj); if (ret) @@ -6747,6 +6756,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, return 0; cleanup_pending: + atomic_dec(&intel_crtc->unpin_work_count); atomic_sub(1 << intel_crtc->plane, &work->old_fb_obj->pending_flip); drm_gem_object_unreference(&work->old_fb_obj->base); drm_gem_object_unreference(&obj->base); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 5515c45..acc1d08 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -203,6 +203,8 @@ struct intel_crtc { } vblank_work; int fdi_lanes; + atomic_t unpin_work_count; + /* Display surface base address adjustement for pageflips. Note that on * gen4+ this only adjusts up to a tile, offsets within a tile are * handled in the hw itself (with the TILEOFF register). */ @@ -387,7 +389,7 @@ intel_get_crtc_for_plane(struct drm_device *dev, int plane) struct intel_unpin_work { struct work_struct work; - struct drm_device *dev; + struct drm_crtc *crtc; struct drm_i915_gem_object *old_fb_obj; struct drm_i915_gem_object *pending_flip_obj; struct drm_pending_vblank_event *event;
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 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_display.c | 20 +++++++++++++++----- drivers/gpu/drm/i915/intel_drv.h | 4 +++- 2 files changed, 18 insertions(+), 6 deletions(-)