| Message ID | 1449676372-6988-3-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > Cc: Alex Dai <yu.dai@intel.com> > Cc: Chris Wilson <chris@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
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@intel.com> > Cc: Alex Dai <yu.dai@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> Hm, did you drop the begin_cpu_access dma-buf one here? See my other mail, I think that one was legit. -Daniel > --- > 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; > + > 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; > + > 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 */ > 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; > > /* Invalidate GuC TLB. */ > if (i915.enable_guc_submission) > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Dec 10, 2015 at 03:06:55PM +0100, Daniel Vetter wrote: > 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@intel.com> > > Cc: Alex Dai <yu.dai@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Hm, did you drop the begin_cpu_access dma-buf one here? See my other mail, > I think that one was legit. I thought begin-access was the sync point around the per-page mmap(), but reading the current code "in kernel users", of which it is just drm/udl. How would we interact with a future dma-buf mmap()? -Chris
On 10/12/15 14:06, Daniel Vetter wrote: > 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@intel.com> >> Cc: Alex Dai <yu.dai@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Hm, did you drop the begin_cpu_access dma-buf one here? See my other mail, > I think that one was legit. > -Daniel I did, because I didn't know whether it was necessary and Chris was dubious. But I can easily reinstate it in the next (v3) series. .Dave.
On Thu, Dec 10, 2015 at 02:52:57PM +0000, Chris Wilson wrote: > On Thu, Dec 10, 2015 at 03:06:55PM +0100, Daniel Vetter wrote: > > 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@intel.com> > > > Cc: Alex Dai <yu.dai@intel.com> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > Hm, did you drop the begin_cpu_access dma-buf one here? See my other mail, > > I think that one was legit. > > I thought begin-access was the sync point around the per-page mmap(), > but reading the current code "in kernel users", of which it is just > drm/udl. How would we interact with a future dma-buf mmap()? We might end up with a bool userspace. Or we could check obj->pages and only set dirty if that's set. A bit evil, but should work even for mmap. -Daniel
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.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; + 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 */ 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; /* Invalidate GuC TLB. */ if (i915.enable_guc_submission)
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@intel.com> Cc: Alex Dai <yu.dai@intel.com> Cc: Chris Wilson <chris@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(-)