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

[Arunpravin Paneer Selvam <arunpravin.paneerselvam@amd.com>]

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.

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
>