[PATCH] drm/i915/execlists: Poison the CSB after use

[PATCH] drm/i915/execlists: Poison the CSB after use

[Chris Wilson]

After reading the event status from the CSB, write back 0 (an invalid
value) so we can detect if the HW should signal a new event without
writing the event in the future.

References: https://bugs.freedesktop.org/show_bug.cgi?id=108315
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 2fad093d644d..38a6c39ca957 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1222,6 +1222,9 @@ static void process_csb(struct intel_engine_cs *engine)
 			  execlists->active);
 
 		status = buf[2 * head];
+		GEM_BUG_ON(!status);
+		GEM_DEBUG_EXEC(WRITE_ONCE(*(u32 *)(buf + 2 * head), 0));
+
 		if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
 			      GEN8_CTX_STATUS_PREEMPTED))
 			execlists_set_active(execlists,
-- 
2.19.1

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

Re: [PATCH] drm/i915/execlists: Poison the CSB after use

[Chris Wilson]

Quoting Chris Wilson (2018-11-21 13:08:23)
> After reading the event status from the CSB, write back 0 (an invalid
> value) so we can detect if the HW should signal a new event without
> writing the event in the future.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=108315
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Wrong branch, ignore this.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/execlists: Poison the CSB after use (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/execlists: Poison the CSB after use (rev2)
URL   : https://patchwork.freedesktop.org/series/51703/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5181 -> Patchwork_10878 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/51703/revisions/2/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10878 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ctx_create@basic-files:
      fi-icl-u2:          PASS -> DMESG-WARN (fdo#107724)

    igt@i915_module_load@reload:
      fi-blb-e6850:       NOTRUN -> INCOMPLETE (fdo#107718)

    igt@i915_selftest@live_hangcheck:
      fi-kbl-7560u:       PASS -> INCOMPLETE (fdo#108044)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-byt-clapper:     PASS -> FAIL (fdo#103191, fdo#107362)

    {igt@runner@aborted}:
      {fi-icl-y}:         NOTRUN -> FAIL (fdo#108070)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s3:
      fi-icl-u2:          DMESG-WARN (fdo#107724) -> PASS

    igt@gem_exec_suspend@basic-s4-devices:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-b:
      fi-byt-clapper:     FAIL (fdo#107362) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107724 https://bugs.freedesktop.org/show_bug.cgi?id=107724
  fdo#108044 https://bugs.freedesktop.org/show_bug.cgi?id=108044
  fdo#108070 https://bugs.freedesktop.org/show_bug.cgi?id=108070


== Participating hosts (50 -> 44) ==

  Additional (2): fi-icl-y fi-pnv-d510 
  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-ctg-p8600 fi-icl-u3 


== Build changes ==

    * Linux: CI_DRM_5181 -> Patchwork_10878

  CI_DRM_5181: 02b58f1ad4ca6ac69b69d9dbc7ee5043ebd25919 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4724: 29ae0925abe1d3a0202059538559468ad947d42d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10878: e2893f8cc5820116cf6ef4c4b4f814b2513ec709 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

e2893f8cc582 drm/i915/execlists: Poison the CSB after use

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10878/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915/execlists: Poison the CSB after use

[Chris Wilson]

After reading the event status from the CSB, write back 0 (an invalid
value) so we can detect if the HW should signal a new event without
writing the event in the future.

References: https://bugs.freedesktop.org/show_bug.cgi?id=108315
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 22b57b8926fc..126efe20d2d6 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -910,6 +910,9 @@ static void process_csb(struct intel_engine_cs *engine)
 			  execlists->active);
 
 		status = buf[2 * head];
+		GEM_BUG_ON(!status);
+		GEM_DEBUG_EXEC(WRITE_ONCE(*(u32 *)(buf + 2 * head), 0));
+
 		if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
 			      GEN8_CTX_STATUS_PREEMPTED))
 			execlists_set_active(execlists,
-- 
2.19.1

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

Re: [PATCH] drm/i915/execlists: Poison the CSB after use

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> After reading the event status from the CSB, write back 0 (an invalid
> value) so we can detect if the HW should signal a new event without
> writing the event in the future.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=108315
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 22b57b8926fc..126efe20d2d6 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -910,6 +910,9 @@ static void process_csb(struct intel_engine_cs *engine)
>  			  execlists->active);
>  
>  		status = buf[2 * head];
> +		GEM_BUG_ON(!status);

Assuming we still have a timing issue in here, how about
we poll a little until status != 0 and then continue with warning?

We could recover by finding the 'bit late' status, instead of
oopsing out.

> +		GEM_DEBUG_EXEC(WRITE_ONCE(*(u32 *)(buf + 2 * head), 0));

What I am afraid here is that we change the timing and cache dynamics
for our debug builds so that we bury the pesky thing.

Perhaps I am wandering too far but lets consider for the csb loop:

read head,tail;
rmb();

for_each_csb() {
  64 bit read 
  64 bit write to zero it, unconditionally 
  act_on_it()
}

Too heavy?

Thanks,
Mika
> +
>  		if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
>  			      GEN8_CTX_STATUS_PREEMPTED))
>  			execlists_set_active(execlists,
> -- 
> 2.19.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/execlists: Poison the CSB after use

[Chris Wilson]

Quoting Mika Kuoppala (2018-10-30 09:31:56)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > After reading the event status from the CSB, write back 0 (an invalid
> > value) so we can detect if the HW should signal a new event without
> > writing the event in the future.
> >
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=108315
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 22b57b8926fc..126efe20d2d6 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -910,6 +910,9 @@ static void process_csb(struct intel_engine_cs *engine)
> >                         execlists->active);
> >  
> >               status = buf[2 * head];
> > +             GEM_BUG_ON(!status);
> 
> Assuming we still have a timing issue in here, how about
> we poll a little until status != 0 and then continue with warning?

If there's any race condition here, we definitely do not want to paper
over it.
 
> We could recover by finding the 'bit late' status, instead of
> oopsing out.

Oopsing out tells us where the problem is very concisely.
 
> > +             GEM_DEBUG_EXEC(WRITE_ONCE(*(u32 *)(buf + 2 * head), 0));
> 
> What I am afraid here is that we change the timing and cache dynamics
> for our debug builds so that we bury the pesky thing.

That too is a result.
 
> Perhaps I am wandering too far but lets consider for the csb loop:
> 
> read head,tail;
> rmb();
> 
> for_each_csb() {
>   64 bit read 
>   64 bit write to zero it, unconditionally 
>   act_on_it()
> }
> 
> Too heavy?

Too papery - shouts that we don't know what we or the hw is doing. We
want to pretend that we know what we are doing at least.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/execlists: Poison the CSB after use

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2018-10-30 09:31:56)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > After reading the event status from the CSB, write back 0 (an invalid
>> > value) so we can detect if the HW should signal a new event without
>> > writing the event in the future.
>> >
>> > References: https://bugs.freedesktop.org/show_bug.cgi?id=108315
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_lrc.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> > index 22b57b8926fc..126efe20d2d6 100644
>> > --- a/drivers/gpu/drm/i915/intel_lrc.c
>> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> > @@ -910,6 +910,9 @@ static void process_csb(struct intel_engine_cs *engine)
>> >                         execlists->active);
>> >  
>> >               status = buf[2 * head];
>> > +             GEM_BUG_ON(!status);
>> 
>> Assuming we still have a timing issue in here, how about
>> we poll a little until status != 0 and then continue with warning?
>
> If there's any race condition here, we definitely do not want to paper
> over it.
>  
>> We could recover by finding the 'bit late' status, instead of
>> oopsing out.
>
> Oopsing out tells us where the problem is very concisely.

It would deliver the same information, so not papering over. Only
benefit is that with this signalling it wont be lost.

>  
>> > +             GEM_DEBUG_EXEC(WRITE_ONCE(*(u32 *)(buf + 2 * head), 0));
>> 
>> What I am afraid here is that we change the timing and cache dynamics
>> for our debug builds so that we bury the pesky thing.
>
> That too is a result.

Agreed, so you want to observe behaviour with and without.

>> Perhaps I am wandering too far but lets consider for the csb loop:
>> 
>> read head,tail;
>> rmb();
>> 
>> for_each_csb() {
>>   64 bit read 
>>   64 bit write to zero it, unconditionally 
>>   act_on_it()
>> }
>> 
>> Too heavy?
>
> Too papery - shouts that we don't know what we or the hw is doing. We
> want to pretend that we know what we are doing at least.

Fair enough. Mainly the amount of reads with and without debugs, changes
inside the csb loop was my concern. But that view should be static to
cpu at this point regardless.

So lets try to find out what exactly how the hardware writes
the csb entries.

This patch does give us more details,
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx