Force preemption
Subsystems (2)
cover Force preemption
From: Jeff McGee <[email protected]>
Force preemption uses engine reset to enforce a limit on the time that a request targeted for preemption can block. This feature is a requirement in automotive systems where the GPU may be shared by clients of critically high priority and clients of low priority that may not have been curated to be preemption friendly. There may be more general applications of this feature. I'm sharing as an RFC to stimulate that discussion and also to get any technical feedback that I can before submitting to the product kernel that needs this. I have developed the patches for ease of rebase, given that this is for the moment considered a non-upstreamable feature. It would be possible to refactor hangcheck to fully incorporate force preemption as another tier of patience (or impatience) with the running request.
Jeff McGee (8):
drm/i915: Downgrade tasklet GEM_BUG_ON for request not completed drm/i915: Skip CSB processing on invalid CSB tail drm/i915: Execlists to mark the HWSP upon preemption finished drm/i915: Add a wait_for routine with more exact timeout drm/i915: Consider preemption when finding the active request drm/i915: Repair the preemption context if hit by reset drm/i915: Allow reset without error capture
drm/i915: Force preemption to complete via engine reset
drivers/gpu/drm/i915/i915_drv.c | 14 +++++- drivers/gpu/drm/i915/i915_drv.h | 4 ++ drivers/gpu/drm/i915/i915_gem.c | 86 ++++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/i915_irq.c | 75 ++++++++++++++++------------ drivers/gpu/drm/i915/i915_params.c | 3 ++ drivers/gpu/drm/i915/i915_params.h | 1 + drivers/gpu/drm/i915/intel_drv.h | 22 +++++++++ drivers/gpu/drm/i915/intel_engine_cs.c | 53 ++++++++++++++++++++ drivers/gpu/drm/i915/intel_lrc.c | 81 +++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_ringbuffer.h | 20 +++++++- 10 files changed, 314 insertions(+), 45 deletions(-)
Signature · 6 lines
2.16.2 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
RFC 1/8 drm/i915: Downgrade tasklet GEM_BUG_ON for request not completed
From: Jeff McGee <[email protected]>
It is possible for the hardware to be reset just as a context is completing. The reset post-processing may see the seqno update and assume that the context escaped corruption, but the context may have been disrupted in the save out process. The corruption may screw up the HEAD and TAIL pointers such that the next submission of the context switches out without running the intended request. The GEM_BUG_ON will be hit in this situation, but it is not really a driver error. So make it a GEM_WARN_ON so that we notice while letting hangcheck detect and clean up.
This patch is required to support the force preemption feature.
Test: Run IGT gem_exec_fpreempt repeatedly with
CONFIG_DRM_I915_DEBUG_GEM.
--- drivers/gpu/drm/i915/intel_lrc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_lrc.c +1 -1
@@ -920,7 +920,7 @@ static void intel_lrc_irq_handler(unsigned long data)GEM_BUG_ON (count ==0 );if (--count ==0 ) {GEM_BUG_ON (status &GEN8_CTX_STATUS_PREEMPTED ); -GEM_BUG_ON (!i915_gem_request_completed (rq)); +GEM_WARN_ON (!i915_gem_request_completed (rq));execlists_context_status_change (rq,INTEL_CONTEXT_SCHEDULE_OUT );trace_i915_gem_request_out (rq);
Signature · 6 lines
2.16.2 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [RFC 1/8] drm/i915: Downgrade tasklet GEM_BUG_ON for request not completed
Quoting [email protected] (2018-03-16 18:30:58)
Quoted 27 lines
From: Jeff McGee <[email protected]>
It is possible for the hardware to be reset just as a context is completing. The reset post-processing may see the seqno update and assume that the context escaped corruption, but the context may have been disrupted in the save out process. The corruption may screw up the HEAD and TAIL pointers such that the next submission of the context switches out without running the intended request. The
GEM_BUG_ONwill be hit in this situation, but it is not really a driver error. So make it aGEM_WARN_ONso that we notice while letting hangcheck detect and clean up.This patch is required to support the force preemption feature.
Test: Run IGT
gem_exec_fpreemptrepeatedly withCONFIG_DRM_I915_DEBUG_GEM.Change-Id I87da4b16bad805fe48153a9ed9169900681ebba7Signed-off-by Jeff McGee <[email protected]>--- drivers/gpu/drm/i915/intel_lrc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/i915/intel_lrc.c +0
-0
drivers/gpu/drm/i915/intel_lrc.c static void intel_lrc_irq_handler(unsigned long data) · line 923
Quoted 5 lines
drivers/gpu/drm/i915/intel_lrc.c +1
-1@@ -920,7 +920,7 @@ static void intel_lrc_irq_handler(unsigned long data)GEM_BUG_ON (count ==0 );if (--count ==0 ) {GEM_BUG_ON (status &GEN8_CTX_STATUS_PREEMPTED ); -GEM_BUG_ON (!i915_gem_request_completed (rq)); +GEM_WARN_ON (!i915_gem_request_completed (rq));
Nope, either way you broke pretty badly.
-Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
RFC 2/8 drm/i915: Skip CSB processing on invalid CSB tail
In reply to [email protected]From: Jeff McGee <[email protected]>
Engine reset is fast. A context switch interrupt may be generated just prior to the reset such that the top half handler is racing with reset post-processing. The handler may set the irq_posted bit again after the reset code has cleared it to start fresh. Then the re-enabled tasklet will read the CSB head and tail from MMIO, which will be at the hardware reset values of 0 and 7 respectively, given that no actual CSB event has occurred since the reset. Mayhem then ensues as the tasklet starts processing invalid CSB entries.
We can handle this corner case without adding any new synchronization between the irq top half and the reset work item. The tasklet can just skip CSB processing if the tail is not sane.
This patch is required to support the force preemption feature.
Test: Run IGT gem_exec_fpreempt repeatedly.
--- drivers/gpu/drm/i915/intel_lrc.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/gpu/drm/i915/intel_lrc.c +9 -0
@@ -851,6 +851,14 @@ static void intel_lrc_irq_handler(unsigned long data) head =readl (dev_priv->regs +i915_mmio_reg_offset (RING_CONTEXT_STATUS_PTR (engine))); tail =GEN8_CSB_WRITE_PTR (head); head =GEN8_CSB_READ_PTR (head); + +/* The MMIO read CSB tail may be at the reset value of +* 0x7 if there hasn't been a valid CSB event since +* the engine reset. Skip on to dequeue if so. +*/ +if (tail >=GEN8_CSB_ENTRIES ) +break ; + execlists->csb_head = head; }else { const int write_idx =
@@ -859,6 +867,7 @@ static void intel_lrc_irq_handler(unsigned long data) head = execlists->csb_head; tail =READ_ONCE (buf[write_idx]); +GEM_BUG_ON (tail >=GEN8_CSB_ENTRIES ); }while (head != tail) {
Signature · 6 lines
2.16.2 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [RFC 2/8] drm/i915: Skip CSB processing on invalid CSB tail
Quoting [email protected] (2018-03-16 18:30:59)
Quoted 11 lines
From: Jeff McGee <[email protected]>
Engine reset is fast. A context switch interrupt may be generated just prior to the reset such that the top half handler is racing with reset post-processing. The handler may set the
irq_postedbit again after the reset code has cleared it to start fresh. Then the re-enabled tasklet will read the CSB head and tail from MMIO, which will be at the hardware reset values of 0 and 7 respectively, given that no actual CSB event has occurred since the reset. Mayhem then ensues as the tasklet starts processing invalid CSB entries.
Quoted 5 lines
We can handle this corner case without adding any new synchronization between the irq top half and the reset work item. The tasklet can just skip CSB processing if the tail is not sane.
This patch is required to support the force preemption feature.
Please note how this is handled currently, because this is not a new problem.
-Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
RFC 3/8 drm/i915: Execlists to mark the HWSP upon preemption finished
In reply to [email protected]From: Jeff McGee <[email protected]>
In the next patch we are improving how we find the active request on an engine with a pending preemption. We need to know if the preemption has completed or not. This determination can be made most robustly by having the preemption context write a preemption finished indicator to the hardware status page.
This patch is required to support the force preemption feature.
--- drivers/gpu/drm/i915/intel_lrc.c | 27 ++++++++++++++++++++++++--- drivers/gpu/drm/i915/intel_ringbuffer.h | 14 +++++++++++++- 2 files changed, 37 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/intel_lrc.c +24 -3
@@ -512,19 +512,37 @@ static void port_assign(struct execlist_port *port,port_set (port,port_pack (i915_gem_request_get (rq),port_count (port))); } +#define PREEMPT_BREADCRUMB_DWORDS 0x8static void inject_preempt_context (struct intel_engine_cs *engine) {struct intel_context *ce = &engine->i915->preempt_context->engine[engine->id]; +u32 *cs = ce->ring->vaddr + ce->ring->tail;u32 __iomem *elsp = engine->i915->regs +i915_mmio_reg_offset (RING_ELSP (engine));unsigned int n;GEM_BUG_ON (engine->i915->preempt_context->hw_id !=PREEMPT_ID ); -GEM_BUG_ON (!IS_ALIGNED (ce->ring->size,WA_TAIL_BYTES )); +GEM_BUG_ON (intel_engine_preempt_finished (engine)); -memset (ce->ring->vaddr + ce->ring->tail,0 ,WA_TAIL_BYTES ); - ce->ring->tail +=WA_TAIL_BYTES ; +if (engine->id ==RCS ) { + cs =gen8_emit_ggtt_write_rcs (cs,I915_GEM_HWS_PREEMPT_FINISHED , +intel_hws_preempt_done_address (engine)); + }else { + cs =gen8_emit_ggtt_write (cs,I915_GEM_HWS_PREEMPT_FINISHED , +intel_hws_preempt_done_address (engine)); + *cs++ =MI_NOOP ; + *cs++ =MI_NOOP ; + } + *cs++ =MI_NOOP ; + *cs++ =MI_NOOP ; + +GEM_BUG_ON (!IS_ALIGNED (ce->ring->size, +PREEMPT_BREADCRUMB_DWORDS *sizeof (u32))); +GEM_BUG_ON ((void *)cs - (ce->ring->vaddr + ce->ring->tail) != +PREEMPT_BREADCRUMB_DWORDS *sizeof (u32)); + + ce->ring->tail +=PREEMPT_BREADCRUMB_DWORDS *sizeof (u32); ce->ring->tail &= (ce->ring->size -1 ); ce->lrc_reg_state[CTX_RING_TAIL +1 ] = ce->ring->tail;
@@ -911,6 +929,8 @@ static void intel_lrc_irq_handler(unsigned long data) EXECLISTS_ACTIVE_PREEMPT));execlists_clear_active (execlists,EXECLISTS_ACTIVE_PREEMPT ); +GEM_BUG_ON (!intel_engine_preempt_finished (engine)); +intel_engine_clear_preempt (engine);continue ; }
@@ -1507,6 +1527,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine) execlists->csb_head =-1 ; execlists->active =0 ; +intel_engine_clear_preempt (engine);/* After a GPU reset, we may have requests to replay */ if (!i915_modparams.enable_guc_submission && execlists->first)
drivers/gpu/drm/i915/intel_ringbuffer.h +13 -1
@@ -625,11 +625,12 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)#define I915_GEM_HWS_INDEX_ADDR (I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT)#define I915_GEM_HWS_PREEMPT_INDEX 0x32#define I915_GEM_HWS_PREEMPT_ADDR (I915_GEM_HWS_PREEMPT_INDEX << MI_STORE_DWORD_INDEX_SHIFT) +#define I915_GEM_HWS_PREEMPT_FINISHED 1#define I915_GEM_HWS_PID_INDEX 0x40#define I915_GEM_HWS_PID_ADDR (I915_GEM_HWS_PID_INDEX << MI_STORE_DWORD_INDEX_SHIFT)#define I915_GEM_HWS_CID_INDEX 0x48#define I915_GEM_HWS_CID_ADDR (I915_GEM_HWS_CID_INDEX << MI_STORE_DWORD_INDEX_SHIFT) -#define I915_GEM_HWS_SCRATCH_INDEX 0x50 +#define I915_GEM_HWS_SCRATCH_INDEX 0x58#define I915_GEM_HWS_SCRATCH_ADDR (I915_GEM_HWS_SCRATCH_INDEX << MI_STORE_DWORD_INDEX_SHIFT)#define I915_HWS_CSB_BUF0_INDEX 0x10
@@ -749,6 +750,17 @@ static inline u32 intel_engine_get_seqno(struct intel_engine_cs *engine)return intel_read_status_page (engine,I915_GEM_HWS_INDEX ); } +static inline bool intel_engine_preempt_finished (struct intel_engine_cs *engine) +{ +return (intel_read_status_page (engine,I915_GEM_HWS_PREEMPT_INDEX ) == +I915_GEM_HWS_PREEMPT_FINISHED ); +} + +static inline void intel_engine_clear_preempt (struct intel_engine_cs *engine) +{ +intel_write_status_page (engine,I915_GEM_HWS_PREEMPT_INDEX ,0 ); +} +static inline u32 intel_engine_last_submit (struct intel_engine_cs *engine) {/* We are only peeking at the tail of the submit queue (and not the
Signature · 6 lines
2.16.2 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
RFC 4/8 drm/i915: Add a wait_for routine with more exact timeout
In reply to [email protected]From: Jeff McGee <[email protected]>
The current non-atomic wait_for routines convert the provided usec timeout into jiffies with upward rounding. This can increase the actual timeout several msecs which is fine in most cases. In the next patch we need the timeout to conform more exactly to what is requested.
This patch is required to support the force preemption feature.
--- drivers/gpu/drm/i915/intel_drv.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
drivers/gpu/drm/i915/intel_drv.h +22 -0
@@ -75,6 +75,28 @@ (Wmax))#define wait_for (COND ,MS ) _wait_for((COND), (MS) * 1000, 10, 1000) +#define _wait_for_exact(COND, US, Wmin, Wmax) ({ \ + u64 timeout__ = local_clock() + (US) * 1000; \ + long wait__ = (Wmin); /* recommended min for usleep is 10 us */ \ + int ret__; \ + might_sleep(); \ + for (;;) { \ + bool expired__ = (local_clock() > timeout__); \ + if (COND) { \ + ret__ = 0; \ + break; \ + } \ + if (expired__) { \ + ret__ = -ETIMEDOUT; \ + break; \ + } \ + usleep_range(wait__, wait__ * 2); \ + if (wait__ < (Wmax)) \ + wait__ <<= 1; \ + } \ + ret__; \ +}) +/* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */ #if defined(CONFIG_DRM_I915_DEBUG ) && defined(CONFIG_PREEMPT_COUNT )# define _WAIT_FOR_ATOMIC_CHECK (ATOMIC ) WARN_ON_ONCE((ATOMIC) && !in_atomic())
Signature · 6 lines
2.16.2 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
RFC 5/8 drm/i915: Consider preemption when finding the active request
In reply to [email protected]From: Jeff McGee <[email protected]>
The active request is found by scanning the engine timeline for the request that follows the last completed request. That method is accurate if there is no preemption in progress, because the engine will certainly have started that request. If there is a preemption in progress, it could have completed leaving the engine idle. In this case the request we identified with the above method is not active and may not even have been started. We must check for this condition to avoid fingering an innocent request during reset.
This patch is required to support the force preemption feature.
Test: Run IGT gem_exec_fpreempt repeatedly.
--- drivers/gpu/drm/i915/i915_gem.c | 49 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 45 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_gem.c +45 -4
@@ -2739,9 +2739,9 @@ static void i915_gem_context_mark_innocent(struct i915_gem_context *ctx) }struct drm_i915_gem_request * -i915_gem_find_active_request (struct intel_engine_cs *engine) +i915_gem_find_pending_request (struct intel_engine_cs *engine) { -struct drm_i915_gem_request *request, *active =NULL ; +struct drm_i915_gem_request *request, *pending =NULL ;unsigned long flags;/* We are called by the error capture and reset at a random
@@ -2762,12 +2762,53 @@ i915_gem_find_active_request(struct intel_engine_cs *engine)GEM_BUG_ON (test_bit (DMA_FENCE_FLAG_SIGNALED_BIT , &request->fence.flags)); - active = request; + pending = request;break ; }spin_unlock_irqrestore (&engine->timeline->lock, flags); -return active; +return pending; +} + +struct drm_i915_gem_request * +i915_gem_find_active_request (struct intel_engine_cs *engine) +{ +struct intel_engine_execlists *const execlists = &engine->execlists; +struct drm_i915_gem_request *request; + +/* The pending request is active if no preemption is in progress */ +if (!execlists_is_active (execlists,EXECLISTS_ACTIVE_PREEMPT )) +return i915_gem_find_pending_request (engine); + +/* Preemption has finished. Engine idle. */ +if (intel_engine_preempt_finished (engine)) +return NULL ; + + request =i915_gem_find_pending_request (engine); + +/* Preemption has flushed all requests off the engine. Engine idle. */ +if (!request) +return NULL ; + +/* The pending request likely is active and blocking the in-progress +* preemption. But there is a race in our previous checks. The request +* that was actually blocking preemption could have completed (a batch- +* boundary preemption) such that the engine is idle and the pending +* request we have identified was the next in line. We must wait for +* at least as long as it would take for the preempt-to-idle context +* to mark the preemption done to verify this. We use 500 usecs to +* account for a worst case delay from the seqno write of the +* completing request and the preempt finished write. +*/ +if (!_wait_for_exact (intel_engine_preempt_finished (engine),500 ,10 ,50 )) +return NULL ; + +/* +* We didn't see the preemption done after a sufficient wait. Thus the +* pending request we sampled above was in fact active and blocking +* the preemption. +*/ +return request; }static bool engine_stalled (struct intel_engine_cs *engine)
Signature · 6 lines
2.16.2 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
RFC 6/8 drm/i915: Repair the preemption context if hit by reset
In reply to [email protected]From: Jeff McGee <[email protected]>
It is possible for the preemption context to be active on an engine when the engine is reset. The preemption context is not tracked via the request mechanism, so the normal reset handling will not detect this scenario and will not be able to fixup possible context corruption. So add some extra logic to do this.
This patch is required to support the force preemption feature.
Test: Run IGT gem_exec_fpreempt repeatedly.
--- drivers/gpu/drm/i915/intel_lrc.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
drivers/gpu/drm/i915/intel_lrc.c +19 -0
@@ -1619,6 +1619,25 @@ static void reset_common_ring(struct intel_engine_cs *engine,spin_unlock_irqrestore (&engine->timeline->lock, flags); +/* If a preemption was pending when the reset occurred, and no +* active request was found when the reset completed, it is +* possible that the preemption context was hit by the reset. +* We must assume that the context is corrupted so repair it. +*/ +if (execlists_is_active (execlists,EXECLISTS_ACTIVE_PREEMPT ) && + !request) { +struct i915_gem_context *ctx = engine->i915->preempt_context; + ce = &ctx->engine[engine->id]; + +execlists_init_reg_state (ce->lrc_reg_state, + ctx, engine, ce->ring); + ce->lrc_reg_state[CTX_RING_BUFFER_START +1 ] = +i915_ggtt_offset (ce->ring->vma); + ce->lrc_reg_state[CTX_RING_HEAD +1 ] = ce->ring->tail; + +return ; + } +/* If the request was innocent, we leave the request in the ELSP * and will try to replay it on restarting. The context image may * have been corrupted by the reset, in which case we may have
Signature · 6 lines
2.16.2 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
RFC 7/8 drm/i915: Allow reset without error capture
In reply to [email protected]From: Jeff McGee <[email protected]>
Pull the reset handling out of i915_handle_error() so that it can be called by that function and directly by the upcoming force preemption handler. This allows the force preemption handler to bypass the error capture that i915_handle_error() does before getting on with the reset. We do not want error capture for force preemption because it adds significant latency (~10 msecs measured on APL).
This patch is required to support the force preemption feature.
--- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_irq.c | 75 +++++++++++++++++++++++------------------ 2 files changed, 45 insertions(+), 32 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +2 -0
@@ -3245,6 +3245,8 @@ __printf(3, 4)void i915_handle_error (struct drm_i915_private *dev_priv,u32 engine_mask,const char *fmt, ...); +void i915_handle_reset (struct drm_i915_private *dev_priv, +u32 engine_mask);extern void intel_irq_init (struct drm_i915_private *dev_priv);extern void intel_irq_fini (struct drm_i915_private *dev_priv);
drivers/gpu/drm/i915/i915_irq.c +43 -32
106 lines not on this page · raw message
Signature · 6 lines
2.16.2 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [RFC 7/8] drm/i915: Allow reset without error capture
Quoting [email protected] (2018-03-16 18:31:04)
Quoted 8 lines
From: Jeff McGee <[email protected]>
Pull the reset handling out of
i915_handle_error()so that it can be called by that function and directly by the upcoming force preemption handler. This allows the force preemption handler to bypass the error capture thati915_handle_error()does before getting on with the reset. We do not want error capture for force preemption because it adds significant latency (~10 msecs measured on APL).
And we don't need to capture the error in many other cases; just add a control flag.
-Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
RFC 8/8 drm/i915: Force preemption to complete via engine reset
In reply to [email protected]From: Jeff McGee <[email protected]>
The hardware can complete the requested preemption at only certain points in execution. Thus an uncooperative request that avoids those points can block a preemption indefinitely. Our only option to bound the preemption latency is to trigger reset and recovery just as we would if a request had hung the hardware. This is so-called forced preemption. This change adds that capability as an option for systems with extremely strict scheduling latency requirements for its high priority requests. This option must be used with great care. The force-preempted requests will be discarded at the point of reset, resulting in various degrees of disruption to the owning application up to and including crash.
The option is enabled by specifying a non-zero value for new i915 module parameter fpreempt_timeout. This value becomes the time in milliseconds after initiation of preemption at which the reset is triggered if the preemption has not completed normally.
Test: Run IGT gem_exec_fpreempt.
--- drivers/gpu/drm/i915/i915_drv.c | 14 ++++++++- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_gem.c | 37 +++++++++++++++++++++-- drivers/gpu/drm/i915/i915_params.c | 3 ++ drivers/gpu/drm/i915/i915_params.h | 1 + drivers/gpu/drm/i915/intel_engine_cs.c | 53 +++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_lrc.c | 24 ++++++++++++++- drivers/gpu/drm/i915/intel_ringbuffer.h | 6 ++++ 8 files changed, 136 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_drv.c +13 -1
@@ -811,8 +811,16 @@ static int i915_workqueues_init(struct drm_i915_private *dev_priv)if (dev_priv->hotplug.dp_wq ==NULL ) goto out_free_wq; +if (INTEL_INFO (dev_priv)->has_logical_ring_preemption) { + dev_priv->fpreempt_wq =alloc_ordered_workqueue ("i915-fpreempt" , +WQ_HIGHPRI ); +if (dev_priv->fpreempt_wq ==NULL ) + goto out_free_dp_wq; + }return 0 ; +out_free_dp_wq: +destroy_workqueue (dev_priv->hotplug.dp_wq); out_free_wq:destroy_workqueue (dev_priv->wq); out_err:
@@ -832,6 +840,8 @@ static void i915_engines_cleanup(struct drm_i915_private *i915)static void i915_workqueues_cleanup (struct drm_i915_private *dev_priv) { +if (INTEL_INFO (dev_priv)->has_logical_ring_preemption) +destroy_workqueue (dev_priv->fpreempt_wq);destroy_workqueue (dev_priv->hotplug.dp_wq);destroy_workqueue (dev_priv->wq); }
@@ -2007,7 +2017,9 @@ int i915_reset_engine(struct intel_engine_cs *engine, unsigned int flags)if (!(flags &I915_RESET_QUIET )) {dev_notice (engine->i915->drm.dev, -"Resetting %s after gpu hang\n" , engine->name); +"Resetting %s %s\n" , engine->name, + engine->fpreempt_active ? +"for force preemption" :"after gpu hang" ); } error->reset_engine_count[engine->id]++;
drivers/gpu/drm/i915/i915_drv.h +2 -0
@@ -2290,6 +2290,8 @@ struct drm_i915_private {*/ struct workqueue_struct *wq; +struct workqueue_struct *fpreempt_wq; +/* Display functions */ struct drm_i915_display_funcs display;
drivers/gpu/drm/i915/i915_gem.c +35 -2
@@ -2811,9 +2811,21 @@ i915_gem_find_active_request(struct intel_engine_cs *engine)return request; } -static bool engine_stalled (struct intel_engine_cs *engine) +static bool engine_stalled (struct intel_engine_cs *engine, +struct drm_i915_gem_request *request) {if (!intel_vgpu_active (engine->i915)) { +if (engine->fpreempt_active) { +/* Pardon the request if it managed to complete or +* preempt prior to the reset. +*/ +if (i915_gem_request_completed (request) || +intel_engine_preempt_finished (engine)) +return false; + +return true; + } +if (!engine->hangcheck.stalled)return false;
@@ -2858,6 +2870,13 @@ i915_gem_reset_prepare_engine(struct intel_engine_cs *engine)tasklet_kill (&engine->execlists.irq_tasklet);tasklet_disable (&engine->execlists.irq_tasklet); +/* There may be a force preemption timer active on this engine but +* not yet expired, i.e. not the reason we are about to reset this +* engine. Cancel it. If force preemption timeout is the reason we +* are resetting the engine, this call will have no efffect. +*/ +intel_engine_cancel_fpreempt (engine); +if (engine->irq_seqno_barrier) engine->irq_seqno_barrier (engine);
@@ -2958,7 +2977,7 @@ i915_gem_reset_request(struct intel_engine_cs *engine,* subsequent hangs. */ -if (engine_stalled (engine)) { +if (engine_stalled (engine, request)) {i915_gem_context_mark_guilty (request->ctx);skip_request (request);
@@ -2966,6 +2985,13 @@ i915_gem_reset_request(struct intel_engine_cs *engine,if (i915_gem_context_is_banned (request->ctx))engine_skip_context (request); }else { +/* If the request that we just pardoned was the target of a +* force preemption there is no possibility of the next +* request in line having started. +*/ +if (engine->fpreempt_active) +return NULL ; +/* * Since this is not the hung engine, it may have advanced * since the hang declaration. Double check by refinding
@@ -3035,6 +3061,13 @@ void i915_gem_reset(struct drm_i915_private *dev_priv)void i915_gem_reset_finish_engine (struct intel_engine_cs *engine) { +/* Mark any active force preemption as complete then kick +* the tasklet. +*/ + engine->fpreempt_active = false; +if (engine->execlists.first) +tasklet_schedule (&engine->execlists.irq_tasklet); +tasklet_enable (&engine->execlists.irq_tasklet);kthread_unpark (engine->breadcrumbs.signaler); }
drivers/gpu/drm/i915/i915_params.c +3 -0
@@ -247,3 +247,6 @@ MODULE_PARM_DESC(enable_conformance_check, "To toggle the GVT guest conformancemodule_param_named (disable_gvt_fw_loading, i915_modparams.disable_gvt_fw_loading, bool,0400 );MODULE_PARM_DESC (disable_gvt_fw_loading,"Disable GVT-g fw loading." ); + +i915_param_named (fpreempt_timeout, uint,0600 , +"Wait time in msecs before forcing a preemption with reset (0:never force [default])" );
drivers/gpu/drm/i915/i915_params.h +1 -0
@@ -54,6 +54,7 @@func (int, edp_vswing); \func (int, reset); \ func(unsigned int, inject_load_failure); \ + func(unsigned int, fpreempt_timeout); \/* leave bools at the end to not create holes */ \func (bool, alpha_support); \func (bool, enable_cmd_parser); \
drivers/gpu/drm/i915/intel_engine_cs.c +53 -0
65 lines not on this page · raw message
drivers/gpu/drm/i915/intel_lrc.c +23 -1
@@ -628,6 +628,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)inject_preempt_context (engine);execlists_set_active (execlists,EXECLISTS_ACTIVE_PREEMPT ); +intel_engine_queue_fpreempt (engine); goto unlock; }else {/*
@@ -846,6 +847,7 @@ static void intel_lrc_irq_handler(unsigned long data)const u32 *buf = &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX ];unsigned int head, tail; +bool defer = false;/* However GVT emulation depends upon intercepting CSB mmio */ if (unlikely (execlists->csb_use_mmio)) {
@@ -919,6 +921,21 @@ static void intel_lrc_irq_handler(unsigned long data)if (status &GEN8_CTX_STATUS_ACTIVE_IDLE && buf[2 *head +1 ] ==PREEMPT_ID ) { +/* Try to cancel any pending force preemption. +* If we are too late, hold off on processing +* the completed preemption until reset has +* run its course. It should recognize that +* the engine has preempted to idle then abort +* the reset. Then we can resume processing +* at this CSB head. +*/ +if (!intel_engine_cancel_fpreempt (engine)) { +if (!head--) + head =GEN8_CSB_ENTRIES -1 ; + defer = true; +break ; + } +execlist_cancel_port_requests (execlists);spin_lock_irq (&engine->timeline->lock);
@@ -973,11 +990,16 @@ static void intel_lrc_irq_handler(unsigned long data)writel (_MASKED_FIELD (GEN8_CSB_READ_PTR_MASK , head <<8 ), dev_priv->regs +i915_mmio_reg_offset (RING_CONTEXT_STATUS_PTR (engine))); } + +if (defer) { +__set_bit (ENGINE_IRQ_EXECLIST , &engine->irq_posted); + goto out; + } }if (!execlists_is_active (execlists,EXECLISTS_ACTIVE_PREEMPT ))execlists_dequeue (engine); - +out:intel_uncore_forcewake_put (dev_priv, execlists->fw_domains); }
drivers/gpu/drm/i915/intel_ringbuffer.h +6 -0
@@ -506,6 +506,10 @@ struct intel_engine_cs {struct intel_engine_hangcheck hangcheck; +struct hrtimer fpreempt_timer; +struct work_struct fpreempt_work; +bool fpreempt_active; +bool needs_cmd_parser;/*
@@ -932,4 +936,6 @@ void intel_engines_reset_default_submission(struct drm_i915_private *i915);bool intel_engine_can_store_dword (struct intel_engine_cs *engine); +void intel_engine_queue_fpreempt (struct intel_engine_cs *engine); +bool intel_engine_cancel_fpreempt (struct intel_engine_cs *engine);#endif /* _INTEL_RINGBUFFER_H_ */
Signature · 6 lines
2.16.2 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [RFC 8/8] drm/i915: Force preemption to complete via engine reset
Quoting [email protected] (2018-03-16 18:31:05)
Quoted 37 lines
From: Jeff McGee <[email protected]>
The hardware can complete the requested preemption at only certain points in execution. Thus an uncooperative request that avoids those points can block a preemption indefinitely. Our only option to bound the preemption latency is to trigger reset and recovery just as we would if a request had hung the hardware. This is so-called forced preemption. This change adds that capability as an option for systems with extremely strict scheduling latency requirements for its high priority requests. This option must be used with great care. The force-preempted requests will be discarded at the point of reset, resulting in various degrees of disruption to the owning application up to and including crash.
The option is enabled by specifying a non-zero value for new i915 module parameter
fpreempt_timeout. This value becomes the time in milliseconds after initiation of preemption at which the reset is triggered if the preemption has not completed normally.Test: Run IGT
gem_exec_fpreempt.Change-Id Iafd3609012621c57fa9e490dfeeac46ae541b5c2Signed-off-by Jeff McGee <[email protected]>--- drivers/gpu/drm/i915/i915_drv.c | 14 ++++++++- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_gem.c | 37 +++++++++++++++++++++-- drivers/gpu/drm/i915/i915_params.c | 3 ++ drivers/gpu/drm/i915/i915_params.h | 1 + drivers/gpu/drm/i915/intel_engine_cs.c | 53 +++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_lrc.c | 24 ++++++++++++++- drivers/gpu/drm/i915/intel_ringbuffer.h | 6 ++++ 8 files changed, 136 insertions(+), 4 deletions(-)drivers/gpu/drm/i915/i915_drv.c +0
-0
drivers/gpu/drm/i915/i915_drv.c static int i915_workqueues_init(struct drm_i915_private *dev_priv) · line 818
Quoted 8 lines
drivers/gpu/drm/i915/i915_drv.c +5
-0@@ -811,8 +811,16 @@ static int i915_workqueues_init(struct drm_i915_private *dev_priv)if (dev_priv->hotplug.dp_wq ==NULL ) goto out_free_wq; +if (INTEL_INFO (dev_priv)->has_logical_ring_preemption) { + dev_priv->fpreempt_wq =alloc_ordered_workqueue ("i915-fpreempt" , +WQ_HIGHPRI ); +if (dev_priv->fpreempt_wq ==NULL ) + goto out_free_dp_wq;
Doesn't require ordering, the resets are if either required to fall back to a full reset.
Quoted 52 lines
drivers/gpu/drm/i915/i915_drv.c +8
-1+ }return 0 ; +out_free_dp_wq: +destroy_workqueue (dev_priv->hotplug.dp_wq); out_free_wq:destroy_workqueue (dev_priv->wq); out_err:@@ -832,6 +840,8 @@ static void i915_engines_cleanup(struct drm_i915_private *i915)static void i915_workqueues_cleanup (struct drm_i915_private *dev_priv) { +if (INTEL_INFO (dev_priv)->has_logical_ring_preemption) +destroy_workqueue (dev_priv->fpreempt_wq);destroy_workqueue (dev_priv->hotplug.dp_wq);destroy_workqueue (dev_priv->wq); }@@ -2007,7 +2017,9 @@ int i915_reset_engine(struct intel_engine_cs *engine, unsigned int flags)if (!(flags &I915_RESET_QUIET )) {dev_notice (engine->i915->drm.dev, -"Resetting %s after gpu hang\n" , engine->name); +"Resetting %s %s\n" , engine->name, + engine->fpreempt_active ? +"for force preemption" :"after gpu hang" ); } error->reset_engine_count[engine->id]++;drivers/gpu/drm/i915/i915_drv.h +2
-0@@ -2290,6 +2290,8 @@ struct drm_i915_private {*/ struct workqueue_struct *wq; +struct workqueue_struct *fpreempt_wq; +/* Display functions */ struct drm_i915_display_funcs display;drivers/gpu/drm/i915/i915_gem.c +2
-1@@ -2811,9 +2811,21 @@ i915_gem_find_active_request(struct intel_engine_cs *engine)return request; } -static bool engine_stalled (struct intel_engine_cs *engine) +static bool engine_stalled (struct intel_engine_cs *engine, +struct drm_i915_gem_request *request)
drivers/gpu/drm/i915/i915_gem.c i915_gem_find_active_request(struct intel_engine_cs *engine) · line 2825
Quoted 10 lines
drivers/gpu/drm/i915/i915_gem.c +8
-0{if (!intel_vgpu_active (engine->i915)) { +if (engine->fpreempt_active) { +/* Pardon the request if it managed to complete or +* preempt prior to the reset. +*/ +if (i915_gem_request_completed (request) || +intel_engine_preempt_finished (engine)) +return false; +
No, once you pull the trigger just go through with it. You should never pull the trigger if you are worried about the consequences.
Quoted 7 lines
drivers/gpu/drm/i915/i915_gem.c +3
-0+return true; + } +if (!engine->hangcheck.stalled)return false;
drivers/gpu/drm/i915/i915_gem.c i915_gem_reset_prepare_engine(struct intel_engine_cs *engine) · line 2877
Quoted 8 lines
drivers/gpu/drm/i915/i915_gem.c +5
-0@@ -2858,6 +2870,13 @@ i915_gem_reset_prepare_engine(struct intel_engine_cs *engine)tasklet_kill (&engine->execlists.irq_tasklet);tasklet_disable (&engine->execlists.irq_tasklet); +/* There may be a force preemption timer active on this engine but +* not yet expired, i.e. not the reason we are about to reset this +* engine. Cancel it. If force preemption timeout is the reason we +* are resetting the engine, this call will have no efffect. +*/
Done by just clearing execlists->active.
Quoted 29 lines
drivers/gpu/drm/i915/i915_gem.c +10
-1+intel_engine_cancel_fpreempt (engine); +if (engine->irq_seqno_barrier) engine->irq_seqno_barrier (engine);@@ -2958,7 +2977,7 @@ i915_gem_reset_request(struct intel_engine_cs *engine,* subsequent hangs. */ -if (engine_stalled (engine)) { +if (engine_stalled (engine, request)) {i915_gem_context_mark_guilty (request->ctx);skip_request (request);@@ -2966,6 +2985,13 @@ i915_gem_reset_request(struct intel_engine_cs *engine,if (i915_gem_context_is_banned (request->ctx))engine_skip_context (request); }else { +/* If the request that we just pardoned was the target of a +* force preemption there is no possibility of the next +* request in line having started. +*/ +if (engine->fpreempt_active) +return NULL ; +/* * Since this is not the hung engine, it may have advanced * since the hang declaration. Double check by refinding
drivers/gpu/drm/i915/i915_gem.c void i915_gem_reset(struct drm_i915_private *dev_priv) · line 3069
Quoted 9 lines
drivers/gpu/drm/i915/i915_gem.c +6
-0@@ -3035,6 +3061,13 @@ void i915_gem_reset(struct drm_i915_private *dev_priv)void i915_gem_reset_finish_engine (struct intel_engine_cs *engine) { +/* Mark any active force preemption as complete then kick +* the tasklet. +*/ + engine->fpreempt_active = false; +if (engine->execlists.first) +tasklet_schedule (&engine->execlists.irq_tasklet);
Stray chunk.
Quoted 53 lines
drivers/gpu/drm/i915/i915_gem.c +1
-0+tasklet_enable (&engine->execlists.irq_tasklet);kthread_unpark (engine->breadcrumbs.signaler); }drivers/gpu/drm/i915/i915_params.c +3
-0@@ -247,3 +247,6 @@ MODULE_PARM_DESC(enable_conformance_check, "To toggle the GVT guest conformancemodule_param_named (disable_gvt_fw_loading, i915_modparams.disable_gvt_fw_loading, bool,0400 );MODULE_PARM_DESC (disable_gvt_fw_loading,"Disable GVT-g fw loading." ); + +i915_param_named (fpreempt_timeout, uint,0600 , +"Wait time in msecs before forcing a preemption with reset (0:never force [default])" );drivers/gpu/drm/i915/i915_params.h +1
-0@@ -54,6 +54,7 @@func (int, edp_vswing); \func (int, reset); \ func(unsigned int, inject_load_failure); \ + func(unsigned int, fpreempt_timeout); \/* leave bools at the end to not create holes */ \func (bool, alpha_support); \func (bool, enable_cmd_parser); \drivers/gpu/drm/i915/intel_engine_cs.c +18
-0@@ -411,6 +411,58 @@ static void intel_engine_init_execlist(struct intel_engine_cs *engine) execlists->first =NULL ; } +void intel_engine_queue_fpreempt (struct intel_engine_cs *engine) +{ +unsigned int timeout = i915_modparams.fpreempt_timeout; + +if (!timeout) +return ; + +GEM_BUG_ON (engine->fpreempt_active); +hrtimer_start (&engine->fpreempt_timer, +ms_to_ktime (timeout),HRTIMER_MODE_REL ); +} + +bool intel_engine_cancel_fpreempt (struct intel_engine_cs *engine) +{ +hrtimer_cancel (&engine->fpreempt_timer); + +return !engine->fpreempt_active; +}
drivers/gpu/drm/i915/intel_engine_cs.c static void intel_engine_init_execlist(struct intel_engine_cs *engine) · line 441
Quoted 10 lines
drivers/gpu/drm/i915/intel_engine_cs.c +10
-0+ +static enum hrtimer_restart +intel_engine_fpreempt_timer (struct hrtimer *hrtimer) +{ +struct intel_engine_cs *engine = + container_of(hrtimer, struct intel_engine_cs, + fpreempt_timer); + + engine->fpreempt_active = true; +queue_work (engine->i915->fpreempt_wq, &engine->fpreempt_work);
Ah I see you didn't check.
Quoted 12 lines
drivers/gpu/drm/i915/intel_engine_cs.c +12
-0+ +return HRTIMER_NORESTART ; +} + +static void intel_engine_fpreempt_work (struct work_struct *work) +{ +struct intel_engine_cs *engine = + container_of(work, struct intel_engine_cs, + fpreempt_work); + +i915_handle_reset (engine->i915,intel_engine_flag (engine)); +}
drivers/gpu/drm/i915/intel_engine_cs.c static void intel_engine_init_execlist(struct intel_engine_cs *engine) · line 463
Quoted 10 lines
drivers/gpu/drm/i915/intel_engine_cs.c +10
-0+ +static void intel_engine_init_fpreempt (struct intel_engine_cs *engine) +{ +if (!INTEL_INFO (engine->i915)->has_logical_ring_preemption) +return ; + +hrtimer_init (&engine->fpreempt_timer, +CLOCK_MONOTONIC ,HRTIMER_MODE_REL ); + engine->fpreempt_timer.function = intel_engine_fpreempt_timer; +INIT_WORK (&engine->fpreempt_work, intel_engine_fpreempt_work);
Just set the few pointers. It's not like we are saving anything.
Quoted 35 lines
drivers/gpu/drm/i915/intel_engine_cs.c +3
-0+} +/** * intel_engines_setup_common - setup engine state not requiring hw access * @engine: Engine to setup. @@ -426,6 +478,7 @@ void intel_engine_setup_common(struct intel_engine_cs *engine)intel_engine_init_timeline (engine);intel_engine_init_hangcheck (engine); +intel_engine_init_fpreempt (engine);i915_gem_batch_pool_init (engine, &engine->batch_pool);intel_engine_init_cmd_parser (engine);drivers/gpu/drm/i915/intel_lrc.c +2
-0@@ -628,6 +628,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)inject_preempt_context (engine);execlists_set_active (execlists,EXECLISTS_ACTIVE_PREEMPT ); +intel_engine_queue_fpreempt (engine); goto unlock; }else {/* @@ -846,6 +847,7 @@ static void intel_lrc_irq_handler(unsigned long data)const u32 *buf = &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX ];unsigned int head, tail; +bool defer = false;/* However GVT emulation depends upon intercepting CSB mmio */ if (unlikely (execlists->csb_use_mmio)) {@@ -919,6 +921,21 @@ static void intel_lrc_irq_handler(unsigned long data)
drivers/gpu/drm/i915/intel_lrc.c static void intel_lrc_irq_handler(unsigned long data) · line 931
Quoted 10 lines
drivers/gpu/drm/i915/intel_lrc.c +8
-0if (status &GEN8_CTX_STATUS_ACTIVE_IDLE && buf[2 *head +1 ] ==PREEMPT_ID ) { +/* Try to cancel any pending force preemption. +* If we are too late, hold off on processing +* the completed preemption until reset has +* run its course. It should recognize that +* the engine has preempted to idle then abort +* the reset. Then we can resume processing +* at this CSB head. +*/
This can be much simpler than implied here.
-Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [RFC 8/8] drm/i915: Force preemption to complete via engine reset
In reply to [email protected]Quoting [email protected] (2018-03-16 18:31:05)
drivers/gpu/drm/i915/intel_engine_cs.c static void intel_engine_init_execlist(struct intel_engine_cs *engine) · line 428
Quoted 3 lines
drivers/gpu/drm/i915/intel_engine_cs.c +3
-0+bool intel_engine_cancel_fpreempt (struct intel_engine_cs *engine) +{ +hrtimer_cancel (&engine->fpreempt_timer);
Can not be used inside the tasklet as the hrtimer may also be a tasklet on the same CPU; so busy spin deadlock.
-Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Patchwork
CI✗ Fi.CI.BAT: failure for Force preemption
In reply to [email protected]== Series Details ==
== Series Details ==
Series: Force preemption
URL : https://patchwork.freedesktop.org/series/40120/
State : failure
== Summary ==
Applying: drm/i915: Downgrade tasklet GEM_BUG_ON for request not completed
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_lrc.c).
error: could not build fake ancestor
Patch failed at 0001 drm/i915: Downgrade tasklet GEM_BUG_ON for request not completed The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting [email protected] (2018-03-16 18:30:57)
Quoted 14 lines
From: Jeff McGee <[email protected]>
Force preemption uses engine reset to enforce a limit on the time that a request targeted for preemption can block. This feature is a requirement in automotive systems where the GPU may be shared by clients of critically high priority and clients of low priority that may not have been curated to be preemption friendly. There may be more general applications of this feature. I'm sharing as an RFC to stimulate that discussion and also to get any technical feedback that I can before submitting to the product kernel that needs this. I have developed the patches for ease of rebase, given that this is for the moment considered a non-upstreamable feature. It would be possible to refactor hangcheck to fully incorporate force preemption as another tier of patience (or impatience) with the running request.
Last night I spent 15mins and wrote a similar RFC as Joonas keep muttering and I thought he was looking for code not that was some...
The real only difference is the approach to handling the pending timer. I do not agree with your reset changes as they appear to percolate even more uncertainity through the code. If the preempt-context is in flight when the reset is triggered, the last active request is still defined by incomplete request; the new request has not been submitted. This is serialised by checking for EXECLISTS_ACTIVE_PREEMPT. Up until the point that is cleared and the next request submitted; we should reset the engine and resubmit. The race in tasklet handling is immaterial at that point, you have already paid the cost in using a timer, the workqueue and serialising for the reset.
drivers/gpu/drm/i915/i915_debugfs.c +2 -2
@@ -3969,8 +3969,8 @@ i915_wedged_set(void *data, u64 val) engine->hangcheck.stalled = true; } -i915_handle_error (i915, val,"Manually set wedged engine mask = %llx" , - val); +i915_handle_error (i915, val,I915_ERROR_CAPTURE , +"Manually set wedged engine mask = %llx" , val); wait_on_bit(&i915->gpu_error.flags, I915_RESET_HANDOFF,
drivers/gpu/drm/i915/i915_drv.h +3 -1
@@ -2755,10 +2755,12 @@ static inline void i915_queue_hangcheck(struct drm_i915_private *dev_priv) &dev_priv->gpu_error.hangcheck_work, delay); } -__printf(3, 4) +__printf(4, 5)void i915_handle_error (struct drm_i915_private *dev_priv,u32 engine_mask, +unsigned long flags,const char *fmt, ...); +#define I915_ERROR_CAPTURE BIT(0)extern void intel_irq_init (struct drm_i915_private *dev_priv);extern void intel_irq_fini (struct drm_i915_private *dev_priv);
drivers/gpu/drm/i915/i915_irq.c +14 -8
@@ -2593,6 +2593,7 @@ static void i915_clear_error_registers(struct drm_i915_private *dev_priv)* i915_handle_error - handle a gpu error * @dev_priv: i915 device private * @engine_mask: mask representing engines that are hung +* @flags: control flags * @fmt: Error message format string * * Do some basic checking of register state at error time and
@@ -2603,16 +2604,11 @@ static void i915_clear_error_registers(struct drm_i915_private *dev_priv)*/ void i915_handle_error (struct drm_i915_private *dev_priv,u32 engine_mask, +unsigned long flags,const char *fmt, ...) {struct intel_engine_cs *engine;unsigned int tmp; -va_list args; -char error_msg[80 ]; - -va_start (args, fmt); -vscnprintf (error_msg,sizeof (error_msg), fmt, args); -va_end (args);/* * In most cases it's guaranteed that we get here with an RPM
@@ -2624,8 +2620,18 @@ void i915_handle_error(struct drm_i915_private *dev_priv,intel_runtime_pm_get (dev_priv); engine_mask &=DEVICE_INFO (dev_priv)->ring_mask; -i915_capture_error_state (dev_priv, engine_mask, error_msg); -i915_clear_error_registers (dev_priv); + +if (flags &I915_ERROR_CAPTURE ) { +char error_msg[80 ]; +va_list args; + +va_start (args, fmt); +vscnprintf (error_msg,sizeof (error_msg), fmt, args); +va_end (args); + +i915_capture_error_state (dev_priv, engine_mask, error_msg); +i915_clear_error_registers (dev_priv); + }/* * Try engine reset when available. We fall back to full reset if
drivers/gpu/drm/i915/intel_hangcheck.c +3 -3
@@ -246,7 +246,7 @@ engine_stuck(struct intel_engine_cs *engine, u64 acthd)*/ tmp =I915_READ_CTL (engine);if (tmp &RING_WAIT ) { -i915_handle_error (dev_priv,0 , +i915_handle_error (dev_priv,0 ,0 ,"Kicking stuck wait on %s" , engine->name);I915_WRITE_CTL (engine, tmp);
@@ -258,7 +258,7 @@ engine_stuck(struct intel_engine_cs *engine, u64 acthd)default :return ENGINE_DEAD ;case 1 : -i915_handle_error (dev_priv,0 , +i915_handle_error (dev_priv,0 ,0 ,"Kicking stuck semaphore on %s" , engine->name);I915_WRITE_CTL (engine, tmp);
@@ -392,7 +392,7 @@ static void hangcheck_declare_hang(struct drm_i915_private *i915, "%s, ", engine->name); msg[len-2 ] ='\0' ; -return i915_handle_error (i915, hung,"%s" , msg); +return i915_handle_error (i915, hung,I915_ERROR_CAPTURE ,"%s" , msg); }/*
drivers/gpu/drm/i915/intel_lrc.c +35 -2
@@ -529,8 +529,35 @@ static void inject_preempt_context(struct intel_engine_cs *engine)if (execlists->ctrl_reg)writel (EL_CTRL_LOAD , execlists->ctrl_reg); -execlists_clear_active (&engine->execlists,EXECLISTS_ACTIVE_HWACK ); -execlists_set_active (&engine->execlists,EXECLISTS_ACTIVE_PREEMPT ); +execlists_clear_active (execlists,EXECLISTS_ACTIVE_HWACK ); +execlists_set_active (execlists,EXECLISTS_ACTIVE_PREEMPT ); + +/* Set a timer to force preemption vs hostile userspace */ +if (execlists->preempt_timeout_ns) +hrtimer_start (&execlists->preempt_timer, +ktime_set (0 , execlists->preempt_timeout_ns), +HRTIMER_MODE_REL ); +} + +static enum hrtimer_restart preempt_timeout (struct hrtimer *hrtimer) +{ +struct intel_engine_cs *engine = +container_of (hrtimer,typeof (*engine), execlists.preempt_timer); + +if (execlists_is_active (&engine->execlists,EXECLISTS_ACTIVE_PREEMPT )) +queue_work (system_highpri_wq, &engine->execlists.preempt_reset); + +return HRTIMER_NORESTART ; +} + +static void preempt_reset (struct work_struct *work) +{ +struct intel_engine_cs *engine = +container_of (work,typeof (*engine), execlists.preempt_reset); + +if (execlists_is_active (&engine->execlists,EXECLISTS_ACTIVE_PREEMPT )) +i915_handle_error (engine->i915,BIT (engine->id),0 , +"preemption timed out on %s" , engine->name); }static void update_rps (struct intel_engine_cs *engine)
@@ -946,6 +973,7 @@ static void execlists_submission_tasklet(unsigned long data) EXECLISTS_ACTIVE_PREEMPT));execlists_clear_active (execlists,EXECLISTS_ACTIVE_PREEMPT ); +hrtimer_try_to_cancel (&execlists->preempt_timer);continue ; }
@@ -2180,6 +2208,11 @@ logical_ring_setup(struct intel_engine_cs *engine)tasklet_init (&engine->execlists.tasklet, execlists_submission_tasklet, (unsigned long )engine); +INIT_WORK (&engine->execlists.preempt_reset, preempt_reset); +hrtimer_init (&engine->execlists.preempt_timer, +CLOCK_MONOTONIC ,HRTIMER_MODE_REL ); + engine->execlists.preempt_timer.function = preempt_timeout; +logical_ring_default_vfuncs (engine);logical_ring_default_irqs (engine); }
drivers/gpu/drm/i915/intel_ringbuffer.h +4 -0
@@ -307,6 +307,10 @@ struct intel_engine_execlists {* @preempt_complete_status: expected CSB upon completing preemption */ u32 preempt_complete_status; + +struct hrtimer preempt_timer; +struct work_struct preempt_reset; +unsigned long preempt_timeout_ns; };#define INTEL_ENGINE_CS_MAX_NAME 8
drivers/gpu/drm/i915/selftests/intel_hangcheck.c +1 -1
@@ -1091,7 +1091,7 @@ static int igt_handle_error(void *arg) engine->hangcheck.stalled = true; engine->hangcheck.seqno =intel_engine_get_seqno (engine); -i915_handle_error (i915,intel_engine_flag (engine),"%s" , __func__); +i915_handle_error (i915,intel_engine_flag (engine),0 ,"%s" , __func__);xchg (&i915->gpu_error.first_error, error);
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Chris Wilson (2018-03-16 20:53:01)
drivers/gpu/drm/i915/intel_lrc.c static void inject_preempt_context(struct intel_engine_cs *engine) · line 557
Quoted 5 lines
drivers/gpu/drm/i915/intel_lrc.c +5
-0+static void preempt_reset (struct work_struct *work) +{ +struct intel_engine_cs *engine = +container_of (work,typeof (*engine), execlists.preempt_reset); +
So thinking about the races you had in the reset, you need
tasklet_kill()tasklet_disable();
drivers/gpu/drm/i915/intel_lrc.c static void inject_preempt_context(struct intel_engine_cs *engine) · line 560
Quoted 3 lines
drivers/gpu/drm/i915/intel_lrc.c +3
-0+if (execlists_is_active (&engine->execlists,EXECLISTS_ACTIVE_PREEMPT )) +i915_handle_error (engine->i915,BIT (engine->id),0 , +"preemption timed out on %s" , engine->name);
tasklet_enable();
here for the serialisation on EXECLISTS_ACTIVE_PREEMPT. -Chris
_______________________________________________
Intel-gfx mailing list [email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 1/1 force-preempt-reset
A more concrete idea of what I think the serialisation should be between reset timer and tasklet.
--- drivers/gpu/drm/i915/intel_lrc.c | 43 +++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +++ 2 files changed, 47 insertions(+)
drivers/gpu/drm/i915/intel_lrc.c +43 -0
@@ -531,6 +531,39 @@ static void inject_preempt_context(struct intel_engine_cs *engine)execlists_clear_active (execlists,EXECLISTS_ACTIVE_HWACK );execlists_set_active (execlists,EXECLISTS_ACTIVE_PREEMPT ); + +/* Set a timer to force preemption vs hostile userspace */ +if (execlists->preempt_timeout_ns) +hrtimer_start (&execlists->preempt_timer, +ktime_set (0 , execlists->preempt_timeout_ns), +HRTIMER_MODE_REL ); +} + +static enum hrtimer_restart preempt_timeout (struct hrtimer *hrtimer) +{ +struct intel_engine_cs *engine = +container_of (hrtimer,typeof (*engine), execlists.preempt_timer); + +if (execlists_is_active (&engine->execlists,EXECLISTS_ACTIVE_PREEMPT )) +queue_work (system_highpri_wq, &engine->execlists.preempt_reset); + +return HRTIMER_NORESTART ; +} + +static void preempt_reset (struct work_struct *work) +{ +struct intel_engine_cs *engine = +container_of (work,typeof (*engine), execlists.preempt_reset); + +tasklet_disable (&engine->execlists.tasklet); + + engine->execlists.tasklet.func (engine->execlists.tasklet.data); + +if (execlists_is_active (&engine->execlists,EXECLISTS_ACTIVE_PREEMPT )) +i915_handle_error (engine->i915,BIT (engine->id),0 , +"preemption timed out on %s" , engine->name); + +tasklet_enable (&engine->execlists.tasklet); }static void complete_preempt_context (struct intel_engine_execlists *execlists)
@@ -539,6 +572,11 @@ static void complete_preempt_context(struct intel_engine_execlists *execlists)execlists_unwind_incomplete_requests (execlists);GEM_BUG_ON (!execlists_is_active (execlists,EXECLISTS_ACTIVE_PREEMPT )); + +/* If the timer already fired, complete the reset */ +if (hrtimer_try_to_cancel (&execlists->preempt_timer) <0 ) +return ; +execlists_clear_active (execlists,EXECLISTS_ACTIVE_PREEMPT ); }
@@ -2182,6 +2220,11 @@ logical_ring_setup(struct intel_engine_cs *engine)tasklet_init (&engine->execlists.tasklet, execlists_submission_tasklet, (unsigned long )engine); +INIT_WORK (&engine->execlists.preempt_reset, preempt_reset); +hrtimer_init (&engine->execlists.preempt_timer, +CLOCK_MONOTONIC ,HRTIMER_MODE_REL ); + engine->execlists.preempt_timer.function = preempt_timeout; +logical_ring_default_vfuncs (engine);logical_ring_default_irqs (engine); }
drivers/gpu/drm/i915/intel_ringbuffer.h +4 -0
@@ -307,6 +307,10 @@ struct intel_engine_execlists {* @preempt_complete_status: expected CSB upon completing preemption */ u32 preempt_complete_status; + +struct hrtimer preempt_timer; +struct work_struct preempt_reset; +unsigned long preempt_timeout_ns; };#define INTEL_ENGINE_CS_MAX_NAME 8
Signature · 6 lines
2.16.2 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Patchwork
CI✗ Fi.CI.BAT: failure for force-preempt-reset
In reply to Chris Wilson== Series Details ==
== Series Details ==
Series: force-preempt-reset
URL : https://patchwork.freedesktop.org/series/40136/
State : failure
== Summary ==
Applying: force-preempt-reset
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_lrc.c).
error: could not build fake ancestor
Patch failed at 0001 force-preempt-reset
The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting [email protected] (2018-03-16 18:30:57)
Quoted 14 lines
From: Jeff McGee <[email protected]>
Force preemption uses engine reset to enforce a limit on the time that a request targeted for preemption can block. This feature is a requirement in automotive systems where the GPU may be shared by clients of critically high priority and clients of low priority that may not have been curated to be preemption friendly. There may be more general applications of this feature. I'm sharing as an RFC to stimulate that discussion and also to get any technical feedback that I can before submitting to the product kernel that needs this. I have developed the patches for ease of rebase, given that this is for the moment considered a non-upstreamable feature. It would be possible to refactor hangcheck to fully incorporate force preemption as another tier of patience (or impatience) with the running request.
So one big problem I should have realised earlier is ksoftirqd screws up any latency sensitive operation. I have an ugly idea for a hack, but we really should cap the latency problems with tasklets.
-Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx