On Mon, Apr 11, 2016 at 04:02:17PM +0100, Peter Antoine wrote:
> >>+ for (index = 0, offset = 0; index < size; index++, offset += 4)
> >>+ {
> >>+ batch[offset] = MI_STORE_REGISTER_MEM_64_BIT_ADDR;
> >>+ batch[offset+1] = reg_base + (index * sizeof(uint32_t));
> >>+ batch[offset+2] = index * sizeof(uint32_t); /* reloc */
> >>+ batch[offset+3] = 0;
> >>+
> >>+ reloc[index].offset = (offset + 2) * sizeof(uint32_t);
> >>+ reloc[index].delta = index * sizeof(uint32_t);
> >>+ reloc[index].target_handle = dst_handle;
> >>+ reloc[index].write_domain = I915_GEM_DOMAIN_RENDER;
> >
> >Will cause an -EINVAL as you are specifing a write_domain but not the
> >corresponding read_domain.
> Removed.
I hope you meant added the missing read_domains!
> (Did not cause an error when tested)
Hmm, ok we don't actually warn about that!
> >>+static void do_read_registers(int fd,
> >>+ uint32_t ctx_id,
> >>+ uint32_t dst_handle,
> >>+ uint32_t reg_base,
> >>+ uint32_t size,
> >>+ uint32_t engine_id)
> >>+{
> >>+ gem_execbuf(fd, &execbuf);
> >>+ gem_wait(fd, dst_handle, &timeout_ns);
> >
> >As mentioned earlier, tried to avoid manual wait/sync so that we stress
> >the kernel harder to do the right thing. Sometimes the sync is required
> >as part of the test, but only rarely and should be well documented.
> replaced with set_domain.
>
> >
> >>+ gem_close(fd, handle);
> >>+}
> >
> >>+static void check_control_registers(int fd,
> >>+ const struct intel_execution_engine *engine,
> >>+ uint32_t ctx_id,
> >>+ bool dirty)
> >>+{
> >>+ uint32_t dst_handle = gem_create(fd, 4096);
> >>+ uint32_t reg_base = get_engine_base(engine->exec_id | engine->flags);
> >>+ uint32_t *read_regs;
> >>+ struct mocs_table table;
> >>+
> >>+ igt_assert(get_mocs_settings(intel_get_drm_devid(fd), &table, dirty));
> >>+
> >>+ do_read_registers(fd,
> >>+ ctx_id,
> >>+ dst_handle,
> >>+ reg_base,
> >>+ table.size,
> >>+ engine->exec_id | engine->flags);
> >>+
> >>+ read_regs = gem_mmap__cpu(fd, dst_handle, 0, 4096, PROT_READ);
> >
> >Here you need the corresponding
> >gem_set_comdin(fd, dst_handle, DOMAIN_CPU, 0);
> >and in check_l3cc_registers.
> Added into do_read_registers() so should handle both.
But less explicit and doesn't document the code as well. Move the
set-domain next to the usage of dst_handle in the CPU domain.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
