Re: [Intel-gfx] [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts

2017-10-23 Thread Chris Wilson 
Quoting Chris Wilson (2017-10-23 21:06:16)
> Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists
> context-switch when idle") we noticed the presence of late
> context-switch interrupts. We were able to filter those out by looking
> at whether the ELSP remained active, but in commit beecec901790
> ("drm/i915/execlists: Preemption!") that became problematic as we now
> anticipate receiving a context-switch event for preemption while ELSP
> may be empty. To restore the spurious interrupt suppression, add a
> counter for the expected number of pending context-switches and skip if
> we do not need to handle this interrupt to make forward progress.

Looking at an example from
https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_1299/
the common case is where we still get the interrupt after already
parsing the whole CSB:

<6>[   22.723238] i915 :00:02.0: [drm] vecs0
<6>[   22.723246] i915 :00:02.0: [drm]  current seqno 8, last 8, 
hangcheck 0 [-277277 ms], inflight 0
<6>[   22.723260] i915 :00:02.0: [drm]  Reset count: 0
<6>[   22.723269] i915 :00:02.0: [drm]  Requests:
<6>[   22.723278] i915 :00:02.0: [drm]  RING_START: 0x007fb000 
[0x]
<6>[   22.723289] i915 :00:02.0: [drm]  RING_HEAD:  0x0278 
[0x]
<6>[   22.723300] i915 :00:02.0: [drm]  RING_TAIL:  0x0278 
[0x]
<6>[   22.723311] i915 :00:02.0: [drm]  RING_CTL:   0x3001 []
<6>[   22.723322] i915 :00:02.0: [drm]  ACTHD:  0x_0278
<6>[   22.72] i915 :00:02.0: [drm]  BBADDR: 0x_0004
<6>[   22.723343] i915 :00:02.0: [drm]  Execlist status: 0x0301 

<6>[   22.723355] i915 :00:02.0: [drm]  Execlist CSB read 1 [1 cached], 
write 1 [1 from hws], interrupt posted? no
<6>[   22.723370] i915 :00:02.0: [drm]  ELSP[0] idle
<6>[   22.723378] i915 :00:02.0: [drm]  ELSP[1] idle
<6>[   22.723387] i915 :00:02.0: [drm]  HW active? 0x0
<6>[   22.723402] i915 :00:02.0: [drm] 


Those should not lead to hitting BUG_ON(gt.awake) though as the tasklet
is flushed before we clear gt.awake. Except if maybe the interrupt
arrives after the tasklet_kill...

Given that we wait for the engines to be idle before parking, we should
be safe enough with

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index bb0e85043e01..fa46137d431a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3327,6 +3327,8 @@ i915_gem_idle_work_handler(struct work_struct *work)
if (new_requests_since_last_retire(dev_priv))
goto out_unlock;
 
+   synchronize_irq(dev_priv->drm.irq);
+
/*
 * We are committed now to parking the engines, make sure there
 * will be no more interrupts arriving later.

to flush a pending irq and not worry about a multi-phase park.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts

2017-10-23 Thread Chris Wilson 
Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists
context-switch when idle") we noticed the presence of late
context-switch interrupts. We were able to filter those out by looking
at whether the ELSP remained active, but in commit beecec901790
("drm/i915/execlists: Preemption!") that became problematic as we now
anticipate receiving a context-switch event for preemption while ELSP
may be empty. To restore the spurious interrupt suppression, add a
counter for the expected number of pending context-switches and skip if
we do not need to handle this interrupt to make forward progress.

v2: Don't forget to switch on for preempt.
v3: Reduce the counter to a on/off boolean tracker. Declare the HW as
active when we first submit, and idle after the final completion event
(with which we confirm the HW says it is idle), and track each source
of activity separately. With a finite number of sources, it should us
debug which gets stuck.

Fixes: beecec901790 ("drm/i915/execlists: Preemption!")
Signed-off-by: Chris Wilson 
Cc: Michal Winiarski 
Cc: Tvrtko Ursulin 
Cc: Arkadiusz Hiler 
Cc: Mika Kuoppala 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/i915_guc_submission.c |  3 +++
 drivers/gpu/drm/i915/i915_irq.c|  6 --
 drivers/gpu/drm/i915/intel_engine_cs.c |  5 +++--
 drivers/gpu/drm/i915/intel_lrc.c   | 27 ++--
 drivers/gpu/drm/i915/intel_ringbuffer.h| 34 --
 5 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index a2e8114b739d..f84c267728fd 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -610,6 +610,7 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
execlists->first = rb;
if (submit) {
port_assign(port, last);
+   execlists_set_active(execlists, EXECLISTS_ACTIVE_USER);
i915_guc_submit(engine);
}
spin_unlock_irq(>timeline->lock);
@@ -633,6 +634,8 @@ static void i915_guc_irq_handler(unsigned long data)
 
rq = port_request([0]);
}
+   if (!rq)
+   execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER);
 
if (!port_isset(last_port))
i915_guc_dequeue(engine);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b1296a55c1e4..f8205841868b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1388,8 +1388,10 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 
iir, int test_shift)
bool tasklet = false;
 
if (iir & (GT_CONTEXT_SWITCH_INTERRUPT << test_shift)) {
-   __set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
-   tasklet = true;
+   if (READ_ONCE(engine->execlists.active)) {
+   __set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
+   tasklet = true;
+   }
}
 
if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
b/drivers/gpu/drm/i915/intel_engine_cs.c
index a47a9c6bea52..ab5bf4e2e28e 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1548,8 +1548,8 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
if (test_bit(ENGINE_IRQ_EXECLIST, >irq_posted))
return false;
 
-   /* Both ports drained, no more ELSP submission? */
-   if (port_request(>execlists.port[0]))
+   /* Waiting to drain ELSP? */
+   if (READ_ONCE(engine->execlists.active))
return false;
 
/* ELSP is empty, but there are ready requests? */
@@ -1749,6 +1749,7 @@ void intel_engine_dump(struct intel_engine_cs *engine, 
struct drm_printer *m)
   idx);
}
}
+   drm_printf(m, "\t\tHW active? 0x%x\n", execlists->active);
rcu_read_unlock();
} else if (INTEL_GEN(dev_priv) > 6) {
drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n",
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 7f45dd7dc3e5..233c992a886b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -575,7 +575,8 @@ static void execlists_dequeue(struct intel_engine_cs 
*engine)
 * the state of the GPU is known (idle).
 */
inject_preempt_context(engine);
-   execlists->preempt = true;
+   execlists_set_active(execlists,
+EXECLISTS_ACTIVE_PREEMPT);

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts

2017-10-20 Thread Chris Wilson 
Quoting Mika Kuoppala (2017-10-20 14:59:44)
> Chris Wilson  writes:
> 
> > Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists
> > context-switch when idle") we noticed the presence of late
> > context-switch interrupts. We were able to filter those out by looking
> > at whether the ELSP remained active, but in commit beecec901790
> > ("drm/i915/execlists: Preemption!") that became problematic as we now
> > anticipate receiving a context-switch event for preemption while ELSP
> > may be empty. To restore the spurious interrupt suppression, add a
> > counter for the expected number of pending context-switches and skip if
> > we do not need to handle this interrupt to make forward progress.
> >
> > Fixes: beecec901790 ("drm/i915/execlists: Preemption!")
> > Signed-off-by: Chris Wilson 
> > Cc: Michal Winiarski 
> > Cc: Tvrtko Ursulin 
> > Cc: Arkadiusz Hiler 
> > Cc: Mika Kuoppala 
> > Cc: Joonas Lahtinen 
> > ---
> >  drivers/gpu/drm/i915/i915_guc_submission.c | 11 ++-
> >  drivers/gpu/drm/i915/i915_irq.c|  6 --
> >  drivers/gpu/drm/i915/intel_engine_cs.c |  5 +++--
> >  drivers/gpu/drm/i915/intel_lrc.c   | 21 +
> >  drivers/gpu/drm/i915/intel_ringbuffer.h|  7 +++
> >  5 files changed, 37 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
> > b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index a2e8114b739d..c22d0a07467c 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -550,11 +550,9 @@ static void port_assign(struct execlist_port *port,
> >   struct drm_i915_gem_request *rq)
> >  {
> >   GEM_BUG_ON(rq == port_request(port));
> > + GEM_BUG_ON(port_isset(port));
> >  
> > - if (port_isset(port))
> > - i915_gem_request_put(port_request(port));
> > -
> > - port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
> > + port_set(port, i915_gem_request_get(rq));
> 
> No lite restore with guc so we can make this more strict and lean?

Yes. We are not coalescing requests into an active slot, so we always
know that we are assigning a new request into a new slot.

> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
> > b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index a47a9c6bea52..8aecbc321786 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -1548,8 +1548,8 @@ bool intel_engine_is_idle(struct intel_engine_cs 
> > *engine)
> >   if (test_bit(ENGINE_IRQ_EXECLIST, >irq_posted))
> >   return false;
> >  
> > - /* Both ports drained, no more ELSP submission? */
> > - if (port_request(>execlists.port[0]))
> > + /* Waiting to drain ELSP? */
> > + if (READ_ONCE(engine->execlists.active))
> >   return false;
> >
> 
> It would not make sense now to keep the port[0] check but
> we could get more fine grained debugging info if we keep
> it?

This function is the antithesis of fine grained debugging. It is called
in a tight loop, watching for idle so reporting from here ends up with a
lot of noise. Often I ask exactly what isn't idle... Now that we do a
single upfront wait, and then a check all is well, moving this to
intel_engine_park where we can be more verbose in the one-off state
check is the next step.

The importance of making this change is so the gen8_cs_irq_handler() and
the idle_worker are in sync about when to drop the GT wakeref.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts

2017-10-20 Thread Mika Kuoppala 
Chris Wilson  writes:

> Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists
> context-switch when idle") we noticed the presence of late
> context-switch interrupts. We were able to filter those out by looking
> at whether the ELSP remained active, but in commit beecec901790
> ("drm/i915/execlists: Preemption!") that became problematic as we now
> anticipate receiving a context-switch event for preemption while ELSP
> may be empty. To restore the spurious interrupt suppression, add a
> counter for the expected number of pending context-switches and skip if
> we do not need to handle this interrupt to make forward progress.
>
> Fixes: beecec901790 ("drm/i915/execlists: Preemption!")
> Signed-off-by: Chris Wilson 
> Cc: Michal Winiarski 
> Cc: Tvrtko Ursulin 
> Cc: Arkadiusz Hiler 
> Cc: Mika Kuoppala 
> Cc: Joonas Lahtinen 
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 11 ++-
>  drivers/gpu/drm/i915/i915_irq.c|  6 --
>  drivers/gpu/drm/i915/intel_engine_cs.c |  5 +++--
>  drivers/gpu/drm/i915/intel_lrc.c   | 21 +
>  drivers/gpu/drm/i915/intel_ringbuffer.h|  7 +++
>  5 files changed, 37 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
> b/drivers/gpu/drm/i915/i915_guc_submission.c
> index a2e8114b739d..c22d0a07467c 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -550,11 +550,9 @@ static void port_assign(struct execlist_port *port,
>   struct drm_i915_gem_request *rq)
>  {
>   GEM_BUG_ON(rq == port_request(port));
> + GEM_BUG_ON(port_isset(port));
>  
> - if (port_isset(port))
> - i915_gem_request_put(port_request(port));
> -
> - port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
> + port_set(port, i915_gem_request_get(rq));

No lite restore with guc so we can make this more strict and lean?

>   nested_enable_signaling(rq);
>  }
>  
> @@ -586,8 +584,10 @@ static void i915_guc_dequeue(struct intel_engine_cs 
> *engine)
>   goto done;
>   }
>  
> - if (submit)
> + if (submit) {
>   port_assign(port, last);
> + execlists->active++;
> + }
>   port++;
>   }
>  
> @@ -610,6 +610,7 @@ static void i915_guc_dequeue(struct intel_engine_cs 
> *engine)
>   execlists->first = rb;
>   if (submit) {
>   port_assign(port, last);
> + execlists->active++;
>   i915_guc_submit(engine);
>   }
>   spin_unlock_irq(>timeline->lock);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b1296a55c1e4..f8205841868b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1388,8 +1388,10 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, 
> u32 iir, int test_shift)
>   bool tasklet = false;
>  
>   if (iir & (GT_CONTEXT_SWITCH_INTERRUPT << test_shift)) {
> - __set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
> - tasklet = true;
> + if (READ_ONCE(engine->execlists.active)) {
> + __set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
> + tasklet = true;
> + }
>   }
>  
>   if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
> b/drivers/gpu/drm/i915/intel_engine_cs.c
> index a47a9c6bea52..8aecbc321786 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1548,8 +1548,8 @@ bool intel_engine_is_idle(struct intel_engine_cs 
> *engine)
>   if (test_bit(ENGINE_IRQ_EXECLIST, >irq_posted))
>   return false;
>  
> - /* Both ports drained, no more ELSP submission? */
> - if (port_request(>execlists.port[0]))
> + /* Waiting to drain ELSP? */
> + if (READ_ONCE(engine->execlists.active))
>   return false;
>

It would not make sense now to keep the port[0] check but
we could get more fine grained debugging info if we keep
it?

-Mika

>   /* ELSP is empty, but there are ready requests? */
> @@ -1749,6 +1749,7 @@ void intel_engine_dump(struct intel_engine_cs *engine, 
> struct drm_printer *m)
>  idx);
>   }
>   }
> + drm_printf(m, "\t\tELSP active %d\n", execlists->active);
>   rcu_read_unlock();
>   } else if (INTEL_GEN(dev_priv) > 6) {
>   drm_printf(m,

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts

2017-10-20 Thread Chris Wilson 
Quoting Mika Kuoppala (2017-10-20 14:31:14)
> Chris Wilson  writes:
> 
> > Quoting Mika Kuoppala (2017-10-20 14:21:08)
> >> Chris Wilson  writes:
> >> 
> >> > Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists
> >> > context-switch when idle") we noticed the presence of late
> >> > context-switch interrupts. We were able to filter those out by looking
> >> > at whether the ELSP remained active, but in commit beecec901790
> >> > ("drm/i915/execlists: Preemption!") that became problematic as we now
> >> > anticipate receiving a context-switch event for preemption while ELSP
> >> > may be empty. To restore the spurious interrupt suppression, add a
> >> 
> >> This confuses me, how can we preempt something that was never submitted.
> >> Could you elaborate?
> >
> > The ELSP may become empty after we told the hw to switch to the preempt
> > context. So the preemption context-switch may appear as a separate
> > interrupt after !port_isset(execlists.port[0]). The joy of asynchronous
> > communications.
> 
> Let me check if I got this right: as we dont use port[0] to
> switch to preempt context and thus we might get 2 interrupts in a
> a row, first one clearing port[0], we can't assume any idleness
> by port[0] status alone?

Correct.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts

2017-10-20 Thread Mika Kuoppala 
Chris Wilson  writes:

> Quoting Mika Kuoppala (2017-10-20 14:21:08)
>> Chris Wilson  writes:
>> 
>> > Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists
>> > context-switch when idle") we noticed the presence of late
>> > context-switch interrupts. We were able to filter those out by looking
>> > at whether the ELSP remained active, but in commit beecec901790
>> > ("drm/i915/execlists: Preemption!") that became problematic as we now
>> > anticipate receiving a context-switch event for preemption while ELSP
>> > may be empty. To restore the spurious interrupt suppression, add a
>> 
>> This confuses me, how can we preempt something that was never submitted.
>> Could you elaborate?
>
> The ELSP may become empty after we told the hw to switch to the preempt
> context. So the preemption context-switch may appear as a separate
> interrupt after !port_isset(execlists.port[0]). The joy of asynchronous
> communications.

Let me check if I got this right: as we dont use port[0] to
switch to preempt context and thus we might get 2 interrupts in a
a row, first one clearing port[0], we can't assume any idleness
by port[0] status alone?

-Mika
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts

2017-10-20 Thread Chris Wilson 
Quoting Mika Kuoppala (2017-10-20 14:21:08)
> Chris Wilson  writes:
> 
> > Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists
> > context-switch when idle") we noticed the presence of late
> > context-switch interrupts. We were able to filter those out by looking
> > at whether the ELSP remained active, but in commit beecec901790
> > ("drm/i915/execlists: Preemption!") that became problematic as we now
> > anticipate receiving a context-switch event for preemption while ELSP
> > may be empty. To restore the spurious interrupt suppression, add a
> 
> This confuses me, how can we preempt something that was never submitted.
> Could you elaborate?

The ELSP may become empty after we told the hw to switch to the preempt
context. So the preemption context-switch may appear as a separate
interrupt after !port_isset(execlists.port[0]). The joy of asynchronous
communications.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts

2017-10-20 Thread Mika Kuoppala 
Chris Wilson  writes:

> Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists
> context-switch when idle") we noticed the presence of late
> context-switch interrupts. We were able to filter those out by looking
> at whether the ELSP remained active, but in commit beecec901790
> ("drm/i915/execlists: Preemption!") that became problematic as we now
> anticipate receiving a context-switch event for preemption while ELSP
> may be empty. To restore the spurious interrupt suppression, add a

This confuses me, how can we preempt something that was never submitted.
Could you elaborate?

Thanks,
-Mika

> counter for the expected number of pending context-switches and skip if
> we do not need to handle this interrupt to make forward progress.
>
> Fixes: beecec901790 ("drm/i915/execlists: Preemption!")
> Signed-off-by: Chris Wilson 
> Cc: Michal Winiarski 
> Cc: Tvrtko Ursulin 
> Cc: Arkadiusz Hiler 
> Cc: Mika Kuoppala 
> Cc: Joonas Lahtinen 
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 11 ++-
>  drivers/gpu/drm/i915/i915_irq.c|  6 --
>  drivers/gpu/drm/i915/intel_engine_cs.c |  5 +++--
>  drivers/gpu/drm/i915/intel_lrc.c   | 21 +
>  drivers/gpu/drm/i915/intel_ringbuffer.h|  7 +++
>  5 files changed, 37 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
> b/drivers/gpu/drm/i915/i915_guc_submission.c
> index a2e8114b739d..c22d0a07467c 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -550,11 +550,9 @@ static void port_assign(struct execlist_port *port,
>   struct drm_i915_gem_request *rq)
>  {
>   GEM_BUG_ON(rq == port_request(port));
> + GEM_BUG_ON(port_isset(port));
>  
> - if (port_isset(port))
> - i915_gem_request_put(port_request(port));
> -
> - port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
> + port_set(port, i915_gem_request_get(rq));
>   nested_enable_signaling(rq);
>  }
>  
> @@ -586,8 +584,10 @@ static void i915_guc_dequeue(struct intel_engine_cs 
> *engine)
>   goto done;
>   }
>  
> - if (submit)
> + if (submit) {
>   port_assign(port, last);
> + execlists->active++;
> + }
>   port++;
>   }
>  
> @@ -610,6 +610,7 @@ static void i915_guc_dequeue(struct intel_engine_cs 
> *engine)
>   execlists->first = rb;
>   if (submit) {
>   port_assign(port, last);
> + execlists->active++;
>   i915_guc_submit(engine);
>   }
>   spin_unlock_irq(>timeline->lock);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b1296a55c1e4..f8205841868b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1388,8 +1388,10 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, 
> u32 iir, int test_shift)
>   bool tasklet = false;
>  
>   if (iir & (GT_CONTEXT_SWITCH_INTERRUPT << test_shift)) {
> - __set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
> - tasklet = true;
> + if (READ_ONCE(engine->execlists.active)) {
> + __set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
> + tasklet = true;
> + }
>   }
>  
>   if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
> b/drivers/gpu/drm/i915/intel_engine_cs.c
> index a47a9c6bea52..8aecbc321786 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1548,8 +1548,8 @@ bool intel_engine_is_idle(struct intel_engine_cs 
> *engine)
>   if (test_bit(ENGINE_IRQ_EXECLIST, >irq_posted))
>   return false;
>  
> - /* Both ports drained, no more ELSP submission? */
> - if (port_request(>execlists.port[0]))
> + /* Waiting to drain ELSP? */
> + if (READ_ONCE(engine->execlists.active))
>   return false;
>  
>   /* ELSP is empty, but there are ready requests? */
> @@ -1749,6 +1749,7 @@ void intel_engine_dump(struct intel_engine_cs *engine, 
> struct drm_printer *m)
>  idx);
>   }
>   }
> + drm_printf(m, "\t\tELSP active %d\n", execlists->active);
>   rcu_read_unlock();
>   } else if (INTEL_GEN(dev_priv) > 6) {
>   drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n",
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
>

[Intel-gfx] [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts

2017-10-20 Thread Chris Wilson 
Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists
context-switch when idle") we noticed the presence of late
context-switch interrupts. We were able to filter those out by looking
at whether the ELSP remained active, but in commit beecec901790
("drm/i915/execlists: Preemption!") that became problematic as we now
anticipate receiving a context-switch event for preemption while ELSP
may be empty. To restore the spurious interrupt suppression, add a
counter for the expected number of pending context-switches and skip if
we do not need to handle this interrupt to make forward progress.

Fixes: beecec901790 ("drm/i915/execlists: Preemption!")
Signed-off-by: Chris Wilson 
Cc: Michal Winiarski 
Cc: Tvrtko Ursulin 
Cc: Arkadiusz Hiler 
Cc: Mika Kuoppala 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 11 ++-
 drivers/gpu/drm/i915/i915_irq.c|  6 --
 drivers/gpu/drm/i915/intel_engine_cs.c |  5 +++--
 drivers/gpu/drm/i915/intel_lrc.c   | 21 +
 drivers/gpu/drm/i915/intel_ringbuffer.h|  7 +++
 5 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index a2e8114b739d..c22d0a07467c 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -550,11 +550,9 @@ static void port_assign(struct execlist_port *port,
struct drm_i915_gem_request *rq)
 {
GEM_BUG_ON(rq == port_request(port));
+   GEM_BUG_ON(port_isset(port));
 
-   if (port_isset(port))
-   i915_gem_request_put(port_request(port));
-
-   port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
+   port_set(port, i915_gem_request_get(rq));
nested_enable_signaling(rq);
 }
 
@@ -586,8 +584,10 @@ static void i915_guc_dequeue(struct intel_engine_cs 
*engine)
goto done;
}
 
-   if (submit)
+   if (submit) {
port_assign(port, last);
+   execlists->active++;
+   }
port++;
}
 
@@ -610,6 +610,7 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
execlists->first = rb;
if (submit) {
port_assign(port, last);
+   execlists->active++;
i915_guc_submit(engine);
}
spin_unlock_irq(>timeline->lock);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b1296a55c1e4..f8205841868b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1388,8 +1388,10 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 
iir, int test_shift)
bool tasklet = false;
 
if (iir & (GT_CONTEXT_SWITCH_INTERRUPT << test_shift)) {
-   __set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
-   tasklet = true;
+   if (READ_ONCE(engine->execlists.active)) {
+   __set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
+   tasklet = true;
+   }
}
 
if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
b/drivers/gpu/drm/i915/intel_engine_cs.c
index a47a9c6bea52..8aecbc321786 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1548,8 +1548,8 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
if (test_bit(ENGINE_IRQ_EXECLIST, >irq_posted))
return false;
 
-   /* Both ports drained, no more ELSP submission? */
-   if (port_request(>execlists.port[0]))
+   /* Waiting to drain ELSP? */
+   if (READ_ONCE(engine->execlists.active))
return false;
 
/* ELSP is empty, but there are ready requests? */
@@ -1749,6 +1749,7 @@ void intel_engine_dump(struct intel_engine_cs *engine, 
struct drm_printer *m)
   idx);
}
}
+   drm_printf(m, "\t\tELSP active %d\n", execlists->active);
rcu_read_unlock();
} else if (INTEL_GEN(dev_priv) > 6) {
drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n",
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 7f45dd7dc3e5..8a47b8211c74 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -482,15 +482,23 @@ static bool can_merge_ctx(const struct i915_gem_context 
*prev,
return true;
 }
 
-static void port_assign(struct