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:00:17 +0200	[thread overview]
Message-ID: <20150630120017.GW30960@phenom.ffwll.local> (raw)
In-Reply-To: <20150630111653.GI1381@nuc-i3427.alporthouse.com>

On Tue, Jun 30, 2015 at 12:16:53PM +0100, Chris Wilson wrote:
> On Tue, Jun 30, 2015 at 12:54:02PM +0200, Daniel Vetter wrote:
> > > +	if (obj->ops->release)
> > > +		obj->ops->release(obj);
> > > +
> > > +	obj->base.filp = file;
> > > +	obj->ops = &i915_gem_object_ops;
> > > +
> > > +	obj->base.read_domains = I915_GEM_DOMAIN_CPU;
> > > +	obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> > 
> > maybe wrap up in a shmem_reinit function and reuse in the gem object creation
> > path for a bit more clarity (and to reduce chances we forget about this).
> 
> A bit dubious about this. I like the explicit domain handling as it
> clearly matches the status of obj->file. The only thing that is then
> shared with the normal creation is obj->ops, which is actually set in
> i915_gem_object_init(). It's not clear that we actually have semantic
> reuse with gem object creation.

Yeah was just wondering really, maybe just as a marker. Doesn't sound like
it'd be useful at all.

> > Wrt igt not sure whether we can do this in general, since it requires a
> > working swap partition and working initrd support for hibernate. And
> > hibernate is kinda a disregarded feature anyway, so imo meh. We could do
> > an igt_system_hibernate_autowake() if we do the test though.
> 
> I was thinking of just a flush-stolen-to-shmemfs debugfs interface. With
> create2 we will be able to create the stolen objects and can use the
> existing debugfs files to verify placement.

Hm yeah manually exercising this code would be great for testing indeed.

> > Imo trying to migrate backwards would be serious pain since we'd need to
> > make sure no shm mmap is around and audit all the other places. Too much
> > trouble for no gain (userspace can just realloc if needed).
> 
> Otoh, that would be a userspace error since part of the stolen ABI is
> that CPU mmaps are verboten. The simplest way to do this would be keep
> the obj->base.filp private (so not to change the ABI of initially stolen
> objects) and then fix the existing i915_gem_object_get_pages_stolen()
> function (which is currently a BUG()) to do the stolen recreation.
> However, that introduces the possibilty of an error being returned to
> userspace, so at that point to hide the failure we probably do want to a
> full shmemfs conversion.
> 
> Hmm, honestly that seems quite tractable and self-contained.

Imo your minimal approach here to convert the bo into a shmem backed one
behind the scenes has a lot of appeal - we don't need to audit any ioctls
at all for userspace to be able to sneak in something nasty. And hence
also no need for negative igt testcases, only for one that exercises the
actual stolen-to-shmem function in a few corner cases (display pinning is
still the only one I can think of). Downside is that once converted it'll
stay converted, but I really don't see the downside of that.
-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