| Message ID | 20210430092508.60710-6-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/13] drm/ttm: add ttm_sys_manager v2 | expand |
On Fri, 30 Apr 2021 at 10:25, Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > > Start with the range manager to make the resource object the base > class for the allocated nodes. > > While at it cleanup a lot of the code around that. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 + > drivers/gpu/drm/drm_gem_vram_helper.c | 2 + > drivers/gpu/drm/nouveau/nouveau_ttm.c | 1 + > drivers/gpu/drm/qxl/qxl_ttm.c | 1 + > drivers/gpu/drm/radeon/radeon_ttm.c | 1 + > drivers/gpu/drm/ttm/ttm_range_manager.c | 56 ++++++++++++++++++------- > drivers/gpu/drm/ttm/ttm_resource.c | 26 ++++++++---- > include/drm/ttm/ttm_bo_driver.h | 26 ------------ > include/drm/ttm/ttm_range_manager.h | 43 +++++++++++++++++++ > include/drm/ttm/ttm_resource.h | 3 ++ > 10 files changed, 110 insertions(+), 50 deletions(-) > create mode 100644 include/drm/ttm/ttm_range_manager.h > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 3fe2482a40b4..3d5863262337 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -46,6 +46,7 @@ > #include <drm/ttm/ttm_bo_api.h> > #include <drm/ttm/ttm_bo_driver.h> > #include <drm/ttm/ttm_placement.h> > +#include <drm/ttm/ttm_range_manager.h> > > #include <drm/amdgpu_drm.h> > > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c > index 83e7258c7f90..17a4c5d47b6a 100644 > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > @@ -17,6 +17,8 @@ > #include <drm/drm_prime.h> > #include <drm/drm_simple_kms_helper.h> > > +#include <drm/ttm/ttm_range_manager.h> > + > static const struct drm_gem_object_funcs drm_gem_vram_object_funcs; > > /** > diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c > index b81ae90b8449..15c7627f8f58 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c > @@ -32,6 +32,7 @@ > #include "nouveau_ttm.h" > > #include <drm/drm_legacy.h> > +#include <drm/ttm/ttm_range_manager.h> > > #include <core/tegra.h> > > diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c > index 8aa87b8edb9c..19fd39d9a00c 100644 > --- a/drivers/gpu/drm/qxl/qxl_ttm.c > +++ b/drivers/gpu/drm/qxl/qxl_ttm.c > @@ -32,6 +32,7 @@ > #include <drm/ttm/ttm_bo_api.h> > #include <drm/ttm/ttm_bo_driver.h> > #include <drm/ttm/ttm_placement.h> > +#include <drm/ttm/ttm_range_manager.h> > > #include "qxl_drv.h" > #include "qxl_object.h" > diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c > index 5191e994bff7..c3d2f1fce71a 100644 > --- a/drivers/gpu/drm/radeon/radeon_ttm.c > +++ b/drivers/gpu/drm/radeon/radeon_ttm.c > @@ -46,6 +46,7 @@ > #include <drm/ttm/ttm_bo_api.h> > #include <drm/ttm/ttm_bo_driver.h> > #include <drm/ttm/ttm_placement.h> > +#include <drm/ttm/ttm_range_manager.h> > > #include "radeon_reg.h" > #include "radeon.h" > diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c b/drivers/gpu/drm/ttm/ttm_range_manager.c > index b9d5da6e6a81..ce5d07ca384c 100644 > --- a/drivers/gpu/drm/ttm/ttm_range_manager.c > +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c > @@ -29,12 +29,13 @@ > * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com> > */ > > -#include <drm/ttm/ttm_bo_driver.h> > +#include <drm/ttm/ttm_device.h> > #include <drm/ttm/ttm_placement.h> > +#include <drm/ttm/ttm_range_manager.h> > +#include <drm/ttm/ttm_bo_api.h> > #include <drm/drm_mm.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > -#include <linux/module.h> > > /* > * Currently we use a spinlock for the lock, but a mutex *may* be > @@ -60,8 +61,8 @@ static int ttm_range_man_alloc(struct ttm_resource_manager *man, > struct ttm_resource *mem) > { > struct ttm_range_manager *rman = to_range_manager(man); > + struct ttm_range_mgr_node *node; > struct drm_mm *mm = &rman->mm; > - struct drm_mm_node *node; > enum drm_mm_insert_mode mode; > unsigned long lpfn; > int ret; > @@ -70,7 +71,7 @@ static int ttm_range_man_alloc(struct ttm_resource_manager *man, > if (!lpfn) > lpfn = man->size; > > - node = kzalloc(sizeof(*node), GFP_KERNEL); > + node = kzalloc(struct_size(node, mm_nodes, 1), GFP_KERNEL); > if (!node) > return -ENOMEM; > > @@ -78,17 +79,19 @@ static int ttm_range_man_alloc(struct ttm_resource_manager *man, > if (place->flags & TTM_PL_FLAG_TOPDOWN) > mode = DRM_MM_INSERT_HIGH; > > + ttm_resource_init(bo, place, &node->base); > + > spin_lock(&rman->lock); > - ret = drm_mm_insert_node_in_range(mm, node, mem->num_pages, > - bo->page_alignment, 0, > + ret = drm_mm_insert_node_in_range(mm, &node->mm_nodes[0], > + mem->num_pages, bo->page_alignment, 0, > place->fpfn, lpfn, mode); > spin_unlock(&rman->lock); > > if (unlikely(ret)) { > kfree(node); > } else { > - mem->mm_node = node; > - mem->start = node->start; > + mem->mm_node = &node->mm_nodes[0]; > + mem->start = node->mm_nodes[0].start; > } > > return ret; > @@ -98,15 +101,19 @@ static void ttm_range_man_free(struct ttm_resource_manager *man, > struct ttm_resource *mem) > { > struct ttm_range_manager *rman = to_range_manager(man); > + struct ttm_range_mgr_node *node; > > - if (mem->mm_node) { > - spin_lock(&rman->lock); > - drm_mm_remove_node(mem->mm_node); > - spin_unlock(&rman->lock); > + if (!mem->mm_node) > + return; > > - kfree(mem->mm_node); > - mem->mm_node = NULL; > - } > + node = to_ttm_range_mgr_node(mem); > + > + spin_lock(&rman->lock); > + drm_mm_remove_node(&node->mm_nodes[0]); > + spin_unlock(&rman->lock); > + > + kfree(node); > + mem->mm_node = NULL; > } > > static void ttm_range_man_debug(struct ttm_resource_manager *man, > @@ -125,6 +132,17 @@ static const struct ttm_resource_manager_func ttm_range_manager_func = { > .debug = ttm_range_man_debug > }; > > +/** > + * ttm_range_man_init > + * > + * @bdev: ttm device > + * @type: memory manager type > + * @use_tt: if the memory manager uses tt > + * @p_size: size of area to be managed in pages. > + * > + * Initialise a generic range manager for the selected memory type. > + * The range manager is installed for this device in the type slot. > + */ > int ttm_range_man_init(struct ttm_device *bdev, > unsigned type, bool use_tt, > unsigned long p_size) > @@ -152,6 +170,14 @@ int ttm_range_man_init(struct ttm_device *bdev, > } > EXPORT_SYMBOL(ttm_range_man_init); > > +/** > + * ttm_range_man_fini > + * > + * @bdev: ttm device > + * @type: memory manager type > + * > + * Remove the generic range manager from a slot and tear it down. > + */ > int ttm_range_man_fini(struct ttm_device *bdev, > unsigned type) > { > diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c > index 7ebcc3e6818c..b412ae515e98 100644 > --- a/drivers/gpu/drm/ttm/ttm_resource.c > +++ b/drivers/gpu/drm/ttm/ttm_resource.c > @@ -25,16 +25,10 @@ > #include <drm/ttm/ttm_resource.h> > #include <drm/ttm/ttm_bo_driver.h> > > -int ttm_resource_alloc(struct ttm_buffer_object *bo, > - const struct ttm_place *place, > - struct ttm_resource **res_ptr) > +void ttm_resource_init(struct ttm_buffer_object *bo, > + const struct ttm_place *place, > + struct ttm_resource *res) > { > - struct ttm_resource_manager *man = > - ttm_manager_type(bo->bdev, place->mem_type); > - struct ttm_resource *res; > - int r; > - > - res = kmalloc(sizeof(*res), GFP_KERNEL); > res->mm_node = NULL; > res->start = 0; > res->num_pages = PFN_UP(bo->base.size); > @@ -44,6 +38,20 @@ int ttm_resource_alloc(struct ttm_buffer_object *bo, > res->bus.offset = 0; > res->bus.is_iomem = false; > res->bus.caching = ttm_cached; > +} > +EXPORT_SYMBOL(ttm_resource_init); > + > +int ttm_resource_alloc(struct ttm_buffer_object *bo, > + const struct ttm_place *place, > + struct ttm_resource **res_ptr) > +{ > + struct ttm_resource_manager *man = > + ttm_manager_type(bo->bdev, place->mem_type); > + struct ttm_resource *res; > + int r; > + > + res = kmalloc(sizeof(*res), GFP_KERNEL); If (!res) > + ttm_resource_init(bo, place, res); > r = man->func->alloc(man, bo, place, res); > if (r) { > kfree(res); > diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h > index ead0ef7136c8..b266971c1974 100644 > --- a/include/drm/ttm/ttm_bo_driver.h > +++ b/include/drm/ttm/ttm_bo_driver.h > @@ -304,30 +304,4 @@ int ttm_bo_tt_bind(struct ttm_buffer_object *bo, struct ttm_resource *mem); > */ > void ttm_bo_tt_destroy(struct ttm_buffer_object *bo); > > -/** > - * ttm_range_man_init > - * > - * @bdev: ttm device > - * @type: memory manager type > - * @use_tt: if the memory manager uses tt > - * @p_size: size of area to be managed in pages. > - * > - * Initialise a generic range manager for the selected memory type. > - * The range manager is installed for this device in the type slot. > - */ > -int ttm_range_man_init(struct ttm_device *bdev, > - unsigned type, bool use_tt, > - unsigned long p_size); > - > -/** > - * ttm_range_man_fini > - * > - * @bdev: ttm device > - * @type: memory manager type > - * > - * Remove the generic range manager from a slot and tear it down. > - */ > -int ttm_range_man_fini(struct ttm_device *bdev, > - unsigned type); > - > #endif > diff --git a/include/drm/ttm/ttm_range_manager.h b/include/drm/ttm/ttm_range_manager.h > new file mode 100644 > index 000000000000..e02b6c8d355e > --- /dev/null > +++ b/include/drm/ttm/ttm_range_manager.h > @@ -0,0 +1,43 @@ > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ > + > +#ifndef _TTM_RANGE_MANAGER_H_ > +#define _TTM_RANGE_MANAGER_H_ > + > +#include <drm/ttm/ttm_resource.h> > +#include <drm/drm_mm.h> > + > +/** > + * struct ttm_range_mgr_node > + * > + * @base: base clase we extend > + * @mm_nodes: MM nodes, usually 1 > + * > + * Extending the ttm_resource object to manage an address space allocation with > + * one or more drm_mm_nodes. > + */ > +struct ttm_range_mgr_node { > + struct ttm_resource base; > + struct drm_mm_node mm_nodes[]; > +}; > + > +/** > + * to_ttm_range_mgr_node > + * > + * @res: the resource to upcast > + * > + * Upcast the ttm_resource object into a ttm_range_mgr_node object. > + */ > +static inline struct ttm_range_mgr_node * > +to_ttm_range_mgr_node(struct ttm_resource *res) > +{ > + return container_of(res->mm_node, struct ttm_range_mgr_node, > + mm_nodes[1]); Should be mm_nodes[0]? Otherwise I think looks good, Reviewed-by: Matthew Auld <matthew.auld@intel.com>
Hi, Christian, On 4/30/21 11:25 AM, Christian König wrote: > Start with the range manager to make the resource object the base > class for the allocated nodes. > > While at it cleanup a lot of the code around that. Could you briefly describe the design thoughts around this. While it's clear to me that we want separately allocated struct ttm_resource objects, it's not clear why the visibility of those are pushed down the interfaces to the range managers? In addition to the need for a separately allocated struct ttm_resource, when looking at TTM-ify i915 I've come across a couple of problems. 1) People have started abusing the range manager interface to attach device private data to the mm_node, or probably really to the struct ttm_resource. That makes it very unclear what the input needed for the managers really are. For examle what members of the bo does the range manager really use and why? Same for the struct ttm_resource. I think in a perfect world, the interface to these range managers should be a struct ttm_placement as input and as output an opaque mm node and perhaps a way to convert that mm node to something useful like a range or a scatter-gather table. 2) But that doesn't really address the problem of drivers wanting to attach device private data to a struct ttm_resource, which at some point caused someone to add a bo to the manager interface. The novueau driver attaches a "kind" member to the mm node that it pulls out of the bo; The i915 driver would want to cache an st table and a radix tree to cache index-to-pfn maps. 3) In the end we'd probably want the kmap iterator methods and the various mapping funtions to be methods of the struct ttm_resource. So basically here 1) Would help making range managers with various functionality simple to write and share. 2) Would help drivers attach private data to a struct ttm_resource without abusing the manager interfaces, 3) Would help clean up the mapping code. But at least 2) here would probably mean that we need a driver callback to allocate a struct ttm_resource rather than having the managers allocate them. Drivers can then embed them in private structs if needed. Or is there a way to achieve these goals or something similar with the approach you are taking here? Thanks, Thomas
Hi Thomas, Am 29.05.21 um 17:48 schrieb Thomas Hellström (Intel): > Hi, Christian, > > On 4/30/21 11:25 AM, Christian König wrote: >> Start with the range manager to make the resource object the base >> class for the allocated nodes. >> >> While at it cleanup a lot of the code around that. > > Could you briefly describe the design thoughts around this. While it's > clear to me that we want separately allocated struct ttm_resource > objects, it's not clear why the visibility of those are pushed down > the interfaces to the range managers? Why do you think the visibility is pushed to the range manger? > > In addition to the need for a separately allocated struct > ttm_resource, when looking at TTM-ify i915 I've come across a couple > of problems. > > 1) People have started abusing the range manager interface to attach > device private data to the mm_node, or probably really to the struct > ttm_resource. That makes it very unclear what the input needed for the > managers really are. For examle what members of the bo does the range > manager really use and why? Same for the struct ttm_resource. I think > in a perfect world, the interface to these range managers should be a > struct ttm_placement as input and as output an opaque mm node and > perhaps a way to convert that mm node to something useful like a range > or a scatter-gather table. Well I don't see that as an abuse. The backend allocation are completely driver specific and the range manager is just implementing some common ground for drm_mm based backends. > > 2) But that doesn't really address the problem of drivers wanting to > attach device private data to a struct ttm_resource, which at some > point caused someone to add a bo to the manager interface. The novueau > driver attaches a "kind" member to the mm node that it pulls out of > the bo; The i915 driver would want to cache an st table and a radix > tree to cache index-to-pfn maps. Driver specific backends should inherit either from the range manager when they want to implement a drm_mm based backend or from ttm_resource directly. > > 3) In the end we'd probably want the kmap iterator methods and the > various mapping funtions to be methods of the struct ttm_resource. Yes moving the iterators into that was also my idea. > > So basically here > > 1) Would help making range managers with various functionality simple > to write and share. I don't think we want that. Instead allocation backends should be driver specific and we should just implement some common ground helpers for drm_mm based backends in TTM. > 2) Would help drivers attach private data to a struct ttm_resource > without abusing the manager interfaces, I don't think that this is abusive in the first place. Drivers should append resource specific information by inheriting from the ttm_resource object. Regards, Christian. > 3) Would help clean up the mapping code. > > But at least 2) here would probably mean that we need a driver > callback to allocate a struct ttm_resource rather than having the > managers allocate them. Drivers can then embed them in private structs > if needed. > > Or is there a way to achieve these goals or something similar with the > approach you are taking here? > > Thanks, > > Thomas > > >
On 5/30/21 6:51 PM, Christian König wrote: > Hi Thomas, > > Am 29.05.21 um 17:48 schrieb Thomas Hellström (Intel): >> Hi, Christian, >> >> On 4/30/21 11:25 AM, Christian König wrote: >>> Start with the range manager to make the resource object the base >>> class for the allocated nodes. >>> >>> While at it cleanup a lot of the code around that. >> >> Could you briefly describe the design thoughts around this. While >> it's clear to me that we want separately allocated struct >> ttm_resource objects, it's not clear why the visibility of those are >> pushed down the interfaces to the range managers? > > Why do you think the visibility is pushed to the range manger? > >> >> In addition to the need for a separately allocated struct >> ttm_resource, when looking at TTM-ify i915 I've come across a couple >> of problems. >> >> 1) People have started abusing the range manager interface to attach >> device private data to the mm_node, or probably really to the struct >> ttm_resource. That makes it very unclear what the input needed for >> the managers really are. For examle what members of the bo does the >> range manager really use and why? Same for the struct ttm_resource. I >> think in a perfect world, the interface to these range managers >> should be a struct ttm_placement as input and as output an opaque mm >> node and perhaps a way to convert that mm node to something useful >> like a range or a scatter-gather table. > > Well I don't see that as an abuse. The backend allocation are > completely driver specific and the range manager is just implementing > some common ground for drm_mm based backends. > >> >> 2) But that doesn't really address the problem of drivers wanting to >> attach device private data to a struct ttm_resource, which at some >> point caused someone to add a bo to the manager interface. The >> novueau driver attaches a "kind" member to the mm node that it pulls >> out of the bo; The i915 driver would want to cache an st table and a >> radix tree to cache index-to-pfn maps. > > Driver specific backends should inherit either from the range manager > when they want to implement a drm_mm based backend or from > ttm_resource directly. Hmm, OK so in our case a driver that needs a driver-specific struct ttm_resource, but still wants to be able to allocate either from drm_mm or from the buddy would then either have to re-implement the TTM drm_mm allocator or live with a pretty awkward construct? struct i915_ttm_resource { union { struct ttm_resource res; struct ttm_range_mgr_node range_node; // Let's hope the struct ttm_resource remains the first member. struct i915_buddy_node buddy_node; }; struct i915_private_stuff common_for_all_backends; }; /Thomas
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 3fe2482a40b4..3d5863262337 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -46,6 +46,7 @@ #include <drm/ttm/ttm_bo_api.h> #include <drm/ttm/ttm_bo_driver.h> #include <drm/ttm/ttm_placement.h> +#include <drm/ttm/ttm_range_manager.h> #include <drm/amdgpu_drm.h> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 83e7258c7f90..17a4c5d47b6a 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -17,6 +17,8 @@ #include <drm/drm_prime.h> #include <drm/drm_simple_kms_helper.h> +#include <drm/ttm/ttm_range_manager.h> + static const struct drm_gem_object_funcs drm_gem_vram_object_funcs; /** diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c index b81ae90b8449..15c7627f8f58 100644 --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c @@ -32,6 +32,7 @@ #include "nouveau_ttm.h" #include <drm/drm_legacy.h> +#include <drm/ttm/ttm_range_manager.h> #include <core/tegra.h> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c index 8aa87b8edb9c..19fd39d9a00c 100644 --- a/drivers/gpu/drm/qxl/qxl_ttm.c +++ b/drivers/gpu/drm/qxl/qxl_ttm.c @@ -32,6 +32,7 @@ #include <drm/ttm/ttm_bo_api.h> #include <drm/ttm/ttm_bo_driver.h> #include <drm/ttm/ttm_placement.h> +#include <drm/ttm/ttm_range_manager.h> #include "qxl_drv.h" #include "qxl_object.h" diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index 5191e994bff7..c3d2f1fce71a 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -46,6 +46,7 @@ #include <drm/ttm/ttm_bo_api.h> #include <drm/ttm/ttm_bo_driver.h> #include <drm/ttm/ttm_placement.h> +#include <drm/ttm/ttm_range_manager.h> #include "radeon_reg.h" #include "radeon.h" diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c b/drivers/gpu/drm/ttm/ttm_range_manager.c index b9d5da6e6a81..ce5d07ca384c 100644 --- a/drivers/gpu/drm/ttm/ttm_range_manager.c +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c @@ -29,12 +29,13 @@ * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com> */ -#include <drm/ttm/ttm_bo_driver.h> +#include <drm/ttm/ttm_device.h> #include <drm/ttm/ttm_placement.h> +#include <drm/ttm/ttm_range_manager.h> +#include <drm/ttm/ttm_bo_api.h> #include <drm/drm_mm.h> #include <linux/slab.h> #include <linux/spinlock.h> -#include <linux/module.h> /* * Currently we use a spinlock for the lock, but a mutex *may* be @@ -60,8 +61,8 @@ static int ttm_range_man_alloc(struct ttm_resource_manager *man, struct ttm_resource *mem) { struct ttm_range_manager *rman = to_range_manager(man); + struct ttm_range_mgr_node *node; struct drm_mm *mm = &rman->mm; - struct drm_mm_node *node; enum drm_mm_insert_mode mode; unsigned long lpfn; int ret; @@ -70,7 +71,7 @@ static int ttm_range_man_alloc(struct ttm_resource_manager *man, if (!lpfn) lpfn = man->size; - node = kzalloc(sizeof(*node), GFP_KERNEL); + node = kzalloc(struct_size(node, mm_nodes, 1), GFP_KERNEL); if (!node) return -ENOMEM; @@ -78,17 +79,19 @@ static int ttm_range_man_alloc(struct ttm_resource_manager *man, if (place->flags & TTM_PL_FLAG_TOPDOWN) mode = DRM_MM_INSERT_HIGH; + ttm_resource_init(bo, place, &node->base); + spin_lock(&rman->lock); - ret = drm_mm_insert_node_in_range(mm, node, mem->num_pages, - bo->page_alignment, 0, + ret = drm_mm_insert_node_in_range(mm, &node->mm_nodes[0], + mem->num_pages, bo->page_alignment, 0, place->fpfn, lpfn, mode); spin_unlock(&rman->lock); if (unlikely(ret)) { kfree(node); } else { - mem->mm_node = node; - mem->start = node->start; + mem->mm_node = &node->mm_nodes[0]; + mem->start = node->mm_nodes[0].start; } return ret; @@ -98,15 +101,19 @@ static void ttm_range_man_free(struct ttm_resource_manager *man, struct ttm_resource *mem) { struct ttm_range_manager *rman = to_range_manager(man); + struct ttm_range_mgr_node *node; - if (mem->mm_node) { - spin_lock(&rman->lock); - drm_mm_remove_node(mem->mm_node); - spin_unlock(&rman->lock); + if (!mem->mm_node) + return; - kfree(mem->mm_node); - mem->mm_node = NULL; - } + node = to_ttm_range_mgr_node(mem); + + spin_lock(&rman->lock); + drm_mm_remove_node(&node->mm_nodes[0]); + spin_unlock(&rman->lock); + + kfree(node); + mem->mm_node = NULL; } static void ttm_range_man_debug(struct ttm_resource_manager *man, @@ -125,6 +132,17 @@ static const struct ttm_resource_manager_func ttm_range_manager_func = { .debug = ttm_range_man_debug }; +/** + * ttm_range_man_init + * + * @bdev: ttm device + * @type: memory manager type + * @use_tt: if the memory manager uses tt + * @p_size: size of area to be managed in pages. + * + * Initialise a generic range manager for the selected memory type. + * The range manager is installed for this device in the type slot. + */ int ttm_range_man_init(struct ttm_device *bdev, unsigned type, bool use_tt, unsigned long p_size) @@ -152,6 +170,14 @@ int ttm_range_man_init(struct ttm_device *bdev, } EXPORT_SYMBOL(ttm_range_man_init); +/** + * ttm_range_man_fini + * + * @bdev: ttm device + * @type: memory manager type + * + * Remove the generic range manager from a slot and tear it down. + */ int ttm_range_man_fini(struct ttm_device *bdev, unsigned type) { diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c index 7ebcc3e6818c..b412ae515e98 100644 --- a/drivers/gpu/drm/ttm/ttm_resource.c +++ b/drivers/gpu/drm/ttm/ttm_resource.c @@ -25,16 +25,10 @@ #include <drm/ttm/ttm_resource.h> #include <drm/ttm/ttm_bo_driver.h> -int ttm_resource_alloc(struct ttm_buffer_object *bo, - const struct ttm_place *place, - struct ttm_resource **res_ptr) +void ttm_resource_init(struct ttm_buffer_object *bo, + const struct ttm_place *place, + struct ttm_resource *res) { - struct ttm_resource_manager *man = - ttm_manager_type(bo->bdev, place->mem_type); - struct ttm_resource *res; - int r; - - res = kmalloc(sizeof(*res), GFP_KERNEL); res->mm_node = NULL; res->start = 0; res->num_pages = PFN_UP(bo->base.size); @@ -44,6 +38,20 @@ int ttm_resource_alloc(struct ttm_buffer_object *bo, res->bus.offset = 0; res->bus.is_iomem = false; res->bus.caching = ttm_cached; +} +EXPORT_SYMBOL(ttm_resource_init); + +int ttm_resource_alloc(struct ttm_buffer_object *bo, + const struct ttm_place *place, + struct ttm_resource **res_ptr) +{ + struct ttm_resource_manager *man = + ttm_manager_type(bo->bdev, place->mem_type); + struct ttm_resource *res; + int r; + + res = kmalloc(sizeof(*res), GFP_KERNEL); + ttm_resource_init(bo, place, res); r = man->func->alloc(man, bo, place, res); if (r) { kfree(res); diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index ead0ef7136c8..b266971c1974 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -304,30 +304,4 @@ int ttm_bo_tt_bind(struct ttm_buffer_object *bo, struct ttm_resource *mem); */ void ttm_bo_tt_destroy(struct ttm_buffer_object *bo); -/** - * ttm_range_man_init - * - * @bdev: ttm device - * @type: memory manager type - * @use_tt: if the memory manager uses tt - * @p_size: size of area to be managed in pages. - * - * Initialise a generic range manager for the selected memory type. - * The range manager is installed for this device in the type slot. - */ -int ttm_range_man_init(struct ttm_device *bdev, - unsigned type, bool use_tt, - unsigned long p_size); - -/** - * ttm_range_man_fini - * - * @bdev: ttm device - * @type: memory manager type - * - * Remove the generic range manager from a slot and tear it down. - */ -int ttm_range_man_fini(struct ttm_device *bdev, - unsigned type); - #endif diff --git a/include/drm/ttm/ttm_range_manager.h b/include/drm/ttm/ttm_range_manager.h new file mode 100644 index 000000000000..e02b6c8d355e --- /dev/null +++ b/include/drm/ttm/ttm_range_manager.h @@ -0,0 +1,43 @@ +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ + +#ifndef _TTM_RANGE_MANAGER_H_ +#define _TTM_RANGE_MANAGER_H_ + +#include <drm/ttm/ttm_resource.h> +#include <drm/drm_mm.h> + +/** + * struct ttm_range_mgr_node + * + * @base: base clase we extend + * @mm_nodes: MM nodes, usually 1 + * + * Extending the ttm_resource object to manage an address space allocation with + * one or more drm_mm_nodes. + */ +struct ttm_range_mgr_node { + struct ttm_resource base; + struct drm_mm_node mm_nodes[]; +}; + +/** + * to_ttm_range_mgr_node + * + * @res: the resource to upcast + * + * Upcast the ttm_resource object into a ttm_range_mgr_node object. + */ +static inline struct ttm_range_mgr_node * +to_ttm_range_mgr_node(struct ttm_resource *res) +{ + return container_of(res->mm_node, struct ttm_range_mgr_node, + mm_nodes[1]); +} + +int ttm_range_man_init(struct ttm_device *bdev, + unsigned type, bool use_tt, + unsigned long p_size); +int ttm_range_man_fini(struct ttm_device *bdev, + unsigned type); + +#endif diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h index c17c1a52070d..803e4875d779 100644 --- a/include/drm/ttm/ttm_resource.h +++ b/include/drm/ttm/ttm_resource.h @@ -223,6 +223,9 @@ ttm_resource_manager_cleanup(struct ttm_resource_manager *man) man->move = NULL; } +void ttm_resource_init(struct ttm_buffer_object *bo, + const struct ttm_place *place, + struct ttm_resource *res); int ttm_resource_alloc(struct ttm_buffer_object *bo, const struct ttm_place *place, struct ttm_resource **res);
Start with the range manager to make the resource object the base class for the allocated nodes. While at it cleanup a lot of the code around that. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 + drivers/gpu/drm/drm_gem_vram_helper.c | 2 + drivers/gpu/drm/nouveau/nouveau_ttm.c | 1 + drivers/gpu/drm/qxl/qxl_ttm.c | 1 + drivers/gpu/drm/radeon/radeon_ttm.c | 1 + drivers/gpu/drm/ttm/ttm_range_manager.c | 56 ++++++++++++++++++------- drivers/gpu/drm/ttm/ttm_resource.c | 26 ++++++++---- include/drm/ttm/ttm_bo_driver.h | 26 ------------ include/drm/ttm/ttm_range_manager.h | 43 +++++++++++++++++++ include/drm/ttm/ttm_resource.h | 3 ++ 10 files changed, 110 insertions(+), 50 deletions(-) create mode 100644 include/drm/ttm/ttm_range_manager.h