| Message ID | 20161020150423.4560-6-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On to, 2016-10-20 at 16:03 +0100, Chris Wilson wrote: > At the moment, we have dependency on the RPM as a barrier itself in both > i915_gem_release_all_mmaps() and i915_gem_restore_fences(). > i915_gem_restore_fences() is also called along !runtime pm paths, but we > can move the markup of lost fences alongside releasing the mmaps into a > common i915_gem_runtime_suspend(). This has the advantage of locating > all the tricky barrier dependencies into one location. > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_drv.c | 6 ++---- > drivers/gpu/drm/i915/i915_drv.h | 3 ++- > drivers/gpu/drm/i915/i915_gem.c | 25 +++++++++++++++++++++++-- > drivers/gpu/drm/i915/i915_gem_fence.c | 12 +++++------- > 4 files changed, 32 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 885d33f341f3..99e4e044e958 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -2278,10 +2278,8 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv, > > vlv_check_no_gt_access(dev_priv); > > - if (rpm_resume) { > + if (rpm_resume) > intel_init_clock_gating(dev); > - i915_gem_restore_fences(dev); > - } > > return ret; > } > @@ -2307,7 +2305,7 @@ static int intel_runtime_suspend(struct device *kdev) > * We are safe here against re-faults, since the fault handler takes > * an RPM reference. > */ > - i915_gem_release_all_mmaps(dev_priv); > + i915_gem_runtime_suspend(dev_priv); > > intel_guc_suspend(dev); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c388361ad717..9434734176a3 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3130,9 +3130,10 @@ void i915_vma_destroy(struct i915_vma *vma); > > int i915_gem_object_unbind(struct drm_i915_gem_object *obj); > int i915_gem_object_put_pages(struct drm_i915_gem_object *obj); > -void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv); > void i915_gem_release_mmap(struct drm_i915_gem_object *obj); > > +void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv); > + > int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj); > > static inline int __sg_page_count(struct scatterlist *sg) > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 64a88ce4b3c6..41088c31ada6 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1952,10 +1952,10 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj) > intel_runtime_pm_put(i915); > } > > -void > -i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv) > +void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv) > { > struct drm_i915_gem_object *obj, *on; > + int i; > > /* > * Only called during RPM suspend. All users of the userfault_list > @@ -1970,6 +1970,27 @@ i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv) > drm_vma_node_unmap(&obj->base.vma_node, > obj->base.dev->anon_inode->i_mapping); > } > + > + /* The fence will be lost when the device powers down. If any were > + * in use by hardware (i.e. they are pinned), we should not be powering > + * down! All other fences will be reacquired by the user upon waking. > + */ > + for (i = 0; i < dev_priv->num_fence_regs; i++) { > + struct drm_i915_fence_reg *reg = &dev_priv->fence_regs[i]; > + struct i915_vma *vma; > + > + if (WARN_ON(reg->pin_count)) > + continue; > + > + vma = fetch_and_zero(®->vma); > + if (!vma) > + continue; > + > + GEM_BUG_ON(!list_empty(&vma->obj->userfault_link)); > + > + list_move(®->link, &dev_priv->mm.fence_list); > + vma->fence = NULL; I suppose the obj can be still bound, so shouldn't the fence reg itself be zeroed as well? Or are you counting with these regs getting reset due to suspend? Not sure if that's guaranteed. > + } > } > > /** > diff --git a/drivers/gpu/drm/i915/i915_gem_fence.c b/drivers/gpu/drm/i915/i915_gem_fence.c > index 67013179b8ed..3c5a8082cac3 100644 > --- a/drivers/gpu/drm/i915/i915_gem_fence.c > +++ b/drivers/gpu/drm/i915/i915_gem_fence.c > @@ -343,6 +343,9 @@ i915_vma_get_fence(struct i915_vma *vma) > struct drm_i915_fence_reg *fence; > struct i915_vma *set = i915_gem_object_is_tiled(vma->obj) ? vma : NULL; > > + /* Note that we revoke fences on runtime suspend. Therefore the user > + * must keep the device awake whilst using the fence. > + */ > assert_rpm_wakelock_held(to_i915(vma->vm->dev)); > > /* Just update our place in the LRU if our fence is getting reused. */ > @@ -368,19 +371,14 @@ i915_vma_get_fence(struct i915_vma *vma) > * @dev: DRM device > * > * Restore the hw fence state to match the software tracking again, to be called > - * after a gpu reset and on resume. > + * after a gpu reset and on resume. Note that on runtime suspend we only cancel > + * the fences, to be reacquired by the user later. > */ > void i915_gem_restore_fences(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = to_i915(dev); > int i; > > - /* Note that this may be called outside of struct_mutex, by > - * runtime suspend/resume. The barrier we require is enforced by > - * rpm itself - all access to fences/GTT are only within an rpm > - * wakeref, and to acquire that wakeref you must pass through here. > - */ > - > for (i = 0; i < dev_priv->num_fence_regs; i++) { > struct drm_i915_fence_reg *reg = &dev_priv->fence_regs[i]; > struct i915_vma *vma = reg->vma;
On Fri, Oct 21, 2016 at 03:48:27PM +0300, Imre Deak wrote: > On to, 2016-10-20 at 16:03 +0100, Chris Wilson wrote: > > At the moment, we have dependency on the RPM as a barrier itself in both > > i915_gem_release_all_mmaps() and i915_gem_restore_fences(). > > i915_gem_restore_fences() is also called along !runtime pm paths, but we > > can move the markup of lost fences alongside releasing the mmaps into a > > common i915_gem_runtime_suspend(). This has the advantage of locating > > all the tricky barrier dependencies into one location. > > > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 6 ++---- > > drivers/gpu/drm/i915/i915_drv.h | 3 ++- > > drivers/gpu/drm/i915/i915_gem.c | 25 +++++++++++++++++++++++-- > > drivers/gpu/drm/i915/i915_gem_fence.c | 12 +++++------- > > 4 files changed, 32 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 885d33f341f3..99e4e044e958 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -2278,10 +2278,8 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv, > > > > vlv_check_no_gt_access(dev_priv); > > > > - if (rpm_resume) { > > + if (rpm_resume) > > intel_init_clock_gating(dev); > > - i915_gem_restore_fences(dev); > > - } > > > > return ret; > > } > > @@ -2307,7 +2305,7 @@ static int intel_runtime_suspend(struct device *kdev) > > * We are safe here against re-faults, since the fault handler takes > > * an RPM reference. > > */ > > - i915_gem_release_all_mmaps(dev_priv); > > + i915_gem_runtime_suspend(dev_priv); > > > > intel_guc_suspend(dev); > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index c388361ad717..9434734176a3 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -3130,9 +3130,10 @@ void i915_vma_destroy(struct i915_vma *vma); > > > > int i915_gem_object_unbind(struct drm_i915_gem_object *obj); > > int i915_gem_object_put_pages(struct drm_i915_gem_object *obj); > > -void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv); > > void i915_gem_release_mmap(struct drm_i915_gem_object *obj); > > > > +void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv); > > + > > int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj); > > > > static inline int __sg_page_count(struct scatterlist *sg) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 64a88ce4b3c6..41088c31ada6 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -1952,10 +1952,10 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj) > > intel_runtime_pm_put(i915); > > } > > > > -void > > -i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv) > > +void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv) > > { > > struct drm_i915_gem_object *obj, *on; > > + int i; > > > > /* > > * Only called during RPM suspend. All users of the userfault_list > > @@ -1970,6 +1970,27 @@ i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv) > > drm_vma_node_unmap(&obj->base.vma_node, > > obj->base.dev->anon_inode->i_mapping); > > } > > + > > + /* The fence will be lost when the device powers down. If any were > > + * in use by hardware (i.e. they are pinned), we should not be powering > > + * down! All other fences will be reacquired by the user upon waking. > > + */ > > + for (i = 0; i < dev_priv->num_fence_regs; i++) { > > + struct drm_i915_fence_reg *reg = &dev_priv->fence_regs[i]; > > + struct i915_vma *vma; > > + > > + if (WARN_ON(reg->pin_count)) > > + continue; > > + > > + vma = fetch_and_zero(®->vma); > > + if (!vma) > > + continue; > > + > > + GEM_BUG_ON(!list_empty(&vma->obj->userfault_link)); > > + > > + list_move(®->link, &dev_priv->mm.fence_list); > > + vma->fence = NULL; > > I suppose the obj can be still bound, so shouldn't the fence reg itself > be zeroed as well? Or are you counting with these regs getting reset > due to suspend? Not sure if that's guaranteed. No, they would be rewritten before use to a bound object. The problem lies not with the current set of objects, but a new object that is allocated into the region of the stale fence. Hmm. rather than removing the link between the object and fence, all I need to do is mark the fence as dirty... Then if we relinquish the fence later (rpm will be active) we will clear it. If we try to use the object, we will rewrite the fence register. -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 885d33f341f3..99e4e044e958 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -2278,10 +2278,8 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv, vlv_check_no_gt_access(dev_priv); - if (rpm_resume) { + if (rpm_resume) intel_init_clock_gating(dev); - i915_gem_restore_fences(dev); - } return ret; } @@ -2307,7 +2305,7 @@ static int intel_runtime_suspend(struct device *kdev) * We are safe here against re-faults, since the fault handler takes * an RPM reference. */ - i915_gem_release_all_mmaps(dev_priv); + i915_gem_runtime_suspend(dev_priv); intel_guc_suspend(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c388361ad717..9434734176a3 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3130,9 +3130,10 @@ void i915_vma_destroy(struct i915_vma *vma); int i915_gem_object_unbind(struct drm_i915_gem_object *obj); int i915_gem_object_put_pages(struct drm_i915_gem_object *obj); -void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv); void i915_gem_release_mmap(struct drm_i915_gem_object *obj); +void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv); + int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj); static inline int __sg_page_count(struct scatterlist *sg) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 64a88ce4b3c6..41088c31ada6 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1952,10 +1952,10 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj) intel_runtime_pm_put(i915); } -void -i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv) +void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv) { struct drm_i915_gem_object *obj, *on; + int i; /* * Only called during RPM suspend. All users of the userfault_list @@ -1970,6 +1970,27 @@ i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv) drm_vma_node_unmap(&obj->base.vma_node, obj->base.dev->anon_inode->i_mapping); } + + /* The fence will be lost when the device powers down. If any were + * in use by hardware (i.e. they are pinned), we should not be powering + * down! All other fences will be reacquired by the user upon waking. + */ + for (i = 0; i < dev_priv->num_fence_regs; i++) { + struct drm_i915_fence_reg *reg = &dev_priv->fence_regs[i]; + struct i915_vma *vma; + + if (WARN_ON(reg->pin_count)) + continue; + + vma = fetch_and_zero(®->vma); + if (!vma) + continue; + + GEM_BUG_ON(!list_empty(&vma->obj->userfault_link)); + + list_move(®->link, &dev_priv->mm.fence_list); + vma->fence = NULL; + } } /** diff --git a/drivers/gpu/drm/i915/i915_gem_fence.c b/drivers/gpu/drm/i915/i915_gem_fence.c index 67013179b8ed..3c5a8082cac3 100644 --- a/drivers/gpu/drm/i915/i915_gem_fence.c +++ b/drivers/gpu/drm/i915/i915_gem_fence.c @@ -343,6 +343,9 @@ i915_vma_get_fence(struct i915_vma *vma) struct drm_i915_fence_reg *fence; struct i915_vma *set = i915_gem_object_is_tiled(vma->obj) ? vma : NULL; + /* Note that we revoke fences on runtime suspend. Therefore the user + * must keep the device awake whilst using the fence. + */ assert_rpm_wakelock_held(to_i915(vma->vm->dev)); /* Just update our place in the LRU if our fence is getting reused. */ @@ -368,19 +371,14 @@ i915_vma_get_fence(struct i915_vma *vma) * @dev: DRM device * * Restore the hw fence state to match the software tracking again, to be called - * after a gpu reset and on resume. + * after a gpu reset and on resume. Note that on runtime suspend we only cancel + * the fences, to be reacquired by the user later. */ void i915_gem_restore_fences(struct drm_device *dev) { struct drm_i915_private *dev_priv = to_i915(dev); int i; - /* Note that this may be called outside of struct_mutex, by - * runtime suspend/resume. The barrier we require is enforced by - * rpm itself - all access to fences/GTT are only within an rpm - * wakeref, and to acquire that wakeref you must pass through here. - */ - for (i = 0; i < dev_priv->num_fence_regs; i++) { struct drm_i915_fence_reg *reg = &dev_priv->fence_regs[i]; struct i915_vma *vma = reg->vma;
At the moment, we have dependency on the RPM as a barrier itself in both i915_gem_release_all_mmaps() and i915_gem_restore_fences(). i915_gem_restore_fences() is also called along !runtime pm paths, but we can move the markup of lost fences alongside releasing the mmaps into a common i915_gem_runtime_suspend(). This has the advantage of locating all the tricky barrier dependencies into one location. Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_drv.c | 6 ++---- drivers/gpu/drm/i915/i915_drv.h | 3 ++- drivers/gpu/drm/i915/i915_gem.c | 25 +++++++++++++++++++++++-- drivers/gpu/drm/i915/i915_gem_fence.c | 12 +++++------- 4 files changed, 32 insertions(+), 14 deletions(-)