On Mon, Mar 06, 2017 at 12:24:46PM +0000, Tvrtko Ursulin wrote: > > On 06/03/2017 09:29, Chris Wilson wrote: > >Before we instantiate/pin the backing store for our use, we > >can prepopulate the shmemfs filp efficiently using the a > >write into the pagecache. We avoid the penalty of instantiating > >all the pages, important if the user is just writing to a few > >and never uses the object on the GPU, and using a direct write > >into shmemfs allows it to avoid the cost of retrieving a page > >(either swapin or clearing-before-use) before it is overwritten. > > > >This can be extended later to provide additional specialisation for > >other backends (other than shmemfs). > > > >References: https://bugs.freedesktop.org/show_bug.cgi?id=99107 > >Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > >Cc: Matthew Auld <matthew.william.auld@xxxxxxxxx> > >Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> > >Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx> > >--- > > drivers/gpu/drm/i915/i915_gem.c | 78 ++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/i915_gem_object.h | 3 ++ > > 2 files changed, 81 insertions(+) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >index 7c20601fe1de..73d5cee23dd7 100644 > >--- a/drivers/gpu/drm/i915/i915_gem.c > >+++ b/drivers/gpu/drm/i915/i915_gem.c > >@@ -1452,6 +1452,12 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, > > > > trace_i915_gem_object_pwrite(obj, args->offset, args->size); > > > >+ ret = -ENODEV; > >+ if (obj->ops->pwrite) > >+ ret = obj->ops->pwrite(obj, args); > > ops->pwrite_nowait or some good name to signify the conditions? > > I am not sure that the obj->mm.pages check can be in the vfunc since > this call happens before the i915_gem_object_wait. Which to me > sounds like we should be explicit at this level that the ops->pwrite > is not allowed to run if obj->mm.pages has already been > instantiated. That doesn't make sense to me. There's no reason that the backend can't handle the wait, or understand special cases for when there are pages. > Or if this is only true for the normal objects, and the future > specialisation may become full, then the standard pattern of > > if (obj->ops->pwrite) > ret = obj->ops->pwrite(); > else > ret = default_obj_pwrite(); > > ? Yeah, is kind of where I think we will end up. However, pwrite is of limited usefulness that I think we should do this case by case. Getting rid the the shmem/phys special cases would be nice. The sketch I have is that each backend has something like: err = fast_pwrite(); if (err != -ENODEV) return err; err = default_gtt_pwrite(); if (err != -ENODEV) return err; return slow_pwrite(); The challenge is trying to limit the duplication of the common steps to avoid struct_mutex. (Ideally by eliminating struct_mutex entirely from this path.) > In which case ops->pwrite would be the correct name to keep. > > The ops->pwrite for the shmemfs objects would then become > i915_gem_object_pwrite_gtt from below followed by > default_obj_pwrite. > > I don't know, maybe I am complicating it too much? For the moment, yes :) This was a quick hack to hide a regression - the real bug fix will be breaking struct_mutex out of the shrinker, I think, and there's some nasty behaviour to resolve when we do hit the shrinker the whole object page-in/page-out behaviour is much, much slower than what should be the equivalent with individual pages via shmemfs. The bonus here is that shmemfs can avoid clearing/swapping-in the page if knows it will be completely overwritten, which makes the patch useful on its own merit. > >+ if (ret != -ENODEV) > >+ goto err; > >+ > > ret = i915_gem_object_wait(obj, > > I915_WAIT_INTERRUPTIBLE | > > I915_WAIT_ALL, > >@@ -2575,6 +2581,75 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, > > goto out_unlock; > > } > > > >+static int > >+i915_gem_object_pwrite_gtt(struct drm_i915_gem_object *obj, > > _shmem ? Or _nowait_shmem depending on the outcome of the above. > Just think _gtt is wrong. Or keep it for consistency since get/put > pages for shmem objects are also called _gtt at the moment, so until > that is changed? Right, until we actually move this to i915_gem_object_shmemfs.c (or just i915_gem_shmemfs) I choose _gtt to match the existing nomenclature. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
