diff mbox

drm/i915: Report context GTT size

Message ID 1444828631-14533-1-git-send-email-chris@chris-wilson.co.uk
State New, archived
Headers show

Commit Message

Chris Wilson Oct. 14, 2015, 1:17 p.m. UTC
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(-)

Comments

Chris Wilson Oct. 16, 2015, 1:22 p.m. UTC | #1
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
Tvrtko Ursulin Oct. 16, 2015, 4:34 p.m. UTC | #2
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
Chris Wilson Oct. 16, 2015, 5:31 p.m. UTC | #3
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
Daniel Vetter Oct. 19, 2015, 10:17 a.m. UTC | #4
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 mbox

Patch

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;
 };