On Tue, Aug 31, 2021 at 02:57:05PM +0200, Christian König wrote:
Am 31.08.21 um 14:52 schrieb Daniel Vetter:
On Mon, Aug 30, 2021 at 10:56:59AM +0200, Christian König wrote:
It makes sense to have this in the common manager for debugging and accounting of how much resources are used.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/ttm/ttm_resource.c | 8 ++++++++ include/drm/ttm/ttm_resource.h | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+)
diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c index a4c495da0040..426e6841fc89 100644 --- a/drivers/gpu/drm/ttm/ttm_resource.c +++ b/drivers/gpu/drm/ttm/ttm_resource.c @@ -33,6 +33,8 @@ void ttm_resource_init(struct ttm_buffer_object *bo, const struct ttm_place *place, struct ttm_resource *res) {
- struct ttm_resource_manager *man;
- res->start = 0; res->num_pages = PFN_UP(bo->base.size); res->mem_type = place->mem_type;
@@ -42,12 +44,16 @@ void ttm_resource_init(struct ttm_buffer_object *bo, res->bus.is_iomem = false; res->bus.caching = ttm_cached; res->bo = bo;
- man = ttm_manager_type(bo->bdev, place->mem_type);
- atomic64_add(bo->base.size, &man->usage); } EXPORT_SYMBOL(ttm_resource_init); void ttm_resource_fini(struct ttm_resource_manager *man, struct ttm_resource *res) {
- atomic64_sub(res->bo->base.size, &man->usage); } EXPORT_SYMBOL(ttm_resource_fini);
@@ -100,6 +106,7 @@ void ttm_resource_manager_init(struct ttm_resource_manager *man, spin_lock_init(&man->move_lock); man->bdev = bdev; man->size = p_size;
- atomic64_set(&man->usage, 0); for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) INIT_LIST_HEAD(&man->lru[i]);
@@ -172,6 +179,7 @@ void ttm_resource_manager_debug(struct ttm_resource_manager *man, drm_printf(p, " use_type: %d\n", man->use_type); drm_printf(p, " use_tt: %d\n", man->use_tt); drm_printf(p, " size: %llu\n", man->size);
- drm_printf(p, " usage: %llu\n", atomic64_read(&man->usage)); if (man->func->debug) man->func->debug(man, p); }
diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h index e8080192cae4..526fe359c603 100644 --- a/include/drm/ttm/ttm_resource.h +++ b/include/drm/ttm/ttm_resource.h @@ -27,6 +27,7 @@ #include <linux/types.h> #include <linux/mutex.h> +#include <linux/atomic.h> #include <linux/dma-buf-map.h> #include <linux/dma-fence.h> #include <drm/drm_print.h> @@ -110,6 +111,7 @@ struct ttm_resource_manager_func {
- static information. bdev::driver::io_mem_free is never used.
- @lru: The lru list for this memory type.
- @move: The fence of the last pipelined move operation.
*/
- @usage: How much of the region is used.
- This structure is used to identify and manage memory types for a device.
@@ -134,6 +136,9 @@ struct ttm_resource_manager { * Protected by @move_lock. */ struct dma_fence *move;
- /* Own protection */
Please document struct members with the inline kerneldoc style, that way the comment here shows up in the kerneldoc too.
Would be good to just convert the entire struct I think.
- atomic64_t usage;
Shouldn't we keep track of this together with the lru updates, under the same spinlock?
Mhm, what should that be good for? As far as I know we use it for two use cases:
- Early abort when size-usage < requested.
But doesn't have all kinds of potential temporary leaks again? Except this time around we can't even fix them eventually, because these allocations aren't on the lru list at all.
- Statistics for debugging.
Especially the first use case is rather important under memory pressure to avoid costly acquiring of a contended lock.
Or maybe I'm just not understanding where this matters? Don't you need to grab the lru lock anyway under memory pressure?
Otherwise this usage here just becomes kinda meaningless I think, and just for some debugging. I really don't like sprinkling random atomic_t around (mostly because i915-gem code has gone totally overboard with them, with complete disregard to complexity of the result).
Well this here just replaces what drivers did anyway and the cost of inc/dec an atomic is pretty much negligible.
Complexity in understanding the locking. The cpu has not problem with this stuff ofc. -Daniel
Christian.
-Daniel
}; /** @@ -260,6 +265,19 @@ ttm_resource_manager_cleanup(struct ttm_resource_manager *man) man->move = NULL; } +/**
- ttm_resource_manager_usage
- @man: A memory manager object.
- Return how many resources are currently used.
- */
+static inline uint64_t +ttm_resource_manager_usage(struct ttm_resource_manager *man) +{
- return atomic64_read(&man->usage);
+}
- void ttm_resource_init(struct ttm_buffer_object *bo, const struct ttm_place *place, struct ttm_resource *res);
-- 2.25.1