| Message ID | 20161222120007.28590-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On to, 2016-12-22 at 12:00 +0000, Chris Wilson wrote: > The idle work handler is self-arming - if it detects that it needs to > run again it will queue itself from its work handler. Take greater care > when trying to drain the idle work, and double check that it is flushed. > > The free worker has a similar issue where it is armed by an RCU task > which may be running concurrently with us. > > This should hopefully help with the sporadic WARN_ON(dev_priv->gt.awake) > from i915_gem_suspend. > > v2: Reuse drain_freed_objects. > v3: Don't try to flush the freed objects from the shrinker, as it may be > underneath the struct_mutex already. <SNIP> > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3207,6 +3207,13 @@ i915_gem_object_create_from_data(struct drm_i915_private *dev_priv, > void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *file); > void i915_gem_free_object(struct drm_gem_object *obj); > > +static inline void i915_gem_drain_freed_objects(struct drm_i915_private *i915) > +{ > + rcu_barrier(); > + while (flush_work(&i915->mm.free_work)) > + rcu_barrier(); Looks much like do { } while(); Also make a comment that we loop just for paranoia. > @@ -4262,8 +4262,10 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv) > > cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); > cancel_delayed_work_sync(&dev_priv->gt.retire_work); > - flush_delayed_work(&dev_priv->gt.idle_work); > - flush_work(&dev_priv->mm.free_work); Put a comment here that idle_work is re-arming. > + while (flush_delayed_work(&dev_priv->gt.idle_work)) > + ; > + > + i915_gem_drain_freed_objects(dev_priv); > > /* Assert that we sucessfully flushed all the work and > * reset the GPU back to its idle, low power state. Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 6428588518aa..2c020eafada6 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -545,8 +545,7 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv) i915_gem_context_fini(dev_priv); mutex_unlock(&dev_priv->drm.struct_mutex); - rcu_barrier(); - flush_work(&dev_priv->mm.free_work); + i915_gem_drain_freed_objects(dev_priv); WARN_ON(!list_empty(&dev_priv->context_list)); } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 77d7a079c51b..37ea557d3cd4 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3207,6 +3207,13 @@ i915_gem_object_create_from_data(struct drm_i915_private *dev_priv, void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *file); void i915_gem_free_object(struct drm_gem_object *obj); +static inline void i915_gem_drain_freed_objects(struct drm_i915_private *i915) +{ + rcu_barrier(); + while (flush_work(&i915->mm.free_work)) + rcu_barrier(); +} + struct i915_vma * __must_check i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj, const struct i915_ggtt_view *view, diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index cc4e0224968f..62e1af5dd1b0 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4262,8 +4262,10 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv) cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); cancel_delayed_work_sync(&dev_priv->gt.retire_work); - flush_delayed_work(&dev_priv->gt.idle_work); - flush_work(&dev_priv->mm.free_work); + while (flush_delayed_work(&dev_priv->gt.idle_work)) + ; + + i915_gem_drain_freed_objects(dev_priv); /* Assert that we sucessfully flushed all the work and * reset the GPU back to its idle, low power state.
The idle work handler is self-arming - if it detects that it needs to run again it will queue itself from its work handler. Take greater care when trying to drain the idle work, and double check that it is flushed. The free worker has a similar issue where it is armed by an RCU task which may be running concurrently with us. This should hopefully help with the sporadic WARN_ON(dev_priv->gt.awake) from i915_gem_suspend. v2: Reuse drain_freed_objects. v3: Don't try to flush the freed objects from the shrinker, as it may be underneath the struct_mutex already. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.c | 3 +-- drivers/gpu/drm/i915/i915_drv.h | 7 +++++++ drivers/gpu/drm/i915/i915_gem.c | 6 ++++-- 3 files changed, 12 insertions(+), 4 deletions(-)