Re: [PATCH v6 1/6] drm/ttm: Add new callbacks to ttm res mgr

["Christian König" <christian.koenig@amd.com>]

Am 06.09.22 um 21:58 schrieb Daniel Vetter:
> On Tue, Aug 16, 2022 at 10:33:16AM +0530, Arunpravin Paneer Selvam wrote:
>>
>> On 8/15/2022 4:35 PM, Christian König wrote:
>>> Am 12.08.22 um 15:30 schrieb Arunpravin Paneer Selvam:
>>>> We are adding two new callbacks to ttm resource manager
>>>> function to handle intersection and compatibility of
>>>> placement and resources.
>>>>
>>>> v2: move the amdgpu and ttm_range_manager changes to
>>>>       separate patches (Christian)
>>>> v3: rename "intersect" to "intersects" (Matthew)
>>>> v4: move !place check to the !res if and return false
>>>>       in ttm_resource_compatible() function (Christian)
>>>> v5: move bits of code from patch number 6 to avoid
>>>>       temporary driver breakup (Christian)
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> Signed-off-by: Arunpravin Paneer Selvam
>>>> <Arunpravin.PaneerSelvam@amd.com>
>>> Patch #6 could still be cleaned up more now that we have the workaround
>>> code in patch #1, but that not really a must have.
>>>
>>> Reviewed-by: Christian König <christian.koenig@amd.com> for the entire
>>> series.
>>>
>>> Do you already have commit rights?
>> Hi Christian,
>> I applied for drm-misc commit rights, waiting for the project maintainers to
>> approve my request.
> Why do all drivers have to implement the current behaviour? Can we have a
> default implementation, which gets called if nothing is set instead?

We do have a default implementation in the range manager which is used 
by radeon, GEM VRAM helpers, VMWGFX and amdgpu (but there only for some 
domains).

> I'm a bit confused why the bloat here ...

Drivers do have specialized implementations of the backend, e.g. VMWGFX 
have his handle backend, amdgpu the VRAM backend with special 
placements, i915 is completely special as well.

Here we only move the decision if resources intersect or are compatible 
into those specialized backends. Previously we had all this in a 
centralized callback for all backends of a driver.

See the switch in amdgpu_ttm_bo_eviction_valuable() for an example. 
Final goal is to move all this stuff into the specialized backends and 
remove this callback.

The only driver where I couldn't figure out why we have duplicated all 
this from the standard implementation is Nouveau.

> Also please document new callbacks precisely with inline kerneldoc. I know
> ttm docs aren't great yet, but they don't get better if we don't start
> somewhere. I think the in-depth comments for modeset vfuncs (e.g. in
> drm_modeset_helper_vtables.h) are a good standard here.

I thought we already did that. Please be a bit more specific.

Thanks,
Christian.

> -Daniel
>
>> Thanks,
>> Arun
>>> Regards,
>>> Christian.
>>>
>>>> ---
>>>>    drivers/gpu/drm/ttm/ttm_bo.c       |  9 ++--
>>>>    drivers/gpu/drm/ttm/ttm_resource.c | 77 +++++++++++++++++++++++++++++-
>>>>    include/drm/ttm/ttm_resource.h     | 40 ++++++++++++++++
>>>>    3 files changed, 119 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>>>> index c1bd006a5525..f066e8124c50 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_intersects(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 20f9adcc3235..0d1f862a582b 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_resource.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
>>>> @@ -253,10 +253,84 @@ void ttm_resource_free(struct
>>>> ttm_buffer_object *bo, struct ttm_resource **res)
>>>>    }
>>>>    EXPORT_SYMBOL(ttm_resource_free);
>>>>    +/**
>>>> + * ttm_resource_intersects - test for intersection
>>>> + *
>>>> + * @bdev: TTM device structure
>>>> + * @res: The resource to test
>>>> + * @place: The placement to test
>>>> + * @size: How many bytes the new allocation needs.
>>>> + *
>>>> + * Test if @res intersects with @place and @size. Used for testing
>>>> if evictions
>>>> + * are valueable or not.
>>>> + *
>>>> + * Returns true if the res placement intersects with @place and @size.
>>>> + */
>>>> +bool ttm_resource_intersects(struct ttm_device *bdev,
>>>> +                 struct ttm_resource *res,
>>>> +                 const struct ttm_place *place,
>>>> +                 size_t size)
>>>> +{
>>>> +    struct ttm_resource_manager *man;
>>>> +
>>>> +    if (!res)
>>>> +        return false;
>>>> +
>>>> +    if (!place)
>>>> +        return true;
>>>> +
>>>> +    man = ttm_manager_type(bdev, res->mem_type);
>>>> +    if (!man->func->intersects) {
>>>> +        if (place->fpfn >= (res->start + res->num_pages) ||
>>>> +            (place->lpfn && place->lpfn <= res->start))
>>>> +            return false;
>>>> +
>>>> +        return true;
>>>> +    }
>>>> +
>>>> +    return man->func->intersects(man, res, place, size);
>>>> +}
>>>> +
>>>> +/**
>>>> + * ttm_resource_compatible - test for compatibility
>>>> + *
>>>> + * @bdev: TTM device structure
>>>> + * @res: The resource to test
>>>> + * @place: The placement to test
>>>> + * @size: How many bytes the new allocation needs.
>>>> + *
>>>> + * Test if @res compatible with @place and @size.
>>>> + *
>>>> + * Returns true if the res placement compatible with @place and @size.
>>>> + */
>>>> +bool ttm_resource_compatible(struct ttm_device *bdev,
>>>> +                 struct ttm_resource *res,
>>>> +                 const struct ttm_place *place,
>>>> +                 size_t size)
>>>> +{
>>>> +    struct ttm_resource_manager *man;
>>>> +
>>>> +    if (!res || !place)
>>>> +        return false;
>>>> +
>>>> +    man = ttm_manager_type(bdev, res->mem_type);
>>>> +    if (!man->func->compatible) {
>>>> +        if (res->start < place->fpfn ||
>>>> +            (place->lpfn && (res->start + res->num_pages) >
>>>> place->lpfn))
>>>> +            return false;
>>>> +
>>>> +        return true;
>>>> +    }
>>>> +
>>>> +    return man->func->compatible(man, res, place, size);
>>>> +}
>>>> +
>>>>    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)
>>>> @@ -265,8 +339,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) &&
>>>> diff --git a/include/drm/ttm/ttm_resource.h
>>>> b/include/drm/ttm/ttm_resource.h
>>>> index ca89a48c2460..5afc6d664fde 100644
>>>> --- a/include/drm/ttm/ttm_resource.h
>>>> +++ b/include/drm/ttm/ttm_resource.h
>>>> @@ -88,6 +88,38 @@ struct ttm_resource_manager_func {
>>>>        void (*free)(struct ttm_resource_manager *man,
>>>>                 struct ttm_resource *res);
>>>>    +    /**
>>>> +     * struct ttm_resource_manager_func member intersects
>>>> +     *
>>>> +     * @man: Pointer to a memory type manager.
>>>> +     * @res: Pointer to a struct ttm_resource to be checked.
>>>> +     * @place: Placement to check against.
>>>> +     * @size: Size of the check.
>>>> +     *
>>>> +     * Test if @res intersects with @place + @size. Used to judge if
>>>> +     * evictions are valueable or not.
>>>> +     */
>>>> +    bool (*intersects)(struct ttm_resource_manager *man,
>>>> +               struct ttm_resource *res,
>>>> +               const struct ttm_place *place,
>>>> +               size_t size);
>>>> +
>>>> +    /**
>>>> +     * struct ttm_resource_manager_func member compatible
>>>> +     *
>>>> +     * @man: Pointer to a memory type manager.
>>>> +     * @res: Pointer to a struct ttm_resource to be checked.
>>>> +     * @place: Placement to check against.
>>>> +     * @size: Size of the check.
>>>> +     *
>>>> +     * Test if @res compatible with @place + @size. Used to check of
>>>> +     * the need to move the backing store or not.
>>>> +     */
>>>> +    bool (*compatible)(struct ttm_resource_manager *man,
>>>> +               struct ttm_resource *res,
>>>> +               const struct ttm_place *place,
>>>> +               size_t size);
>>>> +
>>>>        /**
>>>>         * struct ttm_resource_manager_func member debug
>>>>         *
>>>> @@ -329,6 +361,14 @@ int ttm_resource_alloc(struct ttm_buffer_object
>>>> *bo,
>>>>                   const struct ttm_place *place,
>>>>                   struct ttm_resource **res);
>>>>    void ttm_resource_free(struct ttm_buffer_object *bo, struct
>>>> ttm_resource **res);
>>>> +bool ttm_resource_intersects(struct ttm_device *bdev,
>>>> +                 struct ttm_resource *res,
>>>> +                 const struct ttm_place *place,
>>>> +                 size_t size);
>>>> +bool ttm_resource_compatible(struct ttm_device *bdev,
>>>> +                 struct ttm_resource *res,
>>>> +                 const struct ttm_place *place,
>>>> +                 size_t size);
>>>>    bool ttm_resource_compat(struct ttm_resource *res,
>>>>                 struct ttm_placement *placement);
>>>>    void ttm_resource_set_bo(struct ttm_resource *res,
>>>>
>>>> base-commit: 730c2bf4ad395acf0aa0820535fdb8ea6abe5df1