Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <1eef3b71-ef7c-24d1-b0d7-695fc1d2d353@collabora.com>
Date:   Wed, 16 Mar 2022 13:19:32 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [RFC PATCH 5/7] drm/ttm: add range busy check for range manager
Content-Language: en-US
To:     =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        intel-gfx@lists.freedesktop.org, Huang Rui <ray.huang@amd.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>
Cc:     dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
References: <20220315180444.3327283-1-bob.beckett@collabora.com>
 <20220315180444.3327283-6-bob.beckett@collabora.com>
 <2918e4a2-3bb8-23e0-3b8c-90c620b82328@amd.com>
From:   Robert Beckett <bob.beckett@collabora.com>
In-Reply-To: <2918e4a2-3bb8-23e0-3b8c-90c620b82328@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 16/03/2022 09:54, Christian König wrote:
> Am 15.03.22 um 19:04 schrieb Robert Beckett:
>> RFC: do we want this to become a generic interface in
>> ttm_resource_manager_func?
>>
>> RFC: would we prefer a different interface? e.g.
>> for_each_resource_in_range or for_each_bo_in_range
> 
> Well completely NAK to that. Why do you need that?
> 
> The long term goal is to completely remove the range checks from TTM 
> instead.

ah, I did not know that.
I wanted it just to enable parity with a selftest that checks whether a 
range is allocated before initializing a given range with test data 
behind the allocator's back. It needs to check the range so that it 
doesn't destroy in use data.

I suppose we could add another drm_mm range tracker just for testing and 
shadow track each allocation in the range, but that seemed like a lot of 
extra infrastructure for no general runtime use.

would you mind explaining the rationale for removing range checks? It 
seems to me like a natural fit for a memory manager

> 
> Regards,
> Christian.
> 
>>
>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>> ---
>>   drivers/gpu/drm/ttm/ttm_range_manager.c | 21 +++++++++++++++++++++
>>   include/drm/ttm/ttm_range_manager.h     |  3 +++
>>   2 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c 
>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>> index 8cd4f3fb9f79..5662627bb933 100644
>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>> @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct ttm_device 
>> *bdev,
>>       return 0;
>>   }
>>   EXPORT_SYMBOL(ttm_range_man_fini_nocheck);
>> +
>> +/**
>> + * ttm_range_man_range_busy - Check whether anything is allocated 
>> with a range
>> + *
>> + * @man: memory manager to check
>> + * @fpfn: first page number to check
>> + * @lpfn: last page number to check
>> + *
>> + * Return: true if anything allocated within the range, false otherwise.
>> + */
>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
>> +                  unsigned fpfn, unsigned lpfn)
>> +{
>> +    struct ttm_range_manager *rman = to_range_manager(man);
>> +    struct drm_mm *mm = &rman->mm;
>> +
>> +    if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), PFN_PHYS(lpfn + 
>> 1) - 1))
>> +        return true;
>> +    return false;
>> +}
>> +EXPORT_SYMBOL(ttm_range_man_range_busy);
>> diff --git a/include/drm/ttm/ttm_range_manager.h 
>> b/include/drm/ttm/ttm_range_manager.h
>> index 7963b957e9ef..86794a3f9101 100644
>> --- a/include/drm/ttm/ttm_range_manager.h
>> +++ b/include/drm/ttm/ttm_range_manager.h
>> @@ -53,4 +53,7 @@ static __always_inline int ttm_range_man_fini(struct 
>> ttm_device *bdev,
>>       BUILD_BUG_ON(__builtin_constant_p(type) && type >= 
>> TTM_NUM_MEM_TYPES);
>>       return ttm_range_man_fini_nocheck(bdev, type);
>>   }
>> +
>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
>> +                  unsigned fpfn, unsigned lpfn);
>>   #endif
>