Message ID | 20221018071630.3831-14-niranjana.vishwanathapura@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/i915/vm_bind: Add VM_BIND functionality | expand |
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> > + * > + * 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)) > + 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
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. 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)) >>+ 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
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
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@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. > 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. >> >>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
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@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. >> > > 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
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 + * + * 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)) + 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