| Message ID | 20210518082701.997251-5-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Move LMEM (VRAM) management over to TTM | expand |
On Tue, May 18, 2021 at 10:26:50AM +0200, Thomas Hellström wrote: > i915 mock selftests are run without the device set up. In order to be able > to run the region related mock selftests, export functions in order for the > TTM range manager to be set up without a device to attach it to. > > Cc: Christian König <christian.koenig@amd.com> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > --- > drivers/gpu/drm/ttm/ttm_range_manager.c | 55 +++++++++++++++++-------- > include/drm/ttm/ttm_bo_driver.h | 23 +++++++++++ > 2 files changed, 61 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c b/drivers/gpu/drm/ttm/ttm_range_manager.c > index b9d5da6e6a81..6957dfb0cf5a 100644 > --- a/drivers/gpu/drm/ttm/ttm_range_manager.c > +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c > @@ -125,55 +125,76 @@ static const struct ttm_resource_manager_func ttm_range_manager_func = { > .debug = ttm_range_man_debug > }; > > -int ttm_range_man_init(struct ttm_device *bdev, > - unsigned type, bool use_tt, > - unsigned long p_size) > +struct ttm_resource_manager * > +ttm_range_man_init_standalone(unsigned long size, bool use_tt) > { > struct ttm_resource_manager *man; > struct ttm_range_manager *rman; > > rman = kzalloc(sizeof(*rman), GFP_KERNEL); > if (!rman) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > man = &rman->manager; > man->use_tt = use_tt; > > man->func = &ttm_range_manager_func; > > - ttm_resource_manager_init(man, p_size); > + ttm_resource_manager_init(man, size); > > - drm_mm_init(&rman->mm, 0, p_size); > + drm_mm_init(&rman->mm, 0, size); > spin_lock_init(&rman->lock); > > - ttm_set_driver_manager(bdev, type, &rman->manager); > + return man; > +} > +EXPORT_SYMBOL(ttm_range_man_init_standalone); > + > +int ttm_range_man_init(struct ttm_device *bdev, > + unsigned int type, bool use_tt, > + unsigned long p_size) > +{ > + struct ttm_resource_manager *man; > + > + man = ttm_range_man_init_standalone(p_size, use_tt); > + if (IS_ERR(man)) > + return PTR_ERR(man); > + > ttm_resource_manager_set_used(man, true); > + ttm_set_driver_manager(bdev, type, man); > + > return 0; > } > EXPORT_SYMBOL(ttm_range_man_init); > > +void ttm_range_man_fini_standalone(struct ttm_resource_manager *man) > +{ > + struct ttm_range_manager *rman = to_range_manager(man); > + struct drm_mm *mm = &rman->mm; > + > + spin_lock(&rman->lock); > + drm_mm_clean(mm); > + drm_mm_takedown(mm); > + spin_unlock(&rman->lock); > + > + ttm_resource_manager_cleanup(man); > + kfree(rman); > +} > +EXPORT_SYMBOL(ttm_range_man_fini_standalone); > + > int ttm_range_man_fini(struct ttm_device *bdev, > unsigned type) > { > struct ttm_resource_manager *man = ttm_manager_type(bdev, type); > - struct ttm_range_manager *rman = to_range_manager(man); > - struct drm_mm *mm = &rman->mm; > int ret; > > ttm_resource_manager_set_used(man, false); > - > ret = ttm_resource_manager_evict_all(bdev, man); > if (ret) > return ret; > > - spin_lock(&rman->lock); > - drm_mm_clean(mm); > - drm_mm_takedown(mm); > - spin_unlock(&rman->lock); > - > - ttm_resource_manager_cleanup(man); > ttm_set_driver_manager(bdev, type, NULL); > - kfree(rman); > + ttm_range_man_fini_standalone(man); > + > return 0; > } > EXPORT_SYMBOL(ttm_range_man_fini); > diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h > index dbccac957f8f..734b1712ea72 100644 > --- a/include/drm/ttm/ttm_bo_driver.h > +++ b/include/drm/ttm/ttm_bo_driver.h > @@ -321,6 +321,20 @@ int ttm_range_man_init(struct ttm_device *bdev, > unsigned type, bool use_tt, > unsigned long p_size); > > +/** > + * ttm_range_man_init_standalone - Initialize a ttm range manager without > + * device interaction. > + * @size: Size of the area to be managed in pages. > + * @use_tt: The memory type requires tt backing. > + * > + * This function is intended for selftests. It initializes a range manager > + * without any device interaction. > + * > + * Return: pointer to a range manager on success. Error pointer on failure. > + */ Kerneldoc is great and I'm happy you're updating them (Christian's not so much good for this), but I think would be good to go one step further with a prep patch: - Make sure ttm_bo_driver.h is appropriately included in Documentation/gpu/drm-mm.rst. - Fix up any kerneldoc fallout. Specifically I think common usage at least is that for non-inline functions, the kerneldoc is in the .c file, not in the headers. But also this might be way too much work since ttm hasn't been properly kerneldoc-ified, so maybe later. -Daniel > +struct ttm_resource_manager * > +ttm_range_man_init_standalone(unsigned long size, bool use_tt); > + > /** > * ttm_range_man_fini > * > @@ -332,4 +346,13 @@ int ttm_range_man_init(struct ttm_device *bdev, > int ttm_range_man_fini(struct ttm_device *bdev, > unsigned type); > > +/** > + * ttm_range_man_fini_standalone > + * @man: The range manager > + * > + * Tear down a range manager initialized with > + * ttm_range_manager_init_standalone(). > + */ > +void ttm_range_man_fini_standalone(struct ttm_resource_manager *man); > + > #endif > -- > 2.31.1 >
Am 18.05.21 um 10:26 schrieb Thomas Hellström: > i915 mock selftests are run without the device set up. In order to be able > to run the region related mock selftests, export functions in order for the > TTM range manager to be set up without a device to attach it to. From the code it looks good, but to be honest I don't think that this makes much sense from the organizational point of view. If a self test exercises internals of TTM it should be moved into TTM as well. Christian. > > Cc: Christian König <christian.koenig@amd.com> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > --- > drivers/gpu/drm/ttm/ttm_range_manager.c | 55 +++++++++++++++++-------- > include/drm/ttm/ttm_bo_driver.h | 23 +++++++++++ > 2 files changed, 61 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c b/drivers/gpu/drm/ttm/ttm_range_manager.c > index b9d5da6e6a81..6957dfb0cf5a 100644 > --- a/drivers/gpu/drm/ttm/ttm_range_manager.c > +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c > @@ -125,55 +125,76 @@ static const struct ttm_resource_manager_func ttm_range_manager_func = { > .debug = ttm_range_man_debug > }; > > -int ttm_range_man_init(struct ttm_device *bdev, > - unsigned type, bool use_tt, > - unsigned long p_size) > +struct ttm_resource_manager * > +ttm_range_man_init_standalone(unsigned long size, bool use_tt) > { > struct ttm_resource_manager *man; > struct ttm_range_manager *rman; > > rman = kzalloc(sizeof(*rman), GFP_KERNEL); > if (!rman) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > man = &rman->manager; > man->use_tt = use_tt; > > man->func = &ttm_range_manager_func; > > - ttm_resource_manager_init(man, p_size); > + ttm_resource_manager_init(man, size); > > - drm_mm_init(&rman->mm, 0, p_size); > + drm_mm_init(&rman->mm, 0, size); > spin_lock_init(&rman->lock); > > - ttm_set_driver_manager(bdev, type, &rman->manager); > + return man; > +} > +EXPORT_SYMBOL(ttm_range_man_init_standalone); > + > +int ttm_range_man_init(struct ttm_device *bdev, > + unsigned int type, bool use_tt, > + unsigned long p_size) > +{ > + struct ttm_resource_manager *man; > + > + man = ttm_range_man_init_standalone(p_size, use_tt); > + if (IS_ERR(man)) > + return PTR_ERR(man); > + > ttm_resource_manager_set_used(man, true); > + ttm_set_driver_manager(bdev, type, man); > + > return 0; > } > EXPORT_SYMBOL(ttm_range_man_init); > > +void ttm_range_man_fini_standalone(struct ttm_resource_manager *man) > +{ > + struct ttm_range_manager *rman = to_range_manager(man); > + struct drm_mm *mm = &rman->mm; > + > + spin_lock(&rman->lock); > + drm_mm_clean(mm); > + drm_mm_takedown(mm); > + spin_unlock(&rman->lock); > + > + ttm_resource_manager_cleanup(man); > + kfree(rman); > +} > +EXPORT_SYMBOL(ttm_range_man_fini_standalone); > + > int ttm_range_man_fini(struct ttm_device *bdev, > unsigned type) > { > struct ttm_resource_manager *man = ttm_manager_type(bdev, type); > - struct ttm_range_manager *rman = to_range_manager(man); > - struct drm_mm *mm = &rman->mm; > int ret; > > ttm_resource_manager_set_used(man, false); > - > ret = ttm_resource_manager_evict_all(bdev, man); > if (ret) > return ret; > > - spin_lock(&rman->lock); > - drm_mm_clean(mm); > - drm_mm_takedown(mm); > - spin_unlock(&rman->lock); > - > - ttm_resource_manager_cleanup(man); > ttm_set_driver_manager(bdev, type, NULL); > - kfree(rman); > + ttm_range_man_fini_standalone(man); > + > return 0; > } > EXPORT_SYMBOL(ttm_range_man_fini); > diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h > index dbccac957f8f..734b1712ea72 100644 > --- a/include/drm/ttm/ttm_bo_driver.h > +++ b/include/drm/ttm/ttm_bo_driver.h > @@ -321,6 +321,20 @@ int ttm_range_man_init(struct ttm_device *bdev, > unsigned type, bool use_tt, > unsigned long p_size); > > +/** > + * ttm_range_man_init_standalone - Initialize a ttm range manager without > + * device interaction. > + * @size: Size of the area to be managed in pages. > + * @use_tt: The memory type requires tt backing. > + * > + * This function is intended for selftests. It initializes a range manager > + * without any device interaction. > + * > + * Return: pointer to a range manager on success. Error pointer on failure. > + */ > +struct ttm_resource_manager * > +ttm_range_man_init_standalone(unsigned long size, bool use_tt); > + > /** > * ttm_range_man_fini > * > @@ -332,4 +346,13 @@ int ttm_range_man_init(struct ttm_device *bdev, > int ttm_range_man_fini(struct ttm_device *bdev, > unsigned type); > > +/** > + * ttm_range_man_fini_standalone > + * @man: The range manager > + * > + * Tear down a range manager initialized with > + * ttm_range_manager_init_standalone(). > + */ > +void ttm_range_man_fini_standalone(struct ttm_resource_manager *man); > + > #endif
On 5/18/21 1:51 PM, Christian König wrote: > Am 18.05.21 um 10:26 schrieb Thomas Hellström: >> i915 mock selftests are run without the device set up. In order to be >> able >> to run the region related mock selftests, export functions in order >> for the >> TTM range manager to be set up without a device to attach it to. > > From the code it looks good, but to be honest I don't think that this > makes much sense from the organizational point of view. > > If a self test exercises internals of TTM it should be moved into TTM > as well. This particular selftest actually exercises i915 memory regions which is a level above TTM, but the memory regions are backed by TTM. Since they are mock selftests they don't have a TTM device. For the buddy allocator the situation would be the same, but there we have selftests that exercise the allocator standalone, and those would probably fit best into a TTM selftest infrastructure. Although in this particular case, we could of course add a mock TTM device and be done. Pls let me know what you think. /Thomas
Am 18.05.21 um 15:06 schrieb Thomas Hellström: > > On 5/18/21 1:51 PM, Christian König wrote: >> Am 18.05.21 um 10:26 schrieb Thomas Hellström: >>> i915 mock selftests are run without the device set up. In order to >>> be able >>> to run the region related mock selftests, export functions in order >>> for the >>> TTM range manager to be set up without a device to attach it to. >> >> From the code it looks good, but to be honest I don't think that this >> makes much sense from the organizational point of view. >> >> If a self test exercises internals of TTM it should be moved into TTM >> as well. > > This particular selftest actually exercises i915 memory regions which > is a level above TTM, but the memory regions are backed by TTM. Since > they are mock selftests they don't have a TTM device. For the buddy > allocator the situation would be the same, but there we have selftests > that exercise the allocator standalone, and those would probably fit > best into a TTM selftest infrastructure. > > Although in this particular case, we could of course add a mock TTM > device and be done. Pls let me know what you think. Well if you can do that with a dummy device then that would certainly be the preferred option. Christian. > > /Thomas > >
diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c b/drivers/gpu/drm/ttm/ttm_range_manager.c index b9d5da6e6a81..6957dfb0cf5a 100644 --- a/drivers/gpu/drm/ttm/ttm_range_manager.c +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c @@ -125,55 +125,76 @@ static const struct ttm_resource_manager_func ttm_range_manager_func = { .debug = ttm_range_man_debug }; -int ttm_range_man_init(struct ttm_device *bdev, - unsigned type, bool use_tt, - unsigned long p_size) +struct ttm_resource_manager * +ttm_range_man_init_standalone(unsigned long size, bool use_tt) { struct ttm_resource_manager *man; struct ttm_range_manager *rman; rman = kzalloc(sizeof(*rman), GFP_KERNEL); if (!rman) - return -ENOMEM; + return ERR_PTR(-ENOMEM); man = &rman->manager; man->use_tt = use_tt; man->func = &ttm_range_manager_func; - ttm_resource_manager_init(man, p_size); + ttm_resource_manager_init(man, size); - drm_mm_init(&rman->mm, 0, p_size); + drm_mm_init(&rman->mm, 0, size); spin_lock_init(&rman->lock); - ttm_set_driver_manager(bdev, type, &rman->manager); + return man; +} +EXPORT_SYMBOL(ttm_range_man_init_standalone); + +int ttm_range_man_init(struct ttm_device *bdev, + unsigned int type, bool use_tt, + unsigned long p_size) +{ + struct ttm_resource_manager *man; + + man = ttm_range_man_init_standalone(p_size, use_tt); + if (IS_ERR(man)) + return PTR_ERR(man); + ttm_resource_manager_set_used(man, true); + ttm_set_driver_manager(bdev, type, man); + return 0; } EXPORT_SYMBOL(ttm_range_man_init); +void ttm_range_man_fini_standalone(struct ttm_resource_manager *man) +{ + struct ttm_range_manager *rman = to_range_manager(man); + struct drm_mm *mm = &rman->mm; + + spin_lock(&rman->lock); + drm_mm_clean(mm); + drm_mm_takedown(mm); + spin_unlock(&rman->lock); + + ttm_resource_manager_cleanup(man); + kfree(rman); +} +EXPORT_SYMBOL(ttm_range_man_fini_standalone); + int ttm_range_man_fini(struct ttm_device *bdev, unsigned type) { struct ttm_resource_manager *man = ttm_manager_type(bdev, type); - struct ttm_range_manager *rman = to_range_manager(man); - struct drm_mm *mm = &rman->mm; int ret; ttm_resource_manager_set_used(man, false); - ret = ttm_resource_manager_evict_all(bdev, man); if (ret) return ret; - spin_lock(&rman->lock); - drm_mm_clean(mm); - drm_mm_takedown(mm); - spin_unlock(&rman->lock); - - ttm_resource_manager_cleanup(man); ttm_set_driver_manager(bdev, type, NULL); - kfree(rman); + ttm_range_man_fini_standalone(man); + return 0; } EXPORT_SYMBOL(ttm_range_man_fini); diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index dbccac957f8f..734b1712ea72 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -321,6 +321,20 @@ int ttm_range_man_init(struct ttm_device *bdev, unsigned type, bool use_tt, unsigned long p_size); +/** + * ttm_range_man_init_standalone - Initialize a ttm range manager without + * device interaction. + * @size: Size of the area to be managed in pages. + * @use_tt: The memory type requires tt backing. + * + * This function is intended for selftests. It initializes a range manager + * without any device interaction. + * + * Return: pointer to a range manager on success. Error pointer on failure. + */ +struct ttm_resource_manager * +ttm_range_man_init_standalone(unsigned long size, bool use_tt); + /** * ttm_range_man_fini * @@ -332,4 +346,13 @@ int ttm_range_man_init(struct ttm_device *bdev, int ttm_range_man_fini(struct ttm_device *bdev, unsigned type); +/** + * ttm_range_man_fini_standalone + * @man: The range manager + * + * Tear down a range manager initialized with + * ttm_range_manager_init_standalone(). + */ +void ttm_range_man_fini_standalone(struct ttm_resource_manager *man); + #endif
i915 mock selftests are run without the device set up. In order to be able to run the region related mock selftests, export functions in order for the TTM range manager to be set up without a device to attach it to. Cc: Christian König <christian.koenig@amd.com> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- drivers/gpu/drm/ttm/ttm_range_manager.c | 55 +++++++++++++++++-------- include/drm/ttm/ttm_bo_driver.h | 23 +++++++++++ 2 files changed, 61 insertions(+), 17 deletions(-)