Re: [PATCH 06/38] drm/i915/selftests: Check that whitelisted registers are accessible

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Michał Winiarski (2019-03-01 15:18:58)
> On Fri, Mar 01, 2019 at 02:03:32PM +0000, 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, &ctx->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] != 0xffffffff);
> > +             rsvd = cs[ARRAY_SIZE(values)]; /* detect write masking */
> 
> So we're writing 0xffffffff to get the mask. And there's a comment. And it will
> explode if someone changes the last value.
> 
> Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

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