[Intel-gfx] [PATCH v3] test/gem_mocs_settings: Testing MOCS register settings

[Chris Wilson]

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