[Intel-gfx] [PATCH 06/17] drm/i915: Move obj->dirty:1 to obj->flags

[Chris Wilson]

On Tue, Sep 06, 2016 at 12:37:33PM +0100, Dave Gordon wrote:
> On 22/08/16 09:03, Chris Wilson wrote:
> >The obj->dirty bit is a companion to the obj->active bits that were
> >moved to the obj->flags bitmask. Since we also update this bit inside
> >the i915_vma_move_to_active() hotpath, we can aide gcc by also moving
> >the obj->dirty bit to obj->flags bitmask.
> >
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >---
> > drivers/gpu/drm/i915/i915_debugfs.c        |  2 +-
> > drivers/gpu/drm/i915/i915_drv.h            | 22 +++++++++++++++++++++-
> > drivers/gpu/drm/i915/i915_gem.c            | 20 ++++++++++----------
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c |  3 +--
> > drivers/gpu/drm/i915/i915_gem_userptr.c    |  6 +++---
> > drivers/gpu/drm/i915/i915_gpu_error.c      |  2 +-
> > drivers/gpu/drm/i915/intel_lrc.c           |  6 +++---
> > 7 files changed, 40 insertions(+), 21 deletions(-)
> >
> 
> [snip]
> 
> >@@ -3272,7 +3272,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
> > 	if (write) {
> > 		obj->base.read_domains = I915_GEM_DOMAIN_GTT;
> > 		obj->base.write_domain = I915_GEM_DOMAIN_GTT;
> >-		obj->dirty = 1;
> >+		i915_gem_object_set_dirty(obj);
> 
> At this point the object is or may not be pinned. From our
> conversation back in April ...
> 
> >>What I particularly dislike about the current obj->dirty is that it is
> >>strictly only valid inside a pin_pages/unpin_pages section. That isn't
> >>clear from the API atm.
> >>-Chris
> >
> >So, I tried replacing all instances of "obj->dirty = true" with my
> >newfunction i915_gem_object_mark_dirty(), and added an assertion that
> > it's called only when (pages_pin_count > 0) - and found a failure.
> >
> >Stack is:
> >    i915_gem_object_mark_dirty
> >    i915_gem_object_set_to_gtt_domain
> >    i915_gem_set_domain_ioctl
> >
> >So is i915_gem_object_set_to_gtt_domain() wrong? It's done a
> >get_pages but no pin_pages.
> 
> ... and the same issue is still there. I think I worked out a
> hypothetical scenario where this would lead to data corruption:
> 
> * set to GTT => mark dirty
> * BO paged out => flushed to swap, marked clean
> * BO paged in => still clean

Key step: how do we trigger the page in?

When we flush the pages out they are marked as being in the CPU domain.
In order to page them back in requires user action (set-domain, execbuf,
pwrite, gtt mmap pagefault) being the primary ones. Any of those will mark
them dirty again.

> * update contents => still clean?
> * get paged out => not written out?
> 
> But can this actually happen?

Hopefully not as the sequence should be more like:

* set to GTT => mark dirty
...
* BO paged out => flushed to swap, marked clean, marked as CPU
...
* set to write=GTT, BO paged in => mark dirty

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre