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