Re: [PATCH] Always mark GEM objects as dirty when written by the CPU

[Dave Gordon <david.s.gordon@intel.com>]

On 01/12/15 12:42, Dave Gordon wrote:
> In various places, one or more pages of a GEM object are mapped into CPU
> address space and updated. In each such case, the object should be
> marked dirty, to ensure that the modifications are not discarded if the
> object is evicted under memory pressure.
>
> This is similar to commit
> 	commit 51bc140431e233284660b1d22c47dec9ecdb521e
> 	Author: Chris Wilson <chris@chris-wilson.co.uk>
> 	Date:   Mon Aug 31 15:10:39 2015 +0100
> 	drm/i915: Always mark the object as dirty when used by the GPU
>
> in which Chris ensured that updates by the GPU were not lost due to
> eviction, but this patch applies instead to the multiple places where
> object content is updated by the host CPU.
>
> It also incorporates and supercedes Alex Dai's earlier patch
> [PATCH v1] drm/i915/guc: Fix a fw content lost issue after it is evicted
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Alex Dai <yu.dai@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_cmd_parser.c       | 1 +
>   drivers/gpu/drm/i915/i915_gem.c              | 1 +
>   drivers/gpu/drm/i915/i915_gem_dmabuf.c       | 2 ++
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c   | 2 ++
>   drivers/gpu/drm/i915/i915_gem_render_state.c | 1 +
>   drivers/gpu/drm/i915/i915_guc_submission.c   | 1 +
>   drivers/gpu/drm/i915/intel_lrc.c             | 6 +++++-
>   7 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 814d894..292bd5d 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -945,6 +945,7 @@ static u32 *copy_batch(struct drm_i915_gem_object *dest_obj,
>   		drm_clflush_virt_range(src, batch_len);
>
>   	memcpy(dst, src, batch_len);
> +	dest_obj->dirty = 1;
>
>   unmap_src:
>   	vunmap(src_base);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 33adc8f..76bacba 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5201,6 +5201,7 @@ i915_gem_object_create_from_data(struct drm_device *dev,
>   	i915_gem_object_pin_pages(obj);
>   	sg = obj->pages;
>   	bytes = sg_copy_from_buffer(sg->sgl, sg->nents, (void *)data, size);
> +	obj->dirty = 1;
>   	i915_gem_object_unpin_pages(obj);
>
>   	if (WARN_ON(bytes != size)) {
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index e9c2bfd..49a74c6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -208,6 +208,8 @@ static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, size_t start, size
>   		return ret;
>
>   	ret = i915_gem_object_set_to_cpu_domain(obj, write);
> +	if (write)
> +		obj->dirty = 1;
>   	mutex_unlock(&dev->struct_mutex);
>   	return ret;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index a4c243c..bc28a10 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -281,6 +281,7 @@ relocate_entry_cpu(struct drm_i915_gem_object *obj,
>   	}
>
>   	kunmap_atomic(vaddr);
> +	obj->dirty = 1;
>
>   	return 0;
>   }
> @@ -372,6 +373,7 @@ relocate_entry_clflush(struct drm_i915_gem_object *obj,
>   	}
>
>   	kunmap_atomic(vaddr);
> +	obj->dirty = 1;
>
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> index 5026a62..dd1976c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -144,6 +144,7 @@ static int render_state_setup(struct render_state *so)
>   	so->aux_batch_size = ALIGN(so->aux_batch_size, 8);
>
>   	kunmap(page);
> +	so->obj->dirty = 1;
>
>   	ret = i915_gem_object_set_to_gtt_domain(so->obj, false);
>   	if (ret)
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index a057cbd..b4a99a2 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -583,6 +583,7 @@ static void lr_context_update(struct drm_i915_gem_request *rq)
>   	reg_state[CTX_RING_BUFFER_START+1] = i915_gem_obj_ggtt_offset(rb_obj);
>
>   	kunmap_atomic(reg_state);
> +	ctx_obj->dirty = 1;
>   }
>
>   /**
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 4ebafab..bc77794 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -391,6 +391,7 @@ static int execlists_update_context(struct drm_i915_gem_request *rq)
>   	}
>
>   	kunmap_atomic(reg_state);
> +	ctx_obj->dirty = 1;
>
>   	return 0;
>   }
> @@ -1030,7 +1031,7 @@ static int intel_lr_context_do_pin(struct intel_engine_cs *ring,
>   	if (ret)
>   		goto unpin_ctx_obj;
>
> -	ctx_obj->dirty = true;
> +	ctx_obj->dirty = 1;
>
>   	/* Invalidate GuC TLB. */
>   	if (i915.enable_guc_submission)
> @@ -1461,6 +1462,8 @@ static int intel_init_workaround_bb(struct intel_engine_cs *ring)
>
>   out:
>   	kunmap_atomic(batch);
> +	wa_ctx->obj->dirty = 1;
> +
>   	if (ret)
>   		lrc_destroy_wa_ctx_obj(ring);
>
> @@ -2536,6 +2539,7 @@ void intel_lr_context_reset(struct drm_device *dev,
>   		reg_state[CTX_RING_TAIL+1] = 0;
>
>   		kunmap_atomic(reg_state);
> +		ctx_obj->dirty = 1;
>
>   		ringbuf->head = 0;
>   		ringbuf->tail = 0;
>

I think I missed i915_gem_phys_pwrite().

i915_gem_gtt_pwrite_fast() marks the object dirty for most cases (vit 
set_to_gtt_domain(), but isn't called for all cases (or can return 
before the set_domain). Then we try i915_gem_shmem_pwrite() for non-phys
objects (no check for stolen!) and that already marks the object dirty 
[aside: we might be able to change that to page-by-page?], but 
i915_gem_phys_pwrite() doesn't mark the object dirty, so we might lose 
updates there?

Or maybe we should move the marking up into i915_gem_pwrite_ioctl() 
instead. The target object is surely going to be dirtied, whatever type 
it is.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx