drm/i915: Migrate stolen objects before hibernation
diff mbox

Message ID 20150630112019.GJ1381@nuc-i3427.alporthouse.com
State New
Headers show

Commit Message

Chris Wilson June 30, 2015, 11:20 a.m. UTC
On Tue, Jun 30, 2015 at 12:54:02PM +0200, Daniel Vetter wrote:
> > +	list_add(&obj->stolen_link, &to_i915(dev)->mm.stolen_list);
> > +
> > +	/* By default, treat the contexts of stolen as volatile. If the object
> > +	 * must be saved across hibernation, then the caller must take
> > +	 * action and flag it as WILLNEED.
> > +	 */
> > +	obj->madv = I915_MADV_DONTNEED;
> 
> Won't this interfere with autoreclaim of stolen objects (to make room for
> users which really need it like fbc) which are still in use by userspace?
> I think we need a new madv flag for "REAP_ON_HIBERNATE" or something
> similar. Otherwise this is way too surprising for userspace.

I didn't like this much either, and even in the hibernate loop, we
confuse userspace madv with internal objects. I went with a new

So that we could easily distinguish the internal objects from userspace.
-Chris

Comments

Daniel Vetter June 30, 2015, 12:03 p.m. UTC | #1
On Tue, Jun 30, 2015 at 12:20:19PM +0100, Chris Wilson wrote:
> On Tue, Jun 30, 2015 at 12:54:02PM +0200, Daniel Vetter wrote:
> > > +	list_add(&obj->stolen_link, &to_i915(dev)->mm.stolen_list);
> > > +
> > > +	/* By default, treat the contexts of stolen as volatile. If the object
> > > +	 * must be saved across hibernation, then the caller must take
> > > +	 * action and flag it as WILLNEED.
> > > +	 */
> > > +	obj->madv = I915_MADV_DONTNEED;
> > 
> > Won't this interfere with autoreclaim of stolen objects (to make room for
> > users which really need it like fbc) which are still in use by userspace?
> > I think we need a new madv flag for "REAP_ON_HIBERNATE" or something
> > similar. Otherwise this is way too surprising for userspace.
> 
> I didn't like this much either, and even in the hibernate loop, we
> confuse userspace madv with internal objects. I went with a new
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ec10f389886e..c8ea71713ab8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2064,6 +2064,12 @@ struct drm_i915_gem_object {
>          * Advice: are the backing pages purgeable?
>          */
>         unsigned int madv:2;
> +       /**
> +        * Whereas madv is for userspace, there are certain situations
> +        * where we want I915_MADV_DONTNEED behaviour on internal objects
> +        * without conflating the userspace setting.
> +        */
> +       unsigned int nonvolatile:1;
> 
> (I don't like the double negative, but it's better than int _volatile:1;)
> 
> So that we could easily distinguish the internal objects from userspace.

I'd go with an explicit volatile_stolen or so, which must be explicitly
set. Just to avoid surprises since everywhere else (and let's be honest,
not many use hibernate compared to suspend) we do preserve objects. Hence
I think a safe-by-default setting is called for here.

But yeah separate flag sounds cleaner.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ec10f389886e..c8ea71713ab8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2064,6 +2064,12 @@  struct drm_i915_gem_object {
         * Advice: are the backing pages purgeable?
         */
        unsigned int madv:2;
+       /**
+        * Whereas madv is for userspace, there are certain situations
+        * where we want I915_MADV_DONTNEED behaviour on internal objects
+        * without conflating the userspace setting.
+        */
+       unsigned int nonvolatile:1;

(I don't like the double negative, but it's better than int _volatile:1;)