On 08/12/15 17:03, Chris Wilson wrote:
On Tue, Dec 08, 2015 at 04:51:18PM +0000, Dave Gordon wrote:
This patch covers a couple more places where a GEM object is (or may be)
modified by means of CPU writes, and should therefore be marked dirty to
ensure that the changes are not lost in the evenof of the object is
evicted under memory pressure.
It may be possible to optimise these paths later, by marking only
specific pages of the object as dirty (for objects backed by shmfs
pages); but for now let's ensure correctness by dirtying the whole
object.
Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gem.c | 4 +++-
drivers/gpu/drm/i915/i915_gem_dmabuf.c | 2 ++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 12f68f4..36b9539 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;
Possibly. I'd rather just have shmem_pwrite be consistent and use
set_page_dirty. It is baked into the code that it doesn't access every
page.
It wasn't the shmem path that was the problem; this line was previously
inside i915_gem_shmem_pwrite() above. But i915_gem_phys_pwrite() was
missing the corresponding line, so it was simpler to move marking the
object dirty up to the top level of the ioctl for now, especially as
i915_gem_gtt_pwrite_fast() might or might not have marked the object in
the case where it returns early.
We could at some time in the future devolve object marking to a
class-specific vfunc, at which point this line would disappear again;
but we'd have to implement it in each class, or at least the ones that
users can call pwrite on (shmem, phys, and eventually stolen?). Of
those, shmem can do per-page dirtying, but phys can stolen can't (stolen
doesn't even have "struct page" entries available).
Which is why it's simpler to just mark the whole object here and let
put_pages() deal with it later (if ever -- if the object is never
actually swapped out then marking the object incurs LESS overhead than
marking all the pages).
trace_i915_gem_object_pwrite(obj, args->offset, args->size);
ret = -EFAULT;
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;
No. The accessor here should already be using set_page_dirty.
-Chris
What function would that be? I can't find any calls to set_page_dirty()
in this source file. OTOH, does a dmabuf object have shmfs backing store
anyway?
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
