[Intel-gfx] [PATCH 35/37] drm/i915: Live testing for context execution

[Chris Wilson]

On Fri, Jan 13, 2017 at 04:28:58PM +0200, Joonas Lahtinen wrote:
> On ke, 2017-01-11 at 21:09 +0000, Chris Wilson wrote:
> <SNIP>
> 
> For future synchronization purposes, maybe document where the below was
> cloned from?

It is a routine I've written many times.

> > +	offset = PAGE_SIZE * first_page + offset_in_page;
> > +	offset += vma->node.start;
> > +	for (sz = 0; sz < count; sz++) {
> > +		if (gen >= 8) {
> > +			*cmd++ = MI_STORE_DWORD_IMM_GEN4;
> > +			*cmd++ = lower_32_bits(offset);
> > +			*cmd++ = upper_32_bits(offset);
> > +			*cmd++ = value;
> > +		} else if (gen >= 6) {
> > +			*cmd++ = MI_STORE_DWORD_IMM_GEN4;
> > +			*cmd++ = 0;
> > +			*cmd++ = offset;

> > +static int gpu_fill(struct drm_i915_gem_object *obj,
> > +		    struct i915_gem_context *ctx)
> > +{
> > +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> > +	const unsigned long npages = obj->base.size >> PAGE_SHIFT;
> > +	struct i915_address_space *vm = ctx->ppgtt ? &ctx->ppgtt->base : &i915->ggtt.base;
> 
> vm = &(ctx->ppgtt ?: &i915->ggtt)->base? Or does GCC bork up.

Different types, gcc doesn't like them inside the ternary.
 
> Long line anyway.
> 
> > +	struct intel_engine_cs *engine = i915->engine[RCS];
> 
> rcs_fill as function name (rcs_fill_pages too)?

Now using all engines. Just let me be lazy, just once?

> > +	err = i915_gem_object_set_to_gtt_domain(obj, false);
> 
> Isn't the object most definitely going to be written by GPU?

Yes. But, before we begin I want any CPU writes to be flushed to memory
(if not LLC).
 
> > +
> > +	err = i915_vma_pin(vma, 0, 0, PIN_USER);
> > +	if (err)
> > +		return err;
> > +
> > +	GEM_BUG_ON(!IS_ALIGNED(npages, 1024));
> 
> Ok, #define time 1024 is a very magicy.

You're going to love the replacement magic. No spoilers I'm afraid.
 
> > +		}
> > +
> > +		i915_switch_context(rq);
> 
> GEM_BUG_ON(rq->engine != engine) to help readability.

Don't see how that helps readibility. That's a check that should in the
earlier request construction tests.
 
> This all makes me think how strange our internal API actually is.

Yes.
 
> > +
> > +		ww_mutex_lock(&obj->resv->lock, NULL);
> > +		reservation_object_add_excl_fence(obj->resv, &rq->fence);
> 
> Wasn't there a patch not to mess with the reservation internals (aka,
> wrap it?)

My bad, I keep thinking that it is still private to my tree.
 
> > +		ww_mutex_unlock(&obj->resv->lock);
> > +
> > +		__i915_add_request(rq, true);
> > +	}
> 
> I imagine this work submission helper might come in handy as a separate
> thing?

Not quite yet. But something like this. Kind of waiting for more users
plus. There are some quirks that need some care to avoid upsetting
random tests.
 
> > +	for (n = 0; !err && n < 1024; n++) {
> > +		u32 *map;
> > +
> > +		map = kmap_atomic(i915_gem_object_get_page(obj, n));
> 
> Does some test check the kmap works?

No, it's documented as "just works". So if it doesn't we have plenty of
explosions every where.

> > +static int igt_ctx_exec(void *arg)
> > +{
> 
> <SNIP>
> 
> > +	mutex_lock(&i915->drm.struct_mutex);
> > +	while (!time_after(jiffies, end_time)) {
> 
> Time budgeted function?
> 
> > +		vm = ctx->ppgtt ? &ctx->ppgtt->base : &i915->ggtt.base;
> > +		npages = min(vm->total / 2, 1024ull * 1024 * PAGE_SIZE);
> > +		npages >>= PAGE_SHIFT + 10;
> > +		npages <<= PAGE_SHIFT + 10;
> 
> What? Comment please.

Drop the low 22 bits. What, you'd rather have round_down()? Heresy!

> > +	count = 0;
> > +	list_for_each_entry(obj, &objects, batch_pool_link) {
> > +		if (!err)
> > +			err = cpu_check(obj, count);
> 
> break; count is not used after this point, so why does it matter after?

It was freeing objects until the fake_file trick.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre