Re: [Intel-gfx] [PATCH v4 13/17] drm/i915/vm_bind: Update i915_vma_verify_bind_complete()

[Matthew Auld <matthew.auld@intel.com>]

On 19/10/2022 19:28, Niranjana Vishwanathapura wrote:
> On Wed, Oct 19, 2022 at 05:07:31PM +0100, Matthew Auld wrote:
>> On 18/10/2022 08:16, Niranjana Vishwanathapura wrote:
>>> Ensure i915_vma_verify_bind_complete() handles case where bind
>>> is not initiated. Also make it non static, add documentation
>>> and move it out of CONFIG_DRM_I915_DEBUG_GEM.
>>>
>>> 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 | 16 +++++++++++-----
>>>  drivers/gpu/drm/i915/i915_vma.h |  1 +
>>>  2 files changed, 12 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_vma.c 
>>> b/drivers/gpu/drm/i915/i915_vma.c
>>> index eaa13e9ba966..4975fc662c86 100644
>>> --- a/drivers/gpu/drm/i915/i915_vma.c
>>> +++ b/drivers/gpu/drm/i915/i915_vma.c
>>> @@ -439,12 +439,21 @@ int i915_vma_sync(struct i915_vma *vma)
>>>      return i915_vm_sync(vma->vm);
>>>  }
>>> -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
>>> -static int i915_vma_verify_bind_complete(struct i915_vma *vma)
>>> +/**
>>> + * i915_vma_verify_bind_complete() - Check for the bind completion 
>>> of the vma
>>> + * @vma: vma to check for bind completion
>>
>> Maybe mention the locking since this is now more than just DEBUG_GEM 
>> stuff. I assume we need the object lock or otherwise some guarantee 
>> that the vma is pinned?
>>
>> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>>
> 
> I think they are not needed. The fence reference is obtained under rcu
> lock anyhow (will add this to documentation). Only thing required is
> that vma is not released, but that caller must ensure for all i915_vma
> apis anyhow.

I was thinking more about how this potentially behaves with concurrent 
bind/unbind, and what it might return in such cases. I'm assuming most 
normal users will want to have an active pin and/or be holding the 
object lock when calling this.

> 
> Thanks,
> Niranjana
> 
>>> + *
>>> + * Returns: 0 if the vma bind is completed. Error code otherwise.
>>> + */
>>> +int i915_vma_verify_bind_complete(struct i915_vma *vma)
>>>  {
>>>      struct dma_fence *fence = i915_active_fence_get(&vma->active.excl);
>>>      int err;
>>> +    /* Ensure vma bind is initiated */
>>> +    if (!i915_vma_is_bound(vma, I915_VMA_BIND_MASK))

Just realised that this leaks the fence. I guess we have yet to hit this 
in testing.

>>> +        return -EINVAL;
>>> +
>>>      if (!fence)
>>>          return 0;
>>> @@ -457,9 +466,6 @@ static int i915_vma_verify_bind_complete(struct 
>>> i915_vma *vma)
>>>      return err;
>>>  }
>>> -#else
>>> -#define i915_vma_verify_bind_complete(_vma) 0
>>> -#endif
>>>  I915_SELFTEST_EXPORT void
>>>  i915_vma_resource_init_from_vma(struct i915_vma_resource *vma_res,
>>> diff --git a/drivers/gpu/drm/i915/i915_vma.h 
>>> b/drivers/gpu/drm/i915/i915_vma.h
>>> index 1cadbf8fdedf..04770f8ba815 100644
>>> --- a/drivers/gpu/drm/i915/i915_vma.h
>>> +++ b/drivers/gpu/drm/i915/i915_vma.h
>>> @@ -440,6 +440,7 @@ void i915_vma_make_purgeable(struct i915_vma *vma);
>>>  int i915_vma_wait_for_bind(struct i915_vma *vma);
>>>  int i915_vma_sync(struct i915_vma *vma);
>>> +int i915_vma_verify_bind_complete(struct i915_vma *vma);
>>>  /**
>>>   * i915_vma_get_current_resource - Get the current resource of the vma