Hi, Christian On Fri, 2024-02-23 at 15:30 +0100, Christian König wrote: > Am 06.02.24 um 13:56 schrieb Christian König: > > Am 06.02.24 um 13:53 schrieb Thomas Hellström: > > > Hi, Christian, > > > > > > On Fri, 2024-01-26 at 15:09 +0100, Christian König wrote: > > > > Previously we would never try to move a BO into the preferred > > > > placements > > > > when it ever landed in a busy placement since those were > > > > considered > > > > compatible. > > > > > > > > Rework the whole handling and finally unify the idle and busy > > > > handling. > > > > ttm_bo_validate() is now responsible to try idle placement > > > > first and > > > > then > > > > use the busy placement if that didn't worked. > > > > > > > > Drawback is that we now always try the idle placement first for > > > > each > > > > validation which might cause some additional CPU overhead on > > > > overcommit. > > > > > > > > v2: fix kerneldoc warning and coding style > > > > v3: take care of XE as well > > > > v4: keep the ttm_bo_mem_space functionality as it is for now, > > > > only > > > > add > > > > new handling for ttm_bo_validate as suggested by Thomas > > > > > > > > Signed-off-by: Christian König <christian.koenig@xxxxxxx> > > > > Reviewed-by: Zack Rusin <zack.rusin@xxxxxxxxxxxx> v3 > > > Sending this through xe CI, will try to review asap. > > > > Take your time. At the moment people are bombarding me with work > > and I > > have only two hands and one head as well :( > > So I've digged myself out of that hole and would rather like to get > this > new feature into 6.9. > > Any time to review it? I can also plan some time to review your LRU > changes next week. > > Thanks, > Christian. Sorry for the late response. Was planning to review but saw that there was still an xe CI failure. https://intel-gfx-ci.01.org/tree/intel-xe/xe-pw-129579v1/bat-atsm-2/igt@xe_evict_ccs@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx I haven't really had time to look into what might be causing this, though. /Thomas > > > > > Christian. > > > > > > > > /Thomas > > > > > > > > > > --- > > > > drivers/gpu/drm/ttm/ttm_bo.c | 231 +++++++++++++------- > > > > ------- > > > > -- > > > > drivers/gpu/drm/ttm/ttm_resource.c | 16 +- > > > > include/drm/ttm/ttm_resource.h | 3 +- > > > > 3 files changed, 121 insertions(+), 129 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c > > > > b/drivers/gpu/drm/ttm/ttm_bo.c > > > > index ba3f09e2d7e6..b12f435542a9 100644 > > > > --- a/drivers/gpu/drm/ttm/ttm_bo.c > > > > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > > > > @@ -724,64 +724,36 @@ static int ttm_bo_add_move_fence(struct > > > > ttm_buffer_object *bo, > > > > return ret; > > > > } > > > > -/* > > > > - * Repeatedly evict memory from the LRU for @mem_type until we > > > > create enough > > > > - * space, or we've evicted everything and there isn't enough > > > > space. > > > > - */ > > > > -static int ttm_bo_mem_force_space(struct ttm_buffer_object > > > > *bo, > > > > - const struct ttm_place *place, > > > > - struct ttm_resource **mem, > > > > - struct ttm_operation_ctx *ctx) > > > > -{ > > > > - struct ttm_device *bdev = bo->bdev; > > > > - struct ttm_resource_manager *man; > > > > - struct ww_acquire_ctx *ticket; > > > > - int ret; > > > > - > > > > - man = ttm_manager_type(bdev, place->mem_type); > > > > - ticket = dma_resv_locking_ctx(bo->base.resv); > > > > - do { > > > > - ret = ttm_resource_alloc(bo, place, mem); > > > > - if (likely(!ret)) > > > > - break; > > > > - if (unlikely(ret != -ENOSPC)) > > > > - return ret; > > > > - ret = ttm_mem_evict_first(bdev, man, place, ctx, > > > > - ticket); > > > > - if (unlikely(ret != 0)) > > > > - return ret; > > > > - } while (1); > > > > - > > > > - return ttm_bo_add_move_fence(bo, man, *mem, ctx- > > > > > no_wait_gpu); > > > > -} > > > > - > > > > /** > > > > - * ttm_bo_mem_space > > > > + * ttm_bo_alloc_resource - Allocate backing store for a BO > > > > * > > > > - * @bo: Pointer to a struct ttm_buffer_object. the data of > > > > which > > > > - * we want to allocate space for. > > > > - * @placement: Proposed new placement for the buffer object. > > > > - * @mem: A struct ttm_resource. > > > > + * @bo: Pointer to a struct ttm_buffer_object of which we want > > > > a > > > > resource for > > > > + * @placement: Proposed new placement for the buffer object > > > > * @ctx: if and how to sleep, lock buffers and alloc memory > > > > + * @force_space: If we should evict buffers to force space > > > > + * @res: The resulting struct ttm_resource. > > > > * > > > > - * Allocate memory space for the buffer object pointed to by > > > > @bo, > > > > using > > > > - * the placement flags in @placement, potentially evicting > > > > other > > > > idle buffer objects. > > > > - * This function may sleep while waiting for space to become > > > > available. > > > > + * Allocates a resource for the buffer object pointed to by > > > > @bo, > > > > using the > > > > + * placement flags in @placement, potentially evicting other > > > > buffer > > > > objects when > > > > + * @force_space is true. > > > > + * This function may sleep while waiting for resources to > > > > become > > > > available. > > > > * Returns: > > > > - * -EBUSY: No space available (only if no_wait == 1). > > > > + * -EBUSY: No space available (only if no_wait == true). > > > > * -ENOSPC: Could not allocate space for the buffer object, > > > > either > > > > due to > > > > * fragmentation or concurrent allocators. > > > > * -ERESTARTSYS: An interruptible sleep was interrupted by a > > > > signal. > > > > */ > > > > -int ttm_bo_mem_space(struct ttm_buffer_object *bo, > > > > - struct ttm_placement *placement, > > > > - struct ttm_resource **mem, > > > > - struct ttm_operation_ctx *ctx) > > > > +static int ttm_bo_alloc_resource(struct ttm_buffer_object *bo, > > > > + struct ttm_placement *placement, > > > > + struct ttm_operation_ctx *ctx, > > > > + bool force_space, > > > > + struct ttm_resource **res) > > > > { > > > > struct ttm_device *bdev = bo->bdev; > > > > - bool type_found = false; > > > > + struct ww_acquire_ctx *ticket; > > > > int i, ret; > > > > + ticket = dma_resv_locking_ctx(bo->base.resv); > > > > ret = dma_resv_reserve_fences(bo->base.resv, 1); > > > > if (unlikely(ret)) > > > > return ret; > > > > @@ -790,98 +762,73 @@ int ttm_bo_mem_space(struct > > > > ttm_buffer_object > > > > *bo, > > > > const struct ttm_place *place = &placement- > > > > > placement[i]; > > > > struct ttm_resource_manager *man; > > > > - if (place->flags & TTM_PL_FLAG_FALLBACK) > > > > - continue; > > > > - > > > > man = ttm_manager_type(bdev, place->mem_type); > > > > if (!man || !ttm_resource_manager_used(man)) > > > > continue; > > > > - type_found = true; > > > > - ret = ttm_resource_alloc(bo, place, mem); > > > > - if (ret == -ENOSPC) > > > > + if (place->flags & (force_space ? > > > > TTM_PL_FLAG_DESIRED : > > > > + TTM_PL_FLAG_FALLBACK)) > > > > + continue; > > > > + > > > > + do { > > > > + ret = ttm_resource_alloc(bo, place, res); > > > > + if (unlikely(ret != -ENOSPC)) > > > > + return ret; > > > > + if (likely(!ret) || !force_space) > > > > + break; > > > > + > > > > + ret = ttm_mem_evict_first(bdev, man, place, > > > > ctx, > > > > + ticket); > > > > + if (unlikely(ret == -EBUSY)) > > > > + break; > > > > + if (unlikely(ret)) > > > > + return ret; > > > > + } while (1); > > > > + if (ret) > > > > continue; > > > > - if (unlikely(ret)) > > > > - goto error; > > > > - ret = ttm_bo_add_move_fence(bo, man, *mem, ctx- > > > > > no_wait_gpu); > > > > + ret = ttm_bo_add_move_fence(bo, man, *res, ctx- > > > > > no_wait_gpu); > > > > if (unlikely(ret)) { > > > > - ttm_resource_free(bo, mem); > > > > + ttm_resource_free(bo, res); > > > > if (ret == -EBUSY) > > > > continue; > > > > - goto error; > > > > + return ret; > > > > } > > > > return 0; > > > > } > > > > - for (i = 0; i < placement->num_placement; ++i) { > > > > - const struct ttm_place *place = &placement- > > > > > placement[i]; > > > > - struct ttm_resource_manager *man; > > > > - > > > > - if (place->flags & TTM_PL_FLAG_DESIRED) > > > > - continue; > > > > - > > > > - man = ttm_manager_type(bdev, place->mem_type); > > > > - if (!man || !ttm_resource_manager_used(man)) > > > > - continue; > > > > - > > > > - type_found = true; > > > > - ret = ttm_bo_mem_force_space(bo, place, mem, ctx); > > > > - if (likely(!ret)) > > > > - return 0; > > > > - > > > > - if (ret && ret != -EBUSY) > > > > - goto error; > > > > - } > > > > - > > > > - ret = -ENOSPC; > > > > - if (!type_found) { > > > > - pr_err(TTM_PFX "No compatible memory type found\n"); > > > > - ret = -EINVAL; > > > > - } > > > > - > > > > -error: > > > > - return ret; > > > > + return -ENOSPC; > > > > } > > > > -EXPORT_SYMBOL(ttm_bo_mem_space); > > > > -static int ttm_bo_move_buffer(struct ttm_buffer_object *bo, > > > > - struct ttm_placement *placement, > > > > - struct ttm_operation_ctx *ctx) > > > > +/* > > > > + * ttm_bo_mem_space - Wrapper around ttm_bo_alloc_resource > > > > + * > > > > + * @bo: Pointer to a struct ttm_buffer_object of which we want > > > > a > > > > resource for > > > > + * @placement: Proposed new placement for the buffer object > > > > + * @res: The resulting struct ttm_resource. > > > > + * @ctx: if and how to sleep, lock buffers and alloc memory > > > > + * > > > > + * Tries both idle allocation and forcefully eviction of > > > > buffers. > > > > See > > > > + * ttm_bo_alloc_resource for details. > > > > + */ > > > > +int ttm_bo_mem_space(struct ttm_buffer_object *bo, > > > > + struct ttm_placement *placement, > > > > + struct ttm_resource **res, > > > > + struct ttm_operation_ctx *ctx) > > > > { > > > > - struct ttm_resource *mem; > > > > - struct ttm_place hop; > > > > + bool force_space = false; > > > > int ret; > > > > - dma_resv_assert_held(bo->base.resv); > > > > + do { > > > > + ret = ttm_bo_alloc_resource(bo, placement, ctx, > > > > + force_space, res); > > > > + force_space = !force_space; > > > > + } while (ret == -ENOSPC && force_space); > > > > - /* > > > > - * Determine where to move the buffer. > > > > - * > > > > - * If driver determines move is going to need > > > > - * an extra step then it will return -EMULTIHOP > > > > - * and the buffer will be moved to the temporary > > > > - * stop and the driver will be called to make > > > > - * the second hop. > > > > - */ > > > > - ret = ttm_bo_mem_space(bo, placement, &mem, ctx); > > > > - if (ret) > > > > - return ret; > > > > -bounce: > > > > - ret = ttm_bo_handle_move_mem(bo, mem, false, ctx, &hop); > > > > - if (ret == -EMULTIHOP) { > > > > - ret = ttm_bo_bounce_temp_buffer(bo, &mem, ctx, > > > > &hop); > > > > - if (ret) > > > > - goto out; > > > > - /* try and move to final place now. */ > > > > - goto bounce; > > > > - } > > > > -out: > > > > - if (ret) > > > > - ttm_resource_free(bo, &mem); > > > > return ret; > > > > } > > > > +EXPORT_SYMBOL(ttm_bo_mem_space); > > > > /** > > > > * ttm_bo_validate > > > > @@ -902,6 +849,9 @@ int ttm_bo_validate(struct > > > > ttm_buffer_object *bo, > > > > struct ttm_placement *placement, > > > > struct ttm_operation_ctx *ctx) > > > > { > > > > + struct ttm_resource *res; > > > > + struct ttm_place hop; > > > > + bool force_space; > > > > int ret; > > > > dma_resv_assert_held(bo->base.resv); > > > > @@ -912,20 +862,53 @@ int ttm_bo_validate(struct > > > > ttm_buffer_object > > > > *bo, > > > > if (!placement->num_placement) > > > > return ttm_bo_pipeline_gutting(bo); > > > > - /* Check whether we need to move buffer. */ > > > > - if (bo->resource && ttm_resource_compatible(bo->resource, > > > > placement)) > > > > - return 0; > > > > + force_space = false; > > > > + do { > > > > + /* Check whether we need to move buffer. */ > > > > + if (bo->resource && > > > > + ttm_resource_compatible(bo->resource, placement, > > > > + force_space)) > > > > + return 0; > > > > - /* Moving of pinned BOs is forbidden */ > > > > - if (bo->pin_count) > > > > - return -EINVAL; > > > > + /* Moving of pinned BOs is forbidden */ > > > > + if (bo->pin_count) > > > > + return -EINVAL; > > > > + > > > > + /* > > > > + * Determine where to move the buffer. > > > > + * > > > > + * If driver determines move is going to need > > > > + * an extra step then it will return -EMULTIHOP > > > > + * and the buffer will be moved to the temporary > > > > + * stop and the driver will be called to make > > > > + * the second hop. > > > > + */ > > > > + ret = ttm_bo_alloc_resource(bo, placement, ctx, > > > > force_space, > > > > + &res); > > > > + force_space = !force_space; > > > > + if (ret == -ENOSPC) > > > > + continue; > > > > + if (ret) > > > > + return ret; > > > > + > > > > +bounce: > > > > + ret = ttm_bo_handle_move_mem(bo, res, false, ctx, > > > > &hop); > > > > + if (ret == -EMULTIHOP) { > > > > + ret = ttm_bo_bounce_temp_buffer(bo, &res, > > > > ctx, &hop); > > > > + /* try and move to final place now. */ > > > > + if (!ret) > > > > + goto bounce; > > > > + } > > > > + if (ret) { > > > > + ttm_resource_free(bo, &res); > > > > + return ret; > > > > + } > > > > + > > > > + } while (ret && force_space); > > > > - ret = ttm_bo_move_buffer(bo, placement, ctx); > > > > /* For backward compatibility with userspace */ > > > > if (ret == -ENOSPC) > > > > return -ENOMEM; > > > > - if (ret) > > > > - return ret; > > > > /* > > > > * We might need to add a TTM. > > > > diff --git a/drivers/gpu/drm/ttm/ttm_resource.c > > > > b/drivers/gpu/drm/ttm/ttm_resource.c > > > > index fb14f7716cf8..65155f2013ca 100644 > > > > --- a/drivers/gpu/drm/ttm/ttm_resource.c > > > > +++ b/drivers/gpu/drm/ttm/ttm_resource.c > > > > @@ -295,11 +295,13 @@ bool ttm_resource_intersects(struct > > > > ttm_device > > > > *bdev, > > > > * > > > > * @res: the resource to check > > > > * @placement: the placement to check against > > > > + * @evicting: true if the caller is doing evictions > > > > * > > > > * Returns true if the placement is compatible. > > > > */ > > > > bool ttm_resource_compatible(struct ttm_resource *res, > > > > - struct ttm_placement *placement) > > > > + struct ttm_placement *placement, > > > > + bool evicting) > > > > { > > > > struct ttm_buffer_object *bo = res->bo; > > > > struct ttm_device *bdev = bo->bdev; > > > > @@ -315,14 +317,20 @@ bool ttm_resource_compatible(struct > > > > ttm_resource *res, > > > > if (res->mem_type != place->mem_type) > > > > continue; > > > > + if (place->flags & (evicting ? TTM_PL_FLAG_DESIRED : > > > > + TTM_PL_FLAG_FALLBACK)) > > > > + continue; > > > > + > > > > + if (place->flags & TTM_PL_FLAG_CONTIGUOUS && > > > > + !(res->placement & TTM_PL_FLAG_CONTIGUOUS)) > > > > + continue; > > > > + > > > > man = ttm_manager_type(bdev, res->mem_type); > > > > if (man->func->compatible && > > > > !man->func->compatible(man, res, place, bo- > > > > > base.size)) > > > > continue; > > > > - if ((!(place->flags & TTM_PL_FLAG_CONTIGUOUS) || > > > > - (res->placement & TTM_PL_FLAG_CONTIGUOUS))) > > > > - return true; > > > > + return true; > > > > } > > > > return false; > > > > } > > > > diff --git a/include/drm/ttm/ttm_resource.h > > > > b/include/drm/ttm/ttm_resource.h > > > > index 1afa13f0c22b..7561023db43d 100644 > > > > --- a/include/drm/ttm/ttm_resource.h > > > > +++ b/include/drm/ttm/ttm_resource.h > > > > @@ -366,7 +366,8 @@ bool ttm_resource_intersects(struct > > > > ttm_device > > > > *bdev, > > > > const struct ttm_place *place, > > > > size_t size); > > > > bool ttm_resource_compatible(struct ttm_resource *res, > > > > - struct ttm_placement *placement); > > > > + struct ttm_placement *placement, > > > > + bool evicting); > > > > void ttm_resource_set_bo(struct ttm_resource *res, > > > > struct ttm_buffer_object *bo); > > >
