[Intel-gfx] [PATCH 5/5] drm/ttm: replace busy placement with flags v2

[Matthew Auld]

On Tue, 24 Jan 2023 at 12:57, Christian König
<ckoenig.leichtzumerken at gmail.com> 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 at amd.com>
> ---

<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 at intel.com>