Re: [Intel-gfx] [PATCH v2] drm/i915: Check whitelist registers across resets

[Chris Wilson]

Quoting Oscar Mateo (2018-04-13 18:04:16)
> 
> 
> On 4/13/2018 9:54 AM, Chris Wilson wrote:
> > Quoting Oscar Mateo (2018-04-13 17:46:42)
> >>
> >> On 4/12/2018 8:21 AM, Chris Wilson wrote:
> >>> Add a selftest to ensure that we restore the whitelisted registers after
> >>> rewrite the registers everytime they might be scrubbed, e.g. module
> >>> load, reset and resume. For the other volatile workaround registers, we
> >>> export their presence via debugfs and check in igt/gem_workarounds.
> >>> However, we don't export the whitelist and rather than do so, let's test
> >>> them directly in the kernel.
> >> I guess my question is... why? what was the problem with exporting the
> >> list of whitelist registers in debugfs?
> > We don't... (There's no RING_NONPRIV checking currently)
> 
> There is no checking, but we were showing the full list in debugfs. Ok, 
> I guess it wasn't that useful without the corresponding igt...

Oh no we weren't. wa_ring_whitelist_reg() isn't storing the reg in the
wa list, just that we have used the RING_NONPRIV slot. At one point, I
think the intention was there but that seems to disappeared and now I
removed the notion entirely ;)

> > I wasn't fond
> > of the igt for it is checking kernel implementation rather than behaviour.
> > The kernel gives it a checklist which it dutifully follows... Now that
> > we have selftests, we don't need to write what I think should be unit
> > tests in igt anymore.
> 
> Ah, so I take the plan is to also check the other WAs in selftests? 
> Somehow I thought you wanted to treat whitelisting differently.

Right, the long term goal will be to move all the workaround testing
here. It just so happens that I wrote a buggy patch that CI was happy
with that alerted me to the lack of testing ;)

The only fly in the ointment is doing S3/S4 testing, as doing
suspend/resume from inside the kernel tricky (trickier than even getting
it right from userspace). So I think we may just have to be a little
more creative, and do something like call i915_drv_suspend() directly.
Hmm, now that's an idea. (i915_drv_suspend(); scribble over state;
i915_drv_resume()).
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Check whitelist registers across resets

[Oscar Mateo]

On 4/13/2018 9:54 AM, Chris Wilson wrote:

Quoting Oscar Mateo (2018-04-13 17:46:42)


On 4/12/2018 8:21 AM, Chris Wilson wrote:

Add a selftest to ensure that we restore the whitelisted registers after
rewrite the registers everytime they might be scrubbed, e.g. module
load, reset and resume. For the other volatile workaround registers, we
export their presence via debugfs and check in igt/gem_workarounds.
However, we don't export the whitelist and rather than do so, let's test
them directly in the kernel.

I guess my question is... why? what was the problem with exporting the
list of whitelist registers in debugfs?

We don't... (There's no RING_NONPRIV checking currently)


There is no checking, but we were showing the full list in debugfs. Ok, 
I guess it wasn't that useful without the corresponding igt...



I wasn't fond
of the igt for it is checking kernel implementation rather than behaviour.
The kernel gives it a checklist which it dutifully follows... Now that
we have selftests, we don't need to write what I think should be unit
tests in igt anymore.


Ah, so I take the plan is to also check the other WAs in selftests? 
Somehow I thought you wanted to treat whitelisting differently.



The test we use is to read the registers back from the CS (this helps us
be sure that the registers will be valid for MI_LRI etc). In order to
generate the expected list, we split intel_whitelist_workarounds_emit
into two phases, the first to build the list and the second to apply.
Inside the test, we only build the list and then check that list against
the hw.

v2: Filter out pre-gen8 as they do not have RING_NONPRIV.

Signed-off-by: Chris Wilson 
Cc: Oscar Mateo 
Cc: Mika Kuoppala 
Cc: Joonas Lahtinen 
---
+static struct whitelist *whitelist_build(struct intel_engine_cs *engine,
+  struct whitelist *w)
+{
+ struct drm_i915_private *i915 = engine->i915;
+
+ GEM_BUG_ON(engine->id != RCS);
+
+ w->count = 0;
+ w->nopid = i915_mmio_reg_offset(RING_NOPID(engine->mmio_base));

:)


+
+ if (INTEL_GEN(i915) < 8)
+ return NULL;
+ else if (IS_BROADWELL(i915))
+ bdw_whitelist_build(engine, w);

Is it worth passing the engine around? Even of we end up with
whitelisted register in engines != RCS, we will need more changes than this.

No idea, it was easy enough to pass around, so I did just in case it was
useful in future (pulling out i915 or whatever).
-Chris


I'd say let's cross that bridge when we get to it, but with or without it:

Reviewed-by: Oscar Mateo 

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

Re: [Intel-gfx] [PATCH v2] drm/i915: Check whitelist registers across resets

[Chris Wilson]

Quoting Oscar Mateo (2018-04-13 17:46:42)
> 
> 
> On 4/12/2018 8:21 AM, Chris Wilson wrote:
> > Add a selftest to ensure that we restore the whitelisted registers after
> > rewrite the registers everytime they might be scrubbed, e.g. module
> > load, reset and resume. For the other volatile workaround registers, we
> > export their presence via debugfs and check in igt/gem_workarounds.
> > However, we don't export the whitelist and rather than do so, let's test
> > them directly in the kernel.
> 
> I guess my question is... why? what was the problem with exporting the 
> list of whitelist registers in debugfs?

We don't... (There's no RING_NONPRIV checking currently) I wasn't fond
of the igt for it is checking kernel implementation rather than behaviour.
The kernel gives it a checklist which it dutifully follows... Now that
we have selftests, we don't need to write what I think should be unit
tests in igt anymore.

> > The test we use is to read the registers back from the CS (this helps us
> > be sure that the registers will be valid for MI_LRI etc). In order to
> > generate the expected list, we split intel_whitelist_workarounds_emit
> > into two phases, the first to build the list and the second to apply.
> > Inside the test, we only build the list and then check that list against
> > the hw.
> >
> > v2: Filter out pre-gen8 as they do not have RING_NONPRIV.
> >
> > Signed-off-by: Chris Wilson 
> > Cc: Oscar Mateo 
> > Cc: Mika Kuoppala 
> > Cc: Joonas Lahtinen 
> > ---
> > +static struct whitelist *whitelist_build(struct intel_engine_cs *engine,
> > +  struct whitelist *w)
> > +{
> > + struct drm_i915_private *i915 = engine->i915;
> > +
> > + GEM_BUG_ON(engine->id != RCS);
> > +
> > + w->count = 0;
> > + w->nopid = i915_mmio_reg_offset(RING_NOPID(engine->mmio_base));
> 
> :)
> 
> > +
> > + if (INTEL_GEN(i915) < 8)
> > + return NULL;
> > + else if (IS_BROADWELL(i915))
> > + bdw_whitelist_build(engine, w);
> 
> Is it worth passing the engine around? Even of we end up with 
> whitelisted register in engines != RCS, we will need more changes than this.

No idea, it was easy enough to pass around, so I did just in case it was
useful in future (pulling out i915 or whatever).
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Check whitelist registers across resets

[Oscar Mateo]

On 4/12/2018 8:21 AM, Chris Wilson wrote:

Add a selftest to ensure that we restore the whitelisted registers after
rewrite the registers everytime they might be scrubbed, e.g. module
load, reset and resume. For the other volatile workaround registers, we
export their presence via debugfs and check in igt/gem_workarounds.
However, we don't export the whitelist and rather than do so, let's test
them directly in the kernel.


I guess my question is... why? what was the problem with exporting the 
list of whitelist registers in debugfs?



The test we use is to read the registers back from the CS (this helps us
be sure that the registers will be valid for MI_LRI etc). In order to
generate the expected list, we split intel_whitelist_workarounds_emit
into two phases, the first to build the list and the second to apply.
Inside the test, we only build the list and then check that list against
the hw.

v2: Filter out pre-gen8 as they do not have RING_NONPRIV.

Signed-off-by: Chris Wilson 
Cc: Oscar Mateo 
Cc: Mika Kuoppala 
Cc: Joonas Lahtinen 
---
  drivers/gpu/drm/i915/i915_debugfs.c   |  14 +-
  drivers/gpu/drm/i915/i915_drv.h   |   1 -
  drivers/gpu/drm/i915/intel_lrc.c  |   8 +-
  drivers/gpu/drm/i915/intel_ringbuffer.c   |   4 +-
  drivers/gpu/drm/i915/intel_workarounds.c  | 215 ++---
  drivers/gpu/drm/i915/intel_workarounds.h  |   2 +-
  .../drm/i915/selftests/i915_live_selftests.h  |   1 +
  .../drm/i915/selftests/intel_workarounds.c| 284 ++
  8 files changed, 389 insertions(+), 140 deletions(-)
  create mode 100644 drivers/gpu/drm/i915/selftests/intel_workarounds.c

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 2e6652a9bb9e..e0274f41bc76 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3304,24 +3304,13 @@ static int i915_shared_dplls_info(struct seq_file *m, 
void *unused)
  
  static int i915_wa_registers(struct seq_file *m, void *unused)

  {
-   int i;
-   int ret;
-   struct intel_engine_cs *engine;
struct drm_i915_private *dev_priv = node_to_i915(m->private);
-   struct drm_device *dev = _priv->drm;
struct i915_workarounds *workarounds = _priv->workarounds;
-   enum intel_engine_id id;
-
-   ret = mutex_lock_interruptible(>struct_mutex);
-   if (ret)
-   return ret;
+   int i;
  
  	intel_runtime_pm_get(dev_priv);
  
  	seq_printf(m, "Workarounds applied: %d\n", workarounds->count);

-   for_each_engine(engine, dev_priv, id)
-   seq_printf(m, "HW whitelist count for %s: %d\n",
-  engine->name, workarounds->hw_whitelist_count[id]);
for (i = 0; i < workarounds->count; ++i) {
i915_reg_t addr;
u32 mask, value, read;
@@ -3337,7 +3326,6 @@ static int i915_wa_registers(struct seq_file *m, void 
*unused)
}
  
  	intel_runtime_pm_put(dev_priv);

-   mutex_unlock(>struct_mutex);
  
  	return 0;

  }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 649c0f2f3bae..15e1260be58e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1297,7 +1297,6 @@ struct i915_wa_reg {
  struct i915_workarounds {
struct i915_wa_reg reg[I915_MAX_WA_REGS];
u32 count;
-   u32 hw_whitelist_count[I915_NUM_ENGINES];
  };
  
  struct i915_virtual_gpu {

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c7c85134a84a..4f728587a756 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1744,9 +1744,7 @@ static int gen8_init_render_ring(struct intel_engine_cs 
*engine)
if (ret)
return ret;
  
-	ret = intel_whitelist_workarounds_apply(engine);

-   if (ret)
-   return ret;
+   intel_whitelist_workarounds_apply(engine);
  
  	/* We need to disable the AsyncFlip performance optimisations in order

 * to use MI_WAIT_FOR_EVENT within the CS. It should already be
@@ -1769,9 +1767,7 @@ static int gen9_init_render_ring(struct intel_engine_cs 
*engine)
if (ret)
return ret;
  
-	ret = intel_whitelist_workarounds_apply(engine);

-   if (ret)
-   return ret;
+   intel_whitelist_workarounds_apply(engine);
  
  	return 0;

  }
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c 
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 757bb0990c07..c68ac605b8a9 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -618,9 +618,7 @@ static int init_render_ring(struct intel_engine_cs *engine)
if (ret)
return ret;
  
-	ret = intel_whitelist_workarounds_apply(engine);

-   if (ret)
-   return ret;
+