[igt-dev] [PATCH i-g-t] lib/intel_memory_region: Use separate context for probing offset and alignment

[Zbigniew Kempczyński]

On Mon, May 09, 2022 at 12:43:11PM +0100, Matthew Auld wrote:
> On 09/05/2022 12:30, Zbigniew Kempczyński wrote:
> > Probing alignment/offset (A/O) in default context works properly only
> > when there're no processes which competes on same vm space. To avoid
> > risk that single probe will be called on already used offset in another
> > process lets use dedicated context for this purpose if possible.
> > 
> > In other words when forking occur without A/O cache filled (subject of
> > COW) children will exercise A/O individually. Using same default context
> > leads to risk of probing offset which is in flight in another child
> > thus we can get different A/O. Such behavior is not allowed as allocator
> > infrastructure requires same type, strategy and alignment on single vm.
> > We expect coherent A/O in different children so we try to use separate
> > context to fill this requirement.
> > 
> > v2: on old gens where're no logical contexts use default context
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Matthew Auld <matthew.auld at intel.com>
> > Fixes: https://gitlab.freedesktop.org/drm/intel/-/issues/5729
> > ---
> >   lib/i915/intel_memory_region.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/i915/intel_memory_region.c b/lib/i915/intel_memory_region.c
> > index 593f4bedc8..c0f67bd737 100644
> > --- a/lib/i915/intel_memory_region.c
> > +++ b/lib/i915/intel_memory_region.c
> > @@ -630,7 +630,7 @@ uint64_t gem_detect_min_start_offset_for_region(int i915, uint32_t region)
> >   	struct drm_i915_gem_execbuffer2 eb;
> >   	uint64_t start_offset = 0;
> >   	uint64_t bb_size = PAGE_SIZE;
> > -	uint32_t *batch;
> > +	uint32_t *batch, ctx = 0;
> >   	uint16_t devid = intel_get_drm_devid(i915);
> >   	struct cache_entry *entry, *newentry;
> > @@ -640,12 +640,16 @@ uint64_t gem_detect_min_start_offset_for_region(int i915, uint32_t region)
> >   		goto out;
> >   	pthread_mutex_unlock(&cache_mutex);
> > +	/* Use separate context if possible to avoid offset overlapping */
> > +	__gem_context_create(i915, &ctx);
> > +
> >   	memset(&obj, 0, sizeof(obj));
> >   	memset(&eb, 0, sizeof(eb));
> >   	eb.buffers_ptr = to_user_pointer(&obj);
> >   	eb.buffer_count = 1;
> >   	eb.flags = I915_EXEC_DEFAULT;
> > +	eb.rsvd1 = ctx;
> >   	igt_assert(__gem_create_in_memory_regions(i915, &obj.handle, &bb_size, region) == 0);
> >   	obj.flags = EXEC_OBJECT_PINNED;
> > @@ -670,6 +674,7 @@ uint64_t gem_detect_min_start_offset_for_region(int i915, uint32_t region)
> >   		igt_assert(start_offset <= 1ull << 48);
> >   	}
> >   	gem_close(i915, obj.handle);
> > +	gem_context_destroy(i915, ctx);
> 
> if (ctx)
>     gem_context_destroy();
> 
> ?

Eh, I forgot adding this in second function :/

> 
> r-b still stands, assuming CI is now happy :)

Ok, thanks, let's CI be clear in this case (v3).

--
Zbigniew

> 
> >   	newentry = malloc(sizeof(*newentry));
> >   	if (!newentry)
> > @@ -770,7 +775,7 @@ uint64_t gem_detect_min_alignment_for_regions(int i915,
> >   	struct drm_i915_gem_execbuffer2 eb;
> >   	uint64_t min_alignment = PAGE_SIZE;
> >   	uint64_t bb_size = PAGE_SIZE, obj_size = PAGE_SIZE;
> > -	uint32_t *batch;
> > +	uint32_t *batch, ctx = 0;
> >   	uint16_t devid = intel_get_drm_devid(i915);
> >   	struct cache_entry *entry, *newentry;
> > @@ -780,6 +785,9 @@ uint64_t gem_detect_min_alignment_for_regions(int i915,
> >   		goto out;
> >   	pthread_mutex_unlock(&cache_mutex);
> > +	/* Use separate context if possible to avoid offset overlapping */
> > +	__gem_context_create(i915, &ctx);
> > +
> >   	memset(obj, 0, sizeof(obj));
> >   	memset(&eb, 0, sizeof(eb));
> > @@ -787,6 +795,7 @@ uint64_t gem_detect_min_alignment_for_regions(int i915,
> >   	eb.buffers_ptr = to_user_pointer(obj);
> >   	eb.buffer_count = ARRAY_SIZE(obj);
> >   	eb.flags = I915_EXEC_BATCH_FIRST | I915_EXEC_DEFAULT;
> > +	eb.rsvd1 = ctx;
> >   	igt_assert(__gem_create_in_memory_regions(i915, &obj[0].handle,
> >   						  &bb_size, region1) == 0);
> > @@ -815,6 +824,8 @@ uint64_t gem_detect_min_alignment_for_regions(int i915,
> >   	gem_close(i915, obj[0].handle);
> >   	gem_close(i915, obj[1].handle);
> > +	if (ctx)
> > +		gem_context_destroy(i915, ctx);
> >   	newentry = malloc(sizeof(*newentry));
> >   	if (!newentry)