On Mon, 11 Apr 2016, Chris Wilson wrote:

> On Mon, Apr 11, 2016 at 01:51:25PM +0100, Peter Antoine wrote:
>> The MOCS registers were added in Gen9 and define the caching policy.
>> The registers are split into two sets. The first set controls the
>> EDRAM policy and have a set for each engine, the second set controls
>> the L3 policy. The two sets use the same index.
>>
>> The RCS registers and the L3CC registers are stored in the RCS context.
>>
>> The test checks that the registers are correct by checking the values by
>> directly reading them via MMIO, then again it tests them by reading them
>> from within a batch buffer. RCS engine is tested last as it programs the
>> registers via a batch buffer and this will invalidate the test for
>> workloads that don't use the render ring or don't run a render batch
>> first.
>>
>> v2: Reorganised the structure.
>>     Added more tests. (Chris Wilson)
>> v3: Fixed a few bugs. (Chris Wilson)
>>
>> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
>> ---
>>  lib/ioctl_wrappers.c      |  13 ++
>>  lib/ioctl_wrappers.h      |   1 +
>>  tests/Makefile.sources    |   1 +
>>  tests/gem_mocs_settings.c | 559 ++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 574 insertions(+)
>>  create mode 100644 tests/gem_mocs_settings.c
>>
>> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
>> index 076bce8..8d74128 100644
>> --- a/lib/ioctl_wrappers.c
>> +++ b/lib/ioctl_wrappers.c
>> @@ -1382,6 +1382,19 @@ void gem_require_ring(int fd, unsigned ring)
>>  	igt_require(gem_has_ring(fd, ring));
>>  }
>>
>> +/**
>> + * gem_has_mocs:
>> + * @fd: open i915 drm file descriptor
>> + *
>> + * Feature test macro to query whether the device has MOCS registers.
>> + * These exist gen 9+.
>> + */
>> +void gem_has_mocs(int fd)
>
> gem_has_mocs() returns bool
> gem_require_mocs() calls igt_require() and doesn't return if not
> available.
>
Done.
>> +{
>> +	const int gen = intel_gen(intel_get_drm_devid(fd));
>> +	igt_require(gen >= 9);
>> +}
>> +
>>  /* prime */
>>
>>  /**
>> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
>> index d986f61..ebedf1e 100644
>> --- a/lib/ioctl_wrappers.h
>> +++ b/lib/ioctl_wrappers.h
>> @@ -154,6 +154,7 @@ bool gem_has_softpin(int fd);
>>  void gem_require_caching(int fd);
>>  bool gem_has_ring(int fd, unsigned ring);
>>  void gem_require_ring(int fd, unsigned ring);
>> +void gem_has_mocs(int fd);
>>
>>  /* prime */
>>  struct local_dma_buf_sync {
>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>> index 43f232f..d483c9e 100644
>> --- a/tests/Makefile.sources
>> +++ b/tests/Makefile.sources
>> @@ -148,6 +148,7 @@ TESTS_progs = \
>>  	gem_lut_handle \
>>  	gem_mmap_offset_exhaustion \
>>  	gem_media_fill \
>> +	gem_mocs_settings \
>>  	gem_gpgpu_fill \
>>  	gem_pin \
>>  	gem_reg_read \
>> diff --git a/tests/gem_mocs_settings.c b/tests/gem_mocs_settings.c
>> new file mode 100644
>> index 0000000..03c5089
>> --- /dev/null
>> +++ b/tests/gem_mocs_settings.c
>> @@ -0,0 +1,559 @@
>> +/*
>> + * Copyright © 2016 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +/** @file gem_mocs_settings.c
>> + *
>> + * Check that the MOCs cache settings are valid.
>> + */
>> +
>> +#include "igt.h"
>> +#include "igt_gt.h"
>> +
>> +#define MAX_NUMBER_MOCS_REGISTERS	(64)
>> +
>> +enum {
>> +	NONE,
>> +	RESET,
>> +	SUSPEND,
>> +	HIBERNATE
>> +};
>> +
>> +#define GEN9_LNCFCMOCS0		(0xB020)	/* L3 Cache Control base */
>> +#define GEN9_GFX_MOCS_0		(0xc800)	/* Graphics MOCS base register*/
>> +#define GEN9_MFX0_MOCS_0	(0xc900)	/* Media 0 MOCS base register*/
>> +#define GEN9_MFX1_MOCS_0	(0xcA00)	/* Media 1 MOCS base register*/
>> +#define GEN9_VEBOX_MOCS_0	(0xcB00)	/* Video MOCS base register*/
>> +#define GEN9_BLT_MOCS_0		(0xcc00)	/* Blitter MOCS base register*/
>> +
>> +#define ENGINE_GFX	(I915_EXEC_RENDER)
>> +#define ENGINE_MFX0	(I915_EXEC_BSD)
>> +#define ENGINE_MFX0_ALT	(I915_EXEC_BSD | 1 << 13)
>> +#define ENGINE_MFX1	(I915_EXEC_BSD | 2 << 13)
>> +#define ENGINE_BLT	(I915_EXEC_BLT)
>> +#define ENGINE_VEBOX	(I915_EXEC_VEBOX)
>> +
>> +struct mocs_entry {
>> +	uint32_t	control_value;
>> +	uint16_t	l3cc_value;
>> +};
>> +
>> +struct mocs_table {
>> +	uint32_t		size;
>> +	const struct mocs_entry	*table;
>> +};
>> +
>
> /* The first entries in the MOCS tables are defined by uABI */
>
Added.

>> +static const struct mocs_entry skylake_mocs_table[] = {
>> +	{ 0x00000009, 0x0010 },
>> +	{ 0x00000038, 0x0030 },
>> +	{ 0x0000003b, 0x0030 },
>> +};
>> +
>> +static const struct mocs_entry broxton_mocs_table[] = {
>> +	{ 0x00000009, 0x0010 },
>> +	{ 0x00000038, 0x0030 },
>> +	{ 0x0000003b, 0x0030 },
>> +};
>> +
>> +static const struct mocs_entry dirty_table_mocs_table[] = {
>> +	{ 0x00007FFF, 0x003F },
>> +	{ 0x00007FFF, 0x003F },
>> +	{ 0x00007FFF, 0x003F },
>> +};
>> +
>> +static const uint32_t write_values[] = {
>> +	0xFFFFFFFF,
>> +	0xFFFFFFFF,
>> +	0xFFFFFFFF,
>> +	0xFFFFFFFF
>> +};
>
>> +static void test_mocs_values(int fd)
>> +{
>> +	const struct intel_execution_engine *e;
>> +
>> +	for (e = intel_execution_engines; e->name; e++)
>> +		if (e->exec_id != I915_EXEC_DEFAULT &&
>> +		    gem_has_ring(fd, e->exec_id | e->flags))
>
> You could use
>
> unsigned engine;
>
> for_each_engine(fd, engine)
> 	if (engine != 0) test_mocs_control_values(fd, engine);
>
> Just to hide the gem_has_ring().
Done.

>
>> +
>> +
>> +	test_mocs_l3cc_values(fd);
>> +}
>> +
>> +#define MI_STORE_REGISTER_MEM_64_BIT_ADDR	((0x24 << 23) | 2)
>> +
>> +static int create_read_batch(struct drm_i915_gem_relocation_entry *reloc,
>> +			     uint32_t *batch,
>> +			     uint32_t dst_handle,
>> +			     uint32_t size,
>> +			     uint32_t reg_base)
>> +{
>> +	unsigned int index;
>> +	unsigned int offset = 0;
>> +
>> +	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.
(Did not cause an error when tested)

>
>> +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.

>
>> +
>> +	for (int index = 0; index < table.size; index++)
>> +		igt_assert_eq(read_regs[index],
>> +				table.table[index].control_value);
>> +
>> +	munmap(read_regs, 4096);
>> +	gem_close(fd, dst_handle);
>> +}
>
> Otherwise, looking good. (Could use snooping or gem_read to minimise the
> clflush on bxt, but I don't think anyone will notice over 4k).
> -Chris
>
>
Ok.

Will run it on something not a BXT and then push again.
So the other patched are good as well?

Thanks for the review.
Peter.

--
    Peter Antoine (Android Graphics Driver Software Engineer)
    ---------------------------------------------------------------------
    Intel Corporation (UK) Limited
    Registered No. 1134945 (England)
    Registered Office: Pipers Way, Swindon SN3 1RJ
    VAT No: 860 2173 47