* CI testing of persistence and sysfs
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
Hopefully this works. CI all over to you,
Test-with: 20191024105449.31948-1-chris@chris-wilson.co.uk
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* [Intel-gfx] CI testing of persistence and sysfs
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
Hopefully this works. CI all over to you,
Test-with: 20191024105449.31948-1-chris@chris-wilson.co.uk
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 01/11] drm/i915/gem: Make context persistence optional
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
Our existing behaviour is to allow contexts and their GPU requests to
persist past the point of closure until the requests are complete. This
allows clients to operate in a 'fire-and-forget' manner where they can
setup a rendering pipeline and hand it over to the display server and
immediately exiting. As the rendering pipeline is kept alive until
completion, the display server (or other consumer) can use the results
in the future and present them to the user.
However, not all clients want this persistent behaviour and would prefer
that the contexts are cleaned up immediately upon closure. This ensures
that when clients are run without hangchecking, any GPU hang is
terminated with the process and does not continue to hog resources.
By defining a context property to allow clients to control persistence
explicitly, we can remove the blanket advice to disable hangchecking
that seems to be far too prevalent.
The default behaviour for new controls is the legacy persistence mode.
New clients will have to opt out for immediate cleanup on context
closure. If the hangchecking modparam is disabled, so is persistent
context support -- all contexts will be terminated on closure.
Testcase: igt/gem_ctx_persistence
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 50 ++++++++++++++++++-
drivers/gpu/drm/i915/gem/i915_gem_context.h | 15 ++++++
.../gpu/drm/i915/gem/i915_gem_context_types.h | 1 +
.../gpu/drm/i915/gem/selftests/mock_context.c | 2 +
include/uapi/drm/i915_drm.h | 15 ++++++
5 files changed, 82 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 55f1f93c0925..0f1bbf5d1a11 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -437,12 +437,39 @@ static void context_close(struct i915_gem_context *ctx)
* case we opt to forcibly kill off all remaining requests on
* context close.
*/
- if (!i915_modparams.enable_hangcheck)
+ if (!i915_gem_context_is_persistent(ctx) ||
+ !i915_modparams.enable_hangcheck)
kill_context(ctx);
i915_gem_context_put(ctx);
}
+static int __context_set_persistence(struct i915_gem_context *ctx, bool state)
+{
+ if (i915_gem_context_is_persistent(ctx) == state)
+ return 0;
+
+ if (state) {
+ /*
+ * Only contexts that are short-lived [that will expire or be
+ * reset] are allowed to survive past termination. We require
+ * hangcheck to ensure that the persistent requests are healthy.
+ */
+ if (!i915_modparams.enable_hangcheck)
+ return -EINVAL;
+
+ i915_gem_context_set_persistence(ctx);
+ } else {
+ /* To cancel a context we use "preempt-to-idle" */
+ if (!(ctx->i915->caps.scheduler & I915_SCHEDULER_CAP_PREEMPTION))
+ return -ENODEV;
+
+ i915_gem_context_clear_persistence(ctx);
+ }
+
+ return 0;
+}
+
static struct i915_gem_context *
__create_context(struct drm_i915_private *i915)
{
@@ -477,6 +504,7 @@ __create_context(struct drm_i915_private *i915)
i915_gem_context_set_bannable(ctx);
i915_gem_context_set_recoverable(ctx);
+ __context_set_persistence(ctx, true /* cgroup hook? */);
for (i = 0; i < ARRAY_SIZE(ctx->hang_timestamp); i++)
ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES;
@@ -633,6 +661,7 @@ i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
return ctx;
i915_gem_context_clear_bannable(ctx);
+ i915_gem_context_set_persistence(ctx);
ctx->sched.priority = I915_USER_PRIORITY(prio);
GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
@@ -1743,6 +1772,16 @@ get_engines(struct i915_gem_context *ctx,
return err;
}
+static int
+set_persistence(struct i915_gem_context *ctx,
+ const struct drm_i915_gem_context_param *args)
+{
+ if (args->size)
+ return -EINVAL;
+
+ return __context_set_persistence(ctx, args->value);
+}
+
static int ctx_setparam(struct drm_i915_file_private *fpriv,
struct i915_gem_context *ctx,
struct drm_i915_gem_context_param *args)
@@ -1820,6 +1859,10 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv,
ret = set_engines(ctx, args);
break;
+ case I915_CONTEXT_PARAM_PERSISTENCE:
+ ret = set_persistence(ctx, args);
+ break;
+
case I915_CONTEXT_PARAM_BAN_PERIOD:
default:
ret = -EINVAL;
@@ -2272,6 +2315,11 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
ret = get_engines(ctx, args);
break;
+ case I915_CONTEXT_PARAM_PERSISTENCE:
+ args->size = 0;
+ args->value = i915_gem_context_is_persistent(ctx);
+ break;
+
case I915_CONTEXT_PARAM_BAN_PERIOD:
default:
ret = -EINVAL;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h b/drivers/gpu/drm/i915/gem/i915_gem_context.h
index cfe80590f0ed..18e50a769a6e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
@@ -76,6 +76,21 @@ static inline void i915_gem_context_clear_recoverable(struct i915_gem_context *c
clear_bit(UCONTEXT_RECOVERABLE, &ctx->user_flags);
}
+static inline bool i915_gem_context_is_persistent(const struct i915_gem_context *ctx)
+{
+ return test_bit(UCONTEXT_PERSISTENCE, &ctx->user_flags);
+}
+
+static inline void i915_gem_context_set_persistence(struct i915_gem_context *ctx)
+{
+ set_bit(UCONTEXT_PERSISTENCE, &ctx->user_flags);
+}
+
+static inline void i915_gem_context_clear_persistence(struct i915_gem_context *ctx)
+{
+ clear_bit(UCONTEXT_PERSISTENCE, &ctx->user_flags);
+}
+
static inline bool i915_gem_context_is_banned(const struct i915_gem_context *ctx)
{
return test_bit(CONTEXT_BANNED, &ctx->flags);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
index fe97b8ba4fda..861d7d92fe9f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
@@ -137,6 +137,7 @@ struct i915_gem_context {
#define UCONTEXT_NO_ERROR_CAPTURE 1
#define UCONTEXT_BANNABLE 2
#define UCONTEXT_RECOVERABLE 3
+#define UCONTEXT_PERSISTENCE 4
/**
* @flags: small set of booleans
diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_context.c b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
index 74ddd682c9cd..29b8984f0e47 100644
--- a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
@@ -22,6 +22,8 @@ mock_context(struct drm_i915_private *i915,
INIT_LIST_HEAD(&ctx->link);
ctx->i915 = i915;
+ i915_gem_context_set_persistence(ctx);
+
mutex_init(&ctx->engines_mutex);
e = default_engines(ctx);
if (IS_ERR(e))
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 63d40cba97e0..b5fd5e4bd16e 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1572,6 +1572,21 @@ struct drm_i915_gem_context_param {
* i915_context_engines_bond (I915_CONTEXT_ENGINES_EXT_BOND)
*/
#define I915_CONTEXT_PARAM_ENGINES 0xa
+
+/*
+ * I915_CONTEXT_PARAM_PERSISTENCE:
+ *
+ * Allow the context and active rendering to survive the process until
+ * completion. Persistence allows fire-and-forget clients to queue up a
+ * bunch of work, hand the output over to a display server and the quit.
+ * If the context is not marked as persistent, upon closing (either via
+ * an explicit DRM_I915_GEM_CONTEXT_DESTROY or implicitly from file closure
+ * or process termination), the context and any outstanding requests will be
+ * cancelled (and exported fences for cancelled requests marked as -EIO).
+ *
+ * By default, new contexts allow persistence.
+ */
+#define I915_CONTEXT_PARAM_PERSISTENCE 0xb
/* Must be kept compact -- no holes and well documented */
__u64 value;
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Intel-gfx] [PATCH 01/11] drm/i915/gem: Make context persistence optional
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
Our existing behaviour is to allow contexts and their GPU requests to
persist past the point of closure until the requests are complete. This
allows clients to operate in a 'fire-and-forget' manner where they can
setup a rendering pipeline and hand it over to the display server and
immediately exiting. As the rendering pipeline is kept alive until
completion, the display server (or other consumer) can use the results
in the future and present them to the user.
However, not all clients want this persistent behaviour and would prefer
that the contexts are cleaned up immediately upon closure. This ensures
that when clients are run without hangchecking, any GPU hang is
terminated with the process and does not continue to hog resources.
By defining a context property to allow clients to control persistence
explicitly, we can remove the blanket advice to disable hangchecking
that seems to be far too prevalent.
The default behaviour for new controls is the legacy persistence mode.
New clients will have to opt out for immediate cleanup on context
closure. If the hangchecking modparam is disabled, so is persistent
context support -- all contexts will be terminated on closure.
Testcase: igt/gem_ctx_persistence
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 50 ++++++++++++++++++-
drivers/gpu/drm/i915/gem/i915_gem_context.h | 15 ++++++
.../gpu/drm/i915/gem/i915_gem_context_types.h | 1 +
.../gpu/drm/i915/gem/selftests/mock_context.c | 2 +
include/uapi/drm/i915_drm.h | 15 ++++++
5 files changed, 82 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 55f1f93c0925..0f1bbf5d1a11 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -437,12 +437,39 @@ static void context_close(struct i915_gem_context *ctx)
* case we opt to forcibly kill off all remaining requests on
* context close.
*/
- if (!i915_modparams.enable_hangcheck)
+ if (!i915_gem_context_is_persistent(ctx) ||
+ !i915_modparams.enable_hangcheck)
kill_context(ctx);
i915_gem_context_put(ctx);
}
+static int __context_set_persistence(struct i915_gem_context *ctx, bool state)
+{
+ if (i915_gem_context_is_persistent(ctx) == state)
+ return 0;
+
+ if (state) {
+ /*
+ * Only contexts that are short-lived [that will expire or be
+ * reset] are allowed to survive past termination. We require
+ * hangcheck to ensure that the persistent requests are healthy.
+ */
+ if (!i915_modparams.enable_hangcheck)
+ return -EINVAL;
+
+ i915_gem_context_set_persistence(ctx);
+ } else {
+ /* To cancel a context we use "preempt-to-idle" */
+ if (!(ctx->i915->caps.scheduler & I915_SCHEDULER_CAP_PREEMPTION))
+ return -ENODEV;
+
+ i915_gem_context_clear_persistence(ctx);
+ }
+
+ return 0;
+}
+
static struct i915_gem_context *
__create_context(struct drm_i915_private *i915)
{
@@ -477,6 +504,7 @@ __create_context(struct drm_i915_private *i915)
i915_gem_context_set_bannable(ctx);
i915_gem_context_set_recoverable(ctx);
+ __context_set_persistence(ctx, true /* cgroup hook? */);
for (i = 0; i < ARRAY_SIZE(ctx->hang_timestamp); i++)
ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES;
@@ -633,6 +661,7 @@ i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
return ctx;
i915_gem_context_clear_bannable(ctx);
+ i915_gem_context_set_persistence(ctx);
ctx->sched.priority = I915_USER_PRIORITY(prio);
GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
@@ -1743,6 +1772,16 @@ get_engines(struct i915_gem_context *ctx,
return err;
}
+static int
+set_persistence(struct i915_gem_context *ctx,
+ const struct drm_i915_gem_context_param *args)
+{
+ if (args->size)
+ return -EINVAL;
+
+ return __context_set_persistence(ctx, args->value);
+}
+
static int ctx_setparam(struct drm_i915_file_private *fpriv,
struct i915_gem_context *ctx,
struct drm_i915_gem_context_param *args)
@@ -1820,6 +1859,10 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv,
ret = set_engines(ctx, args);
break;
+ case I915_CONTEXT_PARAM_PERSISTENCE:
+ ret = set_persistence(ctx, args);
+ break;
+
case I915_CONTEXT_PARAM_BAN_PERIOD:
default:
ret = -EINVAL;
@@ -2272,6 +2315,11 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
ret = get_engines(ctx, args);
break;
+ case I915_CONTEXT_PARAM_PERSISTENCE:
+ args->size = 0;
+ args->value = i915_gem_context_is_persistent(ctx);
+ break;
+
case I915_CONTEXT_PARAM_BAN_PERIOD:
default:
ret = -EINVAL;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h b/drivers/gpu/drm/i915/gem/i915_gem_context.h
index cfe80590f0ed..18e50a769a6e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
@@ -76,6 +76,21 @@ static inline void i915_gem_context_clear_recoverable(struct i915_gem_context *c
clear_bit(UCONTEXT_RECOVERABLE, &ctx->user_flags);
}
+static inline bool i915_gem_context_is_persistent(const struct i915_gem_context *ctx)
+{
+ return test_bit(UCONTEXT_PERSISTENCE, &ctx->user_flags);
+}
+
+static inline void i915_gem_context_set_persistence(struct i915_gem_context *ctx)
+{
+ set_bit(UCONTEXT_PERSISTENCE, &ctx->user_flags);
+}
+
+static inline void i915_gem_context_clear_persistence(struct i915_gem_context *ctx)
+{
+ clear_bit(UCONTEXT_PERSISTENCE, &ctx->user_flags);
+}
+
static inline bool i915_gem_context_is_banned(const struct i915_gem_context *ctx)
{
return test_bit(CONTEXT_BANNED, &ctx->flags);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
index fe97b8ba4fda..861d7d92fe9f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
@@ -137,6 +137,7 @@ struct i915_gem_context {
#define UCONTEXT_NO_ERROR_CAPTURE 1
#define UCONTEXT_BANNABLE 2
#define UCONTEXT_RECOVERABLE 3
+#define UCONTEXT_PERSISTENCE 4
/**
* @flags: small set of booleans
diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_context.c b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
index 74ddd682c9cd..29b8984f0e47 100644
--- a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
@@ -22,6 +22,8 @@ mock_context(struct drm_i915_private *i915,
INIT_LIST_HEAD(&ctx->link);
ctx->i915 = i915;
+ i915_gem_context_set_persistence(ctx);
+
mutex_init(&ctx->engines_mutex);
e = default_engines(ctx);
if (IS_ERR(e))
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 63d40cba97e0..b5fd5e4bd16e 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1572,6 +1572,21 @@ struct drm_i915_gem_context_param {
* i915_context_engines_bond (I915_CONTEXT_ENGINES_EXT_BOND)
*/
#define I915_CONTEXT_PARAM_ENGINES 0xa
+
+/*
+ * I915_CONTEXT_PARAM_PERSISTENCE:
+ *
+ * Allow the context and active rendering to survive the process until
+ * completion. Persistence allows fire-and-forget clients to queue up a
+ * bunch of work, hand the output over to a display server and the quit.
+ * If the context is not marked as persistent, upon closing (either via
+ * an explicit DRM_I915_GEM_CONTEXT_DESTROY or implicitly from file closure
+ * or process termination), the context and any outstanding requests will be
+ * cancelled (and exported fences for cancelled requests marked as -EIO).
+ *
+ * By default, new contexts allow persistence.
+ */
+#define I915_CONTEXT_PARAM_PERSISTENCE 0xb
/* Must be kept compact -- no holes and well documented */
__u64 value;
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 02/11] drm/i915: Put future HW and their uAPIs under STAGING & BROKEN
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula, Daniel Vetter, Dave Airlie
We would like some freedom to break the user API/ABI for future HW but
yet still expose the driver for upstream development on that HW.
Currently, we have the i915.force_probe module parameter to avoid binding
to HW while the driver is under development, but that is still a little
too soft with respect to the stringent no-regression rules if we also
plan to be redesigning the uAPI to go along with the new HW.
To allow the uAPI to be changed during development, only expose that API
and in development HW under STAGING (and BROKEN). Hopefully, making it
explicit that such interfaces to that HW are under development and not
to be blindly enabled by distributions.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dave Airlie <airlied@redhat.com>
---
drivers/gpu/drm/i915/Kconfig | 8 ++++++++
drivers/gpu/drm/i915/Kconfig.debug | 1 +
drivers/gpu/drm/i915/Kconfig.unstable | 20 ++++++++++++++++++++
3 files changed, 29 insertions(+)
create mode 100644 drivers/gpu/drm/i915/Kconfig.unstable
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 3c6d57df262d..1fd9e665b742 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -148,3 +148,11 @@ menu "drm/i915 Profile Guided Optimisation"
depends on DRM_I915
source "drivers/gpu/drm/i915/Kconfig.profile"
endmenu
+
+menu "drm/i915 Ustable Evolution"
+ visible if EXPERT
+ visible if STAGING
+ visible if BROKEN
+ depends on DRM_I915
+ source "drivers/gpu/drm/i915/Kconfig.unstable"
+endmenu
diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
index d2ba8f7e5e50..ef123eb29168 100644
--- a/drivers/gpu/drm/i915/Kconfig.debug
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -44,6 +44,7 @@ config DRM_I915_DEBUG
select DRM_I915_SELFTEST
select DRM_I915_DEBUG_RUNTIME_PM
select DRM_I915_DEBUG_MMIO
+ select BROKEN # for prototype uAPI
default n
help
Choose this option to turn on extra driver debugging that may affect
diff --git a/drivers/gpu/drm/i915/Kconfig.unstable b/drivers/gpu/drm/i915/Kconfig.unstable
new file mode 100644
index 000000000000..ecc8458b5a32
--- /dev/null
+++ b/drivers/gpu/drm/i915/Kconfig.unstable
@@ -0,0 +1,20 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config DRM_I915_UNSTABLE
+ bool "Enable unstable API for early prototype development"
+ depends on EXPERT
+ depends on STAGING
+ depends on BROKEN # should never be enabled by distros!
+ # We use the dependency on !COMPILE_TEST to not be enabled in
+ # allmodconfig or allyesconfig configurations
+ depends on !COMPILE_TEST
+ default n
+ help
+ Enable prototype uAPI under general discussion before they are
+ finalized. Such prototypes may be withdrawn or substantially
+ changed before release. They are only enabled here so that a wide
+ number of interested parties (userspace driver developers) can
+ verify that the uAPI meet their expectations.
+
+ Recommended for driver developers _only_.
+
+ If in the slightest bit of doubt, say "N".
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Intel-gfx] [PATCH 02/11] drm/i915: Put future HW and their uAPIs under STAGING & BROKEN
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula, Daniel Vetter, Dave Airlie
We would like some freedom to break the user API/ABI for future HW but
yet still expose the driver for upstream development on that HW.
Currently, we have the i915.force_probe module parameter to avoid binding
to HW while the driver is under development, but that is still a little
too soft with respect to the stringent no-regression rules if we also
plan to be redesigning the uAPI to go along with the new HW.
To allow the uAPI to be changed during development, only expose that API
and in development HW under STAGING (and BROKEN). Hopefully, making it
explicit that such interfaces to that HW are under development and not
to be blindly enabled by distributions.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dave Airlie <airlied@redhat.com>
---
drivers/gpu/drm/i915/Kconfig | 8 ++++++++
drivers/gpu/drm/i915/Kconfig.debug | 1 +
drivers/gpu/drm/i915/Kconfig.unstable | 20 ++++++++++++++++++++
3 files changed, 29 insertions(+)
create mode 100644 drivers/gpu/drm/i915/Kconfig.unstable
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 3c6d57df262d..1fd9e665b742 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -148,3 +148,11 @@ menu "drm/i915 Profile Guided Optimisation"
depends on DRM_I915
source "drivers/gpu/drm/i915/Kconfig.profile"
endmenu
+
+menu "drm/i915 Ustable Evolution"
+ visible if EXPERT
+ visible if STAGING
+ visible if BROKEN
+ depends on DRM_I915
+ source "drivers/gpu/drm/i915/Kconfig.unstable"
+endmenu
diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
index d2ba8f7e5e50..ef123eb29168 100644
--- a/drivers/gpu/drm/i915/Kconfig.debug
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -44,6 +44,7 @@ config DRM_I915_DEBUG
select DRM_I915_SELFTEST
select DRM_I915_DEBUG_RUNTIME_PM
select DRM_I915_DEBUG_MMIO
+ select BROKEN # for prototype uAPI
default n
help
Choose this option to turn on extra driver debugging that may affect
diff --git a/drivers/gpu/drm/i915/Kconfig.unstable b/drivers/gpu/drm/i915/Kconfig.unstable
new file mode 100644
index 000000000000..ecc8458b5a32
--- /dev/null
+++ b/drivers/gpu/drm/i915/Kconfig.unstable
@@ -0,0 +1,20 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config DRM_I915_UNSTABLE
+ bool "Enable unstable API for early prototype development"
+ depends on EXPERT
+ depends on STAGING
+ depends on BROKEN # should never be enabled by distros!
+ # We use the dependency on !COMPILE_TEST to not be enabled in
+ # allmodconfig or allyesconfig configurations
+ depends on !COMPILE_TEST
+ default n
+ help
+ Enable prototype uAPI under general discussion before they are
+ finalized. Such prototypes may be withdrawn or substantially
+ changed before release. They are only enabled here so that a wide
+ number of interested parties (userspace driver developers) can
+ verify that the uAPI meet their expectations.
+
+ Recommended for driver developers _only_.
+
+ If in the slightest bit of doubt, say "N".
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 03/11] drm/i915/gt: Expose engine properties via sysfs
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
Preliminary stub to add engines underneath /sys/class/drm/cardN/, so
that we can expose properties on each engine to the sysadmin.
To start with we have basic analogues of the i915_query ioctl so that we
can pretty print engine discovery from the shell, and flesh out the
directory structure. Later we will add writeable sysadmin properties such
as per-engine timeout controls.
An example tree of the engine properties on Braswell:
/sys/class/drm/card0
└── engine
├── bcs0
│ ├── capabilities
│ ├── class
│ ├── instance
│ ├── known_capabilities
│ └── name
├── rcs0
│ ├── capabilities
│ ├── class
│ ├── instance
│ ├── known_capabilities
│ └── name
├── vcs0
│ ├── capabilities
│ ├── class
│ ├── instance
│ ├── known_capabilities
│ └── name
└── vecs0
├── capabilities
├── class
├── instance
├── known_capabilities
└── name
v2: Include stringified capabilities
v3: Include all known capabilities for futureproofing.
v4: Combine the two caps loops into one
v5: Hide underneath Kconfig.unstable for wider discussion
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/Kconfig.unstable | 7 +
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/gt/intel_engine_sysfs.c | 208 +++++++++++++++++++
drivers/gpu/drm/i915/gt/intel_engine_sysfs.h | 14 ++
drivers/gpu/drm/i915/i915_sysfs.c | 3 +
5 files changed, 233 insertions(+)
create mode 100644 drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
create mode 100644 drivers/gpu/drm/i915/gt/intel_engine_sysfs.h
diff --git a/drivers/gpu/drm/i915/Kconfig.unstable b/drivers/gpu/drm/i915/Kconfig.unstable
index ecc8458b5a32..a0e9adfdd8eb 100644
--- a/drivers/gpu/drm/i915/Kconfig.unstable
+++ b/drivers/gpu/drm/i915/Kconfig.unstable
@@ -18,3 +18,10 @@ config DRM_I915_UNSTABLE
Recommended for driver developers _only_.
If in the slightest bit of doubt, say "N".
+
+config DRM_I915_UNSTABLE_SYSFS
+ bool "Enable the experimental sysfs properties"
+ depends on DRM_I915_UNSTABLE
+ default n
+ help
+ Explore the HW property space from the shell command line!
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 9bf1c0b20370..5021aa7fa187 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -81,6 +81,7 @@ gt-y += \
gt/intel_engine_heartbeat.o \
gt/intel_engine_pm.o \
gt/intel_engine_pool.o \
+ gt/intel_engine_sysfs.o \
gt/intel_engine_user.o \
gt/intel_gt.o \
gt/intel_gt_irq.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
new file mode 100644
index 000000000000..df263af3a9ea
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
@@ -0,0 +1,208 @@
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2019 Intel Corporation
+ */
+
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
+
+#include "i915_drv.h"
+#include "intel_engine.h"
+#include "intel_engine_sysfs.h"
+
+struct kobj_engine {
+ struct kobject base;
+ struct intel_engine_cs *engine;
+};
+
+static struct intel_engine_cs *kobj_to_engine(struct kobject *kobj)
+{
+ return container_of(kobj, struct kobj_engine, base)->engine;
+}
+
+static ssize_t
+name_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%s\n", kobj_to_engine(kobj)->name);
+}
+
+static struct kobj_attribute name_attr =
+__ATTR(name, 0444, name_show, NULL);
+
+static ssize_t
+class_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%d\n", kobj_to_engine(kobj)->uabi_class);
+}
+
+static struct kobj_attribute class_attr =
+__ATTR(class, 0444, class_show, NULL);
+
+static ssize_t
+inst_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%d\n", kobj_to_engine(kobj)->uabi_instance);
+}
+
+static struct kobj_attribute inst_attr =
+__ATTR(instance, 0444, inst_show, NULL);
+
+static const char * const vcs_caps[] = {
+ [ilog2(I915_VIDEO_CLASS_CAPABILITY_HEVC)] = "hevc",
+ [ilog2(I915_VIDEO_AND_ENHANCE_CLASS_CAPABILITY_SFC)] = "sfc",
+};
+
+static const char * const vecs_caps[] = {
+ [ilog2(I915_VIDEO_AND_ENHANCE_CLASS_CAPABILITY_SFC)] = "sfc",
+};
+
+static ssize_t repr_trim(char *buf, ssize_t len)
+{
+ /* Trim off the trailing space and replace with a newline */
+ if (len > PAGE_SIZE)
+ len = PAGE_SIZE;
+ if (len > 0)
+ buf[len - 1] = '\n';
+
+ return len;
+}
+
+static ssize_t
+__caps_show(struct intel_engine_cs *engine,
+ typeof(engine->uabi_capabilities) caps,
+ char *buf, bool show_unknown)
+{
+ const char * const *repr;
+ int count, n;
+ ssize_t len;
+
+ switch (engine->class) {
+ case VIDEO_DECODE_CLASS:
+ repr = vcs_caps;
+ count = ARRAY_SIZE(vcs_caps);
+ break;
+
+ case VIDEO_ENHANCEMENT_CLASS:
+ repr = vecs_caps;
+ count = ARRAY_SIZE(vecs_caps);
+ break;
+
+ default:
+ repr = NULL;
+ count = 0;
+ break;
+ }
+ GEM_BUG_ON(count > BITS_PER_TYPE(typeof(caps)));
+
+ len = 0;
+ for_each_set_bit(n,
+ (unsigned long *)&caps,
+ show_unknown ? BITS_PER_TYPE(typeof(caps)) : count) {
+ if (n >= count || !repr[n]) {
+ if (GEM_WARN_ON(show_unknown))
+ len += snprintf(buf + len, PAGE_SIZE - len,
+ "[%x] ", n);
+ } else {
+ len += snprintf(buf + len, PAGE_SIZE - len,
+ "%s ", repr[n]);
+ }
+ if (GEM_WARN_ON(len >= PAGE_SIZE))
+ break;
+ }
+ return repr_trim(buf, len);
+}
+
+static ssize_t
+caps_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+
+ return __caps_show(engine, engine->uabi_capabilities, buf, true);
+}
+
+static struct kobj_attribute caps_attr =
+__ATTR(capabilities, 0444, caps_show, NULL);
+
+static ssize_t
+all_caps_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ return __caps_show(kobj_to_engine(kobj), -1, buf, false);
+}
+
+static struct kobj_attribute all_caps_attr =
+__ATTR(known_capabilities, 0444, all_caps_show, NULL);
+
+static void kobj_engine_release(struct kobject *kobj)
+{
+ kfree(kobj);
+}
+
+static struct kobj_type kobj_engine_type = {
+ .release = kobj_engine_release,
+ .sysfs_ops = &kobj_sysfs_ops
+};
+
+static struct kobject *
+kobj_engine(struct kobject *dir, struct intel_engine_cs *engine)
+{
+ struct kobj_engine *ke;
+
+ ke = kzalloc(sizeof(*ke), GFP_KERNEL);
+ if (!ke)
+ return NULL;
+
+ kobject_init(&ke->base, &kobj_engine_type);
+ ke->engine = engine;
+
+ if (kobject_add(&ke->base, dir, "%s", engine->name)) {
+ kobject_put(&ke->base);
+ return NULL;
+ }
+
+ /* xfer ownership to sysfs tree */
+ return &ke->base;
+}
+
+void intel_engines_add_sysfs(struct drm_i915_private *i915)
+{
+ static const struct attribute *files[] = {
+ &name_attr.attr,
+ &class_attr.attr,
+ &inst_attr.attr,
+ &caps_attr.attr,
+ &all_caps_attr.attr,
+ NULL
+ };
+
+ struct device *kdev = i915->drm.primary->kdev;
+ struct intel_engine_cs *engine;
+ struct kobject *dir;
+
+ if (!IS_ENABLED(CONFIG_DRM_I915_UNSTABLE_SYSFS))
+ return;
+
+ dir = kobject_create_and_add("engine", &kdev->kobj);
+ if (!dir)
+ return;
+
+ for_each_uabi_engine(engine, i915) {
+ struct kobject *kobj;
+
+ kobj = kobj_engine(dir, engine);
+ if (!kobj)
+ goto err_engine;
+
+ if (sysfs_create_files(kobj, files))
+ goto err_object;
+
+ if (0) {
+err_object:
+ kobject_put(kobj);
+err_engine:
+ dev_err(kdev, "Failed to add sysfs engine '%s'\n",
+ engine->name);
+ break;
+ }
+ }
+}
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.h b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.h
new file mode 100644
index 000000000000..ef44a745b70a
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.h
@@ -0,0 +1,14 @@
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2019 Intel Corporation
+ */
+
+#ifndef INTEL_ENGINE_SYSFS_H
+#define INTEL_ENGINE_SYSFS_H
+
+struct drm_i915_private;
+
+void intel_engines_add_sysfs(struct drm_i915_private *i915);
+
+#endif /* INTEL_ENGINE_SYSFS_H */
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index bf039b8ba593..7b665f69f301 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -30,6 +30,7 @@
#include <linux/stat.h>
#include <linux/sysfs.h>
+#include "gt/intel_engine_sysfs.h"
#include "gt/intel_rc6.h"
#include "i915_drv.h"
@@ -616,6 +617,8 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
DRM_ERROR("RPS sysfs setup failed\n");
i915_setup_error_capture(kdev);
+
+ intel_engines_add_sysfs(dev_priv);
}
void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Intel-gfx] [PATCH 03/11] drm/i915/gt: Expose engine properties via sysfs
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
Preliminary stub to add engines underneath /sys/class/drm/cardN/, so
that we can expose properties on each engine to the sysadmin.
To start with we have basic analogues of the i915_query ioctl so that we
can pretty print engine discovery from the shell, and flesh out the
directory structure. Later we will add writeable sysadmin properties such
as per-engine timeout controls.
An example tree of the engine properties on Braswell:
/sys/class/drm/card0
└── engine
├── bcs0
│ ├── capabilities
│ ├── class
│ ├── instance
│ ├── known_capabilities
│ └── name
├── rcs0
│ ├── capabilities
│ ├── class
│ ├── instance
│ ├── known_capabilities
│ └── name
├── vcs0
│ ├── capabilities
│ ├── class
│ ├── instance
│ ├── known_capabilities
│ └── name
└── vecs0
├── capabilities
├── class
├── instance
├── known_capabilities
└── name
v2: Include stringified capabilities
v3: Include all known capabilities for futureproofing.
v4: Combine the two caps loops into one
v5: Hide underneath Kconfig.unstable for wider discussion
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/Kconfig.unstable | 7 +
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/gt/intel_engine_sysfs.c | 208 +++++++++++++++++++
drivers/gpu/drm/i915/gt/intel_engine_sysfs.h | 14 ++
drivers/gpu/drm/i915/i915_sysfs.c | 3 +
5 files changed, 233 insertions(+)
create mode 100644 drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
create mode 100644 drivers/gpu/drm/i915/gt/intel_engine_sysfs.h
diff --git a/drivers/gpu/drm/i915/Kconfig.unstable b/drivers/gpu/drm/i915/Kconfig.unstable
index ecc8458b5a32..a0e9adfdd8eb 100644
--- a/drivers/gpu/drm/i915/Kconfig.unstable
+++ b/drivers/gpu/drm/i915/Kconfig.unstable
@@ -18,3 +18,10 @@ config DRM_I915_UNSTABLE
Recommended for driver developers _only_.
If in the slightest bit of doubt, say "N".
+
+config DRM_I915_UNSTABLE_SYSFS
+ bool "Enable the experimental sysfs properties"
+ depends on DRM_I915_UNSTABLE
+ default n
+ help
+ Explore the HW property space from the shell command line!
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 9bf1c0b20370..5021aa7fa187 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -81,6 +81,7 @@ gt-y += \
gt/intel_engine_heartbeat.o \
gt/intel_engine_pm.o \
gt/intel_engine_pool.o \
+ gt/intel_engine_sysfs.o \
gt/intel_engine_user.o \
gt/intel_gt.o \
gt/intel_gt_irq.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
new file mode 100644
index 000000000000..df263af3a9ea
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
@@ -0,0 +1,208 @@
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2019 Intel Corporation
+ */
+
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
+
+#include "i915_drv.h"
+#include "intel_engine.h"
+#include "intel_engine_sysfs.h"
+
+struct kobj_engine {
+ struct kobject base;
+ struct intel_engine_cs *engine;
+};
+
+static struct intel_engine_cs *kobj_to_engine(struct kobject *kobj)
+{
+ return container_of(kobj, struct kobj_engine, base)->engine;
+}
+
+static ssize_t
+name_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%s\n", kobj_to_engine(kobj)->name);
+}
+
+static struct kobj_attribute name_attr =
+__ATTR(name, 0444, name_show, NULL);
+
+static ssize_t
+class_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%d\n", kobj_to_engine(kobj)->uabi_class);
+}
+
+static struct kobj_attribute class_attr =
+__ATTR(class, 0444, class_show, NULL);
+
+static ssize_t
+inst_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%d\n", kobj_to_engine(kobj)->uabi_instance);
+}
+
+static struct kobj_attribute inst_attr =
+__ATTR(instance, 0444, inst_show, NULL);
+
+static const char * const vcs_caps[] = {
+ [ilog2(I915_VIDEO_CLASS_CAPABILITY_HEVC)] = "hevc",
+ [ilog2(I915_VIDEO_AND_ENHANCE_CLASS_CAPABILITY_SFC)] = "sfc",
+};
+
+static const char * const vecs_caps[] = {
+ [ilog2(I915_VIDEO_AND_ENHANCE_CLASS_CAPABILITY_SFC)] = "sfc",
+};
+
+static ssize_t repr_trim(char *buf, ssize_t len)
+{
+ /* Trim off the trailing space and replace with a newline */
+ if (len > PAGE_SIZE)
+ len = PAGE_SIZE;
+ if (len > 0)
+ buf[len - 1] = '\n';
+
+ return len;
+}
+
+static ssize_t
+__caps_show(struct intel_engine_cs *engine,
+ typeof(engine->uabi_capabilities) caps,
+ char *buf, bool show_unknown)
+{
+ const char * const *repr;
+ int count, n;
+ ssize_t len;
+
+ switch (engine->class) {
+ case VIDEO_DECODE_CLASS:
+ repr = vcs_caps;
+ count = ARRAY_SIZE(vcs_caps);
+ break;
+
+ case VIDEO_ENHANCEMENT_CLASS:
+ repr = vecs_caps;
+ count = ARRAY_SIZE(vecs_caps);
+ break;
+
+ default:
+ repr = NULL;
+ count = 0;
+ break;
+ }
+ GEM_BUG_ON(count > BITS_PER_TYPE(typeof(caps)));
+
+ len = 0;
+ for_each_set_bit(n,
+ (unsigned long *)&caps,
+ show_unknown ? BITS_PER_TYPE(typeof(caps)) : count) {
+ if (n >= count || !repr[n]) {
+ if (GEM_WARN_ON(show_unknown))
+ len += snprintf(buf + len, PAGE_SIZE - len,
+ "[%x] ", n);
+ } else {
+ len += snprintf(buf + len, PAGE_SIZE - len,
+ "%s ", repr[n]);
+ }
+ if (GEM_WARN_ON(len >= PAGE_SIZE))
+ break;
+ }
+ return repr_trim(buf, len);
+}
+
+static ssize_t
+caps_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+
+ return __caps_show(engine, engine->uabi_capabilities, buf, true);
+}
+
+static struct kobj_attribute caps_attr =
+__ATTR(capabilities, 0444, caps_show, NULL);
+
+static ssize_t
+all_caps_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ return __caps_show(kobj_to_engine(kobj), -1, buf, false);
+}
+
+static struct kobj_attribute all_caps_attr =
+__ATTR(known_capabilities, 0444, all_caps_show, NULL);
+
+static void kobj_engine_release(struct kobject *kobj)
+{
+ kfree(kobj);
+}
+
+static struct kobj_type kobj_engine_type = {
+ .release = kobj_engine_release,
+ .sysfs_ops = &kobj_sysfs_ops
+};
+
+static struct kobject *
+kobj_engine(struct kobject *dir, struct intel_engine_cs *engine)
+{
+ struct kobj_engine *ke;
+
+ ke = kzalloc(sizeof(*ke), GFP_KERNEL);
+ if (!ke)
+ return NULL;
+
+ kobject_init(&ke->base, &kobj_engine_type);
+ ke->engine = engine;
+
+ if (kobject_add(&ke->base, dir, "%s", engine->name)) {
+ kobject_put(&ke->base);
+ return NULL;
+ }
+
+ /* xfer ownership to sysfs tree */
+ return &ke->base;
+}
+
+void intel_engines_add_sysfs(struct drm_i915_private *i915)
+{
+ static const struct attribute *files[] = {
+ &name_attr.attr,
+ &class_attr.attr,
+ &inst_attr.attr,
+ &caps_attr.attr,
+ &all_caps_attr.attr,
+ NULL
+ };
+
+ struct device *kdev = i915->drm.primary->kdev;
+ struct intel_engine_cs *engine;
+ struct kobject *dir;
+
+ if (!IS_ENABLED(CONFIG_DRM_I915_UNSTABLE_SYSFS))
+ return;
+
+ dir = kobject_create_and_add("engine", &kdev->kobj);
+ if (!dir)
+ return;
+
+ for_each_uabi_engine(engine, i915) {
+ struct kobject *kobj;
+
+ kobj = kobj_engine(dir, engine);
+ if (!kobj)
+ goto err_engine;
+
+ if (sysfs_create_files(kobj, files))
+ goto err_object;
+
+ if (0) {
+err_object:
+ kobject_put(kobj);
+err_engine:
+ dev_err(kdev, "Failed to add sysfs engine '%s'\n",
+ engine->name);
+ break;
+ }
+ }
+}
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.h b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.h
new file mode 100644
index 000000000000..ef44a745b70a
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.h
@@ -0,0 +1,14 @@
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2019 Intel Corporation
+ */
+
+#ifndef INTEL_ENGINE_SYSFS_H
+#define INTEL_ENGINE_SYSFS_H
+
+struct drm_i915_private;
+
+void intel_engines_add_sysfs(struct drm_i915_private *i915);
+
+#endif /* INTEL_ENGINE_SYSFS_H */
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index bf039b8ba593..7b665f69f301 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -30,6 +30,7 @@
#include <linux/stat.h>
#include <linux/sysfs.h>
+#include "gt/intel_engine_sysfs.h"
#include "gt/intel_rc6.h"
#include "i915_drv.h"
@@ -616,6 +617,8 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
DRM_ERROR("RPS sysfs setup failed\n");
i915_setup_error_capture(kdev);
+
+ intel_engines_add_sysfs(dev_priv);
}
void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 04/11] drm/i915/gt: Expose engine->mmio_base via sysfs
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
Use the per-engine sysfs directory to let userspace discover the
mmio_base of each engine. Prior to recent generations, the user
accessible registers on each engine are at a fixed offset relative to
each engine -- but require absolute addressing. As the absolute address
depends on the actual physical engine, this is not always possible to
determine from userspace (for example icl may expose vcs1 or vcs2 as the
second vcs engine). Make this easy for userspace to discover by
providing the mmio_base in sysfs.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/gt/intel_engine_sysfs.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
index df263af3a9ea..abddd8d0f9ae 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
@@ -48,6 +48,15 @@ inst_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
static struct kobj_attribute inst_attr =
__ATTR(instance, 0444, inst_show, NULL);
+static ssize_t
+mmio_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ return sprintf(buf, "0x%x\n", kobj_to_engine(kobj)->mmio_base);
+}
+
+static struct kobj_attribute mmio_attr =
+__ATTR(mmio_base, 0444, mmio_show, NULL);
+
static const char * const vcs_caps[] = {
[ilog2(I915_VIDEO_CLASS_CAPABILITY_HEVC)] = "hevc",
[ilog2(I915_VIDEO_AND_ENHANCE_CLASS_CAPABILITY_SFC)] = "sfc",
@@ -170,6 +179,7 @@ void intel_engines_add_sysfs(struct drm_i915_private *i915)
&name_attr.attr,
&class_attr.attr,
&inst_attr.attr,
+ &mmio_attr.attr,
&caps_attr.attr,
&all_caps_attr.attr,
NULL
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Intel-gfx] [PATCH 04/11] drm/i915/gt: Expose engine->mmio_base via sysfs
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
Use the per-engine sysfs directory to let userspace discover the
mmio_base of each engine. Prior to recent generations, the user
accessible registers on each engine are at a fixed offset relative to
each engine -- but require absolute addressing. As the absolute address
depends on the actual physical engine, this is not always possible to
determine from userspace (for example icl may expose vcs1 or vcs2 as the
second vcs engine). Make this easy for userspace to discover by
providing the mmio_base in sysfs.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/gt/intel_engine_sysfs.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
index df263af3a9ea..abddd8d0f9ae 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
@@ -48,6 +48,15 @@ inst_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
static struct kobj_attribute inst_attr =
__ATTR(instance, 0444, inst_show, NULL);
+static ssize_t
+mmio_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ return sprintf(buf, "0x%x\n", kobj_to_engine(kobj)->mmio_base);
+}
+
+static struct kobj_attribute mmio_attr =
+__ATTR(mmio_base, 0444, mmio_show, NULL);
+
static const char * const vcs_caps[] = {
[ilog2(I915_VIDEO_CLASS_CAPABILITY_HEVC)] = "hevc",
[ilog2(I915_VIDEO_AND_ENHANCE_CLASS_CAPABILITY_SFC)] = "sfc",
@@ -170,6 +179,7 @@ void intel_engines_add_sysfs(struct drm_i915_private *i915)
&name_attr.attr,
&class_attr.attr,
&inst_attr.attr,
+ &mmio_attr.attr,
&caps_attr.attr,
&all_caps_attr.attr,
NULL
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 05/11] drm/i915/gt: Expose timeslice duration to sysfs
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
Execlists uses a scheduling quantum (a timeslice) to alternate execution
between ready-to-run contexts of equal priority. This ensures that all
users (though only if they of equal importance) have the opportunity to
run and prevents livelocks where contexts may have implicit ordering due
to userspace semaphores.
The timeslicing mechanism can be compiled out with
./scripts/config --set-val DRM_I915_TIMESLICE_DURATION 0
The timeslice duration can be adjusted per-engine using,
/sys/class/drm/card?/engine/*/timeslice_duration_ms
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/Kconfig.profile | 18 ++++++++
drivers/gpu/drm/i915/gt/intel_engine.h | 9 ++++
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 +
drivers/gpu/drm/i915/gt/intel_engine_sysfs.c | 46 ++++++++++++++++++++
drivers/gpu/drm/i915/gt/intel_engine_types.h | 1 +
drivers/gpu/drm/i915/gt/intel_lrc.c | 39 ++++++++++++-----
drivers/gpu/drm/i915/gt/selftest_lrc.c | 13 +++++-
7 files changed, 114 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/Kconfig.profile b/drivers/gpu/drm/i915/Kconfig.profile
index 8ab7af5eb311..b87c8f485a24 100644
--- a/drivers/gpu/drm/i915/Kconfig.profile
+++ b/drivers/gpu/drm/i915/Kconfig.profile
@@ -59,3 +59,21 @@ config DRM_I915_STOP_TIMEOUT
damage as the system is reset in order to recover. The corollary is
that the reset itself may take longer and so be more disruptive to
interactive or low latency workloads.
+
+config DRM_I915_TIMESLICE_DURATION
+ int "Scheduling quantum for userspace batches (ms, jiffy granularity)"
+ default 1 # milliseconds
+ help
+ When two user batches of equal priority are executing, we will
+ alternate execution of each batch to ensure forward progress of
+ all users. This is necessary in some cases where there may be
+ an implicit dependency between those batches that requires
+ concurrent execution in order for them to proceed, e.g. they
+ interact with each other via userspace semaphores. Each context
+ is scheduled for execution for the timeslice duration, before
+ switching to the next context.
+
+ This is adjustable via
+ /sys/class/drm/card?/engine/*/timeslice_duration_ms
+
+ May be 0 to disable timeslicing.
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index 97bbdd9773c9..d77b9f9f096c 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -390,4 +390,13 @@ intel_engine_has_preempt_reset(const struct intel_engine_cs *engine)
return intel_engine_has_preemption(engine);
}
+static inline bool
+intel_engine_has_timeslices(const struct intel_engine_cs *engine)
+{
+ if (!CONFIG_DRM_I915_TIMESLICE_DURATION)
+ return 0;
+
+ return intel_engine_has_semaphores(engine);
+}
+
#endif /* _INTEL_RINGBUFFER_H_ */
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 9cc1ea6519ec..2afa2ef90482 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -315,6 +315,8 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
CONFIG_DRM_I915_PREEMPT_TIMEOUT;
engine->props.stop_timeout_ms =
CONFIG_DRM_I915_STOP_TIMEOUT;
+ engine->props.timeslice_duration_ms =
+ CONFIG_DRM_I915_TIMESLICE_DURATION;
/*
* To be overridden by the backend on setup. However to facilitate
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
index abddd8d0f9ae..b1bd768b13d7 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
@@ -142,6 +142,48 @@ all_caps_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
static struct kobj_attribute all_caps_attr =
__ATTR(known_capabilities, 0444, all_caps_show, NULL);
+static ssize_t
+timeslice_store(struct kobject *kobj, struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+ unsigned long long duration;
+ int err;
+
+ /*
+ * Execlists uses a scheduling quantum (a timeslice) to alternate
+ * execution between ready-to-run contexts of equal priority. This
+ * ensures that all users (though only if they of equal importance)
+ * have the opportunity to run and prevents livelocks where contexts
+ * may have implicit ordering due to userspace semaphores.
+ */
+
+ err = kstrtoull(buf, 0, &duration);
+ if (err)
+ return err;
+
+ if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
+ return -EINVAL;
+
+ WRITE_ONCE(engine->props.timeslice_duration_ms, duration);
+
+ if (execlists_active(&engine->execlists))
+ set_timer_ms(&engine->execlists.timer, duration);
+
+ return count;
+}
+
+static ssize_t
+timeslice_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+
+ return sprintf(buf, "%lu\n", engine->props.timeslice_duration_ms);
+}
+
+static struct kobj_attribute timeslice_duration_attr =
+__ATTR(timeslice_duration_ms, 0644, timeslice_show, timeslice_store);
+
static void kobj_engine_release(struct kobject *kobj)
{
kfree(kobj);
@@ -206,6 +248,10 @@ void intel_engines_add_sysfs(struct drm_i915_private *i915)
if (sysfs_create_files(kobj, files))
goto err_object;
+ if (intel_engine_has_timeslices(engine) &&
+ sysfs_create_file(kobj, ×lice_duration_attr.attr))
+ goto err_engine;
+
if (0) {
err_object:
kobject_put(kobj);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index e8ea12b96755..c5d1047a4bc5 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -523,6 +523,7 @@ struct intel_engine_cs {
unsigned long heartbeat_interval_ms;
unsigned long preempt_timeout_ms;
unsigned long stop_timeout_ms;
+ unsigned long timeslice_duration_ms;
} props;
};
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 73eae85a2cc9..a7755d6dc48b 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1419,7 +1419,7 @@ need_timeslice(struct intel_engine_cs *engine, const struct i915_request *rq)
{
int hint;
- if (!intel_engine_has_semaphores(engine))
+ if (!intel_engine_has_timeslices(engine))
return false;
if (list_is_last(&rq->sched.link, &engine->active.requests))
@@ -1440,15 +1440,32 @@ switch_prio(struct intel_engine_cs *engine, const struct i915_request *rq)
return rq_prio(list_next_entry(rq, sched.link));
}
-static bool
-enable_timeslice(const struct intel_engine_execlists *execlists)
+static inline unsigned long
+timeslice(const struct intel_engine_cs *engine)
+{
+ return READ_ONCE(engine->props.timeslice_duration_ms);
+}
+
+static unsigned long
+active_timeslice(const struct intel_engine_cs *engine)
{
- const struct i915_request *rq = *execlists->active;
+ const struct i915_request *rq = *engine->execlists.active;
if (i915_request_completed(rq))
- return false;
+ return 0;
+
+ if (engine->execlists.switch_priority_hint < effective_prio(rq))
+ return 0;
+
+ return timeslice(engine);
+}
+
+static void set_timeslice(struct intel_engine_cs *engine)
+{
+ if (!intel_engine_has_timeslices(engine))
+ return;
- return execlists->switch_priority_hint >= effective_prio(rq);
+ set_timer_ms(&engine->execlists.timer, active_timeslice(engine));
}
static void record_preemption(struct intel_engine_execlists *execlists)
@@ -1619,8 +1636,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
*/
if (!execlists->timer.expires &&
need_timeslice(engine, last))
- mod_timer(&execlists->timer,
- jiffies + 1);
+ set_timer_ms(&execlists->timer,
+ timeslice(engine));
+
return;
}
@@ -2044,10 +2062,7 @@ static void process_csb(struct intel_engine_cs *engine)
execlists_num_ports(execlists) *
sizeof(*execlists->pending));
- if (enable_timeslice(execlists))
- mod_timer(&execlists->timer, jiffies + 1);
- else
- cancel_timer(&execlists->timer);
+ set_timeslice(engine);
WRITE_ONCE(execlists->pending[0], NULL);
} else {
diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
index d5d268be554e..15f1ed4d2dd1 100644
--- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
@@ -440,6 +440,8 @@ static int live_timeslice_preempt(void *arg)
* need to preempt the current task and replace it with another
* ready task.
*/
+ if (!CONFIG_DRM_I915_TIMESLICE_DURATION)
+ return 0;
obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
if (IS_ERR(obj))
@@ -514,6 +516,11 @@ static void wait_for_submit(struct intel_engine_cs *engine,
} while (!i915_request_is_active(rq));
}
+static long timeslice_threshold(const struct intel_engine_cs *engine)
+{
+ return 2 * msecs_to_jiffies_timeout(timeslice(engine)) + 1;
+}
+
static int live_timeslice_queue(void *arg)
{
struct intel_gt *gt = arg;
@@ -531,6 +538,8 @@ static int live_timeslice_queue(void *arg)
* ELSP[1] is already occupied, so must rely on timeslicing to
* eject ELSP[0] in favour of the queue.)
*/
+ if (!CONFIG_DRM_I915_TIMESLICE_DURATION)
+ return 0;
obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
if (IS_ERR(obj))
@@ -608,8 +617,8 @@ static int live_timeslice_queue(void *arg)
err = -EINVAL;
}
- /* Timeslice every jiffie, so within 2 we should signal */
- if (i915_request_wait(rq, 0, 3) < 0) {
+ /* Timeslice every jiffy, so within 2 we should signal */
+ if (i915_request_wait(rq, 0, timeslice_threshold(engine)) < 0) {
struct drm_printer p =
drm_info_printer(gt->i915->drm.dev);
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Intel-gfx] [PATCH 05/11] drm/i915/gt: Expose timeslice duration to sysfs
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
Execlists uses a scheduling quantum (a timeslice) to alternate execution
between ready-to-run contexts of equal priority. This ensures that all
users (though only if they of equal importance) have the opportunity to
run and prevents livelocks where contexts may have implicit ordering due
to userspace semaphores.
The timeslicing mechanism can be compiled out with
./scripts/config --set-val DRM_I915_TIMESLICE_DURATION 0
The timeslice duration can be adjusted per-engine using,
/sys/class/drm/card?/engine/*/timeslice_duration_ms
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/Kconfig.profile | 18 ++++++++
drivers/gpu/drm/i915/gt/intel_engine.h | 9 ++++
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 +
drivers/gpu/drm/i915/gt/intel_engine_sysfs.c | 46 ++++++++++++++++++++
drivers/gpu/drm/i915/gt/intel_engine_types.h | 1 +
drivers/gpu/drm/i915/gt/intel_lrc.c | 39 ++++++++++++-----
drivers/gpu/drm/i915/gt/selftest_lrc.c | 13 +++++-
7 files changed, 114 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/Kconfig.profile b/drivers/gpu/drm/i915/Kconfig.profile
index 8ab7af5eb311..b87c8f485a24 100644
--- a/drivers/gpu/drm/i915/Kconfig.profile
+++ b/drivers/gpu/drm/i915/Kconfig.profile
@@ -59,3 +59,21 @@ config DRM_I915_STOP_TIMEOUT
damage as the system is reset in order to recover. The corollary is
that the reset itself may take longer and so be more disruptive to
interactive or low latency workloads.
+
+config DRM_I915_TIMESLICE_DURATION
+ int "Scheduling quantum for userspace batches (ms, jiffy granularity)"
+ default 1 # milliseconds
+ help
+ When two user batches of equal priority are executing, we will
+ alternate execution of each batch to ensure forward progress of
+ all users. This is necessary in some cases where there may be
+ an implicit dependency between those batches that requires
+ concurrent execution in order for them to proceed, e.g. they
+ interact with each other via userspace semaphores. Each context
+ is scheduled for execution for the timeslice duration, before
+ switching to the next context.
+
+ This is adjustable via
+ /sys/class/drm/card?/engine/*/timeslice_duration_ms
+
+ May be 0 to disable timeslicing.
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index 97bbdd9773c9..d77b9f9f096c 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -390,4 +390,13 @@ intel_engine_has_preempt_reset(const struct intel_engine_cs *engine)
return intel_engine_has_preemption(engine);
}
+static inline bool
+intel_engine_has_timeslices(const struct intel_engine_cs *engine)
+{
+ if (!CONFIG_DRM_I915_TIMESLICE_DURATION)
+ return 0;
+
+ return intel_engine_has_semaphores(engine);
+}
+
#endif /* _INTEL_RINGBUFFER_H_ */
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 9cc1ea6519ec..2afa2ef90482 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -315,6 +315,8 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
CONFIG_DRM_I915_PREEMPT_TIMEOUT;
engine->props.stop_timeout_ms =
CONFIG_DRM_I915_STOP_TIMEOUT;
+ engine->props.timeslice_duration_ms =
+ CONFIG_DRM_I915_TIMESLICE_DURATION;
/*
* To be overridden by the backend on setup. However to facilitate
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
index abddd8d0f9ae..b1bd768b13d7 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
@@ -142,6 +142,48 @@ all_caps_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
static struct kobj_attribute all_caps_attr =
__ATTR(known_capabilities, 0444, all_caps_show, NULL);
+static ssize_t
+timeslice_store(struct kobject *kobj, struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+ unsigned long long duration;
+ int err;
+
+ /*
+ * Execlists uses a scheduling quantum (a timeslice) to alternate
+ * execution between ready-to-run contexts of equal priority. This
+ * ensures that all users (though only if they of equal importance)
+ * have the opportunity to run and prevents livelocks where contexts
+ * may have implicit ordering due to userspace semaphores.
+ */
+
+ err = kstrtoull(buf, 0, &duration);
+ if (err)
+ return err;
+
+ if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
+ return -EINVAL;
+
+ WRITE_ONCE(engine->props.timeslice_duration_ms, duration);
+
+ if (execlists_active(&engine->execlists))
+ set_timer_ms(&engine->execlists.timer, duration);
+
+ return count;
+}
+
+static ssize_t
+timeslice_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+
+ return sprintf(buf, "%lu\n", engine->props.timeslice_duration_ms);
+}
+
+static struct kobj_attribute timeslice_duration_attr =
+__ATTR(timeslice_duration_ms, 0644, timeslice_show, timeslice_store);
+
static void kobj_engine_release(struct kobject *kobj)
{
kfree(kobj);
@@ -206,6 +248,10 @@ void intel_engines_add_sysfs(struct drm_i915_private *i915)
if (sysfs_create_files(kobj, files))
goto err_object;
+ if (intel_engine_has_timeslices(engine) &&
+ sysfs_create_file(kobj, ×lice_duration_attr.attr))
+ goto err_engine;
+
if (0) {
err_object:
kobject_put(kobj);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index e8ea12b96755..c5d1047a4bc5 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -523,6 +523,7 @@ struct intel_engine_cs {
unsigned long heartbeat_interval_ms;
unsigned long preempt_timeout_ms;
unsigned long stop_timeout_ms;
+ unsigned long timeslice_duration_ms;
} props;
};
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 73eae85a2cc9..a7755d6dc48b 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1419,7 +1419,7 @@ need_timeslice(struct intel_engine_cs *engine, const struct i915_request *rq)
{
int hint;
- if (!intel_engine_has_semaphores(engine))
+ if (!intel_engine_has_timeslices(engine))
return false;
if (list_is_last(&rq->sched.link, &engine->active.requests))
@@ -1440,15 +1440,32 @@ switch_prio(struct intel_engine_cs *engine, const struct i915_request *rq)
return rq_prio(list_next_entry(rq, sched.link));
}
-static bool
-enable_timeslice(const struct intel_engine_execlists *execlists)
+static inline unsigned long
+timeslice(const struct intel_engine_cs *engine)
+{
+ return READ_ONCE(engine->props.timeslice_duration_ms);
+}
+
+static unsigned long
+active_timeslice(const struct intel_engine_cs *engine)
{
- const struct i915_request *rq = *execlists->active;
+ const struct i915_request *rq = *engine->execlists.active;
if (i915_request_completed(rq))
- return false;
+ return 0;
+
+ if (engine->execlists.switch_priority_hint < effective_prio(rq))
+ return 0;
+
+ return timeslice(engine);
+}
+
+static void set_timeslice(struct intel_engine_cs *engine)
+{
+ if (!intel_engine_has_timeslices(engine))
+ return;
- return execlists->switch_priority_hint >= effective_prio(rq);
+ set_timer_ms(&engine->execlists.timer, active_timeslice(engine));
}
static void record_preemption(struct intel_engine_execlists *execlists)
@@ -1619,8 +1636,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
*/
if (!execlists->timer.expires &&
need_timeslice(engine, last))
- mod_timer(&execlists->timer,
- jiffies + 1);
+ set_timer_ms(&execlists->timer,
+ timeslice(engine));
+
return;
}
@@ -2044,10 +2062,7 @@ static void process_csb(struct intel_engine_cs *engine)
execlists_num_ports(execlists) *
sizeof(*execlists->pending));
- if (enable_timeslice(execlists))
- mod_timer(&execlists->timer, jiffies + 1);
- else
- cancel_timer(&execlists->timer);
+ set_timeslice(engine);
WRITE_ONCE(execlists->pending[0], NULL);
} else {
diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
index d5d268be554e..15f1ed4d2dd1 100644
--- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
@@ -440,6 +440,8 @@ static int live_timeslice_preempt(void *arg)
* need to preempt the current task and replace it with another
* ready task.
*/
+ if (!CONFIG_DRM_I915_TIMESLICE_DURATION)
+ return 0;
obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
if (IS_ERR(obj))
@@ -514,6 +516,11 @@ static void wait_for_submit(struct intel_engine_cs *engine,
} while (!i915_request_is_active(rq));
}
+static long timeslice_threshold(const struct intel_engine_cs *engine)
+{
+ return 2 * msecs_to_jiffies_timeout(timeslice(engine)) + 1;
+}
+
static int live_timeslice_queue(void *arg)
{
struct intel_gt *gt = arg;
@@ -531,6 +538,8 @@ static int live_timeslice_queue(void *arg)
* ELSP[1] is already occupied, so must rely on timeslicing to
* eject ELSP[0] in favour of the queue.)
*/
+ if (!CONFIG_DRM_I915_TIMESLICE_DURATION)
+ return 0;
obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
if (IS_ERR(obj))
@@ -608,8 +617,8 @@ static int live_timeslice_queue(void *arg)
err = -EINVAL;
}
- /* Timeslice every jiffie, so within 2 we should signal */
- if (i915_request_wait(rq, 0, 3) < 0) {
+ /* Timeslice every jiffy, so within 2 we should signal */
+ if (i915_request_wait(rq, 0, timeslice_threshold(engine)) < 0) {
struct drm_printer p =
drm_info_printer(gt->i915->drm.dev);
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 06/11] drm/i915/gt: Expose reset stop timeout via sysfs
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
When we allow ourselves to sleep before a GPU reset after disabling
submission, even for a few milliseconds, gives an innocent context the
opportunity to clear the GPU before the reset occurs. However, how long
to sleep depends on the typical non-preemptible duration (a similar
problem to determining the ideal preempt-reset timeout or even the
heartbeat interval). As this seems of a hard policy decision, punt it to
userspace.
The timeout can be adjusted using
/sys/class/drm/card?/engine/*/stop_timeout_ms
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
---
drivers/gpu/drm/i915/Kconfig.profile | 3 ++
drivers/gpu/drm/i915/gt/intel_engine_sysfs.c | 40 ++++++++++++++++++++
2 files changed, 43 insertions(+)
diff --git a/drivers/gpu/drm/i915/Kconfig.profile b/drivers/gpu/drm/i915/Kconfig.profile
index b87c8f485a24..76145d25ce65 100644
--- a/drivers/gpu/drm/i915/Kconfig.profile
+++ b/drivers/gpu/drm/i915/Kconfig.profile
@@ -60,6 +60,9 @@ config DRM_I915_STOP_TIMEOUT
that the reset itself may take longer and so be more disruptive to
interactive or low latency workloads.
+ This is adjustable via
+ /sys/class/drm/card?/engine/*/stop_timeout_ms
+
config DRM_I915_TIMESLICE_DURATION
int "Scheduling quantum for userspace batches (ms, jiffy granularity)"
default 1 # milliseconds
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
index b1bd768b13d7..86377a4ffe70 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
@@ -184,6 +184,45 @@ timeslice_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
static struct kobj_attribute timeslice_duration_attr =
__ATTR(timeslice_duration_ms, 0644, timeslice_show, timeslice_store);
+static ssize_t
+stop_store(struct kobject *kobj, struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+ unsigned long long duration;
+ int err;
+
+ /*
+ * When we allow ourselves to sleep before a GPU reset after disabling
+ * submission, even for a few milliseconds, gives an innocent context
+ * the opportunity to clear the GPU before the reset occurs. However,
+ * how long to sleep depends on the typical non-preemptible duration
+ * (a similar problem to determining the ideal preempt-reset timeout
+ * or even the heartbeat interval).
+ */
+
+ err = kstrtoull(buf, 0, &duration);
+ if (err)
+ return err;
+
+ if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
+ return -EINVAL;
+
+ WRITE_ONCE(engine->props.stop_timeout_ms, duration);
+ return count;
+}
+
+static ssize_t
+stop_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+
+ return sprintf(buf, "%lu\n", engine->props.stop_timeout_ms);
+}
+
+static struct kobj_attribute stop_timeout_attr =
+__ATTR(stop_timeout_ms, 0644, stop_show, stop_store);
+
static void kobj_engine_release(struct kobject *kobj)
{
kfree(kobj);
@@ -224,6 +263,7 @@ void intel_engines_add_sysfs(struct drm_i915_private *i915)
&mmio_attr.attr,
&caps_attr.attr,
&all_caps_attr.attr,
+ &stop_timeout_attr.attr,
NULL
};
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Intel-gfx] [PATCH 06/11] drm/i915/gt: Expose reset stop timeout via sysfs
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
When we allow ourselves to sleep before a GPU reset after disabling
submission, even for a few milliseconds, gives an innocent context the
opportunity to clear the GPU before the reset occurs. However, how long
to sleep depends on the typical non-preemptible duration (a similar
problem to determining the ideal preempt-reset timeout or even the
heartbeat interval). As this seems of a hard policy decision, punt it to
userspace.
The timeout can be adjusted using
/sys/class/drm/card?/engine/*/stop_timeout_ms
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
---
drivers/gpu/drm/i915/Kconfig.profile | 3 ++
drivers/gpu/drm/i915/gt/intel_engine_sysfs.c | 40 ++++++++++++++++++++
2 files changed, 43 insertions(+)
diff --git a/drivers/gpu/drm/i915/Kconfig.profile b/drivers/gpu/drm/i915/Kconfig.profile
index b87c8f485a24..76145d25ce65 100644
--- a/drivers/gpu/drm/i915/Kconfig.profile
+++ b/drivers/gpu/drm/i915/Kconfig.profile
@@ -60,6 +60,9 @@ config DRM_I915_STOP_TIMEOUT
that the reset itself may take longer and so be more disruptive to
interactive or low latency workloads.
+ This is adjustable via
+ /sys/class/drm/card?/engine/*/stop_timeout_ms
+
config DRM_I915_TIMESLICE_DURATION
int "Scheduling quantum for userspace batches (ms, jiffy granularity)"
default 1 # milliseconds
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
index b1bd768b13d7..86377a4ffe70 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
@@ -184,6 +184,45 @@ timeslice_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
static struct kobj_attribute timeslice_duration_attr =
__ATTR(timeslice_duration_ms, 0644, timeslice_show, timeslice_store);
+static ssize_t
+stop_store(struct kobject *kobj, struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+ unsigned long long duration;
+ int err;
+
+ /*
+ * When we allow ourselves to sleep before a GPU reset after disabling
+ * submission, even for a few milliseconds, gives an innocent context
+ * the opportunity to clear the GPU before the reset occurs. However,
+ * how long to sleep depends on the typical non-preemptible duration
+ * (a similar problem to determining the ideal preempt-reset timeout
+ * or even the heartbeat interval).
+ */
+
+ err = kstrtoull(buf, 0, &duration);
+ if (err)
+ return err;
+
+ if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
+ return -EINVAL;
+
+ WRITE_ONCE(engine->props.stop_timeout_ms, duration);
+ return count;
+}
+
+static ssize_t
+stop_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+
+ return sprintf(buf, "%lu\n", engine->props.stop_timeout_ms);
+}
+
+static struct kobj_attribute stop_timeout_attr =
+__ATTR(stop_timeout_ms, 0644, stop_show, stop_store);
+
static void kobj_engine_release(struct kobject *kobj)
{
kfree(kobj);
@@ -224,6 +263,7 @@ void intel_engines_add_sysfs(struct drm_i915_private *i915)
&mmio_attr.attr,
&caps_attr.attr,
&all_caps_attr.attr,
+ &stop_timeout_attr.attr,
NULL
};
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 07/11] drm/i915/gt: Expose preempt reset timeout via sysfs
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
After initialising a preemption request, we give the current resident a
small amount of time to vacate the GPU. The preemption request is for a
higher priority context and should be immediate to maintain high
quality of service (and avoid priority inversion). However, the
preemption granularity of the GPU can be quite coarse and so we need a
compromise.
The preempt timeout can be adjusted per-engine using,
/sys/class/drm/card?/engine/*/preempt_timeout_ms
and can be disabled by setting it to 0.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/Kconfig.profile | 3 ++
drivers/gpu/drm/i915/gt/intel_engine_sysfs.c | 48 ++++++++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/drivers/gpu/drm/i915/Kconfig.profile b/drivers/gpu/drm/i915/Kconfig.profile
index 76145d25ce65..066ea9ba2756 100644
--- a/drivers/gpu/drm/i915/Kconfig.profile
+++ b/drivers/gpu/drm/i915/Kconfig.profile
@@ -33,6 +33,9 @@ config DRM_I915_PREEMPT_TIMEOUT
expires, the HW will be reset to allow the more important context
to execute.
+ This is adjustable via
+ /sys/class/drm/card?/engine/*/preempt_timeout_ms
+
May be 0 to disable the timeout.
config DRM_I915_SPIN_REQUEST
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
index 86377a4ffe70..25012297aa9e 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
@@ -223,6 +223,50 @@ stop_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
static struct kobj_attribute stop_timeout_attr =
__ATTR(stop_timeout_ms, 0644, stop_show, stop_store);
+static ssize_t
+preempt_timeout_store(struct kobject *kobj, struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+ unsigned long long timeout;
+ int err;
+
+ /*
+ * After initialising a preemption request, we give the current
+ * resident a small amount of time to vacate the GPU. The preemption
+ * request is for a higher priority context and should be immediate to
+ * maintain high quality of service (and avoid priority inversion).
+ * However, the preemption granularity of the GPU can be quite coarse
+ * and so we need a compromise.
+ */
+
+ err = kstrtoull(buf, 0, &timeout);
+ if (err)
+ return err;
+
+ if (timeout > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
+ return -EINVAL;
+
+ WRITE_ONCE(engine->props.preempt_timeout_ms, timeout);
+
+ if (READ_ONCE(engine->execlists.pending[0]))
+ set_timer_ms(&engine->execlists.preempt, timeout);
+
+ return count;
+}
+
+static ssize_t
+preempt_timeout_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+
+ return sprintf(buf, "%lu\n", engine->props.preempt_timeout_ms);
+}
+
+static struct kobj_attribute preempt_timeout_attr =
+__ATTR(preempt_timeout_ms, 0644, preempt_timeout_show, preempt_timeout_store);
+
static void kobj_engine_release(struct kobject *kobj)
{
kfree(kobj);
@@ -292,6 +336,10 @@ void intel_engines_add_sysfs(struct drm_i915_private *i915)
sysfs_create_file(kobj, ×lice_duration_attr.attr))
goto err_engine;
+ if (intel_engine_has_preempt_reset(engine) &&
+ sysfs_create_file(kobj, &preempt_timeout_attr.attr))
+ goto err_engine;
+
if (0) {
err_object:
kobject_put(kobj);
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Intel-gfx] [PATCH 07/11] drm/i915/gt: Expose preempt reset timeout via sysfs
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
After initialising a preemption request, we give the current resident a
small amount of time to vacate the GPU. The preemption request is for a
higher priority context and should be immediate to maintain high
quality of service (and avoid priority inversion). However, the
preemption granularity of the GPU can be quite coarse and so we need a
compromise.
The preempt timeout can be adjusted per-engine using,
/sys/class/drm/card?/engine/*/preempt_timeout_ms
and can be disabled by setting it to 0.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/Kconfig.profile | 3 ++
drivers/gpu/drm/i915/gt/intel_engine_sysfs.c | 48 ++++++++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/drivers/gpu/drm/i915/Kconfig.profile b/drivers/gpu/drm/i915/Kconfig.profile
index 76145d25ce65..066ea9ba2756 100644
--- a/drivers/gpu/drm/i915/Kconfig.profile
+++ b/drivers/gpu/drm/i915/Kconfig.profile
@@ -33,6 +33,9 @@ config DRM_I915_PREEMPT_TIMEOUT
expires, the HW will be reset to allow the more important context
to execute.
+ This is adjustable via
+ /sys/class/drm/card?/engine/*/preempt_timeout_ms
+
May be 0 to disable the timeout.
config DRM_I915_SPIN_REQUEST
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
index 86377a4ffe70..25012297aa9e 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
@@ -223,6 +223,50 @@ stop_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
static struct kobj_attribute stop_timeout_attr =
__ATTR(stop_timeout_ms, 0644, stop_show, stop_store);
+static ssize_t
+preempt_timeout_store(struct kobject *kobj, struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+ unsigned long long timeout;
+ int err;
+
+ /*
+ * After initialising a preemption request, we give the current
+ * resident a small amount of time to vacate the GPU. The preemption
+ * request is for a higher priority context and should be immediate to
+ * maintain high quality of service (and avoid priority inversion).
+ * However, the preemption granularity of the GPU can be quite coarse
+ * and so we need a compromise.
+ */
+
+ err = kstrtoull(buf, 0, &timeout);
+ if (err)
+ return err;
+
+ if (timeout > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
+ return -EINVAL;
+
+ WRITE_ONCE(engine->props.preempt_timeout_ms, timeout);
+
+ if (READ_ONCE(engine->execlists.pending[0]))
+ set_timer_ms(&engine->execlists.preempt, timeout);
+
+ return count;
+}
+
+static ssize_t
+preempt_timeout_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+
+ return sprintf(buf, "%lu\n", engine->props.preempt_timeout_ms);
+}
+
+static struct kobj_attribute preempt_timeout_attr =
+__ATTR(preempt_timeout_ms, 0644, preempt_timeout_show, preempt_timeout_store);
+
static void kobj_engine_release(struct kobject *kobj)
{
kfree(kobj);
@@ -292,6 +336,10 @@ void intel_engines_add_sysfs(struct drm_i915_private *i915)
sysfs_create_file(kobj, ×lice_duration_attr.attr))
goto err_engine;
+ if (intel_engine_has_preempt_reset(engine) &&
+ sysfs_create_file(kobj, &preempt_timeout_attr.attr))
+ goto err_engine;
+
if (0) {
err_object:
kobject_put(kobj);
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 08/11] drm/i915/gt: Expose heartbeat interval via sysfs
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
We monitor the health of the system via periodic heartbeat pulses. The
pulses also provide the opportunity to perform garbage collection.
However, we interpret an incomplete pulse (a missed heartbeat) as an
indication that the system is no longer responsive, i.e. hung, and
perform an engine or full GPU reset. Given that the preemption
granularity can be very coarse on a system, we let the sysadmin override
our legacy timeouts which were "optimised" for desktop applications.
The heartbeat interval can be adjusted per-engine using,
/sys/class/drm/card?/engine/*/heartbeat_interval_ms
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/Kconfig.profile | 3 ++
drivers/gpu/drm/i915/gt/intel_engine_sysfs.c | 47 ++++++++++++++++++++
2 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/i915/Kconfig.profile b/drivers/gpu/drm/i915/Kconfig.profile
index 066ea9ba2756..aad214d3160d 100644
--- a/drivers/gpu/drm/i915/Kconfig.profile
+++ b/drivers/gpu/drm/i915/Kconfig.profile
@@ -20,6 +20,9 @@ config DRM_I915_HEARTBEAT_INTERVAL
check the health of the GPU and undertake regular house-keeping of
internal driver state.
+ This is adjustable via
+ /sys/class/drm/card?/engine/*/heartbeat_interval_ms
+
May be 0 to disable heartbeats and therefore disable automatic GPU
hang detection.
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
index 25012297aa9e..e70a0358bb6e 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
@@ -9,6 +9,7 @@
#include "i915_drv.h"
#include "intel_engine.h"
+#include "intel_engine_heartbeat.h"
#include "intel_engine_sysfs.h"
struct kobj_engine {
@@ -267,6 +268,49 @@ preempt_timeout_show(struct kobject *kobj, struct kobj_attribute *attr,
static struct kobj_attribute preempt_timeout_attr =
__ATTR(preempt_timeout_ms, 0644, preempt_timeout_show, preempt_timeout_store);
+static ssize_t
+heartbeat_store(struct kobject *kobj, struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+ unsigned long long delay;
+ int err;
+
+ /*
+ * We monitor the health of the system via periodic heartbeat pulses.
+ * The pulses also provide the opportunity to perform garbage
+ * collection. However, we interpret an incomplete pulse (a missed
+ * heartbeat) as an indication that the system is no longer responsive,
+ * i.e. hung, and perform an engine or full GPU reset. Given that the
+ * preemption granularity can be very coarse on a system, the optimal
+ * value for any workload is unknowable!
+ */
+
+ err = kstrtoull(buf, 0, &delay);
+ if (err)
+ return err;
+
+ if (delay >= jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
+ return -EINVAL;
+
+ err = intel_engine_set_heartbeat(engine, delay);
+ if (err)
+ return err;
+
+ return count;
+}
+
+static ssize_t
+heartbeat_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+
+ return sprintf(buf, "%lu\n", engine->props.heartbeat_interval_ms);
+}
+
+static struct kobj_attribute heartbeat_interval_attr =
+__ATTR(heartbeat_interval_ms, 0644, heartbeat_show, heartbeat_store);
+
static void kobj_engine_release(struct kobject *kobj)
{
kfree(kobj);
@@ -308,6 +352,9 @@ void intel_engines_add_sysfs(struct drm_i915_private *i915)
&caps_attr.attr,
&all_caps_attr.attr,
&stop_timeout_attr.attr,
+#if CONFIG_DRM_I915_HEARTBEAT_INTERVAL
+ &heartbeat_interval_attr.attr,
+#endif
NULL
};
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Intel-gfx] [PATCH 08/11] drm/i915/gt: Expose heartbeat interval via sysfs
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
We monitor the health of the system via periodic heartbeat pulses. The
pulses also provide the opportunity to perform garbage collection.
However, we interpret an incomplete pulse (a missed heartbeat) as an
indication that the system is no longer responsive, i.e. hung, and
perform an engine or full GPU reset. Given that the preemption
granularity can be very coarse on a system, we let the sysadmin override
our legacy timeouts which were "optimised" for desktop applications.
The heartbeat interval can be adjusted per-engine using,
/sys/class/drm/card?/engine/*/heartbeat_interval_ms
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/Kconfig.profile | 3 ++
drivers/gpu/drm/i915/gt/intel_engine_sysfs.c | 47 ++++++++++++++++++++
2 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/i915/Kconfig.profile b/drivers/gpu/drm/i915/Kconfig.profile
index 066ea9ba2756..aad214d3160d 100644
--- a/drivers/gpu/drm/i915/Kconfig.profile
+++ b/drivers/gpu/drm/i915/Kconfig.profile
@@ -20,6 +20,9 @@ config DRM_I915_HEARTBEAT_INTERVAL
check the health of the GPU and undertake regular house-keeping of
internal driver state.
+ This is adjustable via
+ /sys/class/drm/card?/engine/*/heartbeat_interval_ms
+
May be 0 to disable heartbeats and therefore disable automatic GPU
hang detection.
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
index 25012297aa9e..e70a0358bb6e 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
@@ -9,6 +9,7 @@
#include "i915_drv.h"
#include "intel_engine.h"
+#include "intel_engine_heartbeat.h"
#include "intel_engine_sysfs.h"
struct kobj_engine {
@@ -267,6 +268,49 @@ preempt_timeout_show(struct kobject *kobj, struct kobj_attribute *attr,
static struct kobj_attribute preempt_timeout_attr =
__ATTR(preempt_timeout_ms, 0644, preempt_timeout_show, preempt_timeout_store);
+static ssize_t
+heartbeat_store(struct kobject *kobj, struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+ unsigned long long delay;
+ int err;
+
+ /*
+ * We monitor the health of the system via periodic heartbeat pulses.
+ * The pulses also provide the opportunity to perform garbage
+ * collection. However, we interpret an incomplete pulse (a missed
+ * heartbeat) as an indication that the system is no longer responsive,
+ * i.e. hung, and perform an engine or full GPU reset. Given that the
+ * preemption granularity can be very coarse on a system, the optimal
+ * value for any workload is unknowable!
+ */
+
+ err = kstrtoull(buf, 0, &delay);
+ if (err)
+ return err;
+
+ if (delay >= jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
+ return -EINVAL;
+
+ err = intel_engine_set_heartbeat(engine, delay);
+ if (err)
+ return err;
+
+ return count;
+}
+
+static ssize_t
+heartbeat_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ struct intel_engine_cs *engine = kobj_to_engine(kobj);
+
+ return sprintf(buf, "%lu\n", engine->props.heartbeat_interval_ms);
+}
+
+static struct kobj_attribute heartbeat_interval_attr =
+__ATTR(heartbeat_interval_ms, 0644, heartbeat_show, heartbeat_store);
+
static void kobj_engine_release(struct kobject *kobj)
{
kfree(kobj);
@@ -308,6 +352,9 @@ void intel_engines_add_sysfs(struct drm_i915_private *i915)
&caps_attr.attr,
&all_caps_attr.attr,
&stop_timeout_attr.attr,
+#if CONFIG_DRM_I915_HEARTBEAT_INTERVAL
+ &heartbeat_interval_attr.attr,
+#endif
NULL
};
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 09/11] drm/i915: Flush idle barriers when waiting
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
If we do find ourselves with an idle barrier inside our active while
waiting, attempt to flush it by emitting a pulse using the kernel
context.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_active.c | 21 ++++++++-
drivers/gpu/drm/i915/selftests/i915_active.c | 46 ++++++++++++++++++++
2 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
index 207383dda84d..3e3495838a93 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -6,6 +6,7 @@
#include <linux/debugobjects.h>
+#include "gt/intel_engine_heartbeat.h"
#include "gt/intel_engine_pm.h"
#include "gt/intel_ring.h"
@@ -436,6 +437,21 @@ static void enable_signaling(struct i915_active_fence *active)
dma_fence_put(fence);
}
+static int flush_barrier(struct active_node *it)
+{
+ struct intel_engine_cs *engine;
+
+ if (!is_barrier(&it->base))
+ return 0;
+
+ engine = __barrier_to_engine(it);
+ smp_rmb(); /* serialise with add_active_barriers */
+ if (!is_barrier(&it->base))
+ return 0;
+
+ return intel_engine_flush_barriers(engine);
+}
+
int i915_active_wait(struct i915_active *ref)
{
struct active_node *it, *n;
@@ -449,8 +465,9 @@ int i915_active_wait(struct i915_active *ref)
/* Flush lazy signals */
enable_signaling(&ref->excl);
rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) {
- if (is_barrier(&it->base)) /* unconnected idle barrier */
- continue;
+ err = flush_barrier(it); /* unconnected idle barrier? */
+ if (err)
+ break;
enable_signaling(&it->base);
}
diff --git a/drivers/gpu/drm/i915/selftests/i915_active.c b/drivers/gpu/drm/i915/selftests/i915_active.c
index 96513a7d4739..aca107cb6176 100644
--- a/drivers/gpu/drm/i915/selftests/i915_active.c
+++ b/drivers/gpu/drm/i915/selftests/i915_active.c
@@ -193,11 +193,57 @@ static int live_active_retire(void *arg)
return err;
}
+static int live_active_barrier(void *arg)
+{
+ struct drm_i915_private *i915 = arg;
+ struct intel_engine_cs *engine;
+ struct live_active *active;
+ int err = 0;
+
+ /* Check that we get a callback when requests retire upon waiting */
+
+ active = __live_alloc(i915);
+ if (!active)
+ return -ENOMEM;
+
+ err = i915_active_acquire(&active->base);
+ if (err)
+ goto out;
+
+ for_each_uabi_engine(engine, i915) {
+ err = i915_active_acquire_preallocate_barrier(&active->base,
+ engine);
+ if (err)
+ break;
+
+ i915_active_acquire_barrier(&active->base);
+ }
+
+ i915_active_release(&active->base);
+
+ if (err == 0)
+ err = i915_active_wait(&active->base);
+
+ if (err == 0 && !READ_ONCE(active->retired)) {
+ pr_err("i915_active not retired after flushing barriers!\n");
+ err = -EINVAL;
+ }
+
+out:
+ __live_put(active);
+
+ if (igt_flush_test(i915))
+ err = -EIO;
+
+ return err;
+}
+
int i915_active_live_selftests(struct drm_i915_private *i915)
{
static const struct i915_subtest tests[] = {
SUBTEST(live_active_wait),
SUBTEST(live_active_retire),
+ SUBTEST(live_active_barrier),
};
if (intel_gt_is_wedged(&i915->gt))
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Intel-gfx] [PATCH 09/11] drm/i915: Flush idle barriers when waiting
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
If we do find ourselves with an idle barrier inside our active while
waiting, attempt to flush it by emitting a pulse using the kernel
context.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_active.c | 21 ++++++++-
drivers/gpu/drm/i915/selftests/i915_active.c | 46 ++++++++++++++++++++
2 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
index 207383dda84d..3e3495838a93 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -6,6 +6,7 @@
#include <linux/debugobjects.h>
+#include "gt/intel_engine_heartbeat.h"
#include "gt/intel_engine_pm.h"
#include "gt/intel_ring.h"
@@ -436,6 +437,21 @@ static void enable_signaling(struct i915_active_fence *active)
dma_fence_put(fence);
}
+static int flush_barrier(struct active_node *it)
+{
+ struct intel_engine_cs *engine;
+
+ if (!is_barrier(&it->base))
+ return 0;
+
+ engine = __barrier_to_engine(it);
+ smp_rmb(); /* serialise with add_active_barriers */
+ if (!is_barrier(&it->base))
+ return 0;
+
+ return intel_engine_flush_barriers(engine);
+}
+
int i915_active_wait(struct i915_active *ref)
{
struct active_node *it, *n;
@@ -449,8 +465,9 @@ int i915_active_wait(struct i915_active *ref)
/* Flush lazy signals */
enable_signaling(&ref->excl);
rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) {
- if (is_barrier(&it->base)) /* unconnected idle barrier */
- continue;
+ err = flush_barrier(it); /* unconnected idle barrier? */
+ if (err)
+ break;
enable_signaling(&it->base);
}
diff --git a/drivers/gpu/drm/i915/selftests/i915_active.c b/drivers/gpu/drm/i915/selftests/i915_active.c
index 96513a7d4739..aca107cb6176 100644
--- a/drivers/gpu/drm/i915/selftests/i915_active.c
+++ b/drivers/gpu/drm/i915/selftests/i915_active.c
@@ -193,11 +193,57 @@ static int live_active_retire(void *arg)
return err;
}
+static int live_active_barrier(void *arg)
+{
+ struct drm_i915_private *i915 = arg;
+ struct intel_engine_cs *engine;
+ struct live_active *active;
+ int err = 0;
+
+ /* Check that we get a callback when requests retire upon waiting */
+
+ active = __live_alloc(i915);
+ if (!active)
+ return -ENOMEM;
+
+ err = i915_active_acquire(&active->base);
+ if (err)
+ goto out;
+
+ for_each_uabi_engine(engine, i915) {
+ err = i915_active_acquire_preallocate_barrier(&active->base,
+ engine);
+ if (err)
+ break;
+
+ i915_active_acquire_barrier(&active->base);
+ }
+
+ i915_active_release(&active->base);
+
+ if (err == 0)
+ err = i915_active_wait(&active->base);
+
+ if (err == 0 && !READ_ONCE(active->retired)) {
+ pr_err("i915_active not retired after flushing barriers!\n");
+ err = -EINVAL;
+ }
+
+out:
+ __live_put(active);
+
+ if (igt_flush_test(i915))
+ err = -EIO;
+
+ return err;
+}
+
int i915_active_live_selftests(struct drm_i915_private *i915)
{
static const struct i915_subtest tests[] = {
SUBTEST(live_active_wait),
SUBTEST(live_active_retire),
+ SUBTEST(live_active_barrier),
};
if (intel_gt_is_wedged(&i915->gt))
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 10/11] drm/i915: Allow userspace to specify ringsize on construction
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
No good reason why we must always use a static ringsize, so let
userspace select one during construction.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 139 +++++++++++++++++++-
drivers/gpu/drm/i915/gt/intel_lrc.c | 1 +
include/uapi/drm/i915_drm.h | 19 +++
3 files changed, 153 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 0f1bbf5d1a11..a03bee30fac1 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -520,23 +520,30 @@ __create_context(struct drm_i915_private *i915)
return ERR_PTR(err);
}
-static void
+static int
context_apply_all(struct i915_gem_context *ctx,
- void (*fn)(struct intel_context *ce, void *data),
+ int (*fn)(struct intel_context *ce, void *data),
void *data)
{
struct i915_gem_engines_iter it;
struct intel_context *ce;
+ int err = 0;
- for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it)
- fn(ce, data);
+ for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
+ err = fn(ce, data);
+ if (err)
+ break;
+ }
i915_gem_context_unlock_engines(ctx);
+
+ return err;
}
-static void __apply_ppgtt(struct intel_context *ce, void *vm)
+static int __apply_ppgtt(struct intel_context *ce, void *vm)
{
i915_vm_put(ce->vm);
ce->vm = i915_vm_get(vm);
+ return 0;
}
static struct i915_address_space *
@@ -574,9 +581,10 @@ static void __set_timeline(struct intel_timeline **dst,
intel_timeline_put(old);
}
-static void __apply_timeline(struct intel_context *ce, void *timeline)
+static int __apply_timeline(struct intel_context *ce, void *timeline)
{
__set_timeline(&ce->timeline, timeline);
+ return 0;
}
static void __assign_timeline(struct i915_gem_context *ctx,
@@ -1151,6 +1159,104 @@ static int set_ppgtt(struct drm_i915_file_private *file_priv,
return err;
}
+static int __apply_ringsize(struct intel_context *ce, void *sz)
+{
+ int err;
+
+ err = i915_active_wait(&ce->active);
+ if (err < 0)
+ return err;
+
+ if (intel_context_lock_pinned(ce))
+ return -EINTR;
+
+ if (intel_context_is_pinned(ce)) {
+ err = -EBUSY; /* In active use, come back later! */
+ goto unlock;
+ }
+
+ if (test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) {
+ struct intel_ring *ring;
+
+ /* Replace the existing ringbuffer */
+ ring = intel_engine_create_ring(ce->engine,
+ (unsigned long)sz);
+ if (IS_ERR(ring)) {
+ err = PTR_ERR(ring);
+ goto unlock;
+ }
+
+ intel_ring_put(ce->ring);
+ ce->ring = ring;
+
+ /* Context image will be updated on next pin */
+ } else {
+ ce->ring = sz;
+ }
+
+unlock:
+ intel_context_unlock_pinned(ce);
+ return err;
+}
+
+static int set_ringsize(struct i915_gem_context *ctx,
+ struct drm_i915_gem_context_param *args)
+{
+ if (!HAS_LOGICAL_RING_CONTEXTS(ctx->i915))
+ return -ENODEV;
+
+ if (args->size)
+ return -EINVAL;
+
+ if (!IS_ALIGNED(args->value, I915_GTT_PAGE_SIZE))
+ return -EINVAL;
+
+ if (args->value < I915_GTT_PAGE_SIZE)
+ return -EINVAL;
+
+ if (args->value > 128 * I915_GTT_PAGE_SIZE)
+ return -EINVAL;
+
+ return context_apply_all(ctx,
+ __apply_ringsize,
+ __intel_context_ring_size(args->value));
+}
+
+static int __get_ringsize(struct intel_context *ce, void *arg)
+{
+ int num_pages;
+
+ if (intel_context_lock_pinned(ce))
+ return -EINTR;
+
+ if (test_bit(CONTEXT_ALLOC_BIT, &ce->flags))
+ num_pages = ce->ring->size / I915_GTT_PAGE_SIZE;
+ else
+ num_pages = (uintptr_t)ce->ring / I915_GTT_PAGE_SIZE;
+
+ intel_context_unlock_pinned(ce);
+ return num_pages; /* stop on first engine */
+}
+
+static int get_ringsize(struct i915_gem_context *ctx,
+ struct drm_i915_gem_context_param *args)
+{
+ int num_pages;
+
+ if (!HAS_LOGICAL_RING_CONTEXTS(ctx->i915))
+ return -ENODEV;
+
+ if (args->size)
+ return -EINVAL;
+
+ num_pages = context_apply_all(ctx, __get_ringsize, NULL);
+ if (num_pages < 0)
+ return num_pages;
+
+ args->value = (u64)num_pages * I915_GTT_PAGE_SIZE;
+ return 0;
+}
+
static int gen8_emit_rpcs_config(struct i915_request *rq,
struct intel_context *ce,
struct intel_sseu sseu)
@@ -1863,6 +1969,10 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv,
ret = set_persistence(ctx, args);
break;
+ case I915_CONTEXT_PARAM_RINGSIZE:
+ ret = set_ringsize(ctx, args);
+ break;
+
case I915_CONTEXT_PARAM_BAN_PERIOD:
default:
ret = -EINVAL;
@@ -1931,6 +2041,19 @@ static int clone_engines(struct i915_gem_context *dst,
__free_engines(clone, n);
goto err_unlock;
}
+
+ /* Copy across the preferred ringsize */
+ clone->engines[n]->ring = e->engines[n]->ring;
+ if (test_bit(CONTEXT_ALLOC_BIT, &e->engines[n]->flags)) {
+ if (intel_context_lock_pinned(e->engines[n])) {
+ __free_engines(clone, n + 1);
+ goto err_unlock;
+ }
+
+ clone->engines[n]->ring =
+ __intel_context_ring_size(e->engines[n]->ring->size);
+ intel_context_unlock_pinned(e->engines[n]);
+ }
}
clone->num_engines = n;
@@ -2320,6 +2443,10 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
args->value = i915_gem_context_is_persistent(ctx);
break;
+ case I915_CONTEXT_PARAM_RINGSIZE:
+ ret = get_ringsize(ctx, args);
+ break;
+
case I915_CONTEXT_PARAM_BAN_PERIOD:
default:
ret = -EINVAL;
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index a7755d6dc48b..9d59debfd168 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -2309,6 +2309,7 @@ __execlists_update_reg_state(const struct intel_context *ce,
GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail));
regs[CTX_RING_BUFFER_START] = i915_ggtt_offset(ring->vma);
+ regs[CTX_RING_BUFFER_CONTROL] = RING_CTL_SIZE(ring->size) | RING_VALID;
regs[CTX_RING_HEAD] = ring->head;
regs[CTX_RING_TAIL] = ring->tail;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index b5fd5e4bd16e..cd35036a26a2 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1587,6 +1587,25 @@ struct drm_i915_gem_context_param {
* By default, new contexts allow persistence.
*/
#define I915_CONTEXT_PARAM_PERSISTENCE 0xb
+
+/*
+ * I915_CONTEXT_PARAM_RINGSIZE:
+ *
+ * Sets the size of the CS ringbuffer to use for logical ring contexts. This
+ * applies a limit of how many batches can be queued to HW before the caller
+ * is blocked due to lack of space for more commands.
+ *
+ * Only reliably possible to be set prior to first use, i.e. during
+ * construction. At any later point, the current execution must be flushed as
+ * the ring can only be changed while the context is idle.
+ *
+ * Only applies to the current set of engine and lost when those engines
+ * are replaced by a new mapping (see I915_CONTEXT_PARAM_ENGINES).
+ *
+ * Must be between 4 - 512 KiB, in intervals of page size [4 KiB].
+ * Default is 16 KiB.
+ */
+#define I915_CONTEXT_PARAM_RINGSIZE 0xc
/* Must be kept compact -- no holes and well documented */
__u64 value;
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Intel-gfx] [PATCH 10/11] drm/i915: Allow userspace to specify ringsize on construction
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
No good reason why we must always use a static ringsize, so let
userspace select one during construction.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 139 +++++++++++++++++++-
drivers/gpu/drm/i915/gt/intel_lrc.c | 1 +
include/uapi/drm/i915_drm.h | 19 +++
3 files changed, 153 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 0f1bbf5d1a11..a03bee30fac1 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -520,23 +520,30 @@ __create_context(struct drm_i915_private *i915)
return ERR_PTR(err);
}
-static void
+static int
context_apply_all(struct i915_gem_context *ctx,
- void (*fn)(struct intel_context *ce, void *data),
+ int (*fn)(struct intel_context *ce, void *data),
void *data)
{
struct i915_gem_engines_iter it;
struct intel_context *ce;
+ int err = 0;
- for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it)
- fn(ce, data);
+ for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
+ err = fn(ce, data);
+ if (err)
+ break;
+ }
i915_gem_context_unlock_engines(ctx);
+
+ return err;
}
-static void __apply_ppgtt(struct intel_context *ce, void *vm)
+static int __apply_ppgtt(struct intel_context *ce, void *vm)
{
i915_vm_put(ce->vm);
ce->vm = i915_vm_get(vm);
+ return 0;
}
static struct i915_address_space *
@@ -574,9 +581,10 @@ static void __set_timeline(struct intel_timeline **dst,
intel_timeline_put(old);
}
-static void __apply_timeline(struct intel_context *ce, void *timeline)
+static int __apply_timeline(struct intel_context *ce, void *timeline)
{
__set_timeline(&ce->timeline, timeline);
+ return 0;
}
static void __assign_timeline(struct i915_gem_context *ctx,
@@ -1151,6 +1159,104 @@ static int set_ppgtt(struct drm_i915_file_private *file_priv,
return err;
}
+static int __apply_ringsize(struct intel_context *ce, void *sz)
+{
+ int err;
+
+ err = i915_active_wait(&ce->active);
+ if (err < 0)
+ return err;
+
+ if (intel_context_lock_pinned(ce))
+ return -EINTR;
+
+ if (intel_context_is_pinned(ce)) {
+ err = -EBUSY; /* In active use, come back later! */
+ goto unlock;
+ }
+
+ if (test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) {
+ struct intel_ring *ring;
+
+ /* Replace the existing ringbuffer */
+ ring = intel_engine_create_ring(ce->engine,
+ (unsigned long)sz);
+ if (IS_ERR(ring)) {
+ err = PTR_ERR(ring);
+ goto unlock;
+ }
+
+ intel_ring_put(ce->ring);
+ ce->ring = ring;
+
+ /* Context image will be updated on next pin */
+ } else {
+ ce->ring = sz;
+ }
+
+unlock:
+ intel_context_unlock_pinned(ce);
+ return err;
+}
+
+static int set_ringsize(struct i915_gem_context *ctx,
+ struct drm_i915_gem_context_param *args)
+{
+ if (!HAS_LOGICAL_RING_CONTEXTS(ctx->i915))
+ return -ENODEV;
+
+ if (args->size)
+ return -EINVAL;
+
+ if (!IS_ALIGNED(args->value, I915_GTT_PAGE_SIZE))
+ return -EINVAL;
+
+ if (args->value < I915_GTT_PAGE_SIZE)
+ return -EINVAL;
+
+ if (args->value > 128 * I915_GTT_PAGE_SIZE)
+ return -EINVAL;
+
+ return context_apply_all(ctx,
+ __apply_ringsize,
+ __intel_context_ring_size(args->value));
+}
+
+static int __get_ringsize(struct intel_context *ce, void *arg)
+{
+ int num_pages;
+
+ if (intel_context_lock_pinned(ce))
+ return -EINTR;
+
+ if (test_bit(CONTEXT_ALLOC_BIT, &ce->flags))
+ num_pages = ce->ring->size / I915_GTT_PAGE_SIZE;
+ else
+ num_pages = (uintptr_t)ce->ring / I915_GTT_PAGE_SIZE;
+
+ intel_context_unlock_pinned(ce);
+ return num_pages; /* stop on first engine */
+}
+
+static int get_ringsize(struct i915_gem_context *ctx,
+ struct drm_i915_gem_context_param *args)
+{
+ int num_pages;
+
+ if (!HAS_LOGICAL_RING_CONTEXTS(ctx->i915))
+ return -ENODEV;
+
+ if (args->size)
+ return -EINVAL;
+
+ num_pages = context_apply_all(ctx, __get_ringsize, NULL);
+ if (num_pages < 0)
+ return num_pages;
+
+ args->value = (u64)num_pages * I915_GTT_PAGE_SIZE;
+ return 0;
+}
+
static int gen8_emit_rpcs_config(struct i915_request *rq,
struct intel_context *ce,
struct intel_sseu sseu)
@@ -1863,6 +1969,10 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv,
ret = set_persistence(ctx, args);
break;
+ case I915_CONTEXT_PARAM_RINGSIZE:
+ ret = set_ringsize(ctx, args);
+ break;
+
case I915_CONTEXT_PARAM_BAN_PERIOD:
default:
ret = -EINVAL;
@@ -1931,6 +2041,19 @@ static int clone_engines(struct i915_gem_context *dst,
__free_engines(clone, n);
goto err_unlock;
}
+
+ /* Copy across the preferred ringsize */
+ clone->engines[n]->ring = e->engines[n]->ring;
+ if (test_bit(CONTEXT_ALLOC_BIT, &e->engines[n]->flags)) {
+ if (intel_context_lock_pinned(e->engines[n])) {
+ __free_engines(clone, n + 1);
+ goto err_unlock;
+ }
+
+ clone->engines[n]->ring =
+ __intel_context_ring_size(e->engines[n]->ring->size);
+ intel_context_unlock_pinned(e->engines[n]);
+ }
}
clone->num_engines = n;
@@ -2320,6 +2443,10 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
args->value = i915_gem_context_is_persistent(ctx);
break;
+ case I915_CONTEXT_PARAM_RINGSIZE:
+ ret = get_ringsize(ctx, args);
+ break;
+
case I915_CONTEXT_PARAM_BAN_PERIOD:
default:
ret = -EINVAL;
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index a7755d6dc48b..9d59debfd168 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -2309,6 +2309,7 @@ __execlists_update_reg_state(const struct intel_context *ce,
GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail));
regs[CTX_RING_BUFFER_START] = i915_ggtt_offset(ring->vma);
+ regs[CTX_RING_BUFFER_CONTROL] = RING_CTL_SIZE(ring->size) | RING_VALID;
regs[CTX_RING_HEAD] = ring->head;
regs[CTX_RING_TAIL] = ring->tail;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index b5fd5e4bd16e..cd35036a26a2 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1587,6 +1587,25 @@ struct drm_i915_gem_context_param {
* By default, new contexts allow persistence.
*/
#define I915_CONTEXT_PARAM_PERSISTENCE 0xb
+
+/*
+ * I915_CONTEXT_PARAM_RINGSIZE:
+ *
+ * Sets the size of the CS ringbuffer to use for logical ring contexts. This
+ * applies a limit of how many batches can be queued to HW before the caller
+ * is blocked due to lack of space for more commands.
+ *
+ * Only reliably possible to be set prior to first use, i.e. during
+ * construction. At any later point, the current execution must be flushed as
+ * the ring can only be changed while the context is idle.
+ *
+ * Only applies to the current set of engine and lost when those engines
+ * are replaced by a new mapping (see I915_CONTEXT_PARAM_ENGINES).
+ *
+ * Must be between 4 - 512 KiB, in intervals of page size [4 KiB].
+ * Default is 16 KiB.
+ */
+#define I915_CONTEXT_PARAM_RINGSIZE 0xc
/* Must be kept compact -- no holes and well documented */
__u64 value;
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 11/11] drm/i915/gem: Honour O_NONBLOCK before throttling execbuf submissions
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
Check the user's flags on the struct file before deciding whether or not
to stall before submitting a request. This allows us to reasonably
cheaply honour O_NONBLOCK without checking at more critical phases
during request submission.
Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c | 21 ++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index e4f5c269150a..c88948e4094b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -2193,15 +2193,22 @@ static int __eb_pin_engine(struct i915_execbuffer *eb, struct intel_context *ce)
intel_context_timeline_unlock(tl);
if (rq) {
- if (i915_request_wait(rq,
- I915_WAIT_INTERRUPTIBLE,
- MAX_SCHEDULE_TIMEOUT) < 0) {
- i915_request_put(rq);
- err = -EINTR;
- goto err_exit;
- }
+ bool nonblock = eb->file->filp->f_flags & O_NONBLOCK;
+ long timeout;
+
+ timeout = MAX_SCHEDULE_TIMEOUT;
+ if (nonblock)
+ timeout = 0;
+ timeout = i915_request_wait(rq,
+ I915_WAIT_INTERRUPTIBLE,
+ timeout);
i915_request_put(rq);
+
+ if (timeout < 0) {
+ err = nonblock ? -EWOULDBLOCK : timeout;
+ goto err_exit;
+ }
}
eb->engine = ce->engine;
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Intel-gfx] [PATCH 11/11] drm/i915/gem: Honour O_NONBLOCK before throttling execbuf submissions
@ 2019-10-24 11:40 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-24 11:40 UTC (permalink / raw)
To: intel-gfx
Check the user's flags on the struct file before deciding whether or not
to stall before submitting a request. This allows us to reasonably
cheaply honour O_NONBLOCK without checking at more critical phases
during request submission.
Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c | 21 ++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index e4f5c269150a..c88948e4094b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -2193,15 +2193,22 @@ static int __eb_pin_engine(struct i915_execbuffer *eb, struct intel_context *ce)
intel_context_timeline_unlock(tl);
if (rq) {
- if (i915_request_wait(rq,
- I915_WAIT_INTERRUPTIBLE,
- MAX_SCHEDULE_TIMEOUT) < 0) {
- i915_request_put(rq);
- err = -EINTR;
- goto err_exit;
- }
+ bool nonblock = eb->file->filp->f_flags & O_NONBLOCK;
+ long timeout;
+
+ timeout = MAX_SCHEDULE_TIMEOUT;
+ if (nonblock)
+ timeout = 0;
+ timeout = i915_request_wait(rq,
+ I915_WAIT_INTERRUPTIBLE,
+ timeout);
i915_request_put(rq);
+
+ if (timeout < 0) {
+ err = nonblock ? -EWOULDBLOCK : timeout;
+ goto err_exit;
+ }
}
eb->engine = ce->engine;
--
2.24.0.rc0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH 02/11] drm/i915: Put future HW and their uAPIs under STAGING & BROKEN
@ 2019-10-24 23:25 ` David Airlie
0 siblings, 0 replies; 40+ messages in thread
From: David Airlie @ 2019-10-24 23:25 UTC (permalink / raw)
To: Chris Wilson; +Cc: Jani Nikula, Daniel Vetter, Development, Intel
Acked-by: Dave Airlie <airlied@redhat.com>
On Thu, Oct 24, 2019 at 9:40 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> We would like some freedom to break the user API/ABI for future HW but
> yet still expose the driver for upstream development on that HW.
> Currently, we have the i915.force_probe module parameter to avoid binding
> to HW while the driver is under development, but that is still a little
> too soft with respect to the stringent no-regression rules if we also
> plan to be redesigning the uAPI to go along with the new HW.
>
> To allow the uAPI to be changed during development, only expose that API
> and in development HW under STAGING (and BROKEN). Hopefully, making it
> explicit that such interfaces to that HW are under development and not
> to be blindly enabled by distributions.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
> ---
> drivers/gpu/drm/i915/Kconfig | 8 ++++++++
> drivers/gpu/drm/i915/Kconfig.debug | 1 +
> drivers/gpu/drm/i915/Kconfig.unstable | 20 ++++++++++++++++++++
> 3 files changed, 29 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/Kconfig.unstable
>
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 3c6d57df262d..1fd9e665b742 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -148,3 +148,11 @@ menu "drm/i915 Profile Guided Optimisation"
> depends on DRM_I915
> source "drivers/gpu/drm/i915/Kconfig.profile"
> endmenu
> +
> +menu "drm/i915 Ustable Evolution"
> + visible if EXPERT
> + visible if STAGING
> + visible if BROKEN
> + depends on DRM_I915
> + source "drivers/gpu/drm/i915/Kconfig.unstable"
> +endmenu
> diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
> index d2ba8f7e5e50..ef123eb29168 100644
> --- a/drivers/gpu/drm/i915/Kconfig.debug
> +++ b/drivers/gpu/drm/i915/Kconfig.debug
> @@ -44,6 +44,7 @@ config DRM_I915_DEBUG
> select DRM_I915_SELFTEST
> select DRM_I915_DEBUG_RUNTIME_PM
> select DRM_I915_DEBUG_MMIO
> + select BROKEN # for prototype uAPI
> default n
> help
> Choose this option to turn on extra driver debugging that may affect
> diff --git a/drivers/gpu/drm/i915/Kconfig.unstable b/drivers/gpu/drm/i915/Kconfig.unstable
> new file mode 100644
> index 000000000000..ecc8458b5a32
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/Kconfig.unstable
> @@ -0,0 +1,20 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config DRM_I915_UNSTABLE
> + bool "Enable unstable API for early prototype development"
> + depends on EXPERT
> + depends on STAGING
> + depends on BROKEN # should never be enabled by distros!
> + # We use the dependency on !COMPILE_TEST to not be enabled in
> + # allmodconfig or allyesconfig configurations
> + depends on !COMPILE_TEST
> + default n
> + help
> + Enable prototype uAPI under general discussion before they are
> + finalized. Such prototypes may be withdrawn or substantially
> + changed before release. They are only enabled here so that a wide
> + number of interested parties (userspace driver developers) can
> + verify that the uAPI meet their expectations.
> +
> + Recommended for driver developers _only_.
> +
> + If in the slightest bit of doubt, say "N".
> --
> 2.24.0.rc0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Intel-gfx] [PATCH 02/11] drm/i915: Put future HW and their uAPIs under STAGING & BROKEN
@ 2019-10-24 23:25 ` David Airlie
0 siblings, 0 replies; 40+ messages in thread
From: David Airlie @ 2019-10-24 23:25 UTC (permalink / raw)
To: Chris Wilson; +Cc: Jani Nikula, Daniel Vetter, Development, Intel
Acked-by: Dave Airlie <airlied@redhat.com>
On Thu, Oct 24, 2019 at 9:40 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> We would like some freedom to break the user API/ABI for future HW but
> yet still expose the driver for upstream development on that HW.
> Currently, we have the i915.force_probe module parameter to avoid binding
> to HW while the driver is under development, but that is still a little
> too soft with respect to the stringent no-regression rules if we also
> plan to be redesigning the uAPI to go along with the new HW.
>
> To allow the uAPI to be changed during development, only expose that API
> and in development HW under STAGING (and BROKEN). Hopefully, making it
> explicit that such interfaces to that HW are under development and not
> to be blindly enabled by distributions.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
> ---
> drivers/gpu/drm/i915/Kconfig | 8 ++++++++
> drivers/gpu/drm/i915/Kconfig.debug | 1 +
> drivers/gpu/drm/i915/Kconfig.unstable | 20 ++++++++++++++++++++
> 3 files changed, 29 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/Kconfig.unstable
>
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 3c6d57df262d..1fd9e665b742 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -148,3 +148,11 @@ menu "drm/i915 Profile Guided Optimisation"
> depends on DRM_I915
> source "drivers/gpu/drm/i915/Kconfig.profile"
> endmenu
> +
> +menu "drm/i915 Ustable Evolution"
> + visible if EXPERT
> + visible if STAGING
> + visible if BROKEN
> + depends on DRM_I915
> + source "drivers/gpu/drm/i915/Kconfig.unstable"
> +endmenu
> diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
> index d2ba8f7e5e50..ef123eb29168 100644
> --- a/drivers/gpu/drm/i915/Kconfig.debug
> +++ b/drivers/gpu/drm/i915/Kconfig.debug
> @@ -44,6 +44,7 @@ config DRM_I915_DEBUG
> select DRM_I915_SELFTEST
> select DRM_I915_DEBUG_RUNTIME_PM
> select DRM_I915_DEBUG_MMIO
> + select BROKEN # for prototype uAPI
> default n
> help
> Choose this option to turn on extra driver debugging that may affect
> diff --git a/drivers/gpu/drm/i915/Kconfig.unstable b/drivers/gpu/drm/i915/Kconfig.unstable
> new file mode 100644
> index 000000000000..ecc8458b5a32
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/Kconfig.unstable
> @@ -0,0 +1,20 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config DRM_I915_UNSTABLE
> + bool "Enable unstable API for early prototype development"
> + depends on EXPERT
> + depends on STAGING
> + depends on BROKEN # should never be enabled by distros!
> + # We use the dependency on !COMPILE_TEST to not be enabled in
> + # allmodconfig or allyesconfig configurations
> + depends on !COMPILE_TEST
> + default n
> + help
> + Enable prototype uAPI under general discussion before they are
> + finalized. Such prototypes may be withdrawn or substantially
> + changed before release. They are only enabled here so that a wide
> + number of interested parties (userspace driver developers) can
> + verify that the uAPI meet their expectations.
> +
> + Recommended for driver developers _only_.
> +
> + If in the slightest bit of doubt, say "N".
> --
> 2.24.0.rc0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 02/11] drm/i915: Put future HW and their uAPIs under STAGING & BROKEN
@ 2019-10-24 23:35 ` Rodrigo Vivi
0 siblings, 0 replies; 40+ messages in thread
From: Rodrigo Vivi @ 2019-10-24 23:35 UTC (permalink / raw)
To: Chris Wilson; +Cc: Jani Nikula, Daniel Vetter, intel-gfx, Dave Airlie
On Thu, Oct 24, 2019 at 12:40:19PM +0100, Chris Wilson wrote:
> We would like some freedom to break the user API/ABI for future HW but
> yet still expose the driver for upstream development on that HW.
> Currently, we have the i915.force_probe module parameter to avoid binding
> to HW while the driver is under development, but that is still a little
> too soft with respect to the stringent no-regression rules if we also
> plan to be redesigning the uAPI to go along with the new HW.
>
> To allow the uAPI to be changed during development, only expose that API
> and in development HW under STAGING (and BROKEN). Hopefully, making it
> explicit that such interfaces to that HW are under development and not
> to be blindly enabled by distributions.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/i915/Kconfig | 8 ++++++++
> drivers/gpu/drm/i915/Kconfig.debug | 1 +
> drivers/gpu/drm/i915/Kconfig.unstable | 20 ++++++++++++++++++++
> 3 files changed, 29 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/Kconfig.unstable
>
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 3c6d57df262d..1fd9e665b742 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -148,3 +148,11 @@ menu "drm/i915 Profile Guided Optimisation"
> depends on DRM_I915
> source "drivers/gpu/drm/i915/Kconfig.profile"
> endmenu
> +
> +menu "drm/i915 Ustable Evolution"
> + visible if EXPERT
> + visible if STAGING
> + visible if BROKEN
> + depends on DRM_I915
> + source "drivers/gpu/drm/i915/Kconfig.unstable"
> +endmenu
> diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
> index d2ba8f7e5e50..ef123eb29168 100644
> --- a/drivers/gpu/drm/i915/Kconfig.debug
> +++ b/drivers/gpu/drm/i915/Kconfig.debug
> @@ -44,6 +44,7 @@ config DRM_I915_DEBUG
> select DRM_I915_SELFTEST
> select DRM_I915_DEBUG_RUNTIME_PM
> select DRM_I915_DEBUG_MMIO
> + select BROKEN # for prototype uAPI
> default n
> help
> Choose this option to turn on extra driver debugging that may affect
> diff --git a/drivers/gpu/drm/i915/Kconfig.unstable b/drivers/gpu/drm/i915/Kconfig.unstable
> new file mode 100644
> index 000000000000..ecc8458b5a32
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/Kconfig.unstable
> @@ -0,0 +1,20 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config DRM_I915_UNSTABLE
> + bool "Enable unstable API for early prototype development"
> + depends on EXPERT
> + depends on STAGING
> + depends on BROKEN # should never be enabled by distros!
> + # We use the dependency on !COMPILE_TEST to not be enabled in
> + # allmodconfig or allyesconfig configurations
> + depends on !COMPILE_TEST
> + default n
> + help
> + Enable prototype uAPI under general discussion before they are
> + finalized. Such prototypes may be withdrawn or substantially
> + changed before release. They are only enabled here so that a wide
> + number of interested parties (userspace driver developers) can
> + verify that the uAPI meet their expectations.
> +
> + Recommended for driver developers _only_.
> +
> + If in the slightest bit of doubt, say "N".
> --
> 2.24.0.rc0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Intel-gfx] [PATCH 02/11] drm/i915: Put future HW and their uAPIs under STAGING & BROKEN
@ 2019-10-24 23:35 ` Rodrigo Vivi
0 siblings, 0 replies; 40+ messages in thread
From: Rodrigo Vivi @ 2019-10-24 23:35 UTC (permalink / raw)
To: Chris Wilson; +Cc: Jani Nikula, Daniel Vetter, intel-gfx, Dave Airlie
On Thu, Oct 24, 2019 at 12:40:19PM +0100, Chris Wilson wrote:
> We would like some freedom to break the user API/ABI for future HW but
> yet still expose the driver for upstream development on that HW.
> Currently, we have the i915.force_probe module parameter to avoid binding
> to HW while the driver is under development, but that is still a little
> too soft with respect to the stringent no-regression rules if we also
> plan to be redesigning the uAPI to go along with the new HW.
>
> To allow the uAPI to be changed during development, only expose that API
> and in development HW under STAGING (and BROKEN). Hopefully, making it
> explicit that such interfaces to that HW are under development and not
> to be blindly enabled by distributions.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/i915/Kconfig | 8 ++++++++
> drivers/gpu/drm/i915/Kconfig.debug | 1 +
> drivers/gpu/drm/i915/Kconfig.unstable | 20 ++++++++++++++++++++
> 3 files changed, 29 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/Kconfig.unstable
>
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 3c6d57df262d..1fd9e665b742 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -148,3 +148,11 @@ menu "drm/i915 Profile Guided Optimisation"
> depends on DRM_I915
> source "drivers/gpu/drm/i915/Kconfig.profile"
> endmenu
> +
> +menu "drm/i915 Ustable Evolution"
> + visible if EXPERT
> + visible if STAGING
> + visible if BROKEN
> + depends on DRM_I915
> + source "drivers/gpu/drm/i915/Kconfig.unstable"
> +endmenu
> diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
> index d2ba8f7e5e50..ef123eb29168 100644
> --- a/drivers/gpu/drm/i915/Kconfig.debug
> +++ b/drivers/gpu/drm/i915/Kconfig.debug
> @@ -44,6 +44,7 @@ config DRM_I915_DEBUG
> select DRM_I915_SELFTEST
> select DRM_I915_DEBUG_RUNTIME_PM
> select DRM_I915_DEBUG_MMIO
> + select BROKEN # for prototype uAPI
> default n
> help
> Choose this option to turn on extra driver debugging that may affect
> diff --git a/drivers/gpu/drm/i915/Kconfig.unstable b/drivers/gpu/drm/i915/Kconfig.unstable
> new file mode 100644
> index 000000000000..ecc8458b5a32
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/Kconfig.unstable
> @@ -0,0 +1,20 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config DRM_I915_UNSTABLE
> + bool "Enable unstable API for early prototype development"
> + depends on EXPERT
> + depends on STAGING
> + depends on BROKEN # should never be enabled by distros!
> + # We use the dependency on !COMPILE_TEST to not be enabled in
> + # allmodconfig or allyesconfig configurations
> + depends on !COMPILE_TEST
> + default n
> + help
> + Enable prototype uAPI under general discussion before they are
> + finalized. Such prototypes may be withdrawn or substantially
> + changed before release. They are only enabled here so that a wide
> + number of interested parties (userspace driver developers) can
> + verify that the uAPI meet their expectations.
> +
> + Recommended for driver developers _only_.
> +
> + If in the slightest bit of doubt, say "N".
> --
> 2.24.0.rc0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 02/11] drm/i915: Put future HW and their uAPIs under STAGING & BROKEN
@ 2019-10-25 7:09 ` Jani Nikula
0 siblings, 0 replies; 40+ messages in thread
From: Jani Nikula @ 2019-10-25 7:09 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Daniel Vetter, Dave Airlie
On Thu, 24 Oct 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> We would like some freedom to break the user API/ABI for future HW but
> yet still expose the driver for upstream development on that HW.
> Currently, we have the i915.force_probe module parameter to avoid binding
> to HW while the driver is under development, but that is still a little
> too soft with respect to the stringent no-regression rules if we also
> plan to be redesigning the uAPI to go along with the new HW.
>
> To allow the uAPI to be changed during development, only expose that API
> and in development HW under STAGING (and BROKEN). Hopefully, making it
> explicit that such interfaces to that HW are under development and not
> to be blindly enabled by distributions.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
> ---
> drivers/gpu/drm/i915/Kconfig | 8 ++++++++
> drivers/gpu/drm/i915/Kconfig.debug | 1 +
> drivers/gpu/drm/i915/Kconfig.unstable | 20 ++++++++++++++++++++
> 3 files changed, 29 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/Kconfig.unstable
>
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 3c6d57df262d..1fd9e665b742 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -148,3 +148,11 @@ menu "drm/i915 Profile Guided Optimisation"
> depends on DRM_I915
> source "drivers/gpu/drm/i915/Kconfig.profile"
> endmenu
> +
> +menu "drm/i915 Ustable Evolution"
> + visible if EXPERT
> + visible if STAGING
> + visible if BROKEN
The kconfig docs fail to mention whether multiple "visible if"
statements are combined with OR or AND. The kernel tree lacks any prior
art.
How about
visible if EXPERT && STAGING && BROKEN
which should be fine and apparently matches your intent?
No biggie though, it's the actually config option that matters.
FWIW, this is
Acked-by: Jani Nikula <jani.nikula@intel.com>
though it's Dave's ack that really matters here.
BR,
Jani.
> + depends on DRM_I915
> + source "drivers/gpu/drm/i915/Kconfig.unstable"
> +endmenu
> diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
> index d2ba8f7e5e50..ef123eb29168 100644
> --- a/drivers/gpu/drm/i915/Kconfig.debug
> +++ b/drivers/gpu/drm/i915/Kconfig.debug
> @@ -44,6 +44,7 @@ config DRM_I915_DEBUG
> select DRM_I915_SELFTEST
> select DRM_I915_DEBUG_RUNTIME_PM
> select DRM_I915_DEBUG_MMIO
> + select BROKEN # for prototype uAPI
> default n
> help
> Choose this option to turn on extra driver debugging that may affect
> diff --git a/drivers/gpu/drm/i915/Kconfig.unstable b/drivers/gpu/drm/i915/Kconfig.unstable
> new file mode 100644
> index 000000000000..ecc8458b5a32
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/Kconfig.unstable
> @@ -0,0 +1,20 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config DRM_I915_UNSTABLE
> + bool "Enable unstable API for early prototype development"
> + depends on EXPERT
> + depends on STAGING
> + depends on BROKEN # should never be enabled by distros!
> + # We use the dependency on !COMPILE_TEST to not be enabled in
> + # allmodconfig or allyesconfig configurations
> + depends on !COMPILE_TEST
> + default n
> + help
> + Enable prototype uAPI under general discussion before they are
> + finalized. Such prototypes may be withdrawn or substantially
> + changed before release. They are only enabled here so that a wide
> + number of interested parties (userspace driver developers) can
> + verify that the uAPI meet their expectations.
> +
> + Recommended for driver developers _only_.
> +
> + If in the slightest bit of doubt, say "N".
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Intel-gfx] [PATCH 02/11] drm/i915: Put future HW and their uAPIs under STAGING & BROKEN
@ 2019-10-25 7:09 ` Jani Nikula
0 siblings, 0 replies; 40+ messages in thread
From: Jani Nikula @ 2019-10-25 7:09 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Daniel Vetter, Dave Airlie
On Thu, 24 Oct 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> We would like some freedom to break the user API/ABI for future HW but
> yet still expose the driver for upstream development on that HW.
> Currently, we have the i915.force_probe module parameter to avoid binding
> to HW while the driver is under development, but that is still a little
> too soft with respect to the stringent no-regression rules if we also
> plan to be redesigning the uAPI to go along with the new HW.
>
> To allow the uAPI to be changed during development, only expose that API
> and in development HW under STAGING (and BROKEN). Hopefully, making it
> explicit that such interfaces to that HW are under development and not
> to be blindly enabled by distributions.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
> ---
> drivers/gpu/drm/i915/Kconfig | 8 ++++++++
> drivers/gpu/drm/i915/Kconfig.debug | 1 +
> drivers/gpu/drm/i915/Kconfig.unstable | 20 ++++++++++++++++++++
> 3 files changed, 29 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/Kconfig.unstable
>
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 3c6d57df262d..1fd9e665b742 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -148,3 +148,11 @@ menu "drm/i915 Profile Guided Optimisation"
> depends on DRM_I915
> source "drivers/gpu/drm/i915/Kconfig.profile"
> endmenu
> +
> +menu "drm/i915 Ustable Evolution"
> + visible if EXPERT
> + visible if STAGING
> + visible if BROKEN
The kconfig docs fail to mention whether multiple "visible if"
statements are combined with OR or AND. The kernel tree lacks any prior
art.
How about
visible if EXPERT && STAGING && BROKEN
which should be fine and apparently matches your intent?
No biggie though, it's the actually config option that matters.
FWIW, this is
Acked-by: Jani Nikula <jani.nikula@intel.com>
though it's Dave's ack that really matters here.
BR,
Jani.
> + depends on DRM_I915
> + source "drivers/gpu/drm/i915/Kconfig.unstable"
> +endmenu
> diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
> index d2ba8f7e5e50..ef123eb29168 100644
> --- a/drivers/gpu/drm/i915/Kconfig.debug
> +++ b/drivers/gpu/drm/i915/Kconfig.debug
> @@ -44,6 +44,7 @@ config DRM_I915_DEBUG
> select DRM_I915_SELFTEST
> select DRM_I915_DEBUG_RUNTIME_PM
> select DRM_I915_DEBUG_MMIO
> + select BROKEN # for prototype uAPI
> default n
> help
> Choose this option to turn on extra driver debugging that may affect
> diff --git a/drivers/gpu/drm/i915/Kconfig.unstable b/drivers/gpu/drm/i915/Kconfig.unstable
> new file mode 100644
> index 000000000000..ecc8458b5a32
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/Kconfig.unstable
> @@ -0,0 +1,20 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config DRM_I915_UNSTABLE
> + bool "Enable unstable API for early prototype development"
> + depends on EXPERT
> + depends on STAGING
> + depends on BROKEN # should never be enabled by distros!
> + # We use the dependency on !COMPILE_TEST to not be enabled in
> + # allmodconfig or allyesconfig configurations
> + depends on !COMPILE_TEST
> + default n
> + help
> + Enable prototype uAPI under general discussion before they are
> + finalized. Such prototypes may be withdrawn or substantially
> + changed before release. They are only enabled here so that a wide
> + number of interested parties (userspace driver developers) can
> + verify that the uAPI meet their expectations.
> +
> + Recommended for driver developers _only_.
> +
> + If in the slightest bit of doubt, say "N".
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 01/11] drm/i915/gem: Make context persistence optional
@ 2019-10-25 9:10 ` Joonas Lahtinen
0 siblings, 0 replies; 40+ messages in thread
From: Joonas Lahtinen @ 2019-10-25 9:10 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Quoting Chris Wilson (2019-10-24 14:40:18)
> Our existing behaviour is to allow contexts and their GPU requests to
> persist past the point of closure until the requests are complete. This
> allows clients to operate in a 'fire-and-forget' manner where they can
> setup a rendering pipeline and hand it over to the display server and
> immediately exiting. As the rendering pipeline is kept alive until
> completion, the display server (or other consumer) can use the results
> in the future and present them to the user.
>
> However, not all clients want this persistent behaviour and would prefer
> that the contexts are cleaned up immediately upon closure. This ensures
> that when clients are run without hangchecking, any GPU hang is
> terminated with the process and does not continue to hog resources.
It's worth mentioning GPU compute workloads of indeterminate length
as an example for increased clarity.
> By defining a context property to allow clients to control persistence
> explicitly, we can remove the blanket advice to disable hangchecking
> that seems to be far too prevalent.
I would drop this paragraph from this patch, as it doesn't (yet) make
sense.
> The default behaviour for new controls is the legacy persistence mode.
> New clients will have to opt out for immediate cleanup on context
> closure.
"... have to opt in to immediate ..." reads more clear.
> If the hangchecking modparam is disabled, so is persistent
> context support -- all contexts will be terminated on closure.
Let's add here that it has actually been a source of bug reports
in the past that we don't terminate the workoads. So we expect
this behaviour change to be welcomed by the compute users who
have been instructed to disable the hanghceck in the past.
A couple of comments below. But anyway, with the uAPI comment
and commit message clarified this is:
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Still needs the Link:, though.
> Testcase: igt/gem_ctx_persistence
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
<SNIP>
> +static int __context_set_persistence(struct i915_gem_context *ctx, bool state)
> +{
> + if (i915_gem_context_is_persistent(ctx) == state)
> + return 0;
> +
> + if (state) {
> + /*
> + * Only contexts that are short-lived [that will expire or be
> + * reset] are allowed to survive past termination. We require
> + * hangcheck to ensure that the persistent requests are healthy.
> + */
> + if (!i915_modparams.enable_hangcheck)
> + return -EINVAL;
This is slightly confusing as the default is to enable persistence.
Disabling and re-enabling would result -EINVAL. But I guess it's no
problem to lift such restriction later.
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1572,6 +1572,21 @@ struct drm_i915_gem_context_param {
> * i915_context_engines_bond (I915_CONTEXT_ENGINES_EXT_BOND)
> */
> #define I915_CONTEXT_PARAM_ENGINES 0xa
> +
> +/*
> + * I915_CONTEXT_PARAM_PERSISTENCE:
> + *
> + * Allow the context and active rendering to survive the process until
> + * completion. Persistence allows fire-and-forget clients to queue up a
> + * bunch of work, hand the output over to a display server and the quit.
> + * If the context is not marked as persistent, upon closing (either via
".. is marked as not persistent," is more clear.
Regards, Joonas
> + * an explicit DRM_I915_GEM_CONTEXT_DESTROY or implicitly from file closure
> + * or process termination), the context and any outstanding requests will be
> + * cancelled (and exported fences for cancelled requests marked as -EIO).
> + *
> + * By default, new contexts allow persistence.
> + */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Intel-gfx] [PATCH 01/11] drm/i915/gem: Make context persistence optional
@ 2019-10-25 9:10 ` Joonas Lahtinen
0 siblings, 0 replies; 40+ messages in thread
From: Joonas Lahtinen @ 2019-10-25 9:10 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Quoting Chris Wilson (2019-10-24 14:40:18)
> Our existing behaviour is to allow contexts and their GPU requests to
> persist past the point of closure until the requests are complete. This
> allows clients to operate in a 'fire-and-forget' manner where they can
> setup a rendering pipeline and hand it over to the display server and
> immediately exiting. As the rendering pipeline is kept alive until
> completion, the display server (or other consumer) can use the results
> in the future and present them to the user.
>
> However, not all clients want this persistent behaviour and would prefer
> that the contexts are cleaned up immediately upon closure. This ensures
> that when clients are run without hangchecking, any GPU hang is
> terminated with the process and does not continue to hog resources.
It's worth mentioning GPU compute workloads of indeterminate length
as an example for increased clarity.
> By defining a context property to allow clients to control persistence
> explicitly, we can remove the blanket advice to disable hangchecking
> that seems to be far too prevalent.
I would drop this paragraph from this patch, as it doesn't (yet) make
sense.
> The default behaviour for new controls is the legacy persistence mode.
> New clients will have to opt out for immediate cleanup on context
> closure.
"... have to opt in to immediate ..." reads more clear.
> If the hangchecking modparam is disabled, so is persistent
> context support -- all contexts will be terminated on closure.
Let's add here that it has actually been a source of bug reports
in the past that we don't terminate the workoads. So we expect
this behaviour change to be welcomed by the compute users who
have been instructed to disable the hanghceck in the past.
A couple of comments below. But anyway, with the uAPI comment
and commit message clarified this is:
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Still needs the Link:, though.
> Testcase: igt/gem_ctx_persistence
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
<SNIP>
> +static int __context_set_persistence(struct i915_gem_context *ctx, bool state)
> +{
> + if (i915_gem_context_is_persistent(ctx) == state)
> + return 0;
> +
> + if (state) {
> + /*
> + * Only contexts that are short-lived [that will expire or be
> + * reset] are allowed to survive past termination. We require
> + * hangcheck to ensure that the persistent requests are healthy.
> + */
> + if (!i915_modparams.enable_hangcheck)
> + return -EINVAL;
This is slightly confusing as the default is to enable persistence.
Disabling and re-enabling would result -EINVAL. But I guess it's no
problem to lift such restriction later.
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1572,6 +1572,21 @@ struct drm_i915_gem_context_param {
> * i915_context_engines_bond (I915_CONTEXT_ENGINES_EXT_BOND)
> */
> #define I915_CONTEXT_PARAM_ENGINES 0xa
> +
> +/*
> + * I915_CONTEXT_PARAM_PERSISTENCE:
> + *
> + * Allow the context and active rendering to survive the process until
> + * completion. Persistence allows fire-and-forget clients to queue up a
> + * bunch of work, hand the output over to a display server and the quit.
> + * If the context is not marked as persistent, upon closing (either via
".. is marked as not persistent," is more clear.
Regards, Joonas
> + * an explicit DRM_I915_GEM_CONTEXT_DESTROY or implicitly from file closure
> + * or process termination), the context and any outstanding requests will be
> + * cancelled (and exported fences for cancelled requests marked as -EIO).
> + *
> + * By default, new contexts allow persistence.
> + */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 01/11] drm/i915/gem: Make context persistence optional
@ 2019-10-25 18:22 ` Jason Ekstrand
0 siblings, 0 replies; 40+ messages in thread
From: Jason Ekstrand @ 2019-10-25 18:22 UTC (permalink / raw)
To: Chris Wilson; +Cc: Intel GFX
[-- Attachment #1.1: Type: text/plain, Size: 10118 bytes --]
On Thu, Oct 24, 2019 at 6:40 AM Chris Wilson <chris@chris-wilson.co.uk>
wrote:
> Our existing behaviour is to allow contexts and their GPU requests to
> persist past the point of closure until the requests are complete. This
> allows clients to operate in a 'fire-and-forget' manner where they can
> setup a rendering pipeline and hand it over to the display server and
> immediately exiting. As the rendering pipeline is kept alive until
> completion, the display server (or other consumer) can use the results
> in the future and present them to the user.
>
> However, not all clients want this persistent behaviour and would prefer
> that the contexts are cleaned up immediately upon closure. This ensures
> that when clients are run without hangchecking, any GPU hang is
> terminated with the process and does not continue to hog resources.
>
> By defining a context property to allow clients to control persistence
> explicitly, we can remove the blanket advice to disable hangchecking
> that seems to be far too prevalent.
>
Just to be clear, when you say "disable hangchecking" do you mean disabling
it for all processes via a kernel parameter at boot time or a sysfs entry
or similar? Or is there some mechanism whereby a context can request no
hang checking?
> The default behaviour for new controls is the legacy persistence mode.
> New clients will have to opt out for immediate cleanup on context
> closure. If the hangchecking modparam is disabled, so is persistent
> context support -- all contexts will be terminated on closure.
>
What happens to fences when the context is cancelled? Is it the same
behavior as we have today for when a GPU hang is detected and a context is
banned?
--Jason
> Testcase: igt/gem_ctx_persistence
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 50 ++++++++++++++++++-
> drivers/gpu/drm/i915/gem/i915_gem_context.h | 15 ++++++
> .../gpu/drm/i915/gem/i915_gem_context_types.h | 1 +
> .../gpu/drm/i915/gem/selftests/mock_context.c | 2 +
> include/uapi/drm/i915_drm.h | 15 ++++++
> 5 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 55f1f93c0925..0f1bbf5d1a11 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -437,12 +437,39 @@ static void context_close(struct i915_gem_context
> *ctx)
> * case we opt to forcibly kill off all remaining requests on
> * context close.
> */
> - if (!i915_modparams.enable_hangcheck)
> + if (!i915_gem_context_is_persistent(ctx) ||
> + !i915_modparams.enable_hangcheck)
> kill_context(ctx);
>
> i915_gem_context_put(ctx);
> }
>
> +static int __context_set_persistence(struct i915_gem_context *ctx, bool
> state)
> +{
> + if (i915_gem_context_is_persistent(ctx) == state)
> + return 0;
> +
> + if (state) {
> + /*
> + * Only contexts that are short-lived [that will expire or
> be
> + * reset] are allowed to survive past termination. We
> require
> + * hangcheck to ensure that the persistent requests are
> healthy.
> + */
> + if (!i915_modparams.enable_hangcheck)
> + return -EINVAL;
> +
> + i915_gem_context_set_persistence(ctx);
> + } else {
> + /* To cancel a context we use "preempt-to-idle" */
> + if (!(ctx->i915->caps.scheduler &
> I915_SCHEDULER_CAP_PREEMPTION))
> + return -ENODEV;
> +
> + i915_gem_context_clear_persistence(ctx);
> + }
> +
> + return 0;
> +}
> +
> static struct i915_gem_context *
> __create_context(struct drm_i915_private *i915)
> {
> @@ -477,6 +504,7 @@ __create_context(struct drm_i915_private *i915)
>
> i915_gem_context_set_bannable(ctx);
> i915_gem_context_set_recoverable(ctx);
> + __context_set_persistence(ctx, true /* cgroup hook? */);
>
> for (i = 0; i < ARRAY_SIZE(ctx->hang_timestamp); i++)
> ctx->hang_timestamp[i] = jiffies -
> CONTEXT_FAST_HANG_JIFFIES;
> @@ -633,6 +661,7 @@ i915_gem_context_create_kernel(struct drm_i915_private
> *i915, int prio)
> return ctx;
>
> i915_gem_context_clear_bannable(ctx);
> + i915_gem_context_set_persistence(ctx);
> ctx->sched.priority = I915_USER_PRIORITY(prio);
>
> GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> @@ -1743,6 +1772,16 @@ get_engines(struct i915_gem_context *ctx,
> return err;
> }
>
> +static int
> +set_persistence(struct i915_gem_context *ctx,
> + const struct drm_i915_gem_context_param *args)
> +{
> + if (args->size)
> + return -EINVAL;
> +
> + return __context_set_persistence(ctx, args->value);
> +}
> +
> static int ctx_setparam(struct drm_i915_file_private *fpriv,
> struct i915_gem_context *ctx,
> struct drm_i915_gem_context_param *args)
> @@ -1820,6 +1859,10 @@ static int ctx_setparam(struct
> drm_i915_file_private *fpriv,
> ret = set_engines(ctx, args);
> break;
>
> + case I915_CONTEXT_PARAM_PERSISTENCE:
> + ret = set_persistence(ctx, args);
> + break;
> +
> case I915_CONTEXT_PARAM_BAN_PERIOD:
> default:
> ret = -EINVAL;
> @@ -2272,6 +2315,11 @@ int i915_gem_context_getparam_ioctl(struct
> drm_device *dev, void *data,
> ret = get_engines(ctx, args);
> break;
>
> + case I915_CONTEXT_PARAM_PERSISTENCE:
> + args->size = 0;
> + args->value = i915_gem_context_is_persistent(ctx);
> + break;
> +
> case I915_CONTEXT_PARAM_BAN_PERIOD:
> default:
> ret = -EINVAL;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h
> b/drivers/gpu/drm/i915/gem/i915_gem_context.h
> index cfe80590f0ed..18e50a769a6e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
> @@ -76,6 +76,21 @@ static inline void
> i915_gem_context_clear_recoverable(struct i915_gem_context *c
> clear_bit(UCONTEXT_RECOVERABLE, &ctx->user_flags);
> }
>
> +static inline bool i915_gem_context_is_persistent(const struct
> i915_gem_context *ctx)
> +{
> + return test_bit(UCONTEXT_PERSISTENCE, &ctx->user_flags);
> +}
> +
> +static inline void i915_gem_context_set_persistence(struct
> i915_gem_context *ctx)
> +{
> + set_bit(UCONTEXT_PERSISTENCE, &ctx->user_flags);
> +}
> +
> +static inline void i915_gem_context_clear_persistence(struct
> i915_gem_context *ctx)
> +{
> + clear_bit(UCONTEXT_PERSISTENCE, &ctx->user_flags);
> +}
> +
> static inline bool i915_gem_context_is_banned(const struct
> i915_gem_context *ctx)
> {
> return test_bit(CONTEXT_BANNED, &ctx->flags);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> index fe97b8ba4fda..861d7d92fe9f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> @@ -137,6 +137,7 @@ struct i915_gem_context {
> #define UCONTEXT_NO_ERROR_CAPTURE 1
> #define UCONTEXT_BANNABLE 2
> #define UCONTEXT_RECOVERABLE 3
> +#define UCONTEXT_PERSISTENCE 4
>
> /**
> * @flags: small set of booleans
> diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> index 74ddd682c9cd..29b8984f0e47 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> @@ -22,6 +22,8 @@ mock_context(struct drm_i915_private *i915,
> INIT_LIST_HEAD(&ctx->link);
> ctx->i915 = i915;
>
> + i915_gem_context_set_persistence(ctx);
> +
> mutex_init(&ctx->engines_mutex);
> e = default_engines(ctx);
> if (IS_ERR(e))
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 63d40cba97e0..b5fd5e4bd16e 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1572,6 +1572,21 @@ struct drm_i915_gem_context_param {
> * i915_context_engines_bond (I915_CONTEXT_ENGINES_EXT_BOND)
> */
> #define I915_CONTEXT_PARAM_ENGINES 0xa
> +
> +/*
> + * I915_CONTEXT_PARAM_PERSISTENCE:
> + *
> + * Allow the context and active rendering to survive the process until
> + * completion. Persistence allows fire-and-forget clients to queue up a
> + * bunch of work, hand the output over to a display server and the quit.
> + * If the context is not marked as persistent, upon closing (either via
> + * an explicit DRM_I915_GEM_CONTEXT_DESTROY or implicitly from file
> closure
> + * or process termination), the context and any outstanding requests will
> be
> + * cancelled (and exported fences for cancelled requests marked as -EIO).
> + *
> + * By default, new contexts allow persistence.
> + */
> +#define I915_CONTEXT_PARAM_PERSISTENCE 0xb
> /* Must be kept compact -- no holes and well documented */
>
> __u64 value;
> --
> 2.24.0.rc0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[-- Attachment #1.2: Type: text/html, Size: 12357 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Intel-gfx] [PATCH 01/11] drm/i915/gem: Make context persistence optional
@ 2019-10-25 18:22 ` Jason Ekstrand
0 siblings, 0 replies; 40+ messages in thread
From: Jason Ekstrand @ 2019-10-25 18:22 UTC (permalink / raw)
To: Chris Wilson; +Cc: Intel GFX
[-- Attachment #1.1: Type: text/plain, Size: 10118 bytes --]
On Thu, Oct 24, 2019 at 6:40 AM Chris Wilson <chris@chris-wilson.co.uk>
wrote:
> Our existing behaviour is to allow contexts and their GPU requests to
> persist past the point of closure until the requests are complete. This
> allows clients to operate in a 'fire-and-forget' manner where they can
> setup a rendering pipeline and hand it over to the display server and
> immediately exiting. As the rendering pipeline is kept alive until
> completion, the display server (or other consumer) can use the results
> in the future and present them to the user.
>
> However, not all clients want this persistent behaviour and would prefer
> that the contexts are cleaned up immediately upon closure. This ensures
> that when clients are run without hangchecking, any GPU hang is
> terminated with the process and does not continue to hog resources.
>
> By defining a context property to allow clients to control persistence
> explicitly, we can remove the blanket advice to disable hangchecking
> that seems to be far too prevalent.
>
Just to be clear, when you say "disable hangchecking" do you mean disabling
it for all processes via a kernel parameter at boot time or a sysfs entry
or similar? Or is there some mechanism whereby a context can request no
hang checking?
> The default behaviour for new controls is the legacy persistence mode.
> New clients will have to opt out for immediate cleanup on context
> closure. If the hangchecking modparam is disabled, so is persistent
> context support -- all contexts will be terminated on closure.
>
What happens to fences when the context is cancelled? Is it the same
behavior as we have today for when a GPU hang is detected and a context is
banned?
--Jason
> Testcase: igt/gem_ctx_persistence
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 50 ++++++++++++++++++-
> drivers/gpu/drm/i915/gem/i915_gem_context.h | 15 ++++++
> .../gpu/drm/i915/gem/i915_gem_context_types.h | 1 +
> .../gpu/drm/i915/gem/selftests/mock_context.c | 2 +
> include/uapi/drm/i915_drm.h | 15 ++++++
> 5 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 55f1f93c0925..0f1bbf5d1a11 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -437,12 +437,39 @@ static void context_close(struct i915_gem_context
> *ctx)
> * case we opt to forcibly kill off all remaining requests on
> * context close.
> */
> - if (!i915_modparams.enable_hangcheck)
> + if (!i915_gem_context_is_persistent(ctx) ||
> + !i915_modparams.enable_hangcheck)
> kill_context(ctx);
>
> i915_gem_context_put(ctx);
> }
>
> +static int __context_set_persistence(struct i915_gem_context *ctx, bool
> state)
> +{
> + if (i915_gem_context_is_persistent(ctx) == state)
> + return 0;
> +
> + if (state) {
> + /*
> + * Only contexts that are short-lived [that will expire or
> be
> + * reset] are allowed to survive past termination. We
> require
> + * hangcheck to ensure that the persistent requests are
> healthy.
> + */
> + if (!i915_modparams.enable_hangcheck)
> + return -EINVAL;
> +
> + i915_gem_context_set_persistence(ctx);
> + } else {
> + /* To cancel a context we use "preempt-to-idle" */
> + if (!(ctx->i915->caps.scheduler &
> I915_SCHEDULER_CAP_PREEMPTION))
> + return -ENODEV;
> +
> + i915_gem_context_clear_persistence(ctx);
> + }
> +
> + return 0;
> +}
> +
> static struct i915_gem_context *
> __create_context(struct drm_i915_private *i915)
> {
> @@ -477,6 +504,7 @@ __create_context(struct drm_i915_private *i915)
>
> i915_gem_context_set_bannable(ctx);
> i915_gem_context_set_recoverable(ctx);
> + __context_set_persistence(ctx, true /* cgroup hook? */);
>
> for (i = 0; i < ARRAY_SIZE(ctx->hang_timestamp); i++)
> ctx->hang_timestamp[i] = jiffies -
> CONTEXT_FAST_HANG_JIFFIES;
> @@ -633,6 +661,7 @@ i915_gem_context_create_kernel(struct drm_i915_private
> *i915, int prio)
> return ctx;
>
> i915_gem_context_clear_bannable(ctx);
> + i915_gem_context_set_persistence(ctx);
> ctx->sched.priority = I915_USER_PRIORITY(prio);
>
> GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> @@ -1743,6 +1772,16 @@ get_engines(struct i915_gem_context *ctx,
> return err;
> }
>
> +static int
> +set_persistence(struct i915_gem_context *ctx,
> + const struct drm_i915_gem_context_param *args)
> +{
> + if (args->size)
> + return -EINVAL;
> +
> + return __context_set_persistence(ctx, args->value);
> +}
> +
> static int ctx_setparam(struct drm_i915_file_private *fpriv,
> struct i915_gem_context *ctx,
> struct drm_i915_gem_context_param *args)
> @@ -1820,6 +1859,10 @@ static int ctx_setparam(struct
> drm_i915_file_private *fpriv,
> ret = set_engines(ctx, args);
> break;
>
> + case I915_CONTEXT_PARAM_PERSISTENCE:
> + ret = set_persistence(ctx, args);
> + break;
> +
> case I915_CONTEXT_PARAM_BAN_PERIOD:
> default:
> ret = -EINVAL;
> @@ -2272,6 +2315,11 @@ int i915_gem_context_getparam_ioctl(struct
> drm_device *dev, void *data,
> ret = get_engines(ctx, args);
> break;
>
> + case I915_CONTEXT_PARAM_PERSISTENCE:
> + args->size = 0;
> + args->value = i915_gem_context_is_persistent(ctx);
> + break;
> +
> case I915_CONTEXT_PARAM_BAN_PERIOD:
> default:
> ret = -EINVAL;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h
> b/drivers/gpu/drm/i915/gem/i915_gem_context.h
> index cfe80590f0ed..18e50a769a6e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
> @@ -76,6 +76,21 @@ static inline void
> i915_gem_context_clear_recoverable(struct i915_gem_context *c
> clear_bit(UCONTEXT_RECOVERABLE, &ctx->user_flags);
> }
>
> +static inline bool i915_gem_context_is_persistent(const struct
> i915_gem_context *ctx)
> +{
> + return test_bit(UCONTEXT_PERSISTENCE, &ctx->user_flags);
> +}
> +
> +static inline void i915_gem_context_set_persistence(struct
> i915_gem_context *ctx)
> +{
> + set_bit(UCONTEXT_PERSISTENCE, &ctx->user_flags);
> +}
> +
> +static inline void i915_gem_context_clear_persistence(struct
> i915_gem_context *ctx)
> +{
> + clear_bit(UCONTEXT_PERSISTENCE, &ctx->user_flags);
> +}
> +
> static inline bool i915_gem_context_is_banned(const struct
> i915_gem_context *ctx)
> {
> return test_bit(CONTEXT_BANNED, &ctx->flags);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> index fe97b8ba4fda..861d7d92fe9f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> @@ -137,6 +137,7 @@ struct i915_gem_context {
> #define UCONTEXT_NO_ERROR_CAPTURE 1
> #define UCONTEXT_BANNABLE 2
> #define UCONTEXT_RECOVERABLE 3
> +#define UCONTEXT_PERSISTENCE 4
>
> /**
> * @flags: small set of booleans
> diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> index 74ddd682c9cd..29b8984f0e47 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> @@ -22,6 +22,8 @@ mock_context(struct drm_i915_private *i915,
> INIT_LIST_HEAD(&ctx->link);
> ctx->i915 = i915;
>
> + i915_gem_context_set_persistence(ctx);
> +
> mutex_init(&ctx->engines_mutex);
> e = default_engines(ctx);
> if (IS_ERR(e))
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 63d40cba97e0..b5fd5e4bd16e 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1572,6 +1572,21 @@ struct drm_i915_gem_context_param {
> * i915_context_engines_bond (I915_CONTEXT_ENGINES_EXT_BOND)
> */
> #define I915_CONTEXT_PARAM_ENGINES 0xa
> +
> +/*
> + * I915_CONTEXT_PARAM_PERSISTENCE:
> + *
> + * Allow the context and active rendering to survive the process until
> + * completion. Persistence allows fire-and-forget clients to queue up a
> + * bunch of work, hand the output over to a display server and the quit.
> + * If the context is not marked as persistent, upon closing (either via
> + * an explicit DRM_I915_GEM_CONTEXT_DESTROY or implicitly from file
> closure
> + * or process termination), the context and any outstanding requests will
> be
> + * cancelled (and exported fences for cancelled requests marked as -EIO).
> + *
> + * By default, new contexts allow persistence.
> + */
> +#define I915_CONTEXT_PARAM_PERSISTENCE 0xb
> /* Must be kept compact -- no holes and well documented */
>
> __u64 value;
> --
> 2.24.0.rc0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[-- Attachment #1.2: Type: text/html, Size: 12357 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 01/11] drm/i915/gem: Make context persistence optional
@ 2019-10-25 21:29 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-25 21:29 UTC (permalink / raw)
To: Jason Ekstrand; +Cc: Intel GFX
Quoting Jason Ekstrand (2019-10-25 19:22:04)
> On Thu, Oct 24, 2019 at 6:40 AM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Our existing behaviour is to allow contexts and their GPU requests to
> persist past the point of closure until the requests are complete. This
> allows clients to operate in a 'fire-and-forget' manner where they can
> setup a rendering pipeline and hand it over to the display server and
> immediately exiting. As the rendering pipeline is kept alive until
> completion, the display server (or other consumer) can use the results
> in the future and present them to the user.
>
> However, not all clients want this persistent behaviour and would prefer
> that the contexts are cleaned up immediately upon closure. This ensures
> that when clients are run without hangchecking, any GPU hang is
> terminated with the process and does not continue to hog resources.
>
> By defining a context property to allow clients to control persistence
> explicitly, we can remove the blanket advice to disable hangchecking
> that seems to be far too prevalent.
>
>
> Just to be clear, when you say "disable hangchecking" do you mean disabling it
> for all processes via a kernel parameter at boot time or a sysfs entry or
> similar? Or is there some mechanism whereby a context can request no hang
> checking?
They are being told to use the module parameter i915.enable_hangcheck=0
to globally disable hangchecking. This is what we are trying to wean
them off, and yet still allow indefinitely long kernels. The softer
hangcheck is focused on if you block scheduling or preemption of higher
priority work, then you are forcibly removed from the GPU. However, even
that is too much for some workloads, where they really do expect to
permanently hog the GPU. (All I can say is that they better be dedicated
systems because if you demand interactivity on top of disabling
preemption...)
> The default behaviour for new controls is the legacy persistence mode.
> New clients will have to opt out for immediate cleanup on context
> closure. If the hangchecking modparam is disabled, so is persistent
> context support -- all contexts will be terminated on closure.
>
>
> What happens to fences when the context is cancelled? Is it the same behavior
> as we have today for when a GPU hang is detected and a context is banned?
Yes. The incomplete fence statuses are set to -EIO -- it is the very same
mechanism used to remove this context's future work from the GPU as is
used for banning.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Intel-gfx] [PATCH 01/11] drm/i915/gem: Make context persistence optional
@ 2019-10-25 21:29 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-25 21:29 UTC (permalink / raw)
To: Jason Ekstrand; +Cc: Intel GFX
Quoting Jason Ekstrand (2019-10-25 19:22:04)
> On Thu, Oct 24, 2019 at 6:40 AM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Our existing behaviour is to allow contexts and their GPU requests to
> persist past the point of closure until the requests are complete. This
> allows clients to operate in a 'fire-and-forget' manner where they can
> setup a rendering pipeline and hand it over to the display server and
> immediately exiting. As the rendering pipeline is kept alive until
> completion, the display server (or other consumer) can use the results
> in the future and present them to the user.
>
> However, not all clients want this persistent behaviour and would prefer
> that the contexts are cleaned up immediately upon closure. This ensures
> that when clients are run without hangchecking, any GPU hang is
> terminated with the process and does not continue to hog resources.
>
> By defining a context property to allow clients to control persistence
> explicitly, we can remove the blanket advice to disable hangchecking
> that seems to be far too prevalent.
>
>
> Just to be clear, when you say "disable hangchecking" do you mean disabling it
> for all processes via a kernel parameter at boot time or a sysfs entry or
> similar? Or is there some mechanism whereby a context can request no hang
> checking?
They are being told to use the module parameter i915.enable_hangcheck=0
to globally disable hangchecking. This is what we are trying to wean
them off, and yet still allow indefinitely long kernels. The softer
hangcheck is focused on if you block scheduling or preemption of higher
priority work, then you are forcibly removed from the GPU. However, even
that is too much for some workloads, where they really do expect to
permanently hog the GPU. (All I can say is that they better be dedicated
systems because if you demand interactivity on top of disabling
preemption...)
> The default behaviour for new controls is the legacy persistence mode.
> New clients will have to opt out for immediate cleanup on context
> closure. If the hangchecking modparam is disabled, so is persistent
> context support -- all contexts will be terminated on closure.
>
>
> What happens to fences when the context is cancelled? Is it the same behavior
> as we have today for when a GPU hang is detected and a context is banned?
Yes. The incomplete fence statuses are set to -EIO -- it is the very same
mechanism used to remove this context's future work from the GPU as is
used for banning.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 01/11] drm/i915/gem: Make context persistence optional
@ 2019-10-29 16:19 ` Jason Ekstrand
0 siblings, 0 replies; 40+ messages in thread
From: Jason Ekstrand @ 2019-10-29 16:19 UTC (permalink / raw)
To: Chris Wilson; +Cc: Intel GFX
[-- Attachment #1.1: Type: text/plain, Size: 3724 bytes --]
On Fri, Oct 25, 2019 at 4:29 PM Chris Wilson <chris@chris-wilson.co.uk>
wrote:
> Quoting Jason Ekstrand (2019-10-25 19:22:04)
> > On Thu, Oct 24, 2019 at 6:40 AM Chris Wilson <chris@chris-wilson.co.uk>
> wrote:
> >
> > Our existing behaviour is to allow contexts and their GPU requests to
> > persist past the point of closure until the requests are complete.
> This
> > allows clients to operate in a 'fire-and-forget' manner where they
> can
> > setup a rendering pipeline and hand it over to the display server and
> > immediately exiting. As the rendering pipeline is kept alive until
> > completion, the display server (or other consumer) can use the
> results
> > in the future and present them to the user.
> >
> > However, not all clients want this persistent behaviour and would
> prefer
> > that the contexts are cleaned up immediately upon closure. This
> ensures
> > that when clients are run without hangchecking, any GPU hang is
> > terminated with the process and does not continue to hog resources.
> >
> > By defining a context property to allow clients to control
> persistence
> > explicitly, we can remove the blanket advice to disable hangchecking
> > that seems to be far too prevalent.
> >
> >
> > Just to be clear, when you say "disable hangchecking" do you mean
> disabling it
> > for all processes via a kernel parameter at boot time or a sysfs entry or
> > similar? Or is there some mechanism whereby a context can request no
> hang
> > checking?
>
> They are being told to use the module parameter i915.enable_hangcheck=0
> to globally disable hangchecking. This is what we are trying to wean
> them off, and yet still allow indefinitely long kernels. The softer
> hangcheck is focused on if you block scheduling or preemption of higher
> priority work, then you are forcibly removed from the GPU. However, even
> that is too much for some workloads, where they really do expect to
> permanently hog the GPU. (All I can say is that they better be dedicated
> systems because if you demand interactivity on top of disabling
> preemption...)
>
Ok, thinking out loud here (no need for you to respond): Why should we
take this approach? It seems like there are several other ways we could
solve this:
1. Have a per-context flag (that's what we did here)
2. Have a per-execbuf flag for "don't allow this execbuf to outlive the
process".
3. Have a DRM_IOCTL_I915_KILL_CONTEXT which lets the client manually kill
the context
Option 2 seems like a lot more work in i915 and it doesn't seem that
advantageous. Most drivers are going to either want their batches to
outlive them or not; they aren't going to be making that decision on a
per-batch basis.
Option 3 would work for some cases but it doesn't let the kernel terminate
work if the client is killed unexpectedly by, for instance a segfault. The
client could try to insert a crash handler but calling a DRM ioctl from a
crash handler sounds like a bad plan. On the other hand, the client can
just as easily implement 3 by setting the new context flag and then calling
GEM_CONTEXT_DESTROY.
With that, I think I'm convinced that a context param is the best way to do
this. We may even consider using it in Vulkan when running headless to let
us kill stuff quicker. We aren't seeing any long-running Vulkan compute
workloads yet but they may be coming.
Acked-by: Jason Ekstrand <jason@jlekstrand.net>
One more question: Does this bit fully support being turned on and off or
is it a set-once? I ask because how I'd likely go about doing this in
Vulkan would be to set it on context create and then unset it the moment we
see a buffer shared with the outside world.
--Jason
[-- Attachment #1.2: Type: text/html, Size: 4656 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Intel-gfx] [PATCH 01/11] drm/i915/gem: Make context persistence optional
@ 2019-10-29 16:19 ` Jason Ekstrand
0 siblings, 0 replies; 40+ messages in thread
From: Jason Ekstrand @ 2019-10-29 16:19 UTC (permalink / raw)
To: Chris Wilson; +Cc: Intel GFX
[-- Attachment #1.1: Type: text/plain, Size: 3724 bytes --]
On Fri, Oct 25, 2019 at 4:29 PM Chris Wilson <chris@chris-wilson.co.uk>
wrote:
> Quoting Jason Ekstrand (2019-10-25 19:22:04)
> > On Thu, Oct 24, 2019 at 6:40 AM Chris Wilson <chris@chris-wilson.co.uk>
> wrote:
> >
> > Our existing behaviour is to allow contexts and their GPU requests to
> > persist past the point of closure until the requests are complete.
> This
> > allows clients to operate in a 'fire-and-forget' manner where they
> can
> > setup a rendering pipeline and hand it over to the display server and
> > immediately exiting. As the rendering pipeline is kept alive until
> > completion, the display server (or other consumer) can use the
> results
> > in the future and present them to the user.
> >
> > However, not all clients want this persistent behaviour and would
> prefer
> > that the contexts are cleaned up immediately upon closure. This
> ensures
> > that when clients are run without hangchecking, any GPU hang is
> > terminated with the process and does not continue to hog resources.
> >
> > By defining a context property to allow clients to control
> persistence
> > explicitly, we can remove the blanket advice to disable hangchecking
> > that seems to be far too prevalent.
> >
> >
> > Just to be clear, when you say "disable hangchecking" do you mean
> disabling it
> > for all processes via a kernel parameter at boot time or a sysfs entry or
> > similar? Or is there some mechanism whereby a context can request no
> hang
> > checking?
>
> They are being told to use the module parameter i915.enable_hangcheck=0
> to globally disable hangchecking. This is what we are trying to wean
> them off, and yet still allow indefinitely long kernels. The softer
> hangcheck is focused on if you block scheduling or preemption of higher
> priority work, then you are forcibly removed from the GPU. However, even
> that is too much for some workloads, where they really do expect to
> permanently hog the GPU. (All I can say is that they better be dedicated
> systems because if you demand interactivity on top of disabling
> preemption...)
>
Ok, thinking out loud here (no need for you to respond): Why should we
take this approach? It seems like there are several other ways we could
solve this:
1. Have a per-context flag (that's what we did here)
2. Have a per-execbuf flag for "don't allow this execbuf to outlive the
process".
3. Have a DRM_IOCTL_I915_KILL_CONTEXT which lets the client manually kill
the context
Option 2 seems like a lot more work in i915 and it doesn't seem that
advantageous. Most drivers are going to either want their batches to
outlive them or not; they aren't going to be making that decision on a
per-batch basis.
Option 3 would work for some cases but it doesn't let the kernel terminate
work if the client is killed unexpectedly by, for instance a segfault. The
client could try to insert a crash handler but calling a DRM ioctl from a
crash handler sounds like a bad plan. On the other hand, the client can
just as easily implement 3 by setting the new context flag and then calling
GEM_CONTEXT_DESTROY.
With that, I think I'm convinced that a context param is the best way to do
this. We may even consider using it in Vulkan when running headless to let
us kill stuff quicker. We aren't seeing any long-running Vulkan compute
workloads yet but they may be coming.
Acked-by: Jason Ekstrand <jason@jlekstrand.net>
One more question: Does this bit fully support being turned on and off or
is it a set-once? I ask because how I'd likely go about doing this in
Vulkan would be to set it on context create and then unset it the moment we
see a buffer shared with the outside world.
--Jason
[-- Attachment #1.2: Type: text/html, Size: 4656 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 01/11] drm/i915/gem: Make context persistence optional
@ 2019-10-29 18:02 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-29 18:02 UTC (permalink / raw)
To: Jason Ekstrand; +Cc: Intel GFX
Quoting Jason Ekstrand (2019-10-29 16:19:09)
>
>
> On Fri, Oct 25, 2019 at 4:29 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Jason Ekstrand (2019-10-25 19:22:04)
> > On Thu, Oct 24, 2019 at 6:40 AM Chris Wilson <chris@chris-wilson.co.uk>
> wrote:
> >
> > Our existing behaviour is to allow contexts and their GPU requests to
> > persist past the point of closure until the requests are complete.
> This
> > allows clients to operate in a 'fire-and-forget' manner where they
> can
> > setup a rendering pipeline and hand it over to the display server and
> > immediately exiting. As the rendering pipeline is kept alive until
> > completion, the display server (or other consumer) can use the
> results
> > in the future and present them to the user.
> >
> > However, not all clients want this persistent behaviour and would
> prefer
> > that the contexts are cleaned up immediately upon closure. This
> ensures
> > that when clients are run without hangchecking, any GPU hang is
> > terminated with the process and does not continue to hog resources.
> >
> > By defining a context property to allow clients to control
> persistence
> > explicitly, we can remove the blanket advice to disable hangchecking
> > that seems to be far too prevalent.
> >
> >
> > Just to be clear, when you say "disable hangchecking" do you mean
> disabling it
> > for all processes via a kernel parameter at boot time or a sysfs entry or
> > similar? Or is there some mechanism whereby a context can request no
> hang
> > checking?
>
> They are being told to use the module parameter i915.enable_hangcheck=0
> to globally disable hangchecking. This is what we are trying to wean
> them off, and yet still allow indefinitely long kernels. The softer
> hangcheck is focused on if you block scheduling or preemption of higher
> priority work, then you are forcibly removed from the GPU. However, even
> that is too much for some workloads, where they really do expect to
> permanently hog the GPU. (All I can say is that they better be dedicated
> systems because if you demand interactivity on top of disabling
> preemption...)
>
>
> Ok, thinking out loud here (no need for you to respond): Why should we take
> this approach? It seems like there are several other ways we could solve this:
>
> 1. Have a per-context flag (that's what we did here)
> 2. Have a per-execbuf flag for "don't allow this execbuf to outlive the
> process".
> 3. Have a DRM_IOCTL_I915_KILL_CONTEXT which lets the client manually kill the
> context
>
> Option 2 seems like a lot more work in i915 and it doesn't seem that
> advantageous. Most drivers are going to either want their batches to outlive
> them or not; they aren't going to be making that decision on a per-batch basis.
And processing each batch on context close, deciding how to carry
forward the different options doesn't sound like fun.
> Option 3 would work for some cases but it doesn't let the kernel terminate work
> if the client is killed unexpectedly by, for instance a segfault. The client
> could try to insert a crash handler but calling a DRM ioctl from a crash
> handler sounds like a bad plan. On the other hand, the client can just as
> easily implement 3 by setting the new context flag and then calling
> GEM_CONTEXT_DESTROY.
Exactly. Abnormal process termination is the name of the game.
> With that, I think I'm convinced that a context param is the best way to do
> this. We may even consider using it in Vulkan when running headless to let us
> kill stuff quicker. We aren't seeing any long-running Vulkan compute workloads
> yet but they may be coming.
Yeah, might it find a use for GL robustness? If the app wants a
guarantee that its resources are garbage collected along with it?
> Acked-by: Jason Ekstrand <jason@jlekstrand.net>
>
>
> One more question: Does this bit fully support being turned on and off or is it
> a set-once? I ask because how I'd likely go about doing this in Vulkan would
> be to set it on context create and then unset it the moment we see a buffer
> shared with the outside world.
You can set it as many times as you like, it only takes effect on
context termination. That comes back to whether you want to evaluate it
as a batch/execbuf attribute or on the context. The starting point for
the patches was to close the process termination hole, of which this is
the natural extension for a context to opt into being cancelled on
closure. I think an all-or-nothing makes sense, or rather I don't see
enough advantage in the per-batch attribute to justify the processing.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Intel-gfx] [PATCH 01/11] drm/i915/gem: Make context persistence optional
@ 2019-10-29 18:02 ` Chris Wilson
0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2019-10-29 18:02 UTC (permalink / raw)
To: Jason Ekstrand; +Cc: Intel GFX
Quoting Jason Ekstrand (2019-10-29 16:19:09)
>
>
> On Fri, Oct 25, 2019 at 4:29 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Jason Ekstrand (2019-10-25 19:22:04)
> > On Thu, Oct 24, 2019 at 6:40 AM Chris Wilson <chris@chris-wilson.co.uk>
> wrote:
> >
> > Our existing behaviour is to allow contexts and their GPU requests to
> > persist past the point of closure until the requests are complete.
> This
> > allows clients to operate in a 'fire-and-forget' manner where they
> can
> > setup a rendering pipeline and hand it over to the display server and
> > immediately exiting. As the rendering pipeline is kept alive until
> > completion, the display server (or other consumer) can use the
> results
> > in the future and present them to the user.
> >
> > However, not all clients want this persistent behaviour and would
> prefer
> > that the contexts are cleaned up immediately upon closure. This
> ensures
> > that when clients are run without hangchecking, any GPU hang is
> > terminated with the process and does not continue to hog resources.
> >
> > By defining a context property to allow clients to control
> persistence
> > explicitly, we can remove the blanket advice to disable hangchecking
> > that seems to be far too prevalent.
> >
> >
> > Just to be clear, when you say "disable hangchecking" do you mean
> disabling it
> > for all processes via a kernel parameter at boot time or a sysfs entry or
> > similar? Or is there some mechanism whereby a context can request no
> hang
> > checking?
>
> They are being told to use the module parameter i915.enable_hangcheck=0
> to globally disable hangchecking. This is what we are trying to wean
> them off, and yet still allow indefinitely long kernels. The softer
> hangcheck is focused on if you block scheduling or preemption of higher
> priority work, then you are forcibly removed from the GPU. However, even
> that is too much for some workloads, where they really do expect to
> permanently hog the GPU. (All I can say is that they better be dedicated
> systems because if you demand interactivity on top of disabling
> preemption...)
>
>
> Ok, thinking out loud here (no need for you to respond): Why should we take
> this approach? It seems like there are several other ways we could solve this:
>
> 1. Have a per-context flag (that's what we did here)
> 2. Have a per-execbuf flag for "don't allow this execbuf to outlive the
> process".
> 3. Have a DRM_IOCTL_I915_KILL_CONTEXT which lets the client manually kill the
> context
>
> Option 2 seems like a lot more work in i915 and it doesn't seem that
> advantageous. Most drivers are going to either want their batches to outlive
> them or not; they aren't going to be making that decision on a per-batch basis.
And processing each batch on context close, deciding how to carry
forward the different options doesn't sound like fun.
> Option 3 would work for some cases but it doesn't let the kernel terminate work
> if the client is killed unexpectedly by, for instance a segfault. The client
> could try to insert a crash handler but calling a DRM ioctl from a crash
> handler sounds like a bad plan. On the other hand, the client can just as
> easily implement 3 by setting the new context flag and then calling
> GEM_CONTEXT_DESTROY.
Exactly. Abnormal process termination is the name of the game.
> With that, I think I'm convinced that a context param is the best way to do
> this. We may even consider using it in Vulkan when running headless to let us
> kill stuff quicker. We aren't seeing any long-running Vulkan compute workloads
> yet but they may be coming.
Yeah, might it find a use for GL robustness? If the app wants a
guarantee that its resources are garbage collected along with it?
> Acked-by: Jason Ekstrand <jason@jlekstrand.net>
>
>
> One more question: Does this bit fully support being turned on and off or is it
> a set-once? I ask because how I'd likely go about doing this in Vulkan would
> be to set it on context create and then unset it the moment we see a buffer
> shared with the outside world.
You can set it as many times as you like, it only takes effect on
context termination. That comes back to whether you want to evaluate it
as a batch/execbuf attribute or on the context. The starting point for
the patches was to close the process termination hole, of which this is
the natural extension for a context to opt into being cancelled on
closure. I think an all-or-nothing makes sense, or rather I don't see
enough advantage in the per-batch attribute to justify the processing.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2019-10-29 18:03 UTC | newest]
Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-24 11:40 CI testing of persistence and sysfs Chris Wilson
2019-10-24 11:40 ` [Intel-gfx] " Chris Wilson
2019-10-24 11:40 ` [PATCH 01/11] drm/i915/gem: Make context persistence optional Chris Wilson
2019-10-24 11:40 ` [Intel-gfx] " Chris Wilson
2019-10-25 9:10 ` Joonas Lahtinen
2019-10-25 9:10 ` [Intel-gfx] " Joonas Lahtinen
2019-10-25 18:22 ` Jason Ekstrand
2019-10-25 18:22 ` [Intel-gfx] " Jason Ekstrand
2019-10-25 21:29 ` Chris Wilson
2019-10-25 21:29 ` [Intel-gfx] " Chris Wilson
2019-10-29 16:19 ` Jason Ekstrand
2019-10-29 16:19 ` [Intel-gfx] " Jason Ekstrand
2019-10-29 18:02 ` Chris Wilson
2019-10-29 18:02 ` [Intel-gfx] " Chris Wilson
2019-10-24 11:40 ` [PATCH 02/11] drm/i915: Put future HW and their uAPIs under STAGING & BROKEN Chris Wilson
2019-10-24 11:40 ` [Intel-gfx] " Chris Wilson
2019-10-24 23:25 ` David Airlie
2019-10-24 23:25 ` [Intel-gfx] " David Airlie
2019-10-24 23:35 ` Rodrigo Vivi
2019-10-24 23:35 ` [Intel-gfx] " Rodrigo Vivi
2019-10-25 7:09 ` Jani Nikula
2019-10-25 7:09 ` [Intel-gfx] " Jani Nikula
2019-10-24 11:40 ` [PATCH 03/11] drm/i915/gt: Expose engine properties via sysfs Chris Wilson
2019-10-24 11:40 ` [Intel-gfx] " Chris Wilson
2019-10-24 11:40 ` [PATCH 04/11] drm/i915/gt: Expose engine->mmio_base " Chris Wilson
2019-10-24 11:40 ` [Intel-gfx] " Chris Wilson
2019-10-24 11:40 ` [PATCH 05/11] drm/i915/gt: Expose timeslice duration to sysfs Chris Wilson
2019-10-24 11:40 ` [Intel-gfx] " Chris Wilson
2019-10-24 11:40 ` [PATCH 06/11] drm/i915/gt: Expose reset stop timeout via sysfs Chris Wilson
2019-10-24 11:40 ` [Intel-gfx] " Chris Wilson
2019-10-24 11:40 ` [PATCH 07/11] drm/i915/gt: Expose preempt reset " Chris Wilson
2019-10-24 11:40 ` [Intel-gfx] " Chris Wilson
2019-10-24 11:40 ` [PATCH 08/11] drm/i915/gt: Expose heartbeat interval " Chris Wilson
2019-10-24 11:40 ` [Intel-gfx] " Chris Wilson
2019-10-24 11:40 ` [PATCH 09/11] drm/i915: Flush idle barriers when waiting Chris Wilson
2019-10-24 11:40 ` [Intel-gfx] " Chris Wilson
2019-10-24 11:40 ` [PATCH 10/11] drm/i915: Allow userspace to specify ringsize on construction Chris Wilson
2019-10-24 11:40 ` [Intel-gfx] " Chris Wilson
2019-10-24 11:40 ` [PATCH 11/11] drm/i915/gem: Honour O_NONBLOCK before throttling execbuf submissions Chris Wilson
2019-10-24 11:40 ` [Intel-gfx] " Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.