[Intel-gfx] [PATCH 2/2 v2] drm/i915: mark GEM objects dirty after overwriting their contents

[Chris Wilson]

On Wed, Dec 09, 2015 at 03:52:52PM +0000, Dave Gordon wrote:
> In a few places, we fill a GEM object with data, or overwrite some
> portion of its contents other than a single page. In such cases, we
> should mark the object dirty so that its pages in the pagecache are
> written to backing store (rather than discarded) if the object is
> evicted due to memory pressure.
> 
> The cases where only a single page is touched are dealt with in a
> separate patch.
> 
> This 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 at intel.com>
> Cc: Alex Dai <yu.dai at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 3 +++
>  drivers/gpu/drm/i915/i915_gem.c        | 5 ++++-
>  drivers/gpu/drm/i915/intel_lrc.c       | 2 +-
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 814d894..81a4fa2 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -946,6 +946,9 @@ static u32 *copy_batch(struct drm_i915_gem_object *dest_obj,
>  
>  	memcpy(dst, src, batch_len);
>  
> +	/* After writing on the dest_obj, its backing store is out-of-date */
> +	dest_obj->dirty = 1;

I still think this is superfluous as it doesn't fix any bug and would
rather not introduce new obj->dirty (I regret the limitations of our
coarse whole object granularity), especially just before deleting
copy_batch.

>  unmap_src:
>  	vunmap(src_base);
>  unpin_src:
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 06a5f39..81a770f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -937,7 +937,6 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
>  	i915_gem_object_pin_pages(obj);
>  
>  	offset = args->offset;
> -	obj->dirty = 1;
>  
>  	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents,
>  			 offset >> PAGE_SHIFT) {
> @@ -1074,6 +1073,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>  		goto out;
>  	}
>  
> +	/* Object backing store will be out of date hereafter */
> +	obj->dirty = 1;

I don't feel that reflects the asymmetry of pwrite.

>  	trace_i915_gem_object_pwrite(obj, args->offset, args->size);
>  
>  	ret = -EFAULT;
> @@ -5224,6 +5226,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;		/* Backing store is now out of date */

This is the bug, so should be by itself so that we don't lose it admist
the churn.

I still think that it a bug in the general library function to not mark
the buffer dirty upon copying. However, I can accept this as we do create
the object from the data, so sematically the object is dirty.

>  	i915_gem_object_unpin_pages(obj);
>  
>  	if (WARN_ON(bytes != size)) {
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index ceccecc..c7520b7 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1030,7 +1030,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;

That's just being obstinate! Going the other way and using the novel
bool:1 would be just as useful.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre