[PATCH v6 04/26] drm/gpuvm: Introduce DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE flag

[Ghimiray, Himal Prasad]

On 12-08-2025 22:28, Danilo Krummrich wrote:
> On Thu Aug 7, 2025 at 6:43 PM CEST, Himal Prasad Ghimiray wrote:
>> @@ -2110,6 +2110,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>>   {
>>   	struct drm_gpuva *va, *next;
>>   	u64 req_end = req->op_map.va.addr + req->op_map.va.range;
>> +	bool is_madvise_ops = (req->flags & DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE);
> 
> Let's just call this 'madvise'.

Sure.

> 
>> +	bool needs_map = !is_madvise_ops;
>>   	int ret;
>>   
>>   	if (unlikely(!drm_gpuvm_range_valid(gpuvm, req->op_map.va.addr, req->op_map.va.range)))
>> @@ -2122,26 +2124,35 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>>   		u64 range = va->va.range;
>>   		u64 end = addr + range;
>>   		bool merge = !!va->gem.obj;
>> +		bool skip_madvise_ops = is_madvise_ops && merge;
> 
> IIUC, you're either going for continue or break in this case. I think continue
> would always be correct and break is an optimization if end <= req_end?
> 
> If that's correct, please just do either
> 
> 	if (madvise && va->gem.obj)
> 		continue;

Will use this.>
> or
> 
> 	if (madvise && va->gem.obj) {
> 		if (end > req_end)
> 			break;
> 		else
> 			continue;
> 	}
> 
> instead of sprinkling the skip_madvise_ops checks everywhere.

True, recommended checks make it cleaner.

> 
>>   
>> +		needs_map = !is_madvise_ops;
>>   		if (addr == req->op_map.va.addr) {
>>   			merge &= obj == req->op_map.gem.obj &&
>>   				 offset == req->op_map.gem.offset;
>>   
>>   			if (end == req_end) {
>> -				ret = op_unmap_cb(ops, priv, va, merge);
>> -				if (ret)
>> -					return ret;
>> +				if (!is_madvise_ops) {
>> +					ret = op_unmap_cb(ops, priv, va, merge);
>> +					if (ret)
>> +						return ret;
>> +				}
>>   				break;
>>   			}
>>   
>>   			if (end < req_end) {
>> -				ret = op_unmap_cb(ops, priv, va, merge);
>> -				if (ret)
>> -					return ret;
>> +				if (!is_madvise_ops) {
>> +					ret = op_unmap_cb(ops, priv, va, merge);
> 
> I think we should pass madvise as argument to op_unmap_cb() and make it a noop
> internally rather than having all the conditionals.

Makes sense. Will modify in next version.

> 
>> +					if (ret)
>> +						return ret;
>> +				}
>>   				continue;
>>   			}
>>   
>>   			if (end > req_end) {
>> +				if (skip_madvise_ops)
>> +					break;
>> +
>>   				struct drm_gpuva_op_map n = {
>>   					.va.addr = req_end,
>>   					.va.range = range - req->op_map.va.range,
>> @@ -2156,6 +2167,9 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>>   				ret = op_remap_cb(ops, priv, NULL, &n, &u);
>>   				if (ret)
>>   					return ret;
>> +
>> +				if (is_madvise_ops)
>> +					needs_map = true;
> 
> I don't like this needs_map state...
> 
> Maybe we could have
> 
> 	struct drm_gpuvm_map_req *op_map = madvise ? NULL : req;
> 
> at the beginning of the function and then change this line to
> 
> 	if (madvise)
> 		op_map = req;
> 
> and op_map_cb() can just handle a NULL pointer.
> 
> Yeah, I feel like that's better.

Agreed.

Thanks for the review.