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

Dave Gordon
2016-09-06T11:37:35Z

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@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
* update contents => still clean?
* get paged out => not written out?

But can this actually happen?

.Dave.