Re: [Intel-gfx] [PATCH 06/38] drm/i915/selftests: Check that whitelisted registers are accessible
Quoting Michał Winiarski (2019-03-01 15:18:58)
> On Fri, Mar 01, 2019 at 02:03:32PM +, Chris Wilson wrote:
> > +static struct i915_vma *create_scratch(struct i915_gem_context *ctx)
> > +{
> > + struct drm_i915_gem_object *obj;
> > + struct i915_vma *vma;
> > + void *ptr;
> > + int err;
> > +
> > + obj = i915_gem_object_create_internal(ctx->i915, PAGE_SIZE);
> > + if (IS_ERR(obj))
> > + return ERR_CAST(obj);
> > +
> > + i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
>
> Check return value.
Can't fail, but transform it into set_coherency which is void so you
can't complain :-p
> > + ptr = i915_gem_object_pin_map(obj, I915_MAP_WB);
> > + if (IS_ERR(ptr)) {
> > + err = PTR_ERR(ptr);
> > + goto err_obj;
> > + }
> > + memset(ptr, 0xc5, PAGE_SIZE);
> > + i915_gem_object_unpin_map(obj);
> > +
> > + vma = i915_vma_instance(obj, >ppgtt->vm, NULL);
> > + if (IS_ERR(vma)) {
> > + err = PTR_ERR(vma);
> > + goto err_obj;
> > + }
> > +
> > + err = i915_vma_pin(vma, 0, 0, PIN_USER);
> > + if (err)
> > + goto err_obj;
> > +
> > + err = i915_gem_object_set_to_cpu_domain(obj, false);
> > + if (err)
> > + goto err_obj;
> > +
> > + return vma;
> > +
> > +err_obj:
> > + i915_gem_object_put(obj);
> > + return ERR_PTR(err);
> > +}
> > +
[more snip]
> > + if (wo_register(engine, reg))
> > + continue;
> > +
> > + srm = MI_STORE_REGISTER_MEM;
> > + lrm = MI_LOAD_REGISTER_MEM;
> > + if (INTEL_GEN(ctx->i915) >= 8)
> > + lrm++, srm++;
> > +
> > + pr_debug("%s: Writing garbage to %x {srm=0x%08x,
> > lrm=0x%08x}\n",
> > + engine->name, reg, srm, lrm);
>
> Why are we printing opcodes (srm/lrm)?
In a debug, can you guess? Because despite making a lrm variable I used
MI_LRM later on and spent a few runs wondering why the GPU kept hanging
with the wrong opcode. Consider it gone.
> > + cs = i915_gem_object_pin_map(scratch->obj, I915_MAP_WB);
> > + if (IS_ERR(cs)) {
> > + err = PTR_ERR(cs);
> > + goto out_batch;
> > + }
>
> We're already using cs for batch! Extra pointer pls.
Will someone think of the poor electrons! Or is more, jobs for all!
> > +
> > + GEM_BUG_ON(values[ARRAY_SIZE(values) - 1] != 0x);
> > + rsvd = cs[ARRAY_SIZE(values)]; /* detect write masking */
>
> So we're writing 0x to get the mask. And there's a comment. And it
> will
> explode if someone changes the last value.
>
> Reviewed-by: Michał Winiarski
It'll do for now, there's a bit more I think we can improve on, but
incremental steps.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/38] drm/i915/selftests: Check that whitelisted registers are accessible
On Fri, Mar 01, 2019 at 02:03:32PM +, Chris Wilson wrote:
> There is no point in whitelisting a register that the user then cannot
> write to, so check the register exists before merging such patches.
>
> v2: Mark SLICE_COMMON_ECO_CHICKEN1 [731c] as write-only
>
> Signed-off-by: Chris Wilson
> Cc: Dale B Stimson
> Cc: Michał Winiarski
> ---
> .../drm/i915/selftests/intel_workarounds.c| 376 ++
> 1 file changed, 376 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> index e6ffc8ac22dc..33b3ced83fde 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> @@ -12,6 +12,14 @@
> #include "igt_spinner.h"
> #include "igt_wedge_me.h"
> #include "mock_context.h"
> +#include "mock_drm.h"
> +
> +static const struct wo_register {
> + enum intel_platform platform;
> + u32 reg;
> +} wo_registers[] = {
> + { INTEL_GEMINILAKE, 0x731c }
> +};
>
> #define REF_NAME_MAX (INTEL_ENGINE_CS_MAX_NAME + 4)
> struct wa_lists {
> @@ -331,6 +339,373 @@ static int check_whitelist_across_reset(struct
> intel_engine_cs *engine,
> return err;
> }
>
> +static struct i915_vma *create_scratch(struct i915_gem_context *ctx)
> +{
> + struct drm_i915_gem_object *obj;
> + struct i915_vma *vma;
> + void *ptr;
> + int err;
> +
> + obj = i915_gem_object_create_internal(ctx->i915, PAGE_SIZE);
> + if (IS_ERR(obj))
> + return ERR_CAST(obj);
> +
> + i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
Check return value.
> +
> + ptr = i915_gem_object_pin_map(obj, I915_MAP_WB);
> + if (IS_ERR(ptr)) {
> + err = PTR_ERR(ptr);
> + goto err_obj;
> + }
> + memset(ptr, 0xc5, PAGE_SIZE);
> + i915_gem_object_unpin_map(obj);
> +
> + vma = i915_vma_instance(obj, >ppgtt->vm, NULL);
> + if (IS_ERR(vma)) {
> + err = PTR_ERR(vma);
> + goto err_obj;
> + }
> +
> + err = i915_vma_pin(vma, 0, 0, PIN_USER);
> + if (err)
> + goto err_obj;
> +
> + err = i915_gem_object_set_to_cpu_domain(obj, false);
> + if (err)
> + goto err_obj;
> +
> + return vma;
> +
> +err_obj:
> + i915_gem_object_put(obj);
> + return ERR_PTR(err);
> +}
> +
[SNIP]
> +static int check_dirty_whitelist(struct i915_gem_context *ctx,
> + struct intel_engine_cs *engine)
> +{
> + const u32 values[] = {
> + 0x,
> + 0x01010101,
> + 0x10100101,
> + 0x03030303,
> + 0x30300303,
> + 0x05050505,
> + 0x50500505,
> + 0x0f0f0f0f,
> + 0xf00ff00f,
> + 0x10101010,
> + 0xf0f01010,
> + 0x30303030,
> + 0xa0a03030,
> + 0x50505050,
> + 0xc0c05050,
> + 0xf0f0f0f0,
> + 0x,
> + 0x,
> + 0x,
> + 0x,
> + 0x00ff00ff,
> + 0xffff,
> + 0x00ff,
> + 0x,
> + };
> + struct i915_vma *scratch;
> + struct i915_vma *batch;
> + int err = 0, i, v;
> + u32 *cs;
> +
> + scratch = create_scratch(ctx);
> + if (IS_ERR(scratch))
> + return PTR_ERR(scratch);
> +
> + batch = create_batch(ctx);
> + if (IS_ERR(batch)) {
> + err = PTR_ERR(batch);
> + goto out_scratch;
> + }
> +
> + for (i = 0; i < engine->whitelist.count; i++) {
> + u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> + u64 addr = scratch->node.start;
> + struct i915_request *rq;
> + u32 srm, lrm, rsvd;
> + u32 expect;
> + int idx;
> +
> + if (wo_register(engine, reg))
> + continue;
> +
> + srm = MI_STORE_REGISTER_MEM;
> + lrm = MI_LOAD_REGISTER_MEM;
> + if (INTEL_GEN(ctx->i915) >= 8)
> + lrm++, srm++;
> +
> + pr_debug("%s: Writing garbage to %x {srm=0x%08x, lrm=0x%08x}\n",
> + engine->name, reg, srm, lrm);
Why are we printing opcodes (srm/lrm)?
> +
> + cs = i915_gem_object_pin_map(batch->obj, I915_MAP_WC);
> + if (IS_ERR(cs)) {
> + err = PTR_ERR(cs);
> + goto out_batch;
> + }
> +
> + /* SRM original */
> + *cs++ = srm;
> + *cs++ = reg;
> + *cs++ = lower_32_bits(addr);
> + *cs++ = upper_32_bits(addr);
> +
> + idx = 1;
> + for (v = 0; v < ARRAY_SIZE(values); v++) {
> + /* LRI garbage */
> + *cs++ = MI_LOAD_REGISTER_IMM(1);
> + *cs++ =
[Intel-gfx] [PATCH 06/38] drm/i915/selftests: Check that whitelisted registers are accessible
There is no point in whitelisting a register that the user then cannot
write to, so check the register exists before merging such patches.
v2: Mark SLICE_COMMON_ECO_CHICKEN1 [731c] as write-only
Signed-off-by: Chris Wilson
Cc: Dale B Stimson
Cc: Michał Winiarski
---
.../drm/i915/selftests/intel_workarounds.c| 376 ++
1 file changed, 376 insertions(+)
diff --git a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
index e6ffc8ac22dc..33b3ced83fde 100644
--- a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
@@ -12,6 +12,14 @@
#include "igt_spinner.h"
#include "igt_wedge_me.h"
#include "mock_context.h"
+#include "mock_drm.h"
+
+static const struct wo_register {
+ enum intel_platform platform;
+ u32 reg;
+} wo_registers[] = {
+ { INTEL_GEMINILAKE, 0x731c }
+};
#define REF_NAME_MAX (INTEL_ENGINE_CS_MAX_NAME + 4)
struct wa_lists {
@@ -331,6 +339,373 @@ static int check_whitelist_across_reset(struct
intel_engine_cs *engine,
return err;
}
+static struct i915_vma *create_scratch(struct i915_gem_context *ctx)
+{
+ struct drm_i915_gem_object *obj;
+ struct i915_vma *vma;
+ void *ptr;
+ int err;
+
+ obj = i915_gem_object_create_internal(ctx->i915, PAGE_SIZE);
+ if (IS_ERR(obj))
+ return ERR_CAST(obj);
+
+ i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
+
+ ptr = i915_gem_object_pin_map(obj, I915_MAP_WB);
+ if (IS_ERR(ptr)) {
+ err = PTR_ERR(ptr);
+ goto err_obj;
+ }
+ memset(ptr, 0xc5, PAGE_SIZE);
+ i915_gem_object_unpin_map(obj);
+
+ vma = i915_vma_instance(obj, >ppgtt->vm, NULL);
+ if (IS_ERR(vma)) {
+ err = PTR_ERR(vma);
+ goto err_obj;
+ }
+
+ err = i915_vma_pin(vma, 0, 0, PIN_USER);
+ if (err)
+ goto err_obj;
+
+ err = i915_gem_object_set_to_cpu_domain(obj, false);
+ if (err)
+ goto err_obj;
+
+ return vma;
+
+err_obj:
+ i915_gem_object_put(obj);
+ return ERR_PTR(err);
+}
+
+static struct i915_vma *create_batch(struct i915_gem_context *ctx)
+{
+ struct drm_i915_gem_object *obj;
+ struct i915_vma *vma;
+ int err;
+
+ obj = i915_gem_object_create_internal(ctx->i915, 16 * PAGE_SIZE);
+ if (IS_ERR(obj))
+ return ERR_CAST(obj);
+
+ vma = i915_vma_instance(obj, >ppgtt->vm, NULL);
+ if (IS_ERR(vma)) {
+ err = PTR_ERR(vma);
+ goto err_obj;
+ }
+
+ err = i915_vma_pin(vma, 0, 0, PIN_USER);
+ if (err)
+ goto err_obj;
+
+ err = i915_gem_object_set_to_wc_domain(obj, true);
+ if (err)
+ goto err_obj;
+
+ return vma;
+
+err_obj:
+ i915_gem_object_put(obj);
+ return ERR_PTR(err);
+}
+
+static u32 reg_write(u32 old, u32 new, u32 rsvd)
+{
+ if (rsvd == 0x) {
+ old &= ~(new >> 16);
+ old |= new & (new >> 16);
+ } else {
+ old &= ~rsvd;
+ old |= new & rsvd;
+ }
+
+ return old;
+}
+
+static bool wo_register(struct intel_engine_cs *engine, u32 reg)
+{
+ enum intel_platform platform = INTEL_INFO(engine->i915)->platform;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(wo_registers); i++) {
+ if (wo_registers[i].platform == platform &&
+ wo_registers[i].reg == reg)
+ return true;
+ }
+
+ return false;
+}
+
+static int check_dirty_whitelist(struct i915_gem_context *ctx,
+struct intel_engine_cs *engine)
+{
+ const u32 values[] = {
+ 0x,
+ 0x01010101,
+ 0x10100101,
+ 0x03030303,
+ 0x30300303,
+ 0x05050505,
+ 0x50500505,
+ 0x0f0f0f0f,
+ 0xf00ff00f,
+ 0x10101010,
+ 0xf0f01010,
+ 0x30303030,
+ 0xa0a03030,
+ 0x50505050,
+ 0xc0c05050,
+ 0xf0f0f0f0,
+ 0x,
+ 0x,
+ 0x,
+ 0x,
+ 0x00ff00ff,
+ 0xffff,
+ 0x00ff,
+ 0x,
+ };
+ struct i915_vma *scratch;
+ struct i915_vma *batch;
+ int err = 0, i, v;
+ u32 *cs;
+
+ scratch = create_scratch(ctx);
+ if (IS_ERR(scratch))
+ return PTR_ERR(scratch);
+
+ batch = create_batch(ctx);
+ if (IS_ERR(batch)) {
+ err = PTR_ERR(batch);
+ goto out_scratch;
+ }
+
+ for (i = 0; i < engine->whitelist.count; i++) {
+ u32 reg =
