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

["Danilo Krummrich" <dakr@kernel.org>]

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'.

> +	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;

or

	if (madvise && va->gem.obj) {
		if (end > req_end)
			break;
		else
			continue;
	}

instead of sprinkling the skip_madvise_ops checks everywhere.

>  
> +		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.

> +					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.