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 =