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

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

Hi Matthew,

On 7/26/2022 4:11 PM, Matthew Auld wrote:
> On 25/07/2022 12:42, Arunpravin Paneer Selvam wrote:
>> Implemented a new intersect and compatible callback function
>> fetching start offset from drm buddy allocator.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Arunpravin Paneer Selvam 
>> <Arunpravin.PaneerSelvam@amd.com>
>> ---
>>   drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 43 +++++++++++++++++++
>>   1 file changed, 43 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c 
>> b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
>> index a5109548abc0..b5801c05bd41 100644
>> --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
>> +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
>> @@ -178,6 +178,47 @@ 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,
>> +                     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;
>> +
>> +    /* 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;
>> +}
>
> This looks like a nice idea. We should be able to clean up 
> i915_ttm_eviction_valuable() a fair bit I think, if we now call 
> ttm_bo_eviction_valuable() at the end (like in amdgpu), and move the 
> bits that are specific to buddy_man here?
>
> So something like:
>
> if (!place->fpfn && !place->lpfn)
>     return true;
>
> if (!place->fpfn &&
>     place->lpfn == i915_buddy_man_visible_size(man))
>     return bman_res->used_visible_size > 0;
>
> /* Check each drm buddy block individually */
> ....
> modified in v3
>> +
>> +static bool i915_ttm_buddy_man_compatible(struct 
>> ttm_resource_manager *man,
>> +                      struct ttm_resource *res,
>> +                      const struct ttm_place *place,
>> +                      size_t size)
>
> Is it not possible to derive the size from res->num_pages?
I think it is possible, I will check with Christian for any specific 
case that might
require ttm_buffer_object->base.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;
>> +
>> +    /* 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))
>> +            return false;
>> +    }
>
> if (!place->fpfn && !place->lpfn)
>     return true;
>
> if (!place->fpfn &&
>     place->lpfn == i915_buddy_man_visible_size(man))
>     return bman_res->used_visible_size == res->num_pages;
>
> /* Check each drm buddy block individually */
> ...
> modified in v3
>> +
>> +    return true;
>> +}
>> +
>>   static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man,
>>                        struct drm_printer *printer)
>>   {
>> @@ -205,6 +246,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,
>> +    .intersect = i915_ttm_buddy_man_intersect,
>
> s/intersect/intersects/ ?
> okay.
>> +    .compatible = i915_ttm_buddy_man_compatible,
>>       .debug = i915_ttm_buddy_man_debug,
>>   };