| Message ID | 1444828631-14533-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 14, 2015 at 02:17:11PM +0100, Chris Wilson wrote: > Since the beginning we have conflated the size of the global GTT with > that of the per-process context sizes. In recent times (gen8+), those > are no longer the same where the global GTT is limited to 2/4GiB but the > per-process GTT may be anything up to 256TiB. Userspace knows nothing of > this discrepancy and outside of one or two hacks, uses the getaperture > ioctl to determine the maximum size it can use. Let's leave that as > reporting the global GTT and use the context reporting method to > describe the per-process value (which naturally fallsback to reporting > the aliasing or global on older platforms, so userspace can always use > this method where available). > > Testcase: igt/gem_userptr_blits/minor-normal-sync > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90065 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 8 ++++++++ > include/uapi/drm/i915_drm.h | 5 +++-- > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 339a9d628f1e..cecb156c6f76 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -1087,6 +1087,14 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, > case I915_CONTEXT_PARAM_NO_ZEROMAP: > args->value = ctx->flags & CONTEXT_NO_ZEROMAP; > break; > + case I915_CONTEXT_PARAM_GTT_SIZE: > + if (ctx->ppgtt) > + args->value = ctx->ppgtt->base.total; > + else if (to_i915(dev)->mm.aliasing_ppgtt) > + args->value = to_i915(dev)->mm.aliasing_ppgtt->base.total; > + else > + args->value = to_i915(dev)->gtt.base.total; > + break; > default: > ret = -EINVAL; > break; > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 83f60f01dca2..3f334967aa1b 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -1130,8 +1130,9 @@ struct drm_i915_gem_context_param { > __u32 ctx_id; > __u32 size; > __u64 param; > -#define I915_CONTEXT_PARAM_BAN_PERIOD 0x1 > -#define I915_CONTEXT_PARAM_NO_ZEROMAP 0x2 > +#define I915_CONTEXT_PARAM_BAN_PERIOD 0x1 > +#define I915_CONTEXT_PARAM_NO_ZEROMAP 0x2 > +#define I915_CONTEXT_PARAM_GTT_SIZE 0x3 > __u64 value; > }; Ping any takers? As ABI goes it is pretty simple... -Chris
On 14/10/15 14:17, Chris Wilson wrote: > Since the beginning we have conflated the size of the global GTT with > that of the per-process context sizes. In recent times (gen8+), those > are no longer the same where the global GTT is limited to 2/4GiB but the > per-process GTT may be anything up to 256TiB. Userspace knows nothing of > this discrepancy and outside of one or two hacks, uses the getaperture > ioctl to determine the maximum size it can use. Let's leave that as > reporting the global GTT and use the context reporting method to > describe the per-process value (which naturally fallsback to reporting > the aliasing or global on older platforms, so userspace can always use > this method where available). > > Testcase: igt/gem_userptr_blits/minor-normal-sync > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90065 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 8 ++++++++ > include/uapi/drm/i915_drm.h | 5 +++-- > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 339a9d628f1e..cecb156c6f76 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -1087,6 +1087,14 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, > case I915_CONTEXT_PARAM_NO_ZEROMAP: > args->value = ctx->flags & CONTEXT_NO_ZEROMAP; > break; > + case I915_CONTEXT_PARAM_GTT_SIZE: > + if (ctx->ppgtt) > + args->value = ctx->ppgtt->base.total; > + else if (to_i915(dev)->mm.aliasing_ppgtt) > + args->value = to_i915(dev)->mm.aliasing_ppgtt->base.total; > + else > + args->value = to_i915(dev)->gtt.base.total; > + break; > default: > ret = -EINVAL; > break; > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 83f60f01dca2..3f334967aa1b 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -1130,8 +1130,9 @@ struct drm_i915_gem_context_param { > __u32 ctx_id; > __u32 size; > __u64 param; > -#define I915_CONTEXT_PARAM_BAN_PERIOD 0x1 > -#define I915_CONTEXT_PARAM_NO_ZEROMAP 0x2 > +#define I915_CONTEXT_PARAM_BAN_PERIOD 0x1 > +#define I915_CONTEXT_PARAM_NO_ZEROMAP 0x2 > +#define I915_CONTEXT_PARAM_GTT_SIZE 0x3 > __u64 value; > }; Implementation looks fine, Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> I have a slight unknown relating to how long would this ABI be useful. If things are moving towards SVM, and the fact pre-gen8 platforms can already use get_aperture, would that make it a bit short lived? And get_aperture would even be a better place for this if PPGTT size will always be the same for all clients. Only if we consider that one day we might want to regulate available address space available to clients (so implement also a setter for the context param), then per-ctx makes sense. (If again, SVM does not make this irrelevant.) This address space limiting kind of sounds interesting, together with the incoming GPU scheduling priorities, but then for the both I am not sure if an appropriate mechanism could be constructed to use it in a classical Unix sense, where you could set limits and inherit them from parent to child. To construct some sort of a launcher with lower priority / memory use. Regards, Tvrtko
On Fri, Oct 16, 2015 at 05:34:55PM +0100, Tvrtko Ursulin wrote: > I have a slight unknown relating to how long would this ABI be > useful. If things are moving towards SVM, and the fact pre-gen8 > platforms can already use get_aperture, would that make it a bit > short lived? Optimist. It is certainly a better fit than the cascade of get param and hardcoded sizes based on gen (which ignore differences between kernels) which a fallback to get_aperture. Now although I can't just drop the old code (because I need to support kernels without this interface), it does make that code simpler and more robust against any future change. > And get_aperture would even be a better place for this if PPGTT size > will always be the same for all clients. Hmm, I'm regretting my earlier push for extending get_aperture. Constants like this would be better off in GET_PARAM (or CONTEXT_PER_PARAM as applicable), and only using get_aperture where we want the side-effect of scanning for available space (e.g. the extra stolen information would be more useful in get_aperture as we there do want to know the largest object we can allocate given current usage, but we may also want to stick the total amount of stolen in a param for convenience). [snip] -Chris
On Fri, Oct 16, 2015 at 05:34:55PM +0100, Tvrtko Ursulin wrote: > > On 14/10/15 14:17, Chris Wilson wrote: > >Since the beginning we have conflated the size of the global GTT with > >that of the per-process context sizes. In recent times (gen8+), those > >are no longer the same where the global GTT is limited to 2/4GiB but the > >per-process GTT may be anything up to 256TiB. Userspace knows nothing of > >this discrepancy and outside of one or two hacks, uses the getaperture > >ioctl to determine the maximum size it can use. Let's leave that as > >reporting the global GTT and use the context reporting method to > >describe the per-process value (which naturally fallsback to reporting > >the aliasing or global on older platforms, so userspace can always use > >this method where available). > > > >Testcase: igt/gem_userptr_blits/minor-normal-sync Probably also needs updates to the getparam testcase. > >Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90065 > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >--- > > drivers/gpu/drm/i915/i915_gem_context.c | 8 ++++++++ > > include/uapi/drm/i915_drm.h | 5 +++-- > > 2 files changed, 11 insertions(+), 2 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > >index 339a9d628f1e..cecb156c6f76 100644 > >--- a/drivers/gpu/drm/i915/i915_gem_context.c > >+++ b/drivers/gpu/drm/i915/i915_gem_context.c > >@@ -1087,6 +1087,14 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, > > case I915_CONTEXT_PARAM_NO_ZEROMAP: > > args->value = ctx->flags & CONTEXT_NO_ZEROMAP; > > break; > >+ case I915_CONTEXT_PARAM_GTT_SIZE: > >+ if (ctx->ppgtt) > >+ args->value = ctx->ppgtt->base.total; > >+ else if (to_i915(dev)->mm.aliasing_ppgtt) > >+ args->value = to_i915(dev)->mm.aliasing_ppgtt->base.total; > >+ else > >+ args->value = to_i915(dev)->gtt.base.total; > >+ break; > > default: > > ret = -EINVAL; > > break; > >diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > >index 83f60f01dca2..3f334967aa1b 100644 > >--- a/include/uapi/drm/i915_drm.h > >+++ b/include/uapi/drm/i915_drm.h > >@@ -1130,8 +1130,9 @@ struct drm_i915_gem_context_param { > > __u32 ctx_id; > > __u32 size; > > __u64 param; > >-#define I915_CONTEXT_PARAM_BAN_PERIOD 0x1 > >-#define I915_CONTEXT_PARAM_NO_ZEROMAP 0x2 > >+#define I915_CONTEXT_PARAM_BAN_PERIOD 0x1 > >+#define I915_CONTEXT_PARAM_NO_ZEROMAP 0x2 > >+#define I915_CONTEXT_PARAM_GTT_SIZE 0x3 > > __u64 value; > > }; > > Implementation looks fine, > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Queued for -next, thanks for the patch. -Daniel > > I have a slight unknown relating to how long would this ABI be useful. If > things are moving towards SVM, and the fact pre-gen8 platforms can already > use get_aperture, would that make it a bit short lived? > > And get_aperture would even be a better place for this if PPGTT size will > always be the same for all clients. > > Only if we consider that one day we might want to regulate available address > space available to clients (so implement also a setter for the context > param), then per-ctx makes sense. (If again, SVM does not make this > irrelevant.) > > This address space limiting kind of sounds interesting, together with the > incoming GPU scheduling priorities, but then for the both I am not sure if > an appropriate mechanism could be constructed to use it in a classical Unix > sense, where you could set limits and inherit them from parent to child. To > construct some sort of a launcher with lower priority / memory use. > > Regards, > > Tvrtko > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 339a9d628f1e..cecb156c6f76 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -1087,6 +1087,14 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, case I915_CONTEXT_PARAM_NO_ZEROMAP: args->value = ctx->flags & CONTEXT_NO_ZEROMAP; break; + case I915_CONTEXT_PARAM_GTT_SIZE: + if (ctx->ppgtt) + args->value = ctx->ppgtt->base.total; + else if (to_i915(dev)->mm.aliasing_ppgtt) + args->value = to_i915(dev)->mm.aliasing_ppgtt->base.total; + else + args->value = to_i915(dev)->gtt.base.total; + break; default: ret = -EINVAL; break; diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 83f60f01dca2..3f334967aa1b 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1130,8 +1130,9 @@ struct drm_i915_gem_context_param { __u32 ctx_id; __u32 size; __u64 param; -#define I915_CONTEXT_PARAM_BAN_PERIOD 0x1 -#define I915_CONTEXT_PARAM_NO_ZEROMAP 0x2 +#define I915_CONTEXT_PARAM_BAN_PERIOD 0x1 +#define I915_CONTEXT_PARAM_NO_ZEROMAP 0x2 +#define I915_CONTEXT_PARAM_GTT_SIZE 0x3 __u64 value; };
Since the beginning we have conflated the size of the global GTT with that of the per-process context sizes. In recent times (gen8+), those are no longer the same where the global GTT is limited to 2/4GiB but the per-process GTT may be anything up to 256TiB. Userspace knows nothing of this discrepancy and outside of one or two hacks, uses the getaperture ioctl to determine the maximum size it can use. Let's leave that as reporting the global GTT and use the context reporting method to describe the per-process value (which naturally fallsback to reporting the aliasing or global on older platforms, so userspace can always use this method where available). Testcase: igt/gem_userptr_blits/minor-normal-sync Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90065 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem_context.c | 8 ++++++++ include/uapi/drm/i915_drm.h | 5 +++-- 2 files changed, 11 insertions(+), 2 deletions(-)