Re: [Intel-gfx] [PATCH v2 1/2] drm/i915/guc: Fill preempt context once at init time

2018-02-27 Thread Chris Wilson
Quoting Michał Winiarski (2018-02-26 16:37:59)
> Since we're inhibiting context save of preempt context, we're no longer
> tracking the position of HEAD/TAIL. With GuC, we're adding a new
> breadcrumb for each preemption, which means that the HW will do more and
> more breadcrumb writes. Eventually the ring is filled, and we're
> submitting the preemption context with HEAD==TAIL==0, which won't result
> in breadcrumb write, but will trigger hangcheck instead.
> Instead of writing a new preempt breadcrumb for each preemption, let's
> just fill the ring once at init time (which also saves a couple of
> instructions in the tasklet).
> 
> v2: Assert that context save restore is inhibited, don't assert on ring
> alignment. (Chris)
> 
> Fixes: 517aaffe0c1b ("drm/i915/execlists: Inhibit context save/restore for 
> the fake preempt context")
> Signed-off-by: Michał Winiarski 
> Cc: Chris Wilson 
> Cc: Daniele Ceraolo Spurio 
> Cc: Michel Thierry 
> Cc: Mika Kuoppala 
> Cc: Tvrtko Ursulin 

Reviewed-by: Chris Wilson 

And pushed with the minor whitespace cleanup to appease checkpatch.
Thanks for the fix!
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [PATCH v2 1/2] drm/i915/guc: Fill preempt context once at init time

2018-02-26 Thread Michał Winiarski
Since we're inhibiting context save of preempt context, we're no longer
tracking the position of HEAD/TAIL. With GuC, we're adding a new
breadcrumb for each preemption, which means that the HW will do more and
more breadcrumb writes. Eventually the ring is filled, and we're
submitting the preemption context with HEAD==TAIL==0, which won't result
in breadcrumb write, but will trigger hangcheck instead.
Instead of writing a new preempt breadcrumb for each preemption, let's
just fill the ring once at init time (which also saves a couple of
instructions in the tasklet).

v2: Assert that context save restore is inhibited, don't assert on ring
alignment. (Chris)

Fixes: 517aaffe0c1b ("drm/i915/execlists: Inhibit context save/restore for the 
fake preempt context")
Signed-off-by: Michał Winiarski 
Cc: Chris Wilson 
Cc: Daniele Ceraolo Spurio 
Cc: Michel Thierry 
Cc: Mika Kuoppala 
Cc: Tvrtko Ursulin 
---
 drivers/gpu/drm/i915/intel_guc_submission.c | 86 -
 1 file changed, 59 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c 
b/drivers/gpu/drm/i915/intel_guc_submission.c
index 586dde579903..3805839f567b 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -26,8 +26,13 @@
 #include 
 
 #include "intel_guc_submission.h"
+#include "intel_lrc_reg.h"
 #include "i915_drv.h"
 
+#define GUC_PREEMPT_FINISHED   0x1
+#define GUC_PREEMPT_BREADCRUMB_DWORDS  0x8
+#define GUC_PREEMPT_BREADCRUMB_BYTES   (sizeof(u32) * 
GUC_PREEMPT_BREADCRUMB_DWORDS)
+
 /**
  * DOC: GuC-based command submission
  *
@@ -535,8 +540,6 @@ static void flush_ggtt_writes(struct i915_vma *vma)
POSTING_READ_FW(GUC_STATUS);
 }
 
-#define GUC_PREEMPT_FINISHED 0x1
-#define GUC_PREEMPT_BREADCRUMB_DWORDS 0x8
 static void inject_preempt_context(struct work_struct *work)
 {
struct guc_preempt_work *preempt_work =
@@ -546,37 +549,17 @@ static void inject_preempt_context(struct work_struct 
*work)
 preempt_work[engine->id]);
struct intel_guc_client *client = guc->preempt_client;
struct guc_stage_desc *stage_desc = __get_stage_desc(client);
-   struct intel_ring *ring = client->owner->engine[engine->id].ring;
u32 ctx_desc = lower_32_bits(intel_lr_context_descriptor(client->owner,
 engine));
-   u32 *cs = ring->vaddr + ring->tail;
u32 data[7];
 
-   if (engine->id == RCS) {
-   cs = gen8_emit_ggtt_write_rcs(cs, GUC_PREEMPT_FINISHED,
-   intel_hws_preempt_done_address(engine));
-   } else {
-   cs = gen8_emit_ggtt_write(cs, GUC_PREEMPT_FINISHED,
-   intel_hws_preempt_done_address(engine));
-   *cs++ = MI_NOOP;
-   *cs++ = MI_NOOP;
-   }
-   *cs++ = MI_USER_INTERRUPT;
-   *cs++ = MI_NOOP;
-
-   GEM_BUG_ON(!IS_ALIGNED(ring->size,
-  GUC_PREEMPT_BREADCRUMB_DWORDS * sizeof(u32)));
-   GEM_BUG_ON((void *)cs - (ring->vaddr + ring->tail) !=
-  GUC_PREEMPT_BREADCRUMB_DWORDS * sizeof(u32));
-
-   ring->tail += GUC_PREEMPT_BREADCRUMB_DWORDS * sizeof(u32);
-   ring->tail &= (ring->size - 1);
-
-   flush_ggtt_writes(ring->vma);
-
+   /*
+* The ring should containt commands writing GUC_PREEMPT_FINISHED to
+* HWSP at client initialization time.
+*/
spin_lock_irq(>wq_lock);
guc_wq_item_append(client, engine->guc_id, ctx_desc,
-  ring->tail / sizeof(u64), 0);
+  GUC_PREEMPT_BREADCRUMB_BYTES / sizeof(u64), 0);
spin_unlock_irq(>wq_lock);
 
/*
@@ -972,6 +955,53 @@ static void guc_client_free(struct intel_guc_client 
*client)
kfree(client);
 }
 
+static void guc_fill_preempt_context(struct intel_guc *guc)
+{
+   struct drm_i915_private *dev_priv = guc_to_i915(guc);
+   struct intel_guc_client *client = guc->preempt_client;
+   struct intel_engine_cs *engine;
+   enum intel_engine_id id;
+   u32 *cs;
+
+   for_each_engine(engine, dev_priv, id) {
+   struct intel_context *ce = >owner->engine[id];
+
+   GEM_BUG_ON(!ce->pin_count);
+
+   /*
+* We rely on this context image *not* being saved after
+* preemption. This ensures that the RING_HEAD / RING_TAIL
+* remaing pointing at initial values forever.
+*/
+   GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
+   _MASKED_BIT_ENABLE(
+   CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
+