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@xxxxxxxxx>
> Cc: Alex Dai <yu.dai@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
