Re: [PATCH v5 17/20] drm/gpusvm: Introduce drm_gpusvm_range_find_or_insert_start() function

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 30-04-2025 00:05, Matthew Brost wrote:
> On Tue, Apr 29, 2025 at 04:12:30PM +0530, Himal Prasad Ghimiray wrote:
>> The drm_gpusvm_range_find_or_insert_start() function is used to
>> determine the starting address of a CPU VMA within a specified user
>> range. If the range does not contain any VMA, the function returns
>> ULONG_MAX.
>>
>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> ---
>>   drivers/gpu/drm/drm_gpusvm.c | 29 +++++++++++++++++++++++++++++
>>   include/drm/drm_gpusvm.h     |  5 +++++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
>> index 40a56f38ff8e..bb7c589b8d15 100644
>> --- a/drivers/gpu/drm/drm_gpusvm.c
>> +++ b/drivers/gpu/drm/drm_gpusvm.c
>> @@ -980,6 +980,35 @@ static void drm_gpusvm_driver_lock_held(struct drm_gpusvm *gpusvm)
>>   }
>>   #endif
>>   
>> +/**
>> + * drm_gpusvm_range_find_or_insert_start() - Find or insert start address for range
>> + * @gpusvm: Pointer to the GPU SVM structure
>> + * @start: The inclusive start user address.
>> + * @end: The exclusive end user address.
>> + *
>> + * Returns: The start address of first VMA within the provided range,
>> + * ULONG_MAX otherwise. Assumes start_addr < end_addr.
>> + */
>> +unsigned long
>> +drm_gpusvm_range_find_or_insert_start(struct drm_gpusvm *gpusvm,
>> +				      unsigned long start,
>> +				      unsigned long end)
> 
> We don't really do an insertion here, so maybe:
> 
> s/drm_gpusvm_range_find_or_insert_start/drm_gpusvm_find_vma_start

Sure.

> 
>> +{
>> +	struct mm_struct *mm = gpusvm->mm;
>> +	struct vm_area_struct *vma;
>> +	unsigned long addr = ULONG_MAX;

Does this return in case of no vma found makes sense to you ?
Initially I thought of returning 0, but wasn't sure whether under any 
scenario cpu vma can start at 0 or not.

>> +
> 
> Hmm, do you think we need a mmget here? I'd expect this be called from a
> user context where we'd have a MM ref but maybe to be safe add a
> mmget/put?
Yup, mmget/mmput makes it more reliable. Will add it

> 
> Matt
> 
>> +	mmap_read_lock(mm);
>> +
>> +	vma = find_vma_intersection(mm, start, end);
>> +	if (vma)
>> +		addr =  vma->vm_start;
>> +
>> +	mmap_read_unlock(mm);
>> +	return addr;
>> +}
>> +EXPORT_SYMBOL_GPL(drm_gpusvm_range_find_or_insert_start);
>> +
>>   /**
>>    * drm_gpusvm_range_find_or_insert() - Find or insert GPU SVM range
>>    * @gpusvm: Pointer to the GPU SVM structure
>> diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
>> index cce217bc136f..b0e9ee5c6226 100644
>> --- a/include/drm/drm_gpusvm.h
>> +++ b/include/drm/drm_gpusvm.h
>> @@ -315,6 +315,11 @@ void drm_gpusvm_fini(struct drm_gpusvm *gpusvm);
>>   
>>   void drm_gpusvm_free(struct drm_gpusvm *gpusvm);
>>   
>> +unsigned long
>> +drm_gpusvm_range_find_or_insert_start(struct drm_gpusvm *gpusvm,
>> +				      unsigned long start,
>> +				      unsigned long end);
>> +
>>   struct drm_gpusvm_range *
>>   drm_gpusvm_range_find_or_insert(struct drm_gpusvm *gpusvm,
>>   				unsigned long fault_addr,
>> -- 
>> 2.34.1
>>