Messages in this thread | | | Date | Tue, 31 Oct 2023 17:47:26 +0100 | Subject | Re: [PATCH drm-misc-next v7 1/7] drm/gpuvm: convert WARN() to drm_WARN() variants | From | Danilo Krummrich <> |
| |
On 10/31/23 11:08, Thomas Hellström wrote: > On Mon, 2023-10-23 at 22:16 +0200, Danilo Krummrich wrote: >> Use drm_WARN() and drm_WARN_ON() variants to indicate drivers the >> context the failing VM resides in. >> >> Signed-off-by: Danilo Krummrich <dakr@redhat.com> >> --- >> drivers/gpu/drm/drm_gpuvm.c | 32 ++++++++++++++---------- >> -- >> drivers/gpu/drm/nouveau/nouveau_uvmm.c | 3 ++- >> include/drm/drm_gpuvm.h | 7 ++++++ >> 3 files changed, 26 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gpuvm.c >> b/drivers/gpu/drm/drm_gpuvm.c >> index 08c088319652..d7367a202fee 100644 >> --- a/drivers/gpu/drm/drm_gpuvm.c >> +++ b/drivers/gpu/drm/drm_gpuvm.c >> @@ -614,12 +614,12 @@ static int __drm_gpuva_insert(struct drm_gpuvm >> *gpuvm, >> static void __drm_gpuva_remove(struct drm_gpuva *va); >> >> static bool >> -drm_gpuvm_check_overflow(u64 addr, u64 range) >> +drm_gpuvm_check_overflow(struct drm_gpuvm *gpuvm, u64 addr, u64 >> range) >> { >> u64 end; >> >> - return WARN(check_add_overflow(addr, range, &end), >> - "GPUVA address limited to %zu bytes.\n", >> sizeof(end)); >> + return drm_WARN(gpuvm->drm, check_add_overflow(addr, range, >> &end), >> + "GPUVA address limited to %zu bytes.\n", >> sizeof(end)); >> } >> >> static bool >> @@ -647,7 +647,7 @@ static bool >> drm_gpuvm_range_valid(struct drm_gpuvm *gpuvm, >> u64 addr, u64 range) >> { >> - return !drm_gpuvm_check_overflow(addr, range) && >> + return !drm_gpuvm_check_overflow(gpuvm, addr, range) && >> drm_gpuvm_in_mm_range(gpuvm, addr, range) && >> !drm_gpuvm_in_kernel_node(gpuvm, addr, range); > > >> } >> @@ -656,6 +656,7 @@ drm_gpuvm_range_valid(struct drm_gpuvm *gpuvm, >> * drm_gpuvm_init() - initialize a &drm_gpuvm >> * @gpuvm: pointer to the &drm_gpuvm to initialize >> * @name: the name of the GPU VA space >> + * @drm: the &drm_device this VM resides in >> * @start_offset: the start offset of the GPU VA space >> * @range: the size of the GPU VA space >> * @reserve_offset: the start of the kernel reserved GPU VA area >> @@ -668,8 +669,8 @@ drm_gpuvm_range_valid(struct drm_gpuvm *gpuvm, >> * &name is expected to be managed by the surrounding driver >> structures. >> */ >> void >> -drm_gpuvm_init(struct drm_gpuvm *gpuvm, >> - const char *name, >> +drm_gpuvm_init(struct drm_gpuvm *gpuvm, const char *name, >> + struct drm_device *drm, >> u64 start_offset, u64 range, >> u64 reserve_offset, u64 reserve_range, >> const struct drm_gpuvm_ops *ops) >> @@ -677,20 +678,20 @@ drm_gpuvm_init(struct drm_gpuvm *gpuvm, >> gpuvm->rb.tree = RB_ROOT_CACHED; >> INIT_LIST_HEAD(&gpuvm->rb.list); >> >> - drm_gpuvm_check_overflow(start_offset, range); >> - gpuvm->mm_start = start_offset; >> - gpuvm->mm_range = range; >> - >> gpuvm->name = name ? name : "unknown"; >> gpuvm->ops = ops; >> + gpuvm->drm = drm; >> >> - memset(&gpuvm->kernel_alloc_node, 0, sizeof(struct >> drm_gpuva)); >> + drm_gpuvm_check_overflow(gpuvm, start_offset, range); >> + gpuvm->mm_start = start_offset; >> + gpuvm->mm_range = range; >> >> + memset(&gpuvm->kernel_alloc_node, 0, sizeof(struct >> drm_gpuva)); >> if (reserve_range) { >> gpuvm->kernel_alloc_node.va.addr = reserve_offset; >> gpuvm->kernel_alloc_node.va.range = reserve_range; >> >> - if (likely(!drm_gpuvm_check_overflow(reserve_offset, >> + if (likely(!drm_gpuvm_check_overflow(gpuvm, >> reserve_offset, >> reserve_range))) >> __drm_gpuva_insert(gpuvm, &gpuvm- >>> kernel_alloc_node); >> } >> @@ -712,8 +713,8 @@ drm_gpuvm_destroy(struct drm_gpuvm *gpuvm) >> if (gpuvm->kernel_alloc_node.va.range) >> __drm_gpuva_remove(&gpuvm->kernel_alloc_node); >> >> - WARN(!RB_EMPTY_ROOT(&gpuvm->rb.tree.rb_root), >> - "GPUVA tree is not empty, potentially leaking memory."); >> + drm_WARN(gpuvm->drm, !RB_EMPTY_ROOT(&gpuvm->rb.tree.rb_root), >> + "GPUVA tree is not empty, potentially leaking >> memory.\n"); >> } >> EXPORT_SYMBOL_GPL(drm_gpuvm_destroy); >> >> @@ -795,7 +796,8 @@ drm_gpuva_remove(struct drm_gpuva *va) >> struct drm_gpuvm *gpuvm = va->vm; >> >> if (unlikely(va == &gpuvm->kernel_alloc_node)) { >> - WARN(1, "Can't destroy kernel reserved node.\n"); >> + drm_WARN(gpuvm->drm, 1, >> + "Can't destroy kernel reserved node.\n"); >> return; >> } >> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c >> b/drivers/gpu/drm/nouveau/nouveau_uvmm.c >> index 5cf892c50f43..aaf5d28bd587 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c >> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c >> @@ -1808,6 +1808,7 @@ int >> nouveau_uvmm_init(struct nouveau_uvmm *uvmm, struct nouveau_cli >> *cli, >> u64 kernel_managed_addr, u64 kernel_managed_size) >> { >> + struct drm_device *drm = cli->drm->dev; >> int ret; >> u64 kernel_managed_end = kernel_managed_addr + >> kernel_managed_size; >> >> @@ -1836,7 +1837,7 @@ nouveau_uvmm_init(struct nouveau_uvmm *uvmm, >> struct nouveau_cli *cli, >> uvmm->kernel_managed_addr = kernel_managed_addr; >> uvmm->kernel_managed_size = kernel_managed_size; >> >> - drm_gpuvm_init(&uvmm->base, cli->name, >> + drm_gpuvm_init(&uvmm->base, cli->name, drm, >> NOUVEAU_VA_SPACE_START, >> NOUVEAU_VA_SPACE_END, >> kernel_managed_addr, kernel_managed_size, >> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h >> index bdfafc4a7705..687fd5893624 100644 >> --- a/include/drm/drm_gpuvm.h >> +++ b/include/drm/drm_gpuvm.h >> @@ -29,6 +29,7 @@ >> #include <linux/rbtree.h> >> #include <linux/types.h> >> >> +#include <drm/drm_device.h> >> #include <drm/drm_gem.h> >> >> struct drm_gpuvm; >> @@ -201,6 +202,11 @@ struct drm_gpuvm { >> */ >> const char *name; >> >> + /** >> + * @drm: the &drm_device this VM lives in >> + */ > > Could a one-liner do? > /** <comment> */
There are a few more existing ones that could be a one-liner as well and I like consistency. If you think it's preferrable to keep those ones in one line, I'd probably do it for all in a follow-up patch.
> >> + struct drm_device *drm; >> + >> /** >> * @mm_start: start of the VA space >> */ >> @@ -241,6 +247,7 @@ struct drm_gpuvm { >> }; >> >> void drm_gpuvm_init(struct drm_gpuvm *gpuvm, const char *name, >> + struct drm_device *drm, >> u64 start_offset, u64 range, >> u64 reserve_offset, u64 reserve_range, >> const struct drm_gpuvm_ops *ops); > > I figure Christian's commend can be addressed in a follow-up patch if > neeed.
I already addressed his comment in a local branch, I can also just add the patch to this series.
> > Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >
| |