| Message ID | 1449593478-33649-2-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 08, 2015 at 04:51:16PM +0000, Dave Gordon wrote: > In various places, a GEM object is filled with data by means of CPU > writes. In such cases, the object should be marked dirty, to ensure that > the data is not discarded if the object is evicted under memory > pressure. > > 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: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Alex Dai <yu.dai@intel.com> > --- > drivers/gpu/drm/i915/i915_cmd_parser.c | 1 + > drivers/gpu/drm/i915/i915_gem.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c > index 814d894..292bd5d 100644 > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > @@ -945,6 +945,7 @@ static u32 *copy_batch(struct drm_i915_gem_object *dest_obj, > drm_clflush_virt_range(src, batch_len); > > memcpy(dst, src, batch_len); > + dest_obj->dirty = 1; There is no bug here. -Chris
On 08/12/15 17:00, Chris Wilson wrote: > On Tue, Dec 08, 2015 at 04:51:16PM +0000, Dave Gordon wrote: >> In various places, a GEM object is filled with data by means of CPU >> writes. In such cases, the object should be marked dirty, to ensure that >> the data is not discarded if the object is evicted under memory >> pressure. >> >> 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: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Alex Dai <yu.dai@intel.com> >> --- >> drivers/gpu/drm/i915/i915_cmd_parser.c | 1 + >> drivers/gpu/drm/i915/i915_gem.c | 1 + >> 2 files changed, 2 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c >> index 814d894..292bd5d 100644 >> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c >> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c >> @@ -945,6 +945,7 @@ static u32 *copy_batch(struct drm_i915_gem_object *dest_obj, >> drm_clflush_virt_range(src, batch_len); >> >> memcpy(dst, src, batch_len); >> + dest_obj->dirty = 1; > > There is no bug here. > -Chris Because the shadow batch has been page-pinned by the caller? Two questions, then: 1. Do we reuse the same shadow batch if the same source batch is resubmitted? If so, can we be sure that it has stayed pinned (one way or another) for the entire intervening period? 2. If the shadow batch can never be unpinned, why do we allocate it as a GEM object with backing store which can apparently never be used. We could get rid of the shmfs setup overhead by choosing an object type without backing store. .Dave.
On Tue, Dec 08, 2015 at 06:06:42PM +0000, Dave Gordon wrote: > On 08/12/15 17:00, Chris Wilson wrote: > >On Tue, Dec 08, 2015 at 04:51:16PM +0000, Dave Gordon wrote: > >>In various places, a GEM object is filled with data by means of CPU > >>writes. In such cases, the object should be marked dirty, to ensure that > >>the data is not discarded if the object is evicted under memory > >>pressure. > >> > >>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: Chris Wilson <chris@chris-wilson.co.uk> > >>Cc: Alex Dai <yu.dai@intel.com> > >>--- > >> drivers/gpu/drm/i915/i915_cmd_parser.c | 1 + > >> drivers/gpu/drm/i915/i915_gem.c | 1 + > >> 2 files changed, 2 insertions(+) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c > >>index 814d894..292bd5d 100644 > >>--- a/drivers/gpu/drm/i915/i915_cmd_parser.c > >>+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > >>@@ -945,6 +945,7 @@ static u32 *copy_batch(struct drm_i915_gem_object *dest_obj, > >> drm_clflush_virt_range(src, batch_len); > >> > >> memcpy(dst, src, batch_len); > >>+ dest_obj->dirty = 1; > > > >There is no bug here. > >-Chris > > Because the shadow batch has been page-pinned by the caller? Two questions, > then: > > 1. Do we reuse the same shadow batch if the same source batch is > resubmitted? If so, can we be sure that it has stayed pinned > (one way or another) for the entire intervening period? > > 2. If the shadow batch can never be unpinned, why do we allocate > it as a GEM object with backing store which can apparently > never be used. We could get rid of the shmfs setup overhead by > choosing an object type without backing store. We don't care about coherency once the shadow batch is retired (we mark it as purgeable even, which flat-out ignores ->dirty). We use shmem because we need some memory for it, there's no copying to the backing storage (like ttm does when evicting from vram to shmem). -Daniel
diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index 814d894..292bd5d 100644 --- a/drivers/gpu/drm/i915/i915_cmd_parser.c +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c @@ -945,6 +945,7 @@ static u32 *copy_batch(struct drm_i915_gem_object *dest_obj, drm_clflush_virt_range(src, batch_len); memcpy(dst, src, batch_len); + dest_obj->dirty = 1; unmap_src: vunmap(src_base); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index dfaf25b..12f68f4 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -5209,6 +5209,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; i915_gem_object_unpin_pages(obj); if (WARN_ON(bytes != size)) {
In various places, a GEM object is filled with data by means of CPU writes. In such cases, the object should be marked dirty, to ensure that the data is not discarded if the object is evicted under memory pressure. 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: Chris Wilson <chris@chris-wilson.co.uk> Cc: Alex Dai <yu.dai@intel.com> --- drivers/gpu/drm/i915/i915_cmd_parser.c | 1 + drivers/gpu/drm/i915/i915_gem.c | 1 + 2 files changed, 2 insertions(+)