On 06/03/2017 12:48, Chris Wilson wrote:
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.
It doesn't but I thought it is weird and confusing to have a vfunc with
a completely generic name but very specialized behaviour.
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
Oh it was a real regression and not just an optimisation for a strange
client behaviour? You should add a Fixes: tag then. And explain in the
commit what it was (the regression).
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.
So in this particular case is that becuase it is swapping out even the
untouched pages? And it started doing that recently after some commit or
what?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
