Re: [Intel-gfx] [PATCH 04/16] drm/i915/vm_bind: Add support to create persistent vma

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

On 28/09/2022 07:19, Niranjana Vishwanathapura wrote:
> Add i915_vma_instance_persistent() to create persistent vmas.
> Persistent vmas will use i915_gtt_view to support partial binding.
> 
> vma_lookup is tied to segment of the object instead of section
> of VA space. Hence, it do not support aliasing. ie., multiple
> mappings (at different VA) point to the same gtt_view of object.
> Skip vma_lookup for persistent vmas to support aliasing.
> 
> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_vma.c       | 39 ++++++++++++++++++++++++---
>   drivers/gpu/drm/i915/i915_vma.h       | 16 +++++++++--
>   drivers/gpu/drm/i915/i915_vma_types.h |  7 +++++
>   3 files changed, 57 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index f17c09ead7d7..5839e1f55f00 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -109,7 +109,8 @@ static void __i915_vma_retire(struct i915_active *ref)
>   static struct i915_vma *
>   vma_create(struct drm_i915_gem_object *obj,
>   	   struct i915_address_space *vm,
> -	   const struct i915_gtt_view *view)
> +	   const struct i915_gtt_view *view,
> +	   bool skip_lookup_cache)
>   {
>   	struct i915_vma *pos = ERR_PTR(-E2BIG);
>   	struct i915_vma *vma;
> @@ -196,6 +197,9 @@ vma_create(struct drm_i915_gem_object *obj,
>   		__set_bit(I915_VMA_GGTT_BIT, __i915_vma_flags(vma));
>   	}
>   
> +	if (skip_lookup_cache)
> +		goto skip_rb_insert;
> +
>   	rb = NULL;
>   	p = &obj->vma.tree.rb_node;
>   	while (*p) {
> @@ -220,6 +224,7 @@ vma_create(struct drm_i915_gem_object *obj,
>   	rb_link_node(&vma->obj_node, rb, p);
>   	rb_insert_color(&vma->obj_node, &obj->vma.tree);
>   
> +skip_rb_insert:
>   	if (i915_vma_is_ggtt(vma))
>   		/*
>   		 * We put the GGTT vma at the start of the vma-list, followed
> @@ -299,7 +304,34 @@ i915_vma_instance(struct drm_i915_gem_object *obj,
>   
>   	/* vma_create() will resolve the race if another creates the vma */
>   	if (unlikely(!vma))
> -		vma = vma_create(obj, vm, view);
> +		vma = vma_create(obj, vm, view, false);
> +
> +	GEM_BUG_ON(!IS_ERR(vma) && i915_vma_compare(vma, vm, view));
> +	return vma;
> +}
> +
> +/**
> + * i915_vma_create_persistent - create a persistent VMA
> + * @obj: parent &struct drm_i915_gem_object to be mapped
> + * @vm: address space in which the mapping is located
> + * @view: additional mapping requirements
> + *
> + * Creates a persistent vma.
> + *
> + * Returns the vma, or an error pointer.
> + */
> +struct i915_vma *
> +i915_vma_create_persistent(struct drm_i915_gem_object *obj,
> +			   struct i915_address_space *vm,
> +			   const struct i915_gtt_view *view)
> +{
> +	struct i915_vma *vma;
> +
> +	GEM_BUG_ON(!kref_read(&vm->ref));
> +
> +	vma = vma_create(obj, vm, view, true);
> +	if (!IS_ERR(vma))
> +		i915_vma_set_persistent(vma);
>   
>   	GEM_BUG_ON(!IS_ERR(vma) && i915_vma_compare(vma, vm, view));
>   	return vma;
> @@ -1666,7 +1698,8 @@ static void release_references(struct i915_vma *vma, struct intel_gt *gt,
>   
>   	spin_lock(&obj->vma.lock);
>   	list_del(&vma->obj_link);
> -	if (!RB_EMPTY_NODE(&vma->obj_node))
> +	if (!i915_vma_is_persistent(vma) &&

Thinking out loud - maybe you don't need the extra condition? But it is 
good for self-documenting purposes in any case.

> +	    !RB_EMPTY_NODE(&vma->obj_node))
>   		rb_erase(&vma->obj_node, &obj->vma.tree);
>   
>   	spin_unlock(&obj->vma.lock);
> diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
> index aecd9c64486b..51e712de380a 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -44,6 +44,10 @@ struct i915_vma *
>   i915_vma_instance(struct drm_i915_gem_object *obj,
>   		  struct i915_address_space *vm,
>   		  const struct i915_gtt_view *view);
> +struct i915_vma *
> +i915_vma_create_persistent(struct drm_i915_gem_object *obj,
> +			   struct i915_address_space *vm,
> +			   const struct i915_gtt_view *view);
>   
>   void i915_vma_unpin_and_release(struct i915_vma **p_vma, unsigned int flags);
>   #define I915_VMA_RELEASE_MAP BIT(0)
> @@ -138,6 +142,16 @@ static inline u32 i915_ggtt_pin_bias(struct i915_vma *vma)
>   	return i915_vm_to_ggtt(vma->vm)->pin_bias;
>   }
>   
> +static inline bool i915_vma_is_persistent(const struct i915_vma *vma)
> +{
> +	return test_bit(I915_VMA_PERSISTENT_BIT, __i915_vma_flags(vma));
> +}
> +
> +static inline void i915_vma_set_persistent(struct i915_vma *vma)
> +{
> +	set_bit(I915_VMA_PERSISTENT_BIT, __i915_vma_flags(vma));
> +}
> +
>   static inline struct i915_vma *i915_vma_get(struct i915_vma *vma)
>   {
>   	i915_gem_object_get(vma->obj);
> @@ -164,8 +178,6 @@ i915_vma_compare(struct i915_vma *vma,
>   {
>   	ptrdiff_t cmp;
>   
> -	GEM_BUG_ON(view && !i915_is_ggtt_or_dpt(vm));
Or explicitly add persistent?

Regards,

Tvrtko

> -
>   	cmp = ptrdiff(vma->vm, vm);
>   	if (cmp)
>   		return cmp;
> diff --git a/drivers/gpu/drm/i915/i915_vma_types.h b/drivers/gpu/drm/i915/i915_vma_types.h
> index ec0f6c9f57d0..2200f1f103ba 100644
> --- a/drivers/gpu/drm/i915/i915_vma_types.h
> +++ b/drivers/gpu/drm/i915/i915_vma_types.h
> @@ -264,6 +264,13 @@ struct i915_vma {
>   #define I915_VMA_SCANOUT_BIT	17
>   #define I915_VMA_SCANOUT	((int)BIT(I915_VMA_SCANOUT_BIT))
>   
> +/**
> + * I915_VMA_PERSISTENT_BIT:
> + * The vma is persistent (created with VM_BIND call).
> + */
> +#define I915_VMA_PERSISTENT_BIT	19
> +#define I915_VMA_PERSISTENT	((int)BIT(I915_VMA_PERSISTENT_BIT))
> +
>   	struct i915_active active;
>   
>   #define I915_VMA_PAGES_BIAS 24