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 -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
