| Message ID | 20231124233650.152653-5-dakr@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PowerVR VM fixes | expand |
Hi Christian, do you think it makes sense to handle this in drm_exec_prepare_obj() or even dma_resv_reserve_fences() even? - Danilo On 11/25/23 00:36, Danilo Krummrich wrote: > Fall back to drm_exec_lock_obj() if num_fences is zero for the > drm_gpuvm_prepare_* function family. > > Otherwise dma_resv_reserve_fences() would actually allocate slots even > though num_fences is zero. > > Cc: Christian König <christian.koenig@amd.com> > Signed-off-by: Danilo Krummrich <dakr@redhat.com> > --- > drivers/gpu/drm/drm_gpuvm.c | 36 +++++++++++++++++++++++++++++++++--- > include/drm/drm_gpuvm.h | 23 +++-------------------- > 2 files changed, 36 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c > index 54f5e8851de5..d1d1c2379e44 100644 > --- a/drivers/gpu/drm/drm_gpuvm.c > +++ b/drivers/gpu/drm/drm_gpuvm.c > @@ -1085,6 +1085,36 @@ drm_gpuvm_put(struct drm_gpuvm *gpuvm) > } > EXPORT_SYMBOL_GPL(drm_gpuvm_put); > > +static int > +exec_prepare_obj(struct drm_exec *exec, struct drm_gem_object *obj, > + unsigned int num_fences) > +{ > + return num_fences ? drm_exec_prepare_obj(exec, obj, num_fences) : > + drm_exec_lock_obj(exec, obj); > +} > + > +/** > + * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv > + * @gpuvm: the &drm_gpuvm > + * @exec: the &drm_exec context > + * @num_fences: the amount of &dma_fences to reserve > + * > + * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object. > + * > + * Using this function directly, it is the drivers responsibility to call > + * drm_exec_init() and drm_exec_fini() accordingly. > + * > + * Returns: 0 on success, negative error code on failure. > + */ > +int > +drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm, > + struct drm_exec *exec, > + unsigned int num_fences) > +{ > + return exec_prepare_obj(exec, gpuvm->r_obj, num_fences); > +} > +EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_vm); > + > static int > __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm, > struct drm_exec *exec, > @@ -1095,7 +1125,7 @@ __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm, > int ret = 0; > > for_each_vm_bo_in_list(gpuvm, extobj, &extobjs, vm_bo) { > - ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences); > + ret = exec_prepare_obj(exec, vm_bo->obj, num_fences); > if (ret) > break; > } > @@ -1116,7 +1146,7 @@ drm_gpuvm_prepare_objects_locked(struct drm_gpuvm *gpuvm, > > drm_gpuvm_resv_assert_held(gpuvm); > list_for_each_entry(vm_bo, &gpuvm->extobj.list, list.entry.extobj) { > - ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences); > + ret = exec_prepare_obj(exec, vm_bo->obj, num_fences); > if (ret) > break; > > @@ -1186,7 +1216,7 @@ drm_gpuvm_prepare_range(struct drm_gpuvm *gpuvm, struct drm_exec *exec, > drm_gpuvm_for_each_va_range(va, gpuvm, addr, end) { > struct drm_gem_object *obj = va->gem.obj; > > - ret = drm_exec_prepare_obj(exec, obj, num_fences); > + ret = exec_prepare_obj(exec, obj, num_fences); > if (ret) > return ret; > } > diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h > index f94fec9a8517..b3f82ec7fb17 100644 > --- a/include/drm/drm_gpuvm.h > +++ b/include/drm/drm_gpuvm.h > @@ -544,26 +544,9 @@ struct drm_gpuvm_exec { > } extra; > }; > > -/** > - * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv > - * @gpuvm: the &drm_gpuvm > - * @exec: the &drm_exec context > - * @num_fences: the amount of &dma_fences to reserve > - * > - * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object. > - * > - * Using this function directly, it is the drivers responsibility to call > - * drm_exec_init() and drm_exec_fini() accordingly. > - * > - * Returns: 0 on success, negative error code on failure. > - */ > -static inline int > -drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm, > - struct drm_exec *exec, > - unsigned int num_fences) > -{ > - return drm_exec_prepare_obj(exec, gpuvm->r_obj, num_fences); > -} > +int drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm, > + struct drm_exec *exec, > + unsigned int num_fences); > > int drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm, > struct drm_exec *exec,
Am 25.11.23 um 00:40 schrieb Danilo Krummrich: > Hi Christian, > > do you think it makes sense to handle this in drm_exec_prepare_obj() or > even dma_resv_reserve_fences() even? IIRC for drm_exec the discussion has gone a bit back and forth between handling 0 and having a separate function which doesn't allocate fences. We ended up with the solution using separate calls since you either know that you don't need fences (because you for example only need to look something up) or you need fences and eventually calculate the number you need dynamically and if you then end up with 0 it's a bug. So to sum it up the conclusion was that it's more defensive to complain about 0 fences to reserve (which reminds me that dma_resv_reserve_fences() still needs to get a warning for 0 fences as well). Regards, Christian. > > - Danilo > > On 11/25/23 00:36, Danilo Krummrich wrote: >> Fall back to drm_exec_lock_obj() if num_fences is zero for the >> drm_gpuvm_prepare_* function family. >> >> Otherwise dma_resv_reserve_fences() would actually allocate slots even >> though num_fences is zero. >> >> Cc: Christian König <christian.koenig@amd.com> >> Signed-off-by: Danilo Krummrich <dakr@redhat.com> >> --- >> drivers/gpu/drm/drm_gpuvm.c | 36 +++++++++++++++++++++++++++++++++--- >> include/drm/drm_gpuvm.h | 23 +++-------------------- >> 2 files changed, 36 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c >> index 54f5e8851de5..d1d1c2379e44 100644 >> --- a/drivers/gpu/drm/drm_gpuvm.c >> +++ b/drivers/gpu/drm/drm_gpuvm.c >> @@ -1085,6 +1085,36 @@ drm_gpuvm_put(struct drm_gpuvm *gpuvm) >> } >> EXPORT_SYMBOL_GPL(drm_gpuvm_put); >> +static int >> +exec_prepare_obj(struct drm_exec *exec, struct drm_gem_object *obj, >> + unsigned int num_fences) >> +{ >> + return num_fences ? drm_exec_prepare_obj(exec, obj, num_fences) : >> + drm_exec_lock_obj(exec, obj); >> +} >> + >> +/** >> + * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv >> + * @gpuvm: the &drm_gpuvm >> + * @exec: the &drm_exec context >> + * @num_fences: the amount of &dma_fences to reserve >> + * >> + * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object. >> + * >> + * Using this function directly, it is the drivers responsibility to >> call >> + * drm_exec_init() and drm_exec_fini() accordingly. >> + * >> + * Returns: 0 on success, negative error code on failure. >> + */ >> +int >> +drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm, >> + struct drm_exec *exec, >> + unsigned int num_fences) >> +{ >> + return exec_prepare_obj(exec, gpuvm->r_obj, num_fences); >> +} >> +EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_vm); >> + >> static int >> __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm, >> struct drm_exec *exec, >> @@ -1095,7 +1125,7 @@ __drm_gpuvm_prepare_objects(struct drm_gpuvm >> *gpuvm, >> int ret = 0; >> for_each_vm_bo_in_list(gpuvm, extobj, &extobjs, vm_bo) { >> - ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences); >> + ret = exec_prepare_obj(exec, vm_bo->obj, num_fences); >> if (ret) >> break; >> } >> @@ -1116,7 +1146,7 @@ drm_gpuvm_prepare_objects_locked(struct >> drm_gpuvm *gpuvm, >> drm_gpuvm_resv_assert_held(gpuvm); >> list_for_each_entry(vm_bo, &gpuvm->extobj.list, >> list.entry.extobj) { >> - ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences); >> + ret = exec_prepare_obj(exec, vm_bo->obj, num_fences); >> if (ret) >> break; >> @@ -1186,7 +1216,7 @@ drm_gpuvm_prepare_range(struct drm_gpuvm >> *gpuvm, struct drm_exec *exec, >> drm_gpuvm_for_each_va_range(va, gpuvm, addr, end) { >> struct drm_gem_object *obj = va->gem.obj; >> - ret = drm_exec_prepare_obj(exec, obj, num_fences); >> + ret = exec_prepare_obj(exec, obj, num_fences); >> if (ret) >> return ret; >> } >> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h >> index f94fec9a8517..b3f82ec7fb17 100644 >> --- a/include/drm/drm_gpuvm.h >> +++ b/include/drm/drm_gpuvm.h >> @@ -544,26 +544,9 @@ struct drm_gpuvm_exec { >> } extra; >> }; >> -/** >> - * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv >> - * @gpuvm: the &drm_gpuvm >> - * @exec: the &drm_exec context >> - * @num_fences: the amount of &dma_fences to reserve >> - * >> - * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object. >> - * >> - * Using this function directly, it is the drivers responsibility to >> call >> - * drm_exec_init() and drm_exec_fini() accordingly. >> - * >> - * Returns: 0 on success, negative error code on failure. >> - */ >> -static inline int >> -drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm, >> - struct drm_exec *exec, >> - unsigned int num_fences) >> -{ >> - return drm_exec_prepare_obj(exec, gpuvm->r_obj, num_fences); >> -} >> +int drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm, >> + struct drm_exec *exec, >> + unsigned int num_fences); >> int drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm, >> struct drm_exec *exec, >
Hi, On 11/27/23 14:52, Christian König wrote: > Am 25.11.23 um 00:40 schrieb Danilo Krummrich: >> Hi Christian, >> >> do you think it makes sense to handle this in drm_exec_prepare_obj() or >> even dma_resv_reserve_fences() even? > > IIRC for drm_exec the discussion has gone a bit back and forth between handling 0 and having a separate function which doesn't allocate fences. > > We ended up with the solution using separate calls since you either know that you don't need fences (because you for example only need to look something up) or you need fences and eventually calculate the number you need dynamically and if you then end up with 0 it's a bug. I don't have a strong opinion on that. Though, in GPUVM I'd probably still need some wrapper like +exec_prepare_obj(struct drm_exec *exec, struct drm_gem_object *obj, + unsigned int num_fences) +{ + return num_fences ? drm_exec_prepare_obj(exec, obj, num_fences) : + drm_exec_lock_obj(exec, obj); +} to prevent either duplicate code or rather unnecessary complicated abstractions. And I'm not sure it's super nice that drm_gpuvm_prepare_objects() allows zero fences, whereas drm_exec_prepare_obj() does not. > > So to sum it up the conclusion was that it's more defensive to complain about 0 fences to reserve (which reminds me that dma_resv_reserve_fences() still needs to get a warning for 0 fences as well). What's the logic you'd want to apply there? WARN() but still allocate at least 4 slots or WARN() and return doing nothing? - Danilo > > Regards, > Christian. > >> >> - Danilo >> >> On 11/25/23 00:36, Danilo Krummrich wrote: >>> Fall back to drm_exec_lock_obj() if num_fences is zero for the >>> drm_gpuvm_prepare_* function family. >>> >>> Otherwise dma_resv_reserve_fences() would actually allocate slots even >>> though num_fences is zero. >>> >>> Cc: Christian König <christian.koenig@amd.com> >>> Signed-off-by: Danilo Krummrich <dakr@redhat.com> >>> --- >>> drivers/gpu/drm/drm_gpuvm.c | 36 +++++++++++++++++++++++++++++++++--- >>> include/drm/drm_gpuvm.h | 23 +++-------------------- >>> 2 files changed, 36 insertions(+), 23 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c >>> index 54f5e8851de5..d1d1c2379e44 100644 >>> --- a/drivers/gpu/drm/drm_gpuvm.c >>> +++ b/drivers/gpu/drm/drm_gpuvm.c >>> @@ -1085,6 +1085,36 @@ drm_gpuvm_put(struct drm_gpuvm *gpuvm) >>> } >>> EXPORT_SYMBOL_GPL(drm_gpuvm_put); >>> +static int >>> +exec_prepare_obj(struct drm_exec *exec, struct drm_gem_object *obj, >>> + unsigned int num_fences) >>> +{ >>> + return num_fences ? drm_exec_prepare_obj(exec, obj, num_fences) : >>> + drm_exec_lock_obj(exec, obj); >>> +} >>> + >>> +/** >>> + * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv >>> + * @gpuvm: the &drm_gpuvm >>> + * @exec: the &drm_exec context >>> + * @num_fences: the amount of &dma_fences to reserve >>> + * >>> + * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object. >>> + * >>> + * Using this function directly, it is the drivers responsibility to call >>> + * drm_exec_init() and drm_exec_fini() accordingly. >>> + * >>> + * Returns: 0 on success, negative error code on failure. >>> + */ >>> +int >>> +drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm, >>> + struct drm_exec *exec, >>> + unsigned int num_fences) >>> +{ >>> + return exec_prepare_obj(exec, gpuvm->r_obj, num_fences); >>> +} >>> +EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_vm); >>> + >>> static int >>> __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm, >>> struct drm_exec *exec, >>> @@ -1095,7 +1125,7 @@ __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm, >>> int ret = 0; >>> for_each_vm_bo_in_list(gpuvm, extobj, &extobjs, vm_bo) { >>> - ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences); >>> + ret = exec_prepare_obj(exec, vm_bo->obj, num_fences); >>> if (ret) >>> break; >>> } >>> @@ -1116,7 +1146,7 @@ drm_gpuvm_prepare_objects_locked(struct drm_gpuvm *gpuvm, >>> drm_gpuvm_resv_assert_held(gpuvm); >>> list_for_each_entry(vm_bo, &gpuvm->extobj.list, list.entry.extobj) { >>> - ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences); >>> + ret = exec_prepare_obj(exec, vm_bo->obj, num_fences); >>> if (ret) >>> break; >>> @@ -1186,7 +1216,7 @@ drm_gpuvm_prepare_range(struct drm_gpuvm *gpuvm, struct drm_exec *exec, >>> drm_gpuvm_for_each_va_range(va, gpuvm, addr, end) { >>> struct drm_gem_object *obj = va->gem.obj; >>> - ret = drm_exec_prepare_obj(exec, obj, num_fences); >>> + ret = exec_prepare_obj(exec, obj, num_fences); >>> if (ret) >>> return ret; >>> } >>> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h >>> index f94fec9a8517..b3f82ec7fb17 100644 >>> --- a/include/drm/drm_gpuvm.h >>> +++ b/include/drm/drm_gpuvm.h >>> @@ -544,26 +544,9 @@ struct drm_gpuvm_exec { >>> } extra; >>> }; >>> -/** >>> - * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv >>> - * @gpuvm: the &drm_gpuvm >>> - * @exec: the &drm_exec context >>> - * @num_fences: the amount of &dma_fences to reserve >>> - * >>> - * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object. >>> - * >>> - * Using this function directly, it is the drivers responsibility to call >>> - * drm_exec_init() and drm_exec_fini() accordingly. >>> - * >>> - * Returns: 0 on success, negative error code on failure. >>> - */ >>> -static inline int >>> -drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm, >>> - struct drm_exec *exec, >>> - unsigned int num_fences) >>> -{ >>> - return drm_exec_prepare_obj(exec, gpuvm->r_obj, num_fences); >>> -} >>> +int drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm, >>> + struct drm_exec *exec, >>> + unsigned int num_fences); >>> int drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm, >>> struct drm_exec *exec, >> >
diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c index 54f5e8851de5..d1d1c2379e44 100644 --- a/drivers/gpu/drm/drm_gpuvm.c +++ b/drivers/gpu/drm/drm_gpuvm.c @@ -1085,6 +1085,36 @@ drm_gpuvm_put(struct drm_gpuvm *gpuvm) } EXPORT_SYMBOL_GPL(drm_gpuvm_put); +static int +exec_prepare_obj(struct drm_exec *exec, struct drm_gem_object *obj, + unsigned int num_fences) +{ + return num_fences ? drm_exec_prepare_obj(exec, obj, num_fences) : + drm_exec_lock_obj(exec, obj); +} + +/** + * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv + * @gpuvm: the &drm_gpuvm + * @exec: the &drm_exec context + * @num_fences: the amount of &dma_fences to reserve + * + * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object. + * + * Using this function directly, it is the drivers responsibility to call + * drm_exec_init() and drm_exec_fini() accordingly. + * + * Returns: 0 on success, negative error code on failure. + */ +int +drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm, + struct drm_exec *exec, + unsigned int num_fences) +{ + return exec_prepare_obj(exec, gpuvm->r_obj, num_fences); +} +EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_vm); + static int __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm, struct drm_exec *exec, @@ -1095,7 +1125,7 @@ __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm, int ret = 0; for_each_vm_bo_in_list(gpuvm, extobj, &extobjs, vm_bo) { - ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences); + ret = exec_prepare_obj(exec, vm_bo->obj, num_fences); if (ret) break; } @@ -1116,7 +1146,7 @@ drm_gpuvm_prepare_objects_locked(struct drm_gpuvm *gpuvm, drm_gpuvm_resv_assert_held(gpuvm); list_for_each_entry(vm_bo, &gpuvm->extobj.list, list.entry.extobj) { - ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences); + ret = exec_prepare_obj(exec, vm_bo->obj, num_fences); if (ret) break; @@ -1186,7 +1216,7 @@ drm_gpuvm_prepare_range(struct drm_gpuvm *gpuvm, struct drm_exec *exec, drm_gpuvm_for_each_va_range(va, gpuvm, addr, end) { struct drm_gem_object *obj = va->gem.obj; - ret = drm_exec_prepare_obj(exec, obj, num_fences); + ret = exec_prepare_obj(exec, obj, num_fences); if (ret) return ret; } diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h index f94fec9a8517..b3f82ec7fb17 100644 --- a/include/drm/drm_gpuvm.h +++ b/include/drm/drm_gpuvm.h @@ -544,26 +544,9 @@ struct drm_gpuvm_exec { } extra; }; -/** - * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv - * @gpuvm: the &drm_gpuvm - * @exec: the &drm_exec context - * @num_fences: the amount of &dma_fences to reserve - * - * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object. - * - * Using this function directly, it is the drivers responsibility to call - * drm_exec_init() and drm_exec_fini() accordingly. - * - * Returns: 0 on success, negative error code on failure. - */ -static inline int -drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm, - struct drm_exec *exec, - unsigned int num_fences) -{ - return drm_exec_prepare_obj(exec, gpuvm->r_obj, num_fences); -} +int drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm, + struct drm_exec *exec, + unsigned int num_fences); int drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm, struct drm_exec *exec,
Fall back to drm_exec_lock_obj() if num_fences is zero for the drm_gpuvm_prepare_* function family. Otherwise dma_resv_reserve_fences() would actually allocate slots even though num_fences is zero. Cc: Christian König <christian.koenig@amd.com> Signed-off-by: Danilo Krummrich <dakr@redhat.com> --- drivers/gpu/drm/drm_gpuvm.c | 36 +++++++++++++++++++++++++++++++++--- include/drm/drm_gpuvm.h | 23 +++-------------------- 2 files changed, 36 insertions(+), 23 deletions(-)