Re: [PATCH 1/3] drm/i915: audit bo->resource usage

From: Matthew Auld <matthew.william.auld@gmail.com>
To: "Christian König" <ckoenig.leichtzumerken@gmail.com>
Cc: "Intel Graphics Development" <intel-gfx@lists.freedesktop.org>,
	"Christian König" <christian.koenig@amd.com>,
	"ML dri-devel" <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 1/3] drm/i915: audit bo->resource usage
Date: Wed, 13 Jul 2022 11:08:29 +0100	[thread overview]
Message-ID: <CAM0jSHOiFRZaU=NC9d0U8wD-1OWx_c+K0xfKz5unAxggU6WFtQ@mail.gmail.com> (raw)
In-Reply-To: <20220712114605.52369-1-christian.koenig@amd.com>

On Tue, 12 Jul 2022 at 12:46, Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Make sure we can at least move and alloc TT objects without backing store.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_ttm.c      | 6 ++----
>  drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 2 +-
>  2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 70e2ed4e99df..5449738c262f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -289,8 +289,6 @@ static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
>  {
>         struct drm_i915_private *i915 = container_of(bo->bdev, typeof(*i915),
>                                                      bdev);
> -       struct ttm_resource_manager *man =
> -               ttm_manager_type(bo->bdev, bo->resource->mem_type);
>         struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
>         unsigned long ccs_pages = 0;
>         enum ttm_caching caching;
> @@ -304,8 +302,8 @@ static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
>         if (!i915_tt)
>                 return NULL;
>
> -       if (obj->flags & I915_BO_ALLOC_CPU_CLEAR &&
> -           man->use_tt)
> +       if (obj->flags & I915_BO_ALLOC_CPU_CLEAR && bo->resource &&
> +           ttm_manager_type(bo->bdev, bo->resource->mem_type)->use_tt)
>                 page_flags |= TTM_TT_FLAG_ZERO_ALLOC;

AFAICT it should be safe to make this:

if (obj->flags & I915_BO_ALLOC_CPU_CLEAR)
        page_flags |= TTM_TT_FLAG_ZERO_ALLOC;

Hopefully that fixes the igt_lmem_create_cleared_cpu subtest?

>
>         caching = i915_ttm_select_tt_caching(obj);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> index a10716f4e717..dcb838dffd7b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> @@ -490,7 +490,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
>         bool clear;
>         int ret;
>
> -       if (GEM_WARN_ON(!obj)) {
> +       if (!bo->resource || GEM_WARN_ON(!obj)) {

IIUC in patch 3 we now nuke the bo->resource when doing the "pipeline
gutting" thing, but I think i915 is (ab)using that when swapping out
shmem objects (see i915_ttm_shrink), so I think here we need to
somehow inspect the tt to see if something needs to be swapped in? We
might also need to move it back to lmem after. Or maybe this is
already handled somehow? CI should hopefully be able to confirm
(gem_lmem_swapping).

>                 ttm_bo_move_null(bo, dst_mem);
>                 return 0;
>         }
> --
> 2.25.1
>