Re: [igt-dev] [PATCH i-g-t 1/3] lib/intel_memory_region: Add start offset and alignment detection

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Thu, 30 Dec 2021 10:26:11 -0800, Zbigniew Kempczyński wrote:
>

I am still trying to understand this patch so just some questions/comments
for now.

I am not even looking at the caching part for now, it would have been
better to keep caching as a separate patch since that is performance
improvement but anyway leave as is. I will just review it later.

> With era of new gens we're enforced to use no-reloc (softpin). This
> brings few problems like vm range limitations which were well solved
> by the kernel. This can be handled also in userspace code by adding
> gen related conditionals or by trying to detect the constraints.

So why not just do this since the information is static and gen dependent?

Also what does this offset/alignment really depend on? Is it the minimum
page size supported by a particular gen for a region type? Or is there more
to it?

If we know the minimum page size for each region type per gen can we
compute the min/safe offset/alignemnt values below or do we still need this
dynamic detection?

Also, as far as I see, here we are calculating how generated offsets from
the allocator should be aligned. So rather than do this in the allocator
itself (i.e. introduce a 'region' argument to the allocator) we have these
functions to find the 'alignment' argument to allocator->alloc() function.

But it would be ok to do this inside the allocator itself and introduce a
region argument to the allocator, i.e. generate an offset for this region
or this set of regions? Anyway I see the allocator API at present takes the
alignment argument so it is probably ok as is too.

So at a high level do we need to add these functions here or should we add
them inside the allocator?

> +/**
> + * gem_detect_min_start_offset_for_region:
> + * @i915: drm fd
> + * @region: memory region
> + *
> + * Returns: minimum start offset at which kernel allows placing objects
> + *          for memory region.
> + */
> +uint64_t gem_detect_min_start_offset_for_region(int i915, uint32_t region)

Should this be static, that is should we only expose
gem_detect_safe_start_offset() to the tests? Is there any reason why any
test will call this directly?

Also I suggest:

s/detect/get/ since 'detect' is an implementation detail (and with the
cache we are not even detecting).

> +	batch = gem_mmap__device_coherent(i915, obj.handle, 0, bb_size, PROT_WRITE);
> +	*batch = MI_BATCH_BUFFER_END;
> +	munmap(batch, bb_size);

I prefer gem_write() everywhere since it's a single statement but ok as is too.

> +
> +	while (1) {
> +		obj.offset = start_offset;
> +
> +		if (__gem_execbuf(i915, &eb) == 0)
> +			break;
> +
> +		if (start_offset)
> +			start_offset <<= 1;
> +		else
> +			start_offset = PAGE_SIZE;
> +
> +		igt_assert(start_offset <= 1ull << 48);

s/48/GEN8_GTT_ADDRESS_WIDTH/ or something like that?

> +/**
> + * gem_detect_safe_start_offset:
> + * @i915: drm fd
> + *
> + * Returns: finds start offset which can be used as first one regardless
> + *          memory region. Useful if for some reason some regions don't allow
> + *          starting from 0x0 offset.
> + */
> +uint64_t gem_detect_safe_start_offset(int i915)

Overall I am ok with gem_detect_safe_start_offset() and
gem_detect_min_start_offset_for_region() and understand that we need them.

> +/**
> + * gem_detect_min_alignment_for_regions:
> + * @i915: drm fd
> + * @region1: first region
> + * @region2: second region
> + *
> + * Returns: minimum alignment which must be used when objects from @region1 and
> + * @region2 are going to interact.
> + */
> +uint64_t gem_detect_min_alignment_for_regions(int i915,

Once again can this be static, that is we don't expose to the tests?

> +					      uint32_t region1,
> +					      uint32_t region2)

Also we can probably pass the list of all regions here (similar to what we
do for gem_create_in_memory_regions()) but probably ok as is too the way
it's done in gem_detect_safe_alignment().

> +	obj[0].offset = gem_detect_min_start_offset_for_region(i915, region1);
> +
> +	/* Find appropriate alignment of object */
> +	eb.buffer_count = ARRAY_SIZE(obj);
> +	igt_assert(__gem_create_in_memory_regions(i915, &obj[1].handle,
> +						  &obj_size, region2) == 0);
> +	obj[1].handle = gem_create_in_memory_regions(i915, PAGE_SIZE, region2);
> +	obj[1].flags = EXEC_OBJECT_PINNED;
> +	while (1) {
> +		obj[1].offset = ALIGN(obj[0].offset + bb_size, min_alignment);
> +		igt_assert(obj[1].offset <= 1ull << 32);
> +
> +		if (__gem_execbuf(i915, &eb) == 0)
> +			break;
> +
> +		min_alignment <<= 1;
> +	}

With this loop above I get very lost and don't clearly understand the
reason for it (taking the regions pairwise). Why do we need to do it this
way? Why is the value returned by gem_detect_safe_start_offset() (that is
the max offset over all regions) or even
gem_detect_min_start_offset_for_region() sufficient? Is this algorithm
above always guaranteed to return a value which will work?

Also, do we even need to expose gem_detect_safe_start_offset() to the tests
or all the tests will ever need to call is gem_detect_safe_alignment() so
everything else can be static?

> +/**
> + * gem_get_safe_alignment:

Comment mismatches function name below.

> + * @i915: drm fd
> + *
> + * Returns: safe alignment for all memory regions on @i915 device.
> + * Safe in this case means max() from all minimum alignment for each
> + * region.
> + */
> +uint64_t gem_detect_safe_alignment(int i915)