| Message ID | 20231023201659.25332-2-dakr@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DRM GPUVM features | expand |
Am 23.10.23 um 22:16 schrieb Danilo Krummrich: > 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); When those parameters come from userspace you don't really want a warning in the system log in the first place. Otherwise userspace can trivially spam the system log with warnings. The usual approach is to make this debug level severity instead. Regards, Christian. > } > @@ -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 > + */ > + 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);
On 10/24/23 10:45, Christian König wrote: > > > Am 23.10.23 um 22:16 schrieb Danilo Krummrich: >> 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); > > When those parameters come from userspace you don't really want a warning in the system log in the first place. > > Otherwise userspace can trivially spam the system log with warnings. The usual approach is to make this debug level severity instead. Currently, this function isn't exported and hence the driver should do the relevant sanity checks before attempting to insert the mapping. However, I think it would make sense to export this function and remove the WARN() and instead WARN() in drm_gpuvm_init() explicitly. > > Regards, > Christian. > >> } >> @@ -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 >> + */ >> + 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); >
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> */ > + 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. Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
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> >
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 + */ + 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);
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(-)