| Message ID | 1360232867-26613-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 07, 2013 at 10:27:47AM +0000, Chris Wilson wrote: > The unbound list is an optimisation to track objects that have been > evicted from the GTT but remain untouched by the CPU. After hibernation, > all memory is in the CPU domain (having been restored from a disk image) > and so we need to restore the domain tracking upon the objects. However, > for the unbound list we can simply discard those objects and lazily wait > for them to be reused. > > v2: Perform the unbound discard explicitly during thawing after > hibernation. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_drv.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 9069e71..f5ccb3d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -592,6 +592,30 @@ static int __i915_drm_thaw(struct drm_device *dev) > return error; > } > > +static void i915_gem_discard_unbound(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_i915_gem_object *obj, *next; > + > + /* After hibernation all memory is in the CPU domain and so we need > + * to restore our domain tracking. However, the unbound list is > + * merely an optimisation to track memory that has been evicted from > + * the GTT but remains in the GTT domain. After hibernation, that > + * is no longer the case and we can trim the unbound list. > + */ > + list_for_each_entry_safe(obj, next, > + &dev_priv->mm.unbound_list, gtt_list) { > + obj->base.read_domains = I915_GEM_DOMAIN_CPU; > + obj->base.write_domain = I915_GEM_DOMAIN_CPU; > + if (i915_gem_object_put_pages(obj)) { > + /* Abandon hope all ye who enter here */ > + obj->base.read_domains = I915_GEM_DOMAIN_GTT; > + obj->base.write_domain = I915_GEM_DOMAIN_GTT; > + /* Will be clflushed during restore_gtt_mappings */ > + } > + } > +} > + I'm confused by how you're going about this. To reorder what you said, if an object is on the unbound list the GPU is done with it, and it may require clflushing if on a non-LLC system. Then a hibernate comes along. Almost certainly the PM subsystem would have to invalidate all the caches before moving the image to disk. Assuming the above is true: First question, why do you want to i915_gem_object_put_pages(). This would seem to do a bunch of unnecessary stuff (primarily setting pages as dirty when they need not be). and second, why do you set I915_GEM_DOMAIN_GTT when put_pages fails? Is that just a hack to get the clflush? I think if you can convince me why we need to clflush after resume, I'd be happy. > static int i915_drm_thaw(struct drm_device *dev) > { > int error = 0; > @@ -600,6 +624,7 @@ static int i915_drm_thaw(struct drm_device *dev) > > if (drm_core_check_feature(dev, DRIVER_MODESET)) { > mutex_lock(&dev->struct_mutex); > + i915_gem_discard_unbound(dev); > i915_gem_restore_gtt_mappings(dev); > mutex_unlock(&dev->struct_mutex); > } > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Sun, Feb 10, 2013 at 11:04:17AM -0800, Ben Widawsky wrote: > I'm confused by how you're going about this. To reorder what you said, > if an object is on the unbound list the GPU is done with it, and it may > require clflushing if on a non-LLC system. Right, the objects on the unbound list are explicitly not in the CPU domain and so we believe we can skip the clflush next time we try and use them on the GPU. > Then a hibernate comes along. Almost certainly the PM subsystem would > have to invalidate all the caches before moving the image to disk. s/PM/we/ The PM subsystem just concerns itself with moving pages from ram to disk and back. > Assuming the above is true: > First question, why do you want to i915_gem_object_put_pages(). This > would seem to do a bunch of unnecessary stuff (primarily setting pages > as dirty when they need not be). Not unnecessary. Even after hibernation since we need to keep the VM bookkeeping happy and working in our favour. > and second, why do you set I915_GEM_DOMAIN_GTT when put_pages fails? > Is that just a hack to get the clflush? Right. So that our invariants about the unbound list hold. > I think if you can convince me why we need to clflush after resume, I'd > be happy. Because moving pages around trashed the CPU caches and upset our domain tracking. The clflush is then required to move the object back into the domain we believe it to be. Otherwise we end up with the GPU reading stale data and hanging. -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9069e71..f5ccb3d 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -592,6 +592,30 @@ static int __i915_drm_thaw(struct drm_device *dev) return error; } +static void i915_gem_discard_unbound(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_i915_gem_object *obj, *next; + + /* After hibernation all memory is in the CPU domain and so we need + * to restore our domain tracking. However, the unbound list is + * merely an optimisation to track memory that has been evicted from + * the GTT but remains in the GTT domain. After hibernation, that + * is no longer the case and we can trim the unbound list. + */ + list_for_each_entry_safe(obj, next, + &dev_priv->mm.unbound_list, gtt_list) { + obj->base.read_domains = I915_GEM_DOMAIN_CPU; + obj->base.write_domain = I915_GEM_DOMAIN_CPU; + if (i915_gem_object_put_pages(obj)) { + /* Abandon hope all ye who enter here */ + obj->base.read_domains = I915_GEM_DOMAIN_GTT; + obj->base.write_domain = I915_GEM_DOMAIN_GTT; + /* Will be clflushed during restore_gtt_mappings */ + } + } +} + static int i915_drm_thaw(struct drm_device *dev) { int error = 0; @@ -600,6 +624,7 @@ static int i915_drm_thaw(struct drm_device *dev) if (drm_core_check_feature(dev, DRIVER_MODESET)) { mutex_lock(&dev->struct_mutex); + i915_gem_discard_unbound(dev); i915_gem_restore_gtt_mappings(dev); mutex_unlock(&dev->struct_mutex); }
The unbound list is an optimisation to track objects that have been evicted from the GTT but remain untouched by the CPU. After hibernation, all memory is in the CPU domain (having been restored from a disk image) and so we need to restore the domain tracking upon the objects. However, for the unbound list we can simply discard those objects and lazily wait for them to be reused. v2: Perform the unbound discard explicitly during thawing after hibernation. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)