[Intel-gfx] [PATCH 1/2 v2] drm/i915: mark GEM object pages dirty when mapped & written by the CPU

[Daniel Vetter]

On Thu, Dec 10, 2015 at 09:04:23PM +0000, Chris Wilson wrote:
> On Thu, Dec 10, 2015 at 05:24:42PM +0000, Dave Gordon wrote:
> > On 10/12/15 13:29, Chris Wilson wrote:
> > >On Wed, Dec 09, 2015 at 03:52:51PM +0000, Dave Gordon wrote:
> > >>In various places, a single page of a (regular) GEM object is mapped into
> > >>CPU address space and updated. In each such case, either the page or the
> > >>the object should be marked dirty, to ensure that the modifications are
> > >>not discarded if the object is evicted under memory pressure.
> > >>
> > >>The typical sequence is:
> > >>	va = kmap_atomic(i915_gem_object_get_page(obj, pageno));
> > >>	*(va+offset) = ...
> > >>	kunmap_atomic(va);
> > >>
> > >>Here we introduce i915_gem_object_get_dirty_page(), which performs the
> > >>same operation as i915_gem_object_get_page() but with the side-effect
> > >>of marking the returned page dirty in the pagecache.  This will ensure
> > >>that if the object is subsequently evicted (due to memory pressure),
> > >>the changes are written to backing store rather than discarded.
> > >>
> > >>Note that it works only for regular (shmfs-backed) GEM objects, but (at
> > >>least for now) those are the only ones that are updated in this way --
> > >>the objects in question are contexts and batchbuffers, which are always
> > >>shmfs-backed.
> > >>
> > >>A separate patch deals with the case where whole objects are (or may
> > >>be) dirtied.
> > >>
> > >>Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> > >>Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > >
> > >I like this. There are places were we do both obj->dirty and
> > >set_page_dirty(), but this so much more clearly shows what is going on.
> > >All of these locations should be infrequent (or at least have patches to
> > >make them so), so moving the call out-of-line will also be a benefit.
> > 
> > I think there was only one place that both called set_page_dirty()
> > AND set obj->dirty, which was in populate_lr_context(). You'll see
> > that I've eliminated both in favour of a call to get_dirty_page() :)
> 
> It was things like all GPU objects have obj->dirty set, so really the
> importance of using get_dirty_page() in the relocations and context
> pinning is for documentation. Which is a very good reason, nevertheless.

Hm, I think if you force a fault on relocs and then shrink everything
really hard before actually managing to submit the batch you could provoke
this into a proper bug. one-in-a-billion perhaps ;-)
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch