Re: [PATCH v3] test/gem_mocs_settings: Testing MOCS register settings

[Peter Antoine <peter.antoine@intel.com>]

On Mon, 11 Apr 2016, Chris Wilson wrote:

> 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!
>
BXT no error, SKL error (different versions of the kernel).
Added missing read_domains.

>>>> +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
>
>
Moved out of do_ream_domain()

Found an issue on SKL, it does not have the snoop bit that the BXT has, 
changed test so that the dirty check takes account of the different 
settable bits.

Version 4, coming now.

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