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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
