| Message ID | 1449773486-30822-5-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 10, 2015 at 06:51:26PM +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 event that the object is > evicted under memory pressure. > > One is in i915_gem_begin_cpu_access(); after this call, the GEM object may > be written to by the caller (which may not be part of the i915 driver e.g. > udl). We must therefore assume that the object is dirty hereafter if > the caller has asked for write access. > > Another is in copy_batch(); the destination object is obviously dirty > after being written, but failing to mark it doesn't cause a bug at > present, because the object is page-pinned at this point, and should > remain either page- pinned or GTT-pinned until it's retired, at which > point its content can be discarded. However, if the lifecycle of shadow > batches is ever changed (e.g. by the introduction of a GPU scheduler) > this might no longer be true, so it's safer to mark it correctly (this > introduces no overhead if the buffer is never swappable). It also makes > the content cycle clearer: > > ---allocate--> > [empty buffer acquired from pool] > ---fill--> > [valid buffer full of unsaved data] > ---use--> > [buffer full of unsaved but unwanted data] > --retire--> > [purgeable buffer returned to pool] > ... repeat ... > > The last change here is just for consistency; since 'dirty' has been > declared as an (unsigned int) bitfield, let's not treat it as a bool. > Maybe it should be a byte instead? No, it's just because obj->dirty is older than C's bool type. Changing it to be bool obj->dirty:1 would be fine - except that there is one particular very hot path where moving it to an unsigned obj->flags field would be even better. > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > index e9c2bfd..5eb7887 100644 > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > @@ -208,6 +208,9 @@ 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; > + So looking at the only existing example (drm/udl which only reads from te object anyway) this would fall into bug category. Hence separate patch. But I'll defer to Daniel as to whether the dma-buf is meant to operate at the object level or at the page level with regards to dirty tracking (certainly we would struggle at the moment with dma-buf on massive objects). -Chris
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; + unmap_src: vunmap(src_base); unpin_src: diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index e9c2bfd..5eb7887 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -208,6 +208,9 @@ 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/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; /* Invalidate GuC TLB. */ if (i915.enable_guc_submission)
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 event that the object is evicted under memory pressure. One is in i915_gem_begin_cpu_access(); after this call, the GEM object may be written to by the caller (which may not be part of the i915 driver e.g. udl). We must therefore assume that the object is dirty hereafter if the caller has asked for write access. Another is in copy_batch(); the destination object is obviously dirty after being written, but failing to mark it doesn't cause a bug at present, because the object is page-pinned at this point, and should remain either page- pinned or GTT-pinned until it's retired, at which point its content can be discarded. However, if the lifecycle of shadow batches is ever changed (e.g. by the introduction of a GPU scheduler) this might no longer be true, so it's safer to mark it correctly (this introduces no overhead if the buffer is never swappable). It also makes the content cycle clearer: ---allocate--> [empty buffer acquired from pool] ---fill--> [valid buffer full of unsaved data] ---use--> [buffer full of unsaved but unwanted data] --retire--> [purgeable buffer returned to pool] ... repeat ... The last change here is just for consistency; since 'dirty' has been declared as an (unsigned int) bitfield, let's not treat it as a bool. Maybe it should be a byte instead? Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_cmd_parser.c | 3 +++ drivers/gpu/drm/i915/i915_gem_dmabuf.c | 3 +++ drivers/gpu/drm/i915/intel_lrc.c | 2 +- 3 files changed, 7 insertions(+), 1 deletion(-)