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

[Matthew Auld]

On 20/10/2022 17:51, Niranjana Vishwanathapura wrote:
> On Thu, Oct 20, 2022 at 10:16:06AM +0100, Matthew Auld wrote:
>> 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 at intel.com>
>>>>> Signed-off-by: Andi Shyti <andi.shyti at 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 at 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.
>>
> 
> Unbind will anyway wait behind this fence. As it is just returning the
> exclusive fence status, I think it should be fine even for concurrent
> cases. Any specific sequence you are concerned with?
> For vm_bind use case, we do have the object lock while calling it, but
> I am not certain about the i915_vma_pin_iomap() use case. The permerge
> CI result is clean though.

Yeah, all current users are either holding the lock and/or have an 
active vma pin which they want to check (like in pin_iomap()). I figure 
that's what most users probably want and does seem quite sensible, so 
adding that to the kernel-doc might be useful. But it doesn't really 
matter that much I guess.

> 
>>>
>>> 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.
> 
> Ah...will move i915_active_fence_get() below.
> 
> Thanks,
> Niranjana
> 
>>
>>>>> +        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