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

[Niranjana Vishwanathapura]

On Wed, Sep 28, 2022 at 04:44:08PM +0200, Andi Shyti wrote:
>Hi Niranjana,
>
>On Tue, Sep 27, 2022 at 11:19:06PM -0700, 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 at intel.com>
>> Signed-off-by: Andi Shyti <andi.shyti at 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) &&
>> +	    !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));
>> -
>>  	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))
>> +
>
>are we using I915_VMA_PERSISTENT anywhere?

Thanks, we are not. I just followed the convention here.
Looks like many definitions are not used here.
Will remove.

Regards,
Niranjana

>
>Andi
>
>>  	struct i915_active active;
>>
>>  #define I915_VMA_PAGES_BIAS 24
>> --
>> 2.21.0.rc0.32.g243a4c7e27