| Message ID | 20220701225055.8204-10-niranjana.vishwanathapura@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/vm_bind: Add VM_BIND functionality | expand |
On Fri, 2022-07-01 at 15:50 -0700, Niranjana Vishwanathapura wrote: > vma_lookup is tied to segment of the object instead of section > of VA space. Hence, it do not support aliasing (ie., multiple > bindings to the same section of the object). > Skip vma_lookup for persistent vmas as it supports aliasing. > > Signed-off-by: Niranjana Vishwanathapura > <niranjana.vishwanathapura@intel.com> > --- > drivers/gpu/drm/i915/i915_vma.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_vma.c > b/drivers/gpu/drm/i915/i915_vma.c > index 6adb013579be..9aa38b772b5b 100644 > --- a/drivers/gpu/drm/i915/i915_vma.c > +++ b/drivers/gpu/drm/i915/i915_vma.c > @@ -197,6 +197,10 @@ vma_create(struct drm_i915_gem_object *obj, > __set_bit(I915_VMA_GGTT_BIT, __i915_vma_flags(vma)); > } > > + if (!i915_vma_is_ggtt(vma) && > + (view && view->type == I915_GGTT_VIEW_PARTIAL)) > + goto skip_rb_insert; > + Rather than guessing that a vma with this signature is a persistent vma, which is confusing to the reader, could we have an argument saying we want to create a persistent vma? > rb = NULL; > p = &obj->vma.tree.rb_node; > while (*p) { > @@ -221,6 +225,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 > @@ -292,13 +297,16 @@ i915_vma_instance(struct drm_i915_gem_object > *obj, > struct i915_address_space *vm, > const struct i915_ggtt_view *view) > { > - struct i915_vma *vma; > + struct i915_vma *vma = NULL; > > GEM_BUG_ON(!kref_read(&vm->ref)); > > - spin_lock(&obj->vma.lock); > - vma = i915_vma_lookup(obj, vm, view); > - spin_unlock(&obj->vma.lock); > + if (i915_is_ggtt(vm) || !view || > + view->type != I915_GGTT_VIEW_PARTIAL) { Same here? /Thomas > + spin_lock(&obj->vma.lock); > + vma = i915_vma_lookup(obj, vm, view); > + spin_unlock(&obj->vma.lock); > + } > > /* vma_create() will resolve the race if another creates the > vma */ > if (unlikely(!vma)) > @@ -1670,7 +1678,8 @@ static void release_references(struct i915_vma > *vma, bool vm_ddestroy) > > 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);
On Tue, Jul 05, 2022 at 10:57:17AM +0200, Thomas Hellström wrote: >On Fri, 2022-07-01 at 15:50 -0700, Niranjana Vishwanathapura wrote: >> vma_lookup is tied to segment of the object instead of section >> of VA space. Hence, it do not support aliasing (ie., multiple >> bindings to the same section of the object). >> Skip vma_lookup for persistent vmas as it supports aliasing. >> >> Signed-off-by: Niranjana Vishwanathapura >> <niranjana.vishwanathapura@intel.com> >> --- >> drivers/gpu/drm/i915/i915_vma.c | 19 ++++++++++++++----- >> 1 file changed, 14 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_vma.c >> b/drivers/gpu/drm/i915/i915_vma.c >> index 6adb013579be..9aa38b772b5b 100644 >> --- a/drivers/gpu/drm/i915/i915_vma.c >> +++ b/drivers/gpu/drm/i915/i915_vma.c >> @@ -197,6 +197,10 @@ vma_create(struct drm_i915_gem_object *obj, >> __set_bit(I915_VMA_GGTT_BIT, __i915_vma_flags(vma)); >> } >> >> + if (!i915_vma_is_ggtt(vma) && >> + (view && view->type == I915_GGTT_VIEW_PARTIAL)) >> + goto skip_rb_insert; >> + > >Rather than guessing that a vma with this signature is a persistent >vma, which is confusing to the reader, could we have an argument saying >we want to create a persistent vma? Yah, sounds good. We probably can even check for vm->vm_bind_mode here instead of passing a new argument. I think i915 won't create any internal vmas for this VM, so, should be good to check vm->vm_bind_mode. > >> rb = NULL; >> p = &obj->vma.tree.rb_node; >> while (*p) { >> @@ -221,6 +225,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 >> @@ -292,13 +297,16 @@ i915_vma_instance(struct drm_i915_gem_object >> *obj, >> struct i915_address_space *vm, >> const struct i915_ggtt_view *view) >> { >> - struct i915_vma *vma; >> + struct i915_vma *vma = NULL; >> >> GEM_BUG_ON(!kref_read(&vm->ref)); >> >> - spin_lock(&obj->vma.lock); >> - vma = i915_vma_lookup(obj, vm, view); >> - spin_unlock(&obj->vma.lock); >> + if (i915_is_ggtt(vm) || !view || >> + view->type != I915_GGTT_VIEW_PARTIAL) { > >Same here? We probably can remove this code and have vm_bind ioctl directly call vma_create. Niranjana > >/Thomas > > >> + spin_lock(&obj->vma.lock); >> + vma = i915_vma_lookup(obj, vm, view); >> + spin_unlock(&obj->vma.lock); >> + } >> >> /* vma_create() will resolve the race if another creates the >> vma */ >> if (unlikely(!vma)) >> @@ -1670,7 +1678,8 @@ static void release_references(struct i915_vma >> *vma, bool vm_ddestroy) >> >> 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.c b/drivers/gpu/drm/i915/i915_vma.c index 6adb013579be..9aa38b772b5b 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -197,6 +197,10 @@ vma_create(struct drm_i915_gem_object *obj, __set_bit(I915_VMA_GGTT_BIT, __i915_vma_flags(vma)); } + if (!i915_vma_is_ggtt(vma) && + (view && view->type == I915_GGTT_VIEW_PARTIAL)) + goto skip_rb_insert; + rb = NULL; p = &obj->vma.tree.rb_node; while (*p) { @@ -221,6 +225,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 @@ -292,13 +297,16 @@ i915_vma_instance(struct drm_i915_gem_object *obj, struct i915_address_space *vm, const struct i915_ggtt_view *view) { - struct i915_vma *vma; + struct i915_vma *vma = NULL; GEM_BUG_ON(!kref_read(&vm->ref)); - spin_lock(&obj->vma.lock); - vma = i915_vma_lookup(obj, vm, view); - spin_unlock(&obj->vma.lock); + if (i915_is_ggtt(vm) || !view || + view->type != I915_GGTT_VIEW_PARTIAL) { + spin_lock(&obj->vma.lock); + vma = i915_vma_lookup(obj, vm, view); + spin_unlock(&obj->vma.lock); + } /* vma_create() will resolve the race if another creates the vma */ if (unlikely(!vma)) @@ -1670,7 +1678,8 @@ static void release_references(struct i915_vma *vma, bool vm_ddestroy) 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);
vma_lookup is tied to segment of the object instead of section of VA space. Hence, it do not support aliasing (ie., multiple bindings to the same section of the object). Skip vma_lookup for persistent vmas as it supports aliasing. Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> --- drivers/gpu/drm/i915/i915_vma.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)