Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel@ffwll.ch>,
	intel-gfx@lists.freedesktop.org,
	Imre Deak <imre.deak@linux.intel.com>,
	Akash Goel <akash.goel@intel.com>
Subject: Re: [PATCH] drm/i915: Migrate stolen objects before hibernation
Date: Tue, 30 Jun 2015 14:03:12 +0200	[thread overview]
Message-ID: <20150630120312.GX30960@phenom.ffwll.local> (raw)
In-Reply-To: <20150630112019.GJ1381@nuc-i3427.alporthouse.com>

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx