| Message ID | 1449593478-33649-4-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > 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. > 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
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@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> 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.
On Tue, Dec 08, 2015 at 06:24:40PM +0000, Dave Gordon wrote: > 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@intel.com> > >>Cc: Chris Wilson <chris@chris-wilson.co.uk> > >>--- > >> 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? I think there's indeed a bug here, and setting obj->dirty is the right defensive solution. For mmap access from userspace (once that happens) we'd be covered by the set_page_dirty shmem would do. But for kernel-internal users (dma-buf vmap, used e.g. by udl) there indeed seems to be no one setting obj->dirty. And that code definitely needs to be somewhere in i915. I guess we could make a case whether we should set obj->dirty in vmap instead - since we don't support the dma-buf kmap stuff there's no problem there. But indeed this should be set somewhere. -Daniel
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; + 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; mutex_unlock(&dev->struct_mutex); return ret; }
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@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem.c | 4 +++- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-)