Re: [PATCH v3 6/6] drm/ttm: Switch to using the new res callback

[Arunpravin Paneer Selvam <arunpravin.paneerselvam@xxxxxxx>]

On 7/28/2022 9:07 PM, Matthew Auld wrote:
> On 28/07/2022 15:33, Arunpravin Paneer Selvam wrote:
> > Apply new intersect and compatible callback instead
> > of having a generic placement range verfications.
> >
> > v2: Added a separate callback for compatiblilty
> >      checks (Christian)
> >
> > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> > Signed-off-by: Arunpravin Paneer Selvam 
> > <Arunpravin.PaneerSelvam@xxxxxxx>
>
> There is also some code at the bottom of i915_ttm_buddy_man_alloc() 
> playing games with res->start, which I think can be safely deleted 
> with this series (now that we have proper ->compatible() hook).
>
> Also, is the plan to remove res->start completely, or does that still 
> have a use?
yes we should remove res->start completely, I am working on it, planning 
to send in a separate series as amdgpu uses it in many places, and in 
some places we set res->start to AMDGPU_BO_INVALID_OFFSET,
I should find an alternative to indicate the invalid offset BO. Also, 
res->start used in drm/drm_gem_vram_helper.c at drm_gem_vram_pg_offset() 
function. I am removing all the dependencies, I will send the
patches in a separate series. I think i915 doesn't use res->start in its 
own driver code.
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 45 +++++++------------------
> >   drivers/gpu/drm/ttm/ttm_bo.c            |  9 +++--
> >   drivers/gpu/drm/ttm/ttm_resource.c      |  5 +--
> >   3 files changed, 20 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > index 170935c294f5..7d25a10395c0 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > @@ -1328,11 +1328,12 @@ uint64_t amdgpu_ttm_tt_pte_flags(struct 
> > amdgpu_device *adev, struct ttm_tt *ttm,
> >   static bool amdgpu_ttm_bo_eviction_valuable(struct 
> > ttm_buffer_object *bo,
> >                           const struct ttm_place *place)
> >   {
> > -    unsigned long num_pages = bo->resource->num_pages;
> >       struct dma_resv_iter resv_cursor;
> > -    struct amdgpu_res_cursor cursor;
> >       struct dma_fence *f;
> >   +    if (!amdgpu_bo_is_amdgpu_bo(bo))
> > +        return ttm_bo_eviction_valuable(bo, place);
> > +
> >       /* Swapout? */
> >       if (bo->resource->mem_type == TTM_PL_SYSTEM)
> >           return true;
> > @@ -1351,40 +1352,20 @@ static bool 
> > amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
> >               return false;
> >       }
> >   -    switch (bo->resource->mem_type) {
> > -    case AMDGPU_PL_PREEMPT:
> > -        /* Preemptible BOs don't own system resources managed by the
> > -         * driver (pages, VRAM, GART space). They point to resources
> > -         * owned by someone else (e.g. pageable memory in user mode
> > -         * or a DMABuf). They are used in a preemptible context so we
> > -         * can guarantee no deadlocks and good QoS in case of MMU
> > -         * notifiers or DMABuf move notifiers from the resource owner.
> > -         */
> > +    /* Preemptible BOs don't own system resources managed by the
> > +     * driver (pages, VRAM, GART space). They point to resources
> > +     * owned by someone else (e.g. pageable memory in user mode
> > +     * or a DMABuf). They are used in a preemptible context so we
> > +     * can guarantee no deadlocks and good QoS in case of MMU
> > +     * notifiers or DMABuf move notifiers from the resource owner.
> > +     */
> > +    if (bo->resource->mem_type == AMDGPU_PL_PREEMPT)
> >           return false;
> > -    case TTM_PL_TT:
> > -        if (amdgpu_bo_is_amdgpu_bo(bo) &&
> > -            amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
> > -            return false;
> > -        return true;
> >   -    case TTM_PL_VRAM:
> > -        /* Check each drm MM node individually */
> > -        amdgpu_res_first(bo->resource, 0, (u64)num_pages << PAGE_SHIFT,
> > -                 &cursor);
> > -        while (cursor.remaining) {
> > -            if (place->fpfn < PFN_DOWN(cursor.start + cursor.size)
> > -                && !(place->lpfn &&
> > -                 place->lpfn <= PFN_DOWN(cursor.start)))
> > -                return true;
> > -
> > -            amdgpu_res_next(&cursor, cursor.size);
> > -        }
> > +    if (bo->resource->mem_type == TTM_PL_TT &&
> > +        amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
> >           return false;
> >   -    default:
> > -        break;
> > -    }
> > -
> >       return ttm_bo_eviction_valuable(bo, place);
> >   }
> >   diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
> > b/drivers/gpu/drm/ttm/ttm_bo.c
> > index c1bd006a5525..03409409e43e 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> > @@ -518,6 +518,9 @@ static int ttm_bo_evict(struct ttm_buffer_object 
> > *bo,
> >   bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
> >                     const struct ttm_place *place)
> >   {
> > +    struct ttm_resource *res = bo->resource;
> > +    struct ttm_device *bdev = bo->bdev;
> > +
> >       dma_resv_assert_held(bo->base.resv);
> >       if (bo->resource->mem_type == TTM_PL_SYSTEM)
> >           return true;
> > @@ -525,11 +528,7 @@ bool ttm_bo_eviction_valuable(struct 
> > ttm_buffer_object *bo,
> >       /* Don't evict this BO if it's outside of the
> >        * requested placement range
> >        */
> > -    if (place->fpfn >= (bo->resource->start + 
> > bo->resource->num_pages) ||
> > -        (place->lpfn && place->lpfn <= bo->resource->start))
> > -        return false;
> > -
> > -    return true;
> > +    return ttm_resource_intersect(bdev, res, place, bo->base.size);
> >   }
> >   EXPORT_SYMBOL(ttm_bo_eviction_valuable);
> >   diff --git a/drivers/gpu/drm/ttm/ttm_resource.c 
> > b/drivers/gpu/drm/ttm/ttm_resource.c
> > index 84a6fe9e976e..c745faf72b1a 100644
> > --- a/drivers/gpu/drm/ttm/ttm_resource.c
> > +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> > @@ -316,6 +316,8 @@ static bool ttm_resource_places_compat(struct 
> > ttm_resource *res,
> >                          const struct ttm_place *places,
> >                          unsigned num_placement)
> >   {
> > +    struct ttm_buffer_object *bo = res->bo;
> > +    struct ttm_device *bdev = bo->bdev;
> >       unsigned i;
> >         if (res->placement & TTM_PL_FLAG_TEMPORARY)
> > @@ -324,8 +326,7 @@ static bool ttm_resource_places_compat(struct 
> > ttm_resource *res,
> >       for (i = 0; i < num_placement; i++) {
> >           const struct ttm_place *heap = &places[i];
> >   -        if (res->start < heap->fpfn || (heap->lpfn &&
> > -            (res->start + res->num_pages) > heap->lpfn))
> > +        if (!ttm_resource_compatible(bdev, res, heap, bo->base.size))
> >               continue;
> >             if ((res->mem_type == heap->mem_type) &&