On Tue, 24 Jan 2023 at 12:57, Christian König
<ckoenig.leichtzumerken@xxxxxxxxx> wrote:
>
> Instead of a list of separate busy placement add flags which indicate
> that a placement should only be used when there is room or if we need to
> evict.
>
> v2: add missing TTM_PL_FLAG_IDLE for i915
>
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> ---
<snip>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index c2ec91cc845d..0ab24ca5f419 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -347,27 +347,6 @@ nouveau_bo_new(struct nouveau_cli *cli, u64 size, int align,
> return 0;
> }
>
> -static void
> -set_placement_list(struct ttm_place *pl, unsigned *n, uint32_t domain)
> -{
> - *n = 0;
> -
> - if (domain & NOUVEAU_GEM_DOMAIN_VRAM) {
> - pl[*n].mem_type = TTM_PL_VRAM;
> - pl[*n].flags = 0;
> - (*n)++;
> - }
> - if (domain & NOUVEAU_GEM_DOMAIN_GART) {
> - pl[*n].mem_type = TTM_PL_TT;
> - pl[*n].flags = 0;
> - (*n)++;
> - }
> - if (domain & NOUVEAU_GEM_DOMAIN_CPU) {
> - pl[*n].mem_type = TTM_PL_SYSTEM;
> - pl[(*n)++].flags = 0;
> - }
> -}
> -
> static void
> set_placement_range(struct nouveau_bo *nvbo, uint32_t domain)
> {
> @@ -395,10 +374,6 @@ set_placement_range(struct nouveau_bo *nvbo, uint32_t domain)
> nvbo->placements[i].fpfn = fpfn;
> nvbo->placements[i].lpfn = lpfn;
> }
> - for (i = 0; i < nvbo->placement.num_busy_placement; ++i) {
> - nvbo->busy_placements[i].fpfn = fpfn;
> - nvbo->busy_placements[i].lpfn = lpfn;
> - }
> }
> }
>
> @@ -406,15 +381,32 @@ void
> nouveau_bo_placement_set(struct nouveau_bo *nvbo, uint32_t domain,
> uint32_t busy)
> {
> - struct ttm_placement *pl = &nvbo->placement;
> + struct ttm_place *pl = nvbo->placements;
> + unsigned *n = &nvbo->placement.num_placement;
>
> - pl->placement = nvbo->placements;
> - set_placement_list(nvbo->placements, &pl->num_placement, domain);
> + domain |= busy;
>
> - pl->busy_placement = nvbo->busy_placements;
> - set_placement_list(nvbo->busy_placements, &pl->num_busy_placement,
> - domain | busy);
> + *n = 0;
> + if (domain & NOUVEAU_GEM_DOMAIN_VRAM) {
> + pl[*n].mem_type = TTM_PL_VRAM;
> + pl[*n].flags = busy & NOUVEAU_GEM_DOMAIN_VRAM ?
> + TTM_PL_FLAG_BUSY : 0;
> + (*n)++;
> + }
> + if (domain & NOUVEAU_GEM_DOMAIN_GART) {
> + pl[*n].mem_type = TTM_PL_TT;
> + pl[*n].flags = busy & NOUVEAU_GEM_DOMAIN_GART ?
> + TTM_PL_FLAG_BUSY : 0;
> + (*n)++;
> + }
> + if (domain & NOUVEAU_GEM_DOMAIN_CPU) {
> + pl[*n].mem_type = TTM_PL_SYSTEM;
> + pl[*n].flags = busy & NOUVEAU_GEM_DOMAIN_CPU ?
> + TTM_PL_FLAG_BUSY : 0;
> + (*n)++;
> + }
Should this not be something like:
non_busy = domain;
domain |= busy;
....
if (domain & VRAM) {
if (non_busy & VRAM)
flags = 0
else
flags = FLAG_BUSY
}
Otherwise if VRAM is set in both "busy" and "domain", it will only try
VRAM when all non-busy first fails, which looks like a change in
behaviour?
The rest of the patch looks good to me, so with the above fixed or explained,
Reviewed-by: Matthew Auld <matthew.auld@xxxxxxxxx>
