| Message ID | 20190806134725.25321-5-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/i915: Only enqueue already completed requests | expand |
Quoting Chris Wilson (2019-08-06 14:47:25) > +static int > +set_persistence(struct i915_gem_context *ctx, > + const struct drm_i915_gem_context_param *args) > +{ > + if (args->size) > + return -EINVAL; > + > + if (!args->value) { > + i915_gem_context_clear_persistence(ctx); > + return 0; > + } > + > + if (!(ctx->i915->caps.scheduler & I915_SCHEDULER_CAP_PREEMPTION)) > + return -ENODEV; > + > + i915_gem_context_set_persistence(ctx); > + return 0; > +} Oops, that is back-to-front. We can only switch off persistent behaviour if we can do the preempt. -Chris
> -----Original Message----- > From: Chris Wilson <chris@chris-wilson.co.uk> > Sent: Tuesday, August 6, 2019 6:47 AM > To: intel-gfx@lists.freedesktop.org > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Joonas Lahtinen > <joonas.lahtinen@linux.intel.com>; Winiarski, Michal > <michal.winiarski@intel.com>; Bloomfield, Jon <jon.bloomfield@intel.com> > Subject: [PATCH 5/5] drm/i915: Cancel non-persistent contexts on close > > Normally, we rely on our hangcheck to prevent persistent batches from > hogging the GPU. However, if the user disables hangcheck, this mechanism > breaks down. Despite our insistence that this is unsafe, the users are > equally insistent that they want to use endless batches and will disable > the hangcheck mechanism. We are looking are perhaps replacing hangcheck > with a softer mechanism, that sends a pulse down the engine to check if > it is well. We can use the same preemptive pulse to flush an active > persistent context off the GPU upon context close, preventing resources > being lost and unkillable requests remaining on the GPU, after process > termination. To avoid changing the ABI and accidentally breaking > existing userspace, we make the persistence of a context explicit and > enable it by default. Userspace can opt out of persistent mode (forcing > requests to be cancelled when the context is closed by process > termination or explicitly) by a context parameter, or to facilitate > existing use-cases by disabling hangcheck (i915.enable_hangcheck=0). > (Note, one of the outcomes for supporting endless mode will be the > removal of hangchecking, at which point opting into persistent mode will > be mandatory, or maybe the default.) > > 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> > > --- > Same sort of caveats as for hangcheck, a few corner cases need > struct_mutex and some preallocation. > --- > drivers/gpu/drm/i915/Makefile | 3 +- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 79 +++++++++++++++++++ > drivers/gpu/drm/i915/gem/i915_gem_context.h | 15 ++++ > .../gpu/drm/i915/gem/i915_gem_context_types.h | 1 + > .../gpu/drm/i915/gt/intel_engine_heartbeat.c | 53 +++++++++++++ > .../gpu/drm/i915/gt/intel_engine_heartbeat.h | 14 ++++ > include/uapi/drm/i915_drm.h | 15 ++++ > 7 files changed, 179 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > create mode 100644 drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index a1016858d014..42417d87496e 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -71,9 +71,10 @@ obj-y += gt/ > gt-y += \ > gt/intel_breadcrumbs.o \ > gt/intel_context.o \ > - gt/intel_engine_pool.o \ > gt/intel_engine_cs.o \ > + gt/intel_engine_heartbeat.o \ > gt/intel_engine_pm.o \ > + gt/intel_engine_pool.o \ > gt/intel_gt.o \ > gt/intel_gt_pm.o \ > gt/intel_hangcheck.o \ > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c > b/drivers/gpu/drm/i915/gem/i915_gem_context.c > index 64f7a533e886..5718b74f95b7 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -70,6 +70,7 @@ > #include <drm/i915_drm.h> > > #include "gt/intel_lrc_reg.h" > +#include "gt/intel_engine_heartbeat.h" > > #include "i915_gem_context.h" > #include "i915_globals.h" > @@ -373,6 +374,45 @@ void i915_gem_context_release(struct kref *ref) > queue_work(i915->wq, &i915->contexts.free_work); > } > > +static void kill_context(struct i915_gem_context *ctx) > +{ > + struct i915_gem_engines_iter it; > + struct intel_engine_cs *engine; > + intel_engine_mask_t tmp, active; > + struct intel_context *ce; > + > + if (i915_gem_context_is_banned(ctx)) > + return; > + > + i915_gem_context_set_banned(ctx); > + > + active = 0; > + for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) { > + struct i915_request *rq; > + > + if (!ce->ring) > + continue; > + > + rq = i915_active_request_get_unlocked(&ce->ring->timeline- > >last_request); > + if (!rq) > + continue; > + > + active |= rq->engine->mask; > + i915_request_put(rq); > + } > + i915_gem_context_unlock_engines(ctx); > + > + /* > + * Send a "high priority pulse" down the engine to cause the > + * current request to be momentarily preempted. (If it fails to > + * be preempted, it will be reset). As we have marked our context > + * as banned, any incomplete request, including any running, will > + * be skipped. > + */ > + for_each_engine_masked(engine, ctx->i915, active, tmp) > + intel_engine_pulse(engine); > +} > + > static void context_close(struct i915_gem_context *ctx) > { > mutex_lock(&ctx->mutex); > @@ -394,6 +434,15 @@ static void context_close(struct i915_gem_context > *ctx) > lut_close(ctx); > > mutex_unlock(&ctx->mutex); > + > + /* > + * If the user has disabled hangchecking, we can not be sure that > + * the batches will ever complete and let the context be freed. > + * Forcibly kill off any remaining requests in this case. > + */ Persistence is independent of hangcheck which makes the above comment unrepresentative of the below code. Do we even need a comment here, it looks self-explanatory? Instead maybe comment the defaulting state in __createContext() to make the connection between hangcheck and persistence? > + if (!i915_gem_context_is_persistent(ctx)) > + kill_context(ctx); > + > i915_gem_context_put(ctx); > } > > @@ -433,6 +482,8 @@ __create_context(struct drm_i915_private *i915) > > i915_gem_context_set_bannable(ctx); > i915_gem_context_set_recoverable(ctx); > + if (i915_modparams.enable_hangcheck) > + i915_gem_context_set_persistence(ctx); > > ctx->ring_size = 4 * PAGE_SIZE; > > @@ -1745,6 +1796,25 @@ 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; > + > + if (!args->value) { > + i915_gem_context_clear_persistence(ctx); > + return 0; > + } > + > + if (!(ctx->i915->caps.scheduler & > I915_SCHEDULER_CAP_PREEMPTION)) > + return -ENODEV; > + > + i915_gem_context_set_persistence(ctx); > + return 0; > +} > + > static int ctx_setparam(struct drm_i915_file_private *fpriv, > struct i915_gem_context *ctx, > struct drm_i915_gem_context_param *args) > @@ -1822,6 +1892,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; > @@ -2278,6 +2352,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 106e2ccf7a4c..1834d1df2ac9 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h > @@ -74,6 +74,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 a02d98494078..bf41b43ffa9a 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/gt/intel_engine_heartbeat.c > b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > new file mode 100644 > index 000000000000..0c0632a423a7 > --- /dev/null > +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > @@ -0,0 +1,53 @@ > +/* > + * SPDX-License-Identifier: MIT > + * > + * Copyright © 2019 Intel Corporation > + */ > + > +#include "i915_request.h" > + > +#include "intel_context.h" > +#include "intel_engine_heartbeat.h" > +#include "intel_engine_pm.h" > +#include "intel_engine.h" > +#include "intel_gt.h" > + > +void intel_engine_pulse(struct intel_engine_cs *engine) > +{ > + struct intel_context *ce = engine->kernel_context; > + struct i915_sched_attr attr = { .priority = INT_MAX }; > + struct i915_request *rq; > + int err = 0; > + > + GEM_BUG_ON(!engine->schedule); > + > + if (!intel_engine_pm_get_if_awake(engine)) > + return; > + > + mutex_lock(&ce->ring->timeline->mutex); > + > + intel_context_enter(ce); > + rq = __i915_request_create(ce, GFP_NOWAIT | __GFP_NOWARN); > + intel_context_exit(ce); > + if (IS_ERR(rq)) { > + err = PTR_ERR(rq); > + goto out_unlock; > + } > + i915_request_get(rq); > + > + rq->flags |= I915_REQUEST_SENTINEL; > + __i915_request_commit(rq); > + > + local_bh_disable(); > + engine->schedule(rq, &attr); > + local_bh_enable(); > + > + i915_request_put(rq); > + > +out_unlock: > + mutex_unlock(&ce->ring->timeline->mutex); > + intel_context_timeline_unlock(ce); > + intel_engine_pm_put(engine); > + if (err) /* XXX must not be allowed to fail */ > + DRM_ERROR("Failed to ping %s, err=%d\n", engine->name, > err); > +} > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h > b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h > new file mode 100644 > index 000000000000..86761748dc21 > --- /dev/null > +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h > @@ -0,0 +1,14 @@ > +/* > + * SPDX-License-Identifier: MIT > + * > + * Copyright © 2019 Intel Corporation > + */ > + > +#ifndef INTEL_ENGINE_HEARTBEAT_H > +#define INTEL_ENGINE_HEARTBEAT_H > + > +struct intel_engine_cs; > + > +void intel_engine_pulse(struct intel_engine_cs *engine); > + > +#endif /* INTEL_ENGINE_HEARTBEAT_H */ > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 469dc512cca3..dbc8691d75d0 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -1565,6 +1565,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.23.0.rc1
Quoting Bloomfield, Jon (2019-08-06 15:26:07) > > -----Original Message----- > > From: Chris Wilson <chris@chris-wilson.co.uk> > > + > > + /* > > + * If the user has disabled hangchecking, we can not be sure that > > + * the batches will ever complete and let the context be freed. > > + * Forcibly kill off any remaining requests in this case. > > + */ > > Persistence is independent of hangcheck which makes the above comment unrepresentative of the below code. > Do we even need a comment here, it looks self-explanatory? Instead maybe comment the defaulting state in __createContext() to make the connection between hangcheck and persistence? Indeed, it can be moved almost verbatim to explain the default behaviour and expand upon the cgroup wish. (Rather than using hangcheck modparam, let the sysadmin specify defaults for groups of processes -- planning for when we remove hangcheck entirely.) -Chris
Quoting Chris Wilson (2019-08-06 14:47:25) > @@ -433,6 +482,8 @@ __create_context(struct drm_i915_private *i915) > > i915_gem_context_set_bannable(ctx); > i915_gem_context_set_recoverable(ctx); > + if (i915_modparams.enable_hangcheck) > + i915_gem_context_set_persistence(ctx); I am not fond of this, but from a pragmatic point of view, this does prevent the issue Jon raised: HW resources being pinned indefinitely past process termination. I don't like it because we cannot perform the operation cleanly everywhere, it requires preemption first and foremost (with a cooperating submission backend) and per-engine reset. The alternative is to try and do a full GPU reset if the context is still active. For the sake of the issue raised, I think that (full reset on older HW) is required. That we are baking in a change of ABI due to an unsafe modparam is meh. There are a few more corner cases to deal with before endless just works. But since it is being used in the wild, I'm not sure we can wait for userspace to opt-in, or wait for cgroups. However, since users are being encouraged to disable hangcheck, should we extend the concept of persistence to also mean "survives hangcheck"? -- though it should be a separate parameter, and I'm not sure how exactly to protect it from the heartbeat reset without giving gross privileges to the context. (CPU isolation is nicer from the pov where we can just give up and not even worry about the engine. If userspace can request isolation, it has the privilege to screw up.) -Chris
> -----Original Message----- > From: Chris Wilson <chris@chris-wilson.co.uk> > Sent: Wednesday, August 7, 2019 6:23 AM > To: intel-gfx@lists.freedesktop.org > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Winiarski, Michal > <michal.winiarski@intel.com>; Bloomfield, Jon <jon.bloomfield@intel.com> > Subject: Re: [PATCH 5/5] drm/i915: Cancel non-persistent contexts on close > > Quoting Chris Wilson (2019-08-06 14:47:25) > > @@ -433,6 +482,8 @@ __create_context(struct drm_i915_private *i915) > > > > i915_gem_context_set_bannable(ctx); > > i915_gem_context_set_recoverable(ctx); > > + if (i915_modparams.enable_hangcheck) > > + i915_gem_context_set_persistence(ctx); > > I am not fond of this, but from a pragmatic point of view, this does > prevent the issue Jon raised: HW resources being pinned indefinitely > past process termination. > > I don't like it because we cannot perform the operation cleanly > everywhere, it requires preemption first and foremost (with a cooperating > submission backend) and per-engine reset. The alternative is to try and > do a full GPU reset if the context is still active. For the sake of the > issue raised, I think that (full reset on older HW) is required. > > That we are baking in a change of ABI due to an unsafe modparam is meh. > There are a few more corner cases to deal with before endless just > works. But since it is being used in the wild, I'm not sure we can wait > for userspace to opt-in, or wait for cgroups. However, since users are > being encouraged to disable hangcheck, should we extend the concept of > persistence to also mean "survives hangcheck"? -- though it should be a > separate parameter, and I'm not sure how exactly to protect it from the > heartbeat reset without giving gross privileges to the context. (CPU > isolation is nicer from the pov where we can just give up and not even > worry about the engine. If userspace can request isolation, it has the > privilege to screw up.) > -Chris Ok, so your concern is supporting non-persistence on older non-preempting, engine-reset capable, hardware. Why is that strictly required? Can't we simply make it dependent on the features needed to do it well, and if your hardware cannot, then the advice is not to disable hangcheck? I'm doubtful that anyone would attempt a HPC type workload on n IVB. I'm not sure I understand your "survives hangcheck" idea. You mean instead of simply disabling hangcheck, just opt in to persistence and have that also prevent hangcheck? Isn't that the wrong way around, since persistence is what is happening today?
Quoting Bloomfield, Jon (2019-08-07 15:04:16) > > -----Original Message----- > > From: Chris Wilson <chris@chris-wilson.co.uk> > > Sent: Wednesday, August 7, 2019 6:23 AM > > To: intel-gfx@lists.freedesktop.org > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Winiarski, Michal > > <michal.winiarski@intel.com>; Bloomfield, Jon <jon.bloomfield@intel.com> > > Subject: Re: [PATCH 5/5] drm/i915: Cancel non-persistent contexts on close > > > > Quoting Chris Wilson (2019-08-06 14:47:25) > > > @@ -433,6 +482,8 @@ __create_context(struct drm_i915_private *i915) > > > > > > i915_gem_context_set_bannable(ctx); > > > i915_gem_context_set_recoverable(ctx); > > > + if (i915_modparams.enable_hangcheck) > > > + i915_gem_context_set_persistence(ctx); > > > > I am not fond of this, but from a pragmatic point of view, this does > > prevent the issue Jon raised: HW resources being pinned indefinitely > > past process termination. > > > > I don't like it because we cannot perform the operation cleanly > > everywhere, it requires preemption first and foremost (with a cooperating > > submission backend) and per-engine reset. The alternative is to try and > > do a full GPU reset if the context is still active. For the sake of the > > issue raised, I think that (full reset on older HW) is required. > > > > That we are baking in a change of ABI due to an unsafe modparam is meh. > > There are a few more corner cases to deal with before endless just > > works. But since it is being used in the wild, I'm not sure we can wait > > for userspace to opt-in, or wait for cgroups. However, since users are > > being encouraged to disable hangcheck, should we extend the concept of > > persistence to also mean "survives hangcheck"? -- though it should be a > > separate parameter, and I'm not sure how exactly to protect it from the > > heartbeat reset without giving gross privileges to the context. (CPU > > isolation is nicer from the pov where we can just give up and not even > > worry about the engine. If userspace can request isolation, it has the > > privilege to screw up.) > > Ok, so your concern is supporting non-persistence on older non-preempting, engine-reset capable, hardware. Why is that strictly required? Can't we simply make it dependent on the features needed to do it well, and if your hardware cannot, then the advice is not to disable hangcheck? I'm doubtful that anyone would attempt a HPC type workload on n IVB. Our advice was not to disable hangcheck :) With the cat out of the bag, my concern is dotting the Is and crossing the Ts. Fixing up the error handling path to the reset isn't all that bad. But I'm not going to advertise the persistence-parameter support unless we have a clean solution, and we can advise that compute workloads are better handled with new hardware. > I'm not sure I understand your "survives hangcheck" idea. You mean instead of simply disabling hangcheck, just opt in to persistence and have that also prevent hangcheck? Isn't that the wrong way around, since persistence is what is happening today? Persistence is the clear-and-present danger. I'm just trying to sketch a path for endless support, trying to ask myself questions such as: Is the persistence parameter still required? What other parameters make sense? Does anything less than CPU-esque isolation make sense? :) -Chris
> -----Original Message----- > From: Chris Wilson <chris@chris-wilson.co.uk> > Sent: Wednesday, August 7, 2019 7:14 AM > To: Bloomfield, Jon <jon.bloomfield@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Winiarski, Michal > <michal.winiarski@intel.com> > Subject: RE: [PATCH 5/5] drm/i915: Cancel non-persistent contexts on close > > Quoting Bloomfield, Jon (2019-08-07 15:04:16) > > Ok, so your concern is supporting non-persistence on older non-preempting, > engine-reset capable, hardware. Why is that strictly required? Can't we simply > make it dependent on the features needed to do it well, and if your hardware > cannot, then the advice is not to disable hangcheck? I'm doubtful that anyone > would attempt a HPC type workload on n IVB. > > Our advice was not to disable hangcheck :) > > With the cat out of the bag, my concern is dotting the Is and crossing > the Ts. Fixing up the error handling path to the reset isn't all that > bad. But I'm not going to advertise the persistence-parameter support > unless we have a clean solution, and we can advise that compute > workloads are better handled with new hardware. > > > I'm not sure I understand your "survives hangcheck" idea. You mean instead > of simply disabling hangcheck, just opt in to persistence and have that also > prevent hangcheck? Isn't that the wrong way around, since persistence is what > is happening today? > > Persistence is the clear-and-present danger. I'm just trying to sketch a > path for endless support, trying to ask myself questions such as: Is the > persistence parameter still required? What other parameters make sense? > Does anything less than CPU-esque isolation make sense? :) > -Chris I personally liked your persistence idea :-) Isolation doesn't really solve the problem in this case. So, customer enables isolation for a HPC workload. That workload hangs, and user hits ctrl-C. We still have a hung workload and the next job in the queue still can't run. Also, Isolation is kind of meaningless when there is only one engine that's capable of running your workload. On Gen9, pretty much every type of workload requires some RCS involvement, and RCS is where the compute workloads need to run. So isolation hasn't helped us. I'd settle for umd opting in to non-persistence and not providing the automatic association with hangcheck. That at least ensures well behaved umd's don't kill the system. We didn't explore the idea of terminating orphaned contexts though (where none of their resources are referenced by any other contexts). Is there a reason why this is not feasible? In the case of compute (certainly HPC) workloads, there would be no compositor taking the output so this might be a solution. Jon
Quoting Bloomfield, Jon (2019-08-07 15:33:51)
[skip to end]
> We didn't explore the idea of terminating orphaned contexts though (where none of their resources are referenced by any other contexts). Is there a reason why this is not feasible? In the case of compute (certainly HPC) workloads, there would be no compositor taking the output so this might be a solution.
Sounds easier said than done. We have to go through each request and
determine it if has an external reference (or if the object holding the
reference has an external reference) to see if the output would be
visible to a third party. Sounds like a conservative GC :|
(Coming to that conclusion suggests that we should structure the request
tracking to make reparenting easier.)
We could take a pid-1 approach and move all the orphan timelines over to
a new parent purely responsible for them. That honestly doesn't seem to
achieve anything. (We are still stuck with tasks on the GPU and no way
to kill them.)
In comparison, persistence is a rarely used "feature" and cleaning up on
context close fits in nicely with the process model. It just works as
most users/clients would expect. (Although running in non-persistent
by default hasn't show anything to explode on the desktop, it's too easy
to construct scenarios where persistence turns out to be an advantage,
particularly with chains of clients (the compositor model).) Between the
two modes, we should have most bases covered, it's hard to argue for a
third way (that is until someone has a usecase!)
-Chris
> -----Original Message----- > From: Chris Wilson <chris@chris-wilson.co.uk> > Sent: Wednesday, August 7, 2019 8:08 AM > To: Bloomfield, Jon <jon.bloomfield@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Winiarski, Michal > <michal.winiarski@intel.com> > Subject: RE: [PATCH 5/5] drm/i915: Cancel non-persistent contexts on close > > Quoting Bloomfield, Jon (2019-08-07 15:33:51) > [skip to end] > > We didn't explore the idea of terminating orphaned contexts though > (where none of their resources are referenced by any other contexts). Is > there a reason why this is not feasible? In the case of compute (certainly > HPC) workloads, there would be no compositor taking the output so this > might be a solution. > > Sounds easier said than done. We have to go through each request and > determine it if has an external reference (or if the object holding the > reference has an external reference) to see if the output would be > visible to a third party. Sounds like a conservative GC :| > (Coming to that conclusion suggests that we should structure the request > tracking to make reparenting easier.) > > We could take a pid-1 approach and move all the orphan timelines over to > a new parent purely responsible for them. That honestly doesn't seem to > achieve anything. (We are still stuck with tasks on the GPU and no way > to kill them.) > > In comparison, persistence is a rarely used "feature" and cleaning up on > context close fits in nicely with the process model. It just works as > most users/clients would expect. (Although running in non-persistent > by default hasn't show anything to explode on the desktop, it's too easy > to construct scenarios where persistence turns out to be an advantage, > particularly with chains of clients (the compositor model).) Between the > two modes, we should have most bases covered, it's hard to argue for a > third way (that is until someone has a usecase!) > -Chris Ok, makes sense. Thanks. But have we converged on a decision :-) As I said, requiring compute umd optin should be ok for the immediate HPC issue, but I'd personally argue that it's valid to change the contract for hangcheck=0 and switch the default to non-persistent. Jon
Quoting Bloomfield, Jon (2019-08-07 16:29:55) > > -----Original Message----- > > From: Chris Wilson <chris@chris-wilson.co.uk> > > Sent: Wednesday, August 7, 2019 8:08 AM > > To: Bloomfield, Jon <jon.bloomfield@intel.com>; intel- > > gfx@lists.freedesktop.org > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Winiarski, Michal > > <michal.winiarski@intel.com> > > Subject: RE: [PATCH 5/5] drm/i915: Cancel non-persistent contexts on close > > > > Quoting Bloomfield, Jon (2019-08-07 15:33:51) > > [skip to end] > > > We didn't explore the idea of terminating orphaned contexts though > > (where none of their resources are referenced by any other contexts). Is > > there a reason why this is not feasible? In the case of compute (certainly > > HPC) workloads, there would be no compositor taking the output so this > > might be a solution. > > > > Sounds easier said than done. We have to go through each request and > > determine it if has an external reference (or if the object holding the > > reference has an external reference) to see if the output would be > > visible to a third party. Sounds like a conservative GC :| > > (Coming to that conclusion suggests that we should structure the request > > tracking to make reparenting easier.) > > > > We could take a pid-1 approach and move all the orphan timelines over to > > a new parent purely responsible for them. That honestly doesn't seem to > > achieve anything. (We are still stuck with tasks on the GPU and no way > > to kill them.) > > > > In comparison, persistence is a rarely used "feature" and cleaning up on > > context close fits in nicely with the process model. It just works as > > most users/clients would expect. (Although running in non-persistent > > by default hasn't show anything to explode on the desktop, it's too easy > > to construct scenarios where persistence turns out to be an advantage, > > particularly with chains of clients (the compositor model).) Between the > > two modes, we should have most bases covered, it's hard to argue for a > > third way (that is until someone has a usecase!) > > -Chris > > Ok, makes sense. Thanks. > > But have we converged on a decision :-) > > As I said, requiring compute umd optin should be ok for the immediate HPC issue, but I'd personally argue that it's valid to change the contract for hangcheck=0 and switch the default to non-persistent. I don't have to like it, but I think that's what we have to do for the interim 10 years or so. -Chris
Quoting Bloomfield, Jon (2019-08-07 16:29:55) > > -----Original Message----- > > From: Chris Wilson <chris@chris-wilson.co.uk> > > Sent: Wednesday, August 7, 2019 8:08 AM > > To: Bloomfield, Jon <jon.bloomfield@intel.com>; intel- > > gfx@lists.freedesktop.org > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Winiarski, Michal > > <michal.winiarski@intel.com> > > Subject: RE: [PATCH 5/5] drm/i915: Cancel non-persistent contexts on close > > > > Quoting Bloomfield, Jon (2019-08-07 15:33:51) > > [skip to end] > > > We didn't explore the idea of terminating orphaned contexts though > > (where none of their resources are referenced by any other contexts). Is > > there a reason why this is not feasible? In the case of compute (certainly > > HPC) workloads, there would be no compositor taking the output so this > > might be a solution. > > > > Sounds easier said than done. We have to go through each request and > > determine it if has an external reference (or if the object holding the > > reference has an external reference) to see if the output would be > > visible to a third party. Sounds like a conservative GC :| > > (Coming to that conclusion suggests that we should structure the request > > tracking to make reparenting easier.) > > > > We could take a pid-1 approach and move all the orphan timelines over to > > a new parent purely responsible for them. That honestly doesn't seem to > > achieve anything. (We are still stuck with tasks on the GPU and no way > > to kill them.) > > > > In comparison, persistence is a rarely used "feature" and cleaning up on > > context close fits in nicely with the process model. It just works as > > most users/clients would expect. (Although running in non-persistent > > by default hasn't show anything to explode on the desktop, it's too easy > > to construct scenarios where persistence turns out to be an advantage, > > particularly with chains of clients (the compositor model).) Between the > > two modes, we should have most bases covered, it's hard to argue for a > > third way (that is until someone has a usecase!) > > -Chris > > Ok, makes sense. Thanks. > > But have we converged on a decision :-) > > As I said, requiring compute umd optin should be ok for the immediate HPC issue, but I'd personally argue that it's valid to change the contract for hangcheck=0 and switch the default to non-persistent. Could you tender diff --git a/runtime/os_interface/linux/drm_neo.cpp b/runtime/os_interface/linux/drm_neo.cpp index 31deb68b..8a9af363 100644 --- a/runtime/os_interface/linux/drm_neo.cpp +++ b/runtime/os_interface/linux/drm_neo.cpp @@ -141,11 +141,22 @@ void Drm::setLowPriorityContextParam(uint32_t drmContextId) { UNRECOVERABLE_IF(retVal != 0); } +void setNonPersistent(uint32_t drmContextId) { + drm_i915_gem_context_param gcp = {}; + gcp.ctx_id = drmContextId; + gcp.param = 0xb; /* I915_CONTEXT_PARAM_PERSISTENCE; */ + + ioctl(DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &gcp); +} + uint32_t Drm::createDrmContext() { drm_i915_gem_context_create gcc = {}; auto retVal = ioctl(DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &gcc); UNRECOVERABLE_IF(retVal != 0); + /* enable cleanup of resources on process termination */ + setNonPersistent(gcc.ctx_id); + return gcc.ctx_id; } to interested parties? -Chris
> -----Original Message----- > From: Chris Wilson <chris@chris-wilson.co.uk> > Sent: Wednesday, August 7, 2019 9:51 AM > To: Bloomfield, Jon <jon.bloomfield@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Winiarski, Michal > <michal.winiarski@intel.com> > Subject: RE: [PATCH 5/5] drm/i915: Cancel non-persistent contexts on close > > Quoting Bloomfield, Jon (2019-08-07 16:29:55) > > > -----Original Message----- > > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > Sent: Wednesday, August 7, 2019 8:08 AM > > > To: Bloomfield, Jon <jon.bloomfield@intel.com>; intel- > > > gfx@lists.freedesktop.org > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Winiarski, Michal > > > <michal.winiarski@intel.com> > > > Subject: RE: [PATCH 5/5] drm/i915: Cancel non-persistent contexts on close > > > > > > Quoting Bloomfield, Jon (2019-08-07 15:33:51) > > > [skip to end] > > > > We didn't explore the idea of terminating orphaned contexts though > > > (where none of their resources are referenced by any other contexts). Is > > > there a reason why this is not feasible? In the case of compute (certainly > > > HPC) workloads, there would be no compositor taking the output so this > > > might be a solution. > > > > > > Sounds easier said than done. We have to go through each request and > > > determine it if has an external reference (or if the object holding the > > > reference has an external reference) to see if the output would be > > > visible to a third party. Sounds like a conservative GC :| > > > (Coming to that conclusion suggests that we should structure the request > > > tracking to make reparenting easier.) > > > > > > We could take a pid-1 approach and move all the orphan timelines over to > > > a new parent purely responsible for them. That honestly doesn't seem to > > > achieve anything. (We are still stuck with tasks on the GPU and no way > > > to kill them.) > > > > > > In comparison, persistence is a rarely used "feature" and cleaning up on > > > context close fits in nicely with the process model. It just works as > > > most users/clients would expect. (Although running in non-persistent > > > by default hasn't show anything to explode on the desktop, it's too easy > > > to construct scenarios where persistence turns out to be an advantage, > > > particularly with chains of clients (the compositor model).) Between the > > > two modes, we should have most bases covered, it's hard to argue for a > > > third way (that is until someone has a usecase!) > > > -Chris > > > > Ok, makes sense. Thanks. > > > > But have we converged on a decision :-) > > > > As I said, requiring compute umd optin should be ok for the immediate HPC > issue, but I'd personally argue that it's valid to change the contract for > hangcheck=0 and switch the default to non-persistent. > > Could you tender > > diff --git a/runtime/os_interface/linux/drm_neo.cpp > b/runtime/os_interface/linux/drm_neo.cpp > index 31deb68b..8a9af363 100644 > --- a/runtime/os_interface/linux/drm_neo.cpp > +++ b/runtime/os_interface/linux/drm_neo.cpp > @@ -141,11 +141,22 @@ void Drm::setLowPriorityContextParam(uint32_t > drmContextId) { > UNRECOVERABLE_IF(retVal != 0); > } > > +void setNonPersistent(uint32_t drmContextId) { > + drm_i915_gem_context_param gcp = {}; > + gcp.ctx_id = drmContextId; > + gcp.param = 0xb; /* I915_CONTEXT_PARAM_PERSISTENCE; */ > + > + ioctl(DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &gcp); > +} > + > uint32_t Drm::createDrmContext() { > drm_i915_gem_context_create gcc = {}; > auto retVal = ioctl(DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &gcc); > UNRECOVERABLE_IF(retVal != 0); > > + /* enable cleanup of resources on process termination */ > + setNonPersistent(gcc.ctx_id); > + > return gcc.ctx_id; > } > > to interested parties? > -Chris Yes, that's exactly what I had in mind. I think it's enough to resolve the HPC challenges.
Quoting Chris Wilson (2019-08-06 14:47:25) > Normally, we rely on our hangcheck to prevent persistent batches from > hogging the GPU. However, if the user disables hangcheck, this mechanism > breaks down. Despite our insistence that this is unsafe, the users are > equally insistent that they want to use endless batches and will disable > the hangcheck mechanism. We are looking are perhaps replacing hangcheck > with a softer mechanism, that sends a pulse down the engine to check if > it is well. We can use the same preemptive pulse to flush an active > persistent context off the GPU upon context close, preventing resources > being lost and unkillable requests remaining on the GPU, after process > termination. To avoid changing the ABI and accidentally breaking > existing userspace, we make the persistence of a context explicit and > enable it by default. Userspace can opt out of persistent mode (forcing > requests to be cancelled when the context is closed by process > termination or explicitly) by a context parameter, or to facilitate > existing use-cases by disabling hangcheck (i915.enable_hangcheck=0). > (Note, one of the outcomes for supporting endless mode will be the > removal of hangchecking, at which point opting into persistent mode will > be mandatory, or maybe the default.) For the record, I've finally run into examples of desktop clients exiting before their rendering is shown. No longer hypothetical. -Chris
> -----Original Message----- > From: Chris Wilson <chris@chris-wilson.co.uk> > Sent: Friday, August 9, 2019 4:35 PM > To: intel-gfx@lists.freedesktop.org > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Winiarski, Michal > <michal.winiarski@intel.com>; Bloomfield, Jon <jon.bloomfield@intel.com> > Subject: Re: [PATCH 5/5] drm/i915: Cancel non-persistent contexts on close > > Quoting Chris Wilson (2019-08-06 14:47:25) > > Normally, we rely on our hangcheck to prevent persistent batches from > > hogging the GPU. However, if the user disables hangcheck, this mechanism > > breaks down. Despite our insistence that this is unsafe, the users are > > equally insistent that they want to use endless batches and will disable > > the hangcheck mechanism. We are looking are perhaps replacing hangcheck > > with a softer mechanism, that sends a pulse down the engine to check if > > it is well. We can use the same preemptive pulse to flush an active > > persistent context off the GPU upon context close, preventing resources > > being lost and unkillable requests remaining on the GPU, after process > > termination. To avoid changing the ABI and accidentally breaking > > existing userspace, we make the persistence of a context explicit and > > enable it by default. Userspace can opt out of persistent mode (forcing > > requests to be cancelled when the context is closed by process > > termination or explicitly) by a context parameter, or to facilitate > > existing use-cases by disabling hangcheck (i915.enable_hangcheck=0). > > (Note, one of the outcomes for supporting endless mode will be the > > removal of hangchecking, at which point opting into persistent mode will > > be mandatory, or maybe the default.) > > For the record, I've finally run into examples of desktop clients > exiting before their rendering is shown. No longer hypothetical. > -Chris Can you share any details - Might be useful for testing.
Quoting Bloomfield, Jon (2019-08-12 15:39:33) > > -----Original Message----- > > From: Chris Wilson <chris@chris-wilson.co.uk> > > Sent: Friday, August 9, 2019 4:35 PM > > To: intel-gfx@lists.freedesktop.org > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Winiarski, Michal > > <michal.winiarski@intel.com>; Bloomfield, Jon <jon.bloomfield@intel.com> > > Subject: Re: [PATCH 5/5] drm/i915: Cancel non-persistent contexts on close > > > > Quoting Chris Wilson (2019-08-06 14:47:25) > > > Normally, we rely on our hangcheck to prevent persistent batches from > > > hogging the GPU. However, if the user disables hangcheck, this mechanism > > > breaks down. Despite our insistence that this is unsafe, the users are > > > equally insistent that they want to use endless batches and will disable > > > the hangcheck mechanism. We are looking are perhaps replacing hangcheck > > > with a softer mechanism, that sends a pulse down the engine to check if > > > it is well. We can use the same preemptive pulse to flush an active > > > persistent context off the GPU upon context close, preventing resources > > > being lost and unkillable requests remaining on the GPU, after process > > > termination. To avoid changing the ABI and accidentally breaking > > > existing userspace, we make the persistence of a context explicit and > > > enable it by default. Userspace can opt out of persistent mode (forcing > > > requests to be cancelled when the context is closed by process > > > termination or explicitly) by a context parameter, or to facilitate > > > existing use-cases by disabling hangcheck (i915.enable_hangcheck=0). > > > (Note, one of the outcomes for supporting endless mode will be the > > > removal of hangchecking, at which point opting into persistent mode will > > > be mandatory, or maybe the default.) > > > > For the record, I've finally run into examples of desktop clients > > exiting before their rendering is shown. No longer hypothetical. > > -Chris > > Can you share any details - Might be useful for testing. In cinnamon, the atl-tab switcher seems to be one. Another one seems to be around firefox, but I haven't established the trigger. I should actually log who owned the context! -Chris
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index a1016858d014..42417d87496e 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -71,9 +71,10 @@ obj-y += gt/ gt-y += \ gt/intel_breadcrumbs.o \ gt/intel_context.o \ - gt/intel_engine_pool.o \ gt/intel_engine_cs.o \ + gt/intel_engine_heartbeat.o \ gt/intel_engine_pm.o \ + gt/intel_engine_pool.o \ gt/intel_gt.o \ gt/intel_gt_pm.o \ gt/intel_hangcheck.o \ diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 64f7a533e886..5718b74f95b7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -70,6 +70,7 @@ #include <drm/i915_drm.h> #include "gt/intel_lrc_reg.h" +#include "gt/intel_engine_heartbeat.h" #include "i915_gem_context.h" #include "i915_globals.h" @@ -373,6 +374,45 @@ void i915_gem_context_release(struct kref *ref) queue_work(i915->wq, &i915->contexts.free_work); } +static void kill_context(struct i915_gem_context *ctx) +{ + struct i915_gem_engines_iter it; + struct intel_engine_cs *engine; + intel_engine_mask_t tmp, active; + struct intel_context *ce; + + if (i915_gem_context_is_banned(ctx)) + return; + + i915_gem_context_set_banned(ctx); + + active = 0; + for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) { + struct i915_request *rq; + + if (!ce->ring) + continue; + + rq = i915_active_request_get_unlocked(&ce->ring->timeline->last_request); + if (!rq) + continue; + + active |= rq->engine->mask; + i915_request_put(rq); + } + i915_gem_context_unlock_engines(ctx); + + /* + * Send a "high priority pulse" down the engine to cause the + * current request to be momentarily preempted. (If it fails to + * be preempted, it will be reset). As we have marked our context + * as banned, any incomplete request, including any running, will + * be skipped. + */ + for_each_engine_masked(engine, ctx->i915, active, tmp) + intel_engine_pulse(engine); +} + static void context_close(struct i915_gem_context *ctx) { mutex_lock(&ctx->mutex); @@ -394,6 +434,15 @@ static void context_close(struct i915_gem_context *ctx) lut_close(ctx); mutex_unlock(&ctx->mutex); + + /* + * If the user has disabled hangchecking, we can not be sure that + * the batches will ever complete and let the context be freed. + * Forcibly kill off any remaining requests in this case. + */ + if (!i915_gem_context_is_persistent(ctx)) + kill_context(ctx); + i915_gem_context_put(ctx); } @@ -433,6 +482,8 @@ __create_context(struct drm_i915_private *i915) i915_gem_context_set_bannable(ctx); i915_gem_context_set_recoverable(ctx); + if (i915_modparams.enable_hangcheck) + i915_gem_context_set_persistence(ctx); ctx->ring_size = 4 * PAGE_SIZE; @@ -1745,6 +1796,25 @@ 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; + + if (!args->value) { + i915_gem_context_clear_persistence(ctx); + return 0; + } + + if (!(ctx->i915->caps.scheduler & I915_SCHEDULER_CAP_PREEMPTION)) + return -ENODEV; + + i915_gem_context_set_persistence(ctx); + return 0; +} + static int ctx_setparam(struct drm_i915_file_private *fpriv, struct i915_gem_context *ctx, struct drm_i915_gem_context_param *args) @@ -1822,6 +1892,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; @@ -2278,6 +2352,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 106e2ccf7a4c..1834d1df2ac9 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h @@ -74,6 +74,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 a02d98494078..bf41b43ffa9a 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/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c new file mode 100644 index 000000000000..0c0632a423a7 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c @@ -0,0 +1,53 @@ +/* + * SPDX-License-Identifier: MIT + * + * Copyright © 2019 Intel Corporation + */ + +#include "i915_request.h" + +#include "intel_context.h" +#include "intel_engine_heartbeat.h" +#include "intel_engine_pm.h" +#include "intel_engine.h" +#include "intel_gt.h" + +void intel_engine_pulse(struct intel_engine_cs *engine) +{ + struct intel_context *ce = engine->kernel_context; + struct i915_sched_attr attr = { .priority = INT_MAX }; + struct i915_request *rq; + int err = 0; + + GEM_BUG_ON(!engine->schedule); + + if (!intel_engine_pm_get_if_awake(engine)) + return; + + mutex_lock(&ce->ring->timeline->mutex); + + intel_context_enter(ce); + rq = __i915_request_create(ce, GFP_NOWAIT | __GFP_NOWARN); + intel_context_exit(ce); + if (IS_ERR(rq)) { + err = PTR_ERR(rq); + goto out_unlock; + } + i915_request_get(rq); + + rq->flags |= I915_REQUEST_SENTINEL; + __i915_request_commit(rq); + + local_bh_disable(); + engine->schedule(rq, &attr); + local_bh_enable(); + + i915_request_put(rq); + +out_unlock: + mutex_unlock(&ce->ring->timeline->mutex); + intel_context_timeline_unlock(ce); + intel_engine_pm_put(engine); + if (err) /* XXX must not be allowed to fail */ + DRM_ERROR("Failed to ping %s, err=%d\n", engine->name, err); +} diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h new file mode 100644 index 000000000000..86761748dc21 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h @@ -0,0 +1,14 @@ +/* + * SPDX-License-Identifier: MIT + * + * Copyright © 2019 Intel Corporation + */ + +#ifndef INTEL_ENGINE_HEARTBEAT_H +#define INTEL_ENGINE_HEARTBEAT_H + +struct intel_engine_cs; + +void intel_engine_pulse(struct intel_engine_cs *engine); + +#endif /* INTEL_ENGINE_HEARTBEAT_H */ diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 469dc512cca3..dbc8691d75d0 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1565,6 +1565,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;
Normally, we rely on our hangcheck to prevent persistent batches from hogging the GPU. However, if the user disables hangcheck, this mechanism breaks down. Despite our insistence that this is unsafe, the users are equally insistent that they want to use endless batches and will disable the hangcheck mechanism. We are looking are perhaps replacing hangcheck with a softer mechanism, that sends a pulse down the engine to check if it is well. We can use the same preemptive pulse to flush an active persistent context off the GPU upon context close, preventing resources being lost and unkillable requests remaining on the GPU, after process termination. To avoid changing the ABI and accidentally breaking existing userspace, we make the persistence of a context explicit and enable it by default. Userspace can opt out of persistent mode (forcing requests to be cancelled when the context is closed by process termination or explicitly) by a context parameter, or to facilitate existing use-cases by disabling hangcheck (i915.enable_hangcheck=0). (Note, one of the outcomes for supporting endless mode will be the removal of hangchecking, at which point opting into persistent mode will be mandatory, or maybe the default.) 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> --- Same sort of caveats as for hangcheck, a few corner cases need struct_mutex and some preallocation. --- drivers/gpu/drm/i915/Makefile | 3 +- drivers/gpu/drm/i915/gem/i915_gem_context.c | 79 +++++++++++++++++++ drivers/gpu/drm/i915/gem/i915_gem_context.h | 15 ++++ .../gpu/drm/i915/gem/i915_gem_context_types.h | 1 + .../gpu/drm/i915/gt/intel_engine_heartbeat.c | 53 +++++++++++++ .../gpu/drm/i915/gt/intel_engine_heartbeat.h | 14 ++++ include/uapi/drm/i915_drm.h | 15 ++++ 7 files changed, 179 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c create mode 100644 drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h