Re: [Intel-gfx] [PATCH 23/37] drm/i915/gem: Manage GTT placement bias (starting offset) explicitly

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 05/08/2020 13:22, Chris Wilson wrote:
> Since we can control placement in the ppGTT explicitly, we can specify
> our desired starting offset exactly on a per-vma basis. This prevents us
> falling down a few corner cases where we confuse the user with our choices.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 67 +++++++++----------
>   1 file changed, 31 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 19cab5541dbc..0839397c7e50 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -36,6 +36,7 @@ struct eb_vma {
>   
>   	/** This vma's place in the execbuf reservation list */
>   	struct drm_i915_gem_exec_object2 *exec;
> +	u32 bias;
>   
>   	struct list_head bind_link;
>   	struct list_head unbound_link;
> @@ -61,15 +62,12 @@ struct eb_vma_array {
>   #define __EXEC_OBJECT_HAS_PIN		BIT(31)
>   #define __EXEC_OBJECT_HAS_FENCE		BIT(30)
>   #define __EXEC_OBJECT_NEEDS_MAP		BIT(29)
> -#define __EXEC_OBJECT_NEEDS_BIAS	BIT(28)
> -#define __EXEC_OBJECT_INTERNAL_FLAGS	(~0u << 28) /* all of the above */
> +#define __EXEC_OBJECT_INTERNAL_FLAGS	(~0u << 29) /* all of the above */
>   
>   #define __EXEC_HAS_RELOC	BIT(31)
>   #define __EXEC_INTERNAL_FLAGS	(~0u << 31)
>   #define UPDATE			PIN_OFFSET_FIXED
>   
> -#define BATCH_OFFSET_BIAS (256*1024)
> -
>   #define __I915_EXEC_ILLEGAL_FLAGS \
>   	(__I915_EXEC_UNKNOWN_FLAGS | \
>   	 I915_EXEC_CONSTANTS_MASK  | \
> @@ -291,7 +289,7 @@ struct i915_execbuffer {
>   	} parser;
>   
>   	u64 invalid_flags; /** Set of execobj.flags that are invalid */
> -	u32 context_flags; /** Set of execobj.flags to insert from the ctx */
> +	u32 context_bias;
>   
>   	u32 batch_start_offset; /** Location within object of batch */
>   	u32 batch_len; /** Length of batch within object */
> @@ -491,11 +489,12 @@ static int eb_create(struct i915_execbuffer *eb)
>   	return 0;
>   }
>   
> -static bool
> -eb_vma_misplaced(const struct drm_i915_gem_exec_object2 *entry,
> -		 const struct i915_vma *vma,
> -		 unsigned int flags)
> +static bool eb_vma_misplaced(const struct eb_vma *ev)
>   {
> +	const struct drm_i915_gem_exec_object2 *entry = ev->exec;
> +	const struct i915_vma *vma = ev->vma;
> +	unsigned int flags = ev->flags;
> +
>   	if (test_bit(I915_VMA_ERROR_BIT, __i915_vma_flags(vma)))
>   		return true;
>   
> @@ -509,8 +508,7 @@ eb_vma_misplaced(const struct drm_i915_gem_exec_object2 *entry,
>   	    vma->node.start != entry->offset)
>   		return true;
>   
> -	if (flags & __EXEC_OBJECT_NEEDS_BIAS &&
> -	    vma->node.start < BATCH_OFFSET_BIAS)
> +	if (vma->node.start < ev->bias)
>   		return true;
>   
>   	if (!(flags & EXEC_OBJECT_SUPPORTS_48B_ADDRESS) &&
> @@ -529,10 +527,7 @@ static bool eb_pin_vma_fence_inplace(struct eb_vma *ev)
>   	return false; /* We need to add some new fence serialisation */
>   }
>   
> -static inline bool
> -eb_pin_vma_inplace(struct i915_execbuffer *eb,
> -		   const struct drm_i915_gem_exec_object2 *entry,
> -		   struct eb_vma *ev)
> +static inline bool eb_pin_vma_inplace(struct eb_vma *ev)
>   {
>   	struct i915_vma *vma = ev->vma;
>   	unsigned int pin_flags;
> @@ -541,7 +536,7 @@ eb_pin_vma_inplace(struct i915_execbuffer *eb,
>   	if (!i915_active_is_idle(&vma->vm->binding))
>   		return false;
>   
> -	if (eb_vma_misplaced(entry, vma, ev->flags))
> +	if (eb_vma_misplaced(ev))
>   		return false;
>   
>   	pin_flags = PIN_USER;
> @@ -559,7 +554,7 @@ eb_pin_vma_inplace(struct i915_execbuffer *eb,
>   		}
>   	}
>   
> -	GEM_BUG_ON(eb_vma_misplaced(entry, vma, ev->flags));
> +	GEM_BUG_ON(eb_vma_misplaced(ev));
>   
>   	ev->flags |= __EXEC_OBJECT_HAS_PIN;
>   	return true;
> @@ -608,9 +603,6 @@ eb_validate_vma(struct i915_execbuffer *eb,
>   			entry->flags |= EXEC_OBJECT_NEEDS_GTT | __EXEC_OBJECT_NEEDS_MAP;
>   	}
>   
> -	if (!(entry->flags & EXEC_OBJECT_PINNED))
> -		entry->flags |= eb->context_flags;
> -
>   	return 0;
>   }
>   
> @@ -627,6 +619,7 @@ eb_add_vma(struct i915_execbuffer *eb,
>   	ev->vma = vma;
>   	ev->exec = entry;
>   	ev->flags = entry->flags;
> +	ev->bias = eb->context_bias;
>   
>   	if (eb->lut_size > 0) {
>   		ev->handle = entry->handle;
> @@ -653,7 +646,8 @@ eb_add_vma(struct i915_execbuffer *eb,
>   	if (i == batch_idx) {
>   		if (entry->relocation_count &&
>   		    !(ev->flags & EXEC_OBJECT_PINNED))
> -			ev->flags |= __EXEC_OBJECT_NEEDS_BIAS;
> +			ev->bias = max_t(u32, ev->bias, SZ_256K);

What dictates the 256KiB border? Wondering if this is too hidden in here 
or not.

Regards,

Tvrtko

> +
>   		if (eb->has_fence)
>   			ev->flags |= EXEC_OBJECT_NEEDS_FENCE;
>   
> @@ -979,8 +973,9 @@ static int eb_reserve_vma(struct eb_vm_work *work, struct eb_bind_vma *bind)
>   	const unsigned int exec_flags = bind->ev->flags;
>   	struct i915_vma *vma = bind->ev->vma;
>   	struct i915_address_space *vm = vma->vm;
> -	u64 start = 0, end = vm->total;
>   	u64 align = entry->alignment ?: I915_GTT_MIN_ALIGNMENT;
> +	u64 start = round_up(bind->ev->bias, align);
> +	u64 end = vm->total;
>   	unsigned int bind_flags;
>   	int err;
>   
> @@ -999,7 +994,7 @@ static int eb_reserve_vma(struct eb_vm_work *work, struct eb_bind_vma *bind)
>   	GEM_BUG_ON(!vma->size);
>   
>   	/* Reuse old address (if it doesn't conflict with new requirements) */
> -	if (eb_vma_misplaced(entry, vma, exec_flags)) {
> +	if (eb_vma_misplaced(bind->ev)) {
>   		vma->node.start = entry->offset & PIN_OFFSET_MASK;
>   		vma->node.size = max(entry->pad_to_size, vma->size);
>   		vma->node.color = 0;
> @@ -1021,11 +1016,8 @@ static int eb_reserve_vma(struct eb_vm_work *work, struct eb_bind_vma *bind)
>   		align = max_t(u64, align, vma->fence_alignment);
>   	}
>   
> -	if (exec_flags & __EXEC_OBJECT_NEEDS_BIAS)
> -		start = BATCH_OFFSET_BIAS;
> -
>   	GEM_BUG_ON(!vma->node.size);
> -	if (vma->node.size > end - start)
> +	if (start > end || vma->node.size > end - start)
>   		return -E2BIG;
>   
>   	/* Try the user's preferred location first (mandatory if soft-pinned) */
> @@ -1108,7 +1100,7 @@ static int eb_reserve_vma(struct eb_vm_work *work, struct eb_bind_vma *bind)
>   	}
>   
>   	bind->ev->flags |= __EXEC_OBJECT_HAS_PIN;
> -	GEM_BUG_ON(eb_vma_misplaced(entry, vma, bind->ev->flags));
> +	GEM_BUG_ON(eb_vma_misplaced(bind->ev));
>   
>   	if (unlikely(exec_flags & EXEC_OBJECT_NEEDS_FENCE)) {
>   		err = __i915_vma_pin_fence_async(vma, &work->base);
> @@ -1341,8 +1333,7 @@ static int wait_for_unbinds(struct i915_execbuffer *eb,
>   
>   		GEM_BUG_ON(ev->flags & __EXEC_OBJECT_HAS_PIN);
>   
> -		if (drm_mm_node_allocated(&vma->node) &&
> -		    eb_vma_misplaced(ev->exec, vma, ev->flags)) {
> +		if (drm_mm_node_allocated(&vma->node) && eb_vma_misplaced(ev)) {
>   			err = i915_vma_unbind(vma);
>   			if (err)
>   				return err;
> @@ -1391,10 +1382,10 @@ static int eb_reserve_vm(struct i915_execbuffer *eb)
>   	count = 0;
>   	INIT_LIST_HEAD(&unbound);
>   	list_for_each_entry(ev, &eb->bind_list, bind_link) {
> -		struct drm_i915_gem_exec_object2 *entry = ev->exec;
> -		struct i915_vma *vma = ev->vma;
> +		if (eb_pin_vma_inplace(ev)) {
> +			struct drm_i915_gem_exec_object2 *entry = ev->exec;
> +			struct i915_vma *vma = ev->vma;
>   
> -		if (eb_pin_vma_inplace(eb, entry, ev)) {
>   			if (entry != &no_entry &&
>   			    entry->offset != vma->node.start) {
>   				entry->offset = vma->node.start | UPDATE;
> @@ -1478,7 +1469,7 @@ static int eb_reserve_vm(struct i915_execbuffer *eb)
>   			 * we cannot handle migrating the vma inside the worker.
>   			 */
>   			if (drm_mm_node_allocated(&vma->node)) {
> -				if (eb_vma_misplaced(ev->exec, vma, ev->flags)) {
> +				if (eb_vma_misplaced(ev)) {
>   					err = -ENOSPC;
>   					break;
>   				}
> @@ -1736,9 +1727,13 @@ static int eb_select_context(struct i915_execbuffer *eb)
>   	if (rcu_access_pointer(ctx->vm))
>   		eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
>   
> -	eb->context_flags = 0;
> +	/*
> +	 * At the user's discretion, ensure that we place no objects at
> +	 * offset 0, allowing them to use 0 as a 'NULL' pointer.
> +	 */
> +	eb->context_bias = 0;
>   	if (test_bit(UCONTEXT_NO_ZEROMAP, &ctx->user_flags))
> -		eb->context_flags |= __EXEC_OBJECT_NEEDS_BIAS;
> +		eb->context_bias = I915_GTT_MIN_ALIGNMENT;
>   
>   	return 0;
>   }
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx