Re: [Intel-gfx] [PATCH 3/4] drm/i915: Use full serialisation around engine->irq_posted

[Chris Wilson]

Quoting Chris Wilson (2018-03-31 00:08:47)
> Quoting Chris Wilson (2018-03-22 07:35:32)
> > Using engine->irq_posted for execlists, we are not always serialised by
> > the tasklet as we supposed. On the reset paths, the tasklet is disabled
> > and ignored. Instead, we manipulate the engine->irq_posted directly to
> > account for the reset, but if an interrupt fired before the reset and so
> > wrote to engine->irq_posted, that write may not be flushed from the
> > local CPU's cacheline until much later as the tasklet is already active
> > and so does not generate a mb(). To correctly serialise the interrupt
> > with reset, we need serialisation on the set_bit() itself.
> > 
> > And at last Mika can be happy.
> > 
> > Signed-off-by: Chris Wilson 
> > Cc: Mika Kuoppala 
> > Cc: Michał Winiarski 
> > CC: Michel Thierry 
> > Cc: Jeff McGee 
> > Cc: Tvrtko Ursulin 
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 7 +++
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c 
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index fa7310766217..27aee25429b7 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1405,10 +1405,9 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, 
> > u32 iir)
> > bool tasklet = false;
> >  
> > if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
> > -   if (READ_ONCE(engine->execlists.active)) {
> > -   __set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
> > -   tasklet = true;
> > -   }
> > +   if (READ_ONCE(engine->execlists.active))
> > +   tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
> > +   >irq_posted);
> 
> This is driving me mad. A very rare missed interrupt unless we
> unconditionally kick tasklet:
> 
> if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
> -   if (READ_ONCE(engine->execlists.active))
> -   tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
> -   >irq_posted);
> +   if (READ_ONCE(engine->execlists.active)) {
> +   set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
> +   tasklet = true;
> +   }
> }
> 
> I can't see why.
> 
> Hmm, I wonder if we are seeing READ_ONCE(execlsts->active) false
> negatives.

Fortunately, doesn't appear to be that.

@@ -1405,9 +1405,10 @@  gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 
iir)
bool tasklet = false;
 
if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
-   if (READ_ONCE(engine->execlists.active))
-   tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
-   >irq_posted);
+   GEM_BUG_ON(!READ_ONCE(execlists->tasklet.state) &&
+  test_bit(ENGINE_IRQ_EXECLIST, >irq_posted));
+   tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
+   >irq_posted);
}

Hasn't even hit a BUG, which is a little disconcerting.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/4] drm/i915: Use full serialisation around engine->irq_posted

[Chris Wilson]

Quoting Chris Wilson (2018-03-22 07:35:32)
> Using engine->irq_posted for execlists, we are not always serialised by
> the tasklet as we supposed. On the reset paths, the tasklet is disabled
> and ignored. Instead, we manipulate the engine->irq_posted directly to
> account for the reset, but if an interrupt fired before the reset and so
> wrote to engine->irq_posted, that write may not be flushed from the
> local CPU's cacheline until much later as the tasklet is already active
> and so does not generate a mb(). To correctly serialise the interrupt
> with reset, we need serialisation on the set_bit() itself.
> 
> And at last Mika can be happy.
> 
> Signed-off-by: Chris Wilson 
> Cc: Mika Kuoppala 
> Cc: Michał Winiarski 
> CC: Michel Thierry 
> Cc: Jeff McGee 
> Cc: Tvrtko Ursulin 
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index fa7310766217..27aee25429b7 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1405,10 +1405,9 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, 
> u32 iir)
> bool tasklet = false;
>  
> if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
> -   if (READ_ONCE(engine->execlists.active)) {
> -   __set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
> -   tasklet = true;
> -   }
> +   if (READ_ONCE(engine->execlists.active))
> +   tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
> +   >irq_posted);

This is driving me mad. A very rare missed interrupt unless we
unconditionally kick tasklet:

if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
-   if (READ_ONCE(engine->execlists.active))
-   tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
-   >irq_posted);
+   if (READ_ONCE(engine->execlists.active)) {
+   set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
+   tasklet = true;
+   }
}

I can't see why.

Hmm, I wonder if we are seeing READ_ONCE(execlsts->active) false
negatives.

Getting close to admitting defeat :(
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/4] drm/i915: Use full serialisation around engine->irq_posted

[Chris Wilson]

Quoting Jeff McGee (2018-03-22 15:34:45)
> On Thu, Mar 22, 2018 at 07:35:32AM +, Chris Wilson wrote:
> > Using engine->irq_posted for execlists, we are not always serialised by
> > the tasklet as we supposed. On the reset paths, the tasklet is disabled
> > and ignored. Instead, we manipulate the engine->irq_posted directly to
> > account for the reset, but if an interrupt fired before the reset and so
> > wrote to engine->irq_posted, that write may not be flushed from the
> > local CPU's cacheline until much later as the tasklet is already active
> > and so does not generate a mb(). To correctly serialise the interrupt
> > with reset, we need serialisation on the set_bit() itself.
> > 
> > And at last Mika can be happy.
> > 
> > Signed-off-by: Chris Wilson 
> > Cc: Mika Kuoppala 
> > Cc: Michał Winiarski 
> > CC: Michel Thierry 
> > Cc: Jeff McGee 
> > Cc: Tvrtko Ursulin 
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 7 +++
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c 
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index fa7310766217..27aee25429b7 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1405,10 +1405,9 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, 
> > u32 iir)
> >   bool tasklet = false;
> >  
> >   if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
> > - if (READ_ONCE(engine->execlists.active)) {
> > - __set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
> > - tasklet = true;
> > - }
> > + if (READ_ONCE(engine->execlists.active))
> > + tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
> > + >irq_posted);
> >   }
> >  
> >   if (iir & GT_RENDER_USER_INTERRUPT) {
> > -- 
> > 2.16.2
> > 
> 
> Confirmed that this along with the interrupt flush eliminates the cases
> of finding CSB tail at its reset value (0x7) in the tasklet in my force
> preemption tests.

At the moment, I'm concerned about the failures we have in CI before we
go building on top. So care to complete the set of r-b for us to move
on?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/4] drm/i915: Use full serialisation around engine->irq_posted

[Jeff McGee]

On Thu, Mar 22, 2018 at 07:35:32AM +, Chris Wilson wrote:
> Using engine->irq_posted for execlists, we are not always serialised by
> the tasklet as we supposed. On the reset paths, the tasklet is disabled
> and ignored. Instead, we manipulate the engine->irq_posted directly to
> account for the reset, but if an interrupt fired before the reset and so
> wrote to engine->irq_posted, that write may not be flushed from the
> local CPU's cacheline until much later as the tasklet is already active
> and so does not generate a mb(). To correctly serialise the interrupt
> with reset, we need serialisation on the set_bit() itself.
> 
> And at last Mika can be happy.
> 
> Signed-off-by: Chris Wilson 
> Cc: Mika Kuoppala 
> Cc: Michał Winiarski 
> CC: Michel Thierry 
> Cc: Jeff McGee 
> Cc: Tvrtko Ursulin 
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index fa7310766217..27aee25429b7 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1405,10 +1405,9 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, 
> u32 iir)
>   bool tasklet = false;
>  
>   if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
> - if (READ_ONCE(engine->execlists.active)) {
> - __set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
> - tasklet = true;
> - }
> + if (READ_ONCE(engine->execlists.active))
> + tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
> + >irq_posted);
>   }
>  
>   if (iir & GT_RENDER_USER_INTERRUPT) {
> -- 
> 2.16.2
> 

Confirmed that this along with the interrupt flush eliminates the cases
of finding CSB tail at its reset value (0x7) in the tasklet in my force
preemption tests.

Reviewed-by: Jeff McGee 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/4] drm/i915: Use full serialisation around engine->irq_posted

[Mika Kuoppala]

Chris Wilson  writes:

> Using engine->irq_posted for execlists, we are not always serialised by
> the tasklet as we supposed. On the reset paths, the tasklet is disabled
> and ignored. Instead, we manipulate the engine->irq_posted directly to
> account for the reset, but if an interrupt fired before the reset and so
> wrote to engine->irq_posted, that write may not be flushed from the
> local CPU's cacheline until much later as the tasklet is already active
> and so does not generate a mb(). To correctly serialise the interrupt
> with reset, we need serialisation on the set_bit() itself.
>
> And at last Mika can be happy.

Yes.

>
> Signed-off-by: Chris Wilson 
> Cc: Mika Kuoppala 
> Cc: Michał Winiarski 
> CC: Michel Thierry 
> Cc: Jeff McGee 
> Cc: Tvrtko Ursulin 

Reviewed-by: Mika Kuoppala 

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index fa7310766217..27aee25429b7 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1405,10 +1405,9 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, 
> u32 iir)
>   bool tasklet = false;
>  
>   if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
> - if (READ_ONCE(engine->execlists.active)) {
> - __set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
> - tasklet = true;
> - }
> + if (READ_ONCE(engine->execlists.active))
> + tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
> + >irq_posted);
>   }
>  
>   if (iir & GT_RENDER_USER_INTERRUPT) {
> -- 
> 2.16.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 3/4] drm/i915: Use full serialisation around engine->irq_posted

[Chris Wilson]

Using engine->irq_posted for execlists, we are not always serialised by
the tasklet as we supposed. On the reset paths, the tasklet is disabled
and ignored. Instead, we manipulate the engine->irq_posted directly to
account for the reset, but if an interrupt fired before the reset and so
wrote to engine->irq_posted, that write may not be flushed from the
local CPU's cacheline until much later as the tasklet is already active
and so does not generate a mb(). To correctly serialise the interrupt
with reset, we need serialisation on the set_bit() itself.

And at last Mika can be happy.

Signed-off-by: Chris Wilson 
Cc: Mika Kuoppala 
Cc: Michał Winiarski 
CC: Michel Thierry 
Cc: Jeff McGee 
Cc: Tvrtko Ursulin 
---
 drivers/gpu/drm/i915/i915_irq.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index fa7310766217..27aee25429b7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1405,10 +1405,9 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 
iir)
bool tasklet = false;
 
if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
-   if (READ_ONCE(engine->execlists.active)) {
-   __set_bit(ENGINE_IRQ_EXECLIST, >irq_posted);
-   tasklet = true;
-   }
+   if (READ_ONCE(engine->execlists.active))
+   tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
+   >irq_posted);
}
 
if (iir & GT_RENDER_USER_INTERRUPT) {
-- 
2.16.2

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