Re: [Intel-gfx] [PATCH v3 4/6] drm/i915: Implement intersect/compatible functions

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

On 7/28/2022 8:57 PM, Matthew Auld wrote:
> On 28/07/2022 15:33, Arunpravin Paneer Selvam wrote:
>> Implemented a new intersect and compatible callback function
>> fetching start offset from drm buddy allocator.
>>
>> v2: move the bits that are specific to buddy_man (Matthew)
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Arunpravin Paneer Selvam 
>> <Arunpravin.PaneerSelvam@amd.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c       | 39 +-----------
>>   drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 62 +++++++++++++++++++
>>   2 files changed, 64 insertions(+), 37 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..54eead15d74b 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -396,43 +396,8 @@ static bool i915_ttm_eviction_valuable(struct 
>> ttm_buffer_object *bo,
>>       if (!i915_gem_object_evictable(obj))
>>           return false;
>>   -    switch (res->mem_type) {
>> -    case I915_PL_LMEM0: {
>> -        struct ttm_resource_manager *man =
>> -            ttm_manager_type(bo->bdev, res->mem_type);
>> -        struct i915_ttm_buddy_resource *bman_res =
>> -            to_ttm_buddy_resource(res);
>> -        struct drm_buddy *mm = bman_res->mm;
>> -        struct drm_buddy_block *block;
>> -
>> -        if (!place->fpfn && !place->lpfn)
>> -            return true;
>> -
>> -        GEM_BUG_ON(!place->lpfn);
>> -
>> -        /*
>> -         * If we just want something mappable then we can quickly check
>> -         * if the current victim resource is using any of the CPU
>> -         * visible portion.
>> -         */
>> -        if (!place->fpfn &&
>> -            place->lpfn == i915_ttm_buddy_man_visible_size(man))
>> -            return bman_res->used_visible_size > 0;
>> -
>> -        /* Real range allocation */
>> -        list_for_each_entry(block, &bman_res->blocks, link) {
>> -            unsigned long fpfn =
>> -                drm_buddy_block_offset(block) >> PAGE_SHIFT;
>> -            unsigned long lpfn = fpfn +
>> -                (drm_buddy_block_size(mm, block) >> PAGE_SHIFT);
>> -
>> -            if (place->fpfn < lpfn && place->lpfn > fpfn)
>> -                return true;
>> -        }
>> -        return false;
>> -    } default:
>> -        break;
>> -    }
>> +    if (res->mem_type == I915_PL_LMEM0)
>> +        return ttm_bo_eviction_valuable(bo, place);
>
> We should be able to drop the mem_type == I915_PL_LMEM0 check here I 
> think, and just unconditionally do:
>
> return ttm_bo_eviction_valuable(bo, place);
okay
>
>>         return true;
>>   }
>> diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c 
>> b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
>> index a5109548abc0..9d2a31154d58 100644
>> --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
>> +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
>> @@ -178,6 +178,66 @@ static void i915_ttm_buddy_man_free(struct 
>> ttm_resource_manager *man,
>>       kfree(bman_res);
>>   }
>>   +static bool i915_ttm_buddy_man_intersect(struct 
>> ttm_resource_manager *man,
>
> Nit: intersects
ok
>
>> +                     struct ttm_resource *res,
>> +                     const struct ttm_place *place,
>> +                     size_t size)
>> +{
>> +    struct i915_ttm_buddy_resource *bman_res = 
>> to_ttm_buddy_resource(res);
>> +    u32 start, num_pages = PFN_UP(size);
>> +    struct drm_buddy_block *block;
>> +
>> +    if (!place->fpfn && !place->lpfn)
>> +        return true;
I place bug_on check here
GEM_BUG_ON(!place->lpfn);
>> +
>> +    /*
>> +     * If we just want something mappable then we can quickly check
>> +     * if the current victim resource is using any of the CP
>> +     * visible portion.
>> +     */
>> +    if (!place->fpfn &&
>> +        place->lpfn == i915_ttm_buddy_man_visible_size(man))
>> +        return bman_res->used_visible_size > 0;
>> +
>> +    /* Check each drm buddy block individually */
>> +    list_for_each_entry(block, &bman_res->blocks, link) {
>> +        start = drm_buddy_block_offset(block) >> PAGE_SHIFT;
>> +        /* Don't evict BOs outside of the requested placement range */
>> +        if (place->fpfn >= (start + num_pages) ||
>> +            (place->lpfn && place->lpfn <= start))
>> +            return false;
>> +    }
>> +
>> +    return true;
>
> We need to account for the block size somewhere. Also same bug in the 
> amdgpu patch it seems. We also need to do this the other way around 
> and keep checking until we find something that overlaps, for example 
> if the first block doesn't intersect/overlap we will incorrectly 
> return false here, even if one of the other blocks does intersect.
>
> list_for_each_entry() {
>     fpfn = drm_buddy_block_size(mm, block) >> PAGE_SHIFT;
>     lpfn = fpfn + drm_buddy_block_size(mm, block) >> PAGE_SHIFT);
>
>     if (place->fpfn < lpfn && place->lpfn > fpfn)
>         return true;
> }
>
> return false;
yes, here the final code looks like,
list_for_each_entry(block, &bman_res->blocks, link) {
                 unsigned long fpfn =
                         drm_buddy_block_offset(block) >> PAGE_SHIFT;
                 unsigned long lpfn = fpfn +
                         (drm_buddy_block_size(mm, block) >> PAGE_SHIFT);
                 /* Don't evict BOs outside of the requested placement 
range */
                 if (place->fpfn < lpfn && place->lpfn > fpfn)
                         return true;
         }

return false;

>
>> +}
>> +
>> +static bool i915_ttm_buddy_man_compatible(struct 
>> ttm_resource_manager *man,
>> +                      struct ttm_resource *res,
>> +                      const struct ttm_place *place,
>> +                      size_t size)
>> +{
>> +    struct i915_ttm_buddy_resource *bman_res = 
>> to_ttm_buddy_resource(res);
>> +    u32 start, num_pages = PFN_UP(size);
>> +    struct drm_buddy_block *block;
>> +
>> +    if (!place->fpfn && !place->lpfn)
>> +        return true;
I place bug_on check here
GEM_BUG_ON(!place->lpfn);
>> +
>> +    if (!place->fpfn &&
>> +        place->lpfn == i915_ttm_buddy_man_visible_size(man))
>> +        return bman_res->used_visible_size == res->num_pages;
>> +
>> +    /* Check each drm buddy block individually */
>> +    list_for_each_entry(block, &bman_res->blocks, link) {
>> +        start = drm_buddy_block_offset(block) >> PAGE_SHIFT;
>> +        if (start < place->fpfn ||
>> +            (place->lpfn && (start + num_pages) > place->lpfn))
>
> Same here. We need to consider the block size/range.
ahh somehow missed the block size, here the final code looks like,

/* Check each drm buddy block individually */
     list_for_each_entry(block, &bman_res->blocks, link) {
                 unsigned long fpfn =
                         drm_buddy_block_offset(block) >> PAGE_SHIFT;
                 unsigned long lpfn = fpfn +
                         (drm_buddy_block_size(mm, block) >> PAGE_SHIFT);
                 if (fpfn < place->fpfn || lpfn > place->lpfn)
                         return false;
         }
return true;
>
> Otherwise I think looks good.
>
>> +            return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>   static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man,
>>                        struct drm_printer *printer)
>>   {
>> @@ -205,6 +265,8 @@ static void i915_ttm_buddy_man_debug(struct 
>> ttm_resource_manager *man,
>>   static const struct ttm_resource_manager_func 
>> i915_ttm_buddy_manager_func = {
>>       .alloc = i915_ttm_buddy_man_alloc,
>>       .free = i915_ttm_buddy_man_free,
>> +    .intersects = i915_ttm_buddy_man_intersect,
>> +    .compatible = i915_ttm_buddy_man_compatible,
>>       .debug = i915_ttm_buddy_man_debug,
>>   };