[PATCH v4 13/17] drm/i915/vm_bind: Update i915_vma_verify_bind_complete()
Matthew Auld
matthew.auld at intel.com
Thu Oct 20 17:06:16 UTC 2022
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
More information about the dri-devel
mailing list