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@xxxxxxxxx>
Signed-off-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
---
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@xxxxxxxxx>
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
